Strict fee recipient (#3363)

## Issue Addressed

Resolves #3267
Resolves #3156 

## Proposed Changes

- Move the log for fee recipient checks from proposer cache insertion into block proposal so we are directly checking what we get from the EE
- Only log when there is a discrepancy with the local EE, not when using the builder API. In the `builder-api` branch there is an `info` log when there is a discrepancy, I think it is more likely there will be a difference in fee recipient with the builder api because proposer payments might be made via a transaction in the block. Not really sure what patterns will become commong.
- Upgrade the log from a `warn` to an `error` - not actually sure which we want, but I think this is worth an error because the local EE with default transaction ordering I think should pretty much always use the provided fee recipient
- add a `strict-fee-recipient` flag to the VC so we only sign blocks with matching fee recipients. Falls back from the builder API to the local API if there is a discrepancy .




Co-authored-by: realbigsean <sean@sigmaprime.io>

beacon_node/execution_layer/src/lib.rs

@@ -450,23 +450,6 @@ impl<T: EthSpec> ExecutionLayer<T> {
         if let Some(preparation_data_entry) =
             self.proposer_preparation_data().await.get(&proposer_index)
         {
-            if let Some(suggested_fee_recipient) = self.inner.suggested_fee_recipient {
-                if preparation_data_entry.preparation_data.fee_recipient != suggested_fee_recipient
-                {
-                    warn!(
-                        self.log(),
-                        "Inconsistent fee recipient";
-                        "msg" => "The fee recipient returned from the Execution Engine differs \
-                        from the suggested_fee_recipient set on the beacon node. This could \
-                        indicate that fees are being diverted to another address. Please \
-                        ensure that the value of suggested_fee_recipient is set correctly and \
-                        that the Execution Engine is trusted.",
-                        "proposer_index" => ?proposer_index,
-                        "fee_recipient" => ?preparation_data_entry.preparation_data.fee_recipient,
-                        "suggested_fee_recipient" => ?suggested_fee_recipient,
-                    )
-                }
-            }
             // The values provided via the API have first priority.
             preparation_data_entry.preparation_data.fee_recipient
         } else if let Some(address) = self.inner.suggested_fee_recipient {
@@ -689,6 +672,19 @@ impl<T: EthSpec> ExecutionLayer<T> {
                     .get_payload_v1::<T>(payload_id)
                     .await
                     .map(|full_payload| {
+                        if full_payload.fee_recipient != suggested_fee_recipient {
+                            error!(
+                                self.log(),
+                                "Inconsistent fee recipient";
+                                "msg" => "The fee recipient returned from the Execution Engine differs \
+                                from the suggested_fee_recipient set on the beacon node. This could \
+                                indicate that fees are being diverted to another address. Please \
+                                ensure that the value of suggested_fee_recipient is set correctly and \
+                                that the Execution Engine is trusted.",
+                                "fee_recipient" => ?full_payload.fee_recipient,
+                                "suggested_fee_recipient" => ?suggested_fee_recipient,
+                            );
+                        }
                         if f(self, &full_payload).is_some() {
                             warn!(
                                 self.log(),

book/src/suggested-fee-recipient.md

@@ -10,7 +10,8 @@ coinbase and the recipient of other fees or rewards.
 
 There is no guarantee that an execution node will use the `suggested_fee_recipient` to collect fees,
 it may use any address it chooses. It is assumed that an honest execution node *will* use the
-`suggested_fee_recipient`, but users should note this trust assumption.
+`suggested_fee_recipient`, but users should note this trust assumption. Check out the 
+[strict fee recipient](#strict-fee-recipient) section for how to mitigate this assumption.
 
 The `suggested_fee_recipient` can be provided to the VC, who will transmit it to the BN. The BN also
 has a choice regarding the fee recipient it passes to the execution node, creating another
@@ -61,6 +62,15 @@ validators where a `suggested_fee_recipient` is not loaded from another method.
 The `--suggested-fee-recipient` can be provided to the BN to act as a default value when the
 validator client does not transmit a `suggested_fee_recipient` to the BN.
 
+## Strict Fee Recipient
+
+If the flag `--strict-fee-recipient` is set in the validator client, Lighthouse will refuse to sign any block whose 
+`fee_recipient` does not match the `suggested_fee_recipient` sent by this validator. This applies to both the normal 
+block proposal flow, as well as block proposals through the builder API. Proposals through the builder API are more likely 
+to have a discrepancy in `fee_recipient` so you should be aware of how your connected relay sends proposer payments before 
+using this flag. If this flag is used, a fee recipient mismatch in the builder API flow will result in a fallback to the
+local execution engine for payload construction, where a strict fee recipient check will still be applied.
+
 ## Setting the fee recipient dynamically using the keymanager API
 
 When the [validator client API](api-vc.md) is enabled, the

consensus/types/src/payload.rs

@@ -44,6 +44,7 @@ pub trait ExecPayload<T: EthSpec>:
     fn block_number(&self) -> u64;
     fn timestamp(&self) -> u64;
     fn block_hash(&self) -> ExecutionBlockHash;
+    fn fee_recipient(&self) -> Address;
 }
 
 impl<T: EthSpec> ExecPayload<T> for FullPayload<T> {
@@ -74,6 +75,10 @@ impl<T: EthSpec> ExecPayload<T> for FullPayload<T> {
     fn block_hash(&self) -> ExecutionBlockHash {
         self.execution_payload.block_hash
     }
+
+    fn fee_recipient(&self) -> Address {
+        self.execution_payload.fee_recipient
+    }
 }
 
 impl<T: EthSpec> ExecPayload<T> for BlindedPayload<T> {
@@ -104,6 +109,10 @@ impl<T: EthSpec> ExecPayload<T> for BlindedPayload<T> {
     fn block_hash(&self) -> ExecutionBlockHash {
         self.execution_payload_header.block_hash
     }
+
+    fn fee_recipient(&self) -> Address {
+        self.execution_payload_header.fee_recipient
+    }
 }
 
 #[derive(Debug, Clone, TestRandom, Serialize, Deserialize, Derivative)]

lighthouse/tests/validator_client.rs

@@ -388,3 +388,16 @@ fn no_doppelganger_protection_flag() {
         .run()
         .with_config(|config| assert!(!config.enable_doppelganger_protection));
 }
+#[test]
+fn strict_fee_recipient_flag() {
+    CommandLineTest::new()
+        .flag("strict-fee-recipient", None)
+        .run()
+        .with_config(|config| assert!(config.strict_fee_recipient));
+}
+#[test]
+fn no_strict_fee_recipient_flag() {
+    CommandLineTest::new()
+        .run()
+        .with_config(|config| assert!(!config.strict_fee_recipient));
+}

validator_client/src/block_service.rs

@@ -45,6 +45,7 @@ pub struct BlockServiceBuilder<T, E: EthSpec> {
     graffiti: Option<Graffiti>,
     graffiti_file: Option<GraffitiFile>,
     private_tx_proposals: bool,
+    strict_fee_recipient: bool,
 }
 
 impl<T: SlotClock + 'static, E: EthSpec> BlockServiceBuilder<T, E> {
@@ -57,6 +58,7 @@ impl<T: SlotClock + 'static, E: EthSpec> BlockServiceBuilder<T, E> {
             graffiti: None,
             graffiti_file: None,
             private_tx_proposals: false,
+            strict_fee_recipient: false,
         }
     }
 
@@ -95,6 +97,11 @@ impl<T: SlotClock + 'static, E: EthSpec> BlockServiceBuilder<T, E> {
         self
     }
 
+    pub fn strict_fee_recipient(mut self, strict_fee_recipient: bool) -> Self {
+        self.strict_fee_recipient = strict_fee_recipient;
+        self
+    }
+
     pub fn build(self) -> Result<BlockService<T, E>, String> {
         Ok(BlockService {
             inner: Arc::new(Inner {
@@ -113,6 +120,7 @@ impl<T: SlotClock + 'static, E: EthSpec> BlockServiceBuilder<T, E> {
                 graffiti: self.graffiti,
                 graffiti_file: self.graffiti_file,
                 private_tx_proposals: self.private_tx_proposals,
+                strict_fee_recipient: self.strict_fee_recipient,
             }),
         })
     }
@@ -127,6 +135,7 @@ pub struct Inner<T, E: EthSpec> {
     graffiti: Option<Graffiti>,
     graffiti_file: Option<GraffitiFile>,
     private_tx_proposals: bool,
+    strict_fee_recipient: bool,
 }
 
 /// Attempts to produce attestations for any block producer(s) at the start of the epoch.
@@ -328,6 +337,9 @@ impl<T: SlotClock + 'static, E: EthSpec> BlockService<T, E> {
         let self_ref = &self;
         let proposer_index = self.validator_store.validator_index(&validator_pubkey);
         let validator_pubkey_ref = &validator_pubkey;
+        let fee_recipient = self.validator_store.get_fee_recipient(&validator_pubkey);
+
+        let strict_fee_recipient = self.strict_fee_recipient;
         // Request block from first responsive beacon node.
         let block = self
             .beacon_nodes
@@ -372,6 +384,17 @@ impl<T: SlotClock + 'static, E: EthSpec> BlockService<T, E> {
                 };
                 drop(get_timer);
 
+                // Ensure the correctness of the execution payload's fee recipient.
+                if strict_fee_recipient {
+                    if let Ok(execution_payload) = block.body().execution_payload() {
+                        if Some(execution_payload.fee_recipient()) != fee_recipient {
+                            return Err(BlockError::Recoverable(
+                                "Incorrect fee recipient used by builder".to_string(),
+                            ));
+                        }
+                    }
+                }
+
                 if proposer_index != Some(block.proposer_index()) {
                     return Err(BlockError::Recoverable(
                         "Proposer index does not match block proposer. Beacon chain re-orged"

validator_client/src/cli.rs

@@ -258,4 +258,17 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
                     execution payload construction during proposals.")
                 .takes_value(false),
         )
+        .arg(
+            Arg::with_name("strict-fee-recipient")
+                .long("strict-fee-recipient")
+                .help("If this flag is set, Lighthouse will refuse to sign any block whose \
+                    `fee_recipient` does not match the `suggested_fee_recipient` sent by this validator. \
+                     This applies to both the normal block proposal flow, as well as block proposals \
+                     through the builder API. Proposals through the builder API are more likely to have a \
+                     discrepancy in `fee_recipient` so you should be aware of how your connected relay \
+                     sends proposer payments before using this flag. If this flag is used, a fee recipient \
+                     mismatch in the builder API flow will result in a fallback to the local execution engine \
+                     for payload construction, where a strict fee recipient check will still be applied.")
+                .takes_value(false),
+        )
 }

validator_client/src/config.rs

@@ -56,6 +56,9 @@ pub struct Config {
     /// A list of custom certificates that the validator client will additionally use when
     /// connecting to a beacon node over SSL/TLS.
     pub beacon_nodes_tls_certs: Option<Vec<PathBuf>>,
+    /// Enabling this will make sure the validator client never signs a block whose `fee_recipient`
+    /// does not match the `suggested_fee_recipient`.
+    pub strict_fee_recipient: bool,
 }
 
 impl Default for Config {
@@ -89,6 +92,7 @@ impl Default for Config {
             enable_doppelganger_protection: false,
             beacon_nodes_tls_certs: None,
             private_tx_proposals: false,
+            strict_fee_recipient: false,
         }
     }
 }
@@ -300,6 +304,10 @@ impl Config {
             config.private_tx_proposals = true;
         }
 
+        if cli_args.is_present("strict-fee-recipient") {
+            config.strict_fee_recipient = true;
+        }
+
         Ok(config)
     }
 }

validator_client/src/lib.rs

@@ -414,6 +414,7 @@ impl<T: EthSpec> ProductionValidatorClient<T> {
             .graffiti(config.graffiti)
             .graffiti_file(config.graffiti_file.clone())
             .private_tx_proposals(config.private_tx_proposals)
+            .strict_fee_recipient(config.strict_fee_recipient)
             .build()?;
 
         let attestation_service = AttestationServiceBuilder::new()