feat: consensus#68
Conversation
| @@ -65,7 +65,7 @@ where | |||
|
|
|||
There was a problem hiding this comment.
I know we talked about reintroducing these lines of code:
if self.forkchoice_state.is_genesis() {
let block_num_hash = block.parent_num_hash();
self.forkchoice_state = ForkchoiceState::from_block_info(BlockInfo {
number: block_num_hash.number,
hash: block_num_hash.hash,
});
}and I tried running the node with them, but I didn't see any trigger of the pipeline sync. I think what happened is that it hits this path in the match, meaning it doesn't trigger a sync. My opinion is to keep the code as is, and implement a EL sync and CL sync similar to what optimism are doing.
EDIT: forgot to link the code in Reth.
https://github.com/scroll-tech/reth/blob/1b8ff78f46d5e95dd10876fd8145c27db598a0b0/crates/engine/tree/src/tree/mod.rs#L2075
frisitano
left a comment
frisitano
left a comment
There was a problem hiding this comment.
Looks good. Left some minor nits inline. Can you open an issue so that we can update the logic to track signer rotation on L1 system contract please?
| let chain = ctx.config().chain.clone(); | ||
| let fcs = if let Some(named) = chain.chain.named() { |
There was a problem hiding this comment.
NamedChain is Copy so I don't think we need the clone here.
| let chain = ctx.config().chain.clone(); | |
| let fcs = if let Some(named) = chain.chain.named() { | |
| let fcs = if let Some(named) = ctx.config().chain.chain.named() { |
There was a problem hiding this comment.
nice catch, didn't notice if was Copy.
| indexer: Indexer<CS>, | ||
| /// The consensus algorithm used by the rollup node. | ||
| consensus: C, | ||
| consensus: Box<dyn Consensus + Send>, |
There was a problem hiding this comment.
Should we constrain the Consensus trait to be Send + Debug instead?
pub trait Consensus: std::fmt::Debug + Send {
/// Validates a new block with the given signature.
fn validate_new_block(
&self,
block: &ScrollBlock,
signature: &Signature,
) -> Result<(), ConsensusError>;
}| consensus: Box<dyn Consensus + Send>, | |
| consensus: Box<dyn Consensus>, |
| .field("l1_notification_rx", &self.l1_notification_rx) | ||
| .field("indexer", &self.indexer) | ||
| .field("consensus", &self.consensus) | ||
| .field("consensus", &"Consensus") |
There was a problem hiding this comment.
If Consensus is Debug then we can revert this.
| /// Returns a [`NewBlock`] instance with the provided signature and block. | ||
| pub fn new(signature: Signature, block: reth_scroll_primitives::ScrollBlock) -> Self { | ||
| Self { signature: Bytes::from(signature.serialize_compact().to_vec()), block } | ||
| Self { signature: Bytes::from(Into::<Vec<u8>>::into(signature)), block } |
There was a problem hiding this comment.
This is fine for now but in the future I would like to have a solution that ensures correct typing such that we have:
/// A message that is used to announce a new block to the network.
#[derive(Clone, Debug, PartialEq, Eq, RlpEncodable, RlpDecodable)]
pub struct NewBlock {
/// The signature from the block author.
pub signature: Signature,
/// The block that is being announced.
pub block: reth_scroll_primitives::ScrollBlock,
}The reason we currently don't have this is because Signature does not implement Encodable. We may be able to address this by implementing Encodable ourselves on this type instead of leveraging the macro but I'm not sure. I will open an issue.
There was a problem hiding this comment.
it seems like the implementation of Encodable and Decodable was removed in this PR. Maybe we can ask for it to be reintroduced?
alloy-rs/core@37e08b2#diff-79b82cfeeba5106f10ab06fb251c628206b2d8f648bcd94e6fe01c6091763d1c
There was a problem hiding this comment.
Yes that's a good idea. Would you be able to open an issue upstream for this please?
frisitano
left a comment
frisitano
left a comment
There was a problem hiding this comment.
Please update README to reflect new CLI args before merge.
| /// Whether the rollup node should be run in dev mode. | ||
| #[arg(long)] | ||
| pub dev: bool, |
There was a problem hiding this comment.
Does this conflict with reth's dev CLI arg?
There was a problem hiding this comment.
yes sorry, I changed it to test but it reverted for some reason.
| }; | ||
|
|
||
| // Create the consensus. | ||
| let consensus: Box<dyn Consensus + Send> = if self.config.test { |
There was a problem hiding this comment.
I think Send can be removed here.
2 participants
This PR implements changes related to the consensus verifications:
Resolves #60