trie: use explicit errors in stacktrie (instead of panic) #28361
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
holiman
force-pushed
the
stacktrie_nopanic
branch
from
80bf446 to
76922a4
Compare
holiman
changed the title
holiman
force-pushed
the
stacktrie_nopanic
branch
from
abeef8a to
6e530c3
Compare
holiman
force-pushed
the
stacktrie_nopanic
branch
from
6e530c3 to
566818c
Compare
trie/stacktrie.go
Outdated
| if bytes.Compare(t.last, k) >= 0 { | ||
| return errors.New("non-ascending key order") | ||
| } | ||
| if err := t.insert(t.root, k, value, nil); err != nil { |
There was a problem hiding this comment.
We need to track/update the first/last markers, then do the insertion.
Otherwise the key of current inserted entry will be larger than last, can be detected as right boundary?
|
Closing this for now, might not be needed any more |
|
@rjl493456442 PTAL |
rjl493456442
approved these changes
Oct 25, 2023
devopsbo3
pushed a commit
to HorizenOfficial/go-ethereum
that referenced
this pull request
Nov 10, 2023
…8361) This PR removes panics from stacktrie (mostly), and makes the Update return errors instead. While adding tests for this, I also found that one case of possible corruption was not caught, which is now fixed.
devopsbo3
added a commit
to HorizenOfficial/go-ethereum
that referenced
this pull request
Nov 10, 2023
…hereum#28361)" This reverts commit 5cbd950.
devopsbo3
added a commit
to HorizenOfficial/go-ethereum
that referenced
this pull request
Nov 10, 2023
…hereum#28361)" This reverts commit 5cbd950.
Dergarcon
pushed a commit
to MetaMask/mev-geth-0x2mev
that referenced
this pull request
Jan 31, 2024
…8361) This PR removes panics from stacktrie (mostly), and makes the Update return errors instead. While adding tests for this, I also found that one case of possible corruption was not caught, which is now fixed.
colinlyguo
pushed a commit
to scroll-tech/go-ethereum
that referenced
this pull request
Oct 31, 2024
…8361) This PR removes panics from stacktrie (mostly), and makes the Update return errors instead. While adding tests for this, I also found that one case of possible corruption was not caught, which is now fixed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up to #28350 .
This PR removes
panics from stacktrie (mostly), and makes theUpdatereturn errors instead. While adding tests for this, I also found that one case of possible corruption was not caught.If we inserted into an existing node, we noticed and panic:ed. However, it was possible to insert into an "empty slot" further back, so inserting at
0xaafollowed by0x10did not panic. To handle that, I now added alastPathto the stacktrie, to keep record of the path of the last inserted item.This can also be useful for keeping track of the right hand boundary.