Cleanup apply and validation flows (#4471)

Part of #4318 - main change is to stop passing around Application during validation and application flows, and use AppConnector instead, which is forced to either assert main thread or implement thread-safe methods Resolves #3800 - note that READ_ONLY_WITHOUT_SQL_TXN LedgerTxn mode should go away completely once we switch to mandatory BucketListDB. It looks like we were using RO LedgerTxn during apply scenarios, which I think is not legal? Either way, all READ_ONLY_WITHOUT_SQL_TXN usages except for the legacy one in LedgerSnapshot have been removed now. Note that the second commit has a huge diff but is a no-op, as it's basically a find and replace for `Application -> AppConnector`
stellar · Oct 16, 2024 · bcfbbe4 · bcfbbe4
2 parents f3b2ea9 + a7e7b59
commit bcfbbe4
Show file tree

Hide file tree

Showing 104 changed files with 889 additions and 651 deletions.
diff --git a/src/bucket/test/BucketListTests.cpp b/src/bucket/test/BucketListTests.cpp
@@ -726,8 +726,7 @@ TEST_CASE_VERSIONS("network config snapshots BucketList size", "[bucketlist]")
 
             uint64_t correctAverage = sum / correctWindow.size();
 
-            LedgerTxn ltx(app->getLedgerTxnRoot(), false,
-                          TransactionMode::READ_ONLY_WITHOUT_SQL_TXN);
+            LedgerTxn ltx(app->getLedgerTxnRoot());
             REQUIRE(networkConfig.getAverageBucketListSize() == correctAverage);
 
             // Check on-disk sliding window

diff --git a/src/herder/HerderImpl.cpp b/src/herder/HerderImpl.cpp
@@ -2147,11 +2147,9 @@ HerderImpl::start()
 {
     mMaxTxSize = mApp.getHerder().getMaxClassicTxSize();
     {
-        LedgerTxn ltx(mApp.getLedgerTxnRoot(),
-                      /* shouldUpdateLastModified */ true,
-                      TransactionMode::READ_ONLY_WITHOUT_SQL_TXN);
-
-        uint32_t version = ltx.loadHeader().current().ledgerVersion;
+        uint32_t version = mApp.getLedgerManager()
+                               .getLastClosedLedgerHeader()
+                               .header.ledgerVersion;
         if (protocolVersionStartsFrom(version, SOROBAN_PROTOCOL_VERSION))
         {
             auto const& conf =

diff --git a/src/herder/HerderSCPDriver.cpp b/src/herder/HerderSCPDriver.cpp
@@ -360,8 +360,6 @@ HerderSCPDriver::validateValue(uint64_t slotIndex, Value const& value,
         validateValueHelper(slotIndex, b, nomination);
     if (res != SCPDriver::kInvalidValue)
     {
-        auto const& lcl = mLedgerManager.getLastClosedLedgerHeader();
-
         LedgerUpgradeType lastUpgradeType = LEDGER_UPGRADE_VERSION;
 
         // check upgrades
@@ -419,8 +417,6 @@ HerderSCPDriver::extractValidValue(uint64_t slotIndex, Value const& value)
     if (validateValueHelper(slotIndex, b, true) ==
         SCPDriver::kFullyValidatedValue)
     {
-        auto const& lcl = mLedgerManager.getLastClosedLedgerHeader();
-
         // remove the upgrade steps we don't like
         LedgerUpgradeType thisUpgradeType;
         for (auto it = b.upgrades.begin(); it != b.upgrades.end();)

diff --git a/src/herder/TransactionQueue.cpp b/src/herder/TransactionQueue.cpp
@@ -380,7 +380,7 @@ TransactionQueue::canAdd(
             {
                 auto txResult = tx->createSuccessResult();
                 if (!tx->checkSorobanResourceAndSetError(
-                        mApp,
+                        mApp.getAppConnector(),
                         mApp.getLedgerManager()
                             .getLastClosedLedgerHeader()
                             .header.ledgerVersion,
@@ -457,8 +457,9 @@ TransactionQueue::canAdd(
             mApp.getLedgerManager().getLastClosedLedgerNum() + 1;
     }
 
-    auto txResult = tx->checkValid(
-        mApp, ls, 0, 0, getUpperBoundCloseTimeOffset(mApp, closeTime));
+    auto txResult =
+        tx->checkValid(mApp.getAppConnector(), ls, 0, 0,
+                       getUpperBoundCloseTimeOffset(mApp, closeTime));
     if (!txResult->isSuccess())
     {
         return AddResult(TransactionQueue::AddResultCode::ADD_STATUS_ERROR,

diff --git a/src/herder/TxSetFrame.cpp b/src/herder/TxSetFrame.cpp
@@ -243,7 +243,8 @@ phaseTxsAreValid(TxSetTransactions const& phase, Application& app,
         app.getLedgerManager().getLastClosedLedgerNum() + 1;
     for (auto const& tx : phase)
     {
-        auto txResult = tx->checkValid(app, ls, 0, lowerBoundCloseTimeOffset,
+        auto txResult = tx->checkValid(app.getAppConnector(), ls, 0,
+                                       lowerBoundCloseTimeOffset,
                                        upperBoundCloseTimeOffset);
         if (!txResult->isSuccess())
         {

diff --git a/src/herder/TxSetUtils.cpp b/src/herder/TxSetUtils.cpp
@@ -153,7 +153,8 @@ TxSetUtils::getInvalidTxList(TxSetTransactions const& txs, Application& app,
 
     for (auto const& tx : txs)
     {
-        auto txResult = tx->checkValid(app, ls, 0, lowerBoundCloseTimeOffset,
+        auto txResult = tx->checkValid(app.getAppConnector(), ls, 0,
+                                       lowerBoundCloseTimeOffset,
                                        upperBoundCloseTimeOffset);
         if (!txResult->isSuccess())
         {

diff --git a/src/herder/test/TxSetTests.cpp b/src/herder/test/TxSetTests.cpp
@@ -519,8 +519,7 @@ TEST_CASE("generalized tx set XDR conversion", "[txset]")
         auto txSetFrame = TxSetXDRFrame::makeFromWire(txSetXdr);
         ApplicableTxSetFrameConstPtr applicableFrame;
         {
-            LedgerTxn ltx(app->getLedgerTxnRoot(), false,
-                          TransactionMode::READ_ONLY_WITHOUT_SQL_TXN);
+            LedgerTxn ltx(app->getLedgerTxnRoot());
             applicableFrame = txSetFrame->prepareForApply(*app);
         }
         REQUIRE(applicableFrame->checkValid(*app, 0, 0));
@@ -846,7 +845,8 @@ TEST_CASE("generalized tx set fees", "[txset][soroban]")
             LedgerTxn ltx(app->getLedgerTxnRoot());
             if (validateTx)
             {
-                REQUIRE(tx->checkValidForTesting(*app, ltx, 0, 0, 0));
+                REQUIRE(tx->checkValidForTesting(app->getAppConnector(), ltx, 0,
+                                                 0, 0));
             }
             return tx;
         }

diff --git a/src/herder/test/UpgradesTests.cpp b/src/herder/test/UpgradesTests.cpp
@@ -2433,8 +2433,9 @@ TEST_CASE_VERSIONS("upgrade base reserve", "[upgrades]")
         auto submitTx = [&](TransactionTestFramePtr tx) {
             LedgerTxn ltx(app->getLedgerTxnRoot());
             TransactionMetaFrame txm(ltx.loadHeader().current().ledgerVersion);
-            REQUIRE(tx->checkValidForTesting(*app, ltx, 0, 0, 0));
-            REQUIRE(tx->apply(*app, ltx, txm));
+            REQUIRE(
+                tx->checkValidForTesting(app->getAppConnector(), ltx, 0, 0, 0));
+            REQUIRE(tx->apply(app->getAppConnector(), ltx, txm));
             ltx.commit();
 
             REQUIRE(tx->getResultCode() == txSUCCESS);

diff --git a/src/ledger/LedgerManagerImpl.cpp b/src/ledger/LedgerManagerImpl.cpp
@@ -1139,8 +1139,7 @@ LedgerManagerImpl::setLastClosedLedger(
 
     mRebuildInMemoryState = false;
     advanceLedgerPointers(lastClosed.header);
-    LedgerTxn ltx2(mApp.getLedgerTxnRoot(), false,
-                   TransactionMode::READ_ONLY_WITHOUT_SQL_TXN);
+    LedgerTxn ltx2(mApp.getLedgerTxnRoot());
     if (protocolVersionStartsFrom(ltx2.loadHeader().current().ledgerVersion,
                                   SOROBAN_PROTOCOL_VERSION))
     {
@@ -1322,12 +1321,7 @@ LedgerManagerImpl::updateNetworkConfig(AbstractLedgerTxn& rootLtx)
 {
     ZoneScoped;
 
-    uint32_t ledgerVersion{};
-    {
-        LedgerTxn ltx(rootLtx, false,
-                      TransactionMode::READ_ONLY_WITHOUT_SQL_TXN);
-        ledgerVersion = ltx.loadHeader().current().ledgerVersion;
-    }
+    uint32_t ledgerVersion = rootLtx.loadHeader().current().ledgerVersion;
 
     if (protocolVersionStartsFrom(ledgerVersion, SOROBAN_PROTOCOL_VERSION))
     {
@@ -1567,8 +1561,8 @@ LedgerManagerImpl::applyTransactions(
         }
         ++txNum;
 
-        tx->apply(mApp, ltx, tm, mutableTxResult, subSeed);
-        tx->processPostApply(mApp, ltx, tm, mutableTxResult);
+        tx->apply(mApp.getAppConnector(), ltx, tm, mutableTxResult, subSeed);
+        tx->processPostApply(mApp.getAppConnector(), ltx, tm, mutableTxResult);
         TransactionResultPair results;
         results.transactionHash = tx->getContentsHash();
         results.result = mutableTxResult->getResult();

diff --git a/src/ledger/LedgerTxn.h b/src/ledger/LedgerTxn.h
@@ -262,6 +262,8 @@ enum class LedgerTxnConsistency
     EXTRA_DELETES
 };
 
+// NOTE: Remove READ_ONLY_WITHOUT_SQL_TXN mode when BucketListDB is required
+// and we stop supporting SQL backend for ledger state.
 enum class TransactionMode
 {
     READ_ONLY_WITHOUT_SQL_TXN,

diff --git a/src/ledger/NetworkConfig.cpp b/src/ledger/NetworkConfig.cpp
@@ -1177,7 +1177,7 @@ SorobanNetworkConfig::loadFromLedger(AbstractLedgerTxn& ltxRoot,
 {
     ZoneScoped;
 
-    LedgerTxn ltx(ltxRoot, false, TransactionMode::READ_ONLY_WITHOUT_SQL_TXN);
+    LedgerTxn ltx(ltxRoot);
     loadMaxContractSize(ltx);
     loadMaxContractDataKeySize(ltx);
     loadMaxContractDataEntrySize(ltx);

diff --git a/src/main/AppConnector.cpp b/src/main/AppConnector.cpp
@@ -0,0 +1,132 @@
+#include "main/AppConnector.h"
+#include "herder/Herder.h"
+#include "invariant/InvariantManager.h"
+#include "ledger/LedgerManager.h"
+#include "ledger/LedgerTxn.h"
+#include "main/Application.h"
+#include "overlay/BanManager.h"
+#include "overlay/OverlayManager.h"
+#include "overlay/OverlayMetrics.h"
+#include "util/Timer.h"
+
+namespace stellar
+{
+
+AppConnector::AppConnector(Application& app)
+    : mApp(app), mConfig(app.getConfig())
+{
+}
+
+Herder&
+AppConnector::getHerder()
+{
+    releaseAssert(threadIsMain());
+    return mApp.getHerder();
+}
+
+LedgerManager&
+AppConnector::getLedgerManager()
+{
+    releaseAssert(threadIsMain());
+    return mApp.getLedgerManager();
+}
+
+OverlayManager&
+AppConnector::getOverlayManager()
+{
+    releaseAssert(threadIsMain());
+    return mApp.getOverlayManager();
+}
+
+BanManager&
+AppConnector::getBanManager()
+{
+    releaseAssert(threadIsMain());
+    return mApp.getBanManager();
+}
+
+SorobanNetworkConfig const&
+AppConnector::getSorobanNetworkConfig() const
+{
+    releaseAssert(threadIsMain());
+    return mApp.getLedgerManager().getSorobanNetworkConfig();
+}
+
+medida::MetricsRegistry&
+AppConnector::getMetrics() const
+{
+    releaseAssert(threadIsMain());
+    return mApp.getMetrics();
+}
+
+SorobanMetrics&
+AppConnector::getSorobanMetrics() const
+{
+    releaseAssert(threadIsMain());
+    return mApp.getLedgerManager().getSorobanMetrics();
+}
+
+void
+AppConnector::checkOnOperationApply(Operation const& operation,
+                                    OperationResult const& opres,
+                                    LedgerTxnDelta const& ltxDelta)
+{
+    releaseAssert(threadIsMain());
+    mApp.getInvariantManager().checkOnOperationApply(operation, opres,
+                                                     ltxDelta);
+}
+
+Hash const&
+AppConnector::getNetworkID() const
+{
+    releaseAssert(threadIsMain());
+    return mApp.getNetworkID();
+}
+
+void
+AppConnector::postOnMainThread(std::function<void()>&& f, std::string&& message,
+                               Scheduler::ActionType type)
+{
+    mApp.postOnMainThread(std::move(f), std::move(message), type);
+}
+
+void
+AppConnector::postOnOverlayThread(std::function<void()>&& f,
+                                  std::string const& message)
+{
+    mApp.postOnOverlayThread(std::move(f), message);
+}
+
+Config const&
+AppConnector::getConfig() const
+{
+    return mConfig;
+}
+
+bool
+AppConnector::overlayShuttingDown() const
+{
+    return mApp.getOverlayManager().isShuttingDown();
+}
+
+VirtualClock::time_point
+AppConnector::now() const
+{
+    return mApp.getClock().now();
+}
+
+bool
+AppConnector::shouldYield() const
+{
+    releaseAssert(threadIsMain());
+    return mApp.getClock().shouldYield();
+}
+
+OverlayMetrics&
+AppConnector::getOverlayMetrics()
+{
+    // OverlayMetrics class is thread-safe
+    return mApp.getOverlayManager().getOverlayMetrics();
+}
+
+}
diff --git a/src/overlay/OverlayAppConnector.h → src/main/AppConnector.h b/src/overlay/OverlayAppConnector.h → src/main/AppConnector.h
@@ -1,6 +1,7 @@
 #pragma once
 
 #include "main/Config.h"
+#include "medida/metrics_registry.h"
 
 namespace stellar
 {
@@ -10,25 +11,35 @@ class LedgerManager;
 class Herder;
 class BanManager;
 struct OverlayMetrics;
+class SorobanNetworkConfig;
+class SorobanMetrics;
+struct LedgerTxnDelta;
 
 // Helper class to isolate access to Application; all function helpers must
 // either be called from main or be thread-sade
-class OverlayAppConnector
+class AppConnector
 {
     Application& mApp;
     // Copy config for threads to use, and avoid warnings from thread sanitizer
     // about accessing mApp
     Config const mConfig;
 
   public:
-    OverlayAppConnector(Application& app);
+    AppConnector(Application& app);
 
     // Methods that can only be called from main thread
     Herder& getHerder();
     LedgerManager& getLedgerManager();
     OverlayManager& getOverlayManager();
     BanManager& getBanManager();
     bool shouldYield() const;
+    SorobanNetworkConfig const& getSorobanNetworkConfig() const;
+    medida::MetricsRegistry& getMetrics() const;
+    SorobanMetrics& getSorobanMetrics() const;
+    void checkOnOperationApply(Operation const& operation,
+                               OperationResult const& opres,
+                               LedgerTxnDelta const& ltxDelta);
+    Hash const& getNetworkID() const;
 
     // Thread-safe methods
     void postOnMainThread(

diff --git a/src/main/Application.h b/src/main/Application.h
@@ -46,6 +46,7 @@ class AbstractLedgerTxnParent;
 class BasicWork;
 enum class LoadGenMode;
 struct GeneratedLoadConfig;
+class AppConnector;
 
 #ifdef BUILD_TESTS
 class LoadGenerator;
@@ -326,6 +327,8 @@ class Application
     // (while preserving the overlay data).
     virtual void resetDBForInMemoryMode() = 0;
 
+    virtual AppConnector& getAppConnector() = 0;
+
   protected:
     Application()
     {

diff --git a/src/main/ApplicationImpl.cpp b/src/main/ApplicationImpl.cpp
@@ -40,6 +40,7 @@
 #include "ledger/LedgerHeaderUtils.h"
 #include "ledger/LedgerManager.h"
 #include "ledger/LedgerTxn.h"
+#include "main/AppConnector.h"
 #include "main/ApplicationUtils.h"
 #include "main/CommandHandler.h"
 #include "main/ExternalQueue.h"
@@ -330,6 +331,7 @@ ApplicationImpl::initialize(bool createNewDB, bool forceRebuild)
     mWorkScheduler = WorkScheduler::create(*this);
     mBanManager = BanManager::create(*this);
     mStatusManager = std::make_unique<StatusManager>();
+    mAppConnector = std::make_unique<AppConnector>(*this);
 
     if (getConfig().MODE_USES_IN_MEMORY_LEDGER)
     {
@@ -1635,4 +1637,10 @@ ApplicationImpl::getLedgerTxnRoot()
     return mConfig.MODE_USES_IN_MEMORY_LEDGER ? *mNeverCommittingLedgerTxn
                                               : *mLedgerTxnRoot;
 }
+
+AppConnector&
+ApplicationImpl::getAppConnector()
+{
+    return *mAppConnector;
+}
 }