-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(stage-extensions): New abstraction for non-core stages and tables #4325
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice start. Need to think about this a little more, definitely directionally supportive of the custom stages, and thinking about what the best way to integrate would be.
Maybe one idea for you is to try to configure the abstraction via the CLI like https://github.com/jacobkaufmann/evangelion/blob/x-reth-builder/src/main.rs#L57, so we can experiment with how one can define custom stages OUTSIDE the main node/mode.rs
, when they'd import most of the stuff for the node and want to just add an extra stage or two.
inner_env.set_max_dbs(Tables::ALL.len()); | ||
inner_env.set_max_dbs(max_tables); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Nice! @shekhirin @onbjerg @rkrasiuk we've talked about this before, lifts the limit from having a comp-time defined set of stages
Started to play around with the |
@onbjerg if you have some time - could you please give a vibecheck on the overall design here? |
pub struct MyNamespace<'a, DB: Database> { | ||
/// A reference to the reth database. Used by MyNamespace to construct responses. | ||
database: Arc<DatabaseProviderRO<'a, &'a DB>>, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current uncertainty around my approach here. The goal is to provide any method with the ability to access the database, as shown with my_method()
. Have not yet worked out the exact lifetimes mechanism have the database as a member here. Perhaps a different design is possible
I've just updated the summary describing the design |
Thanks for taking a stab at this! I think there are a few unresolved things with this approach
Do you have some ideas for point 2? |
Ok that is interesting. From the docs ( Then, the Pipeline::run function is called, which starts the pipeline,
executing all of the stages continuously in an infinite loop.
This process syncs the chain, keeping everything up to date with the chain tip. I took this to mean that the stages restarted, looping as new blocks arrive: ## initial sync
A (genesis to tip) -> B (genesis to tip) -> C (genesis to tip) ...
## maintenance
- Block `n`: A -> B -> C ...,
- Block `n + 1`: A -> B -> C ...,
- Block `n + 2`: A -> B -> C ..., I'll look more closely to understand better how it actually works |
PTAL at the |
Here would be one approach I could see:
|
Hit an impasse with this approach as introducing generics in that struct was infeasible. Have played with a few other approaches. Nothing compelling identified yet |
Closing this as stale, we'll re-incorporate these ideas as we venture into custom stages with Reth CLI Extensions and the Declarative Node Builder API that @mattsse is spearheading. |
Appreciate you takign the tiem on these @perama-v, it's effort that we'll take into account seriously as I think this is a good idea |
Thanks for the high level of engagement and openness in my exploration of this. I'm happy for it to close. |
Description
Progress toward
Changes made
Adds a mechanism to add new stages and tables with minimal footprint on existing crates.
As any new stage is likely to be used via a custom endpoint, changes are introduced as extensions to the existing
MyRethCliExt
.Composed of:
RethNodeCommandConfig
trait.&mut
rather thanmut
RethNodeCommandConfig
methodextend_rpc_modules()
:.start_servers(blockchain_db.clone(), ....
and is used via provider traits.DatabaseProvider
(crates/storage/provider/src/traits/database.rs
). The intuition is: if already passing the database, can one grant generic database read access via this provider trait. This includes all future methods and core/non-core tables.tables!
methods under aTableMetadata
trait so that non-core tables can be passed generically via that trait.The main changes to
./bin/reth/src/node.mod.rs
are as follows:Stages: After adding core Reth stages, the builder is passed to add a new stage(s).
Tables: Before the database is created, new non-core tables are fetched and passed for creation after core table creation.
Discussion
Presented for conceptual feedback. In the main tracking issue, it was raised that some new abstractions are likely required to enable additional indices. This PR is an attempt at such an abstraction.
The implementation is incomplete, and the intention is to see if the idea is a good direction before proceeding more. Perhaps there are obvious flaws or preferred alternative directions?
Apologies for touching so many files: many are just involved on the way to get the number of optional new tables to the
set_max_dbs(max_tables)
function.