fix: eth66 serve signature

[yiweichi]

This PR fix the issue reth doesn't include sig during block request, l2geth will fail the block validation of blocks requested from reth.
The solution is that we persist block signature to the database of rollup-node, retrieve and serve it during handling eth66 block request.

Corresponding issue: #329
Corresponding RN PR: scroll-tech/rollup-node#267

[codspeed-hq]

CodSpeed Performance Report

Merging #331 will not alter performance

_{Comparing morty/fix-eth66-serve-signature (0bdc3d7) with scroll (d485f47)}

Summary

✅ 77 untouched benchmarks

[greged93]

couple of comments, mostly related to perf.

[frisitano]

Left some refactoring suggestions inline

[frisitano]

Added some comments inline.

[frisitano]

Can we open an issue and add permalinks to all TODO items that we need to address once we have deprecated l2geth please?

[yiweichi]

Sure, will do this.

[frisitano]

Overview

I will articulate a way in which I propose that we remove the circular dependency.

Solution - Reth

Remove existing rollup-node deps.

Introduce signature provider trait:

#[async_trait::async_trait]
pub trait SignatureProvider {
    type Error: std::fmt::Debug;

    async fn get_signature(&self, hash: B256) -> Result<Option<Signature>, Self::Error>;

    async fn insert_signature(&self, hash: B256, signature: Signature) -> Result<(), Self::Error>;
}

Modify header transform types to be generic over the trait:

/// An implementation of a [`HeaderTransform`] for downloaded headers for Scroll.
#[derive(Debug, Clone)]
#[non_exhaustive]
pub struct ScrollHeaderTransform<ChainSpec, P> {
    chain_spec: ChainSpec,
    provider: Arc<P>,
    signer: Option<Address>,
}

impl<
        ChainSpec: EthChainSpec + ScrollHardforks + Debug + Send + Sync + 'static,
        P: SignatureProvider + Debug + Send + Sync + 'static,
    > ScrollHeaderTransform<ChainSpec, P>
{
    /// Returns a new instance of the [`ScrollHeaderTransform`] from the provider chain spec.
    pub const fn new(chain_spec: ChainSpec, provider: Arc<P>, signer: Option<Address>) -> Self {
        Self { chain_spec, provider, signer }
    }

    /// Returns a new [`ScrollHeaderTransform`] as a [`HeaderTransform`] trait object.
    pub fn boxed<H: BlockHeader>(
        chain_spec: ChainSpec,
        provider: Arc<P>,
        signer: Option<Address>,
    ) -> Box<dyn HeaderTransform<H>> {
        Box::new(Self { chain_spec, provider, signer })
    }
}

impl<
        H: BlockHeader,
        ChainSpec: EthChainSpec + ScrollHardforks + Debug + Send + Sync,
        P: SignatureProvider + Debug + Send + Sync + 'static,
    > HeaderTransform<H> for ScrollHeaderTransform<ChainSpec, P>
{
    fn map(&self, mut header: H) -> H {
        if !self.chain_spec.is_euclid_v2_active_at_timestamp(header.timestamp()) {
            return header;
        }
        // TODO: remove this once we deprecated l2geth
        // Validate and process signature

        if let Err(err) = self.validate_and_store_signature(&mut header, self.signer) {
            debug!(
                target: "scroll::network::response_header_transform",
                "Header signature persistence failed, header hash: {:?}, error: {}",
                header.hash_slow(), err
            );
        }

        header
    }
}

impl<
        ChainSpec: ScrollHardforks + Debug + Send + Sync,
        P: SignatureProvider + Debug + Send + Sync + 'static,
    > ScrollHeaderTransform<ChainSpec, P>
{
    fn validate_and_store_signature<H: BlockHeader>(
        &self,
        header: &mut H,
        authorized_signer: Option<Address>,
    ) -> Result<(), HeaderTransformError> {
        let signature_bytes = std::mem::take(header.extra_data_mut());
        let signature = parse_65b_signature(&signature_bytes)?;

        // Recover and verify signer
        recover_and_verify_signer(&signature, header.hash_slow(), authorized_signer)?;

        // Store signature in database
        persist_signature(self.provider.clone(), header.hash_slow(), signature);

        Ok(())
    }
}

/// An implementation of a [`HeaderTransform`] for header request responses for Scroll.
#[derive(Debug, Clone)]
#[non_exhaustive]
pub(crate) struct ScrollRequestHeaderTransform<ChainSpec, P> {
    chain_spec: ChainSpec,
    provider: Arc<P>,
    signer: Option<Address>,
}

impl<
        H: BlockHeader,
        ChainSpec: EthChainSpec + ScrollHardforks + Debug + Send + Sync,
        P: SignatureProvider + Debug + Send + Sync,
    > HeaderTransform<H> for ScrollRequestHeaderTransform<ChainSpec, P>
{
    fn map(&self, mut header: H) -> H {
        if !self.chain_spec.is_euclid_v2_active_at_timestamp(header.timestamp()) {
            return header;
        }

        // read the signature from the rollup node.
        let signature = tokio::task::block_in_place(|| {
            if let Ok(handle) = tokio::runtime::Handle::try_current() {
                match handle
                    .block_on(async { self.provider.get_signature(header.hash_slow()).await })
                {
                    Ok(sig) => sig,
                    Err(e) => {
                        warn!(target: "scroll::network::request_header_transform",
                            "Failed to get block signature from database, header hash: {:?}, error: {}",
                            header.hash_slow(),
                            HeaderTransformError::DatabaseError(e.to_string())
                        );
                        None
                    }
                }
            } else {
                warn!(
                    target: "scroll::network::request_header_transform",
                    "Failed to get block signature from database, header hash: {:?}, error: {}",
                    header.hash_slow(),
                    HeaderTransformError::NoRuntimeAvailable
                );
                None
            }
        });

        // If we have a signature in the database and it matches configured signer then add it
        // to the extra data field
        if let Some(sig) = signature {
            if let Err(err) = recover_and_verify_signer(&sig, header.hash_slow(), self.signer) {
                warn!(
                target: "scroll::network::request_header_transform",
                    "Found invalid signature(different from the hardcoded signer) for header hash: {:?}, sig: {:?}, error: {}",
                    header.hash_slow(),
                    sig.to_string(),
                    err
                );
            } else {
                *header.extra_data_mut() = sig.as_bytes().into();
            }
        }

        header
    }
}

/// Recover signer from signature and verify authorization.
fn recover_and_verify_signer(
    signature: &Signature,
    hash: B256,
    authorized_signer: Option<Address>,
) -> Result<Address, HeaderTransformError> {
    // Recover signer from signature
    let signer = reth_primitives_traits::crypto::secp256k1::recover_signer(signature, hash)
        .map_err(|_| HeaderTransformError::RecoveryFailed)?;

    // Verify signer is authorized
    if Some(signer) != authorized_signer {
        return Err(HeaderTransformError::InvalidSigner(signer));
    }

    Ok(signer)
}

/// Parse a canonical 65-byte secp256k1 signature: r (32) | s (32) | v (1).
fn parse_65b_signature(bytes: &[u8]) -> Result<Signature, HeaderTransformError> {
    if bytes.len() != 65 {
        return Err(HeaderTransformError::InvalidSignature);
    }

    let signature =
        Signature::from_raw(bytes).map_err(|_| HeaderTransformError::InvalidSignature)?;

    Ok(signature)
}

/// Run the async DB insert from sync code safely.
fn persist_signature<P: SignatureProvider + Send + Sync + 'static>(
    provider: Arc<P>,
    hash: B256,
    signature: Signature,
) {
    tokio::spawn(async move {
        trace!(
            "Persisting block signature to database, block hash: {:?}, sig: {:?}",
            hash,
            signature.to_string()
        );
        if let Err(e) = provider.insert_signature(hash, signature).await {
            warn!(target: "scroll::network::header_transform", "Failed to store signature in database: {:?}", e);
        }
    });
}

Revert current changes in reth on the ScrollNetworkBuilder.

Solution - Rollup Node

Implement SignatureProvider on Database.

Copy the existing version of ScrollNetworkBuilder from this PR (which we should revert in this PR) into the rollup-node repository in the node crate.

/// The network builder for Scroll.
#[derive(Debug, Default)]
pub struct ScrollNetworkBuilder {
    /// Additional `RLPx` sub-protocols to be added to the network.
    scroll_sub_protocols: RlpxSubProtocols,
    /// A reference to the rollup-node `Database`.
    rollup_node_db_path: Option<PathBuf>,
    /// The address for which we should persist and serve signatures for.
    signer: Option<Address>,
}

impl ScrollNetworkBuilder {
    /// Create a new [`ScrollNetworkBuilder`] with default configuration.
    pub fn new() -> Self {
        Self {
            scroll_sub_protocols: RlpxSubProtocols::default(),
            rollup_node_db_path: None,
            signer: None,
        }
    }

    /// Add a scroll sub-protocol to the network builder.
    pub fn with_sub_protocol(mut self, protocol: RlpxSubProtocol) -> Self {
        self.scroll_sub_protocols.push(protocol);
        self
    }

    /// Add a scroll sub-protocol to the network builder.
    pub fn with_database_path(mut self, db_path: Option<PathBuf>) -> Self {
        self.rollup_node_db_path = db_path;
        self
    }

    /// Set the signer for which we will persist and serve signatures if included in block header
    /// extra data field.
    pub const fn with_signer(mut self, signer: Option<Address>) -> Self {
        self.signer = signer;
        self
    }
}

impl<Node, Pool> NetworkBuilder<Node, Pool> for ScrollNetworkBuilder
where
    Node:
        FullNodeTypes<Types: NodeTypes<ChainSpec = ScrollChainSpec, Primitives = ScrollPrimitives>>,
    Pool: TransactionPool<
            Transaction: PoolTransaction<
                Consensus = TxTy<Node::Types>,
                Pooled = scroll_alloy_consensus::ScrollPooledTransaction,
            >,
        > + Unpin
        + 'static,
{
    type Network = NetworkHandle<ScrollNetworkPrimitives>;

    async fn build_network(
        self,
        ctx: &BuilderContext<Node>,
        pool: Pool,
    ) -> eyre::Result<Self::Network> {
        // initialize the rollup node database.
        let db_path = ctx.config().datadir().db();
        let database_path = if let Some(database_path) = self.rollup_node_db_path {
            database_path.to_string_lossy().to_string()
        } else {
            // append the path using strings as using `join(...)` overwrites "sqlite://"
            // if the path is absolute.
            let path = db_path.join("scroll.db?mode=rwc");
            "sqlite://".to_string() + &*path.to_string_lossy()
        };
        let db = Arc::new(Database::new(&database_path).await?);

        // get the header transform.
        let chain_spec = ctx.chain_spec();
        let transform = ScrollHeaderTransform {
            chain_spec: chain_spec.clone(),
            db: db.clone(),
            signer: self.signer,
        };
        let request_transform =
            ScrollRequestHeaderTransform { chain_spec, db: db.clone(), signer: self.signer };

        // set the network mode to work.
        let config = ctx.network_config()?;
        let config = NetworkConfig {
            network_mode: NetworkMode::Work,
            header_transform: Box::new(transform),
            extra_protocols: self.scroll_sub_protocols,
            ..config
        };

        let network = NetworkManager::builder(config).await?;
        let handle = ctx.start_network(network, pool, Some(Box::new(request_transform)));
        info!(target: "reth::cli", enode=%handle.local_node_record(), "P2P networking initialized");
        Ok(handle)
    }
}

Modify the node builder to override the network builder with the network builder that you introduce in the rollup-node create here.

[frisitano]

Added some comments inline.

[frisitano]

Instead of passing the request_transform here, can't we use the same pattern as what we do with the other header transform, where we add it to the NetworkConfig or does that not work?