Bucket snapshot state refactor

[SirTyson]

Description

This completes step 4 of the ledger state refactor, namely removing BucketSnapshotState. This was a pretty hacky struct, where we kept a redundant, mutable LedgerHeader in order to verify against ledgerSeq lcl + 1 when doing checkValid in the TX queue. Commits as follows:

1. Makes the redundant BucketSnapshotState header const and add a validationLedgerSeq override to checkValid when set, this uses the override ledgerSeq instead of the LedgerHeader.
2. Removes BucketSnapshotState entirely. 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 our LedgerStateSnapshot struct.
3. Rename LedgerSnapshot to LedgerReadView. This is a lot of loc but is just a rename. We had LedgerStateSnapshot, which actually held the snapshot data and did loads, and another class LedgerSnapshot, which was just a thin compatibility wrapper around LedgerStateSnapshot and LedgerTxn. I've renamed LedgerSnapshot to LedgerReadView to separate it further and get the point across that it's really just a read-only wrapper.

Checklist

• Reviewed the contributing document
• Rebased on top of master (no merge commits)
• Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
• Compiles
• Ran all tests
• If change impacts performance, include supporting evidence per the performance document

[SirTyson]

I don't love how checkValid has access to 2 potentially divergent ledgerSeq values, the LedgerHeader value from the snapshot and the override value. For the functions that actually require the ledgerSeq, I've made their arguments a little more verbose so you have to manually pass the ledgerSeq and associated values instead of passing the header + override value to make it a little less footgunny.

[dmkozh]

This makes me wonder if we actually care about predicting the future value. You need to be pretty precise to squeeze the tx into mempool just before or just after the valid time window, so maybe we don't need this logic at all? Just some food for thought, I don't think we should do anything in this PR.

[Copilot]

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 validationLedgerSeq parameter to TransactionFrameBase::checkValid and thread it through transaction validation paths (TX queue / txset utils).
• Remove BucketSnapshotState; make LedgerHeaderWrapper bucket-backed headers immutable and update snapshot interfaces accordingly.
• Rename LedgerSnapshot → LedgerReadView and 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).

[Copilot]

CompleteConstLedgerState::createAndMaybeLoadConfig constructs a temporary CompleteConstLedgerState (and LedgerStateSnapshot) just to load SorobanNetworkConfig, then constructs the final CompleteConstLedgerState again. This duplicates snapshot construction work (including BucketListSnapshotData + Searchable* snapshots) on every call where Soroban config is required and not already provided. Consider loading config directly from the current live bucket list without building a full temporary CompleteConstLedgerState (e.g., a lightweight read-only snapshot over the live bucket data + header solely for config loading), or otherwise restructure so the state is constructed once and config is populated without a second full construction.

Suggested change

	std::optional<SorobanNetworkConfig> sorobanConfig;
	if (protocolVersionStartsFrom(lcl.header.ledgerVersion,
	SOROBAN_PROTOCOL_VERSION))
	{
	// Bootstrap: build a lightweight temporary state (no historical
	// snapshots) just to load config from the current live bucket list.
	auto tempState = std::make_shared<CompleteConstLedgerState>(
	liveBL, hotArchiveBL, lcl, has, /*sorobanConfig*/ std::nullopt,
	/*prevState*/ nullptr, /*numHistoricalSnapshots*/ 0);
	LedgerStateSnapshot tempSnap(tempState, metrics);
	sorobanConfig = SorobanNetworkConfig::loadFromLedger(tempSnap);
	}
	return std::make_shared<CompleteConstLedgerState>(
	liveBL, hotArchiveBL, lcl, has, std::move(sorobanConfig),
	std::move(prevState), numHistoricalSnapshots);
	auto state = std::make_shared<CompleteConstLedgerState>(
	liveBL, hotArchiveBL, lcl, has, /*sorobanConfig*/ std::nullopt,
	std::move(prevState), numHistoricalSnapshots);
	if (protocolVersionStartsFrom(lcl.header.ledgerVersion,
	SOROBAN_PROTOCOL_VERSION))
	{
	LedgerStateSnapshot snap(state, metrics);
	state->mSorobanConfig = SorobanNetworkConfig::loadFromLedger(snap);
	}
	return state;

[dmkozh]

I mostly agree with this comment, it is kind of weird that we create two states here. This basically highlights a weird ownership loop for the config. We don't have to resolve this now if there is no simple change, but I think it's worth re-think the config ownership model. Maybe CompleteConstLedgerState should just load the config inside the constructor? Or make this the only factory function and access some internals. Basically it would be nice to have a clear invariant for when the config exists (i.e. it shouldn't be possible to create CompleteConstLedgerState without config starting from p20).

[dmkozh]

This makes me wonder if we actually care about predicting the future value. You need to be pretty precise to squeeze the tx into mempool just before or just after the valid time window, so maybe we don't need this logic at all? Just some food for thought, I don't think we should do anything in this PR.

[dmkozh]

I mostly agree with this comment, it is kind of weird that we create two states here. This basically highlights a weird ownership loop for the config. We don't have to resolve this now if there is no simple change, but I think it's worth re-think the config ownership model. Maybe CompleteConstLedgerState should just load the config inside the constructor? Or make this the only factory function and access some internals. Basically it would be nice to have a clear invariant for when the config exists (i.e. it shouldn't be possible to create CompleteConstLedgerState without config starting from p20).