Maybe we should instead modify processSignatures to deal with this (it's signature related, after all): this code was hard to follow, and this makes it harder.

we could clean this up too:

1. I'd expect normalizeSigners to be called from removeAccountSigner (there is no reason to split this as a an account entry with signers unsorted is illegal)
2. as removing a signer doesn't change the sort order, we don't need to normalize
3. basically, we can just remove the call to normalizeSigners
4. we already check that signers are sorted with the LedgerEntryIsValid invariant (so no need to add an extra assert)

maybe we could add a bool property to signatureChecker (set where we used to add to the map of used one time signatures) to know if one time signers were used at all to decide if we want to call that function?
The new code is always going to loop over all accounts / all signers (so loading up to 101 accounts and perform 101*20=2020 extra checks for no reason)

I don't think this makes sense. In the event that we aren't checking signatures (eg. cv < ValidationType::kInvalidPostAuth) then we have to loop over all accounts and signers, since they haven't been checked by SignatureChecker. In the event that we are checking signatures (eg. cv >= ValidationType::kInvalidPostAuth) then we have already loaded the accounts in the SignatureChecker so they are in memory and looping over the accounts and signers requires no I/O.

In the worst case, we have to do the full loop including I/O. In all other cases, we have already done the I/O so it shouldn't be expensive. If at a future date we assess that this is an unacceptable amount of overhead, let's optimize and make the code more complicated at that point.

I guess this was more a comment on the fact that in the previous version we had 0 overhead when not using one time signers to an implementation that bears the overhead all the time (all that for a feature that is virtually never used).

I am quite not sure what the overhead of this is: maintaining LedgerTxn is not free, we had examples in the past where the overhead was managing maps.

I guess what we can do is perform some performance test comparison of this new implementation to compare it against 12.5.0, replaying protocol 12 ledger ranges with version 13.0.0 both with protocol 12 and 13.

I don't understand the name kMaybeValidCheckSignatures: it's invalid for sure as it failed with txINSUFFICIENT_BALANCE.

If we want to rename something, I'd rename kFullyValid that is kMaybeValid as we didn't check for signatures yet.

Also, per my previous comment, the only reason we're calling processSignature in this case is to remove one time signers, so it would be a lot clearer to move the call to removeOneTimeSignerFromAllSourceAccounts into that processSignature function.

I would change the contract of processSignatures to return if the transaction is valid.
So this code would instead look like

valid = processSignatures(cv, signatureChecker, ltxTx);

(and we'd remove the valid = signaturesValid && (cv == ValidationType::kFullyValid); later)

As we make this change, I think it's better to "fast fail" with the proper error code in protocol 13. This way the transaction will fail with txINSUFFICIENT_BALANCE instead of (potentially) some other error like txBAD_AUTH_EXTRA.

the actual function then becomes:

bool
TransactionFrame::processSignatures(ValidationType cv, SignatureChecker& signatureChecker,
                                    AbstractLedgerTxn& ltxOuter)
{
    auto maybeValid = (cv == ValidationType::kMaybeValid);
    auto allOpsValid = true;
    uint32_t ledgerVersion;
    LedgerTxn ltx(ltxOuter);
    ledgerVersion = ltx.loadHeader().current().ledgerVersion;
    if (ledgerVersion < 10)
    {
        return maybeValid;
    }

    // check if we need to fast fail and use the original error code
    if (ledgerVersion >= 13 && !maybeValid)
    {
        removeOneTimeSignerFromAllSourceAccounts(ltx);
        ltx.commit();
        return false;
    }
    // older versions of the protocol only fast fail in a subset of cases
    if (ledgerVersion < 13 && cv < ValidationType::kInvalidPostAuth)
    {
        return false;
    }

    {
        // scope here to avoid potential side effects of loading source accounts
        LedgerTxn ltx2(ltx);            
        for (auto& op : mOperations)
        {
            if (!op->checkSignature(signatureChecker, ltx2, false))
            {
                allOpsValid = false;
            }
        }
    }

    removeOneTimeSignerFromAllSourceAccounts(ltx);

    ltx.commit();

    if (!allOpsValid)
    {
        markResultFailed();
        return false;
    }

    if (!signatureChecker.checkAllSignaturesUsed())
    {
        getResult().result.code(txBAD_AUTH_EXTRA);
        return false;
    }

    return maybeValid;
}

As I added a comment on the scope around LedgerTxn ltx2(ltx), I realized that:

• either this scope is not needed (pretty much sure it is)
• or we have a potential bug with the new removeOneTimeSignerFromAllSourceAccounts that loads all source accounts - it should only load & modify the ones that get changed

I don't understand the name kMaybeValidCheckSignatures: it's invalid for sure as it failed with txINSUFFICIENT_BALANCE.

As we make this change, I think it's better to "fast fail" with the proper error code in protocol 13. This way the transaction will fail with txINSUFFICIENT_BALANCE instead of (potentially) some other error like txBAD_AUTH_EXTRA.

Yes, you're absolutely right about this. When I wrote that comment I didn't notice that we check the balance then check the signatures regardless of the result of the balance check. In an ideal world, we would never have even checked signatures in this case. Let's make this change in protocol 13. Let's also rename kFullyValid to kMaybeValid as you suggest.

Also in the code @MonsieurNicolas suggested, I don't believe that LedgerTxn ltx(ltxOuter); (line 8) is required. However, the scope with LedgerTxn ltx2(ltx); is required (but let's just call it ltx now).

You are correct that removeOneTimeSignerFromAllSourceAccounts is currently wrong, it should only load and modify the ones that get changed.

I modified removeOneTimeSignerFromAllSourceAccounts to first look for the signer through loadAccountWithoutRecord.

Something to scrutinize here is that I use the iterator derived from the loadAccountWithoutRecord account signers to erase from the loadAccount account signers to avoid a second search. This should be fine since the LedgerEntry from the first load should be cached, but I'm wondering if it might be safer and cleaner to just do the second search if we get a match on the signer (which isn't a common scenario). @jonjove

I also added a check in TxTests.cpp that verifies any delta from an early failure post V13 must have a signature removed.

This isn't safe. Every time a new LedgerEntry is loaded it produces a copy. So the vector of signers for the mutable LedgerTxnEntry will be a copy of your initial vector from the ConstLedgerTxnEntry; the initial vector may even have been destructed.

That being said, you don't actually need to search again. The signers are sorted which guarantees they won't change order, so you can calculate the index of the relevant signer. Then you can just index into the new vector directly (I would recommend adding an assert here to check that you actually found the right signer).

Fixed