Last few bits of cleanup, fix reference bug

src/bucket/BucketSnapshotManager.cpp

@@ -52,44 +52,44 @@ BucketSnapshotManager::BucketSnapshotManager(
     }
 }
 
-SearchableSnapshotConstPtr
-BucketSnapshotManager::copySearchableLiveBucketListSnapshot() const
+template <class BucketT>
+std::map<uint32_t, SnapshotPtrT<BucketT>>
+copyHistoricalSnapshots(
+    std::map<uint32_t, SnapshotPtrT<BucketT>> const& snapshots)
 {
-    std::map<uint32_t, SnapshotPtrT<LiveBucket>> historicalSnapshots;
-    for (auto const& [ledgerSeq, snap] : mLiveHistoricalSnapshots)
+    std::map<uint32_t, SnapshotPtrT<BucketT>> copiedSnapshots;
+    for (auto const& [ledgerSeq, snap] : snapshots)
     {
-        historicalSnapshots.emplace(
+        copiedSnapshots.emplace(
             ledgerSeq,
-            std::make_unique<BucketListSnapshot<LiveBucket> const>(*snap));
+            std::make_unique<BucketListSnapshot<BucketT> const>(*snap));
     }
+    return copiedSnapshots;
+}
+
+SearchableSnapshotConstPtr
+BucketSnapshotManager::copySearchableLiveBucketListSnapshot() const
+{
     // Can't use std::make_shared due to private constructor
     return std::shared_ptr<SearchableLiveBucketListSnapshot>(
         new SearchableLiveBucketListSnapshot(
             *this,
             std::make_unique<BucketListSnapshot<LiveBucket>>(
                 *mCurrLiveSnapshot),
-            std::move(historicalSnapshots)));
+            copyHistoricalSnapshots(mLiveHistoricalSnapshots)));
 }
 
-std::shared_ptr<SearchableHotArchiveBucketListSnapshot const>
+SearchableHotArchiveSnapshotConstPtr
 BucketSnapshotManager::copySearchableHotArchiveBucketListSnapshot() const
 {
     releaseAssert(mCurrHotArchiveSnapshot);
-    std::map<uint32_t, SnapshotPtrT<HotArchiveBucket>> historicalSnapshots;
-    for (auto const& [ledgerSeq, snap] : mHotArchiveHistoricalSnapshots)
-    {
-        historicalSnapshots.emplace(
-            ledgerSeq,
-            std::make_unique<BucketListSnapshot<HotArchiveBucket> const>(
-                *snap));
-    }
     // Can't use std::make_shared due to private constructor
     return std::shared_ptr<SearchableHotArchiveBucketListSnapshot>(
         new SearchableHotArchiveBucketListSnapshot(
             *this,
             std::make_unique<BucketListSnapshot<HotArchiveBucket>>(
                 *mCurrHotArchiveSnapshot),
-            std::move(historicalSnapshots)));
+            copyHistoricalSnapshots(mHotArchiveHistoricalSnapshots)));
 }
 
 medida::Timer&
@@ -132,6 +132,22 @@ BucketSnapshotManager::maybeCopySearchableBucketListSnapshot(
     }
 }
 
+void
+BucketSnapshotManager::maybeCopySearchableHotArchiveBucketListSnapshot(
+    SearchableHotArchiveSnapshotConstPtr& snapshot)
+{
+    // The canonical snapshot held by the BucketSnapshotManager is not being
+    // modified. Rather, a thread is checking it's copy against the canonical
+    // snapshot, so use a shared lock.
+    std::shared_lock<std::shared_mutex> lock(mSnapshotMutex);
+
+    if (!snapshot ||
+        snapshot->getLedgerSeq() < mCurrHotArchiveSnapshot->getLedgerSeq())
+    {
+        snapshot = copySearchableHotArchiveBucketListSnapshot();
+    }
+}
+
 void
 BucketSnapshotManager::updateCurrentSnapshot(
     SnapshotPtrT<LiveBucket>&& liveSnapshot,

src/bucket/BucketSnapshotManager.h

@@ -34,6 +34,8 @@ template <class BucketT>
 using SnapshotPtrT = std::unique_ptr<BucketListSnapshot<BucketT> const>;
 using SearchableSnapshotConstPtr =
     std::shared_ptr<SearchableLiveBucketListSnapshot const>;
+using SearchableHotArchiveSnapshotConstPtr =
+    std::shared_ptr<SearchableHotArchiveBucketListSnapshot const>;
 
 // This class serves as the boundary between non-threadsafe singleton classes
 // (BucketManager, BucketList, Metrics, etc) and threadsafe, parallel BucketList
@@ -82,13 +84,20 @@ class BucketSnapshotManager : NonMovableOrCopyable
                           SnapshotPtrT<HotArchiveBucket>&& hotArchiveSnapshot,
                           uint32_t numHistoricalLedgers);
 
+    // Copy the most recent snapshot for the live bucket list
     SearchableSnapshotConstPtr copySearchableLiveBucketListSnapshot() const;
 
-    std::shared_ptr<SearchableHotArchiveBucketListSnapshot const>
+    // Copy the most recent snapshot for the hot archive bucket list
+    SearchableHotArchiveSnapshotConstPtr
     copySearchableHotArchiveBucketListSnapshot() const;
 
+    // `maybeCopy` interface refreshes `snapshot` if a newer snapshot is
+    // available. It's a no-op otherwise. This is useful to avoid unnecessary
+    // copying.
     void
     maybeCopySearchableBucketListSnapshot(SearchableSnapshotConstPtr& snapshot);
+    void maybeCopySearchableHotArchiveBucketListSnapshot(
+        SearchableHotArchiveSnapshotConstPtr& snapshot);
 
     // All metric recording functions must only be called by the main thread
     void startPointLoadTimer() const;

src/bucket/SearchableBucketList.h

@@ -53,7 +53,7 @@ class SearchableHotArchiveBucketListSnapshot
     std::vector<HotArchiveBucketEntry>
     loadKeys(std::set<LedgerKey, LedgerEntryIdCmp> const& inKeys) const;
 
-    friend std::shared_ptr<SearchableHotArchiveBucketListSnapshot const>
+    friend SearchableHotArchiveSnapshotConstPtr
     BucketSnapshotManager::copySearchableHotArchiveBucketListSnapshot() const;
 };
 }

src/bucket/test/BucketIndexTests.cpp

@@ -803,19 +803,19 @@ TEST_CASE("hot archive bucket lookups", "[bucket][bucketindex][archive]")
             BucketBase::FIRST_PROTOCOL_SUPPORTING_PERSISTENT_EVICTION);
         addHotArchiveBatchAndUpdateSnapshot(*app, header, archivedEntries,
                                             restoredEntries, deletedEntries);
-        searchableBL = app->getBucketManager()
-                           .getBucketSnapshotManager()
-                           .copySearchableHotArchiveBucketListSnapshot();
+        app->getBucketManager()
+            .getBucketSnapshotManager()
+            .maybeCopySearchableHotArchiveBucketListSnapshot(searchableBL);
         checkResult();
 
         // Add a few batches so that entries are no longer in the top bucket
         for (auto i = 0; i < 100; ++i)
         {
             header.ledgerSeq += 1;
             addHotArchiveBatchAndUpdateSnapshot(*app, header, {}, {}, {});
-            searchableBL = app->getBucketManager()
-                               .getBucketSnapshotManager()
-                               .copySearchableHotArchiveBucketListSnapshot();
+            app->getBucketManager()
+                .getBucketSnapshotManager()
+                .maybeCopySearchableHotArchiveBucketListSnapshot(searchableBL);
         }
 
         // Shadow entries via liveEntry
@@ -825,9 +825,9 @@ TEST_CASE("hot archive bucket lookups", "[bucket][bucketindex][archive]")
         header.ledgerSeq += 1;
         addHotArchiveBatchAndUpdateSnapshot(*app, header, {},
                                             {liveShadow1, liveShadow2}, {});
-        searchableBL = app->getBucketManager()
-                           .getBucketSnapshotManager()
-                           .copySearchableHotArchiveBucketListSnapshot();
+        app->getBucketManager()
+            .getBucketSnapshotManager()
+            .maybeCopySearchableHotArchiveBucketListSnapshot(searchableBL);
 
         // Point load
         for (auto const& k : {liveShadow1, liveShadow2})
@@ -847,9 +847,10 @@ TEST_CASE("hot archive bucket lookups", "[bucket][bucketindex][archive]")
         header.ledgerSeq += 1;
         addHotArchiveBatchAndUpdateSnapshot(*app, header, {}, {},
                                             {deletedShadow});
-        searchableBL = app->getBucketManager()
-                           .getBucketSnapshotManager()
-                           .copySearchableHotArchiveBucketListSnapshot();
+        app->getBucketManager()
+            .getBucketSnapshotManager()
+            .maybeCopySearchableHotArchiveBucketListSnapshot(searchableBL);
+
         // Point load
         auto entryPtr = searchableBL->load(deletedShadow);
         REQUIRE(entryPtr);
@@ -870,9 +871,10 @@ TEST_CASE("hot archive bucket lookups", "[bucket][bucketindex][archive]")
         header.ledgerSeq += 1;
         addHotArchiveBatchAndUpdateSnapshot(*app, header, {archivedShadow}, {},
                                             {});
-        searchableBL = app->getBucketManager()
-                           .getBucketSnapshotManager()
-                           .copySearchableHotArchiveBucketListSnapshot();
+        app->getBucketManager()
+            .getBucketSnapshotManager()
+            .maybeCopySearchableHotArchiveBucketListSnapshot(searchableBL);
+
         // Point load
         entryPtr = searchableBL->load(LedgerEntryKey(archivedShadow));
         REQUIRE(entryPtr);

src/bucket/test/BucketListTests.cpp

@@ -868,7 +868,8 @@ TEST_CASE_VERSIONS("network config snapshots BucketList size", "[bucketlist]")
     for_versions_from(20, *app, [&] {
         LedgerManagerForBucketTests& lm = app->getLedgerManager();
 
-        auto& networkConfig = app->getLedgerManager().getSorobanNetworkConfig();
+        auto& networkConfig =
+            app->getLedgerManager().getSorobanNetworkConfigReadOnly();
 
         uint32_t windowSize = networkConfig.stateArchivalSettings()
                                   .bucketListSizeWindowSampleSize;

src/herder/HerderImpl.cpp

@@ -2101,7 +2101,8 @@ HerderImpl::maybeHandleUpgrade()
             // no-op on any earlier protocol
             return;
         }
-        auto const& conf = mApp.getLedgerManager().getSorobanNetworkConfig();
+        auto const& conf =
+            mApp.getLedgerManager().getSorobanNetworkConfigReadOnly();
 
         auto maybeNewMaxTxSize =
             conf.txMaxSizeBytes() + getFlowControlExtraBuffer();
@@ -2153,7 +2154,7 @@ HerderImpl::start()
         if (protocolVersionStartsFrom(version, SOROBAN_PROTOCOL_VERSION))
         {
             auto const& conf =
-                mApp.getLedgerManager().getSorobanNetworkConfig();
+                mApp.getLedgerManager().getSorobanNetworkConfigReadOnly();
             mMaxTxSize = std::max(mMaxTxSize, conf.txMaxSizeBytes() +
                                                   getFlowControlExtraBuffer());
         }

src/herder/TransactionQueue.cpp

@@ -381,7 +381,8 @@ TransactionQueue::canAdd(
                 auto txResult = tx->createSuccessResult();
                 if (!tx->checkSorobanResourceAndSetError(
                         mApp.getAppConnector(),
-                        mApp.getLedgerManager().getSorobanNetworkConfig(),
+                        mApp.getLedgerManager()
+                            .getSorobanNetworkConfigReadOnly(),
                         mApp.getLedgerManager()
                             .getLastClosedLedgerHeader()
                             .header.ledgerVersion,

src/herder/test/HerderTests.cpp

@@ -1007,7 +1007,8 @@ TEST_CASE("tx set hits overlay byte limit during construction",
         cfg.mLedgerMaxInstructions = max;
     });
 
-    auto const& conf = app->getLedgerManager().getSorobanNetworkConfig();
+    auto const& conf =
+        app->getLedgerManager().getSorobanNetworkConfigReadOnly();
     uint32_t maxContractSize = 0;
     maxContractSize = conf.maxContractSizeBytes();
 
@@ -1162,7 +1163,7 @@ TEST_CASE("surge pricing", "[herder][txset][soroban]")
         auto tx = makeMultiPayment(acc1, root, 1, 100, 0, 1);
 
         SorobanNetworkConfig conf =
-            app->getLedgerManager().getSorobanNetworkConfig();
+            app->getLedgerManager().getSorobanNetworkConfigReadOnly();
 
         uint32_t const baseFee = 10'000'000;
         SorobanResources resources;
@@ -3361,7 +3362,7 @@ TEST_CASE("soroban txs accepted by the network",
             for (auto node : nodes)
             {
                 REQUIRE(node->getLedgerManager()
-                            .getSorobanNetworkConfig()
+                            .getSorobanNetworkConfigReadOnly()
                             .ledgerMaxTxCount() == ledgerWideLimit * 10);
             }
 

src/herder/test/TransactionQueueTests.cpp

@@ -1259,7 +1259,7 @@ TEST_CASE("Soroban TransactionQueue limits",
     auto account2 = root.create("a2", minBalance2);
 
     SorobanNetworkConfig conf =
-        app->getLedgerManager().getSorobanNetworkConfig();
+        app->getLedgerManager().getSorobanNetworkConfigReadOnly();
 
     SorobanResources resources;
     resources.instructions = 2'000'000;

src/herder/test/TxSetTests.cpp

@@ -1087,7 +1087,7 @@ TEST_CASE("txset nomination", "[txset]")
             });
 
             auto const& sorobanConfig =
-                app->getLedgerManager().getSorobanNetworkConfig();
+                app->getLedgerManager().getSorobanNetworkConfigReadOnly();
             stellar::uniform_int_distribution<> txReadEntriesDistr(
                 1, sorobanConfig.txMaxReadLedgerEntries());
 

src/herder/test/UpgradesTests.cpp

@@ -251,7 +251,7 @@ makeBucketListSizeWindowSampleSizeTestUpgrade(Application& app,
 {
     // Modify window size
     auto sas = app.getLedgerManager()
-                   .getSorobanNetworkConfig()
+                   .getSorobanNetworkConfigReadOnly()
                    .stateArchivalSettings();
     sas.bucketListSizeWindowSampleSize = newWindowSize;
 
@@ -837,7 +837,7 @@ TEST_CASE("config upgrades applied to ledger", "[soroban][upgrades]")
     executeUpgrade(*app, makeProtocolVersionUpgrade(
                              static_cast<uint32_t>(SOROBAN_PROTOCOL_VERSION)));
     auto const& sorobanConfig =
-        app->getLedgerManager().getSorobanNetworkConfig();
+        app->getLedgerManager().getSorobanNetworkConfigReadOnly();
     SECTION("unknown config upgrade set is ignored")
     {
         auto contractID = autocheck::generator<Hash>()(5);
@@ -905,7 +905,7 @@ TEST_CASE("config upgrades applied to ledger", "[soroban][upgrades]")
             auto const newSize = 20;
             populateValuesAndUpgradeSize(newSize);
             auto const& cfg2 =
-                app->getLedgerManager().getSorobanNetworkConfig();
+                app->getLedgerManager().getSorobanNetworkConfigReadOnly();
 
             // Verify that we popped the 10 oldest values
             auto sum = 0;
@@ -927,7 +927,7 @@ TEST_CASE("config upgrades applied to ledger", "[soroban][upgrades]")
             auto const newSize = 40;
             populateValuesAndUpgradeSize(newSize);
             auto const& cfg2 =
-                app->getLedgerManager().getSorobanNetworkConfig();
+                app->getLedgerManager().getSorobanNetworkConfigReadOnly();
 
             // Verify that we backfill 10 copies of the oldest value
             auto sum = 0;
@@ -957,7 +957,7 @@ TEST_CASE("config upgrades applied to ledger", "[soroban][upgrades]")
                 LedgerTxn ltx2(app->getLedgerTxnRoot());
 
                 auto const& cfg =
-                    app->getLedgerManager().getSorobanNetworkConfig();
+                    app->getLedgerManager().getSorobanNetworkConfigReadOnly();
                 initialSize =
                     cfg.mStateArchivalSettings.bucketListSizeWindowSampleSize;
                 initialWindow = cfg.mBucketListSizeSnapshots;
@@ -972,7 +972,8 @@ TEST_CASE("config upgrades applied to ledger", "[soroban][upgrades]")
             REQUIRE(configUpgradeSet);
             executeUpgrade(*app, makeConfigUpgrade(*configUpgradeSet));
 
-            auto const& cfg = app->getLedgerManager().getSorobanNetworkConfig();
+            auto const& cfg =
+                app->getLedgerManager().getSorobanNetworkConfigReadOnly();
             REQUIRE(cfg.mStateArchivalSettings.bucketListSizeWindowSampleSize ==
                     initialSize);
             REQUIRE(cfg.mBucketListSizeSnapshots == initialWindow);
@@ -1086,7 +1087,7 @@ TEST_CASE("Soroban max tx set size upgrade applied to ledger",
                              static_cast<uint32_t>(SOROBAN_PROTOCOL_VERSION)));
 
     auto const& sorobanConfig =
-        app->getLedgerManager().getSorobanNetworkConfig();
+        app->getLedgerManager().getSorobanNetworkConfigReadOnly();
 
     executeUpgrade(*app, makeMaxSorobanTxSizeUpgrade(123));
     REQUIRE(sorobanConfig.ledgerMaxTxCount() == 123);
@@ -2245,7 +2246,8 @@ TEST_CASE("configuration initialized in version upgrade", "[upgrades]")
             InitialSorobanNetworkConfig::MAX_CONTRACT_SIZE);
 
     // Check that BucketList size window initialized with current BL size
-    auto& networkConfig = app->getLedgerManager().getSorobanNetworkConfig();
+    auto& networkConfig =
+        app->getLedgerManager().getSorobanNetworkConfigReadOnly();
     REQUIRE(networkConfig.getAverageBucketListSize() == blSize);
 
     // Check in memory window

src/ledger/LedgerManager.h

@@ -131,7 +131,7 @@ class LedgerManager
     // 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;
+    virtual SorobanNetworkConfig const& getSorobanNetworkConfigReadOnly() = 0;
     virtual SorobanNetworkConfig const& getSorobanNetworkConfigForApply() = 0;
 
     virtual bool hasSorobanNetworkConfig() const = 0;

src/ledger/LedgerManagerImpl.cpp

@@ -453,7 +453,7 @@ LedgerManagerImpl::maxLedgerResources(bool isSoroban)
 
     if (isSoroban)
     {
-        auto conf = getSorobanNetworkConfig();
+        auto conf = getSorobanNetworkConfigReadOnly();
         std::vector<int64_t> limits = {conf.ledgerMaxTxCount(),
                                        conf.ledgerMaxInstructions(),
                                        conf.ledgerMaxTransactionSizesBytes(),
@@ -475,7 +475,8 @@ LedgerManagerImpl::maxSorobanTransactionResources()
 {
     ZoneScoped;
 
-    auto const& conf = mApp.getLedgerManager().getSorobanNetworkConfig();
+    auto const& conf =
+        mApp.getLedgerManager().getSorobanNetworkConfigReadOnly();
     int64_t const opCount = 1;
     std::vector<int64_t> limits = {opCount,
                                    conf.txMaxInstructions(),
@@ -540,7 +541,7 @@ LedgerManagerImpl::getLastClosedLedgerNum() const
 }
 
 SorobanNetworkConfig const&
-LedgerManagerImpl::getSorobanNetworkConfig()
+LedgerManagerImpl::getSorobanNetworkConfigReadOnly()
 {
     releaseAssert(threadIsMain());
     releaseAssert(hasSorobanNetworkConfig());
@@ -1759,7 +1760,7 @@ LedgerManagerImpl::ledgerClosed(
         protocolVersionStartsFrom(initialLedgerVers, SOROBAN_PROTOCOL_VERSION))
     {
         ledgerCloseMeta->setNetworkConfiguration(
-            getSorobanNetworkConfig(),
+            getSorobanNetworkConfigReadOnly(),
             mApp.getConfig().EMIT_LEDGER_CLOSE_META_EXT_V1);
     }