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

Add StateProviderFactory custom trait #331

Merged
merged 18 commits into from
Jan 9, 2025

Conversation

ferranbt
Copy link
Contributor

@ferranbt ferranbt commented Jan 1, 2025

πŸ“ Summary

πŸ’‘ Motivation and Context


βœ… I have completed the following steps:

  • Run make lint
  • Run make test
  • Added tests (if applicable)

Copy link

github-actions bot commented Jan 2, 2025

Benchmark results for 94d042e

Report: https://flashbots-rbuilder-ci-stats.s3.us-east-2.amazonaws.com/benchmark/94d042e-26db62d/report/index.html

Date (UTC) 2025-01-09T16:55:46+00:00
Commit 94d042e248ab66754a19c3434b26ffed13bfe8d2
Base SHA 26db62de834397c04fbe105f900e393bb6d28f87

Significant changes

Benchmark Mean Status
reference_trie_insert_and_hash_3000 2.58% Performance has degraded.
hashing_3000_elements 2.88% Performance has degraded.

@ferranbt ferranbt marked this pull request as ready for review January 2, 2025 09:16
Copy link
Contributor

@liamaharon liamaharon left a comment

Choose a reason for hiding this comment

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

Nice

@@ -0,0 +1,43 @@
use crate::live_builder::simulation::SimulatedOrderCommand;
Copy link
Contributor

Choose a reason for hiding this comment

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

Doc comment would be nice


pub mod reth_prov;

pub trait StateProviderFactory: Clone + 'static + Send + Sync {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we name this something different so it doesn't get confused with the reth StateProviderFactory? Maybe RbuilderProvider?


pub mod reth_prov;

pub trait StateProviderFactory: Clone + 'static + Send + Sync {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add comments!!!!!


pub mod reth_prov;

pub trait StateProviderFactory: Clone + 'static + Send + Sync {
Copy link
Contributor

Choose a reason for hiding this comment

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

Clone + 'static doesn't seem right, it feels more like specific use constrains you brought to the trait.

Comment on lines 18 to 23
fn run_trie_prefetcher(
&self,
parent_hash: B256,
simulated_orders: broadcast::Receiver<SimulatedOrderCommand>,
cancel: CancellationToken,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Comments!

Comment on lines 37 to 42
fn calculate_state_root(
&self,
parent_hash: B256,
outcome: &ExecutionOutcome,
config: RootHashConfig,
) -> Result<B256, RootHashError>;
Copy link
Contributor

Choose a reason for hiding this comment

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

If feels like the specifics of roothash calculation should be hidden (eg: RootHashConfig) inside the specific implementation and not in the trait (and they are probably going to be constant during the while execution!)

@ZanCorDX
Copy link
Contributor

ZanCorDX commented Jan 2, 2025

All the Default::default() for the SparseTrieSharedCache broke the whole caching thing. This must be carefully rethought or it will not work.

Copy link
Contributor

@ZanCorDX ZanCorDX left a comment

Choose a reason for hiding this comment

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

We must fix the roothash!

@@ -169,23 +168,47 @@ impl BaseConfig {
))
}

/// WARN: opens reth db
pub async fn create_builder<SlotSourceType>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this method in #330

@@ -0,0 +1,77 @@
use super::OrderInputConfig;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file was removed in #327?

@ZanCorDX ZanCorDX merged commit be063d9 into develop Jan 9, 2025
4 checks passed
@ZanCorDX ZanCorDX deleted the ferranbt/state-provider-factory-trait branch January 9, 2025 17:16
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.

3 participants