Skip to content

feat: Make DatabaseStateRoot stateful #15760

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

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

Conversation

iTranscend
Copy link
Contributor

This PR

  • converts DatabaseStateRoot into a regular stateful trait that takes in &self.
  • moves the from_tx method to a new trait StateRootFromTx implemented by StateRoot
  • transfers the responsibility of managing a tx ref to the trait implementer (StateRoot in this case)

resolves: #15478

@github-project-automation github-project-automation bot moved this to Backlog in Reth Tracker Apr 16, 2025
@iTranscend iTranscend changed the title Make DatabaseStateRoot stateful feat: Make DatabaseStateRoot stateful Apr 16, 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.

this seems fine, but unsure if this is what @Rjected had in mind

@jenpaff jenpaff moved this from Backlog to In Review in Reth Tracker Apr 18, 2025
Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

the changes to DatabaseStateRoot look fine, but I have some other suggestions that should make using StateRoot simpler

Comment on lines +22 to +26
/// Trait for creating a new [`StateRoot`] instance from a database transaction.
pub trait StateRootFromTx<'a, TX> {
/// Create a new [`StateRoot`] instance.
fn from_tx(tx: &'a TX) -> Self;
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a trait for this? I am not sure that we do, we should just be able to implement from_tx on the things that implement DatabaseStateRoot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding from_tx to the things that implement DatabaseStateRoot (in this case StateRoot) introduces a cyclic dependency between the reth-trie and the reth-trie-db crates.

If we want from_tx implemented on StateRoot, we'll probably need to separate out the cursor_factorys into a separate crate

@github-project-automation github-project-automation bot moved this from In Review to In Progress in Reth Tracker Apr 22, 2025
@iTranscend iTranscend requested a review from gakonst as a code owner April 25, 2025 17:15
Copy link

codspeed-hq bot commented Apr 25, 2025

CodSpeed Performance Report

Merging #15760 will improve performances by 11.68%

Comparing iTranscend:feat/make-database-state-root-stateful (c5e7fa8) with main (82d6505)

Summary

⚡ 1 improvements
✅ 76 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
hash builder[init size 10000 | update size 100 | num updates 1] 10.2 ms 9.1 ms +11.68%

@iTranscend iTranscend requested review from Rjected and mattsse April 25, 2025 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Make DatabaseStateRoot stateful
3 participants