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: gracefully handle missing persisted_trie_updates #13942

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

klkvr
Copy link
Collaborator

@klkvr klkvr commented Jan 23, 2025

Closes #13940

This PR divides ExecutedBlock into ExecutedBlockWithTrieUpdates and ExecutedBlock. in-memory state only contains ExecutedBlockWithTrieUpdates without tracking the persisted_trie_updates separately.

NewCanonicalChain::Reorg variant now only contains a Vec<ExecutedBlock> for reorg chain, thus not requiring us to obtain TrieUpdates for those.

Reorged blocks are no longer re-inserted into in-memory state

@github-actions github-actions bot added the C-enhancement New feature or request label Jan 23, 2025
@klkvr klkvr force-pushed the klkvr/executed-block-with-trie-updates branch from d358142 to 8e0666a Compare January 23, 2025 11:05
@klkvr klkvr changed the title fix: remove persisted_trie_updates fix: gracefully handle missing persisted_trie_updates Jan 23, 2025
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.

I'm a bit torn about this because this could be very confusing

and not really a fan of introducing another type here.

but having the trienodes separate from the actual block makes a ton of sense and I have a feeling that this will be beneficial in the future.

leaning towards we should make this change
wdyt @rkrasiuk @Rjected

crates/chain-state/src/in_memory.rs Outdated Show resolved Hide resolved
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.

this change kinda grew on me,

one caveat here is that this now always excludes trieupdates for reorged blocks so if users are currently relying on that this will no longer be possible.
but I don't think this is the case or is even something that is useful?

crates/chain-state/src/in_memory.rs Show resolved Hide resolved
crates/chain-state/src/in_memory.rs Outdated Show resolved Hide resolved
@klkvr klkvr force-pushed the klkvr/executed-block-with-trie-updates branch from 01f1884 to a888357 Compare January 23, 2025 16:36
@klkvr
Copy link
Collaborator Author

klkvr commented Jan 23, 2025

but I don't think this is the case or is even something that is useful?

yeah I would expect that you wouldn't need this info generally, gvien that TrieUpdates are only useful when applying blocks to db, @rkrasiuk wdyt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

persisted_trie_updates data lost on restart prevents node from handling reorgs
2 participants