Skip to content

feat: migrate account state to trie#407

Merged
fmoletta merged 61 commits into
mainfrom
integrate-trie-part-two
Sep 17, 2024
Merged

feat: migrate account state to trie#407
fmoletta merged 61 commits into
mainfrom
integrate-trie-part-two

Conversation

@fmoletta

@fmoletta fmoletta commented Sep 16, 2024

Copy link
Copy Markdown
Contributor

Motivation
Migrate handling of account state from DB tables into a state trie

Description

Refactor how we interact with account state across the codebase:

  • Replace apply_state_transitions with get_state_transitions, which returns a list of AccountUpdates and add the method Store::apply_account_updates which applies the changes on top of the given block's state and returns the new state root after the updates.
  • Remove methods that allowed modifying account state from Store (such as add_account, remove_account, add_account_info, etc)
  • Store methods that fetch account state data now receive a block_number argument so that historical state can be accessed

Other:

  • Fix value of withdrawals_root & parent_beacon_block_root for non-cancun Genesis
  • [Ef-Tests] initialize store from Genesis struct
  • Update rpc requests to look for historical data where applicable

Closes #272

@fmoletta fmoletta changed the title [WIP] migrate account state to trie feat: [WIP] migrate account state to trie Sep 16, 2024
@fmoletta fmoletta changed the title feat: [WIP] migrate account state to trie feat: migrate account state to trie Sep 17, 2024
@fmoletta fmoletta marked this pull request as ready for review September 17, 2024 15:32
@fmoletta fmoletta requested a review from a team as a code owner September 17, 2024 15:32

@mpaulucci mpaulucci left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Great work! 🚀 🚀 🚀

As a follow-up, I think we should make a cleaner api that what we have currently in Store, making the Trie just an internal representation that is not leaked to the outside (for example, in the future we might use snapshots to query the state)

For example, we could do something like:

let state_reader = store.get_state_at(block_number);
let info = state_reader.get_account_info(...);

But let's get this merged and then we can do a follow up.

Comment thread crates/evm/db.rs
pub struct StoreWrapper(pub Store);
pub struct StoreWrapper {
pub store: Store,
pub block_number: BlockNumber,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it makes more sense to store a block_hash here since we might want to execute a block that is not from the canonical chain. Thoughts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We only use this field to fetch account_info from the store, which is indexed by block_number, so changing the field to block hash wouldn't fix the issue.
I would leave it to a separate refactor

@fmoletta

Copy link
Copy Markdown
Contributor Author

Great work! 🚀 🚀 🚀

As a follow-up, I think we should make a cleaner api that what we have currently in Store, making the Trie just an internal representation that is not leaked to the outside (for example, in the future we might use snapshots to query the state)

For example, we could do something like:

let state_reader = store.get_state_at(block_number);
let info = state_reader.get_account_info(...);

But let's get this merged and then we can do a follow up.

I don't understand this, the methods exposing the trie are only part of the StorageEngine trait, only accessible inside the Store's implementation, no trie is accessible from Store's public api

@fmoletta fmoletta added this pull request to the merge queue Sep 17, 2024
Merged via the queue into main with commit de55848 Sep 17, 2024
@fmoletta fmoletta deleted the integrate-trie-part-two branch September 17, 2024 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hive Testing: eth_getBalance

3 participants