Some cleanup post-rebase, refresh comments

Author: marta-lokhova

src/bucket/BucketManager.cpp

@@ -18,6 +18,7 @@
 #include "ledger/LedgerManager.h"
 #include "ledger/LedgerTxn.h"
 #include "ledger/LedgerTypeUtils.h"
+#include "ledger/NetworkConfig.h"
 #include "main/Application.h"
 #include "main/Config.h"
 #include "util/Fs.h"
@@ -1080,7 +1081,7 @@ EvictedStateVectors
 BucketManager::resolveBackgroundEvictionScan(
     AbstractLedgerTxn& ltx, uint32_t ledgerSeq,
     LedgerKeySet const& modifiedKeys, uint32_t ledgerVers,
-    SorobanNetworkConfig& networkConfig)
+    SorobanNetworkConfig const& networkConfig)
 {
     ZoneScoped;
     releaseAssert(mEvictionStatistics);

src/bucket/BucketManager.h

@@ -36,6 +36,7 @@ class BucketSnapshotManager;
 class SearchableLiveBucketListSnapshot;
 struct BucketEntryCounters;
 enum class LedgerEntryTypeAndDurability : uint32_t;
+class SorobanNetworkConfig;
 
 struct HistoryArchiveState;
 
@@ -307,7 +308,7 @@ class BucketManager : NonMovableOrCopyable
     resolveBackgroundEvictionScan(AbstractLedgerTxn& ltx, uint32_t ledgerSeq,
                                   LedgerKeySet const& modifiedKeys,
                                   uint32_t ledgerVers,
-                                  SorobanNetworkConfig& networkConfig);
+                                  SorobanNetworkConfig const& networkConfig);
 
     medida::Meter& getBloomMissMeter() const;
     medida::Meter& getBloomLookupMeter() const;

src/bucket/LiveBucketList.h

@@ -9,9 +9,6 @@
 
 namespace stellar
 {
-
-class SorobanNetworkConfig;
-
 // The LiveBucketList stores the current canonical state of the ledger. It is
 // made up of LiveBucket buckets, which in turn store individual entries of type
 // BucketEntry. When an entry is "evicted" from the ledger, it is removed from

src/bucket/test/BucketListTests.cpp

@@ -869,7 +869,7 @@ TEST_CASE_VERSIONS("network config snapshots BucketList size", "[bucketlist]")
         LedgerManagerForBucketTests& lm = app->getLedgerManager();
 
         auto& networkConfig =
-            app->getLedgerManager().getMutableSorobanNetworkConfig();
+            app->getLedgerManager().getSorobanNetworkConfigReadOnly();
 
         uint32_t windowSize = networkConfig.stateArchivalSettings()
                                   .bucketListSizeWindowSampleSize;

src/bucket/test/BucketTestUtils.cpp

@@ -101,10 +101,6 @@ closeLedger(Application& app, std::optional<SecretKey> skToSignValue,
     app.getHerder().externalizeValue(TxSetXDRFrame::makeEmpty(lcl), ledgerNum,
                                      lcl.header.scpValue.closeTime, upgrades,
                                      skToSignValue);
-    testutil::crankUntil(
-        app,
-        [&lm, ledgerNum]() { return lm.getLastClosedLedgerNum() == ledgerNum; },
-        std::chrono::seconds(10));
     return lm.getLastClosedLedgerHeader().hash;
 }
 
@@ -238,7 +234,7 @@ LedgerManagerForBucketTests::transferLedgerEntriesToBucketList(
                     mApp.getBucketManager().resolveBackgroundEvictionScan(
                         ltxEvictions, lh.ledgerSeq, keys, initialLedgerVers,
                         mApp.getLedgerManager()
-                            .getMutableSorobanNetworkConfig());
+                            .getSorobanNetworkConfigForApply());
 
                 if (protocolVersionStartsFrom(
                         initialLedgerVers,

src/catchup/CatchupWork.cpp

@@ -524,7 +524,7 @@ CatchupWork::runCatchupStep()
                 // In this case we should actually have been caught-up during
                 // the replay process and, if judged successful, our LCL should
                 // be the one provided as well.
-                auto lastClosed =
+                auto& lastClosed =
                     mApp.getLedgerManager().getLastClosedLedgerHeader();
                 releaseAssert(mLastApplied.hash == lastClosed.hash);
                 releaseAssert(mLastApplied.header == lastClosed.header);

src/database/Database.h

@@ -141,6 +141,8 @@ class Database : NonMovableOrCopyable
     // Save `vers` as schema version.
     void putSchemaVersion(unsigned long vers);
 
+    // Prepared statements cache may be accessed by mutliple threads (each using
+    // a different session), so use a mutex to synchronize access.
     std::mutex mutable mStatementsMutex;
 
   public:

src/herder/HerderImpl.cpp

@@ -480,6 +480,15 @@ HerderImpl::valueExternalized(uint64 slotIndex, StellarValue const& value,
 
         // Check to see if quorums have changed and we need to reanalyze.
         checkAndMaybeReanalyzeQuorumMap();
+
+        // heart beat *after* doing all the work (ensures that we do not include
+        // the overhead of externalization in the way we track SCP)
+        // Note: this only makes sense in the context of synchronous ledger
+        // application on the main thread.
+        if (!mApp.getConfig().parallelLedgerClose())
+        {
+            trackingHeartBeat();
+        }
     }
     else
     {
@@ -1156,7 +1165,7 @@ HerderImpl::lastClosedLedgerIncreased(bool latest, TxSetXDRFrameConstPtr txSet)
     {
         // Re-start heartbeat tracking _after_ applying the most up-to-date
         // ledger. This guarantees out-of-sync timer won't fire while we have
-        // ledgers to apply.
+        // ledgers to apply (applicable during parallel ledger close).
         trackingHeartBeat();
 
         // Ensure out of sync recovery did not get triggered while we were
@@ -1369,6 +1378,8 @@ HerderImpl::triggerNextLedger(uint32_t ledgerSeqToTrigger,
     // If applying, the next ledger will trigger voting
     if (mLedgerManager.isApplying())
     {
+        // This can only happen when closing ledgers in parallel
+        releaseAssert(mApp.getConfig().parallelLedgerClose());
         CLOG_DEBUG(Herder, "triggerNextLedger: skipping (applying) : {}",
                    mApp.getStateHuman());
         return;
@@ -1560,7 +1571,7 @@ HerderImpl::getUpgradesJson()
 void
 HerderImpl::forceSCPStateIntoSyncWithLastClosedLedger()
 {
-    auto header = mLedgerManager.getLastClosedLedgerHeader().header;
+    auto const& header = mLedgerManager.getLastClosedLedgerHeader().header;
     setTrackingSCPState(header.ledgerSeq, header.scpValue,
                         /* isTrackingNetwork */ true);
 }
@@ -2360,7 +2371,7 @@ HerderImpl::herderOutOfSync()
     // are no ledgers queued to be applied. If there are ledgers queued, it's
     // possible the rest of the network is waiting for this node to vote. In
     // this case we should _still_ remain in tracking and emit nomination; If
-    // the nodes does not hear anything from the network after that, then node
+    // the node does not hear anything from the network after that, then node
     // can go into out of sync recovery.
     releaseAssert(threadIsMain());
     releaseAssert(!mLedgerManager.isApplying());

src/herder/HerderPersistenceImpl.cpp

@@ -236,9 +236,8 @@ HerderPersistence::copySCPHistoryToStream(Database& db, soci::session& sess,
                                           XDROutputFileStream& scpHistory)
 {
     ZoneScoped;
-    // TODO: this may conflict with main thread, as this is done in the
-    // background (this is the case in master today, so can be fixed
-    // later).
+    // Subtle: changing these queries may cause conflicts with main thread
+    // https://github.com/stellar/stellar-core/issues/4589
 
     uint32_t begin = ledgerSeq, end = ledgerSeq + ledgerCount;
     size_t n = 0;

src/herder/HerderSCPDriver.cpp

@@ -221,15 +221,15 @@ HerderSCPDriver::validateValueHelper(uint64_t slotIndex, StellarValue const& b,
         }
     }
 
-    auto lhhe = mLedgerManager.getLastClosedLedgerHeader();
+    auto const& lcl = mLedgerManager.getLastClosedLedgerHeader();
     // when checking close time, start with what we have locally
-    lastCloseTime = lhhe.header.scpValue.closeTime;
+    lastCloseTime = lcl.header.scpValue.closeTime;
 
     // if this value is not for our local state,
     // perform as many checks as we can
-    if (slotIndex != (lhhe.header.ledgerSeq + 1))
+    if (slotIndex != (lcl.header.ledgerSeq + 1))
     {
-        if (slotIndex == lhhe.header.ledgerSeq)
+        if (slotIndex == lcl.header.ledgerSeq)
         {
             // previous ledger
             if (b.closeTime != lastCloseTime)
@@ -240,7 +240,7 @@ HerderSCPDriver::validateValueHelper(uint64_t slotIndex, StellarValue const& b,
                 return SCPDriver::kInvalidValue;
             }
         }
-        else if (slotIndex < lhhe.header.ledgerSeq)
+        else if (slotIndex < lcl.header.ledgerSeq)
         {
             // basic sanity check on older value
             if (b.closeTime >= lastCloseTime)
@@ -323,7 +323,7 @@ HerderSCPDriver::validateValueHelper(uint64_t slotIndex, StellarValue const& b,
 
         res = SCPDriver::kInvalidValue;
     }
-    else if (!checkAndCacheTxSetValid(*txSet, lhhe, closeTimeOffset))
+    else if (!checkAndCacheTxSetValid(*txSet, lcl, closeTimeOffset))
     {
         CLOG_DEBUG(Herder,
                    "HerderSCPDriver::validateValue i: {} invalid txSet {}",
@@ -614,6 +614,7 @@ HerderSCPDriver::combineCandidates(uint64_t slotIndex,
     std::set<TransactionFramePtr> aggSet;
 
     releaseAssert(!mLedgerManager.isApplying());
+    releaseAssert(threadIsMain());
     auto const& lcl = mLedgerManager.getLastClosedLedgerHeader();
 
     Hash candidatesHash;

src/herder/TxSetFrame.cpp

@@ -528,7 +528,7 @@ applySurgePricing(TxSetPhase phase, TxFrameList const& txs, Application& app)
     ZoneScoped;
     releaseAssert(threadIsMain());
     releaseAssert(!app.getLedgerManager().isApplying());
-    
+
     auto const& lclHeader =
         app.getLedgerManager().getLastClosedLedgerHeader().header;
     std::vector<bool> hadTxNotFittingLane;

src/history/test/HistoryTestsUtils.cpp

@@ -947,8 +947,6 @@ CatchupSimulation::externalizeLedger(HerderImpl& herder, uint32_t ledger)
                                           lcd.getLedgerSeq(), lcd.getTxSet());
     herder.getHerderSCPDriver().valueExternalized(
         lcd.getLedgerSeq(), xdr::xdr_to_opaque(lcd.getValue()));
-
-    // TODO: crank the clock
 }
 
 void

src/invariant/BucketListIsConsistentWithDatabase.cpp

@@ -7,7 +7,6 @@
 #include "bucket/BucketManager.h"
 #include "bucket/LiveBucket.h"
 #include "bucket/LiveBucketList.h"
-#include "crypto/Hex.h"
 #include "database/Database.h"
 #include "history/HistoryArchive.h"
 #include "invariant/InvariantManager.h"