Skip to content

feat(trie): implement change set walker#9970

Closed
RomanHodulak wants to merge 2 commits into
paradigmxyz:mainfrom
RomanHodulak:romanhodulak/state-range-walker
Closed

feat(trie): implement change set walker#9970
RomanHodulak wants to merge 2 commits into
paradigmxyz:mainfrom
RomanHodulak:romanhodulak/state-range-walker

Conversation

@RomanHodulak
Copy link
Copy Markdown
Contributor

@RomanHodulak RomanHodulak commented Jul 31, 2024

About

This PR is about decoupling trait and implementation of an iterator over change sets in a transaction.

Motivation

Expanding on #9282 with a focus on state module of reth-trie crate.

Goals

  • Remove dependencies on reth-db and reth-db-api in state module of reth-trie crate.

Solution

  • Decouple trait and a database implementation of an iterator over a range of change sets of accounts and storage.
    • Create a new trait TrieRangeWalker and TrieRangeWalkerFactory
    • Implement above mentioned traits using a newly created DatabaseAccountChangeSetsCursor and DatabaseStorageChangeSetsCursor
    • Update all usages in reth-trie to use the trait only
    • Update all usages outside of reth-trie to supply the proper implementation

Additional notes

The newly created database implementations of TrieRangeWalker and TrieRangeWalkerFactory should be a part of reth-trie-db crate instead of reth-trie as done here. However, this is not a concern of this PR, instead will be a concern of a PR focusing on moving all database implementations of trie readers/writers to reth-trie-db. Mostly in favor of keeping the PRs small and focused as explained in #9585 motivation section.

Copy link
Copy Markdown
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.

nice, this is becoming more clear to me
lgtm

pending @rkrasiuk

@mattsse mattsse added the C-debt A clean up/refactor of existing code label Jul 31, 2024
@RomanHodulak
Copy link
Copy Markdown
Contributor Author

Hey @mattsse it's good to hear from you again, thanks for the quick feedback. I just got back from a week long break so I hope this effort didn't seem too abandoned!

Anywho, I'm glad that this looks more clear to you as I see clarity as important.

@mattsse
Copy link
Copy Markdown
Collaborator

mattsse commented Jul 31, 2024

that's great to hear,
we're getting a lot of requests for sp1 support which requires that some crates that currently depend on reth-trie types need to compile to wasm32-wasip1 and this is now blocker and slightly higher prio.
if you have the capacity to continue with this that'd be amazing

@shekhirin shekhirin added the A-trie Related to Merkle Patricia Trie implementation label Aug 1, 2024
Copy link
Copy Markdown
Member

@shekhirin shekhirin left a comment

Choose a reason for hiding this comment

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

LGTM, minor nits

Comment thread crates/trie/trie/src/trie_cursor/mod.rs Outdated
Comment thread crates/trie/trie/src/trie_cursor/mod.rs Outdated
Comment thread crates/trie/trie/src/trie_cursor/database_cursors.rs Outdated
@rkrasiuk
Copy link
Copy Markdown
Contributor

rkrasiuk commented Aug 1, 2024

@RomanHodulak thanks for the PR, however I do not see any good reason for abstracting away the transaction from PrefixSetLoader given that it's only ever used with database and used for incremental root (the concept that now exists only in reth-trie-db crate), i'll open a quick PR that moves the loader to reth-trie-db crate

@rkrasiuk
Copy link
Copy Markdown
Contributor

rkrasiuk commented Aug 1, 2024

@RomanHodulak jumped the gun and misread the changes

@rkrasiuk
Copy link
Copy Markdown
Contributor

rkrasiuk commented Aug 1, 2024

@RomanHodulak i see, this is for HashedPostState::from_reverts. Still no need to abstract it over factory, just similar to other traits please create the trait in reth-trie-db and implement this method there

pub trait DatabaseHashedState<'a, TX> {
    fn from_reverts(tx: &'a TX, from: BlockNumber) -> Result<Self, DatabaseError>; 
}

@RomanHodulak RomanHodulak changed the title feat(trie): implement trie range walker feat(trie): implement change set walker Aug 1, 2024
@RomanHodulak RomanHodulak force-pushed the romanhodulak/state-range-walker branch 2 times, most recently from cbf756c to 91ec1c2 Compare August 1, 2024 13:46
@rkrasiuk
Copy link
Copy Markdown
Contributor

rkrasiuk commented Aug 1, 2024

closed in favor of #9987

@rkrasiuk rkrasiuk closed this Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-trie Related to Merkle Patricia Trie implementation C-debt A clean up/refactor of existing code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants