feat: consensus #68

greged93 · 2025-04-23T16:56:09Z

This PR implements changes related to the consensus verifications:

Add a valid implementation of the PoA consensus.
Add a dev CLI argument that for now allows to run the node without any consensus checks.
Fixes an issue with the update of the FCS.

Resolves #60

greged93 · 2025-04-24T07:10:57Z

crates/engine/src/driver.rs

@@ -65,7 +65,7 @@ where



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

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?

frisitano · 2025-04-24T09:25:23Z

bin/rollup/src/network.rs

+        let chain = ctx.config().chain.clone();
+        let fcs = if let Some(named) = chain.chain.named() {


NamedChain is Copy so I don't think we need the clone here.

Suggested change

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() {

nice catch, didn't notice if was Copy.

frisitano · 2025-04-24T09:34:45Z

crates/node/src/lib.rs

@@ -64,7 +63,7 @@ pub struct RollupNodeManager<C, EC, P, L1P, L1MP, CS> {
    /// An indexer used to index data for the rollup node.
    indexer: Indexer<CS>,
    /// The consensus algorithm used by the rollup node.
-    consensus: C,
+    consensus: Box<dyn Consensus + Send>,


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>; }

Suggested change

consensus: Box<dyn Consensus + Send>,

consensus: Box<dyn Consensus>,

frisitano · 2025-04-24T09:35:03Z

crates/node/src/lib.rs

@@ -85,7 +84,7 @@ impl<C: Debug, EC: Debug, P: Debug, L1P: Debug, L1MP: Debug, CS: Debug> Debug
            .field("derivation_pipeline", &self.derivation_pipeline)
            .field("l1_notification_rx", &self.l1_notification_rx)
            .field("indexer", &self.indexer)
-            .field("consensus", &self.consensus)
+            .field("consensus", &"Consensus")


If Consensus is Debug then we can revert this.

frisitano · 2025-04-24T09:45:00Z

crates/scroll-wire/src/protocol/proto.rs

@@ -30,7 +32,7 @@ pub struct NewBlock {
 impl NewBlock {
    /// 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 }


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.

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

Yes that's a good idea. Would you be able to open an issue upstream for this please?

frisitano

Please update README to reflect new CLI args before merge.

frisitano · 2025-04-24T10:19:03Z

bin/rollup/src/args.rs

+    /// Whether the rollup node should be run in dev mode.
+    #[arg(long)]
+    pub dev: bool,


Does this conflict with reth's dev CLI arg?

yes sorry, I changed it to test but it reverted for some reason.

frisitano · 2025-04-24T15:44:02Z

bin/rollup/src/network.rs

+        };
+
+        // Create the consensus.
+        let consensus: Box<dyn Consensus + Send> = if self.config.test {


I think Send can be removed here.

oups indeed

greged93 added 7 commits April 22, 2025 19:11

feat: replace secp256 with alloy_primitives

f2d84c9

feat: implement PoAConsensus

5c686e7

Merge branch 'main' into feat/consensus

c703555

feat: initialize the consensus engine

a0582c6

feat: add a dev mode to the rollup node along with a NoopConsensus

c1f66b1

fix: incorrect fcs update

5c07e55

fix: lints

b816c54

greged93 mentioned this pull request Apr 23, 2025

feat: integrate sequencer with node #65

Merged

Merge branch 'main' into feat/consensus

337e423

greged93 commented Apr 24, 2025

View reviewed changes

greged93 requested a review from frisitano April 24, 2025 07:11

greged93 added 2 commits April 24, 2025 11:17

Merge branch 'main' into feat/consensus

ff44868

fix: merge

550eb7b

frisitano approved these changes Apr 24, 2025

View reviewed changes

frisitano mentioned this pull request Apr 24, 2025

[Primitives] Make NewBlock::signature Typed #71

Open

frisitano reviewed Apr 24, 2025

View reviewed changes

greged93 added 2 commits April 24, 2025 13:27

chore: revert args changes

9be380f

fix: address comments

e0516c5

frisitano reviewed Apr 24, 2025

View reviewed changes

fix: address comment

e80153d

greged93 merged commit 80b108b into main Apr 24, 2025
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: consensus #68

feat: consensus #68

greged93 commented Apr 23, 2025 •

edited

Loading

greged93 Apr 24, 2025 •

edited

Loading

frisitano left a comment

frisitano Apr 24, 2025

greged93 Apr 24, 2025

frisitano Apr 24, 2025

frisitano Apr 24, 2025

frisitano Apr 24, 2025

frisitano Apr 24, 2025

greged93 Apr 24, 2025

frisitano Apr 24, 2025

greged93 Apr 24, 2025

frisitano left a comment

frisitano Apr 24, 2025

greged93 Apr 24, 2025

frisitano Apr 24, 2025

greged93 Apr 24, 2025

		let chain = ctx.config().chain.clone();
		let fcs = if let Some(named) = chain.chain.named() {

	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() {

	consensus: Box<dyn Consensus + Send>,
	consensus: Box<dyn Consensus>,

feat: consensus #68

feat: consensus #68

Conversation

greged93 commented Apr 23, 2025 • edited Loading

greged93 Apr 24, 2025 • edited Loading

Choose a reason for hiding this comment

frisitano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frisitano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greged93 commented Apr 23, 2025 •

edited

Loading

greged93 Apr 24, 2025 •

edited

Loading