Skip to content

rpc/jsonrpc: test the receipt cache against physically retired history - #23871

Merged
lupin012 merged 3 commits into
mainfrom
lupin012/prune_gates_physical_history
Sep 9, 2026
Merged

lupin012 merged 3 commits into
mainfrom
lupin012/prune_gates_physical_history

Conversation

@lupin012

@lupin012 lupin012 commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #23322, closing the half of
r3880873497 that
#23759 left open. Test-only change.

setupPruneGating stores the prune mode without pruning anything, so a cell asserting that a
keep-all receipt retention still serves an old block can be answered by re-execution. #23759
added requirePersistedReceipts, which proves the cache holds the receipts, but not that the
cache is what the endpoint reads.

New fixture setupPhysicallyPrunedHistory retires state history on disk: a PoS chain with a
small step size, driven block by block through the forkchoice so finality advances and state
files are built and retired. requireRetiredAbove pins the precondition —
HistoryStartFrom(kv.AccountsDomain) above the block under test — so re-execution has no
state to start from.

Two tests over the 14 endpoints gated on receipts:

Test Retention Expected
TestReceiptCacheServesBlocksWhoseHistoryIsRetired receipts keep-all, cache on every endpoint answers; only the cache can have produced it
TestReceiptsWithoutCacheStopAtRetiredHistory no cache every endpoint returns state.PrunedError

The second is what makes the first mean something: without it the availability leg would pass
on any datadir. newPruneGatingAPIs is extracted from setupPruneGating unchanged and shared
by both fixtures.

Not covered

The shape where the cache is retired alongside history: enabling it needs
statecfg.EnableHistoricalRCache(), which mutates the package-level schema and so changes the
outcome of other tests in this binary.

Verification

go test ./rpc/jsonrpc/ green (45s, the new tests ~3s each), make lint clean.

@lupin012
lupin012 marked this pull request as ready for review September 8, 2026 19:17
@lupin012
lupin012 requested a review from yperbasis as a code owner September 8, 2026 19:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Retirement waits need bounded contexts to prevent package-wide stalls.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds integration coverage proving receipt-gated RPC endpoints work after state history is physically retired.

Changes:

  • Adds a physically pruned PoS-chain fixture.
  • Tests receipt-cache and no-cache behavior across 14 endpoints.
  • Extracts shared API construction.
File summaries
File Description
rpc/jsonrpc/prune_gating_test.go Adds the pruning fixture and shared endpoint helpers.
rpc/jsonrpc/check_prune_gates_test.go Adds positive and negative receipt-cache tests.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

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

Comment thread rpc/jsonrpc/prune_gating_test.go Outdated
// advances, so the chain is PoS and the forkchoice is driven block by block.
func setupPhysicallyPrunedHistory(t *testing.T, cfg prunedHistoryConfig) (pruneGatingAPIs, pruneGatingChain) {
t.Helper()
ctx := t.Context()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@AskAlexSharov

Copy link
Copy Markdown
Collaborator

Ran both tests three times, stable. Checked the two things the design rests on and they hold: WithEnableDomain sets a.d[domain].Enabled on the per-instance aggregator, and statecfg.DomainCfg is embedded by value in Domain, so nothing package-level moves and t.Parallel() is safe. That is a real difference from EnableHistoricalRCache, so the "Not covered" section draws the line in the right place. The table has exactly 14 gatedByBlockReceipts entries and no endpoint uses gatedByReceipts at all, so the filter omits nothing. Two things about what the assertions prove.

The negative test does not test the fixture — it tests arithmetic. With the cache off the gate takes the first branch of checkReceiptSourceAvailable and lands in checkPruneHistory, which is checkPruneField on Mode.History, and that is one comparison: block < amount.PruneTo(latest). With a six-block window and head at 24 the cutoff is 18, and the block under test is 4. It never reads HistoryStartFrom or looks at a file. So TestReceiptsWithoutCacheStopAtRetiredHistory returns PrunedError on a datadir where nothing was ever retired, which is the failure mode the PR opens by describing. Its docstring has it backwards: it is not what stops the availability leg passing anywhere, it is the leg that would.

The same applies to the precondition in the positive test. apis.eth.GetBalance routes to the same window arithmetic, so "re-execution must have no state to start from" is not what that line establishes. The physical guarantee is entirely requireRetiredAbove, which compares HistoryStartFrom(kv.AccountsDomain) against the blocks max txNum, and it already runs in the shared fixture for both tests. The pair is still worth keeping, since together they attribute the answer to the cache, but the comments should say that rather than claim a disk-level check that lives elsewhere.

The negative test also moves the receipt retention, not just the cache. The positive one passes Receipts: prune.KeepAllReceiptsPruneMode; the negative one leaves Receipts unset, and receiptsOrDefault resolves nil to KeepAllBlocksPruneMode, the follow-history default rather than force-keep-all. It does not change the outcome today, because kvcfg.PersistReceipts.Enabled returns false and short-circuits before ReceiptsAmount() is read. But the pair is a controlled comparison only by accident of that ordering, and reordering the gate would quietly turn the control into a second gate test. Setting Receipts: prune.KeepAllReceiptsPruneMode in both leaves receiptCache as the one variable.

Both are about what the tests claim rather than whether they pass. Neither blocks.

Bound the retirement waits in setupPhysicallyPrunedHistory to one minute,
matching the existing execmodule retirement tests. t.Context() is cancelled
only after the test ends, so a missed retirement notification would stall the
whole package until the suite-wide timeout instead of failing that test.

Correct what the two receipt tests claim. With the cache off the gate takes
the !persisted branch of checkReceiptSourceAvailable and lands in
checkPruneHistory, one window comparison that reads neither HistoryStartFrom
nor any file, so the negative test is the control that attributes the positive
test's answers to the cache, not the leg that proves the fixture is retired.
The same arithmetic serves the GetBalance precondition; the disk-level
guarantee is requireRetiredAbove, which already runs in the shared fixture.

Set Receipts: KeepAllReceiptsPruneMode in the negative test too. An unset
field resolves to KeepAllBlocksPruneMode, the follow-history default rather
than force-keep-all; the outcome is unchanged today only because
PersistReceipts.Enabled short-circuits before ReceiptsAmount() is read, which
makes the comparison controlled by accident of gate ordering.
@lupin012

lupin012 commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

@AskAlexSharov All 3 points are addressed

@lupin012
lupin012 enabled auto-merge September 9, 2026 19:58
@lupin012
lupin012 added this pull request to the merge queue Sep 9, 2026
Merged via the queue into main with commit 1722aae Sep 9, 2026
138 checks passed
@lupin012
lupin012 deleted the lupin012/prune_gates_physical_history branch September 9, 2026 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants