Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🟡 Changes recommended
The regression test does not verify that the release callback executes and would pass without the fix.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes leaked trie-state references when transaction-state replay exits with an error.
Changes:
- Releases acquired state unless ownership is returned to the caller.
- Adds an out-of-range transaction test.
File summaries
| File | Description |
|---|---|
eth/state_accessor.go |
Guards state-reference ownership and cleanup. |
eth/state_accessor_test.go |
Exercises the out-of-range error path. |
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.
stateAtTransaction obtains the parent state through StateAtBlock with readOnly set, which takes a real reference on the live trie database whenever the state is available there. The three paths that give up after that returned without releasing it: the message build failure, the replay failure and the out of range index. Only the paths that return the state hand the release function over to the caller, so the reference taken by every give-up path was leaked. Release it with a guard that defaults to releasing and is cancelled when the release function is handed over instead, so a newly added error path cannot leak again. Historical issue rather than one introduced here: the baseline and upstream geth both drop the release function on the same paths (no upstream fix to port). It is reachable through the debug methods that go through stateAtTransaction -- debug_traceTransaction, debug_traceCall and debug_storageRangeAt -- each of which exits with an error after the state has been obtained.
c8eab15 to
b23b66f
Compare
stateAtTransactionobtains the parent state withStateAtBlock(..., readOnly=true), which takes a real reference on the live trie database whenever the state is available there, and hands a release function to its caller. Every path that gives up after that returns a nil release instead of calling it, so the reference is dropped rather than released:eth/state_accessor.go:249, now:261):255, now:267):261, now:273)The out of range path is the easiest one to hit:
debug_traceTransactionwith a bad index replays the whole block and then returns an error, leaking the reference taken on the live trie database. Nothing observes the leak — it is pure reference accounting, with no log, no panic and no failing assertion.Fix
Take the release handle behind a guard that defaults to releasing and is cancelled only on the paths that hand the release over to the caller:
Only the two paths that return the state set
handedOff = true, so a newly added error path cannot leak again.Upstream
Historical issue rather than one introduced on top of the base: geth's
stateAtTransaction(ethereum/go-ethereum@854fbb8ce,eth/state_accessor.go:232-277) drops the release on the same three error paths, so there is no upstream fix to port and no geth PR to reference.Tests
TestStateAtTransactionGiveUpReturnsNoStatedrives the out of range path and pins what is observable from outside: the call returns nothing half-initialised (no transaction, no state and no release function). Whether the reference is actually released is deliberately not asserted — the release function is created insideStateAtBlockand cannot be observed or injected — so that part of the fix rests on the deferred block being executed, not on an assertion.make all,go test ./eth/ -run TestStateAtTransaction,make quick-test,make testandgofmtare clean.Relation to other work
A companion branch #2579 changes the replay inside this same function so that it follows the block processing routing. Both branches touch
eth/state_accessor.go; merging this one first keeps the other a trivial rebase.