TransactionFrame Refactor

src/herder/HerderImpl.cpp

@@ -25,6 +25,7 @@
 #include "overlay/OverlayManager.h"
 #include "scp/LocalNode.h"
 #include "scp/Slot.h"
+#include "transactions/MutableTransactionResult.h"
 #include "transactions/TransactionUtils.h"
 #include "util/DebugMetaUtils.h"
 #include "util/LogSlowExecution.h"
@@ -587,7 +588,8 @@ TransactionQueue::AddResult
 HerderImpl::recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf)
 {
     ZoneScoped;
-    TransactionQueue::AddResult result;
+    TransactionQueue::AddResult result(
+        TransactionQueue::AddResultCode::ADD_STATUS_COUNT);
 
     // Allow txs of the same kind to reach the tx queue in case it can be
     // replaced by fee
@@ -605,7 +607,8 @@ HerderImpl::recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf)
                    "account per ledger limit",
                    hexAbbrev(tx->getFullHash()),
                    KeyUtils::toShortString(tx->getSourceID()));
-        result = TransactionQueue::AddResult::ADD_STATUS_TRY_AGAIN_LATER;
+        result.code =
+            TransactionQueue::AddResultCode::ADD_STATUS_TRY_AGAIN_LATER;
     }
     else if (!tx->isSoroban())
     {
@@ -619,10 +622,10 @@ HerderImpl::recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf)
     {
         // Received Soroban transaction before protocol 20; since this
         // transaction isn't supported yet, return ERROR
-        result = TransactionQueue::AddResult::ADD_STATUS_ERROR;
+        result.code = TransactionQueue::AddResultCode::ADD_STATUS_ERROR;
     }
 
-    if (result == TransactionQueue::AddResult::ADD_STATUS_PENDING)
+    if (result.code == TransactionQueue::AddResultCode::ADD_STATUS_PENDING)
     {
         CLOG_TRACE(Herder, "recv transaction {} for {}",
                    hexAbbrev(tx->getFullHash()),

src/herder/TransactionQueue.cpp

@@ -12,8 +12,8 @@
 #include "ledger/LedgerTxn.h"
 #include "main/Application.h"
 #include "overlay/OverlayManager.h"
-#include "test/TxTests.h"
 #include "transactions/FeeBumpTransactionFrame.h"
+#include "transactions/MutableTransactionResult.h"
 #include "transactions/OperationFrame.h"
 #include "transactions/TransactionBridge.h"
 #include "transactions/TransactionUtils.h"
@@ -38,15 +38,41 @@
 #include <optional>
 #include <random>
 
+#ifdef BUILD_TESTS
+#include "test/TxTests.h"
+#include "transactions/test/TransactionTestFrame.h"
+#endif
+
 namespace stellar
 {
 const uint64_t TransactionQueue::FEE_MULTIPLIER = 10;
 
 std::array<const char*,
-           static_cast<int>(TransactionQueue::AddResult::ADD_STATUS_COUNT)>
+           static_cast<int>(TransactionQueue::AddResultCode::ADD_STATUS_COUNT)>
     TX_STATUS_STRING = std::array{"PENDING", "DUPLICATE", "ERROR",
                                   "TRY_AGAIN_LATER", "FILTERED"};
 
+TransactionQueue::AddResult::AddResult(AddResultCode addCode)
+    : code(addCode), txResult()
+{
+}
+
+TransactionQueue::AddResult::AddResult(AddResultCode addCode,
+                                       MutableTxResultPtr payload)
+    : code(addCode), txResult(payload)
+{
+    releaseAssert(txResult);
+}
+
+TransactionQueue::AddResult::AddResult(AddResultCode addCode,
+                                       TransactionFrameBasePtr tx,
+                                       TransactionResultCode txErrorCode)
+    : code(addCode), txResult(tx->createSuccessResult())
+{
+    releaseAssert(txErrorCode != txSUCCESS);
+    txResult.value()->setResultCode(txErrorCode);
+}
+
 TransactionQueue::TransactionQueue(Application& app, uint32 pendingDepth,
                                    uint32 banDepth, uint32 poolLedgerMultiplier,
                                    bool isSoroban)
@@ -218,9 +244,21 @@ isDuplicateTx(TransactionFrameBasePtr oldTx, TransactionFrameBasePtr newTx)
     }
     else if (oldEnv.type() == ENVELOPE_TYPE_TX_FEE_BUMP)
     {
-        auto oldFeeBump =
-            std::static_pointer_cast<FeeBumpTransactionFrame>(oldTx);
-        return oldFeeBump->getInnerFullHash() == newTx->getFullHash();
+        std::shared_ptr<FeeBumpTransactionFrame const> feeBumpPtr{};
+#ifdef BUILD_TESTS
+        if (oldTx->isTestTx())
+        {
+            auto testFrame =
+                std::static_pointer_cast<TransactionTestFrame const>(oldTx);
+            feeBumpPtr =
+                std::static_pointer_cast<FeeBumpTransactionFrame const>(
+                    testFrame->getTxFramePtr());
+        }
+        else
+#endif
+            feeBumpPtr =
+                std::static_pointer_cast<FeeBumpTransactionFrame const>(oldTx);
+        return feeBumpPtr->getInnerFullHash() == newTx->getFullHash();
     }
     return false;
 }
@@ -239,18 +277,19 @@ TransactionQueue::canAdd(
     ZoneScoped;
     if (isBanned(tx->getFullHash()))
     {
-        return TransactionQueue::AddResult::ADD_STATUS_TRY_AGAIN_LATER;
+        return AddResult(
+            TransactionQueue::AddResultCode::ADD_STATUS_TRY_AGAIN_LATER);
     }
     if (isFiltered(tx))
     {
-        return TransactionQueue::AddResult::ADD_STATUS_FILTERED;
+        return AddResult(TransactionQueue::AddResultCode::ADD_STATUS_FILTERED);
     }
 
     int64_t newFullFee = tx->getFullFee();
     if (newFullFee < 0 || tx->getInclusionFee() < 0)
     {
-        tx->getResult().result.code(txMALFORMED);
-        return TransactionQueue::AddResult::ADD_STATUS_ERROR;
+        return AddResult(TransactionQueue::AddResultCode::ADD_STATUS_ERROR, tx,
+                         txMALFORMED);
     }
 
     stateIter = mAccountStates.find(tx->getSourceID());
@@ -266,53 +305,61 @@ TransactionQueue::canAdd(
             // Check if the tx is a duplicate
             if (isDuplicateTx(currentTx, tx))
             {
-                return TransactionQueue::AddResult::ADD_STATUS_DUPLICATE;
+                return AddResult(
+                    TransactionQueue::AddResultCode::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;
+                return AddResult(
+                    TransactionQueue::AddResultCode::ADD_STATUS_ERROR, tx,
+                    txBAD_SEQ);
             }
 
             // Before rejecting Soroban transactions due to source account
             // limit, check validity of its declared resources, and return an
             // appropriate error message
             if (tx->isSoroban())
             {
+                auto txResult = tx->createSuccessResult();
                 if (!tx->checkSorobanResourceAndSetError(
-                        mApp, mApp.getLedgerManager()
-                                  .getLastClosedLedgerHeader()
-                                  .header.ledgerVersion))
+                        mApp,
+                        mApp.getLedgerManager()
+                            .getLastClosedLedgerHeader()
+                            .header.ledgerVersion,
+                        txResult))
                 {
-                    return TransactionQueue::AddResult::ADD_STATUS_ERROR;
+                    return AddResult(AddResultCode::ADD_STATUS_ERROR, txResult);
                 }
             }
 
             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;
+                return AddResult(TransactionQueue::AddResultCode::
+                                     ADD_STATUS_TRY_AGAIN_LATER);
             }
             else
             {
                 if (tx->getSeqNum() != currentTx->getSeqNum())
                 {
                     // New fee-bump transaction is rejected
-                    return TransactionQueue::AddResult::
-                        ADD_STATUS_TRY_AGAIN_LATER;
+                    return AddResult(TransactionQueue::AddResultCode::
+                                         ADD_STATUS_TRY_AGAIN_LATER);
                 }
 
                 int64_t minFee;
                 if (!canReplaceByFee(tx, currentTx, minFee))
                 {
-                    tx->getResult().result.code(txINSUFFICIENT_FEE);
-                    tx->getResult().feeCharged = minFee;
-                    return TransactionQueue::AddResult::ADD_STATUS_ERROR;
+                    AddResult result(
+                        TransactionQueue::AddResultCode::ADD_STATUS_ERROR, tx,
+                        txINSUFFICIENT_FEE);
+                    result.txResult.value()->getResult().feeCharged = minFee;
+                    return result;
                 }
 
                 if (currentTx->getFeeSourceID() == tx->getFeeSourceID())
@@ -337,11 +384,14 @@ TransactionQueue::canAdd(
         ban({tx});
         if (canAddRes.second != 0)
         {
-            tx->getResult().result.code(txINSUFFICIENT_FEE);
-            tx->getResult().feeCharged = canAddRes.second;
-            return TransactionQueue::AddResult::ADD_STATUS_ERROR;
+            AddResult result(TransactionQueue::AddResultCode::ADD_STATUS_ERROR,
+                             tx, txINSUFFICIENT_FEE);
+            result.txResult.value()->getResult().feeCharged = canAddRes.second;
+            return result;
         }
-        return TransactionQueue::AddResult::ADD_STATUS_TRY_AGAIN_LATER;
+        return AddResult(
+            TransactionQueue::AddResultCode::ADD_STATUS_TRY_AGAIN_LATER,
+            nullptr);
     }
 
     auto closeTime = mApp.getLedgerManager()
@@ -355,10 +405,12 @@ TransactionQueue::canAdd(
             mApp.getLedgerManager().getLastClosedLedgerNum() + 1;
     }
 
-    if (!tx->checkValid(mApp, ltx, 0, 0,
-                        getUpperBoundCloseTimeOffset(mApp, closeTime)))
+    auto txResult = tx->checkValid(
+        mApp, ltx, 0, 0, getUpperBoundCloseTimeOffset(mApp, closeTime));
+    if (!txResult->isSuccess())
     {
-        return TransactionQueue::AddResult::ADD_STATUS_ERROR;
+        return AddResult(TransactionQueue::AddResultCode::ADD_STATUS_ERROR,
+                         txResult);
     }
 
     // Note: stateIter corresponds to getSourceID() which is not necessarily
@@ -371,11 +423,13 @@ TransactionQueue::canAdd(
     if (getAvailableBalance(ltx.loadHeader(), feeSource) - newFullFee <
         totalFees)
     {
-        tx->getResult().result.code(txINSUFFICIENT_BALANCE);
-        return TransactionQueue::AddResult::ADD_STATUS_ERROR;
+        txResult->setResultCode(txINSUFFICIENT_BALANCE);
+        return AddResult(TransactionQueue::AddResultCode::ADD_STATUS_ERROR,
+                         txResult);
     }
 
-    return TransactionQueue::AddResult::ADD_STATUS_PENDING;
+    return AddResult(TransactionQueue::AddResultCode::ADD_STATUS_PENDING,
+                     txResult);
 }
 
 void
@@ -529,15 +583,15 @@ TransactionQueue::tryAdd(TransactionFrameBasePtr tx, bool submittedFromSelf)
     // fast fail when Soroban tx is malformed
     if ((tx->isSoroban() != (c1 || c2)) || !tx->XDRProvidesValidFee())
     {
-        tx->getResult().result.code(txMALFORMED);
-        return TransactionQueue::AddResult::ADD_STATUS_ERROR;
+        return AddResult(TransactionQueue::AddResultCode::ADD_STATUS_ERROR, tx,
+                         txMALFORMED);
     }
 
     AccountStates::iterator stateIter;
 
     std::vector<std::pair<TransactionFrameBasePtr, bool>> txsToEvict;
     auto const res = canAdd(tx, stateIter, txsToEvict);
-    if (res != TransactionQueue::AddResult::ADD_STATUS_PENDING)
+    if (res.code != TransactionQueue::AddResultCode::ADD_STATUS_PENDING)
     {
         return res;
     }

src/herder/TransactionQueue.h

@@ -62,7 +62,7 @@ class TransactionQueue
   public:
     static uint64_t const FEE_MULTIPLIER;
 
-    enum class AddResult
+    enum class AddResultCode
     {
         ADD_STATUS_PENDING = 0,
         ADD_STATUS_DUPLICATE,
@@ -72,6 +72,24 @@ class TransactionQueue
         ADD_STATUS_COUNT
     };
 
+    struct AddResult
+    {
+        TransactionQueue::AddResultCode code;
+        std::optional<MutableTxResultPtr> txResult;
+
+        // AddResult with no txResult
+        explicit AddResult(TransactionQueue::AddResultCode addCode);
+
+        // AddResult from existing transaction result
+        explicit AddResult(TransactionQueue::AddResultCode addCode,
+                           MutableTxResultPtr payload);
+
+        // AddResult with error txResult with the specified txErrorCode
+        explicit AddResult(TransactionQueue::AddResultCode addCode,
+                           TransactionFrameBasePtr tx,
+                           TransactionResultCode txErrorCode);
+    };
+
     /**
      * AccountState stores the following information:
      * - mTotalFees: the sum of feeBid() over every transaction for which this
@@ -200,7 +218,8 @@ class TransactionQueue
         BROADCAST_STATUS_SKIPPED
     };
     BroadcastStatus broadcastTx(TimestampedTx& tx);
-    AddResult
+
+    TransactionQueue::AddResult
     canAdd(TransactionFrameBasePtr tx, AccountStates::iterator& stateIter,
            std::vector<std::pair<TransactionFrameBasePtr, bool>>& txsToEvict);
 
@@ -281,7 +300,6 @@ class ClassicTransactionQueue : public TransactionQueue
 
 extern std::array<const char*,
                   static_cast<int>(
-                      TransactionQueue::AddResult::ADD_STATUS_COUNT)>
+                      TransactionQueue::AddResultCode::ADD_STATUS_COUNT)>
     TX_STATUS_STRING;
-
 }

src/herder/TxSetFrame.cpp

@@ -17,6 +17,7 @@
 #include "main/Application.h"
 #include "main/Config.h"
 #include "overlay/Peer.h"
+#include "transactions/MutableTransactionResult.h"
 #include "transactions/TransactionUtils.h"
 #include "util/GlobalChecks.h"
 #include "util/Logging.h"
@@ -241,14 +242,15 @@ phaseTxsAreValid(TxSetTransactions const& phase, Application& app,
         app.getLedgerManager().getLastClosedLedgerNum() + 1;
     for (auto const& tx : phase)
     {
-        if (!tx->checkValid(app, ltx, 0, lowerBoundCloseTimeOffset,
-                            upperBoundCloseTimeOffset))
+        auto txResult = tx->checkValid(app, ltx, 0, lowerBoundCloseTimeOffset,
+                                       upperBoundCloseTimeOffset);
+        if (!txResult->isSuccess())
         {
 
             CLOG_DEBUG(
                 Herder, "Got bad txSet: tx invalid tx: {} result: {}",
                 xdrToCerealString(tx->getEnvelope(), "TransactionEnvelope"),
-                tx->getResultCode());
+                txResult->getResultCode());
             return false;
         }
     }

src/herder/TxSetUtils.cpp

@@ -14,6 +14,7 @@
 #include "ledger/LedgerTxnHeader.h"
 #include "main/Application.h"
 #include "main/Config.h"
+#include "transactions/MutableTransactionResult.h"
 #include "transactions/TransactionUtils.h"
 #include "util/GlobalChecks.h"
 #include "util/Logging.h"
@@ -161,8 +162,9 @@ TxSetUtils::getInvalidTxList(TxSetTransactions const& txs, Application& app,
 
     for (auto const& tx : txs)
     {
-        if (!tx->checkValid(app, ltx, 0, lowerBoundCloseTimeOffset,
-                            upperBoundCloseTimeOffset))
+        auto txResult = tx->checkValid(app, ltx, 0, lowerBoundCloseTimeOffset,
+                                       upperBoundCloseTimeOffset);
+        if (!txResult->isSuccess())
         {
             invalidTxs.emplace_back(tx);
         }