Skip to content
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

fix: check hashed state for loading TriePrefixSets::destroyed_accounts #12235

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

joshieDo
Copy link
Collaborator

@joshieDo joshieDo commented Oct 31, 2024

hive

During a pipeline unwind, the MerkleUnwind stage will deal with an unwound hashed state and a non-unwound plain state. On normal execution, destroyed_accounts are populated if accounts are no longer present on PlainState. This will eventually delete any existing storage tries.

However, on a unwind to a block where an address is no longer present and used to have storage tries, this does not happen, since we only unwind PlainState after the MerkleUnwind. Resulting in forgotten storage tries. Leading to the the following failure case on holesky:
2631791 → unwind 2631774 ✅ → 2631775  state root error ❌

By looking at hashed state instead, this can be solved

edit:

Performance wise I don't know if this has an impact actually. We're searching a table with 32byte key instead of 20byte. Should additional logic be added? if _unwind_ { check hashed_state } else { check plain_state }

@joshieDo joshieDo added C-bug An unexpected or incorrect behavior A-staged-sync Related to staged sync (pipelines and stages) A-trie Related to Merkle Patricia Trie implementation labels Oct 31, 2024
@joshieDo joshieDo marked this pull request as ready for review October 31, 2024 18:48
@joshieDo joshieDo marked this pull request as draft October 31, 2024 18:48
@joshieDo joshieDo marked this pull request as ready for review November 1, 2024 09:46
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

pending @rkrasiuk

Copy link
Member

@rkrasiuk rkrasiuk left a comment

Choose a reason for hiding this comment

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

@joshieDo good find, lgtm. wouldn't worry about performance too much here since pipeline unwinds are supposed to be rare

@mattsse mattsse added this pull request to the merge queue Nov 1, 2024
Merged via the queue into main with commit 969ca3e Nov 1, 2024
55 checks passed
@mattsse mattsse deleted the joshie/trie branch November 1, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-staged-sync Related to staged sync (pipelines and stages) A-trie Related to Merkle Patricia Trie implementation C-bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants