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

@@ -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");

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

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<types::Signature>,
+    /// 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<BuilderSignature>,
 }
 
 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<SeqTypes>,
         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<Vec<u8>> {
-        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<SeqTypes> 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<SeqTypes> 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<SeqTypes> 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<SeqTypes> for Header {
     }
 }
 
-fn fee_message(
-    amount: u64,
-    payload_commitment: VidCommitment,
-    metadata: &<<SeqTypes as NodeType>::BlockPayload as BlockPayload>::Metadata,
-) -> Vec<u8> {
-    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<SeqTypes> 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,

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<BuilderFee<SeqTypes>> for FeeInfo {
+    fn from(fee: BuilderFee<SeqTypes>) -> Self {
+        Self {
+            amount: fee.fee_amount.into(),
+            account: fee.fee_account,
+        }
+    }
+}
+
 impl From<DepositFilter> for FeeInfo {
     fn from(item: DepositFilter) -> Self {
         Self {