Refactored app out of checkValid

SirTyson · Jul 9, 2024 · 7623494 · 7623494
1 parent 1d0fc8d
commit 7623494
Show file tree

Hide file tree

Showing 14 changed files with 185 additions and 141 deletions.
diff --git a/src/herder/TransactionQueue.cpp b/src/herder/TransactionQueue.cpp
@@ -326,7 +326,8 @@ TransactionQueue::canAdd(TransactionFrameBasePtr tx,
             {
                 auto txResult = tx->createSuccessResult();
                 if (!tx->checkSorobanResourceAndSetError(
-                        mApp,
+                        mApp.getLedgerManager().getSorobanNetworkConfig(),
+                        mApp.getConfig(),
                         mApp.getLedgerManager()
                             .getLastClosedLedgerHeader()
                             .header.ledgerVersion,

diff --git a/src/ledger/LedgerManager.h b/src/ledger/LedgerManager.h
@@ -126,9 +126,12 @@ class LedgerManager
     virtual void updateNetworkConfig(AbstractLedgerTxn& ltx) = 0;
     // Return the network config for Soroban.
     // The config is automatically refreshed on protocol upgrades.
-    // Ledger txn here is needed for the sake of lazy load; it won't be
-    // used most of the time.
     virtual SorobanNetworkConfig const& getSorobanNetworkConfig() = 0;
+
+    // Returns a pointer the network config for Soroban, or nullptr if
+    // ledgerVersion < Protocol 20.
+    virtual SorobanNetworkConfig const*
+    maybeGetSorobanNetworkConfigPtr(uint32_t ledgerVersion) = 0;
     virtual bool hasSorobanNetworkConfig() const = 0;
 
 #ifdef BUILD_TESTS

diff --git a/src/ledger/LedgerManagerImpl.cpp b/src/ledger/LedgerManagerImpl.cpp
@@ -561,6 +561,17 @@ LedgerManagerImpl::getSorobanNetworkConfig()
     return getSorobanNetworkConfigInternal();
 }
 
+SorobanNetworkConfig const*
+LedgerManagerImpl::maybeGetSorobanNetworkConfigPtr(uint32_t ledgerVersion)
+{
+    if (protocolVersionStartsFrom(ledgerVersion, SOROBAN_PROTOCOL_VERSION))
+    {
+        return &getSorobanNetworkConfigInternal();
+    }
+
+    return nullptr;
+}
+
 bool
 LedgerManagerImpl::hasSorobanNetworkConfig() const
 {

diff --git a/src/ledger/LedgerManagerImpl.h b/src/ledger/LedgerManagerImpl.h
@@ -150,6 +150,8 @@ class LedgerManagerImpl : public LedgerManager
     uint32_t getLastTxFee() const override;
     uint32_t getLastClosedLedgerNum() const override;
     SorobanNetworkConfig const& getSorobanNetworkConfig() override;
+    SorobanNetworkConfig const*
+    maybeGetSorobanNetworkConfigPtr(uint32_t ledgerVersion) override;
     bool hasSorobanNetworkConfig() const override;
 
 #ifdef BUILD_TESTS

diff --git a/src/test/FuzzerImpl.cpp b/src/test/FuzzerImpl.cpp
@@ -925,8 +925,10 @@ class FuzzTransactionFrame : public TransactionFrame
             mEnvelope.v1().signatures};
         // if any ill-formed Operations, do not attempt transaction application
         auto isInvalidOperation = [&](auto const& op, auto& opResult) {
-            return !op->checkValid(app, signatureChecker, ltx, false, opResult,
-                                   mTxResult->getSorobanData());
+            return !op->checkValid(
+                &app.getLedgerManager().getSorobanNetworkConfig(),
+                app.getConfig(), signatureChecker, ltx, false, opResult,
+                mTxResult->getSorobanData());
         };
 
         auto const& ops = getOperations();
@@ -944,7 +946,7 @@ class FuzzTransactionFrame : public TransactionFrame
         // while the following method's result is not captured, regardless, for
         // protocols < 8, this triggered buggy caching, and potentially may do
         // so in the future
-        loadSourceAccount(ltx, ltx.loadHeader());
+        loadSourceAccount(ltx, ltx.loadHeader().current());
         processSeqNum(ltx);
         TransactionMetaFrame tm(2);
         applyOperations(signatureChecker, app, ltx, tm, *mTxResult, Hash{});

diff --git a/src/transactions/FeeBumpTransactionFrame.cpp b/src/transactions/FeeBumpTransactionFrame.cpp
@@ -132,8 +132,7 @@ FeeBumpTransactionFrame::processPostApply(Application& app,
     // Note that we are not calling TransactionFrame::processPostApply, so if
     // any logic is added there, we would have to reason through if that logic
     // should also be reflected here.
-    int64_t refund =
-        mInnerTx->processRefund(app, ltx, meta, getFeeSourceID(), *txResult);
+    mInnerTx->processRefund(app, ltx, meta, getFeeSourceID(), *txResult);
 }
 
 bool
@@ -187,8 +186,14 @@ FeeBumpTransactionFrame::checkValid(Application& app,
         return txResult;
     }
 
+    auto& cfg = app.getConfig();
+
+    auto header = ltx.loadHeader().current();
+    auto sorobanCfg = app.getLedgerManager().maybeGetSorobanNetworkConfigPtr(
+        header.ledgerVersion);
+
     auto innerTxResult = mInnerTx->checkValidWithOptionallyChargedFee(
-        app, ltx, current, false, lowerBoundCloseTimeOffset,
+        ltx, cfg, sorobanCfg, header, current, false, lowerBoundCloseTimeOffset,
         upperBoundCloseTimeOffset);
     auto finalTxResult = createSuccessResultWithNewInnerTx(
         std::move(txResult), std::move(innerTxResult), mInnerTx);
@@ -198,10 +203,11 @@ FeeBumpTransactionFrame::checkValid(Application& app,
 
 bool
 FeeBumpTransactionFrame::checkSorobanResourceAndSetError(
-    Application& app, uint32_t ledgerVersion, MutableTxResultPtr txResult) const
+    SorobanNetworkConfig const& sorobanConfig, Config const& cfg,
+    uint32_t ledgerVersion, MutableTxResultPtr txResult) const
 {
-    return mInnerTx->checkSorobanResourceAndSetError(app, ledgerVersion,
-                                                     txResult);
+    return mInnerTx->checkSorobanResourceAndSetError(sorobanConfig, cfg,
+                                                     ledgerVersion, txResult);
 }
 
 bool

diff --git a/src/transactions/FeeBumpTransactionFrame.h b/src/transactions/FeeBumpTransactionFrame.h
@@ -81,7 +81,8 @@ class FeeBumpTransactionFrame : public TransactionFrameBase
                SequenceNumber current, uint64_t lowerBoundCloseTimeOffset,
                uint64_t upperBoundCloseTimeOffset) const override;
     bool
-    checkSorobanResourceAndSetError(Application& app, uint32_t ledgerVersion,
+    checkSorobanResourceAndSetError(SorobanNetworkConfig const& sorobanConfig,
+                                    Config const& cfg, uint32_t ledgerVersion,
                                     MutableTxResultPtr txResult) const override;
 
     MutableTxResultPtr createSuccessResult() const override;

diff --git a/src/transactions/OperationFrame.cpp b/src/transactions/OperationFrame.cpp
@@ -142,8 +142,15 @@ OperationFrame::apply(Application& app, SignatureChecker& signatureChecker,
 {
     ZoneScoped;
     CLOG_TRACE(Tx, "{}", xdrToCerealString(mOperation, "Operation"));
-    bool applyRes =
-        checkValid(app, signatureChecker, ltx, true, res, sorobanData);
+
+    SorobanNetworkConfig const* sorobanCfg = nullptr;
+    if (isSoroban())
+    {
+        sorobanCfg = &app.getLedgerManager().getSorobanNetworkConfig();
+    }
+
+    bool applyRes = checkValid(sorobanCfg, app.getConfig(), signatureChecker,
+                               ltx, true, res, sorobanData);
     if (applyRes)
     {
         if (isSoroban())
@@ -197,7 +204,7 @@ OperationFrame::checkSignature(SignatureChecker& signatureChecker,
     {
         auto neededThreshold =
             getNeededThreshold(sourceAccount, getThresholdLevel());
-        if (!mParentTx.checkSignature(signatureChecker, sourceAccount,
+        if (!mParentTx.checkSignature(signatureChecker, sourceAccount.current(),
                                       neededThreshold))
         {
             res.code(opBAD_AUTH);
@@ -235,7 +242,9 @@ OperationFrame::getSourceID() const
 // make sure sig is correct
 // verifies that the operation is well formed (operation specific)
 bool
-OperationFrame::checkValid(Application& app, SignatureChecker& signatureChecker,
+OperationFrame::checkValid(SorobanNetworkConfig const* const sorobanCfg,
+                           Config const& cfg,
+                           SignatureChecker& signatureChecker,
                            AbstractLedgerTxn& ltxOuter, bool forApply,
                            OperationResult& res,
                            std::shared_ptr<SorobanTxData> sorobanData) const
@@ -274,12 +283,10 @@ OperationFrame::checkValid(Application& app, SignatureChecker& signatureChecker,
     if (protocolVersionStartsFrom(ledgerVersion, SOROBAN_PROTOCOL_VERSION) &&
         isSoroban())
     {
+        releaseAssertOrThrow(sorobanCfg);
         releaseAssertOrThrow(sorobanData);
-        auto const& sorobanConfig =
-            app.getLedgerManager().getSorobanNetworkConfig();
-
-        return doCheckValidForSoroban(sorobanConfig, app.getConfig(),
-                                      ledgerVersion, res, *sorobanData);
+        return doCheckValidForSoroban(*sorobanCfg, cfg, ledgerVersion, res,
+                                      *sorobanData);
     }
     else
     {
@@ -302,7 +309,7 @@ OperationFrame::loadSourceAccount(AbstractLedgerTxn& ltx,
                                   LedgerTxnHeader const& header) const
 {
     ZoneScoped;
-    return mParentTx.loadAccount(ltx, header, getSourceID());
+    return mParentTx.loadAccount(ltx, header.current(), getSourceID());
 }
 
 void

diff --git a/src/transactions/OperationFrame.h b/src/transactions/OperationFrame.h
@@ -85,7 +85,8 @@ class OperationFrame
 
     AccountID getSourceID() const;
 
-    bool checkValid(Application& app, SignatureChecker& signatureChecker,
+    bool checkValid(SorobanNetworkConfig const* const sorobanCfg,
+                    Config const& cfg, SignatureChecker& signatureChecker,
                     AbstractLedgerTxn& ltxOuter, bool forApply,
                     OperationResult& res,
                     std::shared_ptr<SorobanTxData> sorobanData) const;