-
Notifications
You must be signed in to change notification settings - Fork 69
core, trie: refactor trie API #26995 #1147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev-upgrade
Are you sure you want to change the base?
Conversation
8232006 to
9f2b99c
Compare
|
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 You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the trie API to improve clarity and consistency by restructuring method naming conventions. The main changes rename the Try* methods (TryGet, TryUpdate, TryDelete, TryGetNode) to be the primary error-returning methods (Get, Update, Delete, GetNode), while the previous non-error-handling methods are renamed to Must* variants (MustGet, MustUpdate, MustDelete, MustGetNode) that log errors but don't return them.
Key changes:
- Renamed
Try*methods to regular methods (e.g.,TryGet→Get,TryUpdate→Update) as the primary error-returning API - Renamed previous error-suppressing methods to
Must*variants (e.g.,Get→MustGet,Update→MustUpdate) - Updated the
TrieHasherinterface'sUpdatemethod to return an error - Updated all test files and internal usages to use the new API
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| trie/trie.go | Core trie API refactoring: renamed methods and updated internal method calls |
| trie/secure_trie.go | Updated StateTrie wrapper methods to use new API, removed unused log import |
| trie/stacktrie.go | Swapped Update and MustUpdate implementations to match new convention |
| trie/errors.go | Updated comment to reference new method names (Get, Update, Delete) |
| trie/trie_test.go | Updated all test calls to use MustUpdate/MustGet/MustDelete or error-checking variants |
| trie/tracer_test.go | Updated test calls to use MustUpdate/MustDelete |
| trie/sync_test.go | Updated test calls to use MustUpdate/MustGet |
| trie/stacktrie_test.go | Updated test calls to use new Update method signature |
| trie/secure_trie_test.go | Updated test calls to use MustUpdate/MustGet/MustDelete |
| trie/proof_test.go | Updated test calls to use MustUpdate |
| trie/proof.go | Updated StackTrie.Update calls in proof verification |
| trie/iterator_test.go | Updated test calls to use MustUpdate/MustGet |
| core/types/hashing.go | Updated TrieHasher interface to return error from Update method, added explanatory comment |
| core/types/hashing_test.go | Updated testHasher.Update to return error |
| core/types/block_test.go | Updated testHasher.Update to return error |
| core/state/sync_test.go | Updated trie method calls to use GetNode and MustGet |
| core/rawdb/accessors_indexes_test.go | Updated testHasher.Update to return error |
| XDCxlending/lendingstate/XDCx_trie.go | Updated internal trie method calls to use new API (wrapper maintains TryUpdate/TryGet interface) |
| XDCx/tradingstate/XDCx_trie.go | Updated internal trie method calls to use new API (wrapper maintains TryUpdate/TryGet interface) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (t *Trie) MustGet(key []byte) []byte { | ||
| res, err := t.Get(key) | ||
| if err != nil { | ||
| log.Error("Unhandled trie error in Trie.Get", "err", err) |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message references "Trie.Get" but this is in the MustGet function. The error message should reference "Trie.MustGet" for clarity and consistency with the actual function being called.
| log.Error("Unhandled trie error in Trie.Get", "err", err) | |
| log.Error("Unhandled trie error in Trie.MustGet", "err", err) |
In this PR, all TryXXX(e.g. TryGet) APIs of trie are renamed to XXX(e.g. Get) with an error returned. The original XXX(e.g. Get) APIs are renamed to MustXXX(e.g. MustGet) and does not return any error -- they print a log output. A future PR will change the behaviour to panic on errorrs.
9f2b99c to
a81e006
Compare
Proposed changes
Ref: ethereum#26995
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which part of the codebase this PR will touch base on,
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that