feat: support CopyTrie() for reconstructed tries#5394
Conversation
9e57724 to
a18e346
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes failures in RPC paths (e.g., eth_accessList, eth_estimateGas) when they call CopyTrie() on reconstructed account tries by adding support for deep-copying reconstructed tries rather than returning nil.
Changes:
- Added
(*reconstructedAccountTrie).Copy()that clones the underlyingffi.Reconstructedhandle and rebinds the base trie reader to the clone. - Updated
reconstructedStateAccessor.CopyTrie()to return a copied reconstructed trie (and log on copy errors) instead of always returningnil. - Added unit test
TestReconstructedCopyTrie()to validate the copied trie is usable and independent.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| graft/evm/firewood/reconstructed_trie.go | Adds Copy() to deep-copy reconstructed tries by cloning the ffi.Reconstructed handle and rebinding the reader. |
| graft/evm/firewood/reconstructed_state.go | Uses the new Copy() implementation from CopyTrie() for reconstructed account tries. |
| graft/evm/firewood/reconstructed_state_test.go | Adds a unit test verifying CopyTrie() returns an independent reconstructed trie. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a18e346 to
fd604af
Compare
alarso16
left a comment
There was a problem hiding this comment.
The point of this was to fix a unit-testable issue with an API - doesn't this fully fix that issue? Will you add that to this PR?
fd604af to
5b5476b
Compare
@alarso16 I added |
| // generated via block re-execution. In that case, Firewood serves a reconstructed account | ||
| // trie, and copying that trie used to fail as copying reconstructed revisions was | ||
| // previously unsupported. | ||
| func TestFirewoodCopyTrie(t *testing.T) { |
There was a problem hiding this comment.
You describe this test in relation to its fix, but it would be better to describe it relative to what functionality it actually needs: TestFirewoodModifiedStateRPC or something
| restartCTX.NetworkUpgrades = upgradetest.GetConfig(fork) | ||
| restartCTX.ChainDataDir = tvm.Ctx.ChainDataDir |
There was a problem hiding this comment.
these don't make sense immediately, add a short comment
| blockNumOrHash := rpc.BlockNumberOrHashWithNumber(rpc.BlockNumber(blockNum)) | ||
|
|
||
| // eth_estimateGas params. | ||
| var ( |
There was a problem hiding this comment.
subjective: separate subtest per block/transaction? it would help with debugging, but the nesting would likely feel unnecessarily deep
Why this should be merged
Previously, trying to copy a reconstructed account trie returned
nilas reconstructed revisions (i.e. the pointer backing reconstructed account tries) are not safe to share. However, methods such aseth_accessListandeth_estimateGascallCopyTrie()- in the case that the trie is a reconstructed account trie, this would eventually result in these operations trying to read against anil-pointer, resulting in failed queries.This PR fixes this by enabling
CopyTrie()to deep copy a reconstructed account trie if one is passed in.How this works
This PR adds a
Copy()method to the reconstructed account trie type which returns a deep copy. This method utilizes theClone()method which was added as part of Firewoodv0.5.0.How this was tested
CI + added two UTs:
TestReconstructedCopyTrie()TestFirewoodCopyTrie()Need to be documented in RELEASES.md?
No