feat: add OptimismNode type #6610
Conversation
There was a problem hiding this comment.
components are all services, between which components is control flow based on communication (message passing model) or locks (shared memory model)? is control centralised from the type that impl engine trait, or which components can work together without the engine? which component depends on which, can the components in ATs in NodeTypes be run on their own or are they all interdependent? do they need to be started in a particular order? some more docs on control flow, coupling (and cohesion) would be nice.
some more docs on how components communicate with each other and with the network for components that communicate with the network, would be very helpful, e.g. is the communication uni-directional or bidirectional, is the flow of messages a DAG or cyclic? what types of messages are being sent/received, on which channels/sockets?
| impl NodeTypes for OptimismNode { | ||
| type Primitives = (); | ||
| type Engine = OptimismEngineTypes; |
There was a problem hiding this comment.
would be faster to get an idea of how the type system is designed here, if traits have imperative verbs as names and types have nouns as names. so maybe ConfigureNodeAssociatedTypes or ConfigureNodeATs. since we make the abbreviation GATs for generic associated types, ATs shouldn't be to hard to figure out for a rust dev.
| /// This contains various settings that can be configured and take precedence over the node's | ||
| /// config. |
There was a problem hiding this comment.
ok cool, so it's like...the node's config is the /etc/config, and the OptimismPoolBuilder settings is the ~/config.
| reth_transaction_pool::maintain::LocalTransactionBackupConfig::with_local_txs_backup(transactions_path); | ||
|
|
||
| ctx.task_executor().spawn_critical_with_graceful_shutdown_signal( | ||
| "local transactions backup task", |
There was a problem hiding this comment.
it's a backup of the pool, so it will have access too all the pools data behind some lock, ready to take over if the first thread crashes? it's not a load balancer right, that will kick in when the first thread is under too big load without it crashing? just double checking the naming is convenient here
There was a problem hiding this comment.
@emhane this backsup local transactions on disk so we can recover them when we restart the node
There was a problem hiding this comment.
ok cool, please add a doc that I has to do with backing up txns to disk
crates/node-optimism/src/node.rs
Outdated
| /// A basic ethereum payload service. | ||
| #[derive(Debug, Default, Clone)] | ||
| #[non_exhaustive] | ||
| pub struct EthereumPayloadBuilder; |
There was a problem hiding this comment.
what do you actually mean by payload builder? it builds transactions? and it's spawned in a thread, where does it return the payload once it's built, via a callback to the initiator of the job? or is sending a command to this service the entry point to the p2p? some more docs would be good.
There was a problem hiding this comment.
this sits behind the engine API, if the CL tells us to build a payload (block) this type is responsible for building one and serves it to the CL if it requests the payload
the default impl will use the mempool to build a new payload
Co-authored-by: Emilia Hane <[email protected]>
crates/node-optimism/src/node.rs
Outdated
| /// A basic ethereum payload service. | ||
| #[derive(Debug, Default, Clone)] | ||
| #[non_exhaustive] | ||
| pub struct EthereumPayloadBuilder; |
There was a problem hiding this comment.
why is this called EthereumPayloadBuilder, but then it creates the optimism payload builder in spawn_payload_service?
mattsse
force-pushed
the
matt/add-opnode-type
branch
from
b77d731 to
74f87a2
Compare
There was a problem hiding this comment.
lgtm so far, pending one question about compute_pending_block. I also wonder if we can just consolidate the reth_optimism_payload_builder::OptimismPayloadBuilder and reth_node_builder::OptimismPayloadBuilder types. With these new traits and types we're seeing a lot of overloaded names, it's worth thinking about and improving in followups
| ctx: &BuilderContext<Node>, | ||
| pool: Pool, | ||
| ) -> eyre::Result<PayloadBuilderHandle<Node::Engine>> { | ||
| let payload_builder = reth_optimism_payload_builder::OptimismPayloadBuilder::default(); |
There was a problem hiding this comment.
do we need to define compute_pending_block in the above OptimismPayloadBuilder?
more builder prep