Skip to content

Commit 44ec33b

Browse files
graydongraydon
authored
Rename various close things to apply, add comment. (#4647)
This does a few things: 1. Adds a big block comment to LedgerManager.h explaining the flow of ledgers through LedgerApplyManager and LedgerManager in more detail, as well as showing/describing the threading behaviour (assuming parallel apply). 2. Renames various bits named "close" to "apply" when they are things-done-by-the-apply-thread or variables-touched-by-the-apply-thread, in order to more clearly differentiate "close" stuff (on main thread, tracking LCL) from "apply" stuff (on apply thread, possibly ahead of LCL). 3. Partitions state between two structs in the LM, one for apply-state and one for LCL state. 4. Recycles the output-from-the-apply-phase struct to _store_ the LCL state, since they had the same content. 5. Moves the ledger metrics into a sub-struct just so they're a little less cluttered. 6. Renames a few of the later-stages-of-apply/close from generic terms like `ledgerApplied` to more specific terms like `sealLedgerTxnAndStoreInBucketsAndDB`. It's a mouthful but you at least don't have to go re-read the function body each time you see a call to remember what it does. The change is _nearly_ a no-op. Functionality-wise I only added two asserts that ought to (and appear to) always be true.
2 parents 36d768f + 02cf4ce commit 44ec33b

36 files changed

+497
-355
lines changed

‎src/bucket/test/BucketListTests.cpp

+3-2
Original file line numberDiff line numberDiff line change
@@ -869,7 +869,7 @@ TEST_CASE_VERSIONS("network config snapshots BucketList size", "[bucketlist]")
869869
LedgerManagerForBucketTests& lm = app->getLedgerManager();
870870

871871
auto& networkConfig =
872-
app->getLedgerManager().getSorobanNetworkConfigReadOnly();
872+
app->getLedgerManager().getLastClosedSorobanNetworkConfig();
873873

874874
uint32_t windowSize = networkConfig.stateArchivalSettings()
875875
.bucketListSizeWindowSampleSize;
@@ -967,7 +967,8 @@ TEST_CASE_VERSIONS("eviction scan", "[bucketlist][archival]")
967967

968968
auto& networkCfg = [&]() -> SorobanNetworkConfig& {
969969
LedgerTxn ltx(app->getLedgerTxnRoot());
970-
return app->getLedgerManager().getMutableSorobanNetworkConfig();
970+
return app->getLedgerManager()
971+
.getMutableSorobanNetworkConfigForApply();
971972
}();
972973

973974
auto& stateArchivalSettings = networkCfg.stateArchivalSettings();

‎src/bucket/test/BucketTestUtils.cpp

+8-3
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,11 @@ closeLedger(Application& app, std::optional<SecretKey> skToSignValue,
101101
app.getHerder().externalizeValue(TxSetXDRFrame::makeEmpty(lcl), ledgerNum,
102102
lcl.header.scpValue.closeTime, upgrades,
103103
skToSignValue);
104+
// NB: this assert will probably stop being true when background apply is
105+
// turned on by default: externalize will have handed the ledger off to
106+
// apply but not yet received the results of apply or updated LCL. The fix
107+
// should be just to crank here until LCL advances to ledgerSeq.
108+
releaseAssert(lm.getLastClosedLedgerNum() == ledgerNum);
104109
return lm.getLastClosedLedgerHeader().hash;
105110
}
106111

@@ -183,7 +188,7 @@ template size_t countEntries(std::shared_ptr<LiveBucket> bucket);
183188
template size_t countEntries(std::shared_ptr<HotArchiveBucket> bucket);
184189

185190
void
186-
LedgerManagerForBucketTests::transferLedgerEntriesToBucketList(
191+
LedgerManagerForBucketTests::sealLedgerTxnAndTransferEntriesToBucketList(
187192
AbstractLedgerTxn& ltx,
188193
std::unique_ptr<LedgerCloseMetaFrame> const& ledgerCloseMeta,
189194
LedgerHeader lh, uint32_t initialLedgerVers)
@@ -264,7 +269,7 @@ LedgerManagerForBucketTests::transferLedgerEntriesToBucketList(
264269
ltxEvictions.commit();
265270
}
266271
mApp.getLedgerManager()
267-
.getMutableSorobanNetworkConfig()
272+
.getMutableSorobanNetworkConfigForApply()
268273
.maybeSnapshotBucketListSize(lh.ledgerSeq, ltx, mApp);
269274
}
270275

@@ -288,7 +293,7 @@ LedgerManagerForBucketTests::transferLedgerEntriesToBucketList(
288293
}
289294
else
290295
{
291-
LedgerManagerImpl::transferLedgerEntriesToBucketList(
296+
LedgerManagerImpl::sealLedgerTxnAndTransferEntriesToBucketList(
292297
ltx, ledgerCloseMeta, lh, initialLedgerVers);
293298
}
294299
}

‎src/bucket/test/BucketTestUtils.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ class LedgerManagerForBucketTests : public LedgerManagerImpl
6767
std::vector<LedgerKey> mTestDeadEntries;
6868

6969
protected:
70-
void transferLedgerEntriesToBucketList(
70+
void sealLedgerTxnAndTransferEntriesToBucketList(
7171
AbstractLedgerTxn& ltx,
7272
std::unique_ptr<LedgerCloseMetaFrame> const& ledgerCloseMeta,
7373
LedgerHeader lh, uint32_t initialLedgerVers) override;

‎src/catchup/ApplyLedgerWork.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ BasicWork::State
2323
ApplyLedgerWork::onRun()
2424
{
2525
ZoneScoped;
26-
mApp.getLedgerManager().closeLedger(mLedgerCloseData,
26+
mApp.getLedgerManager().applyLedger(mLedgerCloseData,
2727
/* externalize */ false);
2828
return BasicWork::State::WORK_SUCCESS;
2929
}

‎src/catchup/LedgerApplyManagerImpl.cpp

+8-8
Original file line numberDiff line numberDiff line change
@@ -494,7 +494,7 @@ LedgerApplyManagerImpl::tryApplySyncingLedgers()
494494
{
495495
ZoneScoped;
496496
releaseAssert(threadIsMain());
497-
uint32_t nextToClose = *mLastQueuedToApply + 1;
497+
uint32_t nextToApply = *mLastQueuedToApply + 1;
498498
auto lcl = mApp.getLedgerManager().getLastClosedLedgerNum();
499499

500500
// We can apply multiple ledgers here, which might be slow. This is a rare
@@ -505,20 +505,20 @@ LedgerApplyManagerImpl::tryApplySyncingLedgers()
505505
auto const& lcd = it->second;
506506

507507
// we still have a missing ledger
508-
if (nextToClose != lcd.getLedgerSeq())
508+
if (nextToApply != lcd.getLedgerSeq())
509509
{
510510
break;
511511
}
512512

513513
// If we have too many ledgers queued to apply, just stop scheduling
514514
// more and let the node gracefully go into catchup.
515515
releaseAssert(mLastQueuedToApply >= lcl);
516-
if (nextToClose - lcl >= MAX_EXTERNALIZE_LEDGER_APPLY_DRIFT)
516+
if (nextToApply - lcl >= MAX_EXTERNALIZE_LEDGER_APPLY_DRIFT)
517517
{
518518
CLOG_INFO(History,
519519
"Next ledger to apply is {}, but LCL {} is too far "
520520
"behind, waiting",
521-
nextToClose, lcl);
521+
nextToApply, lcl);
522522
break;
523523
}
524524

@@ -533,19 +533,19 @@ LedgerApplyManagerImpl::tryApplySyncingLedgers()
533533
{
534534
return;
535535
}
536-
app.getLedgerManager().closeLedger(lcd,
536+
app.getLedgerManager().applyLedger(lcd,
537537
/* externalize */ true);
538538
},
539-
"closeLedger queue");
539+
"applyLedger queue");
540540
}
541541
else
542542
{
543-
mApp.getLedgerManager().closeLedger(lcd, /* externalize */ true);
543+
mApp.getLedgerManager().applyLedger(lcd, /* externalize */ true);
544544
}
545545
mLastQueuedToApply = lcd.getLedgerSeq();
546546

547547
++it;
548-
++nextToClose;
548+
++nextToApply;
549549
}
550550

551551
mSyncingLedgers.erase(mSyncingLedgers.cbegin(), it);

‎src/catchup/LedgerApplyManagerImpl.h

+4-17
Original file line numberDiff line numberDiff line change
@@ -49,23 +49,10 @@ class LedgerApplyManagerImpl : public LedgerApplyManager
4949
std::map<uint32_t, LedgerCloseData> mSyncingLedgers;
5050
medida::Counter& mSyncingLedgersSize;
5151

52-
// Conceptually, there are three ledger sequences that LedgerManager, Herder
53-
// and LedgerApplyManager rely on:
54-
// - L (mLargestLedgerSeqHeard) = maximum ledger that core heard the
55-
// network externalize, may or may not be applied.
56-
// - Q (mLastQueuedToApply) = Tracks maximum ledger dequeued from
57-
// mSyncingLedgers and passed to apply -- either synchronously or posted to
58-
// background thread queue. Note: Member variable is an optional<> only
59-
// because it is lazily initialized after LCL, it's supposed to have a
60-
// definite value (>= LCL) from then on.
61-
// - LCL = last closed ledger, the last ledger that was externalized, and
62-
// applied by core.
63-
// - Core maintains the following invariant: LCL <= Q <= L. New ledgers
64-
// cause each number to increment, from right to left. First externalize
65-
// enqueues a ledger in mSyncingLedgers, incrementing L. Then a ledger
66-
// is dequeued from mSyncingLedgers and sent to apply, incrementing Q
67-
// towards L. Then a ledger finishes apply, incrementing LCL towards Q.
68-
// Eventually every ledger passes through each of these phases.
52+
// These state variables track the flow of ledgers through mSyncingLedgers,
53+
// they are the variables Q and L in the diagram in LedgerManager.h. See
54+
// that diagram for details and discussion of the threading model and
55+
// flow of ledgers between LedgerApplyManager and LedgerManager.
6956
std::optional<uint32_t> mLastQueuedToApply;
7057
uint32_t mLargestLedgerSeqHeard;
7158

‎src/catchup/ReplayDebugMetaWork.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ ReplayDebugMetaWork::applyLastLedger()
163163
auto lcl = mApp.getLedgerManager().getLastClosedLedgerNum();
164164
if (lcl + 1 == debugTxSet.ledgerSeq)
165165
{
166-
mApp.getLedgerManager().closeLedger(
166+
mApp.getLedgerManager().applyLedger(
167167
LedgerCloseData::toLedgerCloseData(debugTxSet),
168168
/* externalize */ false);
169169
}

‎src/herder/HerderImpl.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -1165,7 +1165,7 @@ HerderImpl::lastClosedLedgerIncreased(bool latest, TxSetXDRFrameConstPtr txSet)
11651165
{
11661166
// Re-start heartbeat tracking _after_ applying the most up-to-date
11671167
// ledger. This guarantees out-of-sync timer won't fire while we have
1168-
// ledgers to apply (applicable during parallel ledger close).
1168+
// ledgers to apply (applicable during parallel ledger apply).
11691169
trackingHeartBeat();
11701170

11711171
// Ensure out of sync recovery did not get triggered while we were
@@ -2145,7 +2145,7 @@ HerderImpl::maybeHandleUpgrade()
21452145
return;
21462146
}
21472147
auto const& conf =
2148-
mApp.getLedgerManager().getSorobanNetworkConfigReadOnly();
2148+
mApp.getLedgerManager().getLastClosedSorobanNetworkConfig();
21492149

21502150
auto maybeNewMaxTxSize =
21512151
conf.txMaxSizeBytes() + getFlowControlExtraBuffer();
@@ -2197,7 +2197,7 @@ HerderImpl::start()
21972197
if (protocolVersionStartsFrom(version, SOROBAN_PROTOCOL_VERSION))
21982198
{
21992199
auto const& conf =
2200-
mApp.getLedgerManager().getSorobanNetworkConfigReadOnly();
2200+
mApp.getLedgerManager().getLastClosedSorobanNetworkConfig();
22012201
mMaxTxSize = std::max(mMaxTxSize, conf.txMaxSizeBytes() +
22022202
getFlowControlExtraBuffer());
22032203
}

‎src/herder/TransactionQueue.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -382,7 +382,7 @@ TransactionQueue::canAdd(
382382
if (!tx->checkSorobanResourceAndSetError(
383383
mApp.getAppConnector(),
384384
mApp.getLedgerManager()
385-
.getSorobanNetworkConfigReadOnly(),
385+
.getLastClosedSorobanNetworkConfig(),
386386
mApp.getLedgerManager()
387387
.getLastClosedLedgerHeader()
388388
.header.ledgerVersion,

‎src/herder/TxSetFrame.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -1610,7 +1610,7 @@ TxSetPhaseFrame::checkValid(Application& app,
16101610
isSoroban
16111611
? checkValidSoroban(
16121612
lcl.header,
1613-
app.getLedgerManager().getSorobanNetworkConfigReadOnly())
1613+
app.getLedgerManager().getLastClosedSorobanNetworkConfig())
16141614
: checkValidClassic(lcl.header);
16151615
if (!checkPhaseSpecific)
16161616
{

‎src/herder/test/HerderTests.cpp

+6-6
Original file line numberDiff line numberDiff line change
@@ -995,7 +995,7 @@ TEST_CASE("tx set hits overlay byte limit during construction",
995995
});
996996

997997
auto const& conf =
998-
app->getLedgerManager().getSorobanNetworkConfigReadOnly();
998+
app->getLedgerManager().getLastClosedSorobanNetworkConfig();
999999
uint32_t maxContractSize = 0;
10001000
maxContractSize = conf.maxContractSizeBytes();
10011001

@@ -1150,7 +1150,7 @@ TEST_CASE("surge pricing", "[herder][txset][soroban]")
11501150
auto tx = makeMultiPayment(acc1, root, 1, 100, 0, 1);
11511151

11521152
SorobanNetworkConfig conf =
1153-
app->getLedgerManager().getSorobanNetworkConfigReadOnly();
1153+
app->getLedgerManager().getLastClosedSorobanNetworkConfig();
11541154

11551155
uint32_t const baseFee = 10'000'000;
11561156
SorobanResources resources;
@@ -3232,7 +3232,7 @@ TEST_CASE("overlay parallel processing")
32323232
Topologies::core(4, 1, Simulation::OVER_TCP, networkID, [](int i) {
32333233
auto cfg = getTestConfig(i, Config::TESTDB_POSTGRESQL);
32343234
cfg.TESTING_UPGRADE_MAX_TX_SET_SIZE = 100;
3235-
cfg.EXPERIMENTAL_PARALLEL_LEDGER_CLOSE = true;
3235+
cfg.EXPERIMENTAL_PARALLEL_LEDGER_APPLY = true;
32363236
cfg.ARTIFICIALLY_DELAY_LEDGER_CLOSE_FOR_TESTING =
32373237
std::chrono::milliseconds(500);
32383238
return cfg;
@@ -3390,7 +3390,7 @@ TEST_CASE("soroban txs accepted by the network",
33903390
for (auto node : nodes)
33913391
{
33923392
REQUIRE(node->getLedgerManager()
3393-
.getSorobanNetworkConfigReadOnly()
3393+
.getLastClosedSorobanNetworkConfig()
33943394
.ledgerMaxTxCount() == ledgerWideLimit * 10);
33953395
}
33963396

@@ -3535,14 +3535,14 @@ herderExternalizesValuesWithProtocol(uint32_t version,
35353535
#ifdef USE_POSTGRES
35363536
dbMode = Config::TESTDB_POSTGRESQL;
35373537
#else
3538-
FAIL("Parallel ledger close requires postgres");
3538+
FAIL("Parallel ledger apply requires postgres");
35393539
#endif
35403540
}
35413541
auto cfg = getTestConfig(i, dbMode);
35423542
cfg.TESTING_UPGRADE_LEDGER_PROTOCOL_VERSION = version;
35433543
if (parallelLedgerClose)
35443544
{
3545-
cfg.EXPERIMENTAL_PARALLEL_LEDGER_CLOSE = true;
3545+
cfg.EXPERIMENTAL_PARALLEL_LEDGER_APPLY = true;
35463546
// Add artifical delay to ledger close to increase chances of
35473547
// conflicts
35483548
cfg.ARTIFICIALLY_DELAY_LEDGER_CLOSE_FOR_TESTING =

‎src/herder/test/TransactionQueueTests.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -1259,7 +1259,7 @@ TEST_CASE("Soroban TransactionQueue limits",
12591259
auto account2 = root.create("a2", minBalance2);
12601260

12611261
SorobanNetworkConfig conf =
1262-
app->getLedgerManager().getSorobanNetworkConfigReadOnly();
1262+
app->getLedgerManager().getLastClosedSorobanNetworkConfig();
12631263

12641264
SorobanResources resources;
12651265
resources.instructions = 2'000'000;

‎src/herder/test/TxSetTests.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -1768,7 +1768,7 @@ TEST_CASE("txset nomination", "[txset]")
17681768
});
17691769

17701770
auto const& sorobanConfig =
1771-
app->getLedgerManager().getSorobanNetworkConfigReadOnly();
1771+
app->getLedgerManager().getLastClosedSorobanNetworkConfig();
17721772
stellar::uniform_int_distribution<> txReadEntriesDistr(
17731773
1, sorobanConfig.txMaxReadLedgerEntries());
17741774

0 commit comments

Comments
 (0)