From d0beecca20803445b19a68993af210fc4d7fc99f Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 2 Aug 2022 07:58:42 +0000 Subject: [PATCH 01/21] Make fork choice prune again (#3408) ## Issue Addressed NA ## Proposed Changes There was a regression in #3244 (released in v2.4.0) which stopped pruning fork choice (see [here](https://github.com/sigp/lighthouse/pull/3244#discussion_r935187485)). This would form a very slow memory leak, using ~100mb per month. The release has been out for ~11 days, so users should not be seeing a dangerous increase in memory, *yet*. Credits to @michaelsproul for noticing this :tada: ## Additional Info NA --- beacon_node/beacon_chain/src/canonical_head.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/beacon_node/beacon_chain/src/canonical_head.rs b/beacon_node/beacon_chain/src/canonical_head.rs index 709382f05b..6559487980 100644 --- a/beacon_node/beacon_chain/src/canonical_head.rs +++ b/beacon_node/beacon_chain/src/canonical_head.rs @@ -719,6 +719,9 @@ impl BeaconChain { drop(old_cached_head); // If the finalized checkpoint changed, perform some updates. + // + // The `after_finalization` function will take a write-lock on `fork_choice`, therefore it + // is a dead-lock risk to hold any other lock on fork choice at this point. if new_view.finalized_checkpoint != old_view.finalized_checkpoint { if let Err(e) = self.after_finalization(&new_cached_head, new_view, finalized_proto_block) @@ -878,6 +881,9 @@ impl BeaconChain { /// Perform updates to caches and other components after the finalized checkpoint has been /// changed. + /// + /// This function will take a write-lock on `canonical_head.fork_choice`, therefore it would be + /// unwise to hold any lock on fork choice while calling this function. fn after_finalization( self: &Arc, new_cached_head: &CachedHead, @@ -966,6 +972,9 @@ impl BeaconChain { self.head_tracker.clone(), )?; + // Take a write-lock on the canonical head and signal for it to prune. + self.canonical_head.fork_choice_write_lock().prune()?; + Ok(()) } From e24552d61ad34e388ddc14744dd66f2a68a8c428 Mon Sep 17 00:00:00 2001 From: Mac L Date: Tue, 2 Aug 2022 23:20:51 +0000 Subject: [PATCH 02/21] Restore backwards compatibility when using older BNs (#3410) ## Issue Addressed https://github.com/status-im/nimbus-eth2/issues/3930 ## Proposed Changes We can trivially support beacon nodes which do not provide the `is_optimistic` field by wrapping the field in an `Option`. --- beacon_node/http_api/src/lib.rs | 2 +- beacon_node/http_api/tests/tests.rs | 2 +- common/eth2/src/types.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index a8e305f3c1..0152f20e98 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -1704,7 +1704,7 @@ pub fn serve( let syncing_data = api_types::SyncingData { is_syncing: network_globals.sync_state.read().is_syncing(), - is_optimistic, + is_optimistic: Some(is_optimistic), head_slot, sync_distance, }; diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index 38c06848cf..cc0281e454 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -1300,7 +1300,7 @@ impl ApiTester { let expected = SyncingData { is_syncing: false, - is_optimistic: false, + is_optimistic: Some(false), head_slot, sync_distance, }; diff --git a/common/eth2/src/types.rs b/common/eth2/src/types.rs index 3e480e0827..340d38b85a 100644 --- a/common/eth2/src/types.rs +++ b/common/eth2/src/types.rs @@ -552,7 +552,7 @@ pub struct VersionData { #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct SyncingData { pub is_syncing: bool, - pub is_optimistic: bool, + pub is_optimistic: Option, pub head_slot: Slot, pub sync_distance: Slot, } From 553a7949942b74df45357e0a699c1e3006252326 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 2 Aug 2022 23:20:52 +0000 Subject: [PATCH 03/21] Ignore RUSTSEC-2022-0040 - `owning_ref` soundness (#3415) ## Issue Addressed NA ## Proposed Changes We are unaffected by this issue: https://github.com/sigp/lighthouse/pull/3410#issuecomment-1203244792 ## Additional Info NA --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 53fd4143d9..55e987be8b 100644 --- a/Makefile +++ b/Makefile @@ -169,7 +169,7 @@ arbitrary-fuzz: # Runs cargo audit (Audit Cargo.lock files for crates with security vulnerabilities reported to the RustSec Advisory Database) audit: cargo install --force cargo-audit - cargo audit --ignore RUSTSEC-2020-0071 --ignore RUSTSEC-2020-0159 + cargo audit --ignore RUSTSEC-2020-0071 --ignore RUSTSEC-2020-0159 --ignore RUSTSEC-2022-0040 # Runs `cargo vendor` to make sure dependencies can be vendored for packaging, reproducibility and archival purpose. vendor: From df51a73272489fe154bd10995c96199062b6c3f7 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 3 Aug 2022 04:23:09 +0000 Subject: [PATCH 04/21] Release v2.5.1 (#3406) ## Issue Addressed Patch release to address fork choice issues in the presence of clock drift: https://github.com/sigp/lighthouse/pull/3402 --- Cargo.lock | 8 ++++---- beacon_node/Cargo.toml | 2 +- boot_node/Cargo.toml | 2 +- common/lighthouse_version/src/lib.rs | 4 ++-- lcli/Cargo.toml | 2 +- lighthouse/Cargo.toml | 2 +- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c42dd38ac6..1160609bec 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -436,7 +436,7 @@ dependencies = [ [[package]] name = "beacon_node" -version = "2.5.0" +version = "2.5.1" dependencies = [ "beacon_chain", "clap", @@ -593,7 +593,7 @@ dependencies = [ [[package]] name = "boot_node" -version = "2.5.0" +version = "2.5.1" dependencies = [ "beacon_node", "clap", @@ -3102,7 +3102,7 @@ checksum = "830d08ce1d1d941e6b30645f1a0eb5643013d835ce3779a5fc208261dbe10f55" [[package]] name = "lcli" -version = "2.5.0" +version = "2.5.1" dependencies = [ "account_utils", "bls", @@ -3601,7 +3601,7 @@ dependencies = [ [[package]] name = "lighthouse" -version = "2.5.0" +version = "2.5.1" dependencies = [ "account_manager", "account_utils", diff --git a/beacon_node/Cargo.toml b/beacon_node/Cargo.toml index ef430c2bc3..9c6385e8ed 100644 --- a/beacon_node/Cargo.toml +++ b/beacon_node/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "beacon_node" -version = "2.5.0" +version = "2.5.1" authors = ["Paul Hauner ", "Age Manning "] edition = "2021" diff --git a/common/lighthouse_version/src/lib.rs b/common/lighthouse_version/src/lib.rs index 7ba1afac60..f5d4d44878 100644 --- a/common/lighthouse_version/src/lib.rs +++ b/common/lighthouse_version/src/lib.rs @@ -17,8 +17,8 @@ pub const VERSION: &str = git_version!( // NOTE: using --match instead of --exclude for compatibility with old Git "--match=thiswillnevermatchlol" ], - prefix = "Lighthouse/v2.5.0-", - fallback = "Lighthouse/v2.5.0" + prefix = "Lighthouse/v2.5.1-", + fallback = "Lighthouse/v2.5.1" ); /// Returns `VERSION`, but with platform information appended to the end. diff --git a/lcli/Cargo.toml b/lcli/Cargo.toml index dfc8aac7bd..e54d9d8c95 100644 --- a/lcli/Cargo.toml +++ b/lcli/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "lcli" description = "Lighthouse CLI (modeled after zcli)" -version = "2.5.0" +version = "2.5.1" authors = ["Paul Hauner "] edition = "2021" diff --git a/lighthouse/Cargo.toml b/lighthouse/Cargo.toml index da4ca81884..7792ad074e 100644 --- a/lighthouse/Cargo.toml +++ b/lighthouse/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "lighthouse" -version = "2.5.0" +version = "2.5.1" authors = ["Sigma Prime "] edition = "2021" autotests = false From fe6af05bf6672bb7f8e2577d0a571eee7a83bc67 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 3 Aug 2022 17:13:14 +0000 Subject: [PATCH 05/21] Use latest Geth release in EE integration tests (#3395) ## Issue Addressed NA ## Proposed Changes This PR reverts #3382 and adds the `--syncmode=full` as described here: https://github.com/sigp/lighthouse/pull/3382#issuecomment-1197680345 ## Additional Info - Blocked on #3392 --- testing/execution_engine_integration/src/geth.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/testing/execution_engine_integration/src/geth.rs b/testing/execution_engine_integration/src/geth.rs index ae5210b2a3..467fd8b430 100644 --- a/testing/execution_engine_integration/src/geth.rs +++ b/testing/execution_engine_integration/src/geth.rs @@ -7,7 +7,7 @@ use std::{env, fs::File}; use tempfile::TempDir; use unused_port::unused_tcp_port; -// const GETH_BRANCH: &str = "master"; +const GETH_BRANCH: &str = "master"; const GETH_REPO_URL: &str = "https://github.com/ethereum/go-ethereum"; pub fn build_result(repo_dir: &Path) -> Output { @@ -26,13 +26,8 @@ pub fn build(execution_clients_dir: &Path) { build_utils::clone_repo(execution_clients_dir, GETH_REPO_URL).unwrap(); } - // TODO: this should be set back to the latest release once the following issue is resolved: - // - // - https://github.com/ethereum/go-ethereum/issues/25427 - // // Get the latest tag on the branch - // let last_release = build_utils::get_latest_release(&repo_dir, GETH_BRANCH).unwrap(); - let last_release = "v1.10.20"; + let last_release = build_utils::get_latest_release(&repo_dir, GETH_BRANCH).unwrap(); build_utils::checkout(&repo_dir, dbg!(&last_release)).unwrap(); // Build geth @@ -105,6 +100,11 @@ impl GenericExecutionEngine for GethEngine { .arg("--allow-insecure-unlock") .arg("--authrpc.jwtsecret") .arg(jwt_secret_path.as_path().to_str().unwrap()) + // This flag is required to help Geth perform reliably when feeding it blocks + // one-by-one. For more information, see: + // + // https://github.com/sigp/lighthouse/pull/3382#issuecomment-1197680345 + .arg("--syncmode=full") .stdout(build_utils::build_stdio()) .stderr(build_utils::build_stdio()) .spawn() From 43ce0de73f992d0f184c722e58ab203b7086fead Mon Sep 17 00:00:00 2001 From: realbigsean Date: Wed, 3 Aug 2022 17:13:15 +0000 Subject: [PATCH 06/21] Downgrade log for 204 from builder (#3411) ## Issue Addressed A 204 from the connected builder just indicates there's no payload available from the builder, not that there's an issue. So I don't actually think this should be a warn. During the merge transition when we are pre-finalization a 204 will actually be expected. And maybe even longer if the relay chooses to delay providing payloads for a longer period post-merge. Co-authored-by: realbigsean --- beacon_node/execution_layer/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index aea952a57d..59c8f009fa 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -605,7 +605,7 @@ impl ExecutionLayer { Ok(local) } (Ok(None), Ok(local)) => { - warn!( + info!( self.log(), "No payload provided by connected builder. \ Attempting to propose through local execution engine" From 386ced1aed4c493d8e8cbbd357d94e8cd949e588 Mon Sep 17 00:00:00 2001 From: Ramana Kumar Date: Fri, 5 Aug 2022 01:51:39 +0000 Subject: [PATCH 07/21] Include validator indices in attestation logs (#3393) ## Issue Addressed Fixes #2967 ## Proposed Changes Collect validator indices alongside attestations when creating signed attestations (and aggregates) for inclusion in the logs. ## Additional Info This is my first time looking at Lighthouse source code and using Rust, so newbie feedback appreciated! --- validator_client/src/attestation_service.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/validator_client/src/attestation_service.rs b/validator_client/src/attestation_service.rs index 95500fc947..cdc9b88f68 100644 --- a/validator_client/src/attestation_service.rs +++ b/validator_client/src/attestation_service.rs @@ -389,12 +389,13 @@ impl AttestationService { ) .await { - Ok(()) => Some(attestation), + Ok(()) => Some((attestation, duty.validator_index)), Err(e) => { crit!( log, "Failed to sign attestation"; "error" => ?e, + "validator" => ?duty.pubkey, "committee_index" => committee_index, "slot" => slot.as_u64(), ); @@ -404,11 +405,11 @@ impl AttestationService { }); // Execute all the futures in parallel, collecting any successful results. - let attestations = &join_all(signing_futures) + let (ref attestations, ref validator_indices): (Vec<_>, Vec<_>) = join_all(signing_futures) .await .into_iter() .flatten() - .collect::>>(); + .unzip(); // Post the attestations to the BN. match self @@ -428,6 +429,7 @@ impl AttestationService { log, "Successfully published attestations"; "count" => attestations.len(), + "validator_indices" => ?validator_indices, "head_block" => ?attestation_data.beacon_block_root, "committee_index" => attestation_data.index, "slot" => attestation_data.slot.as_u64(), @@ -549,7 +551,7 @@ impl AttestationService { let attestation = &signed_aggregate_and_proof.message.aggregate; info!( log, - "Successfully published attestations"; + "Successfully published attestation"; "aggregator" => signed_aggregate_and_proof.message.aggregator_index, "signatures" => attestation.aggregation_bits.num_set_bits(), "head_block" => format!("{:?}", attestation.data.beacon_block_root), @@ -566,6 +568,7 @@ impl AttestationService { log, "Failed to publish attestation"; "error" => %e, + "aggregator" => signed_aggregate_and_proof.message.aggregator_index, "committee_index" => attestation.data.index, "slot" => attestation.data.slot.as_u64(), "type" => "aggregated", From 5d317779bb1f77d4aa563f70736b763993ed7ab8 Mon Sep 17 00:00:00 2001 From: Mac L Date: Fri, 5 Aug 2022 06:46:58 +0000 Subject: [PATCH 08/21] Ensure `validator/blinded_blocks/{slot}` endpoint conforms to spec (#3429) ## Issue Addressed #3418 ## Proposed Changes - Remove `eth/v2/validator/blinded_blocks/{slot}` as this endpoint does not exist in the spec. - Return `version` in the `eth/v1/validator/blinded_blocks/{slot}` endpoint. ## Additional Info Since this removes the `v2` endpoint, this is *technically* a breaking change, but as this does not exist in the spec users may or may not be relying on this. Depending on what we feel is appropriate, I'm happy to edit this so we keep the `v2` endpoint for now but simply bring the `v1` endpoint in line with `v2`. --- beacon_node/http_api/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 0152f20e98..ba1dd01cc3 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -1988,7 +1988,7 @@ pub fn serve( ); // GET validator/blinded_blocks/{slot} - let get_validator_blinded_blocks = any_version + let get_validator_blinded_blocks = eth_v1 .and(warp::path("validator")) .and(warp::path("blinded_blocks")) .and(warp::path::param::().or_else(|_| async { @@ -2001,8 +2001,7 @@ pub fn serve( .and(warp::query::()) .and(chain_filter.clone()) .and_then( - |endpoint_version: EndpointVersion, - slot: Slot, + |slot: Slot, query: api_types::ValidatorBlocksQuery, chain: Arc>| async move { let randao_reveal = query.randao_reveal.as_ref().map_or_else( @@ -2044,7 +2043,8 @@ pub fn serve( .to_ref() .fork_name(&chain.spec) .map_err(inconsistent_fork_rejection)?; - fork_versioned_response(endpoint_version, fork_name, block) + // Pose as a V2 endpoint so we return the fork `version`. + fork_versioned_response(V2, fork_name, block) .map(|response| warp::reply::json(&response)) }, ); From 83666e04fd0aaff31249f94c7c01383a20927f58 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 5 Aug 2022 06:46:59 +0000 Subject: [PATCH 09/21] Expand merge migration docs (#3430) ## Issue Addressed Resolves #3424 ## Proposed Changes This PR expands the merge migration docs to include (hopefully) clearer guidance on the steps required. It's inspired by @winksaville's work in #3426 but takes a more drastic approach to rewriting large sections. * Add a prominent _When?_ section * Add links to execution engine configuration guides * Add links to community guides * Fix the location of the _Strict fee recipient_ section --- book/src/merge-migration.md | 96 ++++++++++++++++++++++++----- book/src/suggested-fee-recipient.md | 41 +++++++----- 2 files changed, 108 insertions(+), 29 deletions(-) diff --git a/book/src/merge-migration.md b/book/src/merge-migration.md index 6ed6a9977a..e2d54ea0aa 100644 --- a/book/src/merge-migration.md +++ b/book/src/merge-migration.md @@ -1,20 +1,69 @@ # Merge Migration -This document provides detail for users who have been running a Lighthouse node *before* the merge -and are now preparing their node for the merge transition. +This document provides detail for users who want to run a merge-ready Lighthouse node. -## "Pre-Merge" and "Post-Merge" +> If you are running a testnet node, this configuration is necessary _now_. -As of [v2.4.0](https://github.com/sigp/lighthouse/releases/tag/v2.4.0) Lighthouse can be considered -to have two modes: +## Necessary Configuration -- "Pre-merge": `--execution-endpoint` flag *is not* provided. -- "Post-merge": `--execution-endpoint` flag *is* provided. +There are two configuration changes required for a Lighthouse node to operate correctly throughout +the merge: -A "pre-merge" node, by definition, will fail to transition through the merge. Such a node *must* be -upgraded before the Bellatrix upgrade. +1. You *must* run your own execution engine such as Geth or Nethermind alongside Lighthouse. + You *must* update your Lighthouse configuration to connect to the execution engine using new + flags which are documented on this page in the + [Connecting to an execution engine](#connecting-to-an-execution-engine) section. +2. If your Lighthouse node has validators attached you *must* nominate an Ethereum address to + receive transactions tips from blocks proposed by your validators. This is covered on the + [Suggested fee recipient](./suggested-fee-recipient.md) page. -## Migration +Additionally, you _must_ update Lighthouse to a merge-compatible release in the weeks before +the merge. Merge releases are available now for all testnets. + +## When? + +You must configure your node to be merge-ready before the Bellatrix fork occurs on the network +on which your node is operating. + +* **Mainnet**: the Bellatrix fork epoch has not yet been announced. It's possible to set up a + merge-ready node now, but some execution engines will require additional configuration. Please see + the section on [Execution engine configuration](#execution-engine-configuration) below. + +* **Goerli (Prater)**, **Ropsten**, **Sepolia**, **Kiln**: you must have a merge-ready configuration + right now. + +## Connecting to an execution engine + +The Lighthouse beacon node must connect to an execution engine in order to validate the transactions +present in post-merge blocks. Two new flags are used to configure this connection: + +- `--execution-endpoint `: the URL of the execution engine API. Often this will be + `http://localhost:8551`. +- `--execution-jwt `: the path to the file containing the JWT secret shared by Lighthouse and the + execution engine. + +If you set up an execution engine with `--execution-endpoint` then you *must* provide a JWT secret +using `--execution-jwt`. This is a mandatory form of authentication that ensures that Lighthouse +has authority to control the execution engine. + +### Execution engine configuration + +Each execution engine has its own flags for configuring the engine API and JWT. Please consult +the relevant page for your execution engine for the required flags: + +- [Geth: Connecting to Consensus Clients](https://geth.ethereum.org/docs/interface/consensus-clients) +- [Nethermind: Running Nethermind Post Merge](https://docs.nethermind.io/nethermind/first-steps-with-nethermind/running-nethermind-post-merge) +- [Besu: Prepare For The Merge](https://besu.hyperledger.org/en/stable/HowTo/Upgrade/Prepare-for-The-Merge/) + +Once you have configured your execution engine to open up the engine API (usually on port 8551) you +should add the URL to your `lighthouse bn` flags with `--execution-endpoint `, as well as +the path to the JWT secret with `--execution-jwt `. + +> NOTE: Geth v1.10.21 or earlier requires a manual TTD override to communicate with Lighthouse over +> the engine API on mainnet. We recommend waiting for a compatible Geth release before configuring +> Lighthouse-Geth on mainnet. + +### Example Let us look at an example of the command line arguments for a pre-merge production staking BN: @@ -60,10 +109,7 @@ merge are ensuring that `--execution-endpoint` and `--execution-jwt` flags are p you can even leave the `--eth1-endpoints` flag there, it will be ignored. This is not recommended as a deprecation warning will be logged and Lighthouse *may* remove these flags in the future. -There are no changes required for the validator client, apart from ensure it has been updated to the -same version as the beacon node. Check the version with `lighthouse --version`. - -## The relationship between `--eth1-endpoints` and `--execution-endpoint` +### The relationship between `--eth1-endpoints` and `--execution-endpoint` Pre-merge users will be familiar with the `--eth1-endpoints` flag. This provides a list of Ethereum "eth1" nodes (e.g., Geth, Nethermind, etc). Each beacon node (BN) can have multiple eth1 endpoints @@ -90,7 +136,16 @@ contains the transaction history of the Ethereum chain, there is no longer a nee be used for all such queries. Therefore we can say that where `--execution-endpoint` is included `--eth1-endpoints` should be omitted. -## What about multiple execution endpoints? +## FAQ + +### Can I use `http://localhost:8545` for the execution endpoint? + +Most execution nodes use port `8545` for the Ethereum JSON-RPC API. Unless custom configuration is +used, an execution node _will not_ provide the necessary engine API on port `8545`. You should +not attempt to use `http://localhost:8545` as your engine URL and should instead use +`http://localhost:8551`. + +### What about multiple execution endpoints? Since an execution engine can only have one connected BN, the value of having multiple execution engines connected to the same BN is very low. An execution engine cannot be shared between BNs to @@ -99,3 +154,14 @@ reduce costs. Whilst having multiple execution engines connected to a single BN might be useful for advanced testing scenarios, Lighthouse (and other consensus clients) have decided to support *only one* execution endpoint. Such scenarios could be resolved with a custom-made HTTP proxy. + +## Additional Resources + +There are several community-maintained guides which provide more background information, as well as +guidance for specific setups. + +- [Ethereum.org: The Merge](https://ethereum.org/en/upgrades/merge/) +- [Ethereum Staking Launchpad: Merge Readiness](https://launchpad.ethereum.org/en/merge-readiness). +- [CoinCashew: Ethereum Merge Upgrade Checklist](https://www.coincashew.com/coins/overview-eth/ethereum-merge-upgrade-checklist-for-home-stakers-and-validators) +- [EthDocker: Merge Preparation](https://eth-docker.net/docs/About/MergePrep/) +- [Remy Roy: How to join the Goerli/Prater merge testnet](https://github.com/remyroy/ethstaker/blob/main/merge-goerli-prater.md) diff --git a/book/src/suggested-fee-recipient.md b/book/src/suggested-fee-recipient.md index a584be306f..d862cf1a6c 100644 --- a/book/src/suggested-fee-recipient.md +++ b/book/src/suggested-fee-recipient.md @@ -1,8 +1,10 @@ # Suggested Fee Recipient -*Note: these documents are not relevant until the Bellatrix (Merge) upgrade has occurred.* +The _fee recipient_ is an Ethereum address nominated by a beacon chain validator to receive +tips from user transactions. If you run validators on a network that has already merged +or is due to merge soon then you should nominate a fee recipient for your validators. -## Fee recipient trust assumptions +## Background 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 @@ -13,32 +15,25 @@ it may use any address it chooses. It is assumed that an honest execution node * `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 +The `suggested_fee_recipient` can be provided to the VC, which will transmit it to the BN. The BN also 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 +## How to configure a suggested fee recipient + +The Lighthouse VC provides two 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: +Assuming trustworthy nodes, the priority for the three methods is: 1. `validator_definitions.yml` 1. `--suggested-fee-recipient` provided to the VC. 1. `--suggested-fee-recipient` provided 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 and 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. - ### 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` @@ -168,4 +163,22 @@ curl -X DELETE \ null ``` +## 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 and 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. + +## FAQ + +### Why do I have to nominate an Ethereum address as the fee recipient? + +You might wonder why the validator can't just accumulate transactions fees in the same way that it +accumulates other staking rewards. The reason for this is that transaction fees are computed and +validated by the execution node, and therefore need to be paid to an address that exists on the +execution chain. Validators use BLS keys which do not correspond to Ethereum addresses, so they +have no "presence" on the execution chain. Therefore it's necessary for each validator to nominate +a separate fee recipient address. From 6bc4a2cc91193438db698006740747a4b83664ef Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 5 Aug 2022 23:41:09 +0000 Subject: [PATCH 10/21] Update invalid head tests (#3400) ## Proposed Changes Update the invalid head tests so that they work with the current default fork choice configuration. Thanks @realbigsean for fixing the persistence test and the EF tests. Co-authored-by: realbigsean --- beacon_node/beacon_chain/src/chain_config.rs | 2 +- beacon_node/beacon_chain/src/test_utils.rs | 11 +- .../beacon_chain/tests/block_verification.rs | 28 ++-- .../tests/payload_invalidation.rs | 144 +++++++++--------- testing/ef_tests/src/cases/fork_choice.rs | 2 +- 5 files changed, 101 insertions(+), 86 deletions(-) diff --git a/beacon_node/beacon_chain/src/chain_config.rs b/beacon_node/beacon_chain/src/chain_config.rs index 2c43ca53ed..aa7ff02af1 100644 --- a/beacon_node/beacon_chain/src/chain_config.rs +++ b/beacon_node/beacon_chain/src/chain_config.rs @@ -51,7 +51,7 @@ impl Default for ChainConfig { builder_fallback_skips_per_epoch: 8, builder_fallback_epochs_since_finalization: 3, builder_fallback_disable_checks: false, - count_unrealized: false, + count_unrealized: true, } } } diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 6771861dfd..411bd7b1fd 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -157,6 +157,7 @@ pub struct Builder { execution_layer: Option>, mock_execution_layer: Option>, mock_builder: Option>, + testing_slot_clock: Option, runtime: TestRuntime, log: Logger, } @@ -289,6 +290,7 @@ where execution_layer: None, mock_execution_layer: None, mock_builder: None, + testing_slot_clock: None, runtime, log, } @@ -435,6 +437,11 @@ where self } + pub fn testing_slot_clock(mut self, slot_clock: TestingSlotClock) -> Self { + self.testing_slot_clock = Some(slot_clock); + self + } + pub fn build(self) -> BeaconChainHarness> { let (shutdown_tx, shutdown_receiver) = futures::channel::mpsc::channel(1); @@ -475,7 +482,9 @@ where }; // Initialize the slot clock only if it hasn't already been initialized. - builder = if builder.get_slot_clock().is_none() { + builder = if let Some(testing_slot_clock) = self.testing_slot_clock { + builder.slot_clock(testing_slot_clock) + } else if builder.get_slot_clock().is_none() { builder .testing_slot_clock(Duration::from_secs(seconds_per_slot)) .expect("should configure testing slot clock") diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index 88d6914036..c2283321cb 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -327,6 +327,9 @@ async fn assert_invalid_signature( item ); + // Call fork choice to update cached head (including finalization). + harness.chain.recompute_head_at_current_slot().await; + // Ensure the block will be rejected if imported on its own (without gossip checking). let ancestor_blocks = chain_segment .iter() @@ -339,19 +342,20 @@ async fn assert_invalid_signature( .chain .process_chain_segment(ancestor_blocks, CountUnrealized::True) .await; + harness.chain.recompute_head_at_current_slot().await; + + let process_res = harness + .chain + .process_block( + snapshots[block_index].beacon_block.clone(), + CountUnrealized::True, + ) + .await; assert!( - matches!( - harness - .chain - .process_block( - snapshots[block_index].beacon_block.clone(), - CountUnrealized::True - ) - .await, - Err(BlockError::InvalidSignature) - ), - "should not import individual block with an invalid {} signature", - item + matches!(process_res, Err(BlockError::InvalidSignature)), + "should not import individual block with an invalid {} signature, got: {:?}", + item, + process_res ); // NOTE: we choose not to check gossip verification here. It only checks one signature diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index 5e03ef2335..7728b319d9 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -179,7 +179,7 @@ impl InvalidPayloadRig { /// Import a block while setting the newPayload and forkchoiceUpdated responses to `is_valid`. async fn import_block(&mut self, is_valid: Payload) -> Hash256 { - self.import_block_parametric(is_valid, is_valid, |error| { + self.import_block_parametric(is_valid, is_valid, None, |error| { matches!( error, BlockError::ExecutionPayloadError( @@ -210,13 +210,14 @@ impl InvalidPayloadRig { &mut self, new_payload_response: Payload, forkchoice_response: Payload, + slot_override: Option, evaluate_error: F, ) -> Hash256 { let mock_execution_layer = self.harness.mock_execution_layer.as_ref().unwrap(); let head = self.harness.chain.head_snapshot(); let state = head.beacon_state.clone_with_only_committee_caches(); - let slot = state.slot() + 1; + let slot = slot_override.unwrap_or(state.slot() + 1); let (block, post_state) = self.harness.make_block(state, slot).await; let block_root = block.canonical_root(); @@ -445,9 +446,12 @@ async fn immediate_forkchoice_update_invalid_test( // Import a block which returns syncing when supplied via newPayload, and then // invalid when the forkchoice update is sent. - rig.import_block_parametric(Payload::Syncing, invalid_payload(latest_valid_hash), |_| { - false - }) + rig.import_block_parametric( + Payload::Syncing, + invalid_payload(latest_valid_hash), + None, + |_| false, + ) .await; // The head should be the latest valid block. @@ -497,7 +501,7 @@ async fn justified_checkpoint_becomes_invalid() { let is_valid = Payload::Invalid { latest_valid_hash: Some(parent_hash_of_justified), }; - rig.import_block_parametric(is_valid, is_valid, |error| { + rig.import_block_parametric(is_valid, is_valid, None, |error| { matches!( error, // The block import should fail since the beacon chain knows the justified payload @@ -1757,11 +1761,11 @@ async fn optimistic_transition_block_invalid_finalized() { ); } -/// Helper for running tests where we generate a chain with an invalid head and then some -/// `fork_blocks` to recover it. +/// Helper for running tests where we generate a chain with an invalid head and then a +/// `fork_block` to recover it. struct InvalidHeadSetup { rig: InvalidPayloadRig, - fork_blocks: Vec>>, + fork_block: Arc>, invalid_head: CachedHead, } @@ -1776,11 +1780,59 @@ impl InvalidHeadSetup { rig.import_block(Payload::Syncing).await; } + let slots_per_epoch = E::slots_per_epoch(); + let start_slot = rig.cached_head().head_slot() + 1; + let mut opt_fork_block = None; + + assert_eq!(start_slot % slots_per_epoch, 1); + for i in 0..slots_per_epoch - 1 { + let slot = start_slot + i; + let slot_offset = slot.as_u64() % slots_per_epoch; + + rig.harness.set_current_slot(slot); + + if slot_offset == slots_per_epoch - 1 { + // Optimistic head block right before epoch boundary. + let is_valid = Payload::Syncing; + rig.import_block_parametric(is_valid, is_valid, Some(slot), |error| { + matches!( + error, + BlockError::ExecutionPayloadError( + ExecutionPayloadError::RejectedByExecutionEngine { .. } + ) + ) + }) + .await; + } else if 3 * slot_offset < 2 * slots_per_epoch { + // Valid block in previous epoch. + rig.import_block(Payload::Valid).await; + } else if slot_offset == slots_per_epoch - 2 { + // Fork block one slot prior to invalid head, not applied immediately. + let parent_state = rig + .harness + .chain + .state_at_slot(slot - 1, StateSkipConfig::WithStateRoots) + .unwrap(); + let (fork_block, _) = rig.harness.make_block(parent_state, slot).await; + opt_fork_block = Some(Arc::new(fork_block)); + } else { + // Skipped slot. + }; + } + let invalid_head = rig.cached_head(); + assert_eq!( + invalid_head.head_slot() % slots_per_epoch, + slots_per_epoch - 1 + ); + + // Advance clock to new epoch to realize the justification of soon-to-be-invalid head block. + rig.harness.set_current_slot(invalid_head.head_slot() + 1); // Invalidate the head block. rig.invalidate_manually(invalid_head.head_block_root()) .await; + assert!(rig .canonical_head() .head_execution_status() @@ -1790,27 +1842,9 @@ impl InvalidHeadSetup { // Finding a new head should fail since the only possible head is not valid. rig.assert_get_head_error_contains("InvalidBestNode"); - // Build three "fork" blocks that conflict with the current canonical head. Don't apply them to - // the chain yet. - let mut fork_blocks = vec![]; - let mut parent_state = rig - .harness - .chain - .state_at_slot( - invalid_head.head_slot() - 3, - StateSkipConfig::WithStateRoots, - ) - .unwrap(); - for _ in 0..3 { - let slot = parent_state.slot() + 1; - let (fork_block, post_state) = rig.harness.make_block(parent_state, slot).await; - parent_state = post_state; - fork_blocks.push(Arc::new(fork_block)) - } - Self { rig, - fork_blocks, + fork_block: opt_fork_block.unwrap(), invalid_head, } } @@ -1820,57 +1854,22 @@ impl InvalidHeadSetup { async fn recover_from_invalid_head_by_importing_blocks() { let InvalidHeadSetup { rig, - fork_blocks, - invalid_head, + fork_block, + invalid_head: _, } = InvalidHeadSetup::new().await; - // Import the first two blocks, they should not become the head. - for i in 0..2 { - if i == 0 { - // The first block should be `VALID` during import. - rig.harness - .mock_execution_layer - .as_ref() - .unwrap() - .server - .all_payloads_valid_on_new_payload(); - } else { - // All blocks after the first block should return `SYNCING`. - rig.harness - .mock_execution_layer - .as_ref() - .unwrap() - .server - .all_payloads_syncing_on_new_payload(true); - } - - rig.harness - .chain - .process_block(fork_blocks[i].clone(), CountUnrealized::True) - .await - .unwrap(); - rig.recompute_head().await; - rig.assert_get_head_error_contains("InvalidBestNode"); - let new_head = rig.cached_head(); - assert_eq!( - new_head.head_block_root(), - invalid_head.head_block_root(), - "the head should not change" - ); - } - - // Import the third block, it should become the head. + // Import the fork block, it should become the head. rig.harness .chain - .process_block(fork_blocks[2].clone(), CountUnrealized::True) + .process_block(fork_block.clone(), CountUnrealized::True) .await .unwrap(); rig.recompute_head().await; let new_head = rig.cached_head(); assert_eq!( new_head.head_block_root(), - fork_blocks[2].canonical_root(), - "the third block should become the head" + fork_block.canonical_root(), + "the fork block should become the head" ); let manual_get_head = rig @@ -1880,17 +1879,19 @@ async fn recover_from_invalid_head_by_importing_blocks() { .fork_choice_write_lock() .get_head(rig.harness.chain.slot().unwrap(), &rig.harness.chain.spec) .unwrap(); - assert_eq!(manual_get_head, new_head.head_block_root(),); + assert_eq!(manual_get_head, new_head.head_block_root()); } #[tokio::test] async fn recover_from_invalid_head_after_persist_and_reboot() { let InvalidHeadSetup { rig, - fork_blocks: _, + fork_block: _, invalid_head, } = InvalidHeadSetup::new().await; + let slot_clock = rig.harness.chain.slot_clock.clone(); + // Forcefully persist the head and fork choice. rig.harness.chain.persist_head_and_fork_choice().unwrap(); @@ -1899,6 +1900,7 @@ async fn recover_from_invalid_head_after_persist_and_reboot() { .deterministic_keypairs(VALIDATOR_COUNT) .resumed_ephemeral_store(rig.harness.chain.store.clone()) .mock_execution_layer() + .testing_slot_clock(slot_clock) .build(); // Forget the original rig so we don't accidentally use it again. diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 65872efbe9..9efb7ada12 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -341,7 +341,7 @@ impl Tester { let result = self.block_on_dangerous( self.harness .chain - .process_block(block.clone(), CountUnrealized::True), + .process_block(block.clone(), CountUnrealized::False), )?; if result.is_ok() != valid { return Err(Error::DidntFail(format!( From aba52251479100a5900f88ff9308139435a0a625 Mon Sep 17 00:00:00 2001 From: Kirill Date: Mon, 8 Aug 2022 23:56:59 +0000 Subject: [PATCH 11/21] `crypto/bls`: make `blst` dependency optional (#3387) ## Issue Addressed #3386 ## Proposed Changes * make `blst` crate `optional` * include `blst` dependency into `supranational` feature * hide `blst`-related code with `supranational` feature Co-authored-by: Kirill --- crypto/bls/Cargo.toml | 4 ++-- crypto/bls/src/impls/mod.rs | 1 + crypto/bls/src/lib.rs | 4 ++++ 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/crypto/bls/Cargo.toml b/crypto/bls/Cargo.toml index 912f49c6f0..9ac468d227 100644 --- a/crypto/bls/Cargo.toml +++ b/crypto/bls/Cargo.toml @@ -17,12 +17,12 @@ eth2_hashing = "0.3.0" ethereum-types = "0.12.1" arbitrary = { version = "1.0", features = ["derive"], optional = true } zeroize = { version = "1.4.2", features = ["zeroize_derive"] } -blst = "0.3.3" +blst = { version = "0.3.3", optional = true } [features] default = ["supranational"] fake_crypto = [] milagro = ["milagro_bls"] -supranational = [] +supranational = ["blst"] supranational-portable = ["supranational", "blst/portable"] supranational-force-adx = ["supranational", "blst/force-adx"] diff --git a/crypto/bls/src/impls/mod.rs b/crypto/bls/src/impls/mod.rs index 7a99798be3..b3f2da77b1 100644 --- a/crypto/bls/src/impls/mod.rs +++ b/crypto/bls/src/impls/mod.rs @@ -1,3 +1,4 @@ +#[cfg(feature = "supranational")] pub mod blst; pub mod fake_crypto; #[cfg(feature = "milagro")] diff --git a/crypto/bls/src/lib.rs b/crypto/bls/src/lib.rs index 8a31a90a14..eacbc2b268 100644 --- a/crypto/bls/src/lib.rs +++ b/crypto/bls/src/lib.rs @@ -41,6 +41,7 @@ pub use generic_signature::{INFINITY_SIGNATURE, SIGNATURE_BYTES_LEN}; pub use get_withdrawal_credentials::get_withdrawal_credentials; pub use zeroize_hash::ZeroizeHash; +#[cfg(feature = "supranational")] use blst::BLST_ERROR as BlstError; #[cfg(feature = "milagro")] use milagro_bls::AmclError; @@ -53,6 +54,7 @@ pub enum Error { #[cfg(feature = "milagro")] MilagroError(AmclError), /// An error was raised from the Supranational BLST BLS library. + #[cfg(feature = "supranational")] BlstError(BlstError), /// The provided bytes were an incorrect length. InvalidByteLength { got: usize, expected: usize }, @@ -71,6 +73,7 @@ impl From for Error { } } +#[cfg(feature = "supranational")] impl From for Error { fn from(e: BlstError) -> Error { Error::BlstError(e) @@ -130,6 +133,7 @@ macro_rules! define_mod { #[cfg(feature = "milagro")] define_mod!(milagro_implementations, crate::impls::milagro::types); +#[cfg(feature = "supranational")] define_mod!(blst_implementations, crate::impls::blst::types); #[cfg(feature = "fake_crypto")] define_mod!( From e26004461ff20eac1379edd5ce9020c0f5e8f8d6 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Mon, 8 Aug 2022 23:57:00 +0000 Subject: [PATCH 12/21] Don't attempt to register validators that are pre-activation (#3441) ## Issue Addressed https://github.com/sigp/lighthouse/issues/3440 ## Proposed Changes Don't consider pre-activation validators for validator registration. Co-authored-by: sean Co-authored-by: Michael Sproul --- validator_client/src/preparation_service.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/validator_client/src/preparation_service.rs b/validator_client/src/preparation_service.rs index b138d3e4ee..6dc8e7d56e 100644 --- a/validator_client/src/preparation_service.rs +++ b/validator_client/src/preparation_service.rs @@ -285,6 +285,9 @@ impl PreparationService { fn collect_validator_registration_keys(&self) -> Vec { self.collect_proposal_data(|pubkey, proposal_data| { + // Ignore fee recipients for keys without indices, they are inactive. + proposal_data.validator_index?; + // We don't log for missing fee recipients here because this will be logged more // frequently in `collect_preparation_data`. proposal_data.fee_recipient.and_then(|fee_recipient| { From 68bd7cae21b6890236f5d892f5847363fefd8307 Mon Sep 17 00:00:00 2001 From: kayla-henrie <109097759+kayla-henrie@users.noreply.github.com> Date: Tue, 9 Aug 2022 02:27:04 +0000 Subject: [PATCH 13/21] [Contribution docs] Add GitPOAP Badge to Display Number of Minted GitPOAPs for Contributors (#3343) ## Issue Addressed - N/A ## Proposed Changes Adding badge to contribution docs that shows the number of minted GitPOAPs ## Additional Info Hey all, this PR adds a [GitPOAP Badge](https://docs.gitpoap.io/api#get-v1repoownernamebadge) to the contribution docs that displays the number of minted GitPOAPs for this repository by contributors to this repo. You can see an example of this in [our Documentation repository](https://github.com/gitpoap/gitpoap-docs#gitpoap-docs). This should help would-be contributors as well as existing contributors find out that they will/have received GitPOAPs for their contributions. CC: @colfax23 @kayla-henrie Replaces: https://github.com/sigp/lighthouse/pull/3330 Co-authored-by: Michael Sproul --- CONTRIBUTING.md | 1 + book/src/contributing.md | 1 + 2 files changed, 2 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 72f5e73920..489d12eb88 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,4 +1,5 @@ # Contributors Guide +[![GitPOAP badge](https://public-api.gitpoap.io/v1/repo/sigp/lighthouse/badge)](https://www.gitpoap.io/gh/sigp/lighthouse) Lighthouse is an open-source Ethereum 2.0 client. We're community driven and welcome all contribution. We aim to provide a constructive, respectful and fun diff --git a/book/src/contributing.md b/book/src/contributing.md index 4b21d1ecf2..6b84843a69 100644 --- a/book/src/contributing.md +++ b/book/src/contributing.md @@ -1,6 +1,7 @@ # Contributing to Lighthouse [![Chat Badge]][Chat Link] +[![GitPOAP Badge](https://public-api.gitpoap.io/v1/repo/sigp/lighthouse/badge)](https://www.gitpoap.io/gh/sigp/lighthouse) [Chat Badge]: https://img.shields.io/badge/chat-discord-%237289da [Chat Link]: https://discord.gg/cyAszAh From a6886219191e2553177a9f45ad2a046faa1674d3 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 9 Aug 2022 06:05:13 +0000 Subject: [PATCH 14/21] Add support for beaconAPI in `lcli` functions (#3252) ## Issue Addressed NA ## Proposed Changes Modifies `lcli skip-slots` and `lcli transition-blocks` allow them to source blocks/states from a beaconAPI and also gives them some more features to assist with benchmarking. ## Additional Info Breaks the current `lcli skip-slots` and `lcli transition-blocks` APIs by changing some flag names. It should be simple enough to figure out the changes via `--help`. Currently blocked on #3263. --- Cargo.lock | 2 + beacon_node/beacon_chain/src/lib.rs | 2 +- .../src/validator_pubkey_cache.rs | 5 + common/eth2/src/lib.rs | 2 +- common/sensitive_url/src/lib.rs | 15 + .../src/per_block_processing.rs | 2 +- lcli/Cargo.toml | 2 + lcli/src/main.rs | 150 +++++-- lcli/src/skip_slots.rs | 160 +++++-- lcli/src/transition_blocks.rs | 410 ++++++++++++++---- 10 files changed, 607 insertions(+), 143 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1160609bec..3702b95485 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3105,6 +3105,7 @@ name = "lcli" version = "2.5.1" dependencies = [ "account_utils", + "beacon_chain", "bls", "clap", "clap_utils", @@ -3128,6 +3129,7 @@ dependencies = [ "serde_yaml", "snap", "state_processing", + "store", "tree_hash", "types", "validator_dir", diff --git a/beacon_node/beacon_chain/src/lib.rs b/beacon_node/beacon_chain/src/lib.rs index ed6c2459eb..481b1ae736 100644 --- a/beacon_node/beacon_chain/src/lib.rs +++ b/beacon_node/beacon_chain/src/lib.rs @@ -41,7 +41,7 @@ pub mod sync_committee_verification; pub mod test_utils; mod timeout_rw_lock; pub mod validator_monitor; -mod validator_pubkey_cache; +pub mod validator_pubkey_cache; pub use self::beacon_chain::{ AttestationProcessingOutcome, BeaconChain, BeaconChainTypes, BeaconStore, ChainSegmentResult, diff --git a/beacon_node/beacon_chain/src/validator_pubkey_cache.rs b/beacon_node/beacon_chain/src/validator_pubkey_cache.rs index beb8da8b64..60fdb607c8 100644 --- a/beacon_node/beacon_chain/src/validator_pubkey_cache.rs +++ b/beacon_node/beacon_chain/src/validator_pubkey_cache.rs @@ -156,6 +156,11 @@ impl ValidatorPubkeyCache { pub fn len(&self) -> usize { self.indices.len() } + + /// Returns `true` if there are no validators in the cache. + pub fn is_empty(&self) -> bool { + self.indices.is_empty() + } } /// Wrapper for a public key stored in the database. diff --git a/common/eth2/src/lib.rs b/common/eth2/src/lib.rs index 8cd138e980..21608ba6dd 100644 --- a/common/eth2/src/lib.rs +++ b/common/eth2/src/lib.rs @@ -23,7 +23,7 @@ use lighthouse_network::PeerId; pub use reqwest; use reqwest::{IntoUrl, RequestBuilder, Response}; pub use reqwest::{StatusCode, Url}; -use sensitive_url::SensitiveUrl; +pub use sensitive_url::SensitiveUrl; use serde::{de::DeserializeOwned, Serialize}; use std::convert::TryFrom; use std::fmt; diff --git a/common/sensitive_url/src/lib.rs b/common/sensitive_url/src/lib.rs index 7a3cbae20c..aac4cb5500 100644 --- a/common/sensitive_url/src/lib.rs +++ b/common/sensitive_url/src/lib.rs @@ -1,5 +1,6 @@ use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; use std::fmt; +use std::str::FromStr; use url::Url; #[derive(Debug)] @@ -9,6 +10,12 @@ pub enum SensitiveError { RedactError(String), } +impl fmt::Display for SensitiveError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{:?}", self) + } +} + // Wrapper around Url which provides a custom `Display` implementation to protect user secrets. #[derive(Clone, PartialEq)] pub struct SensitiveUrl { @@ -54,6 +61,14 @@ impl<'de> Deserialize<'de> for SensitiveUrl { } } +impl FromStr for SensitiveUrl { + type Err = SensitiveError; + + fn from_str(s: &str) -> Result { + Self::parse(s) + } +} + impl SensitiveUrl { pub fn parse(url: &str) -> Result { let surl = Url::parse(url).map_err(SensitiveError::ParseError)?; diff --git a/consensus/state_processing/src/per_block_processing.rs b/consensus/state_processing/src/per_block_processing.rs index 89cb76e0a1..e409372ddd 100644 --- a/consensus/state_processing/src/per_block_processing.rs +++ b/consensus/state_processing/src/per_block_processing.rs @@ -40,7 +40,7 @@ use arbitrary::Arbitrary; /// The strategy to be used when validating the block's signatures. #[cfg_attr(feature = "arbitrary-fuzz", derive(Arbitrary))] -#[derive(PartialEq, Clone, Copy)] +#[derive(PartialEq, Clone, Copy, Debug)] pub enum BlockSignatureStrategy { /// Do not validate any signature. Use with caution. NoVerification, diff --git a/lcli/Cargo.toml b/lcli/Cargo.toml index e54d9d8c95..5d94a50461 100644 --- a/lcli/Cargo.toml +++ b/lcli/Cargo.toml @@ -38,3 +38,5 @@ eth1_test_rig = { path = "../testing/eth1_test_rig" } sensitive_url = { path = "../common/sensitive_url" } eth2 = { path = "../common/eth2" } snap = "1.0.1" +beacon_chain = { path = "../beacon_node/beacon_chain" } +store = { path = "../beacon_node/store" } diff --git a/lcli/src/main.rs b/lcli/src/main.rs index c440f50008..2fd0538850 100644 --- a/lcli/src/main.rs +++ b/lcli/src/main.rs @@ -22,7 +22,6 @@ use parse_ssz::run_parse_ssz; use std::path::PathBuf; use std::process; use std::str::FromStr; -use transition_blocks::run_transition_blocks; use types::{EthSpec, EthSpecId}; fn main() { @@ -57,52 +56,128 @@ fn main() { "Performs a state transition from some state across some number of skip slots", ) .arg( - Arg::with_name("pre-state") - .value_name("BEACON_STATE") + Arg::with_name("output-path") + .long("output-path") + .value_name("PATH") .takes_value(true) - .required(true) + .help("Path to output a SSZ file."), + ) + .arg( + Arg::with_name("pre-state-path") + .long("pre-state-path") + .value_name("PATH") + .takes_value(true) + .conflicts_with("beacon-url") .help("Path to a SSZ file of the pre-state."), ) .arg( - Arg::with_name("slots") - .value_name("SLOT_COUNT") + Arg::with_name("beacon-url") + .long("beacon-url") + .value_name("URL") .takes_value(true) - .required(true) - .help("Number of slots to skip before outputting a state.."), + .help("URL to a beacon-API provider."), ) .arg( - Arg::with_name("output") - .value_name("SSZ_FILE") + Arg::with_name("state-id") + .long("state-id") + .value_name("STATE_ID") .takes_value(true) - .required(true) - .default_value("./output.ssz") - .help("Path to output a SSZ file."), - ), + .requires("beacon-url") + .help("Identifier for a state as per beacon-API standards (slot, root, etc.)"), + ) + .arg( + Arg::with_name("runs") + .long("runs") + .value_name("INTEGER") + .takes_value(true) + .default_value("1") + .help("Number of repeat runs, useful for benchmarking."), + ) + .arg( + Arg::with_name("state-root") + .long("state-root") + .value_name("HASH256") + .takes_value(true) + .help("Tree hash root of the provided state, to avoid computing it."), + ) + .arg( + Arg::with_name("slots") + .long("slots") + .value_name("INTEGER") + .takes_value(true) + .help("Number of slots to skip forward."), + ) + .arg( + Arg::with_name("partial-state-advance") + .long("partial-state-advance") + .takes_value(false) + .help("If present, don't compute state roots when skipping forward."), + ) ) .subcommand( SubCommand::with_name("transition-blocks") .about("Performs a state transition given a pre-state and block") .arg( - Arg::with_name("pre-state") - .value_name("BEACON_STATE") + Arg::with_name("pre-state-path") + .long("pre-state-path") + .value_name("PATH") .takes_value(true) - .required(true) - .help("Path to a SSZ file of the pre-state."), + .conflicts_with("beacon-url") + .requires("block-path") + .help("Path to load a BeaconState from file as SSZ."), ) .arg( - Arg::with_name("block") - .value_name("BEACON_BLOCK") + Arg::with_name("block-path") + .long("block-path") + .value_name("PATH") .takes_value(true) - .required(true) - .help("Path to a SSZ file of the block to apply to pre-state."), + .conflicts_with("beacon-url") + .requires("pre-state-path") + .help("Path to load a SignedBeaconBlock from file as SSZ."), ) .arg( - Arg::with_name("output") - .value_name("SSZ_FILE") + Arg::with_name("post-state-output-path") + .long("post-state-output-path") + .value_name("PATH") .takes_value(true) - .required(true) - .default_value("./output.ssz") - .help("Path to output a SSZ file."), + .help("Path to output the post-state."), + ) + .arg( + Arg::with_name("pre-state-output-path") + .long("pre-state-output-path") + .value_name("PATH") + .takes_value(true) + .help("Path to output the pre-state, useful when used with --beacon-url."), + ) + .arg( + Arg::with_name("block-output-path") + .long("block-output-path") + .value_name("PATH") + .takes_value(true) + .help("Path to output the block, useful when used with --beacon-url."), + ) + .arg( + Arg::with_name("beacon-url") + .long("beacon-url") + .value_name("URL") + .takes_value(true) + .help("URL to a beacon-API provider."), + ) + .arg( + Arg::with_name("block-id") + .long("block-id") + .value_name("BLOCK_ID") + .takes_value(true) + .requires("beacon-url") + .help("Identifier for a block as per beacon-API standards (slot, root, etc.)"), + ) + .arg( + Arg::with_name("runs") + .long("runs") + .value_name("INTEGER") + .takes_value(true) + .default_value("1") + .help("Number of repeat runs, useful for benchmarking."), ) .arg( Arg::with_name("no-signature-verification") @@ -110,6 +185,20 @@ fn main() { .takes_value(false) .help("Disable signature verification.") ) + .arg( + Arg::with_name("exclude-cache-builds") + .long("exclude-cache-builds") + .takes_value(false) + .help("If present, pre-build the committee and tree-hash caches without \ + including them in the timings."), + ) + .arg( + Arg::with_name("exclude-post-block-thc") + .long("exclude-post-block-thc") + .takes_value(false) + .help("If present, don't rebuild the tree-hash-cache after applying \ + the block."), + ) ) .subcommand( SubCommand::with_name("pretty-ssz") @@ -673,10 +762,11 @@ fn run( )?; match matches.subcommand() { - ("transition-blocks", Some(matches)) => run_transition_blocks::(testnet_dir, matches) + ("transition-blocks", Some(matches)) => transition_blocks::run::(env, matches) .map_err(|e| format!("Failed to transition blocks: {}", e)), - ("skip-slots", Some(matches)) => skip_slots::run::(testnet_dir, matches) - .map_err(|e| format!("Failed to skip slots: {}", e)), + ("skip-slots", Some(matches)) => { + skip_slots::run::(env, matches).map_err(|e| format!("Failed to skip slots: {}", e)) + } ("pretty-ssz", Some(matches)) => { run_parse_ssz::(matches).map_err(|e| format!("Failed to pretty print hex: {}", e)) } diff --git a/lcli/src/skip_slots.rs b/lcli/src/skip_slots.rs index cb502d37ae..28310f7683 100644 --- a/lcli/src/skip_slots.rs +++ b/lcli/src/skip_slots.rs @@ -1,58 +1,150 @@ +//! # Skip-Slots +//! +//! Use this tool to process a `BeaconState` through empty slots. Useful for benchmarking or +//! troubleshooting consensus failures. +//! +//! It can load states from file or pull them from a beaconAPI. States pulled from a beaconAPI can +//! be saved to disk to reduce future calls to that server. +//! +//! ## Examples +//! +//! ### Example 1. +//! +//! Download a state from a HTTP endpoint and skip forward an epoch, twice (the initial state is +//! advanced 32 slots twice, rather than it being advanced 64 slots): +//! +//! ```ignore +//! lcli skip-slots \ +//! --beacon-url http://localhost:5052 \ +//! --state-id 0x3cdc33cd02713d8d6cc33a6dbe2d3a5bf9af1d357de0d175a403496486ff845e \\ +//! --slots 32 \ +//! --runs 2 +//! ``` +//! +//! ### Example 2. +//! +//! Download a state to a SSZ file (without modifying it): +//! +//! ```ignore +//! lcli skip-slots \ +//! --beacon-url http://localhost:5052 \ +//! --state-id 0x3cdc33cd02713d8d6cc33a6dbe2d3a5bf9af1d357de0d175a403496486ff845e \ +//! --slots 0 \ +//! --runs 0 \ +//! --output-path /tmp/state-0x3cdc.ssz +//! ``` +//! +//! ### Example 3. +//! +//! Do two runs over the state that was downloaded in the previous example: +//! +//! ```ignore +//! lcli skip-slots \ +//! --pre-state-path /tmp/state-0x3cdc.ssz \ +//! --slots 32 \ +//! --runs 2 +//! ``` use crate::transition_blocks::load_from_ssz_with; use clap::ArgMatches; -use eth2_network_config::Eth2NetworkConfig; +use clap_utils::{parse_optional, parse_required}; +use environment::Environment; +use eth2::{types::StateId, BeaconNodeHttpClient, SensitiveUrl, Timeouts}; use ssz::Encode; -use state_processing::per_slot_processing; +use state_processing::state_advance::{complete_state_advance, partial_state_advance}; use std::fs::File; use std::io::prelude::*; use std::path::PathBuf; -use types::{BeaconState, EthSpec}; +use std::time::{Duration, Instant}; +use types::{BeaconState, CloneConfig, EthSpec, Hash256}; -pub fn run(testnet_dir: PathBuf, matches: &ArgMatches) -> Result<(), String> { - let pre_state_path = matches - .value_of("pre-state") - .ok_or("No pre-state file supplied")? - .parse::() - .map_err(|e| format!("Failed to parse pre-state path: {}", e))?; +const HTTP_TIMEOUT: Duration = Duration::from_secs(10); - let slots = matches - .value_of("slots") - .ok_or("No slots supplied")? - .parse::() - .map_err(|e| format!("Failed to parse slots: {}", e))?; +pub fn run(mut env: Environment, matches: &ArgMatches) -> Result<(), String> { + let spec = &T::default_spec(); + let executor = env.core_context().executor; - let output_path = matches - .value_of("output") - .ok_or("No output file supplied")? - .parse::() - .map_err(|e| format!("Failed to parse output path: {}", e))?; + let output_path: Option = parse_optional(matches, "output-path")?; + let state_path: Option = parse_optional(matches, "pre-state-path")?; + let beacon_url: Option = parse_optional(matches, "beacon-url")?; + let runs: usize = parse_required(matches, "runs")?; + let slots: u64 = parse_required(matches, "slots")?; + let cli_state_root: Option = parse_optional(matches, "state-root")?; + let partial: bool = matches.is_present("partial-state-advance"); info!("Using {} spec", T::spec_name()); - info!("Pre-state path: {:?}", pre_state_path); - info!("Slots: {:?}", slots); + info!("Advancing {} slots", slots); + info!("Doing {} runs", runs); - let eth2_network_config = Eth2NetworkConfig::load(testnet_dir)?; - let spec = ð2_network_config.chain_spec::()?; + let (mut state, state_root) = match (state_path, beacon_url) { + (Some(state_path), None) => { + info!("State path: {:?}", state_path); + let state = load_from_ssz_with(&state_path, spec, BeaconState::from_ssz_bytes)?; + (state, None) + } + (None, Some(beacon_url)) => { + let state_id: StateId = parse_required(matches, "state-id")?; + let client = BeaconNodeHttpClient::new(beacon_url, Timeouts::set_all(HTTP_TIMEOUT)); + let state = executor + .handle() + .ok_or("shutdown in progress")? + .block_on(async move { + client + .get_debug_beacon_states::(state_id) + .await + .map_err(|e| format!("Failed to download state: {:?}", e)) + }) + .map_err(|e| format!("Failed to complete task: {:?}", e))? + .ok_or_else(|| format!("Unable to locate state at {:?}", state_id))? + .data; + let state_root = match state_id { + StateId::Root(root) => Some(root), + _ => None, + }; + (state, state_root) + } + _ => return Err("must supply either --state-path or --beacon-url".into()), + }; - let mut state: BeaconState = - load_from_ssz_with(&pre_state_path, spec, BeaconState::from_ssz_bytes)?; + let initial_slot = state.slot(); + let target_slot = initial_slot + slots; state .build_all_caches(spec) .map_err(|e| format!("Unable to build caches: {:?}", e))?; - // Transition the parent state to the block slot. - for i in 0..slots { - per_slot_processing(&mut state, None, spec) - .map_err(|e| format!("Failed to advance slot on iteration {}: {:?}", i, e))?; + let state_root = if let Some(root) = cli_state_root.or(state_root) { + root + } else { + state + .update_tree_hash_cache() + .map_err(|e| format!("Unable to build THC: {:?}", e))? + }; + + for i in 0..runs { + let mut state = state.clone_with(CloneConfig::committee_caches_only()); + + let start = Instant::now(); + + if partial { + partial_state_advance(&mut state, Some(state_root), target_slot, spec) + .map_err(|e| format!("Unable to perform partial advance: {:?}", e))?; + } else { + complete_state_advance(&mut state, Some(state_root), target_slot, spec) + .map_err(|e| format!("Unable to perform complete advance: {:?}", e))?; + } + + let duration = Instant::now().duration_since(start); + info!("Run {}: {:?}", i, duration); } - let mut output_file = - File::create(output_path).map_err(|e| format!("Unable to create output file: {:?}", e))?; + if let Some(output_path) = output_path { + let mut output_file = File::create(output_path) + .map_err(|e| format!("Unable to create output file: {:?}", e))?; - output_file - .write_all(&state.as_ssz_bytes()) - .map_err(|e| format!("Unable to write to output file: {:?}", e))?; + output_file + .write_all(&state.as_ssz_bytes()) + .map_err(|e| format!("Unable to write to output file: {:?}", e))?; + } Ok(()) } diff --git a/lcli/src/transition_blocks.rs b/lcli/src/transition_blocks.rs index 74be1e6284..793bdb6422 100644 --- a/lcli/src/transition_blocks.rs +++ b/lcli/src/transition_blocks.rs @@ -1,125 +1,387 @@ +//! # Transition Blocks +//! +//! Use this tool to apply a `SignedBeaconBlock` to a `BeaconState`. Useful for benchmarking or +//! troubleshooting consensus failures. +//! +//! It can load states and blocks from file or pull them from a beaconAPI. Objects pulled from a +//! beaconAPI can be saved to disk to reduce future calls to that server. +//! +//! ## Examples +//! +//! ### Run using a block from a beaconAPI +//! +//! Download the 0x6c69 block and its pre-state (the state from its parent block) from the +//! beaconAPI. Advance the pre-state to the slot of the 0x6c69 block and apply that block to the +//! pre-state. +//! +//! ```ignore +//! lcli transition-blocks \ +//! --beacon-url http://localhost:5052 \ +//! --block-id 0x6c69cf50a451f1ec905e954bf1fa22970f371a72a5aa9f8e3a43a18fdd980bec \ +//! --runs 10 +//! ``` +//! +//! ### Download a block and pre-state from a beaconAPI to the filesystem +//! +//! Download a block and pre-state to the filesystem, without performing any transitions: +//! +//! ```ignore +//! lcli transition-blocks \ +//! --beacon-url http://localhost:5052 \ +//! --block-id 0x6c69cf50a451f1ec905e954bf1fa22970f371a72a5aa9f8e3a43a18fdd980bec \ +//! --runs 0 \ +//! --block-output-path /tmp/block-0x6c69.ssz \ +//! --pre-state-output-path /tmp/pre-state-0x6c69.ssz +//! ``` +//! +//! ### Use a block and pre-state from the filesystem +//! +//! Do one run over the block and pre-state downloaded in the previous example and save the post +//! state to file: +//! +//! ```ignore +//! lcli transition-blocks \ +//! --block-path /tmp/block-0x6c69.ssz \ +//! --pre-state-path /tmp/pre-state-0x6c69.ssz +//! --post-state-output-path /tmp/post-state-0x6c69.ssz +//! ``` +//! +//! ### Isolate block processing for benchmarking +//! +//! Try to isolate block processing as much as possible for benchmarking: +//! +//! ```ignore +//! lcli transition-blocks \ +//! --block-path /tmp/block-0x6c69.ssz \ +//! --pre-state-path /tmp/pre-state-0x6c69.ssz \ +//! --runs 10 \ +//! --exclude-cache-builds \ +//! --exclude-post-block-thc +//! ``` +use beacon_chain::{ + test_utils::EphemeralHarnessType, validator_pubkey_cache::ValidatorPubkeyCache, +}; use clap::ArgMatches; -use eth2_network_config::Eth2NetworkConfig; +use clap_utils::{parse_optional, parse_required}; +use environment::{null_logger, Environment}; +use eth2::{ + types::{BlockId, StateId}, + BeaconNodeHttpClient, SensitiveUrl, Timeouts, +}; use ssz::Encode; use state_processing::{ - per_block_processing, per_slot_processing, BlockSignatureStrategy, VerifyBlockRoot, + block_signature_verifier::BlockSignatureVerifier, per_block_processing, per_slot_processing, + BlockSignatureStrategy, VerifyBlockRoot, }; +use std::borrow::Cow; use std::fs::File; use std::io::prelude::*; use std::path::{Path, PathBuf}; -use std::time::Instant; -use types::{BeaconState, ChainSpec, EthSpec, SignedBeaconBlock}; +use std::sync::Arc; +use std::time::{Duration, Instant}; +use store::HotColdDB; +use types::{BeaconState, ChainSpec, CloneConfig, EthSpec, Hash256, SignedBeaconBlock}; -pub fn run_transition_blocks( - testnet_dir: PathBuf, - matches: &ArgMatches, -) -> Result<(), String> { - let pre_state_path = matches - .value_of("pre-state") - .ok_or("No pre-state file supplied")? - .parse::() - .map_err(|e| format!("Failed to parse pre-state path: {}", e))?; +const HTTP_TIMEOUT: Duration = Duration::from_secs(10); - let block_path = matches - .value_of("block") - .ok_or("No block file supplied")? - .parse::() - .map_err(|e| format!("Failed to parse block path: {}", e))?; +#[derive(Debug)] +struct Config { + no_signature_verification: bool, + exclude_cache_builds: bool, + exclude_post_block_thc: bool, +} - let output_path = matches - .value_of("output") - .ok_or("No output file supplied")? - .parse::() - .map_err(|e| format!("Failed to parse output path: {}", e))?; +pub fn run(mut env: Environment, matches: &ArgMatches) -> Result<(), String> { + let spec = &T::default_spec(); + let executor = env.core_context().executor; - let no_signature_verification = matches.is_present("no-signature-verification"); - let signature_strategy = if no_signature_verification { - BlockSignatureStrategy::NoVerification - } else { - BlockSignatureStrategy::VerifyIndividual + /* + * Parse (most) CLI arguments. + */ + + let pre_state_path: Option = parse_optional(matches, "pre-state-path")?; + let block_path: Option = parse_optional(matches, "block-path")?; + let post_state_output_path: Option = + parse_optional(matches, "post-state-output-path")?; + let pre_state_output_path: Option = parse_optional(matches, "pre-state-output-path")?; + let block_output_path: Option = parse_optional(matches, "block-output-path")?; + let beacon_url: Option = parse_optional(matches, "beacon-url")?; + let runs: usize = parse_required(matches, "runs")?; + let config = Config { + no_signature_verification: matches.is_present("no-signature-verification"), + exclude_cache_builds: matches.is_present("exclude-cache-builds"), + exclude_post_block_thc: matches.is_present("exclude-post-block-thc"), }; info!("Using {} spec", T::spec_name()); - info!("Pre-state path: {:?}", pre_state_path); - info!("Block path: {:?}", block_path); + info!("Doing {} runs", runs); + info!("{:?}", &config); - let eth2_network_config = Eth2NetworkConfig::load(testnet_dir)?; - let spec = ð2_network_config.chain_spec::()?; + /* + * Load the block and pre-state from disk or beaconAPI URL. + */ - let pre_state: BeaconState = - load_from_ssz_with(&pre_state_path, spec, BeaconState::from_ssz_bytes)?; - let block: SignedBeaconBlock = - load_from_ssz_with(&block_path, spec, SignedBeaconBlock::from_ssz_bytes)?; + let (mut pre_state, mut state_root_opt, block) = match (pre_state_path, block_path, beacon_url) + { + (Some(pre_state_path), Some(block_path), None) => { + info!("Block path: {:?}", pre_state_path); + info!("Pre-state path: {:?}", block_path); + let pre_state = load_from_ssz_with(&pre_state_path, spec, BeaconState::from_ssz_bytes)?; + let block = load_from_ssz_with(&block_path, spec, SignedBeaconBlock::from_ssz_bytes)?; + (pre_state, None, block) + } + (None, None, Some(beacon_url)) => { + let block_id: BlockId = parse_required(matches, "block-id")?; + let client = BeaconNodeHttpClient::new(beacon_url, Timeouts::set_all(HTTP_TIMEOUT)); + executor + .handle() + .ok_or("shutdown in progress")? + .block_on(async move { + let block = client + .get_beacon_blocks(block_id) + .await + .map_err(|e| format!("Failed to download block: {:?}", e))? + .ok_or_else(|| format!("Unable to locate block at {:?}", block_id))? + .data; - let t = Instant::now(); - let post_state = do_transition(pre_state, block, signature_strategy, spec)?; - println!("Total transition time: {}ms", t.elapsed().as_millis()); + if block.slot() == spec.genesis_slot { + return Err("Cannot run on the genesis block".to_string()); + } - let mut output_file = - File::create(output_path).map_err(|e| format!("Unable to create output file: {:?}", e))?; + let parent_block: SignedBeaconBlock = client + .get_beacon_blocks(BlockId::Root(block.parent_root())) + .await + .map_err(|e| format!("Failed to download parent block: {:?}", e))? + .ok_or_else(|| format!("Unable to locate parent block at {:?}", block_id))? + .data; - output_file - .write_all(&post_state.as_ssz_bytes()) - .map_err(|e| format!("Unable to write to output file: {:?}", e))?; + let state_root = parent_block.state_root(); + let state_id = StateId::Root(state_root); + let pre_state = client + .get_debug_beacon_states::(state_id) + .await + .map_err(|e| format!("Failed to download state: {:?}", e))? + .ok_or_else(|| format!("Unable to locate state at {:?}", state_id))? + .data; + + Ok((pre_state, Some(state_root), block)) + }) + .map_err(|e| format!("Failed to complete task: {:?}", e))? + } + _ => { + return Err( + "must supply *both* --pre-state-path and --block-path *or* only --beacon-url" + .into(), + ) + } + }; + + // Compute the block root. + let block_root = block.canonical_root(); + + /* + * Create a `BeaconStore` and `ValidatorPubkeyCache` for block signature verification. + */ + + let store = HotColdDB::open_ephemeral( + <_>::default(), + spec.clone(), + null_logger().map_err(|e| format!("Failed to create null_logger: {:?}", e))?, + ) + .map_err(|e| format!("Failed to create ephemeral store: {:?}", e))?; + let store = Arc::new(store); + + debug!("Building pubkey cache (might take some time)"); + let validator_pubkey_cache = ValidatorPubkeyCache::new(&pre_state, store) + .map_err(|e| format!("Failed to create pubkey cache: {:?}", e))?; + + /* + * If cache builds are excluded from the timings, build them early so they are available for + * each run. + */ + + if config.exclude_cache_builds { + pre_state + .build_all_caches(spec) + .map_err(|e| format!("Unable to build caches: {:?}", e))?; + let state_root = pre_state + .update_tree_hash_cache() + .map_err(|e| format!("Unable to build THC: {:?}", e))?; + + if state_root_opt.map_or(false, |expected| expected != state_root) { + return Err(format!( + "State root mismatch! Expected {}, computed {}", + state_root_opt.unwrap(), + state_root + )); + } + state_root_opt = Some(state_root); + } + + /* + * Perform the core "runs". + */ + + let mut output_post_state = None; + for i in 0..runs { + let pre_state = pre_state.clone_with(CloneConfig::all()); + let block = block.clone(); + + let start = Instant::now(); + + let post_state = do_transition( + pre_state, + block_root, + block, + state_root_opt, + &config, + &validator_pubkey_cache, + spec, + )?; + + let duration = Instant::now().duration_since(start); + info!("Run {}: {:?}", i, duration); + + if output_post_state.is_none() { + output_post_state = Some(post_state) + } + } + + /* + * Write artifacts to disk, if required. + */ + + if let Some(path) = post_state_output_path { + let output_post_state = output_post_state.ok_or_else(|| { + format!( + "Post state was not computed, cannot save to disk (runs = {})", + runs + ) + })?; + + let mut output_file = + File::create(path).map_err(|e| format!("Unable to create output file: {:?}", e))?; + + output_file + .write_all(&output_post_state.as_ssz_bytes()) + .map_err(|e| format!("Unable to write to output file: {:?}", e))?; + } + + if let Some(path) = pre_state_output_path { + let mut output_file = + File::create(path).map_err(|e| format!("Unable to create output file: {:?}", e))?; + + output_file + .write_all(&pre_state.as_ssz_bytes()) + .map_err(|e| format!("Unable to write to output file: {:?}", e))?; + } + + if let Some(path) = block_output_path { + let mut output_file = + File::create(path).map_err(|e| format!("Unable to create output file: {:?}", e))?; + + output_file + .write_all(&block.as_ssz_bytes()) + .map_err(|e| format!("Unable to write to output file: {:?}", e))?; + } Ok(()) } fn do_transition( mut pre_state: BeaconState, + block_root: Hash256, block: SignedBeaconBlock, - signature_strategy: BlockSignatureStrategy, + mut state_root_opt: Option, + config: &Config, + validator_pubkey_cache: &ValidatorPubkeyCache>, spec: &ChainSpec, ) -> Result, String> { - let t = Instant::now(); - pre_state - .build_all_caches(spec) - .map_err(|e| format!("Unable to build caches: {:?}", e))?; - println!("Build caches: {}ms", t.elapsed().as_millis()); + if !config.exclude_cache_builds { + let t = Instant::now(); + pre_state + .build_all_caches(spec) + .map_err(|e| format!("Unable to build caches: {:?}", e))?; + debug!("Build caches: {:?}", t.elapsed()); - let t = Instant::now(); - pre_state - .update_tree_hash_cache() - .map_err(|e| format!("Unable to build tree hash cache: {:?}", e))?; - println!("Initial tree hash: {}ms", t.elapsed().as_millis()); + let t = Instant::now(); + let state_root = pre_state + .update_tree_hash_cache() + .map_err(|e| format!("Unable to build tree hash cache: {:?}", e))?; + debug!("Initial tree hash: {:?}", t.elapsed()); + + if state_root_opt.map_or(false, |expected| expected != state_root) { + return Err(format!( + "State root mismatch! Expected {}, computed {}", + state_root_opt.unwrap(), + state_root + )); + } + state_root_opt = Some(state_root); + } + + let state_root = state_root_opt.ok_or("Failed to compute state root, internal error")?; // Transition the parent state to the block slot. let t = Instant::now(); for i in pre_state.slot().as_u64()..block.slot().as_u64() { - per_slot_processing(&mut pre_state, None, spec) + per_slot_processing(&mut pre_state, Some(state_root), spec) .map_err(|e| format!("Failed to advance slot on iteration {}: {:?}", i, e))?; } - println!("Slot processing: {}ms", t.elapsed().as_millis()); - - let t = Instant::now(); - pre_state - .update_tree_hash_cache() - .map_err(|e| format!("Unable to build tree hash cache: {:?}", e))?; - println!("Pre-block tree hash: {}ms", t.elapsed().as_millis()); + debug!("Slot processing: {:?}", t.elapsed()); let t = Instant::now(); pre_state .build_all_caches(spec) .map_err(|e| format!("Unable to build caches: {:?}", e))?; - println!("Build all caches (again): {}ms", t.elapsed().as_millis()); + debug!("Build all caches (again): {:?}", t.elapsed()); + + if !config.no_signature_verification { + let get_pubkey = move |validator_index| { + validator_pubkey_cache + .get(validator_index) + .map(Cow::Borrowed) + }; + + let decompressor = move |pk_bytes| { + // Map compressed pubkey to validator index. + let validator_index = validator_pubkey_cache.get_index(pk_bytes)?; + // Map validator index to pubkey (respecting guard on unknown validators). + get_pubkey(validator_index) + }; + + let t = Instant::now(); + BlockSignatureVerifier::verify_entire_block( + &pre_state, + get_pubkey, + decompressor, + &block, + Some(block_root), + spec, + ) + .map_err(|e| format!("Invalid block signature: {:?}", e))?; + debug!("Batch verify block signatures: {:?}", t.elapsed()); + } let t = Instant::now(); per_block_processing( &mut pre_state, &block, None, - signature_strategy, + BlockSignatureStrategy::NoVerification, VerifyBlockRoot::True, spec, ) .map_err(|e| format!("State transition failed: {:?}", e))?; - println!("Process block: {}ms", t.elapsed().as_millis()); + debug!("Process block: {:?}", t.elapsed()); - let t = Instant::now(); - pre_state - .update_tree_hash_cache() - .map_err(|e| format!("Unable to build tree hash cache: {:?}", e))?; - println!("Post-block tree hash: {}ms", t.elapsed().as_millis()); + if !config.exclude_post_block_thc { + let t = Instant::now(); + pre_state + .update_tree_hash_cache() + .map_err(|e| format!("Unable to build tree hash cache: {:?}", e))?; + debug!("Post-block tree hash: {:?}", t.elapsed()); + } Ok(pre_state) } @@ -136,10 +398,6 @@ pub fn load_from_ssz_with( .map_err(|e| format!("Unable to read from file {:?}: {:?}", path, e))?; let t = Instant::now(); let result = decoder(&bytes, spec).map_err(|e| format!("Ssz decode failed: {:?}", e)); - println!( - "SSZ decoding {}: {}ms", - path.display(), - t.elapsed().as_millis() - ); + debug!("SSZ decoding {}: {:?}", path.display(), t.elapsed()); result } From 5bb4aada92882ff25e2c33c31661cde91b651f13 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 9 Aug 2022 06:05:15 +0000 Subject: [PATCH 15/21] Update Prater ENRs (#3396) ## Issue Addressed NA ## Proposed Changes Update bootnodes for Prater. There are new IP addresses for the Sigma Prime nodes. Teku and Nimbus nodes were also added. ## Additional Info Related: https://github.com/eth-clients/goerli/commit/24760cd4b46c4f2274cf333375bfbf6133f44401 --- .../built_in_network_configs/prater/boot_enr.yaml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/common/eth2_network_config/built_in_network_configs/prater/boot_enr.yaml b/common/eth2_network_config/built_in_network_configs/prater/boot_enr.yaml index fcb2d5342b..7000ff0bbc 100644 --- a/common/eth2_network_config/built_in_network_configs/prater/boot_enr.yaml +++ b/common/eth2_network_config/built_in_network_configs/prater/boot_enr.yaml @@ -7,4 +7,11 @@ # Prysm bootnode #1 - enr:-Ku4QFmUkNp0g9bsLX2PfVeIyT-9WO-PZlrqZBNtEyofOOfLMScDjaTzGxIb1Ns9Wo5Pm_8nlq-SZwcQfTH2cgO-s88Bh2F0dG5ldHOIAAAAAAAAAACEZXRoMpDkvpOTAAAQIP__________gmlkgnY0gmlwhBLf22SJc2VjcDI1NmsxoQLV_jMOIxKbjHFKgrkFvwDvpexo6Nd58TK5k7ss4Vt0IoN1ZHCCG1g # Lighthouse bootnode #1 -- enr:-LK4QLINdtobGquK7jukLDAKmsrH2ZuHM4k0TklY5jDTD4ZgfxR9weZmo5Jwu81hlKu3qPAvk24xHGBDjYs4o8f1gZ0Bh2F0dG5ldHOIAAAAAAAAAACEZXRoMpB53wQoAAAQIP__________gmlkgnY0gmlwhDRN_P6Jc2VjcDI1NmsxoQJuNujTgsJUHUgVZML3pzrtgNtYg7rQ4K1tkWERgl0DdoN0Y3CCIyiDdWRwgiMo +- enr:-Ly4QFPk-cTMxZ3jWTafiNblEZkQIXGF2aVzCIGW0uHp6KaEAvBMoctE8S7YU0qZtuS7By0AA4YMfKoN9ls_GJRccVpFh2F0dG5ldHOI__________-EZXRoMpCC9KcrAgAQIIS2AQAAAAAAgmlkgnY0gmlwhKh3joWJc2VjcDI1NmsxoQKrxz8M1IHwJqRIpDqdVW_U1PeixMW5SfnBD-8idYIQrIhzeW5jbmV0cw-DdGNwgiMog3VkcIIjKA +# Lighthouse bootnode #2 +- enr:-L64QJmwSDtaHVgGiqIxJWUtxWg6uLCipsms6j-8BdsOJfTWAs7CLF9HJnVqFE728O-JYUDCxzKvRdeMqBSauHVCMdaCAVWHYXR0bmV0c4j__________4RldGgykIL0pysCABAghLYBAAAAAACCaWSCdjSCaXCEQWxOdolzZWNwMjU2azGhA7Qmod9fK86WidPOzLsn5_8QyzL7ZcJ1Reca7RnD54vuiHN5bmNuZXRzD4N0Y3CCIyiDdWRwgiMo +# Nimbus bootstrap nodes +- enr:-LK4QMzPq4Q7w5R-rnGQDcI8BYky6oPVBGQTbS1JJLVtNi_8PzBLV7Bdzsoame9nJK5bcJYpGHn4SkaDN2CM6tR5G_4Bh2F0dG5ldHOIAAAAAAAAAACEZXRoMpB53wQoAAAQIP__________gmlkgnY0gmlwhAN4yvyJc2VjcDI1NmsxoQKa8Qnp_P2clLIP6VqLKOp_INvEjLszalEnW0LoBZo4YYN0Y3CCI4yDdWRwgiOM +- enr:-LK4QLM_pPHa78R8xlcU_s40Y3XhFjlb3kPddW9lRlY67N5qeFE2Wo7RgzDgRs2KLCXODnacVHMFw1SfpsW3R474RZEBh2F0dG5ldHOIAAAAAAAAAACEZXRoMpB53wQoAAAQIP__________gmlkgnY0gmlwhANBY-yJc2VjcDI1NmsxoQNsZkFXgKbTzuxF7uwxlGauTGJelE6HD269CcFlZ_R7A4N0Y3CCI4yDdWRwgiOM +# Teku bootnode +- enr:-KK4QH0RsNJmIG0EX9LSnVxMvg-CAOr3ZFF92hunU63uE7wcYBjG1cFbUTvEa5G_4nDJkRhUq9q2ck9xY-VX1RtBsruBtIRldGgykIL0pysBABAg__________-CaWSCdjSCaXCEEnXQ0YlzZWNwMjU2azGhA1grTzOdMgBvjNrk-vqWtTZsYQIi0QawrhoZrsn5Hd56g3RjcIIjKIN1ZHCCIyg From 6f13727fbef75ff2501eb3248315842eaa83247b Mon Sep 17 00:00:00 2001 From: realbigsean Date: Tue, 9 Aug 2022 06:05:16 +0000 Subject: [PATCH 16/21] Don't use the builder network if the head is optimistic (#3412) ## Issue Addressed Resolves https://github.com/sigp/lighthouse/issues/3394 Adds a check in `is_healthy` about whether the head is optimistic when choosing whether to use the builder network. Co-authored-by: realbigsean --- beacon_node/beacon_chain/src/beacon_chain.rs | 15 +++++- beacon_node/execution_layer/src/lib.rs | 4 ++ beacon_node/http_api/tests/tests.rs | 57 ++++++++++++++++++++ 3 files changed, 74 insertions(+), 2 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index fec7fe25ff..d6503d687c 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -3333,7 +3333,7 @@ impl BeaconChain { pubkey, slot: state.slot(), chain_health: self - .is_healthy() + .is_healthy(&parent_root) .map_err(BlockProductionError::BeaconChain)?, }; @@ -4562,7 +4562,7 @@ impl BeaconChain { /// /// Since we are likely calling this during the slot we are going to propose in, don't take into /// account the current slot when accounting for skips. - pub fn is_healthy(&self) -> Result { + pub fn is_healthy(&self, parent_root: &Hash256) -> Result { // Check if the merge has been finalized. if let Some(finalized_hash) = self .canonical_head @@ -4577,6 +4577,17 @@ impl BeaconChain { return Ok(ChainHealth::PreMerge); }; + // Check that the parent is NOT optimistic. + if let Some(execution_status) = self + .canonical_head + .fork_choice_read_lock() + .get_block_execution_status(parent_root) + { + if execution_status.is_strictly_optimistic() { + return Ok(ChainHealth::Optimistic); + } + } + if self.config.builder_fallback_disable_checks { return Ok(ChainHealth::Healthy); } diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index 59c8f009fa..f56ea8f797 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -114,6 +114,7 @@ pub struct BuilderParams { pub enum ChainHealth { Healthy, Unhealthy(FailedCondition), + Optimistic, PreMerge, } @@ -695,6 +696,9 @@ impl ExecutionLayer { } // Intentional no-op, so we never attempt builder API proposals pre-merge. ChainHealth::PreMerge => (), + ChainHealth::Optimistic => info!(self.log(), "The local execution engine is syncing \ + so the builder network cannot safely be used. Attempting \ + to build a block with the local execution engine"), } } self.get_full_payload_caching( diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index cc0281e454..fa41102292 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -3044,6 +3044,55 @@ impl ApiTester { self } + pub async fn test_builder_chain_health_optimistic_head(self) -> Self { + // Make sure the next payload verification will return optimistic before advancing the chain. + self.harness.mock_execution_layer.as_ref().map(|el| { + el.server.all_payloads_syncing(true); + el + }); + self.harness + .extend_chain( + 1, + BlockStrategy::OnCanonicalHead, + AttestationStrategy::AllValidators, + ) + .await; + self.harness.advance_slot(); + + let slot = self.chain.slot().unwrap(); + let epoch = self.chain.epoch().unwrap(); + + let (proposer_index, randao_reveal) = self.get_test_randao(slot, epoch).await; + + let payload = self + .client + .get_validator_blinded_blocks::>(slot, &randao_reveal, None) + .await + .unwrap() + .data + .body() + .execution_payload() + .unwrap() + .clone(); + + let expected_fee_recipient = Address::from_low_u64_be(proposer_index as u64); + assert_eq!( + payload.execution_payload_header.fee_recipient, + expected_fee_recipient + ); + + // If this cache is populated, it indicates fallback to the local EE was correctly used. + assert!(self + .chain + .execution_layer + .as_ref() + .unwrap() + .get_payload_by_root(&payload.tree_hash_root()) + .is_some()); + + self + } + #[cfg(target_os = "linux")] pub async fn test_get_lighthouse_health(self) -> Self { self.client.get_lighthouse_health().await.unwrap(); @@ -4000,6 +4049,14 @@ async fn builder_chain_health_epochs_since_finalization() { .await; } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn builder_chain_health_optimistic_head() { + ApiTester::new_mev_tester() + .await + .test_builder_chain_health_optimistic_head() + .await; +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn lighthouse_endpoints() { ApiTester::new() From 052d5cf31f3028157cbf7812a092ccc409ba927a Mon Sep 17 00:00:00 2001 From: Brendan Timmons Date: Tue, 9 Aug 2022 06:05:17 +0000 Subject: [PATCH 17/21] fix: incorrectly formatted MEV link in Lighthouse Book (#3434) ## Issue Addressed N/A ## Proposed Changes Simply fix the incorrect formatting on markdown link. Co-authored-by: Michael Sproul --- book/src/builders.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/book/src/builders.md b/book/src/builders.md index 78a80899cc..1a034e0820 100644 --- a/book/src/builders.md +++ b/book/src/builders.md @@ -6,7 +6,7 @@ knowledge of the transactions included in the block. This enables Lighthouse to transaction gathering/ordering within a block to parties specialized in this particular task. For economic reasons, these parties will refuse to reveal the list of transactions to the validator before the validator has committed to (i.e. signed) the block. A primer on MEV can be found -[here]([MEV](https://ethereum.org/en/developers/docs/mev/)). +[here](https://ethereum.org/en/developers/docs/mev). Using the builder API is not known to introduce additional slashing risks, however a live-ness risk (i.e. the ability for the chain to produce valid blocks) is introduced because your node will be From 2de26b20f80579db6c794d5f25f6326783973372 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 10 Aug 2022 07:52:57 +0000 Subject: [PATCH 18/21] Don't return errors on HTTP API for already-known messages (#3341) ## Issue Addressed - Resolves #3266 ## Proposed Changes Return 200 OK rather than an error when a block, attestation or sync message is already known. Presently, we will log return an error which causes a BN to go "offline" from the VCs perspective which causes the fallback mechanism to do work to try and avoid and upcheck offline nodes. This can be observed as instability in the `vc_beacon_nodes_available_count` metric. The current behaviour also causes scary logs for the user. There's nothing to *actually* be concerned about when we see duplicate messages, this can happen on fallback systems (see code comments). ## Additional Info NA --- beacon_node/http_api/src/lib.rs | 53 +++++++++++++++++++++ beacon_node/http_api/src/publish_blocks.rs | 25 +++++++++- beacon_node/http_api/src/sync_committees.rs | 30 +++++++++++- 3 files changed, 105 insertions(+), 3 deletions(-) diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index ba1dd01cc3..ec34dd0663 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -1168,12 +1168,46 @@ pub fn serve( blocking_json_task(move || { let seen_timestamp = timestamp_now(); let mut failures = Vec::new(); + let mut num_already_known = 0; for (index, attestation) in attestations.as_slice().iter().enumerate() { let attestation = match chain .verify_unaggregated_attestation_for_gossip(attestation, None) { Ok(attestation) => attestation, + Err(AttnError::PriorAttestationKnown { .. }) => { + num_already_known += 1; + + // Skip to the next attestation since an attestation for this + // validator is already known in this epoch. + // + // There's little value for the network in validating a second + // attestation for another validator since it is either: + // + // 1. A duplicate. + // 2. Slashable. + // 3. Invalid. + // + // We are likely to get duplicates in the case where a VC is using + // fallback BNs. If the first BN actually publishes some/all of a + // batch of attestations but fails to respond in a timely fashion, + // the VC is likely to try publishing the attestations on another + // BN. That second BN may have already seen the attestations from + // the first BN and therefore indicate that the attestations are + // "already seen". An attestation that has already been seen has + // been published on the network so there's no actual error from + // the perspective of the user. + // + // It's better to prevent slashable attestations from ever + // appearing on the network than trying to slash validators, + // especially those validators connected to the local API. + // + // There might be *some* value in determining that this attestation + // is invalid, but since a valid attestation already it exists it + // appears that this validator is capable of producing valid + // attestations and there's no immediate cause for concern. + continue; + } Err(e) => { error!(log, "Failure verifying attestation for gossip"; @@ -1240,6 +1274,15 @@ pub fn serve( )); } } + + if num_already_known > 0 { + debug!( + log, + "Some unagg attestations already known"; + "count" => num_already_known + ); + } + if failures.is_empty() { Ok(()) } else { @@ -2234,6 +2277,16 @@ pub fn serve( // identical aggregates, especially if they're using the same beacon // node. Err(AttnError::AttestationAlreadyKnown(_)) => continue, + // If we've already seen this aggregator produce an aggregate, just + // skip this one. + // + // We're likely to see this with VCs that use fallback BNs. The first + // BN might time-out *after* publishing the aggregate and then the + // second BN will indicate it's already seen the aggregate. + // + // There's no actual error for the user or the network since the + // aggregate has been successfully published by some other node. + Err(AttnError::AggregatorAlreadyKnown(_)) => continue, Err(e) => { error!(log, "Failure verifying aggregate and proofs"; diff --git a/beacon_node/http_api/src/publish_blocks.rs b/beacon_node/http_api/src/publish_blocks.rs index b282e6f490..60ca8f2328 100644 --- a/beacon_node/http_api/src/publish_blocks.rs +++ b/beacon_node/http_api/src/publish_blocks.rs @@ -1,9 +1,9 @@ use crate::metrics; use beacon_chain::validator_monitor::{get_block_delay_ms, timestamp_now}; -use beacon_chain::{BeaconChain, BeaconChainTypes, CountUnrealized}; +use beacon_chain::{BeaconChain, BeaconChainTypes, BlockError, CountUnrealized}; use lighthouse_network::PubsubMessage; use network::NetworkMessage; -use slog::{crit, error, info, Logger}; +use slog::{crit, error, info, warn, Logger}; use slot_clock::SlotClock; use std::sync::Arc; use tokio::sync::mpsc::UnboundedSender; @@ -86,6 +86,27 @@ pub async fn publish_block( Ok(()) } + Err(BlockError::BlockIsAlreadyKnown) => { + info!( + log, + "Block from HTTP API already known"; + "block" => ?block.canonical_root(), + "slot" => block.slot(), + ); + Ok(()) + } + Err(BlockError::RepeatProposal { proposer, slot }) => { + warn!( + log, + "Block ignored due to repeat proposal"; + "msg" => "this can happen when a VC uses fallback BNs. \ + whilst this is not necessarily an error, it can indicate issues with a BN \ + or between the VC and BN.", + "slot" => slot, + "proposer" => proposer, + ); + Ok(()) + } Err(e) => { let msg = format!("{:?}", e); error!( diff --git a/beacon_node/http_api/src/sync_committees.rs b/beacon_node/http_api/src/sync_committees.rs index 77becef7df..a6acf308fa 100644 --- a/beacon_node/http_api/src/sync_committees.rs +++ b/beacon_node/http_api/src/sync_committees.rs @@ -11,7 +11,7 @@ use beacon_chain::{ use eth2::types::{self as api_types}; use lighthouse_network::PubsubMessage; use network::NetworkMessage; -use slog::{error, warn, Logger}; +use slog::{debug, error, warn, Logger}; use slot_clock::SlotClock; use std::cmp::max; use std::collections::HashMap; @@ -189,6 +189,24 @@ pub fn process_sync_committee_signatures( verified_for_pool = Some(verified); } + // If this validator has already published a sync message, just ignore this message + // without returning an error. + // + // This is likely to happen when a VC uses fallback BNs. If the first BN publishes + // the message and then fails to respond in a timely fashion then the VC will move + // to the second BN. The BN will then report that this message has already been + // seen, which is not actually an error as far as the network or user are concerned. + Err(SyncVerificationError::PriorSyncCommitteeMessageKnown { + validator_index, + slot, + }) => { + debug!( + log, + "Ignoring already-known sync message"; + "slot" => slot, + "validator_index" => validator_index, + ); + } Err(e) => { error!( log, @@ -283,6 +301,16 @@ pub fn process_signed_contribution_and_proofs( // If we already know the contribution, don't broadcast it or attempt to // further verify it. Return success. Err(SyncVerificationError::SyncContributionAlreadyKnown(_)) => continue, + // If we've already seen this aggregator produce an aggregate, just + // skip this one. + // + // We're likely to see this with VCs that use fallback BNs. The first + // BN might time-out *after* publishing the aggregate and then the + // second BN will indicate it's already seen the aggregate. + // + // There's no actual error for the user or the network since the + // aggregate has been successfully published by some other node. + Err(SyncVerificationError::AggregatorAlreadyKnown(_)) => continue, Err(e) => { error!( log, From c25934956b30dc7beb915b860f02eb7f68ad3a24 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Wed, 10 Aug 2022 07:52:58 +0000 Subject: [PATCH 19/21] Remove INVALID_TERMINAL_BLOCK (#3385) ## Issue Addressed Resolves #3379 ## Proposed Changes Remove instances of `InvalidTerminalBlock` in lighthouse and use `Invalid {latest_valid_hash: "0x0000000000000000000000000000000000000000000000000000000000000000"}` to represent that status. --- beacon_node/beacon_chain/src/beacon_chain.rs | 35 ++++++++----- .../beacon_chain/src/execution_payload.rs | 7 ++- .../tests/payload_invalidation.rs | 52 ++++++++++--------- beacon_node/execution_layer/src/engine_api.rs | 1 - .../src/engine_api/json_structures.rs | 7 --- .../execution_layer/src/payload_status.rs | 19 ------- .../execution_layer/src/test_utils/mod.rs | 4 +- 7 files changed, 58 insertions(+), 67 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index d6503d687c..54c961e34d 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4084,21 +4084,32 @@ impl BeaconChain { "Fork choice update invalidated payload"; "status" => ?status ); - // The execution engine has stated that all blocks between the - // `head_execution_block_hash` and `latest_valid_hash` are invalid. - self.process_invalid_execution_payload( - &InvalidationOperation::InvalidateMany { - head_block_root, - always_invalidate_head: true, - latest_valid_ancestor: latest_valid_hash, - }, - ) - .await?; + + // This implies that the terminal block was invalid. We are being explicit in + // invalidating only the head block in this case. + if latest_valid_hash == ExecutionBlockHash::zero() { + self.process_invalid_execution_payload( + &InvalidationOperation::InvalidateOne { + block_root: head_block_root, + }, + ) + .await?; + } else { + // The execution engine has stated that all blocks between the + // `head_execution_block_hash` and `latest_valid_hash` are invalid. + self.process_invalid_execution_payload( + &InvalidationOperation::InvalidateMany { + head_block_root, + always_invalidate_head: true, + latest_valid_ancestor: latest_valid_hash, + }, + ) + .await?; + } Err(BeaconChainError::ExecutionForkChoiceUpdateInvalid { status }) } - PayloadStatus::InvalidTerminalBlock { .. } - | PayloadStatus::InvalidBlockHash { .. } => { + PayloadStatus::InvalidBlockHash { .. } => { warn!( self.log, "Fork choice update invalidated payload"; diff --git a/beacon_node/beacon_chain/src/execution_payload.rs b/beacon_node/beacon_chain/src/execution_payload.rs index 3c530aaac8..7af171b794 100644 --- a/beacon_node/beacon_chain/src/execution_payload.rs +++ b/beacon_node/beacon_chain/src/execution_payload.rs @@ -114,6 +114,11 @@ async fn notify_new_payload<'a, T: BeaconChainTypes>( PayloadStatus::Invalid { latest_valid_hash, .. } => { + // latest_valid_hash == 0 implies that this was the terminal block + // Hence, we don't need to run `BeaconChain::process_invalid_execution_payload`. + if latest_valid_hash == ExecutionBlockHash::zero() { + return Err(ExecutionPayloadError::RejectedByExecutionEngine { status }.into()); + } // This block has not yet been applied to fork choice, so the latest block that was // imported to fork choice was the parent. let latest_root = block.parent_root(); @@ -127,7 +132,7 @@ async fn notify_new_payload<'a, T: BeaconChainTypes>( Err(ExecutionPayloadError::RejectedByExecutionEngine { status }.into()) } - PayloadStatus::InvalidTerminalBlock { .. } | PayloadStatus::InvalidBlockHash { .. } => { + PayloadStatus::InvalidBlockHash { .. } => { // Returning an error here should be sufficient to invalidate the block. We have no // information to indicate its parent is invalid, so no need to run // `BeaconChain::process_invalid_execution_payload`. diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index 7728b319d9..027a708cfa 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -40,7 +40,6 @@ enum Payload { }, Syncing, InvalidBlockHash, - InvalidTerminalBlock, } struct InvalidPayloadRig { @@ -231,16 +230,20 @@ impl InvalidPayloadRig { Payload::Invalid { latest_valid_hash } => { let latest_valid_hash = latest_valid_hash .unwrap_or_else(|| self.block_hash(block.message().parent_root())); - mock_execution_layer - .server - .all_payloads_invalid_on_new_payload(latest_valid_hash) + if latest_valid_hash == ExecutionBlockHash::zero() { + mock_execution_layer + .server + .all_payloads_invalid_terminal_block_on_new_payload() + } else { + mock_execution_layer + .server + .all_payloads_invalid_on_new_payload(latest_valid_hash) + } } + Payload::InvalidBlockHash => mock_execution_layer .server .all_payloads_invalid_block_hash_on_new_payload(), - Payload::InvalidTerminalBlock => mock_execution_layer - .server - .all_payloads_invalid_terminal_block_on_new_payload(), }; let set_forkchoice_updated = |payload: Payload| match payload { Payload::Valid => mock_execution_layer @@ -252,16 +255,20 @@ impl InvalidPayloadRig { Payload::Invalid { latest_valid_hash } => { let latest_valid_hash = latest_valid_hash .unwrap_or_else(|| self.block_hash(block.message().parent_root())); - mock_execution_layer - .server - .all_payloads_invalid_on_forkchoice_updated(latest_valid_hash) + if latest_valid_hash == ExecutionBlockHash::zero() { + mock_execution_layer + .server + .all_payloads_invalid_terminal_block_on_forkchoice_updated() + } else { + mock_execution_layer + .server + .all_payloads_invalid_on_forkchoice_updated(latest_valid_hash) + } } + Payload::InvalidBlockHash => mock_execution_layer .server .all_payloads_invalid_block_hash_on_forkchoice_updated(), - Payload::InvalidTerminalBlock => mock_execution_layer - .server - .all_payloads_invalid_terminal_block_on_forkchoice_updated(), }; match (new_payload_response, forkchoice_response) { @@ -294,9 +301,7 @@ impl InvalidPayloadRig { match forkchoice_response { Payload::Syncing => assert!(execution_status.is_strictly_optimistic()), Payload::Valid => assert!(execution_status.is_valid_and_post_bellatrix()), - Payload::Invalid { .. } - | Payload::InvalidBlockHash - | Payload::InvalidTerminalBlock => unreachable!(), + Payload::Invalid { .. } | Payload::InvalidBlockHash => unreachable!(), } assert_eq!( @@ -310,14 +315,8 @@ impl InvalidPayloadRig { "block from db must match block imported" ); } - ( - Payload::Invalid { .. } | Payload::InvalidBlockHash | Payload::InvalidTerminalBlock, - _, - ) - | ( - _, - Payload::Invalid { .. } | Payload::InvalidBlockHash | Payload::InvalidTerminalBlock, - ) => { + (Payload::Invalid { .. } | Payload::InvalidBlockHash, _) + | (_, Payload::Invalid { .. } | Payload::InvalidBlockHash) => { set_new_payload(new_payload_response); set_forkchoice_updated(forkchoice_response); @@ -473,7 +472,10 @@ async fn immediate_forkchoice_update_payload_invalid_block_hash() { #[tokio::test] async fn immediate_forkchoice_update_payload_invalid_terminal_block() { - immediate_forkchoice_update_invalid_test(|_| Payload::InvalidTerminalBlock).await + immediate_forkchoice_update_invalid_test(|_| Payload::Invalid { + latest_valid_hash: Some(ExecutionBlockHash::zero()), + }) + .await } /// Ensure the client tries to exit when the justified checkpoint is invalidated. diff --git a/beacon_node/execution_layer/src/engine_api.rs b/beacon_node/execution_layer/src/engine_api.rs index 4f957d6387..c370985ec0 100644 --- a/beacon_node/execution_layer/src/engine_api.rs +++ b/beacon_node/execution_layer/src/engine_api.rs @@ -78,7 +78,6 @@ pub enum PayloadStatusV1Status { Syncing, Accepted, InvalidBlockHash, - InvalidTerminalBlock, } #[derive(Clone, Debug, PartialEq)] diff --git a/beacon_node/execution_layer/src/engine_api/json_structures.rs b/beacon_node/execution_layer/src/engine_api/json_structures.rs index 9ed38b61b0..31aa79f055 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -319,7 +319,6 @@ pub enum JsonPayloadStatusV1Status { Syncing, Accepted, InvalidBlockHash, - InvalidTerminalBlock, } #[derive(Debug, PartialEq, Serialize, Deserialize)] @@ -338,9 +337,6 @@ impl From for JsonPayloadStatusV1Status { PayloadStatusV1Status::Syncing => JsonPayloadStatusV1Status::Syncing, PayloadStatusV1Status::Accepted => JsonPayloadStatusV1Status::Accepted, PayloadStatusV1Status::InvalidBlockHash => JsonPayloadStatusV1Status::InvalidBlockHash, - PayloadStatusV1Status::InvalidTerminalBlock => { - JsonPayloadStatusV1Status::InvalidTerminalBlock - } } } } @@ -352,9 +348,6 @@ impl From for PayloadStatusV1Status { JsonPayloadStatusV1Status::Syncing => PayloadStatusV1Status::Syncing, JsonPayloadStatusV1Status::Accepted => PayloadStatusV1Status::Accepted, JsonPayloadStatusV1Status::InvalidBlockHash => PayloadStatusV1Status::InvalidBlockHash, - JsonPayloadStatusV1Status::InvalidTerminalBlock => { - PayloadStatusV1Status::InvalidTerminalBlock - } } } } diff --git a/beacon_node/execution_layer/src/payload_status.rs b/beacon_node/execution_layer/src/payload_status.rs index 46917a0aa5..7db8e234d1 100644 --- a/beacon_node/execution_layer/src/payload_status.rs +++ b/beacon_node/execution_layer/src/payload_status.rs @@ -18,9 +18,6 @@ pub enum PayloadStatus { InvalidBlockHash { validation_error: Option, }, - InvalidTerminalBlock { - validation_error: Option, - }, } /// Processes the response from the execution engine. @@ -90,22 +87,6 @@ pub fn process_payload_status( validation_error: response.validation_error.clone(), }) } - PayloadStatusV1Status::InvalidTerminalBlock => { - // In the interests of being liberal with what we accept, only raise a - // warning here. - if response.latest_valid_hash.is_some() { - warn!( - log, - "Malformed response from execution engine"; - "msg" => "expected a null latest_valid_hash", - "status" => ?response.status - ) - } - - Ok(PayloadStatus::InvalidTerminalBlock { - validation_error: response.validation_error.clone(), - }) - } PayloadStatusV1Status::Syncing => { // In the interests of being liberal with what we accept, only raise a // warning here. diff --git a/beacon_node/execution_layer/src/test_utils/mod.rs b/beacon_node/execution_layer/src/test_utils/mod.rs index 462e34e910..18612bf303 100644 --- a/beacon_node/execution_layer/src/test_utils/mod.rs +++ b/beacon_node/execution_layer/src/test_utils/mod.rs @@ -245,8 +245,8 @@ impl MockServer { fn invalid_terminal_block_status() -> PayloadStatusV1 { PayloadStatusV1 { - status: PayloadStatusV1Status::InvalidTerminalBlock, - latest_valid_hash: None, + status: PayloadStatusV1Status::Invalid, + latest_valid_hash: Some(ExecutionBlockHash::zero()), validation_error: Some("static response".into()), } } From 4e05f19fb526237fd7423801a8bdee3b7e0ec0b7 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 10 Aug 2022 07:52:59 +0000 Subject: [PATCH 20/21] Serve Bellatrix preset in BN API (#3425) ## Issue Addressed Resolves #3388 Resolves #2638 ## Proposed Changes - Return the `BellatrixPreset` on `/eth/v1/config/spec` by default. - Allow users to opt out of this by providing `--http-spec-fork=altair` (unless there's a Bellatrix fork epoch set). - Add the Altair constants from #2638 and make serving the constants non-optional (the `http-disable-legacy-spec` flag is deprecated). - Modify the VC to only read the `Config` and not to log extra fields. This prevents it from having to muck around parsing the `ConfigAndPreset` fields it doesn't need. ## Additional Info This change is backwards-compatible for the VC and the BN, but is marked as a breaking change for the removal of `--http-disable-legacy-spec`. I tried making `Config` a `superstruct` too, but getting the automatic decoding to work was a huge pain and was going to require a lot of hacks, so I gave up in favour of keeping the default-based approach we have now. --- Cargo.lock | 1 + beacon_node/beacon_chain/src/test_utils.rs | 3 +- beacon_node/http_api/src/lib.rs | 13 +- beacon_node/http_api/tests/common.rs | 2 +- beacon_node/http_api/tests/tests.rs | 11 +- beacon_node/src/cli.rs | 10 +- beacon_node/src/config.rs | 9 +- common/eth2/src/lib.rs | 4 +- common/eth2/src/lighthouse_vc/http_client.rs | 4 +- common/eth2/src/mixin.rs | 6 +- consensus/types/Cargo.toml | 1 + consensus/types/src/chain_spec.rs | 21 +-- consensus/types/src/config_and_preset.rs | 143 ++++++++++--------- consensus/types/src/fork_name.rs | 12 +- consensus/types/src/lib.rs | 4 +- lighthouse/tests/beacon_node.rs | 17 ++- validator_client/src/beacon_node_fallback.rs | 30 ++-- validator_client/src/http_api/mod.rs | 3 +- validator_client/src/http_api/tests.rs | 15 +- 19 files changed, 167 insertions(+), 142 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3702b95485..a6b5f56374 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7073,6 +7073,7 @@ dependencies = [ "itertools", "lazy_static", "log", + "maplit", "parking_lot 0.12.1", "rand 0.8.5", "rand_xorshift", diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 411bd7b1fd..9b62590703 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -132,8 +132,7 @@ pub fn test_spec() -> ChainSpec { FORK_NAME_ENV_VAR, e ) }); - let fork = ForkName::from_str(fork_name.as_str()) - .unwrap_or_else(|()| panic!("unknown FORK_NAME: {}", fork_name)); + let fork = ForkName::from_str(fork_name.as_str()).unwrap(); fork.make_genesis_spec(E::default_spec()) } else { E::default_spec() diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index ec34dd0663..bcd8788465 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -104,9 +104,9 @@ pub struct Config { pub listen_addr: IpAddr, pub listen_port: u16, pub allow_origin: Option, - pub serve_legacy_spec: bool, pub tls_config: Option, pub allow_sync_stalled: bool, + pub spec_fork_name: Option, } impl Default for Config { @@ -116,9 +116,9 @@ impl Default for Config { listen_addr: IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), listen_port: 5052, allow_origin: None, - serve_legacy_spec: true, tls_config: None, allow_sync_stalled: false, + spec_fork_name: None, } } } @@ -1534,18 +1534,15 @@ pub fn serve( }); // GET config/spec - let serve_legacy_spec = ctx.config.serve_legacy_spec; + let spec_fork_name = ctx.config.spec_fork_name; let get_config_spec = config_path .and(warp::path("spec")) .and(warp::path::end()) .and(chain_filter.clone()) .and_then(move |chain: Arc>| { blocking_json_task(move || { - let mut config_and_preset = - ConfigAndPreset::from_chain_spec::(&chain.spec); - if serve_legacy_spec { - config_and_preset.make_backwards_compat(&chain.spec); - } + let config_and_preset = + ConfigAndPreset::from_chain_spec::(&chain.spec, spec_fork_name); Ok(api_types::GenericResponse::from(config_and_preset)) }) }); diff --git a/beacon_node/http_api/tests/common.rs b/beacon_node/http_api/tests/common.rs index 8f9856991f..1dd7aea923 100644 --- a/beacon_node/http_api/tests/common.rs +++ b/beacon_node/http_api/tests/common.rs @@ -141,9 +141,9 @@ pub async fn create_api_server_on_port( listen_addr: IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), listen_port: port, allow_origin: None, - serve_legacy_spec: true, tls_config: None, allow_sync_stalled: false, + spec_fork_name: None, }, chain: Some(chain.clone()), network_tx: Some(network_tx), diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index fa41102292..bd25450a47 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -1253,10 +1253,13 @@ impl ApiTester { } pub async fn test_get_config_spec(self) -> Self { - let result = self.client.get_config_spec().await.unwrap().data; - - let mut expected = ConfigAndPreset::from_chain_spec::(&self.chain.spec); - expected.make_backwards_compat(&self.chain.spec); + let result = self + .client + .get_config_spec::() + .await + .map(|res| ConfigAndPreset::Bellatrix(res.data)) + .unwrap(); + let expected = ConfigAndPreset::from_chain_spec::(&self.chain.spec, None); assert_eq!(result, expected); diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs index 3515263878..edf79ad34f 100644 --- a/beacon_node/src/cli.rs +++ b/beacon_node/src/cli.rs @@ -229,8 +229,14 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { .arg( Arg::with_name("http-disable-legacy-spec") .long("http-disable-legacy-spec") - .help("Disable serving of legacy data on the /config/spec endpoint. May be \ - disabled by default in a future release.") + .hidden(true) + ) + .arg( + Arg::with_name("http-spec-fork") + .long("http-spec-fork") + .help("Serve the spec for a specific hard fork on /eth/v1/config/spec. It should \ + not be necessary to set this flag.") + .takes_value(true) ) .arg( Arg::with_name("http-enable-tls") diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index 6daee50de0..35d566d76e 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -116,7 +116,14 @@ pub fn get_config( } if cli_args.is_present("http-disable-legacy-spec") { - client_config.http_api.serve_legacy_spec = false; + warn!( + log, + "The flag --http-disable-legacy-spec is deprecated and will be removed" + ); + } + + if let Some(fork_name) = clap_utils::parse_optional(cli_args, "http-spec-fork")? { + client_config.http_api.spec_fork_name = Some(fork_name); } if cli_args.is_present("http-enable-tls") { diff --git a/common/eth2/src/lib.rs b/common/eth2/src/lib.rs index 21608ba6dd..6317523fee 100644 --- a/common/eth2/src/lib.rs +++ b/common/eth2/src/lib.rs @@ -977,7 +977,9 @@ impl BeaconNodeHttpClient { } /// `GET config/spec` - pub async fn get_config_spec(&self) -> Result, Error> { + pub async fn get_config_spec( + &self, + ) -> Result, Error> { let mut path = self.eth_path(V1)?; path.path_segments_mut() diff --git a/common/eth2/src/lighthouse_vc/http_client.rs b/common/eth2/src/lighthouse_vc/http_client.rs index abed4fe5e7..5f83e81aa0 100644 --- a/common/eth2/src/lighthouse_vc/http_client.rs +++ b/common/eth2/src/lighthouse_vc/http_client.rs @@ -354,7 +354,9 @@ impl ValidatorClientHttpClient { } /// `GET lighthouse/spec` - pub async fn get_lighthouse_spec(&self) -> Result, Error> { + pub async fn get_lighthouse_spec( + &self, + ) -> Result, Error> { let mut path = self.server.full.clone(); path.path_segments_mut() diff --git a/common/eth2/src/mixin.rs b/common/eth2/src/mixin.rs index 1de26961e6..a33cf8a40c 100644 --- a/common/eth2/src/mixin.rs +++ b/common/eth2/src/mixin.rs @@ -21,17 +21,17 @@ impl ResponseOptional for Result { /// Trait for extracting the fork name from the headers of a response. pub trait ResponseForkName { #[allow(clippy::result_unit_err)] - fn fork_name_from_header(&self) -> Result, ()>; + fn fork_name_from_header(&self) -> Result, String>; } impl ResponseForkName for Response { - fn fork_name_from_header(&self) -> Result, ()> { + fn fork_name_from_header(&self) -> Result, String> { self.headers() .get(CONSENSUS_VERSION_HEADER) .map(|fork_name| { fork_name .to_str() - .map_err(|_| ()) + .map_err(|e| e.to_string()) .and_then(ForkName::from_str) }) .transpose() diff --git a/consensus/types/Cargo.toml b/consensus/types/Cargo.toml index c3e454fdfc..68fdbf7990 100644 --- a/consensus/types/Cargo.toml +++ b/consensus/types/Cargo.toml @@ -47,6 +47,7 @@ superstruct = "0.5.0" serde_json = "1.0.74" smallvec = "1.8.0" serde_with = "1.13.0" +maplit = "1.0.2" [dev-dependencies] criterion = "0.3.3" diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index 3668d0524c..8d56ce2da9 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -803,6 +803,10 @@ impl Default for ChainSpec { } /// Exact implementation of the *config* object from the Ethereum spec (YAML/JSON). +/// +/// Fields relevant to hard forks after Altair should be optional so that we can continue +/// to parse Altair configs. This default approach turns out to be much simpler than trying to +/// make `Config` a superstruct because of the hassle of deserializing an untagged enum. #[derive(Serialize, Deserialize, Debug, PartialEq, Clone)] #[serde(rename_all = "UPPERCASE")] pub struct Config { @@ -813,17 +817,13 @@ pub struct Config { #[serde(default)] pub preset_base: String, - // TODO(merge): remove this default #[serde(default = "default_terminal_total_difficulty")] #[serde(with = "eth2_serde_utils::quoted_u256")] pub terminal_total_difficulty: Uint256, - // TODO(merge): remove this default #[serde(default = "default_terminal_block_hash")] pub terminal_block_hash: ExecutionBlockHash, - // TODO(merge): remove this default #[serde(default = "default_terminal_block_hash_activation_epoch")] pub terminal_block_hash_activation_epoch: Epoch, - // TODO(merge): remove this default #[serde(default = "default_safe_slots_to_import_optimistically")] #[serde(with = "eth2_serde_utils::quoted_u64")] pub safe_slots_to_import_optimistically: u64, @@ -843,12 +843,10 @@ pub struct Config { #[serde(deserialize_with = "deserialize_fork_epoch")] pub altair_fork_epoch: Option>, - // TODO(merge): remove this default #[serde(default = "default_bellatrix_fork_version")] #[serde(with = "eth2_serde_utils::bytes_4_hex")] bellatrix_fork_version: [u8; 4], - // TODO(merge): remove this default - #[serde(default = "default_bellatrix_fork_epoch")] + #[serde(default)] #[serde(serialize_with = "serialize_fork_epoch")] #[serde(deserialize_with = "deserialize_fork_epoch")] pub bellatrix_fork_epoch: Option>, @@ -890,10 +888,6 @@ fn default_bellatrix_fork_version() -> [u8; 4] { [0xff, 0xff, 0xff, 0xff] } -fn default_bellatrix_fork_epoch() -> Option> { - None -} - /// Placeholder value: 2^256-2^10 (115792089237316195423570985008687907853269984665640564039457584007913129638912). /// /// Taken from https://github.com/ethereum/consensus-specs/blob/d5e4828aecafaf1c57ef67a5f23c4ae7b08c5137/configs/mainnet.yaml#L15-L16 @@ -1335,10 +1329,7 @@ mod yaml_tests { default_safe_slots_to_import_optimistically() ); - assert_eq!( - chain_spec.bellatrix_fork_epoch, - default_bellatrix_fork_epoch() - ); + assert_eq!(chain_spec.bellatrix_fork_epoch, None); assert_eq!( chain_spec.bellatrix_fork_version, diff --git a/consensus/types/src/config_and_preset.rs b/consensus/types/src/config_and_preset.rs index 8b3a753bd5..e624afe2db 100644 --- a/consensus/types/src/config_and_preset.rs +++ b/consensus/types/src/config_and_preset.rs @@ -1,12 +1,21 @@ -use crate::{AltairPreset, BasePreset, BellatrixPreset, ChainSpec, Config, EthSpec}; +use crate::{ + consts::altair, AltairPreset, BasePreset, BellatrixPreset, ChainSpec, Config, EthSpec, ForkName, +}; +use maplit::hashmap; use serde_derive::{Deserialize, Serialize}; use serde_json::Value; use std::collections::HashMap; +use superstruct::superstruct; /// Fusion of a runtime-config with the compile-time preset values. /// /// Mostly useful for the API. +#[superstruct( + variants(Altair, Bellatrix), + variant_attributes(derive(Serialize, Deserialize, Debug, PartialEq, Clone)) +)] #[derive(Serialize, Deserialize, Debug, PartialEq, Clone)] +#[serde(untagged)] pub struct ConfigAndPreset { #[serde(flatten)] pub config: Config, @@ -15,80 +24,75 @@ pub struct ConfigAndPreset { pub base_preset: BasePreset, #[serde(flatten)] pub altair_preset: AltairPreset, - // TODO(merge): re-enable - // #[serde(flatten)] - // pub bellatrix_preset: BellatrixPreset, + #[superstruct(only(Bellatrix))] + #[serde(flatten)] + pub bellatrix_preset: BellatrixPreset, /// The `extra_fields` map allows us to gracefully decode fields intended for future hard forks. #[serde(flatten)] pub extra_fields: HashMap, } impl ConfigAndPreset { - pub fn from_chain_spec(spec: &ChainSpec) -> Self { + pub fn from_chain_spec(spec: &ChainSpec, fork_name: Option) -> Self { let config = Config::from_chain_spec::(spec); let base_preset = BasePreset::from_chain_spec::(spec); let altair_preset = AltairPreset::from_chain_spec::(spec); - // TODO(merge): re-enable - let _bellatrix_preset = BellatrixPreset::from_chain_spec::(spec); - let extra_fields = HashMap::new(); + let extra_fields = get_extra_fields(spec); - Self { - config, - base_preset, - altair_preset, - extra_fields, + if spec.bellatrix_fork_epoch.is_some() + || fork_name == None + || fork_name == Some(ForkName::Merge) + { + let bellatrix_preset = BellatrixPreset::from_chain_spec::(spec); + + ConfigAndPreset::Bellatrix(ConfigAndPresetBellatrix { + config, + base_preset, + altair_preset, + bellatrix_preset, + extra_fields, + }) + } else { + ConfigAndPreset::Altair(ConfigAndPresetAltair { + config, + base_preset, + altair_preset, + extra_fields, + }) } } +} - /// Add fields that were previously part of the config but are now constants. - pub fn make_backwards_compat(&mut self, spec: &ChainSpec) { - let hex_string = |value: &[u8]| format!("0x{}", hex::encode(&value)); - let u32_hex = |v: u32| hex_string(&v.to_le_bytes()); - let u8_hex = |v: u8| hex_string(&v.to_le_bytes()); - let fields = vec![ - ( - "bls_withdrawal_prefix", - u8_hex(spec.bls_withdrawal_prefix_byte), - ), - ( - "domain_beacon_proposer", - u32_hex(spec.domain_beacon_proposer), - ), - ( - "domain_beacon_attester", - u32_hex(spec.domain_beacon_attester), - ), - ("domain_randao", u32_hex(spec.domain_randao)), - ("domain_deposit", u32_hex(spec.domain_deposit)), - ("domain_voluntary_exit", u32_hex(spec.domain_voluntary_exit)), - ( - "domain_selection_proof", - u32_hex(spec.domain_selection_proof), - ), - ( - "domain_aggregate_and_proof", - u32_hex(spec.domain_aggregate_and_proof), - ), - ( - "domain_application_mask", - u32_hex(spec.domain_application_mask), - ), - ( - "target_aggregators_per_committee", - spec.target_aggregators_per_committee.to_string(), - ), - ( - "random_subnets_per_validator", - spec.random_subnets_per_validator.to_string(), - ), - ( - "epochs_per_random_subnet_subscription", - spec.epochs_per_random_subnet_subscription.to_string(), - ), - ]; - for (key, value) in fields { - self.extra_fields.insert(key.to_uppercase(), value.into()); - } +/// Get a hashmap of constants to add to the `PresetAndConfig` +pub fn get_extra_fields(spec: &ChainSpec) -> HashMap { + let hex_string = |value: &[u8]| format!("0x{}", hex::encode(&value)).into(); + let u32_hex = |v: u32| hex_string(&v.to_le_bytes()); + let u8_hex = |v: u8| hex_string(&v.to_le_bytes()); + hashmap! { + "bls_withdrawal_prefix".to_uppercase() => u8_hex(spec.bls_withdrawal_prefix_byte), + "domain_beacon_proposer".to_uppercase() => u32_hex(spec.domain_beacon_proposer), + "domain_beacon_attester".to_uppercase() => u32_hex(spec.domain_beacon_attester), + "domain_randao".to_uppercase()=> u32_hex(spec.domain_randao), + "domain_deposit".to_uppercase()=> u32_hex(spec.domain_deposit), + "domain_voluntary_exit".to_uppercase() => u32_hex(spec.domain_voluntary_exit), + "domain_selection_proof".to_uppercase() => u32_hex(spec.domain_selection_proof), + "domain_aggregate_and_proof".to_uppercase() => u32_hex(spec.domain_aggregate_and_proof), + "domain_application_mask".to_uppercase()=> u32_hex(spec.domain_application_mask), + "target_aggregators_per_committee".to_uppercase() => + spec.target_aggregators_per_committee.to_string().into(), + "random_subnets_per_validator".to_uppercase() => + spec.random_subnets_per_validator.to_string().into(), + "epochs_per_random_subnet_subscription".to_uppercase() => + spec.epochs_per_random_subnet_subscription.to_string().into(), + "domain_contribution_and_proof".to_uppercase() => + u32_hex(spec.domain_contribution_and_proof), + "domain_sync_committee".to_uppercase() => u32_hex(spec.domain_sync_committee), + "domain_sync_committee_selection_proof".to_uppercase() => + u32_hex(spec.domain_sync_committee_selection_proof), + "sync_committee_subnet_count".to_uppercase() => + altair::SYNC_COMMITTEE_SUBNET_COUNT.to_string().into(), + "target_aggregators_per_sync_subcommittee".to_uppercase() => + altair::TARGET_AGGREGATORS_PER_SYNC_SUBCOMMITTEE.to_string().into(), } } @@ -108,15 +112,16 @@ mod test { .open(tmp_file.as_ref()) .expect("error opening file"); let mainnet_spec = ChainSpec::mainnet(); - let mut yamlconfig = ConfigAndPreset::from_chain_spec::(&mainnet_spec); + let mut yamlconfig = + ConfigAndPreset::from_chain_spec::(&mainnet_spec, None); let (k1, v1) = ("SAMPLE_HARDFORK_KEY1", "123456789"); let (k2, v2) = ("SAMPLE_HARDFORK_KEY2", "987654321"); let (k3, v3) = ("SAMPLE_HARDFORK_KEY3", 32); let (k4, v4) = ("SAMPLE_HARDFORK_KEY4", Value::Null); - yamlconfig.extra_fields.insert(k1.into(), v1.into()); - yamlconfig.extra_fields.insert(k2.into(), v2.into()); - yamlconfig.extra_fields.insert(k3.into(), v3.into()); - yamlconfig.extra_fields.insert(k4.into(), v4); + yamlconfig.extra_fields_mut().insert(k1.into(), v1.into()); + yamlconfig.extra_fields_mut().insert(k2.into(), v2.into()); + yamlconfig.extra_fields_mut().insert(k3.into(), v3.into()); + yamlconfig.extra_fields_mut().insert(k4.into(), v4); serde_yaml::to_writer(writer, &yamlconfig).expect("failed to write or serialize"); @@ -125,8 +130,8 @@ mod test { .write(false) .open(tmp_file.as_ref()) .expect("error while opening the file"); - let from: ConfigAndPreset = + let from: ConfigAndPresetBellatrix = serde_yaml::from_reader(reader).expect("error while deserializing"); - assert_eq!(from, yamlconfig); + assert_eq!(ConfigAndPreset::Bellatrix(from), yamlconfig); } } diff --git a/consensus/types/src/fork_name.rs b/consensus/types/src/fork_name.rs index 4a2e762087..e97b08309b 100644 --- a/consensus/types/src/fork_name.rs +++ b/consensus/types/src/fork_name.rs @@ -106,14 +106,14 @@ macro_rules! map_fork_name_with { } impl FromStr for ForkName { - type Err = (); + type Err = String; - fn from_str(fork_name: &str) -> Result { + fn from_str(fork_name: &str) -> Result { Ok(match fork_name.to_lowercase().as_ref() { "phase0" | "base" => ForkName::Base, "altair" => ForkName::Altair, "bellatrix" | "merge" => ForkName::Merge, - _ => return Err(()), + _ => return Err(format!("unknown fork name: {}", fork_name)), }) } } @@ -138,7 +138,7 @@ impl TryFrom for ForkName { type Error = String; fn try_from(s: String) -> Result { - Self::from_str(&s).map_err(|()| format!("Invalid fork name: {}", s)) + Self::from_str(&s) } } @@ -178,8 +178,8 @@ mod test { assert_eq!(ForkName::from_str("AlTaIr"), Ok(ForkName::Altair)); assert_eq!(ForkName::from_str("altair"), Ok(ForkName::Altair)); - assert_eq!(ForkName::from_str("NO_NAME"), Err(())); - assert_eq!(ForkName::from_str("no_name"), Err(())); + assert!(ForkName::from_str("NO_NAME").is_err()); + assert!(ForkName::from_str("no_name").is_err()); } #[test] diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index 7823ec223c..f05012c0b7 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -110,7 +110,9 @@ pub use crate::beacon_committee::{BeaconCommittee, OwnedBeaconCommittee}; pub use crate::beacon_state::{BeaconTreeHashCache, Error as BeaconStateError, *}; pub use crate::chain_spec::{ChainSpec, Config, Domain}; pub use crate::checkpoint::Checkpoint; -pub use crate::config_and_preset::ConfigAndPreset; +pub use crate::config_and_preset::{ + ConfigAndPreset, ConfigAndPresetAltair, ConfigAndPresetBellatrix, +}; pub use crate::contribution_and_proof::ContributionAndProof; pub use crate::deposit::{Deposit, DEPOSIT_TREE_DEPTH}; pub use crate::deposit_data::DepositData; diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index 0236ba6589..9d952e5cc5 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -11,7 +11,7 @@ use std::process::Command; use std::str::FromStr; use std::string::ToString; use tempfile::TempDir; -use types::{Address, Checkpoint, Epoch, ExecutionBlockHash, Hash256, MainnetEthSpec}; +use types::{Address, Checkpoint, Epoch, ExecutionBlockHash, ForkName, Hash256, MainnetEthSpec}; use unused_port::{unused_tcp_port, unused_udp_port}; const DEFAULT_ETH1_ENDPOINT: &str = "http://localhost:8545/"; @@ -949,6 +949,21 @@ fn http_tls_flags() { }); } +#[test] +fn http_spec_fork_default() { + CommandLineTest::new() + .run_with_zero_port() + .with_config(|config| assert_eq!(config.http_api.spec_fork_name, None)); +} + +#[test] +fn http_spec_fork_override() { + CommandLineTest::new() + .flag("http-spec-fork", Some("altair")) + .run_with_zero_port() + .with_config(|config| assert_eq!(config.http_api.spec_fork_name, Some(ForkName::Altair))); +} + // Tests for Metrics flags. #[test] fn metrics_flag() { diff --git a/validator_client/src/beacon_node_fallback.rs b/validator_client/src/beacon_node_fallback.rs index d4f7c6c874..0b808e71bb 100644 --- a/validator_client/src/beacon_node_fallback.rs +++ b/validator_client/src/beacon_node_fallback.rs @@ -7,7 +7,7 @@ use crate::http_metrics::metrics::{inc_counter_vec, ENDPOINT_ERRORS, ENDPOINT_RE use environment::RuntimeContext; use eth2::BeaconNodeHttpClient; use futures::future; -use slog::{debug, error, info, warn, Logger}; +use slog::{error, info, warn, Logger}; use slot_clock::SlotClock; use std::fmt; use std::fmt::Debug; @@ -16,7 +16,7 @@ use std::marker::PhantomData; use std::sync::Arc; use std::time::Duration; use tokio::{sync::RwLock, time::sleep}; -use types::{ChainSpec, EthSpec}; +use types::{ChainSpec, Config, EthSpec}; /// The number of seconds *prior* to slot start that we will try and update the state of fallback /// nodes. @@ -213,9 +213,9 @@ impl CandidateBeaconNode { /// Checks if the node has the correct specification. async fn is_compatible(&self, spec: &ChainSpec, log: &Logger) -> Result<(), CandidateError> { - let config_and_preset = self + let config = self .beacon_node - .get_config_spec() + .get_config_spec::() .await .map_err(|e| { error!( @@ -228,25 +228,15 @@ impl CandidateBeaconNode { })? .data; - let beacon_node_spec = - ChainSpec::from_config::(&config_and_preset.config).ok_or_else(|| { - error!( - log, - "The minimal/mainnet spec type of the beacon node does not match the validator \ - client. See the --network command."; - "endpoint" => %self.beacon_node, - ); - CandidateError::Incompatible - })?; - - if !config_and_preset.extra_fields.is_empty() { - debug!( + let beacon_node_spec = ChainSpec::from_config::(&config).ok_or_else(|| { + error!( log, - "Beacon spec includes unknown fields"; + "The minimal/mainnet spec type of the beacon node does not match the validator \ + client. See the --network command."; "endpoint" => %self.beacon_node, - "fields" => ?config_and_preset.extra_fields, ); - } + CandidateError::Incompatible + })?; if beacon_node_spec.genesis_fork_version != spec.genesis_fork_version { error!( diff --git a/validator_client/src/http_api/mod.rs b/validator_client/src/http_api/mod.rs index a5d8d0e71c..1e48e86c05 100644 --- a/validator_client/src/http_api/mod.rs +++ b/validator_client/src/http_api/mod.rs @@ -217,8 +217,7 @@ pub fn serve( .and(signer.clone()) .and_then(|spec: Arc<_>, signer| { blocking_signed_json_task(signer, move || { - let mut config = ConfigAndPreset::from_chain_spec::(&spec); - config.make_backwards_compat(&spec); + let config = ConfigAndPreset::from_chain_spec::(&spec, None); Ok(api_types::GenericResponse::from(config)) }) }); diff --git a/validator_client/src/http_api/tests.rs b/validator_client/src/http_api/tests.rs index e67a82634c..b121dda5b1 100644 --- a/validator_client/src/http_api/tests.rs +++ b/validator_client/src/http_api/tests.rs @@ -208,10 +208,13 @@ impl ApiTester { } pub async fn test_get_lighthouse_spec(self) -> Self { - let result = self.client.get_lighthouse_spec().await.unwrap().data; - - let mut expected = ConfigAndPreset::from_chain_spec::(&E::default_spec()); - expected.make_backwards_compat(&E::default_spec()); + let result = self + .client + .get_lighthouse_spec::() + .await + .map(|res| ConfigAndPreset::Bellatrix(res.data)) + .unwrap(); + let expected = ConfigAndPreset::from_chain_spec::(&E::default_spec(), None); assert_eq!(result, expected); @@ -623,7 +626,9 @@ fn routes_with_invalid_auth() { .await .test_with_invalid_auth(|client| async move { client.get_lighthouse_health().await }) .await - .test_with_invalid_auth(|client| async move { client.get_lighthouse_spec().await }) + .test_with_invalid_auth(|client| async move { + client.get_lighthouse_spec::().await + }) .await .test_with_invalid_auth( |client| async move { client.get_lighthouse_validators().await }, From 4fc0cb121c6f90b12ff10a87d4d091cc11285270 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 10 Aug 2022 13:06:46 +0000 Subject: [PATCH 21/21] Remove some "wontfix" TODOs for the merge (#3449) ## Issue Addressed NA ## Proposed Changes Removes three types of TODOs: 1. `execution_layer/src/lib.rs`: It was [determined](https://github.com/ethereum/consensus-specs/issues/2636#issuecomment-988688742) that there is no action required here. 2. `beacon_processor/worker/gossip_methods.rs`: Removed TODOs relating to peer scoring that have already been addressed via `epe.penalize_peer()`. - It seems `cargo fmt` wanted to adjust some things here as well :shrug: 3. `proto_array_fork_choice.rs`: it would be nice to remove that useless `bool` for cleanliness, but I don't think it's something we need to do and the TODO just makes things look messier IMO. ## Additional Info There should be no functional changes to the code in this PR. There are still some TODOs lingering, those ones require actual changes or more thought. --- beacon_node/execution_layer/src/lib.rs | 8 -------- .../beacon_processor/worker/gossip_methods.rs | 16 +++++++++++----- .../proto_array/src/proto_array_fork_choice.rs | 2 +- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index f56ea8f797..e7bbc6cd5e 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -1267,14 +1267,6 @@ impl ExecutionLayer { } /// Maps to the `eth_getBlockByHash` JSON-RPC call. - /// - /// ## TODO(merge) - /// - /// This will return an execution block regardless of whether or not it was created by a PoW - /// miner (pre-merge) or a PoS validator (post-merge). It's not immediately clear if this is - /// correct or not, see the discussion here: - /// - /// https://github.com/ethereum/consensus-specs/issues/2636 async fn get_pow_block( &self, engine: &Engine, diff --git a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs index 12172e0e53..e6625e43f8 100644 --- a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs @@ -771,12 +771,15 @@ impl Worker { debug!(self.log, "Could not verify block for gossip, ignoring the block"; "error" => %e); // Prevent recurring behaviour by penalizing the peer slightly. - self.gossip_penalize_peer(peer_id, PeerAction::HighToleranceError, "gossip_block_high"); + self.gossip_penalize_peer( + peer_id, + PeerAction::HighToleranceError, + "gossip_block_high", + ); self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore); return None; } - // TODO(merge): reconsider peer scoring for this event. - Err(ref e @BlockError::ExecutionPayloadError(ref epe)) if !epe.penalize_peer() => { + Err(ref e @ BlockError::ExecutionPayloadError(ref epe)) if !epe.penalize_peer() => { debug!(self.log, "Could not verify block for gossip, ignoring the block"; "error" => %e); self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore); @@ -795,7 +798,6 @@ impl Worker { | Err(e @ BlockError::TooManySkippedSlots { .. }) | Err(e @ BlockError::WeakSubjectivityConflict) | Err(e @ BlockError::InconsistentFork(_)) - // TODO(merge): reconsider peer scoring for this event. | Err(e @ BlockError::ExecutionPayloadError(_)) // TODO(merge): reconsider peer scoring for this event. | Err(e @ BlockError::ParentExecutionPayloadInvalid { .. }) @@ -803,7 +805,11 @@ impl Worker { warn!(self.log, "Could not verify block for gossip, rejecting the block"; "error" => %e); self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Reject); - self.gossip_penalize_peer(peer_id, PeerAction::LowToleranceError, "gossip_block_low"); + self.gossip_penalize_peer( + peer_id, + PeerAction::LowToleranceError, + "gossip_block_low", + ); return None; } }; diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 306c986018..9902ccb1cc 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -38,7 +38,7 @@ pub enum ExecutionStatus { /// /// This `bool` only exists to satisfy our SSZ implementation which requires all variants /// to have a value. It can be set to anything. - Irrelevant(bool), // TODO(merge): fix bool. + Irrelevant(bool), } impl ExecutionStatus {