Merge pull request #2379 from sisuresh/cap_28

cap28 - remove pre auth signers on signature verification failure Reviewed-by: MonsieurNicolas
stellar · Apr 3, 2020 · 467666f · 467666f
2 parents f25cabe + 70878d5
commit 467666f
Show file tree

Hide file tree

Showing 6 changed files with 281 additions and 74 deletions.
diff --git a/src/test/TxTests.cpp b/src/test/TxTests.cpp
@@ -224,16 +224,18 @@ applyCheck(TransactionFramePtr tx, Application& app, bool checkSeqNum)
                 // do not perform the check if there was a failure before
                 // or during the sequence number processing
                 auto header = ltxTx.loadHeader();
-                if (checkSeqNum && header.current().ledgerVersion >= 10 &&
-                    !earlyFailure)
+                auto ledgerVersion = header.current().ledgerVersion;
+                if (checkSeqNum && ledgerVersion >= 10 && !earlyFailure)
                 {
                     REQUIRE(srcAccountAfter.current().data.account().seqNum ==
                             (srcAccountBefore.seqNum + 1));
                 }
                 // on failure, no other changes should have been made
                 if (!res)
                 {
-                    if (earlyFailure || header.current().ledgerVersion <= 9)
+                    bool noChangeOnEarlyFailure =
+                        earlyFailure && ledgerVersion < 13;
+                    if (noChangeOnEarlyFailure || ledgerVersion <= 9)
                     {
                         // no changes during an early failure
                         REQUIRE(ltxTx.getDelta().entry.empty());
@@ -247,6 +249,21 @@ applyCheck(TransactionFramePtr tx, Application& app, bool checkSeqNum)
                             REQUIRE(current);
                             auto previous = kvp.second.previous;
                             REQUIRE(previous);
+
+                            // From V13, it's possible to remove one-time
+                            // signers on early failures
+                            if (ledgerVersion >= 13 && earlyFailure)
+                            {
+                                auto currAcc = current->data.account();
+                                auto prevAcc = previous->data.account();
+                                REQUIRE(currAcc.signers.size() + 1 ==
+                                        prevAcc.signers.size());
+                                // signers should be the only change so this
+                                // should make the accounts equivalent
+                                currAcc.signers = prevAcc.signers;
+                                currAcc.numSubEntries = prevAcc.numSubEntries;
+                                REQUIRE(currAcc == prevAcc);
+                            }
                         }
                         // could check more here if needed
                     }

diff --git a/src/transactions/SignatureChecker.cpp b/src/transactions/SignatureChecker.cpp
@@ -54,7 +54,6 @@ SignatureChecker::checkSignature(AccountID const& accountID,
     {
         if (signerKey.key.preAuthTx() == mContentsHash)
         {
-            mUsedOneTimeSignerKeys[accountID].insert(signerKey.key);
             auto w = signerKey.weight;
             if (mProtocolVersion > 9 && w > UINT8_MAX)
             {
@@ -141,10 +140,4 @@ SignatureChecker::checkAllSignaturesUsed() const
     }
     return true;
 }
-
-const UsedOneTimeSignerKeys&
-SignatureChecker::usedOneTimeSignerKeys() const
-{
-    return mUsedOneTimeSignerKeys;
-}
 };
diff --git a/src/transactions/SignatureChecker.h b/src/transactions/SignatureChecker.h
@@ -16,8 +16,6 @@
 namespace stellar
 {
 
-using UsedOneTimeSignerKeys = std::map<AccountID, std::set<SignerKey>>;
-
 class SignatureChecker
 {
   public:
@@ -30,14 +28,11 @@ class SignatureChecker
                         int32_t neededWeight);
     bool checkAllSignaturesUsed() const;
 
-    const UsedOneTimeSignerKeys& usedOneTimeSignerKeys() const;
-
   private:
     uint32_t mProtocolVersion;
     Hash const& mContentsHash;
     xdr::xvector<DecoratedSignature, 20> const& mSignatures;
 
     std::vector<bool> mUsedSignatures;
-    UsedOneTimeSignerKeys mUsedOneTimeSignerKeys;
 };
 };
diff --git a/src/transactions/TransactionFrame.cpp b/src/transactions/TransactionFrame.cpp
@@ -8,6 +8,7 @@
 #include "crypto/Hex.h"
 #include "crypto/SHA.h"
 #include "crypto/SignerKey.h"
+#include "crypto/SignerKeyUtils.h"
 #include "database/Database.h"
 #include "database/DatabaseUtils.h"
 #include "herder/TxSetFrame.h"
@@ -380,17 +381,33 @@ TransactionFrame::processSeqNum(AbstractLedgerTxn& ltx)
 }
 
 bool
-TransactionFrame::processSignatures(SignatureChecker& signatureChecker,
+TransactionFrame::processSignatures(ValidationType cv,
+                                    SignatureChecker& signatureChecker,
                                     AbstractLedgerTxn& ltxOuter)
 {
-    auto allOpsValid = true;
+    bool maybeValid = (cv == ValidationType::kMaybeValid);
+    uint32_t ledgerVersion = ltxOuter.loadHeader().current().ledgerVersion;
+    if (ledgerVersion < 10)
     {
-        LedgerTxn ltx(ltxOuter);
-        if (ltx.loadHeader().current().ledgerVersion < 10)
-        {
-            return true;
-        }
+        return maybeValid;
+    }
 
+    // check if we need to fast fail and use the original error code
+    if (ledgerVersion >= 13 && !maybeValid)
+    {
+        removeOneTimeSignerFromAllSourceAccounts(ltxOuter);
+        return false;
+    }
+    // older versions of the protocol only fast fail in a subset of cases
+    if (ledgerVersion < 13 && cv < ValidationType::kInvalidPostAuth)
+    {
+        return false;
+    }
+
+    bool allOpsValid = true;
+    {
+        // scope here to avoid potential side effects of loading source accounts
+        LedgerTxn ltx(ltxOuter);
         for (auto& op : mOperations)
         {
             if (!op->checkSignature(signatureChecker, ltx, false))
@@ -400,7 +417,7 @@ TransactionFrame::processSignatures(SignatureChecker& signatureChecker,
         }
     }
 
-    removeUsedOneTimeSignerKeys(signatureChecker, ltxOuter);
+    removeOneTimeSignerFromAllSourceAccounts(ltxOuter);
 
     if (!allOpsValid)
     {
@@ -414,7 +431,7 @@ TransactionFrame::processSignatures(SignatureChecker& signatureChecker,
         return false;
     }
 
-    return true;
+    return maybeValid;
 }
 
 bool
@@ -481,7 +498,7 @@ TransactionFrame::commonValid(SignatureChecker& signatureChecker,
         return res;
     }
 
-    return ValidationType::kFullyValid;
+    return ValidationType::kMaybeValid;
 }
 
 void
@@ -523,57 +540,54 @@ TransactionFrame::processFeeSeqNum(AbstractLedgerTxn& ltx, int64_t baseFee)
 }
 
 void
-TransactionFrame::removeUsedOneTimeSignerKeys(
-    SignatureChecker& signatureChecker, AbstractLedgerTxn& ltx)
+TransactionFrame::removeOneTimeSignerFromAllSourceAccounts(
+    AbstractLedgerTxn& ltx) const
 {
-    for (auto const& usedAccount : signatureChecker.usedOneTimeSignerKeys())
+    auto ledgerVersion = ltx.loadHeader().current().ledgerVersion;
+    if (ledgerVersion == 7)
     {
-        removeUsedOneTimeSignerKeys(ltx, usedAccount.first, usedAccount.second);
+        return;
     }
-}
 
-void
-TransactionFrame::removeUsedOneTimeSignerKeys(
-    AbstractLedgerTxn& ltx, AccountID const& accountID,
-    std::set<SignerKey> const& keys) const
-{
-    auto account = stellar::loadAccount(ltx, accountID);
-    if (!account)
+    std::unordered_set<AccountID> accounts{getSourceID()};
+    for (auto& op : mOperations)
     {
-        return; // probably account was removed due to merge operation
+        accounts.emplace(op->getSourceID());
     }
 
-    auto header = ltx.loadHeader();
-    auto changed = std::accumulate(
-        std::begin(keys), std::end(keys), false,
-        [&](bool r, const SignerKey& signerKey) {
-            return r || removeAccountSigner(header, account, signerKey);
-        });
-
-    if (changed)
+    auto signerKey = SignerKeyUtils::preAuthTxKey(*this);
+    for (auto const& accountID : accounts)
     {
-        normalizeSigners(account);
+        removeAccountSigner(ltx, accountID, signerKey);
     }
 }
 
-bool
-TransactionFrame::removeAccountSigner(LedgerTxnHeader const& header,
-                                      LedgerTxnEntry& account,
+void
+TransactionFrame::removeAccountSigner(AbstractLedgerTxn& ltxOuter,
+                                      AccountID const& accountID,
                                       SignerKey const& signerKey) const
 {
-    auto& acc = account.current().data.account();
+    LedgerTxn ltx(ltxOuter);
+
+    auto account = stellar::loadAccount(ltx, accountID);
+    if (!account)
+    {
+        return; // probably account was removed due to merge operation
+    }
+
+    auto& signers = account.current().data.account().signers;
     auto it = std::find_if(
-        std::begin(acc.signers), std::end(acc.signers),
+        std::begin(signers), std::end(signers),
         [&signerKey](Signer const& signer) { return signer.key == signerKey; });
-    if (it != std::end(acc.signers))
+
+    if (it != std::end(signers))
     {
+        auto header = ltx.loadHeader();
         auto removed = stellar::addNumEntries(header, account, -1);
         assert(removed == AddSubentryResult::SUCCESS);
-        acc.signers.erase(it);
-        return true;
+        signers.erase(it);
+        ltx.commit();
     }
-
-    return false;
 }
 
 bool
@@ -590,7 +604,7 @@ TransactionFrame::checkValid(AbstractLedgerTxn& ltxOuter,
                                       getContentsHash(),
                                       getSignatures(mEnvelope)};
     bool res = commonValid(signatureChecker, ltx, current, false, chargeFee) ==
-               ValidationType::kFullyValid;
+               ValidationType::kMaybeValid;
     if (res)
     {
         for (auto& op : mOperations)
@@ -710,7 +724,7 @@ TransactionFrame::applyOperations(SignatureChecker& signatureChecker,
                 // if an error occurred, it is responsibility of account's owner
                 // to remove that signer
                 LedgerTxn ltxAfter(ltxTx);
-                removeUsedOneTimeSignerKeys(signatureChecker, ltxAfter);
+                removeOneTimeSignerFromAllSourceAccounts(ltxAfter);
                 newMeta.v2().txChangesAfter = ltxAfter.getChanges();
                 ltxAfter.commit();
             }
@@ -793,8 +807,8 @@ TransactionFrame::apply(Application& app, AbstractLedgerTxn& ltx,
         {
             processSeqNum(ltxTx);
         }
-        auto signaturesValid = cv >= (ValidationType::kInvalidPostAuth) &&
-                               processSignatures(signatureChecker, ltxTx);
+
+        bool signaturesValid = processSignatures(cv, signatureChecker, ltxTx);
 
         auto& txChanges =
             meta.v() == 1 ? meta.v1().txChanges : meta.v2().txChangesBefore;
@@ -803,7 +817,7 @@ TransactionFrame::apply(Application& app, AbstractLedgerTxn& ltx,
                   std::back_inserter(txChanges));
         ltxTx.commit();
 
-        bool valid = signaturesValid && cv == ValidationType::kFullyValid;
+        bool valid = signaturesValid && cv == ValidationType::kMaybeValid;
         try
         {
             // This should only throw if the logging during exception handling

diff --git a/src/transactions/TransactionFrame.h b/src/transactions/TransactionFrame.h
@@ -61,7 +61,7 @@ class TransactionFrame : public TransactionFrameBase
                               // should be updated
         kInvalidPostAuth,     // transaction is invalid but its sequence number
                               // should be updated and one-time signers removed
-        kFullyValid
+        kMaybeValid
     };
 
     virtual bool isTooEarly(LedgerTxnHeader const& header) const;
@@ -79,15 +79,10 @@ class TransactionFrame : public TransactionFrameBase
     virtual std::shared_ptr<OperationFrame>
     makeOperation(Operation const& op, OperationResult& res, size_t index);
 
-    void removeUsedOneTimeSignerKeys(SignatureChecker& signatureChecker,
-                                     AbstractLedgerTxn& ltx);
+    void removeOneTimeSignerFromAllSourceAccounts(AbstractLedgerTxn& ltx) const;
 
-    void removeUsedOneTimeSignerKeys(AbstractLedgerTxn& ltx,
-                                     AccountID const& accountID,
-                                     std::set<SignerKey> const& keys) const;
-
-    bool removeAccountSigner(LedgerTxnHeader const& header,
-                             LedgerTxnEntry& account,
+    void removeAccountSigner(AbstractLedgerTxn& ltxOuter,
+                             AccountID const& accountID,
                              SignerKey const& signerKey) const;
 
     void markResultFailed();
@@ -97,7 +92,8 @@ class TransactionFrame : public TransactionFrameBase
 
     void processSeqNum(AbstractLedgerTxn& ltx);
 
-    bool processSignatures(SignatureChecker& signatureChecker,
+    bool processSignatures(ValidationType cv,
+                           SignatureChecker& signatureChecker,
                            AbstractLedgerTxn& ltxOuter);
 
   public: