feat: Full node mode #13
Conversation
| @@ -27,6 +27,8 @@ mod tests; | |||
| pub use crate::config::Config; | |||
| use crate::peers::PeerStates; | |||
|
|
|||
| // FIXME(slowli): when run on validator node, the actor creates unnecessary `GetBlocks` requests | |||
There was a problem hiding this comment.
I'd prefer to create a separate PR to fix this.
| replica_state_store: Arc<dyn ReplicaStateStore>, | ||
| blocks_sender: channel::UnboundedSender<FinalBlock>, |
There was a problem hiding this comment.
These options could be made more fluent (e.g., if S implements ReplicaStateStore, it doesn't need to be provided again, and "standard" blocks_senders could be considered (e.g., one that saves a block to the store, or one that does nothing). I've decided against this to make code simpler.
| } | ||
|
|
||
| impl StateMachine { | ||
| /// Creates a new StateMachine struct. We try to recover a past state from the storage module, | ||
| /// otherwise we initialize the state machine with whatever head block we have. | ||
| pub(crate) async fn new( | ||
| ctx: &ctx::Ctx, | ||
| storage: Arc<dyn ReplicaStateStore>, | ||
| storage: FallbackReplicaStateStore, |
There was a problem hiding this comment.
why "fallback"? It is supposed to be the primary source of state, with empty state being the fallback, no?
There was a problem hiding this comment.
or you mean WithFallback? But then do we need that verbosity?
There was a problem hiding this comment.
Yeah, I've meant "store with fallback" (fallback being based on a BlockStore). Would something like FullReplicaStateStore work better?
There was a problem hiding this comment.
I guess that would be better. But why not just ReplicaStateStore? The fallback is an implementation detail from the pov of this function. Perhaps we should make FallbackReplicaStateStore into a trait instead of ReplicaStateStore?
| optional string metrics_server_addr = 2; // IpAddr | ||
|
|
||
| // Consensus network config. | ||
| optional ConsensusConfig consensus = 3; |
There was a problem hiding this comment.
while you are at it please annotate all fields in this file as either optional or required (by just suffixing the field definition with a comment: "// required" or "// optional".
node/Cargo.toml
Outdated
| @@ -26,7 +26,7 @@ assert_matches = "1.5.0" | |||
| async-trait = "0.1.71" | |||
| bit-vec = "0.6" | |||
| blst = "0.3.10" | |||
| clap = { version = "4.3.3", features = ["derive"] } | |||
| clap = { version = "4.3.3", features = ["derive", "env"] } | |||
There was a problem hiding this comment.
IMO we can drop support for specifying stuff via env vars. We don't use it and we will use eventually the zksync-era style configuration anyway.
node/actors/executor/src/lib.rs
Outdated
| .iter() | ||
| .any(|validator_key| validator_key == public), | ||
| "Key {public:?} is not a validator per validator set {:?}", | ||
| self.executor_config.validators |
There was a problem hiding this comment.
Having a validator config, even though you are not a validator currently should be a valid setup. Once the validator set is dynamic, a node should be able to join and leave the consensus network. For now it would be enough to just ignore the ConsensusNetwork config if node is not a validator.
What ❔
Allows to run a node without the consensus module. Refactors the
executorlibrary to be usable as a library and moves its binary to thetoolscrate.Why ❔
We need to be able to use
executoras a library from the consensus code; including when running an external node.