Bucket snapshot state refactor#5212
Merged
SirTyson merged 3 commits intoApr 18, 2026
Merged
Conversation
SirTyson
commented
Apr 9, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR completes step 4 of the ledger state refactor by removing BucketSnapshotState, tightening snapshot/header mutability, and renaming the read-only snapshot wrapper from LedgerSnapshot to LedgerReadView. It also introduces an explicit validationLedgerSeq override to support pre-consensus validation against LCL+1 without mutating snapshot headers.
Changes:
- Add
validationLedgerSeqparameter toTransactionFrameBase::checkValidand thread it through transaction validation paths (TX queue / txset utils). - Remove
BucketSnapshotState; makeLedgerHeaderWrapperbucket-backed headers immutable and update snapshot interfaces accordingly. - Rename
LedgerSnapshot→LedgerReadViewand update call sites across core, overlay, simulation, and tests.
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/transactions/TransactionFrameBase.h | Extends checkValid API with optional validationLedgerSeq; updates snapshot type to LedgerReadView. |
| src/transactions/TransactionFrame.h | Threads LedgerReadView + validationLedgerSeq through validation helpers and signature checks. |
| src/transactions/TransactionFrame.cpp | Implements validation override behavior (ledger-seq-dependent preconditions) and updates snapshot usage. |
| src/transactions/test/TransactionTestFrame.h | Updates test wrapper to accept LedgerReadView and propagate optional validationLedgerSeq. |
| src/transactions/test/TransactionTestFrame.cpp | Updates test wrapper implementation to construct/use LedgerReadView. |
| src/transactions/test/SorobanTxTestUtils.cpp | Updates validation helpers to use LedgerReadView. |
| src/transactions/test/PaymentTests.cpp | Replaces LedgerSnapshot usage with LedgerReadView. |
| src/transactions/test/ParallelApplyTest.cpp | Replaces LedgerSnapshot usage with LedgerReadView in apply tests. |
| src/transactions/test/InvokeHostFunctionTests.cpp | Replaces LedgerSnapshot usage with LedgerReadView in Soroban tests. |
| src/transactions/test/FrozenLedgerKeysTests.cpp | Replaces LedgerSnapshot usage with LedgerReadView throughout frozen-keys tests. |
| src/transactions/OperationFrame.h | Updates operation validation/signature APIs to use LedgerReadView. |
| src/transactions/OperationFrame.cpp | Uses LedgerReadView during apply/validation and account loading. |
| src/transactions/FeeBumpTransactionFrame.h | Updates fee-bump validation/signature APIs to use LedgerReadView and new checkValid signature. |
| src/transactions/FeeBumpTransactionFrame.cpp | Threads LedgerReadView + validationLedgerSeq into inner-tx validation. |
| src/test/TxTests.cpp | Updates helper snapshot usage to LedgerReadView. |
| src/test/TestAccount.cpp | Updates read-only lookups to LedgerReadView (plus minor local refactors). |
| src/test/FuzzerImpl.cpp | Updates fuzzer validation snapshot wrapper to LedgerReadView. |
| src/simulation/TxGenerator.cpp | Updates ledger lookups to use LedgerReadView. |
| src/simulation/LoadGenerator.cpp | Updates state-sync checks to use LedgerReadView. |
| src/simulation/ApplyLoad.cpp | Updates generated-tx validation to use LedgerReadView. |
| src/overlay/test/OverlayTests.cpp | Updates overlay tests to use LedgerReadView for validation. |
| src/overlay/Peer.cpp | Uses overlay-thread snapshot via LedgerReadView during background signature cache population. |
| src/main/CommandHandler.cpp | Updates CLI handlers that read config upgrade sets / config entries to use LedgerReadView. |
| src/ledger/NetworkConfig.h | Switches config loader API to accept AbstractLedgerStateSnapshot instead of LedgerSnapshot. |
| src/ledger/NetworkConfig.cpp | Implements snapshot-agnostic Soroban config loading via AbstractLedgerStateSnapshot. |
| src/ledger/LedgerStateSnapshot.h | Removes BucketSnapshotState, makes bucket-backed headers immutable, renames LedgerSnapshot → LedgerReadView, and formalizes snapshot interface inheritance. |
| src/ledger/LedgerStateSnapshot.cpp | Removes BucketSnapshotState, implements LedgerReadView, and adds CompleteConstLedgerState::createAndMaybeLoadConfig. |
| src/ledger/LedgerManagerImpl.cpp | Moves Soroban config bootstrapping into CompleteConstLedgerState::createAndMaybeLoadConfig and updates upgrade validation snapshot type. |
| src/ledger/InMemorySorobanState.cpp | Loads Soroban config directly from ApplyLedgerStateSnapshot via snapshot-agnostic API. |
| src/invariant/ConservationOfLumens.cpp | Updates ledger-header access to account for wrapper-based header retrieval. |
| src/invariant/BucketListStateConsistency.cpp | Updates ledger-header access and loads Soroban config from snapshot-agnostic API. |
| src/invariant/ArchivedStateConsistency.cpp | Updates ledger-header access to account for wrapper-based header retrieval. |
| src/herder/Upgrades.h | Renames upgrade inspection/validation APIs to take LedgerReadView. |
| src/herder/Upgrades.cpp | Updates upgrade creation/validation/config-upgrade loading to use LedgerReadView. |
| src/herder/TxSetUtils.cpp | Uses validationLedgerSeq override instead of mutating snapshot header. |
| src/herder/TransactionQueue.cpp | Uses validationLedgerSeq override instead of mutating snapshot header during pre-consensus checks. |
| src/herder/test/UpgradesTests.cpp | Updates tests to use LedgerReadView. |
| src/herder/test/TxSetTests.cpp | Updates tests to use LedgerReadView. |
| src/herder/test/HerderTests.cpp | Updates tests to use LedgerReadView. |
| src/bucket/test/BucketTestUtils.cpp | Updates test-only config bootstrapping for bucket-injected upgrades via createAndMaybeLoadConfig. |
| src/bucket/test/BucketListTests.cpp | Updates tests to use LedgerReadView for config entry lookups. |
| src/bucket/BucketManager.cpp | Updates eviction scan resolution to use snapshot-agnostic config loading and wrapper-based header access. |
| src/bucket/BucketListSnapshot.h | Removes BucketSnapshotState friendship (now deleted). |
ceef55c to
edb405a
Compare
dmkozh
previously approved these changes
Apr 10, 2026
284cc96 to
7817145
Compare
marta-lokhova
requested changes
Apr 15, 2026
7817145 to
5e39b5c
Compare
5e39b5c to
0c6d574
Compare
marta-lokhova
approved these changes
Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This completes step 4 of the ledger state refactor, namely removing
BucketSnapshotState. This was a pretty hacky struct, where we kept a redundant, mutableLedgerHeaderin order to verify against ledgerSeqlcl + 1when doingcheckValidin the TX queue. Commits as follows:BucketSnapshotStateheader const and add avalidationLedgerSeqoverride tocheckValidwhen set, this uses the override ledgerSeq instead of the LedgerHeader.BucketSnapshotStateentirely. This also cleans up some of the snapshot interface in general. Specifically, we load the network config a lot, and this can now be done from ourLedgerStateSnapshotstruct.LedgerSnapshottoLedgerReadView. This is a lot of loc but is just a rename. We hadLedgerStateSnapshot, which actually held the snapshot data and did loads, and another classLedgerSnapshot, which was just a thin compatibility wrapper aroundLedgerStateSnapshotandLedgerTxn. I've renamedLedgerSnapshottoLedgerReadViewto separate it further and get the point across that it's really just a read-only wrapper.Checklist
clang-formatv8.0.0 (viamake formator the Visual Studio extension)