Address feedback

Author: marta-lokhova

Diff for: src/herder/TransactionQueue.cpp

@@ -186,49 +186,53 @@ TransactionQueue::canAdd(TransactionFrameBasePtr tx,
     TransactionFrameBasePtr currentTx;
     if (stateIter != mAccountStates.end())
     {
-        auto transaction = stateIter->second.mTransaction;
+        auto const& transaction = stateIter->second.mTransaction;
 
         if (transaction)
         {
             currentTx = transaction->mTx;
-            if (tx->getEnvelope().type() != ENVELOPE_TYPE_TX_FEE_BUMP)
+
+            // Check if the tx is a duplicate
+            if (isDuplicateTx(currentTx, tx))
             {
-                if (tx->getSeqNum() == transaction->mTx->getSeqNum() &&
-                    isDuplicateTx(transaction->mTx, tx))
-                {
-                    return TransactionQueue::AddResult::ADD_STATUS_DUPLICATE;
-                }
+                return TransactionQueue::AddResult::ADD_STATUS_DUPLICATE;
+            }
 
+            // Any transaction older than the current one is invalid
+            if (tx->getSeqNum() < currentTx->getSeqNum())
+            {
+                // If the transaction is older than the one in the queue, we
+                // reject it
+                tx->getResult().result.code(txBAD_SEQ);
+                return TransactionQueue::AddResult::ADD_STATUS_ERROR;
+            }
+
+            if (tx->getEnvelope().type() != ENVELOPE_TYPE_TX_FEE_BUMP)
+            {
                 // If there's already a transaction in the queue, we reject
                 // any new transaction
                 return TransactionQueue::AddResult::ADD_STATUS_TRY_AGAIN_LATER;
             }
             else
             {
-                if (tx->getSeqNum() != transaction->mTx->getSeqNum())
+                if (tx->getSeqNum() != currentTx->getSeqNum())
                 {
                     // New fee-bump transaction is rejected
                     return TransactionQueue::AddResult::
                         ADD_STATUS_TRY_AGAIN_LATER;
                 }
 
-                // Replace-by-fee logic
-                if (isDuplicateTx(transaction->mTx, tx))
-                {
-                    return TransactionQueue::AddResult::ADD_STATUS_DUPLICATE;
-                }
-
                 int64_t minFee;
-                if (!canReplaceByFee(tx, transaction->mTx, minFee))
+                if (!canReplaceByFee(tx, currentTx, minFee))
                 {
                     tx->getResult().result.code(txINSUFFICIENT_FEE);
                     tx->getResult().feeCharged = minFee;
                     return TransactionQueue::AddResult::ADD_STATUS_ERROR;
                 }
 
-                if (transaction->mTx->getFeeSourceID() == tx->getFeeSourceID())
+                if (currentTx->getFeeSourceID() == tx->getFeeSourceID())
                 {
-                    newFullFee -= transaction->mTx->getFullFee();
+                    newFullFee -= currentTx->getFullFee();
                 }
             }
         }
@@ -485,7 +489,7 @@ TransactionQueue::tryAdd(TransactionFrameBasePtr tx, bool submittedFromSelf)
 }
 
 void
-TransactionQueue::dropTransactions(AccountStates::iterator stateIter)
+TransactionQueue::dropTransaction(AccountStates::iterator stateIter)
 {
     ZoneScoped;
     // Remove fees and update queue size for each transaction to be dropped.
@@ -526,7 +530,7 @@ TransactionQueue::removeApplied(Transactions const& appliedTxs)
         {
             // If there are no transactions in the queue for this source
             // account, then there is nothing to do
-            auto transaction = stateIter->second.mTransaction;
+            auto const& transaction = stateIter->second.mTransaction;
             if (transaction)
             {
                 // We care about matching the sequence number rather than
@@ -555,7 +559,7 @@ TransactionQueue::removeApplied(Transactions const& appliedTxs)
 
                     // WARNING: stateIter and everything that references it may
                     // be invalid from this point onward and should not be used.
-                    dropTransactions(stateIter);
+                    dropTransaction(stateIter);
                 }
             }
         }
@@ -599,15 +603,17 @@ TransactionQueue::ban(Transactions const& banTxs)
         auto stateIter = mAccountStates.find(kv.first);
         if (stateIter != mAccountStates.end())
         {
-            auto transaction = stateIter->second.mTransaction;
+            auto const& transaction = stateIter->second.mTransaction;
             // Only ban transactions that are actually present in the queue.
             // Transactions with higher sequence numbers than banned
             // transactions remain in the queue.
             if (transaction &&
                 transaction->mTx->getFullHash() == kv.second->getFullHash())
             {
                 mSizeByAge[stateIter->second.mAge]->dec();
-                dropTransactions(stateIter);
+                // WARNING: stateIter and everything that references it may
+                // be invalid from this point onward and should not be used.
+                dropTransaction(stateIter);
             }
         }
     }

Diff for: src/herder/TransactionQueue.h

@@ -109,8 +109,9 @@ class TransactionQueue
 
     AddResult tryAdd(TransactionFrameBasePtr tx, bool submittedFromSelf);
     void removeApplied(Transactions const& txs);
-    // Ban transactions that are no longer valid; transaction per account limit
-    // applies here, so `txs` should have no duplicate source accounts
+    // Ban transactions that are no longer valid or have insufficient fee;
+    // transaction per account limit applies here, so `txs` should have no
+    // duplicate source accounts
     void ban(Transactions const& txs);
 
     /**
@@ -192,7 +193,7 @@ class TransactionQueue
     void releaseFeeMaybeEraseAccountState(TransactionFrameBasePtr tx);
 
     void prepareDropTransaction(AccountState& as);
-    void dropTransactions(AccountStates::iterator stateIter);
+    void dropTransaction(AccountStates::iterator stateIter);
 
     void clearAll();
 

Diff for: src/herder/test/TransactionQueueTests.cpp

@@ -556,19 +556,21 @@ testTransactionQueueBasicScenarios()
         test.add(txSeqA1T1, TransactionQueue::AddResult::ADD_STATUS_DUPLICATE);
         test.check(state);
 
-        auto status = TransactionQueue::AddResult::ADD_STATUS_TRY_AGAIN_LATER;
-        test.add(txSeqA1T2, status);
+        test.add(txSeqA1T2,
+                 TransactionQueue::AddResult::ADD_STATUS_TRY_AGAIN_LATER);
         test.check(state);
 
-        // Regardless of seqnum validity, tx is rejected due to limit
+        // Tx is rejected due to limit or bad seqnum
         // too low
-        test.add(txSeqA1T0, status);
+        test.add(txSeqA1T0, TransactionQueue::AddResult::ADD_STATUS_ERROR);
         test.check(state);
         // too high
-        test.add(txSeqA1T4, status);
+        test.add(txSeqA1T4,
+                 TransactionQueue::AddResult::ADD_STATUS_TRY_AGAIN_LATER);
         test.check(state);
         // just right
-        test.add(txSeqA1T3, status);
+        test.add(txSeqA1T3,
+                 TransactionQueue::AddResult::ADD_STATUS_TRY_AGAIN_LATER);
         test.check(state);
     }