From a82f075af77b6aa7763d7c8d58811dbeb8aa3fc3 Mon Sep 17 00:00:00 2001 From: Jeb Bearer Date: Tue, 7 May 2024 11:48:17 -0400 Subject: [PATCH] Clean up signature recovery * Use fee message defined in HotShot trait, rather than constructing it ourselves, to avoid accidentally going out of sync with how the builder is signing * Fail signature recovery if the recovered address is wrong, rather than proceeding with a random address --- builder/src/non_permissioned.rs | 1 - sequencer/src/eth_signature_key.rs | 8 ++- sequencer/src/header.rs | 95 ++++++++++++------------------ sequencer/src/state.rs | 29 +++++++-- 4 files changed, 67 insertions(+), 66 deletions(-) diff --git a/builder/src/non_permissioned.rs b/builder/src/non_permissioned.rs index 420bc43ffe..9e35e81446 100644 --- a/builder/src/non_permissioned.rs +++ b/builder/src/non_permissioned.rs @@ -118,7 +118,6 @@ impl BuilderConfig { let builder_commitment = genesis_payload.builder_commitment(&genesis_ns_table); let vid_commitment = { - // TODO we should not need to collect payload bytes just to compute vid_commitment let payload_bytes = genesis_payload .encode() .expect("unable to encode genesis payload"); diff --git a/sequencer/src/eth_signature_key.rs b/sequencer/src/eth_signature_key.rs index 6c935235d7..da822372bc 100644 --- a/sequencer/src/eth_signature_key.rs +++ b/sequencer/src/eth_signature_key.rs @@ -112,9 +112,11 @@ impl Ord for EthKeyPair { #[derive(Clone, Debug, Snafu)] pub struct SigningError; +pub type BuilderSignature = Signature; + impl BuilderSignatureKey for FeeAccount { type BuilderPrivateKey = EthKeyPair; - type BuilderSignature = Signature; + type BuilderSignature = BuilderSignature; type SignError = SigningError; fn validate_builder_signature(&self, signature: &Self::BuilderSignature, data: &[u8]) -> bool { @@ -194,12 +196,12 @@ mod tests { let sig = FeeAccount::sign_builder_message(&key, msg).unwrap(); assert!(key.fee_account().validate_builder_signature(&sig, msg)); - // Recovery fails if signed with other key + // Validation fails if signed with other key. let other_key = FeeAccount::generated_from_seed_indexed([0u8; 32], 1).1; let sig = FeeAccount::sign_builder_message(&other_key, msg).unwrap(); assert!(!key.fee_account().validate_builder_signature(&sig, msg)); - // Recovery fails if another message was signed + // Validation fails if another message was signed let sig = FeeAccount::sign_builder_message(&key, b"hello world XYZ").unwrap(); assert!(!key.fee_account().validate_builder_signature(&sig, msg)); } diff --git a/sequencer/src/header.rs b/sequencer/src/header.rs index 9c9c7cff12..502f5c7ba4 100644 --- a/sequencer/src/header.rs +++ b/sequencer/src/header.rs @@ -1,20 +1,21 @@ use crate::{ block::{entry::TxTableEntryWord, tables::NameSpaceTable, NsTable}, chain_config::ResolvableChainConfig, + eth_signature_key::BuilderSignature, l1_client::L1Snapshot, - state::{BlockMerkleCommitment, FeeAccount, FeeInfo, FeeMerkleCommitment}, + state::{BlockMerkleCommitment, FeeInfo, FeeMerkleCommitment}, ChainConfig, L1BlockInfo, Leaf, NodeState, SeqTypes, ValidatedState, }; -use anyhow::Context; +use anyhow::{ensure, Context}; use ark_serialize::CanonicalSerialize; use committable::{Commitment, Committable, RawCommitmentBuilder}; -use ethers::types; use hotshot_query_service::availability::QueryableHeader; use hotshot_types::{ traits::{ block_contents::{BlockHeader, BlockPayload, BuilderFee}, node_implementation::NodeType, - EncodeBytes, ValidatedState as HotShotState, + signature_key::BuilderSignatureKey, + ValidatedState as HotShotState, }, utils::BuilderCommitment, vid::{VidCommitment, VidCommon}, @@ -81,9 +82,17 @@ pub struct Header { pub block_merkle_tree_root: BlockMerkleCommitment, /// Root Commitment of `FeeMerkleTree` pub fee_merkle_tree_root: FeeMerkleCommitment, - /// Account (etheruem address) of builder - pub builder_signature: Option, + /// Fee paid by the block builder pub fee_info: FeeInfo, + /// Account (etheruem address) of builder + /// + /// This signature is not considered formally part of the header; it is just evidence proving + /// that other parts of the header (`fee_info`) are correct. It exists in the header so that it + /// is available to all nodes to be used during validation. But since it is checked during + /// consensus, any downstream client who has a proof of consensus finality of a header can trust + /// that `fee_info` is correct without relying on the signature. Thus, this signature is not + /// included in the header commitment. + pub builder_signature: Option, } impl Committable for Header { @@ -142,8 +151,7 @@ impl Header { parent_leaf: &Leaf, mut l1: L1Snapshot, l1_deposits: &[FeeInfo], - builder_fee: FeeInfo, - builder_signature: types::Signature, + builder_fee: BuilderFee, mut timestamp: u64, mut state: ValidatedState, chain_config: ChainConfig, @@ -206,9 +214,20 @@ impl Header { } // Charge the builder fee. + ensure!( + builder_fee.fee_account.validate_fee_signature( + &builder_fee.fee_signature, + builder_fee.fee_amount, + &ns_table, + &payload_commitment, + ), + "invalid builder signature" + ); + let builder_signature = Some(builder_fee.fee_signature); + let fee_info = builder_fee.into(); state - .burn_fee(builder_fee, chain_config.fee_burn_account) - .context(format!("invalid builder fee {builder_fee:?}"))?; + .burn_fee(fee_info, chain_config.fee_burn_account) + .context(format!("invalid builder fee {fee_info:?}"))?; let fee_merkle_tree_root = state.fee_merkle_tree.commitment(); Ok(Self { @@ -222,26 +241,10 @@ impl Header { ns_table, fee_merkle_tree_root, block_merkle_tree_root, - fee_info: builder_fee, - builder_signature: Some(builder_signature), + fee_info, + builder_signature, }) } - - /// Message authorizing a fee payment for inclusion of a certain payload. - /// - /// This message relates the fee info in this header to the payload corresponding to the header. - /// The message is signed by the builder (or whoever is paying for inclusion of the block) and - /// validated by consensus, as authentication for charging the fee to the builder account. - pub fn fee_message(&self) -> anyhow::Result> { - Ok(fee_message( - self.fee_info.amount().as_u64().context(format!( - "fee amount out of range: {:?}", - self.fee_info.amount() - ))?, - self.payload_commitment, - self.metadata(), - )) - } } #[derive(Debug, Snafu)] @@ -289,16 +292,6 @@ impl BlockHeader for Header { let mut validated_state = parent_state.clone(); - // Validate the builder's signature, recovering their fee account address so that we can - // fetch the account state if missing. - let fee_msg = fee_message(builder_fee.fee_amount, payload_commitment, &metadata); - let builder_account = FeeAccount::from( - builder_fee - .fee_signature - .recover(fee_msg) - .context("invalid builder signature")?, - ); - // Fetch the latest L1 snapshot. let l1_snapshot = instance_state.l1_client().snapshot().await; // Fetch the new L1 deposits between parent and current finalized L1 block. @@ -323,7 +316,7 @@ impl BlockHeader for Header { // fee and the burn account which is receiving it, plus any counts receiving deposits in // this block. let missing_accounts = parent_state.forgotten_accounts( - [builder_account, chain_config.fee_burn_account] + [builder_fee.fee_account, chain_config.fee_burn_account] .into_iter() .chain(l1_deposits.iter().map(|info| info.account())), ); @@ -374,8 +367,7 @@ impl BlockHeader for Header { parent_leaf, l1_snapshot, &l1_deposits, - FeeInfo::new(builder_account, builder_fee.fee_amount), - builder_fee.fee_signature, + builder_fee, OffsetDateTime::now_utc().unix_timestamp() as u64, validated_state, chain_config, @@ -431,18 +423,6 @@ impl BlockHeader for Header { } } -fn fee_message( - amount: u64, - payload_commitment: VidCommitment, - metadata: &<::BlockPayload as BlockPayload>::Metadata, -) -> Vec { - let mut data = vec![]; - data.extend_from_slice(amount.to_be_bytes().as_ref()); - data.extend_from_slice(metadata.encode().as_ref()); - data.extend_from_slice(payload_commitment.as_ref()); - data -} - impl QueryableHeader for Header { fn timestamp(&self) -> u64 { self.timestamp @@ -458,7 +438,7 @@ mod test_headers { catchup::mock::MockStateCatchup, eth_signature_key::EthKeyPair, l1_client::L1Client, - state::{validate_proposal, BlockMerkleTree, FeeMerkleTree}, + state::{validate_proposal, BlockMerkleTree, FeeAccount, FeeMerkleTree}, NodeState, }; use async_compatibility_layer::logging::{setup_backtrace, setup_logging}; @@ -542,8 +522,11 @@ mod test_headers { finalized: self.l1_finalized, }, &self.l1_deposits, - FeeInfo::new(fee_account, fee_amount), - fee_signature, + BuilderFee { + fee_account, + fee_amount, + fee_signature, + }, self.timestamp, validated_state.clone(), genesis.instance_state.chain_config, diff --git a/sequencer/src/state.rs b/sequencer/src/state.rs index 9abff38fb4..0f6f5e748f 100644 --- a/sequencer/src/state.rs +++ b/sequencer/src/state.rs @@ -24,7 +24,10 @@ use hotshot_query_service::{ use hotshot_types::{ data::{BlockError, ViewNumber}, traits::{ - node_implementation::ConsensusTime, signature_key::BuilderSignatureKey, states::StateDelta, + block_contents::{BlockHeader, BuilderFee}, + node_implementation::ConsensusTime, + signature_key::BuilderSignatureKey, + states::StateDelta, }, vid::{VidCommon, VidSchemeType}, }; @@ -293,13 +296,18 @@ fn validate_builder_fee(proposed_header: &Header) -> anyhow::Result<()> { let signature = proposed_header .builder_signature .ok_or_else(|| anyhow::anyhow!("Builder signature not found"))?; - let msg = proposed_header.fee_message().context("invalid fee")?; + let fee_amount = proposed_header.fee_info.amount().as_u64().context(format!( + "fee amount out of range: {:?}", + proposed_header.fee_info.amount() + ))?; // verify signature anyhow::ensure!( - proposed_header - .fee_info - .account - .validate_builder_signature(&signature, msg.as_ref()), + proposed_header.fee_info.account.validate_fee_signature( + &signature, + fee_amount, + proposed_header.metadata(), + &proposed_header.payload_commitment() + ), "Invalid Builder Signature" ); @@ -875,6 +883,15 @@ impl FeeInfo { } } +impl From> for FeeInfo { + fn from(fee: BuilderFee) -> Self { + Self { + amount: fee.fee_amount.into(), + account: fee.fee_account, + } + } +} + impl From for FeeInfo { fn from(item: DepositFilter) -> Self { Self {