store/LedgerReaderTx: replace per-ledger reads on LedgerReader and LedgerReaderTx with ScanLedgers iterator

[cjonas9]

What

store.LedgerReaderTx and store.LedgerReader now expose one read idiom: ScanLedgers(ctx, start, end) iter.Seq2[RawLedger, error]. Each RawLedger lends its bytes for one loop body. The point reads become store helpers over a scan of one (store.GetLedger, store.WithLedgerRaw), and both backends implement the scan once:

• sqlite: one ordered range query inside the reader's transaction, or on the pooled connection for the non-Tx reader. A scan of one is a primary-key equality query. The blob is scanned as sql.RawBytes, so database/sql lends the driver's buffer instead of cloning it; that is the loan the contract already promises.
• v2 adapter: the view's ScanLedgers, with start raised to the view's floor so a below-floor request yields nothing instead of a *RangeError. A scan of one routes to the view's pinned point read behind the window gate, so getLatestLedger's I/O is unchanged.

This replaces: WithLedgerRaw on the Tx, BatchGetLedgers, LedgerMetadataChunk, the adapter's walk, walkSpanCap and rawHeaderFromLCMBytes, and the handlers' readLedgerPage and procErr.

• getTransactions is one scan over [start, min(latest, start+LedgerScanLimit-1)]. A yielded sequence that is not the one expected, or a range that runs dry, returns the same InvalidParams naming the missing ledger as before.
• getLedgers collects its page (at most 200 ledgers) out of the scan and renders it after the scan's readers close, slicing the header off the view in parseLedgerInfo. The datastore fallback marshals each ledger into the same function. See the benchmark section for why the page is collected first.

Why

#1004. The point-read shape forced a walk protocol the adapter had to police, cloned every ledger of a getLedgers page only to encode it, and made a v2 in-window gap answer InternalError where v1 answers InvalidParams. The scan makes sequential access the contract.

Extra Notes

Behavior

Wire output is unchanged on every pinned path. The benchmark's response-byte metric is identical on all 21 rows. One case converges: a v2 in-window hot-store gap now returns v1's InvalidParams. Missing ledgers stay silent for getLedgers, as this is the behavior that we have today.

Tests

The getTransactions differential suite passes unchanged, including error parity. Added: scan-gap and dry-tail cells for getTransactions; ordering, gap, below-floor, empty-range, cancellation, early-break and chunk-border tests for the adapter; an ordering and gap test for sqlite; a parseLedgerInfo header test over all three LCM wire versions; a getLedgers test that a scan error yielded after a full page is reported rather than dropped; a ScanLedgersFrom test that an inclusive range ending at MaxUint32 terminates. Removed: the fourteen tests on the retired walk and batch surface.

The getEvents differential suite (from #982) now validates the scan as well. Its frozen reference no longer reads the events table, and instead re-derives each request window's events from the ledgers themselves, one ScanLedgers pass over the window. This new scan approach and the old events table, its SQL prefilters and the view path are each other's oracles.

One tangential optimization

In xdr2json/conversion.go, convertAnyBytes copied the JSON out of C twice: C.GoString allocates a Go string and memcpys the C bytes into it, then json.RawMessage(jsonStr) converts string to byte slice, which must allocate and copy again because strings are immutable. A single C.GoBytes does one copy. It sits on the hot path of every JSON response, and the eight-line change is worth 8 to 11% of bytes/op on the JSON rows across all three backends.

Benchmark

BenchmarkLedgerReads (rpcv2): getTransactions, getLedgers and getLatestLedger over 865 real pubnet ledgers on v2-hot, v2-cold and v1-sqlite through an in-memory JSON-RPC pipe. Interleaved builds, -count 6, M4 Pro, base vs head:

	time	bytes/op	allocs/op
v2-hot / v2-cold getLedgers, xdr [1]	flat	flat	-3% to -11%
v2-hot / v2-cold getLedgers, json [1]	flat	-8% to -9%	-13% to -31%
v1 getLedgers, xdr	flat	flat	+6% to +7% [3]
v1 getLedgers-20/json	flat	-8%	flat
v1 getLedgers-200/json	flat [2]	-9%	-5%
getTransactions-50, xdr	flat	flat (v2), -26% (v1)	flat
getTransactions-50, json	flat	-10% to -23%	-18% to -24%
getLatestLedger, all backends	flat	flat (v2), -7% (v1)	+5 to +6 allocs [3]

[1] Measured with a page clone before rendering that a later commit removed. The removal alone (ABBA, benchtime 3x, n=4/side): hot json -3.5% time (p=0.057), -3.8% bytes/op; cold flat; allocs -11% to -15% on both.
[2] +8% (p=0.04) in the full-suite run; an eight-round rerun of the row alone: 15.59s vs 15.54s (p=0.57).
[3] The two-column scan and the scan-of-one closure, a handful of allocations per request.

Known Limitations

N/A

[Copilot]

🟡 Changes recommended

The scan helper can loop indefinitely at MaxUint32, and v2 point scans can yield work after cancellation.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Replaces ledger point reads with a unified streaming ScanLedgers contract across v1 and v2 storage.

Changes:

• Adds snapshot-aware ledger scanning and point-read adapters.
• Migrates ledger/transaction handlers and tests to scans.
• Adds cross-backend performance benchmarks.

File summaries

File	Description
internal/store/ledger.go	Defines scan contracts and adapters.
internal/rpcv2/rpcv2test/rpcv2test.go	Adds streaming fixtures and local client.
internal/rpcv2/ledger_reads_bench_test.go	Benchmarks ledger APIs across backends.
internal/rpcv2/corestate/corestate_test.go	Updates reader stub.
internal/rpcv2/chunk/chunk.go	Updates scan-bound documentation.
internal/rpcv2/backfill/process_test.go	Generalizes fixture callbacks.
internal/rpcv2/adapters/view_context_test.go	Uses point-read adapter.
internal/rpcv2/adapters/ledger_reader.go	Implements v2 scanning.
internal/rpcv2/adapters/ledger_reader_test.go	Tests v2 scan behavior.
internal/rpcv1/sqlitedb/transaction_test.go	Migrates ledger lookup.
internal/rpcv1/sqlitedb/mocks.go	Implements mock scanning.
internal/rpcv1/sqlitedb/ledger.go	Implements ordered SQL scanning.
internal/rpcv1/sqlitedb/ledger_test.go	Tests and benchmarks SQL scans.
internal/methods/util.go	Uses point-read adapter.
internal/methods/util_test.go	Updates utility tests.
internal/methods/simulate_transaction.go	Migrates preflight ledger lookup.
internal/methods/simulate_transaction_test.go	Updates serializable fixtures.
internal/methods/mocks.go	Adds scan-capable method mocks.
internal/methods/json.go	Accepts raw ledger/header bytes.
internal/methods/get_transactions.go	Streams ledger ranges with gap checks.
internal/methods/get_transactions_test.go	Tests scan gaps and limits.
internal/methods/get_transactions_differential_test.go	Adapts the frozen reference.
internal/methods/get_ledgers.go	Streams and formats ledger pages.
internal/methods/get_ledgers_test.go	Updates scan and parsing coverage.
internal/methods/get_latest_ledger.go	Uses the point-read helper.
internal/methods/get_latest_ledger_test.go	Updates constant reader fixture.

Review details

• Files reviewed: 26/26 changed files
• Comments generated: 2
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Scan error propagation, maximum-range termination, and the stated benchmark acceptance regression remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

cmd/stellar-rpc/internal/store/ledger.go:86

• This inclusive uint32 loop never terminates when end == math.MaxUint32: after processing the endpoint, seq++ wraps to zero and the condition remains true. Iterate with a wider counter so every valid uint32 range terminates.

		for seq := start; seq <= end; seq++ {

• Files reviewed: 27/27 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

The scan helper can overflow indefinitely, and getLedgers can suppress terminal scan errors.

Review details

Suppressed comments (2)

cmd/stellar-rpc/internal/store/ledger.go:86

• This loop does not terminate when end == math.MaxUint32: after processing (or skipping) that sequence, seq++ wraps to zero and seq <= end remains true. Make the endpoint check explicit so draining a scan that reaches the maximum ledger cannot loop forever.

		for seq := start; seq <= end; seq++ {

cmd/stellar-rpc/internal/methods/get_ledgers.go:220

• Check err before the page-limit break. A Seq2 may emit a terminal error after its last successful ledger (the SQLite implementation does this via rows.Err()); once result reaches limit, this ordering silently converts that cancellation/read failure into a successful response, unlike the previous batch path.

		for ledger, err := range readTx.ScanLedgers(ctx, start, end) {
			if len(result) >= int(limit) {
				break
			}
			if err != nil {

• Files reviewed: 27/27 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

Copilot review overview

🔵 Needs a closer look

ScanLedgersFrom can loop forever when the inclusive range ends at math.MaxUint32.

Review effort: Balanced
Findings: 3 · 2

Open (5)

• WithLedgerRaw returns from the range body on the first value, which makes the iterator's yield…
• Check the yielded error before the page-size guard. Once result reaches limit, a final error…
• This inclusive uint32 loop never terminates when end == math.MaxUint32: after processing the…
• The linked #1004 acceptance requires every benchmark row to remain flat or improve, but the PR…
• Cancellation can occur after the initial check and while WithLedger is reading; this callback…

[Copilot]

Copilot review overview

🟡 Changes recommended

The new LedgerReader documentation promises live reads although rpcv2 uses a request-frozen view.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 2 · 1

Open (4)

• WithLedgerRaw returns from the range body on the first value, which makes the iterator's yield…
• The linked #1004 acceptance requires every benchmark row to remain flat or improve, but the PR…
• Cancellation can occur after the initial check and while WithLedger is reading; this callback…
• Correct v2 read view consistency contract · New

Resolved since last review (2)

• Check the yielded error before the page-size guard. Once result reaches limit, a final error…
• This inclusive uint32 loop never terminates when end == math.MaxUint32: after processing the…

[Copilot]

Copilot review overview

🔵 Needs a closer look

The cross-backend storage contract and borrowed-buffer lifecycle changes warrant final human validation despite extensive tests.

Review effort: Balanced
Findings: None

Resolved since last review (5)

• Removing suppression exceeds funlen limit and fails lint
• WithLedgerRaw returns from the range body on the first value, which makes the iterator's yield…
• Cancellation can occur after the initial check and while WithLedger is reading; this callback…
• Implementation contradicts benchmarked rendering behavior
• Correct v2 read view consistency contract

[Copilot]

Copilot review overview

🔵 Needs a closer look

The scan contract spans both storage backends, resource lifetimes, and several wire-facing handlers, warranting final human validation.

Review effort: Balanced
Findings: None

[Copilot]

Copilot review overview

🔵 Needs a closer look

The broad storage-interface migration spans multiple backends, RPC handlers, lifecycle semantics, and an FFI optimization, warranting final human validation.

Review effort: Balanced
Findings: None

[tamirms]

A blob does not go through the shared per-Rows buffer. database/sql uses rows.rawbuf() only for string and time.Time sources; for a []byte source the *RawBytes case is a plain *d = s ($GOROOT/src/database/sql/convert.go:274-279), aliasing whatever the driver left in the row. With mattn/go-sqlite3 that is a fresh C.GoBytes copy per row (sqlite3.go:2228), so nothing is reused here and the next Scan overwrites nothing.

The loan itself is right and should stay. The standard library only promises the slice until the next Next, Scan or Close, another driver may well hand back a reused buffer, and on v2 the reuse is real.

This PR already had the accurate wording, in 93326c0, before e76193a rewrote it:

var meta sql.RawBytes // the driver's buffer, valid until the next Next: RawLedger's loan

[cjonas9]

good catch, done

[Copilot]

Copilot review overview

🔵 Needs a closer look

The broad storage-contract rewrite spans resource lifetimes, two backends, FFI, and multiple wire-facing handlers, warranting final human validation.

Review effort: Balanced
Findings: None

[Copilot]

Copilot review overview

🔵 Needs a closer look

getLedgers still encodes while scan readers are held, contrary to the described reader lifecycle.

Review effort: Balanced
Findings: None