Kintsugi review comments (#2831)

* Fix makefile * Return on invalid finalized block * Fix todo in gossip scoring * Require --merge for --fee-recipient * Bump eth2_serde_utils * Change schema versions * Swap hash/uint256 test_random impls * Use default for ExecutionPayload::empty * Check for DBs before removing * Remove kintsugi docker image * Fix CLI default value
2021-11-30 09:32:53 +11:00 · 2021-11-30 09:32:53 +11:00 · 1b56ebf85e
commit 1b56ebf85e
parent 82a81524e3
22 changed files with 57 additions and 114 deletions
--- a/.github/workflows/docker-kintsugi.yml
+++ b/.github/workflows/docker-kintsugi.yml
@ -1,45 +0,0 @@
-name: docker kintsugi
-
-on:
-    push:
-        branches:
-            - kintsugi
-
-env:
-    DOCKER_PASSWORD: ${{ secrets.DOCKER_PASSWORD }}
-    DOCKER_USERNAME: ${{ secrets.DOCKER_USERNAME }}
-    IMAGE_NAME: ${{ github.repository_owner}}/lighthouse
-    LCLI_IMAGE_NAME: ${{ github.repository_owner }}/lcli
-    BRANCH_NAME: kintsugi
-
-jobs:
-    build-docker-amd64:
-        runs-on: ubuntu-18.04
-        steps:
-            - uses: actions/checkout@v2
-            - name: Update Rust
-              run: rustup update stable
-            - name: Dockerhub login
-              run: |
-                  echo "${DOCKER_PASSWORD}" | docker login --username ${DOCKER_USERNAME} --password-stdin
-            - name: Build AMD64 dockerfile (with push)
-              run: |
-                  docker build \
-                      --build-arg PORTABLE=true \
-                      --tag ${IMAGE_NAME}:${BRANCH_NAME} \
-                      --file ./Dockerfile .
-                  docker push ${IMAGE_NAME}:${BRANCH_NAME}
-    build-docker-lcli:
-        runs-on: ubuntu-18.04
-        steps:
-            - uses: actions/checkout@v2
-            - name: Dockerhub login
-              run: |
-                  echo "${DOCKER_PASSWORD}" | docker login --username ${DOCKER_USERNAME} --password-stdin
-            - name: Build lcli dockerfile (with push)
-              run: |
-                  docker build \
-                      --build-arg PORTABLE=true \
-                      --tag ${LCLI_IMAGE_NAME}:${BRANCH_NAME} \
-                      --file ./lcli/Dockerfile .
-                  docker push ${LCLI_IMAGE_NAME}:${BRANCH_NAME}
--- a/Cargo.lock
+++ b/Cargo.lock
@ -451,7 +451,7 @@ dependencies = [
 "arbitrary",
 "blst",
 "eth2_hashing 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)",
- "eth2_serde_utils 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
+ "eth2_serde_utils",
 "eth2_ssz",
 "ethereum-types 0.12.1",
 "hex",
@ -1460,7 +1460,7 @@ dependencies = [
 "account_utils",
 "bytes",
 "eth2_keystore",
- "eth2_serde_utils 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
+ "eth2_serde_utils",
 "eth2_ssz",
 "eth2_ssz_derive 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)",
 "futures",
@ -1575,7 +1575,7 @@ dependencies = [

 [[package]]
 name = "eth2_serde_utils"
-version = "0.1.0"
+version = "0.1.1"
 dependencies = [
 "ethereum-types 0.12.1",
 "hex",
@ -1584,17 +1584,6 @@ dependencies = [
 "serde_json",
 ]

-[[package]]
-name = "eth2_serde_utils"
-version = "0.1.0"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "477fffc25490dfc866288273f96344c6879676a1337187fc39245cd422e10825"
-dependencies = [
- "hex",
- "serde",
- "serde_derive",
-]
-
 [[package]]
 name = "eth2_ssz"
 version = "0.4.1"
@ -1631,7 +1620,7 @@ name = "eth2_ssz_types"
 version = "0.2.2"
 dependencies = [
 "arbitrary",
- "eth2_serde_utils 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
+ "eth2_serde_utils",
 "eth2_ssz",
 "serde",
 "serde_derive",
@ -1772,7 +1761,7 @@ dependencies = [
 "bytes",
 "environment",
 "eth1",
- "eth2_serde_utils 0.1.0",
+ "eth2_serde_utils",
 "eth2_ssz_types",
 "exit-future",
 "futures",
@ -5350,7 +5339,7 @@ dependencies = [
 name = "slashing_protection"
 version = "0.1.0"
 dependencies = [
- "eth2_serde_utils 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
+ "eth2_serde_utils",
 "filesystem",
 "lazy_static",
 "r2d2",
@ -6340,7 +6329,7 @@ dependencies = [
 "derivative",
 "eth2_hashing 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)",
 "eth2_interop_keypairs",
- "eth2_serde_utils 0.1.0",
+ "eth2_serde_utils",
 "eth2_ssz",
 "eth2_ssz_derive 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)",
 "eth2_ssz_types",
@ -6528,7 +6517,7 @@ dependencies = [
 "environment",
 "eth2",
 "eth2_keystore",
- "eth2_serde_utils 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
+ "eth2_serde_utils",
 "exit-future",
 "filesystem",
 "futures",
--- a/Cargo.toml
+++ b/Cargo.toml
@ -91,3 +91,4 @@ warp = { git = "https://github.com/macladson/warp", rev ="7e75acc" }
 eth2_ssz = { path = "consensus/ssz" }
 eth2_ssz_types = { path = "consensus/ssz_types" }
 tree_hash = { path = "consensus/tree_hash" }
+eth2_serde_utils = { path = "consensus/serde_utils" }
--- a/4
+++ b/4
@ -23,9 +23,9 @@ FORKS=phase0 altair
 # Binaries will most likely be found in `./target/release`
 install:
 ifeq ($(PORTABLE), true)
-	cargo install --path lighthouse --force --locked --features portable,spec-minimal
+	cargo install --path lighthouse --force --locked --features portable
 else
-	cargo install --path lighthouse --force --locked --features spec-minimal
+	cargo install --path lighthouse --force --locked
 endif

 # Builds the lcli binary in release (optimized).
--- a/beacon_node/beacon_chain/src/beacon_chain.rs
+++ b/beacon_node/beacon_chain/src/beacon_chain.rs
@ -3281,6 +3281,9 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
                        "Finalized block has an invalid execution payload.",
                    ))
                    .map_err(BeaconChainError::InvalidFinalizedPayloadShutdownError)?;
+
+                // Exit now, the node is in an invalid state.
+                return Ok(());
            }

            // Due to race conditions, it's technically possible that the head we load here is
--- a/beacon_node/execution_layer/Cargo.toml
+++ b/beacon_node/execution_layer/Cargo.toml
@ -13,7 +13,7 @@ slog = "2.5.2"
 futures = "0.3.7"
 sensitive_url = { path = "../../common/sensitive_url" }
 reqwest = { version = "0.11.0", features = ["json","stream"] }
-eth2_serde_utils = { path = "../../consensus/serde_utils" }
+eth2_serde_utils = "0.1.1"
 serde_json = "1.0.58"
 serde = { version = "1.0.116", features = ["derive"] }
 eth1 = { path = "../eth1" }
--- a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs
+++ b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs
@ -747,9 +747,7 @@ impl<T: BeaconChainTypes> Worker<T> {
                self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore);
                return None;
            }
-            // TODO: check that this is what we're supposed to do when we don't want to
-            //       penalize a peer for our configuration issue
-            // in the verification process BUT is this the proper way to handle it?
+            // TODO(merge): reconsider peer scoring for this event.
            Err(e @BlockError::ExecutionPayloadError(ExecutionPayloadError::RequestFailed(_)))
            | Err(e @BlockError::ExecutionPayloadError(ExecutionPayloadError::NoExecutionConnection)) => {
                debug!(self.log, "Could not verify block for gossip, ignoring the block";
--- a/beacon_node/src/cli.rs
+++ b/beacon_node/src/cli.rs
@ -406,9 +406,7 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
                       collected from any blocks produced by this node. Defaults to a junk \
                       address whilst the merge is in development stages. THE DEFAULT VALUE \
                       WILL BE REMOVED BEFORE THE MERGE ENTERS PRODUCTION")
-                // TODO: remove this default value. It's just there to make life easy during merge
-                // testnets.
-                .default_value("0x0000000000000000000000000000000000000001"),
+                .requires("merge")
        )

        /*
--- a/beacon_node/src/config.rs
+++ b/beacon_node/src/config.rs
@ -14,7 +14,12 @@ use std::net::{IpAddr, Ipv4Addr, ToSocketAddrs};
 use std::net::{TcpListener, UdpSocket};
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
-use types::{Checkpoint, Epoch, EthSpec, Hash256, PublicKeyBytes, GRAFFITI_BYTES_LEN};
+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_FEE_RECIPIENT: [u8; 20] =
+    [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1];

 /// Gets the fully-initialized global client.
 ///
@ -38,12 +43,18 @@ pub fn get_config<E: EthSpec>(
    // If necessary, remove any existing database and configuration
    if client_config.data_dir.exists() && cli_args.is_present("purge-db") {
        // Remove the chain_db.
-        fs::remove_dir_all(client_config.get_db_path())
+        let chain_db = client_config.get_db_path();
+        if chain_db.exists() {
+            fs::remove_dir_all(chain_db)
                .map_err(|err| format!("Failed to remove chain_db: {}", err))?;
+        }

        // Remove the freezer db.
-        fs::remove_dir_all(client_config.get_freezer_db_path())
-            .map_err(|err| format!("Failed to remove chain_db: {}", err))?;
+        let freezer_db = client_config.get_freezer_db_path();
+        if freezer_db.exists() {
+            fs::remove_dir_all(freezer_db)
+                .map_err(|err| format!("Failed to remove freezer_db: {}", err))?;
+        }
    }

    // Create `datadir` and any non-existing parent directories.
@ -242,7 +253,12 @@ pub fn get_config<E: EthSpec>(
        client_config.execution_endpoints = Some(client_config.eth1.endpoints.clone());
    }

-    client_config.fee_recipient = clap_utils::parse_optional(cli_args, "fee-recipient")?;
+    client_config.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_FEE_RECIPIENT)),
+    );

    if let Some(freezer_dir) = cli_args.value_of("freezer-dir") {
        client_config.freezer_db_path = Some(PathBuf::from(freezer_dir));
--- a/common/eth2/Cargo.toml
+++ b/common/eth2/Cargo.toml
@ -13,7 +13,7 @@ types = { path = "../../consensus/types" }
 reqwest = { version = "0.11.0", features = ["json","stream"] }
 lighthouse_network = { path = "../../beacon_node/lighthouse_network" }
 proto_array = { path = "../../consensus/proto_array", optional = true }
-eth2_serde_utils = "0.1.0"
+eth2_serde_utils = "0.1.1"
 eth2_keystore = { path = "../../crypto/eth2_keystore" }
 libsecp256k1 = "0.6.0"
 ring = "0.16.19"
--- a/consensus/proto_array/src/proto_array_fork_choice.rs
+++ b/consensus/proto_array/src/proto_array_fork_choice.rs
@ -296,7 +296,7 @@ impl ProtoArrayForkChoice {
    }

    /// Only used for SSZ deserialization of the persisted fork choice during the database migration
-    /// from schema 4 to schema 5.
+    /// from schema 5 to schema 6.
    pub fn from_bytes_legacy(bytes: &[u8]) -> Result<Self, String> {
        LegacySszContainer::from_ssz_bytes(bytes)
            .map(|legacy_container| {
--- a/consensus/proto_array/src/ssz_container.rs
+++ b/consensus/proto_array/src/ssz_container.rs
@ -19,7 +19,7 @@ pub struct SszContainer {
 }

 /// Only used for SSZ deserialization of the persisted fork choice during the database migration
-/// from schema 4 to schema 5.
+/// from schema 5 to schema 6.
 #[derive(Encode, Decode)]
 pub struct LegacySszContainer {
    votes: Vec<VoteTracker>,
--- a/consensus/serde_utils/Cargo.toml
+++ b/consensus/serde_utils/Cargo.toml
@ -1,6 +1,6 @@
 [package]
 name = "eth2_serde_utils"
-version = "0.1.0"
+version = "0.1.1"
 authors = ["Paul Hauner <paul@paulhauner.com", "Michael Sproul <michael@sigmaprime.io>"]
 edition = "2018"
 description = "Serialization and deserialization utilities useful for JSON representations of Ethereum 2.0 types."
--- a/consensus/ssz_types/Cargo.toml
+++ b/consensus/ssz_types/Cargo.toml
@ -13,7 +13,7 @@ name = "ssz_types"
 tree_hash = "0.4.1"
 serde = "1.0.116"
 serde_derive = "1.0.116"
-eth2_serde_utils = "0.1.0"
+eth2_serde_utils = "0.1.1"
 eth2_ssz = "0.4.1"
 typenum = "1.12.0"
 arbitrary = { version = "1.0", features = ["derive"], optional = true }
--- a/consensus/types/Cargo.toml
+++ b/consensus/types/Cargo.toml
@ -38,7 +38,7 @@ tempfile = "3.1.0"
 derivative = "2.1.1"
 rusqlite = { version = "0.25.3", features = ["bundled"], optional = true }
 arbitrary = { version = "1.0", features = ["derive"], optional = true }
-eth2_serde_utils = { path = "../serde_utils" }
+eth2_serde_utils = "0.1.1"
 regex = "1.3.9"
 lazy_static = "1.4.0"
 parking_lot = "0.11.1"
--- a/consensus/types/src/execution_payload.rs
+++ b/consensus/types/src/execution_payload.rs
@ -40,24 +40,8 @@ pub struct ExecutionPayload<T: EthSpec> {
 }

 impl<T: EthSpec> ExecutionPayload<T> {
-    // TODO: check this whole thing later
    pub fn empty() -> Self {
-        Self {
-            parent_hash: Hash256::zero(),
-            coinbase: Address::default(),
-            state_root: Hash256::zero(),
-            receipt_root: Hash256::zero(),
-            logs_bloom: FixedVector::default(),
-            random: Hash256::zero(),
-            block_number: 0,
-            gas_limit: 0,
-            gas_used: 0,
-            timestamp: 0,
-            extra_data: VariableList::empty(),
-            base_fee_per_gas: Uint256::zero(),
-            block_hash: Hash256::zero(),
-            transactions: VariableList::empty(),
-        }
+        Self::default()
    }

    /// Returns the ssz size of `self`.
--- a/consensus/types/src/execution_payload_header.rs
+++ b/consensus/types/src/execution_payload_header.rs
@ -33,7 +33,6 @@ pub struct ExecutionPayloadHeader<T: EthSpec> {
 }

 impl<T: EthSpec> ExecutionPayloadHeader<T> {
-    // TODO: check this whole thing later
    pub fn empty() -> Self {
        Self::default()
    }
--- a/consensus/types/src/test_utils/test_random/hash256.rs
+++ b/consensus/types/src/test_utils/test_random/hash256.rs
@ -1,10 +1,10 @@
 use super::*;
-use crate::Uint256;
+use crate::Hash256;

-impl TestRandom for Uint256 {
+impl TestRandom for Hash256 {
    fn random_for_test(rng: &mut impl RngCore) -> Self {
-        let mut key_bytes = [0; 32];
+        let mut key_bytes = vec![0; 32];
        rng.fill_bytes(&mut key_bytes);
-        Self::from_little_endian(&key_bytes[..])
+        Hash256::from_slice(&key_bytes[..])
    }
 }
--- a/consensus/types/src/test_utils/test_random/uint256.rs
+++ b/consensus/types/src/test_utils/test_random/uint256.rs
@ -1,10 +1,10 @@
 use super::*;
-use crate::Hash256;
+use crate::Uint256;

-impl TestRandom for Hash256 {
+impl TestRandom for Uint256 {
    fn random_for_test(rng: &mut impl RngCore) -> Self {
-        let mut key_bytes = vec![0; 32];
+        let mut key_bytes = [0; 32];
        rng.fill_bytes(&mut key_bytes);
-        Hash256::from_slice(&key_bytes[..])
+        Self::from_little_endian(&key_bytes[..])
    }
 }
--- a/crypto/bls/Cargo.toml
+++ b/crypto/bls/Cargo.toml
@ -11,7 +11,7 @@ milagro_bls = { git = "https://github.com/sigp/milagro_bls", tag = "v1.4.2", opt
 rand = "0.7.3"
 serde = "1.0.116"
 serde_derive = "1.0.116"
-eth2_serde_utils = "0.1.0"
+eth2_serde_utils = "0.1.1"
 hex = "0.4.2"
 eth2_hashing = "0.2.0"
 ethereum-types = "0.12.1"
--- a/validator_client/Cargo.toml
+++ b/validator_client/Cargo.toml
@ -45,7 +45,7 @@ lighthouse_version = { path = "../common/lighthouse_version" }
 warp_utils = { path = "../common/warp_utils" }
 warp = "0.3.2"
 hyper = "0.14.4"
-eth2_serde_utils = "0.1.0"
+eth2_serde_utils = "0.1.1"
 libsecp256k1 = "0.6.0"
 ring = "0.16.19"
 rand = "0.7.3"
--- a/validator_client/slashing_protection/Cargo.toml
+++ b/validator_client/slashing_protection/Cargo.toml
@ -13,7 +13,7 @@ r2d2_sqlite = "0.18.0"
 serde = "1.0.116"
 serde_derive = "1.0.116"
 serde_json = "1.0.58"
-eth2_serde_utils = "0.1.0"
+eth2_serde_utils = "0.1.1"
 filesystem = { path = "../../common/filesystem" }

 [dev-dependencies]