Suggestions for fee recipieint changes

account_manager/src/validator/import.rs

@@ -273,9 +273,15 @@ pub fn cli_run(matches: &ArgMatches, validator_dir: PathBuf) -> Result<(), Strin
         eprintln!("Successfully imported keystore.");
         num_imported_keystores += 1;
 
-        let validator_def =
-            ValidatorDefinition::new_keystore_with_password(&dest_keystore, password_opt, None)
-                .map_err(|e| format!("Unable to create new validator definition: {:?}", e))?;
+        let graffiti = None;
+        let suggested_fee_recipient = None;
+        let validator_def = ValidatorDefinition::new_keystore_with_password(
+            &dest_keystore,
+            password_opt,
+            graffiti,
+            suggested_fee_recipient,
+        )
+        .map_err(|e| format!("Unable to create new validator definition: {:?}", e))?;
 
         defs.push(validator_def);
 

beacon_node/src/cli.rs

@@ -108,7 +108,7 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
             Arg::with_name("network-load")
                 .long("network-load")
                 .value_name("INTEGER")
-                .help("Lighthouse's network can be tuned for bandwidth/performance. Setting this to a high value, will increase the bandwidth lighthouse uses, increasing the likelihood of redundant information in exchange for faster communication. This can increase profit of validators marginally by receiving messages faster on the network. Lower values decrease bandwidth usage, but makes communication slower which can lead to validator performance reduction. Values are in the range [1,5].") 
+                .help("Lighthouse's network can be tuned for bandwidth/performance. Setting this to a high value, will increase the bandwidth lighthouse uses, increasing the likelihood of redundant information in exchange for faster communication. This can increase profit of validators marginally by receiving messages faster on the network. Lower values decrease bandwidth usage, but makes communication slower which can lead to validator performance reduction. Values are in the range [1,5].")
                 .default_value("3")
                 .set(clap::ArgSettings::Hidden)
                 .takes_value(true),
@@ -409,9 +409,9 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
                 .takes_value(true)
         )
         .arg(
-            Arg::with_name("fee-recipient")
-                .long("fee-recipient")
-                .value_name("FEE-RECIPIENT")
+            Arg::with_name("suggested-fee-recipient")
+                .long("suggested-fee-recipient")
+                .value_name("SUGGESTED-FEE-RECIPIENT")
                 .help("Once the merge has happened, this address will receive transaction fees \
                        collected from any blocks produced by this node. Defaults to a junk \
                        address whilst the merge is in development stages. THE DEFAULT VALUE \

beacon_node/src/config.rs

@@ -14,12 +14,7 @@ use std::net::{IpAddr, Ipv4Addr, ToSocketAddrs};
 use std::net::{TcpListener, UdpSocket};
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
-use types::{Address, Checkpoint, Epoch, EthSpec, Hash256, PublicKeyBytes, GRAFFITI_BYTES_LEN};
-
-// TODO(merge): remove this default value. It's just there to make life easy during
-// early testnets.
-const DEFAULT_SUGGESTED_FEE_RECIPIENT: [u8; 20] =
-    [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1];
+use types::{Checkpoint, Epoch, EthSpec, Hash256, PublicKeyBytes, GRAFFITI_BYTES_LEN};
 
 /// Gets the fully-initialized global client.
 ///
@@ -253,12 +248,8 @@ pub fn get_config<E: EthSpec>(
         client_config.execution_endpoints = Some(client_config.eth1.endpoints.clone());
     }
 
-    client_config.suggested_fee_recipient = Some(
-        clap_utils::parse_optional(cli_args, "fee-recipient")?
-            // TODO(merge): remove this default value. It's just there to make life easy during
-            // early testnets.
-            .unwrap_or_else(|| Address::from(DEFAULT_SUGGESTED_FEE_RECIPIENT)),
-    );
+    client_config.suggested_fee_recipient =
+        clap_utils::parse_optional(cli_args, "suggested-fee-recipient")?;
 
     if let Some(freezer_dir) = cli_args.value_of("freezer-dir") {
         client_config.freezer_db_path = Some(PathBuf::from(freezer_dir));

book/src/SUMMARY.md

@@ -21,6 +21,7 @@
     * [Voluntary Exits](./voluntary-exit.md)
     * [Validator Monitoring](./validator-monitoring.md)
     * [Doppelganger Protection](./validator-doppelganger.md)
+    * [Suggested Fee Recipient](./suggested-fee-recipient.md)
 * [APIs](./api.md)
 	* [Beacon Node API](./api-bn.md)
 		* [/lighthouse](./api-lighthouse.md)

book/src/suggested-fee-recipient.md

@@ -0,0 +1,91 @@
+# Suggested Fee Recipient
+
+*Note: these documents are not relevant until the Bellatrix (Merge) upgrade has occurred.*
+
+## Fee recipient trust assumptions
+
+During post-merge block production, the Beacon Node (BN) will provide a `suggested_fee_recipient` to
+the execution node. This is a 20-byte Ethereum address which the EL might choose to set as the
+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.
+
+The `suggested_fee_recipient` can be provided to the VC, who will transmit it to the BN. The also BN
+has a choice regarding the fee recipient it passes to the execution node, creating another
+noteworthy trust assumption.
+
+To be sure *you* control your fee recipient value, run your own BN and execution node (don't use
+third-party services).
+
+The Lighthouse VC provides three methods for setting the `suggested_fee_recipient` (also known
+simply as the "fee recipient") to be passed to the execution layer during block production. The
+Lighthouse BN also provides a method for defining this value, should the VC not transmit a value.
+
+Assuming trustworthy nodes, the priority for the four methods is:
+
+1. `validator_definitions.yml`
+1. `--suggested-fee-recipient-file`
+1. `--suggested-fee-recipient` provided to the VC.
+1. `--suggested-fee-recipient` provided to the BN.
+
+Users may configure the fee recipient via `validator_definitions.yml` or via the
+`--suggested-fee-recipient-file` flag. The value in `validator_definitions.yml` will always take
+precedence.
+
+### 1. Setting the fee recipient in the `validator_definitions.yml`
+
+Users can set the fee recipient in `validator_definitions.yml` with the `suggested_fee_recipient`
+key. This option is recommended for most users, where each validator has a fixed fee recipient.
+
+Below is an example of the validator_definitions.yml with `suggested_fee_recipient` values:
+
+```
+---
+- enabled: true
+  voting_public_key: "0x87a580d31d7bc69069b55f5a01995a610dd391a26dc9e36e81057a17211983a79266800ab8531f21f1083d7d84085007"
+  type: local_keystore
+  voting_keystore_path: /home/paul/.lighthouse/validators/0x87a580d31d7bc69069b55f5a01995a610dd391a26dc9e36e81057a17211983a79266800ab8531f21f1083d7d84085007/voting-keystore.json
+  voting_keystore_password_path: /home/paul/.lighthouse/secrets/0x87a580d31d7bc69069b55f5a01995a610dd391a26dc9e36e81057a17211983a79266800ab8531f21f1083d7d84085007
+  suggested_fee_recipient: "0x6cc8dcbca744a6e4ffedb98e1d0df903b10abd21"
+- enabled: false
+  voting_public_key: "0xa5566f9ec3c6e1fdf362634ebec9ef7aceb0e460e5079714808388e5d48f4ae1e12897fed1bea951c17fa389d511e477"
+  type: local_keystore voting_keystore_path: /home/paul/.lighthouse/validators/0xa5566f9ec3c6e1fdf362634ebec9ef7aceb0e460e5079714808388e5d48f4ae1e12897fed1bea951c17fa389d511e477/voting-keystore.json
+  voting_keystore_password: myStrongpa55word123&$
+  suggested_fee_recipient: "0xa2e334e71511686bcfe38bb3ee1ad8f6babcc03d"
+```
+
+### 2. Using the "--suggested-fee-recipient-file" flag on the validator client
+
+Users can specify a file with the `--suggested-fee-recipient-file` flag. This option is useful for dynamically
+changing fee recipients. This file is reloaded each time a validator is chosen to propose a block.
+
+Usage:
+`lighthouse vc --suggested-fee-recipient-file fee_recipient.txt`
+
+The file should contain key value pairs corresponding to validator public keys and their associated
+fee recipient. The file can optionally contain a `default` key for the default case.
+
+The following example sets the default and the values for the validators with pubkeys `0x87a5` and
+`0xa556`:
+
+```
+default: 0x6cc8dcbca744a6e4ffedb98e1d0df903b10abd21
+0x87a580d31d7bc69069b55f5a01995a610dd391a26dc9e36e81057a17211983a79266800ab8531f21f1083d7d84085007: 0x6cc8dcbca744a6e4ffedb98e1d0df903b10abd21
+0xa5566f9ec3c6e1fdf362634ebec9ef7aceb0e460e5079714808388e5d48f4ae1e12897fed1bea951c17fa389d511e477: 0xa2e334e71511686bcfe38bb3ee1ad8f6babcc03d
+```
+
+Lighthouse will first search for the fee recipient corresponding to the public key of the proposing
+validator, if there are no matches for the public key, then it uses the address corresponding to the
+default key (if present).
+
+### 3. Using the "--suggested-fee-recipient" flag on the validator client
+
+The `--suggested-fee-recipient` can be provided to the VC to act as a default value for all
+validators where a `suggested_fee_recipient` is not loaded from another method.
+
+### 4. Using the "--suggested-fee-recipient" flag on the beacon node
+
+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.

common/account_utils/src/validator_definitions.rs

@@ -13,7 +13,7 @@ use std::collections::HashSet;
 use std::fs::{self, OpenOptions};
 use std::io;
 use std::path::{Path, PathBuf};
-use types::{graffiti::GraffitiString, PublicKey};
+use types::{graffiti::GraffitiString, Address, PublicKey};
 use validator_dir::VOTING_KEYSTORE_FILE;
 
 /// The file name for the serialized `ValidatorDefinitions` struct.
@@ -90,6 +90,9 @@ pub struct ValidatorDefinition {
     #[serde(skip_serializing_if = "Option::is_none")]
     pub graffiti: Option<GraffitiString>,
     #[serde(default)]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub suggested_fee_recipient: Option<Address>,
+    #[serde(default)]
     pub description: String,
     #[serde(flatten)]
     pub signing_definition: SigningDefinition,
@@ -106,6 +109,7 @@ impl ValidatorDefinition {
         voting_keystore_path: P,
         voting_keystore_password: Option<ZeroizeString>,
         graffiti: Option<GraffitiString>,
+        suggested_fee_recipient: Option<Address>,
     ) -> Result<Self, Error> {
         let voting_keystore_path = voting_keystore_path.as_ref().into();
         let keystore =
@@ -117,6 +121,7 @@ impl ValidatorDefinition {
             voting_public_key,
             description: keystore.description().unwrap_or("").to_string(),
             graffiti,
+            suggested_fee_recipient,
             signing_definition: SigningDefinition::LocalKeystore {
                 voting_keystore_path,
                 voting_keystore_password_path: None,
@@ -262,6 +267,7 @@ impl ValidatorDefinitions {
                     voting_public_key,
                     description: keystore.description().unwrap_or("").to_string(),
                     graffiti: None,
+                    suggested_fee_recipient: None,
                     signing_definition: SigningDefinition::LocalKeystore {
                         voting_keystore_path,
                         voting_keystore_password_path,
@@ -458,4 +464,45 @@ mod tests {
             Some(GraffitiString::from_str("mrfwashere").unwrap())
         );
     }
+
+    #[test]
+    fn suggested_fee_recipient_checks() {
+        let no_suggested_fee_recipient = r#"---
+        description: ""
+        enabled: true
+        type: local_keystore
+        voting_keystore_path: ""
+        voting_public_key: "0xaf3c7ddab7e293834710fca2d39d068f884455ede270e0d0293dc818e4f2f0f975355067e8437955cb29aec674e5c9e7"
+        "#;
+        let def: ValidatorDefinition = serde_yaml::from_str(no_suggested_fee_recipient).unwrap();
+        assert!(def.suggested_fee_recipient.is_none());
+
+        let invalid_suggested_fee_recipient = r#"---
+        description: ""
+        enabled: true
+        type: local_keystore
+        suggested_fee_recipient: "foopy"
+        voting_keystore_path: ""
+        voting_public_key: "0xaf3c7ddab7e293834710fca2d39d068f884455ede270e0d0293dc818e4f2f0f975355067e8437955cb29aec674e5c9e7"
+        "#;
+
+        let def: Result<ValidatorDefinition, _> =
+            serde_yaml::from_str(invalid_suggested_fee_recipient);
+        assert!(def.is_err());
+
+        let valid_suggested_fee_recipient = r#"---
+        description: ""
+        enabled: true
+        type: local_keystore
+        suggested_fee_recipient: "0xa2e334e71511686bcfe38bb3ee1ad8f6babcc03d"
+        voting_keystore_path: ""
+        voting_public_key: "0xaf3c7ddab7e293834710fca2d39d068f884455ede270e0d0293dc818e4f2f0f975355067e8437955cb29aec674e5c9e7"
+        "#;
+
+        let def: ValidatorDefinition = serde_yaml::from_str(valid_suggested_fee_recipient).unwrap();
+        assert_eq!(
+            def.suggested_fee_recipient,
+            Some(Address::from_str("0xa2e334e71511686bcfe38bb3ee1ad8f6babcc03d").unwrap())
+        );
+    }
 }

common/eth2/src/lighthouse_vc/types.rs

@@ -22,6 +22,9 @@ pub struct ValidatorRequest {
     #[serde(default)]
     #[serde(skip_serializing_if = "Option::is_none")]
     pub graffiti: Option<GraffitiString>,
+    #[serde(default)]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub suggested_fee_recipient: Option<Address>,
     #[serde(with = "eth2_serde_utils::quoted_u64")]
     pub deposit_gwei: u64,
 }
@@ -42,6 +45,9 @@ pub struct CreatedValidator {
     #[serde(default)]
     #[serde(skip_serializing_if = "Option::is_none")]
     pub graffiti: Option<GraffitiString>,
+    #[serde(default)]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub suggested_fee_recipient: Option<Address>,
     pub eth1_deposit_tx_data: String,
     #[serde(with = "eth2_serde_utils::quoted_u64")]
     pub deposit_gwei: u64,
@@ -64,6 +70,7 @@ pub struct KeystoreValidatorsPostRequest {
     pub enable: bool,
     pub keystore: Keystore,
     pub graffiti: Option<GraffitiString>,
+    pub suggested_fee_recipient: Option<Address>,
 }
 
 #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
@@ -73,6 +80,9 @@ pub struct Web3SignerValidatorRequest {
     #[serde(default)]
     #[serde(skip_serializing_if = "Option::is_none")]
     pub graffiti: Option<GraffitiString>,
+    #[serde(default)]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub suggested_fee_recipient: Option<Address>,
     pub voting_public_key: PublicKey,
     pub url: String,
     #[serde(default)]

lighthouse/tests/account_manager.rs

@@ -493,6 +493,7 @@ fn validator_import_launchpad() {
         enabled: false,
         description: "".into(),
         graffiti: None,
+        suggested_fee_recipient: None,
         voting_public_key: keystore.public_key().unwrap(),
         signing_definition: SigningDefinition::LocalKeystore {
             voting_keystore_path,
@@ -612,6 +613,7 @@ fn validator_import_launchpad_no_password_then_add_password() {
         enabled: true,
         description: "".into(),
         graffiti: None,
+        suggested_fee_recipient: None,
         voting_public_key: keystore.public_key().unwrap(),
         signing_definition: SigningDefinition::LocalKeystore {
             voting_keystore_path,
@@ -635,6 +637,7 @@ fn validator_import_launchpad_no_password_then_add_password() {
         enabled: true,
         description: "".into(),
         graffiti: None,
+        suggested_fee_recipient: None,
         voting_public_key: keystore.public_key().unwrap(),
         signing_definition: SigningDefinition::LocalKeystore {
             voting_keystore_path: dst_keystore_dir.join(KEYSTORE_NAME),
@@ -734,6 +737,7 @@ fn validator_import_launchpad_password_file() {
         description: "".into(),
         voting_public_key: keystore.public_key().unwrap(),
         graffiti: None,
+        suggested_fee_recipient: None,
         signing_definition: SigningDefinition::LocalKeystore {
             voting_keystore_path,
             voting_keystore_password_path: None,

lighthouse/tests/beacon_node.rs

@@ -212,7 +212,7 @@ fn merge_fee_recipient_flag() {
     CommandLineTest::new()
         .flag("merge", None)
         .flag(
-            "fee-recipient",
+            "suggested-fee-recipient",
             Some("0x00000000219ab540356cbb839cbe05303d7705fa"),
         )
         .run_with_zero_port()

lighthouse/tests/validator_client.rs

@@ -220,12 +220,12 @@ fn graffiti_file_with_pk_flag() {
         });
 }
 
-// Tests for fee-recipient flags.
+// Tests for suggested-fee-recipient flags.
 #[test]
 fn fee_recipient_flag() {
     CommandLineTest::new()
         .flag(
-            "fee-recipient",
+            "suggested-fee-recipient",
             Some("0x00000000219ab540356cbb839cbe05303d7705fa"),
         )
         .run()
@@ -248,7 +248,7 @@ fn fee_recipient_file_flag() {
         .expect("Unable to write to file");
     CommandLineTest::new()
         .flag(
-            "fee-recipient-file",
+            "suggested-fee-recipient-file",
             dir.path().join("fee_recipient.txt").as_os_str().to_str(),
         )
         .run()
@@ -280,7 +280,7 @@ fn fee_recipient_file_with_pk_flag() {
         .expect("Unable to write to file");
     CommandLineTest::new()
         .flag(
-            "fee-recipient-file",
+            "suggested-fee-recipient-file",
             dir.path().join("fee_recipient.txt").as_os_str().to_str(),
         )
         .run()

testing/web3signer_tests/src/lib.rs

@@ -336,6 +336,7 @@ mod tests {
                     enabled: true,
                     voting_public_key: validator_pubkey.clone(),
                     graffiti: None,
+                    suggested_fee_recipient: None,
                     description: String::default(),
                     signing_definition: SigningDefinition::LocalKeystore {
                         voting_keystore_path: signer_rig.keystore_path.clone(),
@@ -351,6 +352,7 @@ mod tests {
                     enabled: true,
                     voting_public_key: validator_pubkey.clone(),
                     graffiti: None,
+                    suggested_fee_recipient: None,
                     description: String::default(),
                     signing_definition: SigningDefinition::Web3Signer {
                         url: signer_rig.url.to_string(),