From 904dd6252447d8162b6059e83de25424060dfe34 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Tue, 26 Jul 2022 02:17:24 +0000 Subject: [PATCH] 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 --- beacon_node/execution_layer/src/lib.rs | 30 +++++++++++--------------- book/src/suggested-fee-recipient.md | 12 ++++++++++- consensus/types/src/payload.rs | 9 ++++++++ lighthouse/tests/validator_client.rs | 13 +++++++++++ validator_client/src/block_service.rs | 23 ++++++++++++++++++++ validator_client/src/cli.rs | 13 +++++++++++ validator_client/src/config.rs | 8 +++++++ validator_client/src/lib.rs | 1 + 8 files changed, 91 insertions(+), 18 deletions(-) diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index e89e9ba814..5b82018749 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -450,23 +450,6 @@ impl ExecutionLayer { 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 ExecutionLayer { .get_payload_v1::(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(), diff --git a/book/src/suggested-fee-recipient.md b/book/src/suggested-fee-recipient.md index 5c77081c39..35338549e9 100644 --- a/book/src/suggested-fee-recipient.md +++ b/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 diff --git a/consensus/types/src/payload.rs b/consensus/types/src/payload.rs index a21eeb63c2..4a8552d249 100644 --- a/consensus/types/src/payload.rs +++ b/consensus/types/src/payload.rs @@ -44,6 +44,7 @@ pub trait ExecPayload: fn block_number(&self) -> u64; fn timestamp(&self) -> u64; fn block_hash(&self) -> ExecutionBlockHash; + fn fee_recipient(&self) -> Address; } impl ExecPayload for FullPayload { @@ -74,6 +75,10 @@ impl ExecPayload for FullPayload { fn block_hash(&self) -> ExecutionBlockHash { self.execution_payload.block_hash } + + fn fee_recipient(&self) -> Address { + self.execution_payload.fee_recipient + } } impl ExecPayload for BlindedPayload { @@ -104,6 +109,10 @@ impl ExecPayload for BlindedPayload { 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)] diff --git a/lighthouse/tests/validator_client.rs b/lighthouse/tests/validator_client.rs index 4ff5434687..98b159e996 100644 --- a/lighthouse/tests/validator_client.rs +++ b/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)); +} diff --git a/validator_client/src/block_service.rs b/validator_client/src/block_service.rs index 2ba81eac7a..649f240645 100644 --- a/validator_client/src/block_service.rs +++ b/validator_client/src/block_service.rs @@ -45,6 +45,7 @@ pub struct BlockServiceBuilder { graffiti: Option, graffiti_file: Option, private_tx_proposals: bool, + strict_fee_recipient: bool, } impl BlockServiceBuilder { @@ -57,6 +58,7 @@ impl BlockServiceBuilder { graffiti: None, graffiti_file: None, private_tx_proposals: false, + strict_fee_recipient: false, } } @@ -95,6 +97,11 @@ impl BlockServiceBuilder { 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, String> { Ok(BlockService { inner: Arc::new(Inner { @@ -113,6 +120,7 @@ impl BlockServiceBuilder { 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 { graffiti: Option, graffiti_file: Option, 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 BlockService { 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 BlockService { }; 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" diff --git a/validator_client/src/cli.rs b/validator_client/src/cli.rs index 414be2d90f..1f8b7b08ba 100644 --- a/validator_client/src/cli.rs +++ b/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), + ) } diff --git a/validator_client/src/config.rs b/validator_client/src/config.rs index ddbe7f3630..725414b1b9 100644 --- a/validator_client/src/config.rs +++ b/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>, + /// 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) } } diff --git a/validator_client/src/lib.rs b/validator_client/src/lib.rs index b78b072cf8..1baa9f6bb2 100644 --- a/validator_client/src/lib.rs +++ b/validator_client/src/lib.rs @@ -414,6 +414,7 @@ impl ProductionValidatorClient { .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()