Use loser-tree merge for ConservationOfLumens - #5427
Conversation
There was a problem hiding this comment.
Pull request overview
Generalizes loser-tree bucket scans to support hot archive entries and reduce ConservationOfLumens memory usage.
Changes:
- Adds generic live/hot-archive current-entry scans.
- Migrates the invariant to deduplicated scans.
- Updates callbacks, indexes, and tests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/ledger/InMemorySorobanState.cpp |
Adapts scan callbacks. |
src/ledger/ImmutableLedgerView.h |
Exposes generic scans. |
src/ledger/ImmutableLedgerView.cpp |
Delegates snapshot scans. |
src/invariant/ConservationOfLumens.cpp |
Uses loser-tree scans. |
src/bucket/test/BucketIndexTests.cpp |
Updates live scan tests. |
src/bucket/LiveBucket.h |
Moves range API to base. |
src/bucket/LiveBucket.cpp |
Removes redundant implementation. |
src/bucket/HotArchiveBucketIndex.h |
Adds type-range lookup. |
src/bucket/HotArchiveBucketIndex.cpp |
Implements range lookup. |
src/bucket/BucketListSnapshot.h |
Declares generic current-entry scan. |
src/bucket/BucketListSnapshot.cpp |
Generalizes loser-tree merging. |
src/bucket/BucketBase.h |
Defines shared range API. |
src/bucket/BucketBase.cpp |
Implements shared range lookup. |
Suppressed comments (1)
src/bucket/BucketListSnapshot.cpp:726
- The dependent return type also requires
typename; otherwise this member declaration does not compile.
BucketT::EntryT const&
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Helper function that processes an entry if it hasn't been seen before. | ||
| // Returns true on success, false on error (with error set in errorMsg). |
There was a problem hiding this comment.
updated to remove the second half of the sentence
| static_assert(std::is_same_v<BucketT, HotArchiveBucket>, | ||
| "unexpected bucket type"); | ||
| if (entry.type() == HOT_ARCHIVE_ARCHIVED) | ||
| { | ||
| if (callback(entry.archivedEntry(), key) == Loop::COMPLETE) |
| TEST_CASE("scanForCurrentEntriesOfType randomized testing", | ||
| "[bucket][bucketindex]") |
SirTyson
left a comment
SirTyson
left a comment
There was a problem hiding this comment.
Overall this looks good to me! Given that the scanning primitive is subtlety different for the HotArchive vs. LiveBucketlist, I'd like to see an explicit test of the loser-tree scanning logic for the Hot Arcchive. There's also a few minor nits, but a helpful change!
| void scanCurrentLiveEntriesOfType( | ||
| LedgerEntryType type, | ||
| std::function<void(LedgerEntry const&, LedgerKey const&)> callback) | ||
| std::function<Loop(LedgerEntry const&, LedgerKey const&)> callback) |
There was a problem hiding this comment.
Just curious, why did this change from void to loop? We see this type a lot now, it might be nice to have a typedef for it.
| } | ||
|
|
||
| void | ||
| ImmutableLedgerView::scanCurrentHotArchiveEntriesOfType( |
There was a problem hiding this comment.
I like the change, but can we extend the iteration tests to cover the hot archive BucketList as well?
| applyView.scanLiveEntriesOfType( | ||
| type, [&](BucketEntry const& be) -> Loop { | ||
| applyView.scanCurrentLiveEntriesOfType( | ||
| type, [&](LedgerEntry const& le, LedgerKey const& key) -> Loop { |
There was a problem hiding this comment.
Nit: Is it possible that this isStopping condition could run for a long time now? We enter the callback much less often now, so we could potentially do large batches of IO without hitting the callback in old buckets with mostly dead entries. Not a big deal, but if there's a trivial way to stop early more aggressively it might be nice, not a big deal either way.
| static_assert(std::is_same_v<BucketT, HotArchiveBucket>, | ||
| "unexpected bucket type"); | ||
| if (entry.type() == HOT_ARCHIVE_ARCHIVED) | ||
| { | ||
| if (callback(entry.archivedEntry(), key) == Loop::COMPLETE) |
Adapts the loser tree merge from #5252 to support the hot archive bucket list, as well. This lets us use the merge for the conservation of lumens invariant, which lowers the memory usage. On a noble dev watcher, the node no longer runs into memory problems; over the last week, memory usage (on commit 6c2ccfe) settled to 11.8–12 GB (vs 12–14 on a different watcher running 27.0.0 jammy during the same timespan).