diff --git a/Cargo.lock b/Cargo.lock index 3e426228f..c08719091 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -330,6 +330,7 @@ dependencies = [ "state_processing", "store", "strum", + "superstruct", "task_executor", "tempfile", "tokio", @@ -976,38 +977,14 @@ dependencies = [ "zeroize", ] -[[package]] -name = "darling" -version = "0.12.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5f2c43f534ea4b0b049015d00269734195e6d3f0f6635cb692251aca6f9f8b3c" -dependencies = [ - "darling_core 0.12.4", - "darling_macro 0.12.4", -] - [[package]] name = "darling" version = "0.13.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "757c0ded2af11d8e739c4daea1ac623dd1624b06c844cf3f5a39f1bdbd99bb12" dependencies = [ - "darling_core 0.13.0", - "darling_macro 0.13.0", -] - -[[package]] -name = "darling_core" -version = "0.12.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8e91455b86830a1c21799d94524df0845183fa55bafd9aa137b01c7d1065fa36" -dependencies = [ - "fnv", - "ident_case", - "proc-macro2", - "quote", - "strsim 0.10.0", - "syn", + "darling_core", + "darling_macro", ] [[package]] @@ -1024,24 +1001,13 @@ dependencies = [ "syn", ] -[[package]] -name = "darling_macro" -version = "0.12.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "29b5acf0dea37a7f66f7b25d2c5e93fd46f8f6968b1a5d7a3e02e97768afc95a" -dependencies = [ - "darling_core 0.12.4", - "quote", - "syn", -] - [[package]] name = "darling_macro" version = "0.13.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ade7bff147130fe5e6d39f089c6bd49ec0250f35d70b2eebf72afdfc919f15cc" dependencies = [ - "darling_core 0.13.0", + "darling_core", "quote", "syn", ] @@ -1121,14 +1087,14 @@ dependencies = [ [[package]] name = "derive_more" -version = "0.99.16" +version = "0.99.17" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "40eebddd2156ce1bb37b20bbe5151340a31828b1f2d22ba4141f3531710e38df" +checksum = "4fb810d30a7c1953f91334de7244731fc3f3c10d7fe163338a35b9f640960321" dependencies = [ "convert_case", "proc-macro2", "quote", - "rustc_version 0.3.3", + "rustc_version 0.4.0", "syn", ] @@ -1597,7 +1563,7 @@ dependencies = [ name = "eth2_ssz_derive" version = "0.3.0" dependencies = [ - "darling 0.13.0", + "darling", "proc-macro2", "quote", "syn", @@ -1609,7 +1575,7 @@ version = "0.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "635b86d2c941bb71e7419a571e1763d65c93e51a1bafc400352e3bef6ff59fc9" dependencies = [ - "darling 0.13.0", + "darling", "proc-macro2", "quote", "syn", @@ -5671,11 +5637,11 @@ checksum = "6bdef32e8150c2a081110b42772ffe7d7c9032b606bc226c8260fd97e0976601" [[package]] name = "superstruct" -version = "0.2.0" +version = "0.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8bf7f6700d7c135cf4e4900c2cfba9a12ecad1fdc45594aad48f6b344b2589a0" +checksum = "ecffe12af481bd0b8950f90676d61fb1e5fc33f1f1c41ce5df11e83fb509aaab" dependencies = [ - "darling 0.12.4", + "darling", "itertools", "proc-macro2", "quote", @@ -5693,9 +5659,9 @@ dependencies = [ [[package]] name = "syn" -version = "1.0.81" +version = "1.0.82" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f2afee18b8beb5a596ecb4a2dce128c719b4ba399d34126b9e4396e3f9860966" +checksum = "8daf5dd0bb60cbd4137b1b587d2fc0ae729bc07cf01cd70b36a1ed5ade3b9d59" dependencies = [ "proc-macro2", "quote", @@ -6195,7 +6161,7 @@ dependencies = [ name = "tree_hash_derive" version = "0.4.0" dependencies = [ - "darling 0.13.0", + "darling", "quote", "syn", ] @@ -6206,7 +6172,7 @@ version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3cd22d128157837a4434bb51119aef11103f17bfe8c402ce688cf25aa1e608ad" dependencies = [ - "darling 0.13.0", + "darling", "quote", "syn", ] diff --git a/beacon_node/beacon_chain/Cargo.toml b/beacon_node/beacon_chain/Cargo.toml index d503a01b8..c4bd3bf7b 100644 --- a/beacon_node/beacon_chain/Cargo.toml +++ b/beacon_node/beacon_chain/Cargo.toml @@ -58,6 +58,7 @@ strum = { version = "0.21.0", features = ["derive"] } logging = { path = "../../common/logging" } execution_layer = { path = "../execution_layer" } sensitive_url = { path = "../../common/sensitive_url" } +superstruct = "0.3.0" [[test]] name = "beacon_chain_tests" diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index ca11c8a7b..1f36e0e65 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -51,7 +51,7 @@ use eth2::types::{ EventKind, SseBlock, SseChainReorg, SseFinalizedCheckpoint, SseHead, SseLateHead, SyncDuty, }; use execution_layer::ExecutionLayer; -use fork_choice::ForkChoice; +use fork_choice::{AttestationFromBlock, ForkChoice}; use futures::channel::mpsc::Sender; use itertools::process_results; use itertools::Itertools; @@ -1700,7 +1700,11 @@ impl BeaconChain { self.fork_choice .write() - .on_attestation(self.slot()?, verified.indexed_attestation()) + .on_attestation( + self.slot()?, + verified.indexed_attestation(), + AttestationFromBlock::False, + ) .map_err(Into::into) } @@ -2443,11 +2447,17 @@ impl BeaconChain { { let _fork_choice_block_timer = metrics::start_timer(&metrics::FORK_CHOICE_PROCESS_BLOCK_TIMES); + let block_delay = self + .slot_clock + .seconds_from_current_slot_start(self.spec.seconds_per_slot) + .ok_or(Error::UnableToComputeTimeAtSlot)?; + fork_choice .on_block( current_slot, &block, block_root, + block_delay, &state, payload_verification_status, &self.spec, @@ -2472,7 +2482,11 @@ impl BeaconChain { let indexed_attestation = get_indexed_attestation(committee.committee, attestation) .map_err(|e| BlockError::BeaconChainError(e.into()))?; - match fork_choice.on_attestation(current_slot, &indexed_attestation) { + match fork_choice.on_attestation( + current_slot, + &indexed_attestation, + AttestationFromBlock::True, + ) { Ok(()) => Ok(()), // Ignore invalid attestations whilst importing attestations from a block. The // block might be very old and therefore the attestations useless to fork choice. @@ -3009,7 +3023,10 @@ impl BeaconChain { fn fork_choice_internal(&self) -> Result<(), Error> { // Determine the root of the block that is the head of the chain. - let beacon_block_root = self.fork_choice.write().get_head(self.slot()?)?; + let beacon_block_root = self + .fork_choice + .write() + .get_head(self.slot()?, &self.spec)?; let current_head = self.head_info()?; let old_finalized_checkpoint = current_head.finalized_checkpoint; diff --git a/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs b/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs index 7d9e42fb8..956c50e03 100644 --- a/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs +++ b/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs @@ -1,15 +1,17 @@ //! Defines the `BeaconForkChoiceStore` which provides the persistent storage for the `ForkChoice` //! struct. //! -//! Additionally, the private `BalancesCache` struct is defined; a cache designed to avoid database +//! Additionally, the `BalancesCache` struct is defined; a cache designed to avoid database //! reads when fork choice requires the validator balances of the justified state. use crate::{metrics, BeaconSnapshot}; +use derivative::Derivative; use fork_choice::ForkChoiceStore; use ssz_derive::{Decode, Encode}; use std::marker::PhantomData; use std::sync::Arc; use store::{Error as StoreError, HotColdDB, ItemStore}; +use superstruct::superstruct; use types::{BeaconBlock, BeaconState, BeaconStateError, Checkpoint, EthSpec, Hash256, Slot}; #[derive(Debug)] @@ -68,7 +70,7 @@ struct CacheItem { /// /// It is effectively a mapping of `epoch_boundary_block_root -> state.balances`. #[derive(PartialEq, Clone, Default, Debug, Encode, Decode)] -struct BalancesCache { +pub struct BalancesCache { items: Vec, } @@ -154,8 +156,10 @@ impl BalancesCache { /// Implements `fork_choice::ForkChoiceStore` in order to provide a persistent backing to the /// `fork_choice::ForkChoice` struct. -#[derive(Debug)] +#[derive(Debug, Derivative)] +#[derivative(PartialEq(bound = "E: EthSpec, Hot: ItemStore, Cold: ItemStore"))] pub struct BeaconForkChoiceStore, Cold: ItemStore> { + #[derivative(PartialEq = "ignore")] store: Arc>, balances_cache: BalancesCache, time: Slot, @@ -163,26 +167,10 @@ pub struct BeaconForkChoiceStore, Cold: ItemStore< justified_checkpoint: Checkpoint, justified_balances: Vec, best_justified_checkpoint: Checkpoint, + proposer_boost_root: Hash256, _phantom: PhantomData, } -impl PartialEq for BeaconForkChoiceStore -where - E: EthSpec, - Hot: ItemStore, - Cold: ItemStore, -{ - /// This implementation ignores the `store` and `slot_clock`. - fn eq(&self, other: &Self) -> bool { - self.balances_cache == other.balances_cache - && self.time == other.time - && self.finalized_checkpoint == other.finalized_checkpoint - && self.justified_checkpoint == other.justified_checkpoint - && self.justified_balances == other.justified_balances - && self.best_justified_checkpoint == other.best_justified_checkpoint - } -} - impl BeaconForkChoiceStore where E: EthSpec, @@ -225,6 +213,7 @@ where justified_balances: anchor_state.balances().clone().into(), finalized_checkpoint, best_justified_checkpoint: justified_checkpoint, + proposer_boost_root: Hash256::zero(), _phantom: PhantomData, } } @@ -239,6 +228,7 @@ where justified_checkpoint: self.justified_checkpoint, justified_balances: self.justified_balances.clone(), best_justified_checkpoint: self.best_justified_checkpoint, + proposer_boost_root: self.proposer_boost_root, } } @@ -255,6 +245,7 @@ where justified_checkpoint: persisted.justified_checkpoint, justified_balances: persisted.justified_balances, best_justified_checkpoint: persisted.best_justified_checkpoint, + proposer_boost_root: persisted.proposer_boost_root, _phantom: PhantomData, }) } @@ -301,6 +292,10 @@ where &self.finalized_checkpoint } + fn proposer_boost_root(&self) -> Hash256 { + self.proposer_boost_root + } + fn set_finalized_checkpoint(&mut self, checkpoint: Checkpoint) { self.finalized_checkpoint = checkpoint } @@ -336,15 +331,23 @@ where fn set_best_justified_checkpoint(&mut self, checkpoint: Checkpoint) { self.best_justified_checkpoint = checkpoint } + + fn set_proposer_boost_root(&mut self, proposer_boost_root: Hash256) { + self.proposer_boost_root = proposer_boost_root; + } } /// A container which allows persisting the `BeaconForkChoiceStore` to the on-disk database. -#[derive(Encode, Decode)] +#[superstruct(variants(V1, V7), variant_attributes(derive(Encode, Decode)), no_enum)] pub struct PersistedForkChoiceStore { - balances_cache: BalancesCache, - time: Slot, - finalized_checkpoint: Checkpoint, - justified_checkpoint: Checkpoint, - justified_balances: Vec, - best_justified_checkpoint: Checkpoint, + pub balances_cache: BalancesCache, + pub time: Slot, + pub finalized_checkpoint: Checkpoint, + pub justified_checkpoint: Checkpoint, + pub justified_balances: Vec, + pub best_justified_checkpoint: Checkpoint, + #[superstruct(only(V7))] + pub proposer_boost_root: Hash256, } + +pub type PersistedForkChoiceStore = PersistedForkChoiceStoreV7; diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index 48e3ff6a4..54397a7d5 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -591,7 +591,7 @@ where }; let initial_head_block_root = fork_choice - .get_head(current_slot) + .get_head(current_slot, &self.spec) .map_err(|e| format!("Unable to get fork choice head: {:?}", e))?; // Try to decode the head block according to the current fork, if that fails, try diff --git a/beacon_node/beacon_chain/src/fork_revert.rs b/beacon_node/beacon_chain/src/fork_revert.rs index 610e27eb9..880eb8e67 100644 --- a/beacon_node/beacon_chain/src/fork_revert.rs +++ b/beacon_node/beacon_chain/src/fork_revert.rs @@ -5,6 +5,7 @@ use slog::{info, warn, Logger}; use state_processing::state_advance::complete_state_advance; use state_processing::{per_block_processing, per_block_processing::BlockSignatureStrategy}; use std::sync::Arc; +use std::time::Duration; use store::{iter::ParentRootBlockIterator, HotColdDB, ItemStore}; use types::{BeaconState, ChainSpec, EthSpec, ForkName, Hash256, SignedBeaconBlock, Slot}; @@ -176,6 +177,8 @@ pub fn reset_fork_choice_to_finalization, Cold: It block.slot(), &block, block.canonical_root(), + // Reward proposer boost. We are reinforcing the canonical chain. + Duration::from_secs(0), &state, payload_verification_status, spec, diff --git a/beacon_node/beacon_chain/src/persisted_fork_choice.rs b/beacon_node/beacon_chain/src/persisted_fork_choice.rs index ed84b7fc2..666ae6e85 100644 --- a/beacon_node/beacon_chain/src/persisted_fork_choice.rs +++ b/beacon_node/beacon_chain/src/persisted_fork_choice.rs @@ -1,25 +1,38 @@ -use crate::beacon_fork_choice_store::PersistedForkChoiceStore as ForkChoiceStore; -use fork_choice::PersistedForkChoice as ForkChoice; +use crate::beacon_fork_choice_store::{PersistedForkChoiceStoreV1, PersistedForkChoiceStoreV7}; use ssz::{Decode, Encode}; use ssz_derive::{Decode, Encode}; use store::{DBColumn, Error, StoreItem}; +use superstruct::superstruct; -#[derive(Encode, Decode)] +// If adding a new version you should update this type alias and fix the breakages. +pub type PersistedForkChoice = PersistedForkChoiceV7; + +#[superstruct(variants(V1, V7), variant_attributes(derive(Encode, Decode)), no_enum)] pub struct PersistedForkChoice { - pub fork_choice: ForkChoice, - pub fork_choice_store: ForkChoiceStore, + pub fork_choice: fork_choice::PersistedForkChoice, + #[superstruct(only(V1))] + pub fork_choice_store: PersistedForkChoiceStoreV1, + #[superstruct(only(V7))] + pub fork_choice_store: PersistedForkChoiceStoreV7, } -impl StoreItem for PersistedForkChoice { - fn db_column() -> DBColumn { - DBColumn::ForkChoice - } +macro_rules! impl_store_item { + ($type:ty) => { + impl StoreItem for $type { + fn db_column() -> DBColumn { + DBColumn::ForkChoice + } - fn as_store_bytes(&self) -> Vec { - self.as_ssz_bytes() - } + fn as_store_bytes(&self) -> Vec { + self.as_ssz_bytes() + } - fn from_store_bytes(bytes: &[u8]) -> std::result::Result { - Self::from_ssz_bytes(bytes).map_err(Into::into) - } + fn from_store_bytes(bytes: &[u8]) -> std::result::Result { + Self::from_ssz_bytes(bytes).map_err(Into::into) + } + } + }; } + +impl_store_item!(PersistedForkChoiceV1); +impl_store_item!(PersistedForkChoiceV7); diff --git a/beacon_node/beacon_chain/src/schema_change.rs b/beacon_node/beacon_chain/src/schema_change.rs index 45f973147..c0ab245df 100644 --- a/beacon_node/beacon_chain/src/schema_change.rs +++ b/beacon_node/beacon_chain/src/schema_change.rs @@ -1,9 +1,14 @@ //! Utilities for managing database schema changes. +mod migration_schema_v6; +mod migration_schema_v7; +mod types; + use crate::beacon_chain::{BeaconChainTypes, FORK_CHOICE_DB_KEY, OP_POOL_DB_KEY}; -use crate::persisted_fork_choice::PersistedForkChoice; +use crate::persisted_fork_choice::{PersistedForkChoiceV1, PersistedForkChoiceV7}; +use crate::store::{get_key_for_col, KeyValueStoreOp}; use crate::validator_pubkey_cache::ValidatorPubkeyCache; use operation_pool::{PersistedOperationPool, PersistedOperationPoolBase}; -use proto_array::ProtoArrayForkChoice; +use slog::{warn, Logger}; use ssz::{Decode, Encode}; use ssz_derive::{Decode, Encode}; use std::fs; @@ -22,6 +27,7 @@ pub fn migrate_schema( datadir: &Path, from: SchemaVersion, to: SchemaVersion, + log: Logger, ) -> Result<(), StoreError> { match (from, to) { // Migrating from the current schema version to iself is always OK, a no-op. @@ -29,8 +35,8 @@ pub fn migrate_schema( // Migrate across multiple versions by recursively migrating one step at a time. (_, _) if from.as_u64() + 1 < to.as_u64() => { let next = SchemaVersion(from.as_u64() + 1); - migrate_schema::(db.clone(), datadir, from, next)?; - migrate_schema::(db, datadir, next, to) + migrate_schema::(db.clone(), datadir, from, next, log.clone())?; + migrate_schema::(db, datadir, next, to, log) } // Migration from v0.3.0 to v0.3.x, adding the temporary states column. // Nothing actually needs to be done, but once a DB uses v2 it shouldn't go back. @@ -95,25 +101,77 @@ pub fn migrate_schema( Ok(()) } - // Migration for adding `is_merge_complete` field to the fork choice store. + // Migration for adding `execution_status` field to the fork choice store. (SchemaVersion(5), SchemaVersion(6)) => { - let fork_choice_opt = db - .get_item::(&FORK_CHOICE_DB_KEY)? - .map(|mut persisted_fork_choice| { - let fork_choice = ProtoArrayForkChoice::from_bytes_legacy( - &persisted_fork_choice.fork_choice.proto_array_bytes, - )?; - persisted_fork_choice.fork_choice.proto_array_bytes = fork_choice.as_bytes(); - Ok::<_, String>(persisted_fork_choice) - }) - .transpose() - .map_err(StoreError::SchemaMigrationError)?; - if let Some(fork_choice) = fork_choice_opt { - // Store the converted fork choice store under the same key. - db.put_item::(&FORK_CHOICE_DB_KEY, &fork_choice)?; + // Database operations to be done atomically + let mut ops = vec![]; + + // The top-level `PersistedForkChoice` struct is still V1 but will have its internal + // bytes for the fork choice updated to V6. + let fork_choice_opt = db.get_item::(&FORK_CHOICE_DB_KEY)?; + if let Some(mut persisted_fork_choice) = fork_choice_opt { + migration_schema_v6::update_execution_statuses::(&mut persisted_fork_choice) + .map_err(StoreError::SchemaMigrationError)?; + + let column = PersistedForkChoiceV1::db_column().into(); + let key = FORK_CHOICE_DB_KEY.as_bytes(); + let db_key = get_key_for_col(column, key); + let op = + KeyValueStoreOp::PutKeyValue(db_key, persisted_fork_choice.as_store_bytes()); + ops.push(op); } - db.store_schema_version(to)?; + db.store_schema_version_atomically(to, ops)?; + + Ok(()) + } + // 1. Add `proposer_boost_root`. + // 2. Update `justified_epoch` to `justified_checkpoint` and `finalized_epoch` to + // `finalized_checkpoint`. + // 3. This migration also includes a potential update to the justified + // checkpoint in case the fork choice store's justified checkpoint and finalized checkpoint + // combination does not actually exist for any blocks in fork choice. This was possible in + // the consensus spec prior to v1.1.6. + // + // Relevant issues: + // + // https://github.com/sigp/lighthouse/issues/2741 + // https://github.com/ethereum/consensus-specs/pull/2727 + // https://github.com/ethereum/consensus-specs/pull/2730 + (SchemaVersion(6), SchemaVersion(7)) => { + // Database operations to be done atomically + let mut ops = vec![]; + + let fork_choice_opt = db.get_item::(&FORK_CHOICE_DB_KEY)?; + if let Some(persisted_fork_choice_v1) = fork_choice_opt { + // This migrates the `PersistedForkChoiceStore`, adding the `proposer_boost_root` field. + let mut persisted_fork_choice_v7 = persisted_fork_choice_v1.into(); + + let result = migration_schema_v7::update_fork_choice::( + &mut persisted_fork_choice_v7, + db.clone(), + ); + + // Fall back to re-initializing fork choice from an anchor state if necessary. + if let Err(e) = result { + warn!(log, "Unable to migrate to database schema 7, re-initializing fork choice"; "error" => ?e); + migration_schema_v7::update_with_reinitialized_fork_choice::( + &mut persisted_fork_choice_v7, + db.clone(), + ) + .map_err(StoreError::SchemaMigrationError)?; + } + + // Store the converted fork choice store under the same key. + let column = PersistedForkChoiceV7::db_column().into(); + let key = FORK_CHOICE_DB_KEY.as_bytes(); + let db_key = get_key_for_col(column, key); + let op = + KeyValueStoreOp::PutKeyValue(db_key, persisted_fork_choice_v7.as_store_bytes()); + ops.push(op); + } + + db.store_schema_version_atomically(to, ops)?; Ok(()) } diff --git a/beacon_node/beacon_chain/src/schema_change/README.md b/beacon_node/beacon_chain/src/schema_change/README.md new file mode 100644 index 000000000..1a33b3c12 --- /dev/null +++ b/beacon_node/beacon_chain/src/schema_change/README.md @@ -0,0 +1,74 @@ +Database Schema Migrations +==== + +This document is an attempt to record some best practices and design conventions for applying +database schema migrations within Lighthouse. + +## General Structure + +If you make a breaking change to an on-disk data structure you need to increment the +`SCHEMA_VERSION` in `beacon_node/store/src/metadata.rs` and add a migration from the previous +version to the new version. + +The entry-point for database migrations is in `schema_change.rs`, _not_ `migrate.rs` (which deals +with finalization). Supporting code for a specific migration may be added in +`schema_change/migration_schema_vX.rs`, where `X` is the version being migrated _to_. + +## Combining Schema Changes + +Schema changes may be combined if they are part of the same pull request to +`unstable`. Once a schema version is defined in `unstable` we should not apply changes to it +without incrementing the version. This prevents conflicts between versions that appear to be the +same. This allows us to deploy `unstable` to nodes without having to worry about needing to resync +because of a sneaky schema change. + +Changing the on-disk structure for a version _before_ it is merged to `unstable` is OK. You will +just have to handle manually resyncing any test nodes (use checkpoint sync). + +## Naming Conventions + +Prefer to name versions of structs by _the version at which the change was introduced_. For example +if you add a field to `Foo` in v9, call the previous version `FooV1` (assuming this is `Foo`'s first +migration) and write a schema change that migrates from `FooV1` to `FooV9`. + +Prefer to use explicit version names in `schema_change.rs` and the `schema_change` module. To +interface with the outside either: + +1. Define a type alias to the latest version, e.g. `pub type Foo = FooV9`, or +2. Define a mapping from the latest version to the version used elsewhere, e.g. + ```rust + impl From for Foo {} + ``` + +Avoid names like: + +* `LegacyFoo` +* `OldFoo` +* `FooWithoutX` + +## First-version vs Last-version + +Previously the schema migration code would name types by the _last_ version at which they were +valid. For example if `Foo` changed in `V9` then we would name the two variants `FooV8` and `FooV9`. +The problem with this scheme is that if `Foo` changes again in the future at say v12 then `FooV9` would +need to be renamed to `FooV11`, which is annoying. Using the _first_ valid version as described +above does not have this issue. + +## Using SuperStruct + +If possible, consider using [`superstruct`](https://crates.io/crates/superstruct) to handle data +structure changes between versions. + +* Use `superstruct(no_enum)` to avoid generating an unnecessary top-level enum. + +## Example + +A field is added to `Foo` in v9, and there are two variants: `FooV1` and `FooV9`. There is a +migration from `FooV1` to `FooV9`. `Foo` is aliased to `FooV9`. + +Some time later another field is added to `Foo` in v12. A new `FooV12` is created, along with a +migration from `FooV9` to `FooV12`. The primary `Foo` type gets re-aliased to `FooV12`. The previous +migration from V1 to V9 shouldn't break because the schema migration refers to `FooV9` explicitly +rather than `Foo`. Due to the re-aliasing (or re-mapping) the compiler will check every usage +of `Foo` to make sure that it still makes sense with `FooV12`. + diff --git a/beacon_node/beacon_chain/src/schema_change/migration_schema_v6.rs b/beacon_node/beacon_chain/src/schema_change/migration_schema_v6.rs new file mode 100644 index 000000000..231da838c --- /dev/null +++ b/beacon_node/beacon_chain/src/schema_change/migration_schema_v6.rs @@ -0,0 +1,28 @@ +///! These functions and structs are only relevant to the database migration from schema 5 to 6. +use crate::persisted_fork_choice::PersistedForkChoiceV1; +use crate::schema_change::types::{SszContainerV1, SszContainerV6}; +use crate::BeaconChainTypes; +use ssz::four_byte_option_impl; +use ssz::{Decode, Encode}; + +// Define a "legacy" implementation of `Option` which uses four bytes for encoding the union +// selector. +four_byte_option_impl!(four_byte_option_usize, usize); + +pub(crate) fn update_execution_statuses( + persisted_fork_choice: &mut PersistedForkChoiceV1, +) -> Result<(), String> { + let ssz_container_v1 = + SszContainerV1::from_ssz_bytes(&persisted_fork_choice.fork_choice.proto_array_bytes) + .map_err(|e| { + format!( + "Failed to decode ProtoArrayForkChoice during schema migration: {:?}", + e + ) + })?; + + let ssz_container_v6: SszContainerV6 = ssz_container_v1.into(); + + persisted_fork_choice.fork_choice.proto_array_bytes = ssz_container_v6.as_ssz_bytes(); + Ok(()) +} diff --git a/beacon_node/beacon_chain/src/schema_change/migration_schema_v7.rs b/beacon_node/beacon_chain/src/schema_change/migration_schema_v7.rs new file mode 100644 index 000000000..a40b33412 --- /dev/null +++ b/beacon_node/beacon_chain/src/schema_change/migration_schema_v7.rs @@ -0,0 +1,327 @@ +///! These functions and structs are only relevant to the database migration from schema 6 to 7. +use crate::beacon_chain::BeaconChainTypes; +use crate::beacon_fork_choice_store::{PersistedForkChoiceStoreV1, PersistedForkChoiceStoreV7}; +use crate::persisted_fork_choice::{PersistedForkChoiceV1, PersistedForkChoiceV7}; +use crate::schema_change::types::{ProtoNodeV6, SszContainerV6, SszContainerV7}; +use crate::types::{Checkpoint, Epoch, Hash256}; +use crate::types::{EthSpec, Slot}; +use crate::{BeaconForkChoiceStore, BeaconSnapshot}; +use fork_choice::ForkChoice; +use proto_array::{core::ProtoNode, core::SszContainer, ProtoArrayForkChoice}; +use ssz::four_byte_option_impl; +use ssz::{Decode, Encode}; +use std::collections::{HashMap, HashSet}; +use std::sync::Arc; +use store::hot_cold_store::HotColdDB; +use store::iter::BlockRootsIterator; +use store::Error as StoreError; + +// Define a "legacy" implementation of `Option` which uses four bytes for encoding the union +// selector. +four_byte_option_impl!(four_byte_option_usize, usize); + +/// This method is used to re-initialize fork choice from the finalized state in case we hit an +/// error during this migration. +pub(crate) fn update_with_reinitialized_fork_choice( + persisted_fork_choice: &mut PersistedForkChoiceV7, + db: Arc>, +) -> Result<(), String> { + let anchor_block_root = persisted_fork_choice + .fork_choice_store + .finalized_checkpoint + .root; + let anchor_block = db + .get_block(&anchor_block_root) + .map_err(|e| format!("{:?}", e))? + .ok_or_else(|| "Missing anchor beacon block".to_string())?; + let anchor_state = db + .get_state(&anchor_block.state_root(), Some(anchor_block.slot())) + .map_err(|e| format!("{:?}", e))? + .ok_or_else(|| "Missing anchor beacon state".to_string())?; + let snapshot = BeaconSnapshot { + beacon_block: anchor_block, + beacon_block_root: anchor_block_root, + beacon_state: anchor_state, + }; + let store = BeaconForkChoiceStore::get_forkchoice_store(db, &snapshot); + let fork_choice = ForkChoice::from_anchor( + store, + anchor_block_root, + &snapshot.beacon_block, + &snapshot.beacon_state, + ) + .map_err(|e| format!("{:?}", e))?; + persisted_fork_choice.fork_choice = fork_choice.to_persisted(); + Ok(()) +} + +pub(crate) fn update_fork_choice( + persisted_fork_choice: &mut PersistedForkChoiceV7, + db: Arc>, +) -> Result<(), StoreError> { + // `PersistedForkChoice` stores the `ProtoArray` as a `Vec`. Deserialize these + // bytes assuming the legacy struct, and transform them to the new struct before + // re-serializing. + let ssz_container_v6 = + SszContainerV6::from_ssz_bytes(&persisted_fork_choice.fork_choice.proto_array_bytes) + .map_err(|e| { + StoreError::SchemaMigrationError(format!( + "Failed to decode ProtoArrayForkChoice during schema migration: {:?}", + e + )) + })?; + + // Clone the V6 proto nodes in order to maintain information about `node.justified_epoch` + // and `node.finalized_epoch`. + let nodes_v6 = ssz_container_v6.nodes.clone(); + + let justified_checkpoint = persisted_fork_choice.fork_choice_store.justified_checkpoint; + let finalized_checkpoint = persisted_fork_choice.fork_choice_store.finalized_checkpoint; + + // These transformations instantiate `node.justified_checkpoint` and `node.finalized_checkpoint` + // to `None`. + let ssz_container_v7: SszContainerV7 = + ssz_container_v6.into_ssz_container_v7(justified_checkpoint, finalized_checkpoint); + let ssz_container: SszContainer = ssz_container_v7.into(); + let mut fork_choice: ProtoArrayForkChoice = ssz_container.into(); + + update_checkpoints::(finalized_checkpoint.root, &nodes_v6, &mut fork_choice, db) + .map_err(StoreError::SchemaMigrationError)?; + + // Update the justified checkpoint in the store in case we have a discrepancy + // between the store and the proto array nodes. + update_store_justified_checkpoint(persisted_fork_choice, &mut fork_choice) + .map_err(StoreError::SchemaMigrationError)?; + + Ok(()) +} + +struct HeadInfo { + index: usize, + root: Hash256, + slot: Slot, +} + +fn update_checkpoints( + finalized_root: Hash256, + nodes_v6: &[ProtoNodeV6], + fork_choice: &mut ProtoArrayForkChoice, + db: Arc>, +) -> Result<(), String> { + let heads = find_finalized_descendant_heads(finalized_root, fork_choice); + + // For each head, first gather all epochs we will need to find justified or finalized roots for. + for head in heads { + // `relevant_epochs` are epochs for which we will need to find the root at the start slot. + // We don't need to worry about whether the are finalized or justified epochs. + let mut relevant_epochs = HashSet::new(); + let relevant_epoch_finder = |index, _: &mut ProtoNode| { + let (justified_epoch, finalized_epoch) = nodes_v6 + .get(index) + .map(|node: &ProtoNodeV6| (node.justified_epoch, node.finalized_epoch)) + .ok_or_else(|| "Index not found in legacy proto nodes".to_string())?; + relevant_epochs.insert(justified_epoch); + relevant_epochs.insert(finalized_epoch); + Ok(()) + }; + + apply_to_chain_of_ancestors( + finalized_root, + head.index, + fork_choice, + relevant_epoch_finder, + )?; + + // find the block roots associated with each relevant epoch. + let roots_by_epoch = + map_relevant_epochs_to_roots::(head.root, head.slot, relevant_epochs, db.clone())?; + + // Apply this mutator to the chain of descendants from this head, adding justified + // and finalized checkpoints for each. + let node_mutator = |index, node: &mut ProtoNode| { + let (justified_epoch, finalized_epoch) = nodes_v6 + .get(index) + .map(|node: &ProtoNodeV6| (node.justified_epoch, node.finalized_epoch)) + .ok_or_else(|| "Index not found in legacy proto nodes".to_string())?; + + // Update the checkpoints only if they haven't already been populated. + if node.justified_checkpoint.is_none() { + let justified_checkpoint = + roots_by_epoch + .get(&justified_epoch) + .map(|&root| Checkpoint { + epoch: justified_epoch, + root, + }); + node.justified_checkpoint = justified_checkpoint; + } + if node.finalized_checkpoint.is_none() { + let finalized_checkpoint = + roots_by_epoch + .get(&finalized_epoch) + .map(|&root| Checkpoint { + epoch: finalized_epoch, + root, + }); + node.finalized_checkpoint = finalized_checkpoint; + } + + Ok(()) + }; + + apply_to_chain_of_ancestors(finalized_root, head.index, fork_choice, node_mutator)?; + } + Ok(()) +} + +/// Coverts the given `HashSet` to a `Vec` then reverse sorts by `Epoch`. Next, a +/// single `BlockRootsIterator` is created which is used to iterate backwards from the given +/// `head_root` and `head_slot`, finding the block root at the start slot of each epoch. +fn map_relevant_epochs_to_roots( + head_root: Hash256, + head_slot: Slot, + epochs: HashSet, + db: Arc>, +) -> Result, String> { + // Convert the `HashSet` to a `Vec` and reverse sort the epochs. + let mut relevant_epochs = epochs.into_iter().collect::>(); + relevant_epochs.sort_unstable_by(|a, b| b.cmp(a)); + + // Iterate backwards from the given `head_root` and `head_slot` and find the block root at each epoch. + let mut iter = std::iter::once(Ok((head_root, head_slot))) + .chain(BlockRootsIterator::from_block(db, head_root).map_err(|e| format!("{:?}", e))?); + let mut roots_by_epoch = HashMap::new(); + for epoch in relevant_epochs { + let start_slot = epoch.start_slot(T::EthSpec::slots_per_epoch()); + + let root = iter + .find_map(|next| match next { + Ok((root, slot)) => (slot == start_slot).then(|| Ok(root)), + Err(e) => Some(Err(format!("{:?}", e))), + }) + .transpose()? + .ok_or_else(|| "Justified root not found".to_string())?; + roots_by_epoch.insert(epoch, root); + } + Ok(roots_by_epoch) +} + +/// Applies a mutator to every node in a chain, starting from the node at the given +/// `head_index` and iterating through ancestors until the `finalized_root` is reached. +fn apply_to_chain_of_ancestors( + finalized_root: Hash256, + head_index: usize, + fork_choice: &mut ProtoArrayForkChoice, + mut node_mutator: F, +) -> Result<(), String> +where + F: FnMut(usize, &mut ProtoNode) -> Result<(), String>, +{ + let head = fork_choice + .core_proto_array_mut() + .nodes + .get_mut(head_index) + .ok_or_else(|| "Head index not found in proto nodes".to_string())?; + + node_mutator(head_index, head)?; + + let mut parent_index_opt = head.parent; + let mut parent_opt = + parent_index_opt.and_then(|index| fork_choice.core_proto_array_mut().nodes.get_mut(index)); + + // Iterate backwards through all parents until there is no reference to a parent or we reach + // the `finalized_root` node. + while let (Some(parent), Some(parent_index)) = (parent_opt, parent_index_opt) { + node_mutator(parent_index, parent)?; + + // Break out of this while loop *after* the `node_mutator` has been applied to the finalized + // node. + if parent.root == finalized_root { + break; + } + + // Update parent values + parent_index_opt = parent.parent; + parent_opt = parent_index_opt + .and_then(|index| fork_choice.core_proto_array_mut().nodes.get_mut(index)); + } + Ok(()) +} + +/// Finds all heads by finding all nodes in the proto array that are not referenced as parents. Then +/// checks that these nodes are descendants of the finalized root in order to determine if they are +/// relevant. +fn find_finalized_descendant_heads( + finalized_root: Hash256, + fork_choice: &ProtoArrayForkChoice, +) -> Vec { + let nodes_referenced_as_parents: HashSet = fork_choice + .core_proto_array() + .nodes + .iter() + .filter_map(|node| node.parent) + .collect::>(); + + fork_choice + .core_proto_array() + .nodes + .iter() + .enumerate() + .filter_map(|(index, node)| { + (!nodes_referenced_as_parents.contains(&index) + && fork_choice.is_descendant(finalized_root, node.root)) + .then(|| HeadInfo { + index, + root: node.root, + slot: node.slot, + }) + }) + .collect::>() +} + +fn update_store_justified_checkpoint( + persisted_fork_choice: &mut PersistedForkChoiceV7, + fork_choice: &mut ProtoArrayForkChoice, +) -> Result<(), String> { + let justified_checkpoint = fork_choice + .core_proto_array() + .nodes + .iter() + .filter_map(|node| { + (node.finalized_checkpoint + == Some(persisted_fork_choice.fork_choice_store.finalized_checkpoint)) + .then(|| node.justified_checkpoint) + .flatten() + }) + .max_by_key(|justified_checkpoint| justified_checkpoint.epoch) + .ok_or("Proto node with current finalized checkpoint not found")?; + + fork_choice.core_proto_array_mut().justified_checkpoint = justified_checkpoint; + persisted_fork_choice.fork_choice.proto_array_bytes = fork_choice.as_bytes(); + persisted_fork_choice.fork_choice_store.justified_checkpoint = justified_checkpoint; + Ok(()) +} + +// Add a zero `proposer_boost_root` when migrating from V1-6 to V7. +impl From for PersistedForkChoiceStoreV7 { + fn from(other: PersistedForkChoiceStoreV1) -> Self { + Self { + balances_cache: other.balances_cache, + time: other.time, + finalized_checkpoint: other.finalized_checkpoint, + justified_checkpoint: other.justified_checkpoint, + justified_balances: other.justified_balances, + best_justified_checkpoint: other.best_justified_checkpoint, + proposer_boost_root: Hash256::zero(), + } + } +} + +impl From for PersistedForkChoiceV7 { + fn from(other: PersistedForkChoiceV1) -> Self { + Self { + fork_choice: other.fork_choice, + fork_choice_store: other.fork_choice_store.into(), + } + } +} diff --git a/beacon_node/beacon_chain/src/schema_change/types.rs b/beacon_node/beacon_chain/src/schema_change/types.rs new file mode 100644 index 000000000..8d41a384f --- /dev/null +++ b/beacon_node/beacon_chain/src/schema_change/types.rs @@ -0,0 +1,192 @@ +use crate::types::{AttestationShufflingId, Checkpoint, Epoch, Hash256, Slot}; +use proto_array::core::{ProposerBoost, ProtoNode, SszContainer, VoteTracker}; +use proto_array::ExecutionStatus; +use ssz::four_byte_option_impl; +use ssz::Encode; +use ssz_derive::{Decode, Encode}; +use superstruct::superstruct; + +// Define a "legacy" implementation of `Option` which uses four bytes for encoding the union +// selector. +four_byte_option_impl!(four_byte_option_usize, usize); +four_byte_option_impl!(four_byte_option_checkpoint, Checkpoint); + +#[superstruct( + variants(V1, V6, V7), + variant_attributes(derive(Clone, PartialEq, Debug, Encode, Decode)), + no_enum +)] +pub struct ProtoNode { + pub slot: Slot, + pub state_root: Hash256, + pub target_root: Hash256, + pub current_epoch_shuffling_id: AttestationShufflingId, + pub next_epoch_shuffling_id: AttestationShufflingId, + pub root: Hash256, + #[ssz(with = "four_byte_option_usize")] + pub parent: Option, + #[superstruct(only(V1, V6))] + pub justified_epoch: Epoch, + #[superstruct(only(V1, V6))] + pub finalized_epoch: Epoch, + #[ssz(with = "four_byte_option_checkpoint")] + #[superstruct(only(V7))] + pub justified_checkpoint: Option, + #[ssz(with = "four_byte_option_checkpoint")] + #[superstruct(only(V7))] + pub finalized_checkpoint: Option, + pub weight: u64, + #[ssz(with = "four_byte_option_usize")] + pub best_child: Option, + #[ssz(with = "four_byte_option_usize")] + pub best_descendant: Option, + #[superstruct(only(V6, V7))] + pub execution_status: ExecutionStatus, +} + +impl Into for ProtoNodeV1 { + fn into(self) -> ProtoNodeV6 { + ProtoNodeV6 { + slot: self.slot, + state_root: self.state_root, + target_root: self.target_root, + current_epoch_shuffling_id: self.current_epoch_shuffling_id, + next_epoch_shuffling_id: self.next_epoch_shuffling_id, + root: self.root, + parent: self.parent, + justified_epoch: self.justified_epoch, + finalized_epoch: self.finalized_epoch, + weight: self.weight, + best_child: self.best_child, + best_descendant: self.best_descendant, + // We set the following execution value as if the block is a pre-merge-fork block. This + // is safe as long as we never import a merge block with the old version of proto-array. + // This will be safe since we can't actually process merge blocks until we've made this + // change to fork choice. + execution_status: ExecutionStatus::irrelevant(), + } + } +} + +impl Into for ProtoNodeV6 { + fn into(self) -> ProtoNodeV7 { + ProtoNodeV7 { + slot: self.slot, + state_root: self.state_root, + target_root: self.target_root, + current_epoch_shuffling_id: self.current_epoch_shuffling_id, + next_epoch_shuffling_id: self.next_epoch_shuffling_id, + root: self.root, + parent: self.parent, + justified_checkpoint: None, + finalized_checkpoint: None, + weight: self.weight, + best_child: self.best_child, + best_descendant: self.best_descendant, + execution_status: self.execution_status, + } + } +} + +impl Into for ProtoNodeV7 { + fn into(self) -> ProtoNode { + ProtoNode { + slot: self.slot, + state_root: self.state_root, + target_root: self.target_root, + current_epoch_shuffling_id: self.current_epoch_shuffling_id, + next_epoch_shuffling_id: self.next_epoch_shuffling_id, + root: self.root, + parent: self.parent, + justified_checkpoint: self.justified_checkpoint, + finalized_checkpoint: self.finalized_checkpoint, + weight: self.weight, + best_child: self.best_child, + best_descendant: self.best_descendant, + execution_status: self.execution_status, + } + } +} + +#[superstruct( + variants(V1, V6, V7), + variant_attributes(derive(Encode, Decode)), + no_enum +)] +#[derive(Encode, Decode)] +pub struct SszContainer { + pub votes: Vec, + pub balances: Vec, + pub prune_threshold: usize, + #[superstruct(only(V1, V6))] + pub justified_epoch: Epoch, + #[superstruct(only(V1, V6))] + pub finalized_epoch: Epoch, + #[superstruct(only(V7))] + pub justified_checkpoint: Checkpoint, + #[superstruct(only(V7))] + pub finalized_checkpoint: Checkpoint, + #[superstruct(only(V1))] + pub nodes: Vec, + #[superstruct(only(V6))] + pub nodes: Vec, + #[superstruct(only(V7))] + pub nodes: Vec, + pub indices: Vec<(Hash256, usize)>, + #[superstruct(only(V7))] + pub previous_proposer_boost: ProposerBoost, +} + +impl Into for SszContainerV1 { + fn into(self) -> SszContainerV6 { + let nodes = self.nodes.into_iter().map(Into::into).collect(); + + SszContainerV6 { + votes: self.votes, + balances: self.balances, + prune_threshold: self.prune_threshold, + justified_epoch: self.justified_epoch, + finalized_epoch: self.finalized_epoch, + nodes, + indices: self.indices, + } + } +} + +impl SszContainerV6 { + pub(crate) fn into_ssz_container_v7( + self, + justified_checkpoint: Checkpoint, + finalized_checkpoint: Checkpoint, + ) -> SszContainerV7 { + let nodes = self.nodes.into_iter().map(Into::into).collect(); + + SszContainerV7 { + votes: self.votes, + balances: self.balances, + prune_threshold: self.prune_threshold, + justified_checkpoint, + finalized_checkpoint, + nodes, + indices: self.indices, + previous_proposer_boost: ProposerBoost::default(), + } + } +} + +impl Into for SszContainerV7 { + fn into(self) -> SszContainer { + let nodes = self.nodes.into_iter().map(Into::into).collect(); + + SszContainer { + votes: self.votes, + balances: self.balances, + prune_threshold: self.prune_threshold, + justified_checkpoint: self.justified_checkpoint, + finalized_checkpoint: self.finalized_checkpoint, + nodes, + indices: self.indices, + previous_proposer_boost: self.previous_proposer_boost, + } + } +} diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index f9af16bbe..24aba9e20 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -2223,9 +2223,10 @@ fn assert_chains_pretty_much_the_same(a: &BeaconChain, b ); let slot = a.slot().unwrap(); + let spec = T::EthSpec::default_spec(); assert!( - a.fork_choice.write().get_head(slot).unwrap() - == b.fork_choice.write().get_head(slot).unwrap(), + a.fork_choice.write().get_head(slot, &spec).unwrap() + == b.fork_choice.write().get_head(slot, &spec).unwrap(), "fork_choice heads should be equal" ); } diff --git a/beacon_node/client/src/builder.rs b/beacon_node/client/src/builder.rs index bcf0ee198..30bc34dda 100644 --- a/beacon_node/client/src/builder.rs +++ b/beacon_node/client/src/builder.rs @@ -23,7 +23,7 @@ use monitoring_api::{MonitoringHttpClient, ProcessType}; use network::{NetworkConfig, NetworkMessage, NetworkService}; use slasher::Slasher; use slasher_service::SlasherService; -use slog::{debug, info, warn}; +use slog::{debug, info, warn, Logger}; use std::net::TcpListener; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -748,6 +748,7 @@ where hot_path: &Path, cold_path: &Path, config: StoreConfig, + log: Logger, ) -> Result { let context = self .runtime_context @@ -763,7 +764,7 @@ where self.freezer_db_path = Some(cold_path.into()); let schema_upgrade = |db, from, to| { - migrate_schema::>(db, datadir, from, to) + migrate_schema::>(db, datadir, from, to, log) }; let store = HotColdDB::open( diff --git a/beacon_node/lighthouse_network/Cargo.toml b/beacon_node/lighthouse_network/Cargo.toml index 710a705f0..4945dbfdf 100644 --- a/beacon_node/lighthouse_network/Cargo.toml +++ b/beacon_node/lighthouse_network/Cargo.toml @@ -37,7 +37,7 @@ rand = "0.7.3" directory = { path = "../../common/directory" } regex = "1.3.9" strum = { version = "0.21.0", features = ["derive"] } -superstruct = "0.2.0" +superstruct = "0.3.0" [dependencies.libp2p] version = "0.41.0" diff --git a/beacon_node/src/lib.rs b/beacon_node/src/lib.rs index 4ff474571..773a0d2eb 100644 --- a/beacon_node/src/lib.rs +++ b/beacon_node/src/lib.rs @@ -84,7 +84,13 @@ impl ProductionBeaconNode { .runtime_context(context) .chain_spec(spec) .http_api_config(client_config.http_api.clone()) - .disk_store(&datadir, &db_path, &freezer_db_path, store_config)?; + .disk_store( + &datadir, + &db_path, + &freezer_db_path, + store_config, + log.clone(), + )?; let builder = if let Some(slasher_config) = client_config.slasher.clone() { let slasher = Arc::new( diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index cfa49847d..05a0eb3dd 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -975,6 +975,21 @@ impl, Cold: ItemStore> HotColdDB self.hot_db.put(&SCHEMA_VERSION_KEY, &schema_version) } + /// Store the database schema version atomically with additional operations. + pub fn store_schema_version_atomically( + &self, + schema_version: SchemaVersion, + mut ops: Vec, + ) -> Result<(), Error> { + let column = SchemaVersion::db_column().into(); + let key = SCHEMA_VERSION_KEY.as_bytes(); + let db_key = get_key_for_col(column, key); + let op = KeyValueStoreOp::PutKeyValue(db_key, schema_version.as_store_bytes()); + ops.push(op); + + self.hot_db.do_atomically(ops) + } + /// Initialise the anchor info for checkpoint sync starting from `block`. pub fn init_anchor_info(&self, block: BeaconBlockRef<'_, E>) -> Result { let anchor_slot = block.slot(); diff --git a/beacon_node/store/src/iter.rs b/beacon_node/store/src/iter.rs index 8b11a6cc9..a4d34cd3c 100644 --- a/beacon_node/store/src/iter.rs +++ b/beacon_node/store/src/iter.rs @@ -125,6 +125,15 @@ impl<'a, T: EthSpec, Hot: ItemStore, Cold: ItemStore> BlockRootsIterator<' inner: RootsIterator::owned(store, beacon_state), } } + + pub fn from_block( + store: Arc>, + block_hash: Hash256, + ) -> Result { + Ok(Self { + inner: RootsIterator::from_block(store, block_hash)?, + }) + } } impl<'a, T: EthSpec, Hot: ItemStore, Cold: ItemStore> Iterator diff --git a/beacon_node/store/src/metadata.rs b/beacon_node/store/src/metadata.rs index cc0535ef5..17800bb6c 100644 --- a/beacon_node/store/src/metadata.rs +++ b/beacon_node/store/src/metadata.rs @@ -4,7 +4,7 @@ use ssz::{Decode, Encode}; use ssz_derive::{Decode, Encode}; use types::{Checkpoint, Hash256, Slot}; -pub const CURRENT_SCHEMA_VERSION: SchemaVersion = SchemaVersion(6); +pub const CURRENT_SCHEMA_VERSION: SchemaVersion = SchemaVersion(7); // All the keys that get stored under the `BeaconMeta` column. // diff --git a/common/eth2_network_config/built_in_network_configs/mainnet/config.yaml b/common/eth2_network_config/built_in_network_configs/mainnet/config.yaml index a1d305bac..4d17356ce 100644 --- a/common/eth2_network_config/built_in_network_configs/mainnet/config.yaml +++ b/common/eth2_network_config/built_in_network_configs/mainnet/config.yaml @@ -71,6 +71,12 @@ MIN_PER_EPOCH_CHURN_LIMIT: 4 CHURN_LIMIT_QUOTIENT: 65536 +# Fork choice +# --------------------------------------------------------------- +# TODO: enable once proposer boosting is desired on mainnet +# 70% +# PROPOSER_SCORE_BOOST: 70 + # Deposit contract # --------------------------------------------------------------- # Ethereum PoW Mainnet diff --git a/common/eth2_network_config/built_in_network_configs/prater/config.yaml b/common/eth2_network_config/built_in_network_configs/prater/config.yaml index 5fc23d6af..aa375ab2e 100644 --- a/common/eth2_network_config/built_in_network_configs/prater/config.yaml +++ b/common/eth2_network_config/built_in_network_configs/prater/config.yaml @@ -71,6 +71,11 @@ MIN_PER_EPOCH_CHURN_LIMIT: 4 CHURN_LIMIT_QUOTIENT: 65536 +# Fork choice +# --------------------------------------------------------------- +# 70% +PROPOSER_SCORE_BOOST: 70 + # Deposit contract # --------------------------------------------------------------- # Ethereum Goerli testnet diff --git a/common/eth2_network_config/built_in_network_configs/pyrmont/config.yaml b/common/eth2_network_config/built_in_network_configs/pyrmont/config.yaml index 352a4e918..b5f841580 100644 --- a/common/eth2_network_config/built_in_network_configs/pyrmont/config.yaml +++ b/common/eth2_network_config/built_in_network_configs/pyrmont/config.yaml @@ -71,6 +71,11 @@ MIN_PER_EPOCH_CHURN_LIMIT: 4 CHURN_LIMIT_QUOTIENT: 65536 +# Fork choice +# --------------------------------------------------------------- +# 70% +PROPOSER_SCORE_BOOST: 70 + # Deposit contract # --------------------------------------------------------------- # Ethereum Goerli testnet diff --git a/common/slot_clock/src/lib.rs b/common/slot_clock/src/lib.rs index 9fa24a022..f50931c6f 100644 --- a/common/slot_clock/src/lib.rs +++ b/common/slot_clock/src/lib.rs @@ -11,6 +11,7 @@ pub use crate::manual_slot_clock::ManualSlotClock; pub use crate::manual_slot_clock::ManualSlotClock as TestingSlotClock; pub use crate::system_time_slot_clock::SystemTimeSlotClock; pub use metrics::scrape_for_metrics; +use types::consts::merge::INTERVALS_PER_SLOT; pub use types::Slot; /// A clock that reports the current slot. @@ -82,24 +83,33 @@ pub trait SlotClock: Send + Sync + Sized + Clone { /// Returns the delay between the start of the slot and when unaggregated attestations should be /// produced. fn unagg_attestation_production_delay(&self) -> Duration { - self.slot_duration() / 3 + self.slot_duration() / INTERVALS_PER_SLOT as u32 } /// Returns the delay between the start of the slot and when sync committee messages should be /// produced. fn sync_committee_message_production_delay(&self) -> Duration { - self.slot_duration() / 3 + self.slot_duration() / INTERVALS_PER_SLOT as u32 } /// Returns the delay between the start of the slot and when aggregated attestations should be /// produced. fn agg_attestation_production_delay(&self) -> Duration { - self.slot_duration() * 2 / 3 + self.slot_duration() * 2 / INTERVALS_PER_SLOT as u32 } /// Returns the delay between the start of the slot and when partially aggregated `SyncCommitteeContribution` should be /// produced. fn sync_committee_contribution_production_delay(&self) -> Duration { - self.slot_duration() * 2 / 3 + self.slot_duration() * 2 / INTERVALS_PER_SLOT as u32 + } + + /// Returns the `Duration` since the start of the current `Slot`. Useful in determining whether to apply proposer boosts. + fn seconds_from_current_slot_start(&self, seconds_per_slot: u64) -> Option { + self.now_duration() + .and_then(|now| now.checked_sub(self.genesis_duration())) + .map(|duration_into_slot| { + Duration::from_secs(duration_into_slot.as_secs() % seconds_per_slot) + }) } } diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 93ed1c3ba..86b32aab1 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -1,14 +1,14 @@ -use std::marker::PhantomData; - +use crate::ForkChoiceStore; use proto_array::{Block as ProtoBlock, ExecutionStatus, ProtoArrayForkChoice}; use ssz_derive::{Decode, Encode}; -use types::{ - AttestationShufflingId, BeaconBlock, BeaconState, BeaconStateError, ChainSpec, Checkpoint, - Epoch, EthSpec, Hash256, IndexedAttestation, RelativeEpoch, SignedBeaconBlock, Slot, -}; - -use crate::ForkChoiceStore; use std::cmp::Ordering; +use std::marker::PhantomData; +use std::time::Duration; +use types::{ + consts::merge::INTERVALS_PER_SLOT, AttestationShufflingId, BeaconBlock, BeaconState, + BeaconStateError, ChainSpec, Checkpoint, Epoch, EthSpec, Hash256, IndexedAttestation, + RelativeEpoch, SignedBeaconBlock, Slot, +}; #[derive(Debug)] pub enum Error { @@ -168,6 +168,13 @@ where store.set_current_slot(time); let current_slot = store.get_current_slot(); + + // Reset proposer boost if this is a new slot. + if current_slot > previous_slot { + store.set_proposer_boost_root(Hash256::zero()); + } + + // Not a new epoch, return. if !(current_slot > previous_slot && compute_slots_since_epoch_start::(current_slot) == 0) { return Ok(()); } @@ -218,6 +225,15 @@ fn dequeue_attestations( std::mem::replace(queued_attestations, remaining) } +/// Denotes whether an attestation we are processing was received from a block or from gossip. +/// Equivalent to the `is_from_block` `bool` in: +/// +/// https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/fork-choice.md#validate_on_attestation +pub enum AttestationFromBlock { + True, + False, +} + /// Provides an implementation of "Ethereum 2.0 Phase 0 -- Beacon Chain Fork Choice": /// /// https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/fork-choice.md#ethereum-20-phase-0----beacon-chain-fork-choice @@ -292,9 +308,8 @@ where let proto_array = ProtoArrayForkChoice::new( finalized_block_slot, finalized_block_state_root, - fc_store.justified_checkpoint().epoch, - fc_store.finalized_checkpoint().epoch, - fc_store.finalized_checkpoint().root, + *fc_store.justified_checkpoint(), + *fc_store.finalized_checkpoint(), current_epoch_shuffling_id, next_epoch_shuffling_id, execution_status, @@ -377,17 +392,22 @@ where /// Is equivalent to: /// /// https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/fork-choice.md#get_head - pub fn get_head(&mut self, current_slot: Slot) -> Result> { + pub fn get_head( + &mut self, + current_slot: Slot, + spec: &ChainSpec, + ) -> Result> { self.update_time(current_slot)?; let store = &mut self.fc_store; self.proto_array - .find_head( - store.justified_checkpoint().epoch, - store.justified_checkpoint().root, - store.finalized_checkpoint().epoch, + .find_head::( + *store.justified_checkpoint(), + *store.finalized_checkpoint(), store.justified_balances(), + store.proposer_boost_root(), + spec, ) .map_err(Into::into) } @@ -462,11 +482,13 @@ where /// /// The supplied block **must** pass the `state_transition` function as it will not be run /// here. + #[allow(clippy::too_many_arguments)] pub fn on_block( &mut self, current_slot: Slot, block: &BeaconBlock, block_root: Hash256, + block_delay: Duration, state: &BeaconState, payload_verification_status: PayloadVerificationStatus, spec: &ChainSpec, @@ -520,6 +542,13 @@ where })); } + // Add proposer score boost if the block is timely. + let is_before_attesting_interval = + block_delay < Duration::from_secs(spec.seconds_per_slot / INTERVALS_PER_SLOT); + if current_slot == block.slot() && is_before_attesting_interval { + self.fc_store.set_proposer_boost_root(block_root); + } + // Update justified checkpoint. if state.current_justified_checkpoint().epoch > self.fc_store.justified_checkpoint().epoch { if state.current_justified_checkpoint().epoch @@ -539,25 +568,9 @@ where if state.finalized_checkpoint().epoch > self.fc_store.finalized_checkpoint().epoch { self.fc_store .set_finalized_checkpoint(state.finalized_checkpoint()); - let finalized_slot = - compute_start_slot_at_epoch::(self.fc_store.finalized_checkpoint().epoch); - - // Note: the `if` statement here is not part of the specification, but I claim that it - // is an optimization and equivalent to the specification. See this PR for more - // information: - // - // https://github.com/ethereum/eth2.0-specs/pull/1880 - if *self.fc_store.justified_checkpoint() != state.current_justified_checkpoint() - && (state.current_justified_checkpoint().epoch - > self.fc_store.justified_checkpoint().epoch - || self - .get_ancestor(self.fc_store.justified_checkpoint().root, finalized_slot)? - != Some(self.fc_store.finalized_checkpoint().root)) - { - self.fc_store - .set_justified_checkpoint(state.current_justified_checkpoint()) - .map_err(Error::UnableToSetJustifiedCheckpoint)?; - } + self.fc_store + .set_justified_checkpoint(state.current_justified_checkpoint()) + .map_err(Error::UnableToSetJustifiedCheckpoint)?; } let target_slot = block @@ -623,14 +636,43 @@ where ) .map_err(Error::BeaconStateError)?, state_root: block.state_root(), - justified_epoch: state.current_justified_checkpoint().epoch, - finalized_epoch: state.finalized_checkpoint().epoch, + justified_checkpoint: state.current_justified_checkpoint(), + finalized_checkpoint: state.finalized_checkpoint(), execution_status, })?; Ok(()) } + /// Validates the `epoch` against the current time according to the fork choice store. + /// + /// ## Specification + /// + /// Equivalent to: + /// + /// https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/fork-choice.md#validate_target_epoch_against_current_time + fn validate_target_epoch_against_current_time( + &self, + target_epoch: Epoch, + ) -> Result<(), InvalidAttestation> { + let slot_now = self.fc_store.get_current_slot(); + let epoch_now = slot_now.epoch(E::slots_per_epoch()); + + // Attestation must be from the current or previous epoch. + if target_epoch > epoch_now { + return Err(InvalidAttestation::FutureEpoch { + attestation_epoch: target_epoch, + current_epoch: epoch_now, + }); + } else if target_epoch + 1 < epoch_now { + return Err(InvalidAttestation::PastEpoch { + attestation_epoch: target_epoch, + current_epoch: epoch_now, + }); + } + Ok(()) + } + /// Validates the `indexed_attestation` for application to fork choice. /// /// ## Specification @@ -641,6 +683,7 @@ where fn validate_on_attestation( &self, indexed_attestation: &IndexedAttestation, + is_from_block: AttestationFromBlock, ) -> Result<(), InvalidAttestation> { // There is no point in processing an attestation with an empty bitfield. Reject // it immediately. @@ -651,21 +694,10 @@ where return Err(InvalidAttestation::EmptyAggregationBitfield); } - let slot_now = self.fc_store.get_current_slot(); - let epoch_now = slot_now.epoch(E::slots_per_epoch()); let target = indexed_attestation.data.target; - // Attestation must be from the current or previous epoch. - if target.epoch > epoch_now { - return Err(InvalidAttestation::FutureEpoch { - attestation_epoch: target.epoch, - current_epoch: epoch_now, - }); - } else if target.epoch + 1 < epoch_now { - return Err(InvalidAttestation::PastEpoch { - attestation_epoch: target.epoch, - current_epoch: epoch_now, - }); + if matches!(is_from_block, AttestationFromBlock::False) { + self.validate_target_epoch_against_current_time(target.epoch)?; } if target.epoch != indexed_attestation.data.slot.epoch(E::slots_per_epoch()) { @@ -748,6 +780,7 @@ where &mut self, current_slot: Slot, attestation: &IndexedAttestation, + is_from_block: AttestationFromBlock, ) -> Result<(), Error> { // Ensure the store is up-to-date. self.update_time(current_slot)?; @@ -769,7 +802,7 @@ where return Ok(()); } - self.validate_on_attestation(attestation)?; + self.validate_on_attestation(attestation, is_from_block)?; if attestation.data.slot < self.fc_store.get_current_slot() { for validator_index in attestation.attesting_indices.iter() { @@ -895,6 +928,11 @@ where &self.queued_attestations } + /// Returns the store's `proposer_boost_root`. + pub fn proposer_boost_root(&self) -> Hash256 { + self.fc_store.proposer_boost_root() + } + /// Prunes the underlying fork choice DAG. pub fn prune(&mut self) -> Result<(), Error> { let finalized_root = self.fc_store.finalized_checkpoint().root; diff --git a/consensus/fork_choice/src/fork_choice_store.rs b/consensus/fork_choice/src/fork_choice_store.rs index c74610cc0..9b85708f3 100644 --- a/consensus/fork_choice/src/fork_choice_store.rs +++ b/consensus/fork_choice/src/fork_choice_store.rs @@ -19,7 +19,7 @@ use types::{BeaconBlock, BeaconState, Checkpoint, EthSpec, Hash256, Slot}; pub trait ForkChoiceStore: Sized { type Error; - /// Returns the last value passed to `Self::update_time`. + /// Returns the last value passed to `Self::set_current_slot`. fn get_current_slot(&self) -> Slot; /// Set the value to be returned by `Self::get_current_slot`. @@ -50,6 +50,9 @@ pub trait ForkChoiceStore: Sized { /// Returns the `finalized_checkpoint`. fn finalized_checkpoint(&self) -> &Checkpoint; + /// Returns the `proposer_boost_root`. + fn proposer_boost_root(&self) -> Hash256; + /// Sets `finalized_checkpoint`. fn set_finalized_checkpoint(&mut self, checkpoint: Checkpoint); @@ -58,4 +61,7 @@ pub trait ForkChoiceStore: Sized { /// Sets the `best_justified_checkpoint`. fn set_best_justified_checkpoint(&mut self, checkpoint: Checkpoint); + + /// Sets the proposer boost root. + fn set_proposer_boost_root(&mut self, proposer_boost_root: Hash256); } diff --git a/consensus/fork_choice/src/lib.rs b/consensus/fork_choice/src/lib.rs index 7dd80b798..ba031cdf7 100644 --- a/consensus/fork_choice/src/lib.rs +++ b/consensus/fork_choice/src/lib.rs @@ -2,8 +2,8 @@ mod fork_choice; mod fork_choice_store; pub use crate::fork_choice::{ - Error, ForkChoice, InvalidAttestation, InvalidBlock, PayloadVerificationStatus, - PersistedForkChoice, QueuedAttestation, + AttestationFromBlock, Error, ForkChoice, InvalidAttestation, InvalidBlock, + PayloadVerificationStatus, PersistedForkChoice, QueuedAttestation, }; pub use fork_choice_store::ForkChoiceStore; pub use proto_array::Block as ProtoBlock; diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 129b79c39..42b56f6ab 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -2,6 +2,7 @@ use std::fmt; use std::sync::Mutex; +use std::time::Duration; use beacon_chain::test_utils::{ AttestationStrategy, BeaconChainHarness, BlockStrategy, EphemeralHarnessType, @@ -274,6 +275,7 @@ impl ForkChoiceTest { current_slot, &block, block.canonical_root(), + Duration::from_secs(0), &state, PayloadVerificationStatus::Verified, &self.harness.chain.spec, @@ -316,6 +318,7 @@ impl ForkChoiceTest { current_slot, &block, block.canonical_root(), + Duration::from_secs(0), &state, PayloadVerificationStatus::Verified, &self.harness.chain.spec, diff --git a/consensus/proto_array/src/error.rs b/consensus/proto_array/src/error.rs index c3892bde5..adb10c035 100644 --- a/consensus/proto_array/src/error.rs +++ b/consensus/proto_array/src/error.rs @@ -1,4 +1,4 @@ -use types::{Epoch, Hash256}; +use types::{Checkpoint, Epoch, Hash256}; #[derive(Clone, PartialEq, Debug)] pub enum Error { @@ -13,6 +13,7 @@ pub enum Error { InvalidParentDelta(usize), InvalidNodeDelta(usize), DeltaOverflow(usize), + ProposerBoostOverflow(usize), IndexOverflow(&'static str), InvalidDeltaLen { deltas: usize, @@ -22,16 +23,19 @@ pub enum Error { current_finalized_epoch: Epoch, new_finalized_epoch: Epoch, }, - InvalidBestNode { - start_root: Hash256, - justified_epoch: Epoch, - finalized_epoch: Epoch, - head_root: Hash256, - head_justified_epoch: Epoch, - head_finalized_epoch: Epoch, - }, + InvalidBestNode(Box), InvalidAncestorOfValidPayload { ancestor_block_root: Hash256, ancestor_payload_block_hash: Hash256, }, } + +#[derive(Clone, PartialEq, Debug)] +pub struct InvalidBestNodeInfo { + pub start_root: Hash256, + pub justified_checkpoint: Checkpoint, + pub finalized_checkpoint: Checkpoint, + pub head_root: Hash256, + pub head_justified_checkpoint: Option, + pub head_finalized_checkpoint: Option, +} diff --git a/consensus/proto_array/src/fork_choice_test_definition.rs b/consensus/proto_array/src/fork_choice_test_definition.rs index 44036911c..e28fc6771 100644 --- a/consensus/proto_array/src/fork_choice_test_definition.rs +++ b/consensus/proto_array/src/fork_choice_test_definition.rs @@ -4,7 +4,7 @@ mod votes; use crate::proto_array_fork_choice::{Block, ExecutionStatus, ProtoArrayForkChoice}; use serde_derive::{Deserialize, Serialize}; -use types::{AttestationShufflingId, Epoch, Hash256, Slot}; +use types::{AttestationShufflingId, Checkpoint, Epoch, EthSpec, Hash256, MainnetEthSpec, Slot}; pub use ffg_updates::*; pub use no_votes::*; @@ -13,24 +13,22 @@ pub use votes::*; #[derive(Debug, Clone, Serialize, Deserialize)] pub enum Operation { FindHead { - justified_epoch: Epoch, - justified_root: Hash256, - finalized_epoch: Epoch, + justified_checkpoint: Checkpoint, + finalized_checkpoint: Checkpoint, justified_state_balances: Vec, expected_head: Hash256, }, InvalidFindHead { - justified_epoch: Epoch, - justified_root: Hash256, - finalized_epoch: Epoch, + justified_checkpoint: Checkpoint, + finalized_checkpoint: Checkpoint, justified_state_balances: Vec, }, ProcessBlock { slot: Slot, root: Hash256, parent_root: Hash256, - justified_epoch: Epoch, - finalized_epoch: Epoch, + justified_checkpoint: Checkpoint, + finalized_checkpoint: Checkpoint, }, ProcessAttestation { validator_index: usize, @@ -47,9 +45,8 @@ pub enum Operation { #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ForkChoiceTestDefinition { pub finalized_block_slot: Slot, - pub justified_epoch: Epoch, - pub finalized_epoch: Epoch, - pub finalized_root: Hash256, + pub justified_checkpoint: Checkpoint, + pub finalized_checkpoint: Checkpoint, pub operations: Vec, } @@ -61,9 +58,8 @@ impl ForkChoiceTestDefinition { let mut fork_choice = ProtoArrayForkChoice::new( self.finalized_block_slot, Hash256::zero(), - self.justified_epoch, - self.finalized_epoch, - self.finalized_root, + self.justified_checkpoint, + self.finalized_checkpoint, junk_shuffling_id.clone(), junk_shuffling_id, execution_status, @@ -73,21 +69,22 @@ impl ForkChoiceTestDefinition { for (op_index, op) in self.operations.into_iter().enumerate() { match op.clone() { Operation::FindHead { - justified_epoch, - justified_root, - finalized_epoch, + justified_checkpoint, + finalized_checkpoint, justified_state_balances, expected_head, } => { let head = fork_choice - .find_head( - justified_epoch, - justified_root, - finalized_epoch, + .find_head::( + justified_checkpoint, + finalized_checkpoint, &justified_state_balances, + Hash256::zero(), + &MainnetEthSpec::default_spec(), ) - .unwrap_or_else(|_| { - panic!("find_head op at index {} returned error", op_index) + .map_err(|e| e) + .unwrap_or_else(|e| { + panic!("find_head op at index {} returned error {}", op_index, e) }); assert_eq!( @@ -98,16 +95,16 @@ impl ForkChoiceTestDefinition { check_bytes_round_trip(&fork_choice); } Operation::InvalidFindHead { - justified_epoch, - justified_root, - finalized_epoch, + justified_checkpoint, + finalized_checkpoint, justified_state_balances, } => { - let result = fork_choice.find_head( - justified_epoch, - justified_root, - finalized_epoch, + let result = fork_choice.find_head::( + justified_checkpoint, + finalized_checkpoint, &justified_state_balances, + Hash256::zero(), + &MainnetEthSpec::default_spec(), ); assert!( @@ -122,8 +119,8 @@ impl ForkChoiceTestDefinition { slot, root, parent_root, - justified_epoch, - finalized_epoch, + justified_checkpoint, + finalized_checkpoint, } => { let block = Block { slot, @@ -139,8 +136,8 @@ impl ForkChoiceTestDefinition { Epoch::new(0), Hash256::zero(), ), - justified_epoch, - finalized_epoch, + justified_checkpoint, + finalized_checkpoint, execution_status, }; fork_choice.process_block(block).unwrap_or_else(|e| { @@ -193,7 +190,16 @@ impl ForkChoiceTestDefinition { /// Gives a hash that is not the zero hash (unless i is `usize::max_value)`. fn get_hash(i: u64) -> Hash256 { - Hash256::from_low_u64_be(i) + Hash256::from_low_u64_be(i + 1) +} + +/// Gives a checkpoint with a root that is not the zero hash (unless i is `usize::max_value)`. +/// `Epoch` will always equal `i`. +fn get_checkpoint(i: u64) -> Checkpoint { + Checkpoint { + epoch: Epoch::new(i), + root: get_hash(i), + } } fn check_bytes_round_trip(original: &ProtoArrayForkChoice) { diff --git a/consensus/proto_array/src/fork_choice_test_definition/ffg_updates.rs b/consensus/proto_array/src/fork_choice_test_definition/ffg_updates.rs index 4b7eb25d7..a12906450 100644 --- a/consensus/proto_array/src/fork_choice_test_definition/ffg_updates.rs +++ b/consensus/proto_array/src/fork_choice_test_definition/ffg_updates.rs @@ -6,9 +6,8 @@ pub fn get_ffg_case_01_test_definition() -> ForkChoiceTestDefinition { // Ensure that the head starts at the finalized block. ops.push(Operation::FindHead { - justified_epoch: Epoch::new(0), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), expected_head: get_hash(0), }); @@ -26,22 +25,22 @@ pub fn get_ffg_case_01_test_definition() -> ForkChoiceTestDefinition { slot: Slot::new(1), root: get_hash(1), parent_root: get_hash(0), - justified_epoch: Epoch::new(0), - finalized_epoch: Epoch::new(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), }); ops.push(Operation::ProcessBlock { slot: Slot::new(2), root: get_hash(2), parent_root: get_hash(1), - justified_epoch: Epoch::new(1), - finalized_epoch: Epoch::new(0), + justified_checkpoint: get_checkpoint(1), + finalized_checkpoint: get_checkpoint(0), }); ops.push(Operation::ProcessBlock { slot: Slot::new(3), root: get_hash(3), parent_root: get_hash(2), - justified_epoch: Epoch::new(2), - finalized_epoch: Epoch::new(1), + justified_checkpoint: get_checkpoint(2), + finalized_checkpoint: get_checkpoint(1), }); // Ensure that with justified epoch 0 we find 3 @@ -54,9 +53,8 @@ pub fn get_ffg_case_01_test_definition() -> ForkChoiceTestDefinition { // | // 3 <- head ops.push(Operation::FindHead { - justified_epoch: Epoch::new(0), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), expected_head: get_hash(3), }); @@ -71,9 +69,8 @@ pub fn get_ffg_case_01_test_definition() -> ForkChoiceTestDefinition { // | // 3 <- head ops.push(Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: get_hash(2), - finalized_epoch: Epoch::new(0), + justified_checkpoint: get_checkpoint(1), + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), expected_head: get_hash(2), }); @@ -88,9 +85,8 @@ pub fn get_ffg_case_01_test_definition() -> ForkChoiceTestDefinition { // | // 3 <- start + head ops.push(Operation::FindHead { - justified_epoch: Epoch::new(2), - justified_root: get_hash(3), - finalized_epoch: Epoch::new(1), + justified_checkpoint: get_checkpoint(2), + finalized_checkpoint: get_checkpoint(1), justified_state_balances: balances, expected_head: get_hash(3), }); @@ -98,9 +94,8 @@ pub fn get_ffg_case_01_test_definition() -> ForkChoiceTestDefinition { // END OF TESTS ForkChoiceTestDefinition { finalized_block_slot: Slot::new(0), - justified_epoch: Epoch::new(1), - finalized_epoch: Epoch::new(1), - finalized_root: get_hash(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), operations: ops, } } @@ -111,9 +106,8 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { // Ensure that the head starts at the finalized block. ops.push(Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(1), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), expected_head: get_hash(0), }); @@ -137,36 +131,48 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { slot: Slot::new(1), root: get_hash(1), parent_root: get_hash(0), - justified_epoch: Epoch::new(0), - finalized_epoch: Epoch::new(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), }); ops.push(Operation::ProcessBlock { slot: Slot::new(2), root: get_hash(3), parent_root: get_hash(1), - justified_epoch: Epoch::new(1), - finalized_epoch: Epoch::new(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(1), + }, + finalized_checkpoint: get_checkpoint(0), }); ops.push(Operation::ProcessBlock { slot: Slot::new(3), root: get_hash(5), parent_root: get_hash(3), - justified_epoch: Epoch::new(1), - finalized_epoch: Epoch::new(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(1), + }, + finalized_checkpoint: get_checkpoint(0), }); ops.push(Operation::ProcessBlock { slot: Slot::new(4), root: get_hash(7), parent_root: get_hash(5), - justified_epoch: Epoch::new(1), - finalized_epoch: Epoch::new(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(1), + }, + finalized_checkpoint: get_checkpoint(0), }); ops.push(Operation::ProcessBlock { - slot: Slot::new(4), + slot: Slot::new(5), root: get_hash(9), parent_root: get_hash(7), - justified_epoch: Epoch::new(2), - finalized_epoch: Epoch::new(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(3), + }, + finalized_checkpoint: get_checkpoint(0), }); // Right branch @@ -174,36 +180,42 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { slot: Slot::new(1), root: get_hash(2), parent_root: get_hash(0), - justified_epoch: Epoch::new(0), - finalized_epoch: Epoch::new(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), }); ops.push(Operation::ProcessBlock { slot: Slot::new(2), root: get_hash(4), parent_root: get_hash(2), - justified_epoch: Epoch::new(0), - finalized_epoch: Epoch::new(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), }); ops.push(Operation::ProcessBlock { slot: Slot::new(3), root: get_hash(6), parent_root: get_hash(4), - justified_epoch: Epoch::new(0), - finalized_epoch: Epoch::new(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), }); ops.push(Operation::ProcessBlock { slot: Slot::new(4), root: get_hash(8), parent_root: get_hash(6), - justified_epoch: Epoch::new(1), - finalized_epoch: Epoch::new(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(2), + }, + finalized_checkpoint: get_checkpoint(0), }); ops.push(Operation::ProcessBlock { - slot: Slot::new(4), + slot: Slot::new(5), root: get_hash(10), parent_root: get_hash(8), - justified_epoch: Epoch::new(2), - finalized_epoch: Epoch::new(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(4), + }, + finalized_checkpoint: get_checkpoint(0), }); // Ensure that if we start at 0 we find 10 (just: 0, fin: 0). @@ -220,25 +232,28 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { // | | // 9 10 <-- head ops.push(Operation::FindHead { - justified_epoch: Epoch::new(0), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), expected_head: get_hash(10), }); // Same as above, but with justified epoch 2. ops.push(Operation::FindHead { - justified_epoch: Epoch::new(2), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(4), + }, + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), expected_head: get_hash(10), }); // Same as above, but with justified epoch 3 (should be invalid). ops.push(Operation::InvalidFindHead { - justified_epoch: Epoch::new(3), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(3), + root: get_hash(6), + }, + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), }); @@ -275,25 +290,28 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { // | | // head -> 9 10 ops.push(Operation::FindHead { - justified_epoch: Epoch::new(0), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), expected_head: get_hash(9), }); // Save as above but justified epoch 2. ops.push(Operation::FindHead { - justified_epoch: Epoch::new(2), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(3), + }, + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), expected_head: get_hash(9), }); // Save as above but justified epoch 3 (should fail). ops.push(Operation::InvalidFindHead { - justified_epoch: Epoch::new(3), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(3), + root: get_hash(5), + }, + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), }); @@ -330,25 +348,28 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { // | | // 9 10 <-- head ops.push(Operation::FindHead { - justified_epoch: Epoch::new(0), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), expected_head: get_hash(10), }); // Same as above but justified epoch 2. ops.push(Operation::FindHead { - justified_epoch: Epoch::new(2), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(4), + }, + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), expected_head: get_hash(10), }); // Same as above but justified epoch 3 (should fail). ops.push(Operation::InvalidFindHead { - justified_epoch: Epoch::new(3), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(3), + root: get_hash(6), + }, + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), }); @@ -366,25 +387,31 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { // | | // head -> 9 10 ops.push(Operation::FindHead { - justified_epoch: Epoch::new(0), - justified_root: get_hash(1), - finalized_epoch: Epoch::new(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(0), + root: get_hash(1), + }, + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), expected_head: get_hash(9), }); // Same as above but justified epoch 2. ops.push(Operation::FindHead { - justified_epoch: Epoch::new(2), - justified_root: get_hash(1), - finalized_epoch: Epoch::new(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(3), + }, + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), expected_head: get_hash(9), }); // Same as above but justified epoch 3 (should fail). ops.push(Operation::InvalidFindHead { - justified_epoch: Epoch::new(3), - justified_root: get_hash(1), - finalized_epoch: Epoch::new(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(3), + root: get_hash(5), + }, + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), }); @@ -402,34 +429,36 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { // | | // 9 10 <- head ops.push(Operation::FindHead { - justified_epoch: Epoch::new(0), - justified_root: get_hash(2), - finalized_epoch: Epoch::new(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), expected_head: get_hash(10), }); // Same as above but justified epoch 2. ops.push(Operation::FindHead { - justified_epoch: Epoch::new(2), - justified_root: get_hash(2), - finalized_epoch: Epoch::new(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(4), + }, + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances.clone(), expected_head: get_hash(10), }); // Same as above but justified epoch 3 (should fail). ops.push(Operation::InvalidFindHead { - justified_epoch: Epoch::new(3), - justified_root: get_hash(2), - finalized_epoch: Epoch::new(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(3), + root: get_hash(6), + }, + finalized_checkpoint: get_checkpoint(0), justified_state_balances: balances, }); // END OF TESTS ForkChoiceTestDefinition { finalized_block_slot: Slot::new(0), - justified_epoch: Epoch::new(1), - finalized_epoch: Epoch::new(1), - finalized_root: get_hash(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), operations: ops, } } diff --git a/consensus/proto_array/src/fork_choice_test_definition/no_votes.rs b/consensus/proto_array/src/fork_choice_test_definition/no_votes.rs index e42abe288..0fbcafc5d 100644 --- a/consensus/proto_array/src/fork_choice_test_definition/no_votes.rs +++ b/consensus/proto_array/src/fork_choice_test_definition/no_votes.rs @@ -6,9 +6,14 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { let operations = vec![ // Check that the head is the finalized block. Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: Hash256::zero(), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, justified_state_balances: balances.clone(), expected_head: Hash256::zero(), }, @@ -18,11 +23,17 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { // / // 2 Operation::ProcessBlock { - slot: Slot::new(0), + slot: Slot::new(1), root: get_hash(2), - parent_root: get_hash(0), - justified_epoch: Epoch::new(1), - finalized_epoch: Epoch::new(1), + parent_root: Hash256::zero(), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, }, // Ensure the head is 2 // @@ -30,9 +41,14 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { // / // 2 <- head Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: Hash256::zero(), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, justified_state_balances: balances.clone(), expected_head: get_hash(2), }, @@ -42,11 +58,17 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { // / \ // 2 1 Operation::ProcessBlock { - slot: Slot::new(0), + slot: Slot::new(1), root: get_hash(1), parent_root: get_hash(0), - justified_epoch: Epoch::new(1), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, }, // Ensure the head is still 2 // @@ -54,9 +76,14 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { // / \ // head-> 2 1 Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: Hash256::zero(), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, justified_state_balances: balances.clone(), expected_head: get_hash(2), }, @@ -68,11 +95,17 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { // | // 3 Operation::ProcessBlock { - slot: Slot::new(0), + slot: Slot::new(2), root: get_hash(3), parent_root: get_hash(1), - justified_epoch: Epoch::new(1), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, }, // Ensure 2 is still the head // @@ -82,9 +115,14 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { // | // 3 Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: Hash256::zero(), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, justified_state_balances: balances.clone(), expected_head: get_hash(2), }, @@ -96,11 +134,17 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { // | | // 4 3 Operation::ProcessBlock { - slot: Slot::new(0), + slot: Slot::new(2), root: get_hash(4), parent_root: get_hash(2), - justified_epoch: Epoch::new(1), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, }, // Ensure the head is 4. // @@ -110,9 +154,14 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { // | | // head-> 4 3 Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: Hash256::zero(), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, justified_state_balances: balances.clone(), expected_head: get_hash(4), }, @@ -126,11 +175,14 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { // | // 5 <- justified epoch = 2 Operation::ProcessBlock { - slot: Slot::new(0), + slot: Slot::new(3), root: get_hash(5), parent_root: get_hash(4), - justified_epoch: Epoch::new(2), - finalized_epoch: Epoch::new(1), + justified_checkpoint: get_checkpoint(2), + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, }, // Ensure the head is still 4 whilst the justified epoch is 0. // @@ -142,9 +194,14 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { // | // 5 Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: Hash256::zero(), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, justified_state_balances: balances.clone(), expected_head: get_hash(4), }, @@ -158,9 +215,14 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { // | // 5 <- starting from 5 with justified epoch 0 should error. Operation::InvalidFindHead { - justified_epoch: Epoch::new(1), - justified_root: get_hash(5), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(5), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, justified_state_balances: balances.clone(), }, // Set the justified epoch to 2 and the start block to 5 and ensure 5 is the head. @@ -173,9 +235,11 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { // | // 5 <- head Operation::FindHead { - justified_epoch: Epoch::new(2), - justified_root: get_hash(5), - finalized_epoch: Epoch::new(1), + justified_checkpoint: get_checkpoint(2), + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, justified_state_balances: balances.clone(), expected_head: get_hash(5), }, @@ -191,11 +255,14 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { // | // 6 Operation::ProcessBlock { - slot: Slot::new(0), + slot: Slot::new(4), root: get_hash(6), parent_root: get_hash(5), - justified_epoch: Epoch::new(2), - finalized_epoch: Epoch::new(1), + justified_checkpoint: get_checkpoint(2), + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, }, // Ensure 6 is the head // @@ -209,9 +276,11 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { // | // 6 <- head Operation::FindHead { - justified_epoch: Epoch::new(2), - justified_root: get_hash(5), - finalized_epoch: Epoch::new(1), + justified_checkpoint: get_checkpoint(2), + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, justified_state_balances: balances, expected_head: get_hash(6), }, @@ -219,9 +288,14 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { ForkChoiceTestDefinition { finalized_block_slot: Slot::new(0), - justified_epoch: Epoch::new(1), - finalized_epoch: Epoch::new(1), - finalized_root: get_hash(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: Hash256::zero(), + }, operations, } } diff --git a/consensus/proto_array/src/fork_choice_test_definition/votes.rs b/consensus/proto_array/src/fork_choice_test_definition/votes.rs index ac9513c5f..f65177a84 100644 --- a/consensus/proto_array/src/fork_choice_test_definition/votes.rs +++ b/consensus/proto_array/src/fork_choice_test_definition/votes.rs @@ -6,9 +6,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // Ensure that the head starts at the finalized block. ops.push(Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, justified_state_balances: balances.clone(), expected_head: get_hash(0), }); @@ -19,11 +24,17 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // / // 2 ops.push(Operation::ProcessBlock { - slot: Slot::new(0), + slot: Slot::new(1), root: get_hash(2), parent_root: get_hash(0), - justified_epoch: Epoch::new(1), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, }); // Ensure that the head is 2 @@ -32,9 +43,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // / // head-> 2 ops.push(Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, justified_state_balances: balances.clone(), expected_head: get_hash(2), }); @@ -46,11 +62,17 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // / \ // 2 1 ops.push(Operation::ProcessBlock { - slot: Slot::new(0), + slot: Slot::new(1), root: get_hash(1), parent_root: get_hash(0), - justified_epoch: Epoch::new(1), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, }); // Ensure that the head is still 2 @@ -59,9 +81,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // / \ // head-> 2 1 ops.push(Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, justified_state_balances: balances.clone(), expected_head: get_hash(2), }); @@ -77,15 +104,20 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { target_epoch: Epoch::new(2), }); - // Ensure that the head is now 1, beacuse 1 has a vote. + // Ensure that the head is now 1, because 1 has a vote. // // 0 // / \ // 2 1 <- head ops.push(Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, justified_state_balances: balances.clone(), expected_head: get_hash(1), }); @@ -107,9 +139,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // / \ // head-> 2 1 ops.push(Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, justified_state_balances: balances.clone(), expected_head: get_hash(2), }); @@ -122,11 +159,17 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // | // 3 ops.push(Operation::ProcessBlock { - slot: Slot::new(0), + slot: Slot::new(2), root: get_hash(3), parent_root: get_hash(1), - justified_epoch: Epoch::new(1), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, }); // Ensure that the head is still 2 @@ -137,9 +180,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // | // 3 ops.push(Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, justified_state_balances: balances.clone(), expected_head: get_hash(2), }); @@ -165,9 +213,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // | // 3 ops.push(Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, justified_state_balances: balances.clone(), expected_head: get_hash(2), }); @@ -194,9 +247,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // | // 3 <- head ops.push(Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, justified_state_balances: balances.clone(), expected_head: get_hash(3), }); @@ -211,11 +269,17 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // | // 4 ops.push(Operation::ProcessBlock { - slot: Slot::new(0), + slot: Slot::new(3), root: get_hash(4), parent_root: get_hash(3), - justified_epoch: Epoch::new(1), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, }); // Ensure that the head is now 4 @@ -228,9 +292,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // | // 4 <- head ops.push(Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, justified_state_balances: balances.clone(), expected_head: get_hash(4), }); @@ -247,11 +316,17 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // / // 5 <- justified epoch = 2 ops.push(Operation::ProcessBlock { - slot: Slot::new(0), + slot: Slot::new(4), root: get_hash(5), parent_root: get_hash(4), - justified_epoch: Epoch::new(2), - finalized_epoch: Epoch::new(2), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(1), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(1), + }, }); // Ensure that 5 is filtered out and the head stays at 4. @@ -266,9 +341,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // / // 5 ops.push(Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, justified_state_balances: balances.clone(), expected_head: get_hash(4), }); @@ -288,8 +368,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { slot: Slot::new(0), root: get_hash(6), parent_root: get_hash(4), - justified_epoch: Epoch::new(1), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, }); // Move both votes to 5. @@ -336,22 +422,40 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { slot: Slot::new(0), root: get_hash(7), parent_root: get_hash(5), - justified_epoch: Epoch::new(2), - finalized_epoch: Epoch::new(2), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, }); ops.push(Operation::ProcessBlock { slot: Slot::new(0), root: get_hash(8), parent_root: get_hash(7), - justified_epoch: Epoch::new(2), - finalized_epoch: Epoch::new(2), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, }); ops.push(Operation::ProcessBlock { slot: Slot::new(0), root: get_hash(9), parent_root: get_hash(8), - justified_epoch: Epoch::new(2), - finalized_epoch: Epoch::new(2), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, }); // Ensure that 6 is the head, even though 5 has all the votes. This is testing to ensure @@ -373,9 +477,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // / // 9 ops.push(Operation::FindHead { - justified_epoch: Epoch::new(1), - justified_root: get_hash(0), - finalized_epoch: Epoch::new(1), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, justified_state_balances: balances.clone(), expected_head: get_hash(6), }); @@ -401,9 +510,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // / // head-> 9 ops.push(Operation::FindHead { - justified_epoch: Epoch::new(2), - justified_root: get_hash(5), - finalized_epoch: Epoch::new(2), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, justified_state_balances: balances.clone(), expected_head: get_hash(9), }); @@ -460,15 +574,26 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { slot: Slot::new(0), root: get_hash(10), parent_root: get_hash(8), - justified_epoch: Epoch::new(2), - finalized_epoch: Epoch::new(2), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, }); // Double-check the head is still 9 (no diagram this time) ops.push(Operation::FindHead { - justified_epoch: Epoch::new(2), - justified_root: get_hash(5), - finalized_epoch: Epoch::new(2), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, justified_state_balances: balances.clone(), expected_head: get_hash(9), }); @@ -522,9 +647,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // / \ // 9 10 <- head ops.push(Operation::FindHead { - justified_epoch: Epoch::new(2), - justified_root: get_hash(5), - finalized_epoch: Epoch::new(2), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, justified_state_balances: balances.clone(), expected_head: get_hash(10), }); @@ -542,9 +672,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // / \ // head-> 9 10 ops.push(Operation::FindHead { - justified_epoch: Epoch::new(2), - justified_root: get_hash(5), - finalized_epoch: Epoch::new(2), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, justified_state_balances: balances.clone(), expected_head: get_hash(9), }); @@ -562,9 +697,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // / \ // 9 10 <- head ops.push(Operation::FindHead { - justified_epoch: Epoch::new(2), - justified_root: get_hash(5), - finalized_epoch: Epoch::new(2), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, justified_state_balances: balances.clone(), expected_head: get_hash(10), }); @@ -583,9 +723,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // / \ // head-> 9 10 ops.push(Operation::FindHead { - justified_epoch: Epoch::new(2), - justified_root: get_hash(5), - finalized_epoch: Epoch::new(2), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, justified_state_balances: balances.clone(), expected_head: get_hash(9), }); @@ -599,9 +744,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // Run find-head, ensure the no-op prune didn't change the head. ops.push(Operation::FindHead { - justified_epoch: Epoch::new(2), - justified_root: get_hash(5), - finalized_epoch: Epoch::new(2), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, justified_state_balances: balances.clone(), expected_head: get_hash(9), }); @@ -632,9 +782,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // Run find-head, ensure the prune didn't change the head. ops.push(Operation::FindHead { - justified_epoch: Epoch::new(2), - justified_root: get_hash(5), - finalized_epoch: Epoch::new(2), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, justified_state_balances: balances.clone(), expected_head: get_hash(9), }); @@ -654,8 +809,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { slot: Slot::new(0), root: get_hash(11), parent_root: get_hash(9), - justified_epoch: Epoch::new(2), - finalized_epoch: Epoch::new(2), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, }); // Ensure the head is now 11 @@ -670,18 +831,28 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // | // head-> 11 ops.push(Operation::FindHead { - justified_epoch: Epoch::new(2), - justified_root: get_hash(5), - finalized_epoch: Epoch::new(2), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(2), + root: get_hash(5), + }, justified_state_balances: balances, expected_head: get_hash(11), }); ForkChoiceTestDefinition { finalized_block_slot: Slot::new(0), - justified_epoch: Epoch::new(1), - finalized_epoch: Epoch::new(1), - finalized_root: get_hash(0), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, + finalized_checkpoint: Checkpoint { + epoch: Epoch::new(1), + root: get_hash(0), + }, operations: ops, } } diff --git a/consensus/proto_array/src/lib.rs b/consensus/proto_array/src/lib.rs index 7594f5b12..216d189fb 100644 --- a/consensus/proto_array/src/lib.rs +++ b/consensus/proto_array/src/lib.rs @@ -8,5 +8,7 @@ pub use crate::proto_array_fork_choice::{Block, ExecutionStatus, ProtoArrayForkC pub use error::Error; pub mod core { - pub use super::proto_array::ProtoArray; + pub use super::proto_array::{ProposerBoost, ProtoArray, ProtoNode}; + pub use super::proto_array_fork_choice::VoteTracker; + pub use super::ssz_container::SszContainer; } diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 6732e0fba..465ef9d4f 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -1,13 +1,16 @@ +use crate::error::InvalidBestNodeInfo; use crate::{error::Error, Block, ExecutionStatus}; use serde_derive::{Deserialize, Serialize}; use ssz::four_byte_option_impl; +use ssz::Encode; use ssz_derive::{Decode, Encode}; use std::collections::HashMap; -use types::{AttestationShufflingId, Epoch, Hash256, Slot}; +use types::{AttestationShufflingId, ChainSpec, Checkpoint, Epoch, EthSpec, Hash256, Slot}; // Define a "legacy" implementation of `Option` which uses four bytes for encoding the union // selector. four_byte_option_impl!(four_byte_option_usize, usize); +four_byte_option_impl!(four_byte_option_checkpoint, Checkpoint); #[derive(Clone, PartialEq, Debug, Encode, Decode, Serialize, Deserialize)] pub struct ProtoNode { @@ -28,59 +31,31 @@ pub struct ProtoNode { pub root: Hash256, #[ssz(with = "four_byte_option_usize")] pub parent: Option, - pub justified_epoch: Epoch, - pub finalized_epoch: Epoch, - weight: u64, + #[ssz(with = "four_byte_option_checkpoint")] + pub justified_checkpoint: Option, + #[ssz(with = "four_byte_option_checkpoint")] + pub finalized_checkpoint: Option, + pub weight: u64, #[ssz(with = "four_byte_option_usize")] - best_child: Option, + pub best_child: Option, #[ssz(with = "four_byte_option_usize")] - best_descendant: Option, + pub best_descendant: Option, /// Indicates if an execution node has marked this block as valid. Also contains the execution /// block hash. pub execution_status: ExecutionStatus, } -/// Only used for SSZ deserialization of the persisted fork choice during the database migration -/// from schema 4 to schema 5. -#[derive(Encode, Decode)] -pub struct LegacyProtoNode { - pub slot: Slot, - pub state_root: Hash256, - pub target_root: Hash256, - pub current_epoch_shuffling_id: AttestationShufflingId, - pub next_epoch_shuffling_id: AttestationShufflingId, +#[derive(PartialEq, Debug, Encode, Decode, Serialize, Deserialize, Copy, Clone)] +pub struct ProposerBoost { pub root: Hash256, - #[ssz(with = "four_byte_option_usize")] - pub parent: Option, - pub justified_epoch: Epoch, - pub finalized_epoch: Epoch, - weight: u64, - #[ssz(with = "four_byte_option_usize")] - best_child: Option, - #[ssz(with = "four_byte_option_usize")] - best_descendant: Option, + pub score: u64, } -impl Into for LegacyProtoNode { - fn into(self) -> ProtoNode { - ProtoNode { - slot: self.slot, - state_root: self.state_root, - target_root: self.target_root, - current_epoch_shuffling_id: self.current_epoch_shuffling_id, - next_epoch_shuffling_id: self.next_epoch_shuffling_id, - root: self.root, - parent: self.parent, - justified_epoch: self.justified_epoch, - finalized_epoch: self.finalized_epoch, - weight: self.weight, - best_child: self.best_child, - best_descendant: self.best_descendant, - // We set the following execution value as if the block is a pre-merge-fork block. This - // is safe as long as we never import a merge block with the old version of proto-array. - // This will be safe since we can't actually process merge blocks until we've made this - // change to fork choice. - execution_status: ExecutionStatus::irrelevant(), +impl Default for ProposerBoost { + fn default() -> Self { + Self { + root: Hash256::zero(), + score: 0, } } } @@ -90,10 +65,11 @@ pub struct ProtoArray { /// Do not attempt to prune the tree unless it has at least this many nodes. Small prunes /// simply waste time. pub prune_threshold: usize, - pub justified_epoch: Epoch, - pub finalized_epoch: Epoch, + pub justified_checkpoint: Checkpoint, + pub finalized_checkpoint: Checkpoint, pub nodes: Vec, pub indices: HashMap, + pub previous_proposer_boost: ProposerBoost, } impl ProtoArray { @@ -110,11 +86,14 @@ impl ProtoArray { /// - Compare the current node with the parents best-child, updating it if the current node /// should become the best child. /// - If required, update the parents best-descendant with the current node or its best-descendant. - pub fn apply_score_changes( + pub fn apply_score_changes( &mut self, mut deltas: Vec, - justified_epoch: Epoch, - finalized_epoch: Epoch, + justified_checkpoint: Checkpoint, + finalized_checkpoint: Checkpoint, + new_balances: &[u64], + proposer_boost_root: Hash256, + spec: &ChainSpec, ) -> Result<(), Error> { if deltas.len() != self.indices.len() { return Err(Error::InvalidDeltaLen { @@ -123,11 +102,16 @@ impl ProtoArray { }); } - if justified_epoch != self.justified_epoch || finalized_epoch != self.finalized_epoch { - self.justified_epoch = justified_epoch; - self.finalized_epoch = finalized_epoch; + if justified_checkpoint != self.justified_checkpoint + || finalized_checkpoint != self.finalized_checkpoint + { + self.justified_checkpoint = justified_checkpoint; + self.finalized_checkpoint = finalized_checkpoint; } + // Default the proposer boost score to zero. + let mut proposer_score = 0; + // Iterate backwards through all indices in `self.nodes`. for node_index in (0..self.nodes.len()).rev() { let node = self @@ -142,11 +126,35 @@ impl ProtoArray { continue; } - let node_delta = deltas + let mut node_delta = deltas .get(node_index) .copied() .ok_or(Error::InvalidNodeDelta(node_index))?; + // If we find the node for which the proposer boost was previously applied, decrease + // the delta by the previous score amount. + if self.previous_proposer_boost.root != Hash256::zero() + && self.previous_proposer_boost.root == node.root + { + node_delta = node_delta + .checked_sub(self.previous_proposer_boost.score as i64) + .ok_or(Error::DeltaOverflow(node_index))?; + } + // If we find the node matching the current proposer boost root, increase + // the delta by the new score amount. + // + // https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/fork-choice.md#get_latest_attesting_balance + if let Some(proposer_score_boost) = spec.proposer_score_boost { + if proposer_boost_root != Hash256::zero() && proposer_boost_root == node.root { + proposer_score = + calculate_proposer_boost::(new_balances, proposer_score_boost) + .ok_or(Error::ProposerBoostOverflow(node_index))?; + node_delta = node_delta + .checked_add(proposer_score as i64) + .ok_or(Error::DeltaOverflow(node_index))?; + } + } + // Apply the delta to the node. if node_delta < 0 { // Note: I am conflicted about whether to use `saturating_sub` or `checked_sub` @@ -180,6 +188,12 @@ impl ProtoArray { } } + // After applying all deltas, update the `previous_proposer_boost`. + self.previous_proposer_boost = ProposerBoost { + root: proposer_boost_root, + score: proposer_score, + }; + // A second time, iterate backwards through all indices in `self.nodes`. // // We _must_ perform these functions separate from the weight-updating loop above to ensure @@ -221,8 +235,8 @@ impl ProtoArray { parent: block .parent_root .and_then(|parent| self.indices.get(&parent).copied()), - justified_epoch: block.justified_epoch, - finalized_epoch: block.finalized_epoch, + justified_checkpoint: Some(block.justified_checkpoint), + finalized_checkpoint: Some(block.finalized_checkpoint), weight: 0, best_child: None, best_descendant: None, @@ -315,14 +329,14 @@ impl ProtoArray { // Perform a sanity check that the node is indeed valid to be the head. if !self.node_is_viable_for_head(best_node) { - return Err(Error::InvalidBestNode { + return Err(Error::InvalidBestNode(Box::new(InvalidBestNodeInfo { start_root: *justified_root, - justified_epoch: self.justified_epoch, - finalized_epoch: self.finalized_epoch, + justified_checkpoint: self.justified_checkpoint, + finalized_checkpoint: self.finalized_checkpoint, head_root: justified_node.root, - head_justified_epoch: justified_node.justified_epoch, - head_finalized_epoch: justified_node.finalized_epoch, - }); + head_justified_checkpoint: justified_node.justified_checkpoint, + head_finalized_checkpoint: justified_node.finalized_checkpoint, + }))); } Ok(best_node.root) @@ -523,9 +537,16 @@ impl ProtoArray { /// Any node that has a different finalized or justified epoch should not be viable for the /// head. fn node_is_viable_for_head(&self, node: &ProtoNode) -> bool { - (node.justified_epoch == self.justified_epoch || self.justified_epoch == Epoch::new(0)) - && (node.finalized_epoch == self.finalized_epoch - || self.finalized_epoch == Epoch::new(0)) + if let (Some(node_justified_checkpoint), Some(node_finalized_checkpoint)) = + (node.justified_checkpoint, node.finalized_checkpoint) + { + (node_justified_checkpoint == self.justified_checkpoint + || self.justified_checkpoint.epoch == Epoch::new(0)) + && (node_finalized_checkpoint == self.finalized_checkpoint + || self.finalized_checkpoint.epoch == Epoch::new(0)) + } else { + false + } } /// Return a reverse iterator over the nodes which comprise the chain ending at `block_root`. @@ -549,6 +570,38 @@ impl ProtoArray { } } +/// A helper method to calculate the proposer boost based on the given `validator_balances`. +/// This does *not* do any verification about whether a boost should or should not be applied. +/// The `validator_balances` array used here is assumed to be structured like the one stored in +/// the `BalancesCache`, where *effective* balances are stored and inactive balances are defaulted +/// to zero. +/// +/// Returns `None` if there is an overflow or underflow when calculating the score. +/// +/// https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/fork-choice.md#get_latest_attesting_balance +fn calculate_proposer_boost( + validator_balances: &[u64], + proposer_score_boost: u64, +) -> Option { + let mut total_balance: u64 = 0; + let mut num_validators: u64 = 0; + for &balance in validator_balances { + // We need to filter zero balances here to get an accurate active validator count. + // This is because we default inactive validator balances to zero when creating + // this balances array. + if balance != 0 { + total_balance = total_balance.checked_add(balance)?; + num_validators = num_validators.checked_add(1)?; + } + } + let average_balance = total_balance.checked_div(num_validators)?; + let committee_size = num_validators.checked_div(E::slots_per_epoch())?; + let committee_weight = committee_size.checked_mul(average_balance)?; + committee_weight + .checked_mul(proposer_score_boost)? + .checked_div(100) +} + /// Reverse iterator over one path through a `ProtoArray`. pub struct Iter<'a> { next_node_index: Option, diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index d0abea4f1..891eafabe 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -1,11 +1,11 @@ use crate::error::Error; -use crate::proto_array::ProtoArray; -use crate::ssz_container::{LegacySszContainer, SszContainer}; +use crate::proto_array::{ProposerBoost, ProtoArray}; +use crate::ssz_container::SszContainer; use serde_derive::{Deserialize, Serialize}; use ssz::{Decode, Encode}; use ssz_derive::{Decode, Encode}; use std::collections::HashMap; -use types::{AttestationShufflingId, Epoch, Hash256, Slot}; +use types::{AttestationShufflingId, ChainSpec, Checkpoint, Epoch, EthSpec, Hash256, Slot}; pub const DEFAULT_PRUNE_THRESHOLD: usize = 256; @@ -63,8 +63,8 @@ pub struct Block { pub target_root: Hash256, pub current_epoch_shuffling_id: AttestationShufflingId, pub next_epoch_shuffling_id: AttestationShufflingId, - pub justified_epoch: Epoch, - pub finalized_epoch: Epoch, + pub justified_checkpoint: Checkpoint, + pub finalized_checkpoint: Checkpoint, /// Indicates if an execution node has marked this block as valid. Also contains the execution /// block hash. pub execution_status: ExecutionStatus, @@ -109,33 +109,33 @@ impl ProtoArrayForkChoice { pub fn new( finalized_block_slot: Slot, finalized_block_state_root: Hash256, - justified_epoch: Epoch, - finalized_epoch: Epoch, - finalized_root: Hash256, + justified_checkpoint: Checkpoint, + finalized_checkpoint: Checkpoint, current_epoch_shuffling_id: AttestationShufflingId, next_epoch_shuffling_id: AttestationShufflingId, execution_status: ExecutionStatus, ) -> Result { let mut proto_array = ProtoArray { prune_threshold: DEFAULT_PRUNE_THRESHOLD, - justified_epoch, - finalized_epoch, + justified_checkpoint, + finalized_checkpoint, nodes: Vec::with_capacity(1), indices: HashMap::with_capacity(1), + previous_proposer_boost: ProposerBoost::default(), }; let block = Block { slot: finalized_block_slot, - root: finalized_root, + root: finalized_checkpoint.root, parent_root: None, state_root: finalized_block_state_root, // We are using the finalized_root as the target_root, since it always lies on an // epoch boundary. - target_root: finalized_root, + target_root: finalized_checkpoint.root, current_epoch_shuffling_id, next_epoch_shuffling_id, - justified_epoch, - finalized_epoch, + justified_checkpoint, + finalized_checkpoint, execution_status, }; @@ -176,12 +176,13 @@ impl ProtoArrayForkChoice { .map_err(|e| format!("process_block_error: {:?}", e)) } - pub fn find_head( + pub fn find_head( &mut self, - justified_epoch: Epoch, - justified_root: Hash256, - finalized_epoch: Epoch, + justified_checkpoint: Checkpoint, + finalized_checkpoint: Checkpoint, justified_state_balances: &[u64], + proposer_boost_root: Hash256, + spec: &ChainSpec, ) -> Result { let old_balances = &mut self.balances; @@ -196,13 +197,20 @@ impl ProtoArrayForkChoice { .map_err(|e| format!("find_head compute_deltas failed: {:?}", e))?; self.proto_array - .apply_score_changes(deltas, justified_epoch, finalized_epoch) + .apply_score_changes::( + deltas, + justified_checkpoint, + finalized_checkpoint, + new_balances, + proposer_boost_root, + spec, + ) .map_err(|e| format!("find_head apply_score_changes failed: {:?}", e))?; *old_balances = new_balances.to_vec(); self.proto_array - .find_head(&justified_root) + .find_head(&justified_checkpoint.root) .map_err(|e| format!("find_head failed: {:?}", e)) } @@ -236,18 +244,27 @@ impl ProtoArrayForkChoice { .and_then(|i| self.proto_array.nodes.get(i)) .map(|parent| parent.root); - Some(Block { - slot: block.slot, - root: block.root, - parent_root, - state_root: block.state_root, - target_root: block.target_root, - current_epoch_shuffling_id: block.current_epoch_shuffling_id.clone(), - next_epoch_shuffling_id: block.next_epoch_shuffling_id.clone(), - justified_epoch: block.justified_epoch, - finalized_epoch: block.finalized_epoch, - execution_status: block.execution_status, - }) + // If a node does not have a `finalized_checkpoint` or `justified_checkpoint` populated, + // it means it is not a descendant of the finalized checkpoint, so it is valid to return + // `None` here. + if let (Some(justified_checkpoint), Some(finalized_checkpoint)) = + (block.justified_checkpoint, block.finalized_checkpoint) + { + Some(Block { + slot: block.slot, + root: block.root, + parent_root, + state_root: block.state_root, + target_root: block.target_root, + current_epoch_shuffling_id: block.current_epoch_shuffling_id.clone(), + next_epoch_shuffling_id: block.next_epoch_shuffling_id.clone(), + justified_checkpoint, + finalized_checkpoint, + execution_status: block.execution_status, + }) + } else { + None + } } /// Returns `true` if the `descendant_root` has an ancestor with `ancestor_root`. Always @@ -295,28 +312,19 @@ impl ProtoArrayForkChoice { .map_err(|e| format!("Failed to decode ProtoArrayForkChoice: {:?}", e)) } - /// Only used for SSZ deserialization of the persisted fork choice during the database migration - /// from schema 5 to schema 6. - pub fn from_bytes_legacy(bytes: &[u8]) -> Result { - LegacySszContainer::from_ssz_bytes(bytes) - .map(|legacy_container| { - let container: SszContainer = legacy_container.into(); - container.into() - }) - .map_err(|e| { - format!( - "Failed to decode ProtoArrayForkChoice during schema migration: {:?}", - e - ) - }) - } - /// Returns a read-lock to core `ProtoArray` struct. /// /// Should only be used when encoding/decoding during troubleshooting. pub fn core_proto_array(&self) -> &ProtoArray { &self.proto_array } + + /// Returns a mutable reference to the core `ProtoArray` struct. + /// + /// Should only be used during database schema migrations. + pub fn core_proto_array_mut(&mut self) -> &mut ProtoArray { + &mut self.proto_array + } } /// Returns a list of `deltas`, where there is one delta for each of the indices in @@ -412,12 +420,16 @@ mod test_compute_deltas { AttestationShufflingId::from_components(Epoch::new(0), Hash256::zero()); let execution_status = ExecutionStatus::irrelevant(); + let genesis_checkpoint = Checkpoint { + epoch: genesis_epoch, + root: finalized_root, + }; + let mut fc = ProtoArrayForkChoice::new( genesis_slot, state_root, - genesis_epoch, - genesis_epoch, - finalized_root, + genesis_checkpoint, + genesis_checkpoint, junk_shuffling_id.clone(), junk_shuffling_id.clone(), execution_status, @@ -434,8 +446,8 @@ mod test_compute_deltas { target_root: finalized_root, current_epoch_shuffling_id: junk_shuffling_id.clone(), next_epoch_shuffling_id: junk_shuffling_id.clone(), - justified_epoch: genesis_epoch, - finalized_epoch: genesis_epoch, + justified_checkpoint: genesis_checkpoint, + finalized_checkpoint: genesis_checkpoint, execution_status, }) .unwrap(); @@ -450,8 +462,8 @@ mod test_compute_deltas { target_root: finalized_root, current_epoch_shuffling_id: junk_shuffling_id.clone(), next_epoch_shuffling_id: junk_shuffling_id, - justified_epoch: genesis_epoch, - finalized_epoch: genesis_epoch, + justified_checkpoint: genesis_checkpoint, + finalized_checkpoint: genesis_checkpoint, execution_status, }) .unwrap(); diff --git a/consensus/proto_array/src/ssz_container.rs b/consensus/proto_array/src/ssz_container.rs index a0aaf1941..7f7ef79fe 100644 --- a/consensus/proto_array/src/ssz_container.rs +++ b/consensus/proto_array/src/ssz_container.rs @@ -1,50 +1,27 @@ -use crate::proto_array::LegacyProtoNode; +use crate::proto_array::ProposerBoost; use crate::{ proto_array::{ProtoArray, ProtoNode}, proto_array_fork_choice::{ElasticList, ProtoArrayForkChoice, VoteTracker}, }; +use ssz::{four_byte_option_impl, Encode}; use ssz_derive::{Decode, Encode}; use std::collections::HashMap; -use types::{Epoch, Hash256}; +use types::{Checkpoint, Hash256}; + +// Define a "legacy" implementation of `Option` which uses four bytes for encoding the union +// selector. +four_byte_option_impl!(four_byte_option_checkpoint, Checkpoint); #[derive(Encode, Decode)] pub struct SszContainer { - votes: Vec, - balances: Vec, - prune_threshold: usize, - justified_epoch: Epoch, - finalized_epoch: Epoch, - nodes: Vec, - indices: Vec<(Hash256, usize)>, -} - -/// Only used for SSZ deserialization of the persisted fork choice during the database migration -/// from schema 5 to schema 6. -#[derive(Encode, Decode)] -pub struct LegacySszContainer { - votes: Vec, - balances: Vec, - prune_threshold: usize, - justified_epoch: Epoch, - finalized_epoch: Epoch, - nodes: Vec, - indices: Vec<(Hash256, usize)>, -} - -impl Into for LegacySszContainer { - fn into(self) -> SszContainer { - let nodes = self.nodes.into_iter().map(Into::into).collect(); - - SszContainer { - votes: self.votes, - balances: self.balances, - prune_threshold: self.prune_threshold, - justified_epoch: self.justified_epoch, - finalized_epoch: self.finalized_epoch, - nodes, - indices: self.indices, - } - } + pub votes: Vec, + pub balances: Vec, + pub prune_threshold: usize, + pub justified_checkpoint: Checkpoint, + pub finalized_checkpoint: Checkpoint, + pub nodes: Vec, + pub indices: Vec<(Hash256, usize)>, + pub previous_proposer_boost: ProposerBoost, } impl From<&ProtoArrayForkChoice> for SszContainer { @@ -55,10 +32,11 @@ impl From<&ProtoArrayForkChoice> for SszContainer { votes: from.votes.0.clone(), balances: from.balances.clone(), prune_threshold: proto_array.prune_threshold, - justified_epoch: proto_array.justified_epoch, - finalized_epoch: proto_array.finalized_epoch, + justified_checkpoint: proto_array.justified_checkpoint, + finalized_checkpoint: proto_array.finalized_checkpoint, nodes: proto_array.nodes.clone(), indices: proto_array.indices.iter().map(|(k, v)| (*k, *v)).collect(), + previous_proposer_boost: proto_array.previous_proposer_boost, } } } @@ -67,10 +45,11 @@ impl From for ProtoArrayForkChoice { fn from(from: SszContainer) -> Self { let proto_array = ProtoArray { prune_threshold: from.prune_threshold, - justified_epoch: from.justified_epoch, - finalized_epoch: from.finalized_epoch, + justified_checkpoint: from.justified_checkpoint, + finalized_checkpoint: from.finalized_checkpoint, nodes: from.nodes, indices: from.indices.into_iter().collect::>(), + previous_proposer_boost: from.previous_proposer_boost, }; Self { diff --git a/consensus/types/Cargo.toml b/consensus/types/Cargo.toml index aa3c6c32c..f62fcf599 100644 --- a/consensus/types/Cargo.toml +++ b/consensus/types/Cargo.toml @@ -43,7 +43,7 @@ regex = "1.3.9" lazy_static = "1.4.0" parking_lot = "0.11.1" itertools = "0.10.0" -superstruct = "0.2.0" +superstruct = "0.3.0" [dev-dependencies] criterion = "0.3.3" diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index e5eabec20..68a5175a9 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -101,6 +101,7 @@ pub struct ChainSpec { * Fork choice */ pub safe_slots_to_update_justified: u64, + pub proposer_score_boost: Option, /* * Eth1 @@ -489,6 +490,7 @@ impl ChainSpec { * Fork choice */ safe_slots_to_update_justified: 8, + proposer_score_boost: None, /* * Eth1 @@ -657,6 +659,8 @@ pub struct Config { #[serde(with = "eth2_serde_utils::quoted_u64")] churn_limit_quotient: u64, + proposer_score_boost: Option>, + #[serde(with = "eth2_serde_utils::quoted_u64")] deposit_chain_id: u64, #[serde(with = "eth2_serde_utils::quoted_u64")] @@ -746,6 +750,8 @@ impl Config { churn_limit_quotient: spec.churn_limit_quotient, min_per_epoch_churn_limit: spec.min_per_epoch_churn_limit, + proposer_score_boost: spec.proposer_score_boost.map(|value| MaybeQuoted { value }), + deposit_chain_id: spec.deposit_chain_id, deposit_network_id: spec.deposit_network_id, deposit_contract_address: spec.deposit_contract_address, @@ -784,6 +790,7 @@ impl Config { ejection_balance, min_per_epoch_churn_limit, churn_limit_quotient, + proposer_score_boost, deposit_chain_id, deposit_network_id, deposit_contract_address, @@ -812,6 +819,7 @@ impl Config { ejection_balance, min_per_epoch_churn_limit, churn_limit_quotient, + proposer_score_boost: proposer_score_boost.map(|q| q.value), deposit_chain_id, deposit_network_id, deposit_contract_address, diff --git a/consensus/types/src/consts.rs b/consensus/types/src/consts.rs index 04e8e60ee..a9377bc3e 100644 --- a/consensus/types/src/consts.rs +++ b/consensus/types/src/consts.rs @@ -19,3 +19,6 @@ pub mod altair { pub const NUM_FLAG_INDICES: usize = 3; } +pub mod merge { + pub const INTERVALS_PER_SLOT: u64 = 3; +} diff --git a/lighthouse/environment/tests/testnet_dir/config.yaml b/lighthouse/environment/tests/testnet_dir/config.yaml index c06e89653..ac5403efd 100644 --- a/lighthouse/environment/tests/testnet_dir/config.yaml +++ b/lighthouse/environment/tests/testnet_dir/config.yaml @@ -70,6 +70,10 @@ MIN_PER_EPOCH_CHURN_LIMIT: 4 # 2**16 (= 65,536) CHURN_LIMIT_QUOTIENT: 65536 +# Fork choice +# --------------------------------------------------------------- +# 70% +PROPOSER_SCORE_BOOST: 70 # Deposit contract # --------------------------------------------------------------- diff --git a/testing/ef_tests/check_all_files_accessed.py b/testing/ef_tests/check_all_files_accessed.py index b1dfbdb4f..ce9e1d6b4 100755 --- a/testing/ef_tests/check_all_files_accessed.py +++ b/testing/ef_tests/check_all_files_accessed.py @@ -39,19 +39,9 @@ excluded_paths = [ "tests/minimal/altair/merkle/single_proof", "tests/mainnet/merge/merkle/single_proof", "tests/minimal/merge/merkle/single_proof", - # Temporarily disabled due to addition of proposer boosting. - # - # These tests will be reintroduced in: - # https://github.com/sigp/lighthouse/pull/2822 - "tests/minimal/phase0/fork_choice", - "tests/minimal/altair/fork_choice", - "tests/minimal/merge/fork_choice", - "tests/mainnet/phase0/fork_choice", - "tests/mainnet/altair/fork_choice", - "tests/mainnet/merge/fork_choice", - # Tests yet to be implemented. - "tests/mainnet/merge/transition", - "tests/minimal/merge/transition", + # FIXME(merge): Merge transition tests are now available but not yet passing + "tests/mainnet/merge/transition/", + "tests/minimal/merge/transition/", ] def normalize_path(path): diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 682fa8146..ecdfebc28 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -1,6 +1,7 @@ use super::*; use crate::decode::{ssz_decode_file, ssz_decode_file_with, ssz_decode_state, yaml_decode_file}; use ::fork_choice::PayloadVerificationStatus; +use beacon_chain::slot_clock::SlotClock; use beacon_chain::{ attestation_verification::{ obtain_indexed_attestation_and_committees_per_slot, VerifiedAttestation, @@ -23,10 +24,6 @@ pub struct PowBlock { pub block_hash: Hash256, pub parent_hash: Hash256, pub total_difficulty: Uint256, - // This field is not used and I expect it to be removed. See: - // - // https://github.com/ethereum/consensus-specs/pull/2720 - pub difficulty: Uint256, } #[derive(Debug, Clone, Copy, PartialEq, Deserialize)] @@ -46,6 +43,7 @@ pub struct Checks { justified_checkpoint_root: Option, finalized_checkpoint: Option, best_justified_checkpoint: Option, + proposer_boost_root: Option, } #[derive(Debug, Clone, Deserialize)] @@ -74,6 +72,15 @@ pub struct ForkChoiceTest { pub steps: Vec, Attestation, PowBlock>>, } +/// Spec for fork choice tests, with proposer boosting enabled. +/// +/// This function can be deleted once `ChainSpec::mainnet` enables proposer boosting by default. +pub fn fork_choice_spec(fork_name: ForkName) -> ChainSpec { + let mut spec = testing_spec::(fork_name); + spec.proposer_score_boost = Some(70); + spec +} + impl LoadCase for ForkChoiceTest { fn load_from_dir(path: &Path, fork_name: ForkName) -> Result { let description = path @@ -83,7 +90,7 @@ impl LoadCase for ForkChoiceTest { .to_str() .expect("path must be valid OsStr") .to_string(); - let spec = &testing_spec::(fork_name); + let spec = &fork_choice_spec::(fork_name); let steps: Vec> = yaml_decode_file(&path.join("steps.yaml"))?; // Resolve the object names in `steps.yaml` into actual decoded block/attestation objects. let steps = steps @@ -145,18 +152,15 @@ impl Case for ForkChoiceTest { } fn result(&self, _case_index: usize, fork_name: ForkName) -> Result<(), Error> { - let tester = Tester::new(self, testing_spec::(fork_name))?; + let tester = Tester::new(self, fork_choice_spec::(fork_name))?; - // The reason for this failure is documented here: + // TODO(merge): enable these tests before production. + // This test will fail until this PR is merged and released: // - // https://github.com/sigp/lighthouse/issues/2741 - // - // We should eventually solve the above issue and remove this `SkippedKnownFailure`. - if self.description == "new_finalized_slot_is_justified_checkpoint_ancestor" + // https://github.com/ethereum/consensus-specs/pull/2760 + if self.description == "shorter_chain_but_heavier_weight" // This test is skipped until we can do retrospective confirmations of the terminal // block after an optimistic sync. - // - // TODO(merge): enable this test before production. || self.description == "block_lookup_failed" { return Err(Error::SkippedKnownFailure); @@ -180,6 +184,7 @@ impl Case for ForkChoiceTest { justified_checkpoint_root, finalized_checkpoint, best_justified_checkpoint, + proposer_boost_root, } = checks.as_ref(); if let Some(expected_head) = head { @@ -211,6 +216,10 @@ impl Case for ForkChoiceTest { tester .check_best_justified_checkpoint(*expected_best_justified_checkpoint)?; } + + if let Some(expected_proposer_boost_root) = proposer_boost_root { + tester.check_expected_proposer_boost_root(*expected_proposer_boost_root)?; + } } } } @@ -352,11 +361,19 @@ impl Tester { ) .unwrap(); + let block_delay = self + .harness + .chain + .slot_clock + .seconds_from_current_slot_start(self.spec.seconds_per_slot) + .unwrap(); + let (block, _) = block.deconstruct(); let result = self.harness.chain.fork_choice.write().on_block( self.harness.chain.slot().unwrap(), &block, block_root, + block_delay, &state, PayloadVerificationStatus::Irrelevant, &self.harness.chain.spec, @@ -494,6 +511,18 @@ impl Tester { expected_checkpoint, ) } + + pub fn check_expected_proposer_boost_root( + &self, + expected_proposer_boost_root: Hash256, + ) -> Result<(), Error> { + let proposer_boost_root = self.harness.chain.fork_choice.read().proposer_boost_root(); + check_equal( + "proposer_boost_root", + proposer_boost_root, + expected_proposer_boost_root, + ) + } } /// Checks that the `head` checkpoint from the beacon chain head matches the `fc` checkpoint gleaned diff --git a/testing/ef_tests/src/cases/transition.rs b/testing/ef_tests/src/cases/transition.rs index 5d8fa1434..6ac56858a 100644 --- a/testing/ef_tests/src/cases/transition.rs +++ b/testing/ef_tests/src/cases/transition.rs @@ -72,7 +72,7 @@ impl Case for TransitionTest { fn is_enabled_for_fork(fork_name: ForkName) -> bool { // Upgrades exist targeting all forks except phase0/base. // Transition tests also need BLS. - // FIXME(merge): enable merge tests once available + // FIXME(merge): Merge transition tests are now available but not yet passing cfg!(not(feature = "fake_crypto")) && fork_name != ForkName::Base && fork_name != ForkName::Merge diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index a74f0a0ba..2201bc5ee 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -411,12 +411,6 @@ fn finality() { FinalityHandler::::default().run(); } -/* - * Temporarily disabled due to addition of proposer boosting. - * - * These tests will be reintroduced in: - * https://github.com/sigp/lighthouse/pull/2822 - * #[test] fn fork_choice_get_head() { ForkChoiceGetHeadHandler::::default().run(); @@ -434,7 +428,6 @@ fn fork_choice_on_merge_block() { ForkChoiceOnMergeBlockHandler::::default().run(); ForkChoiceOnMergeBlockHandler::::default().run(); } -*/ #[test] fn genesis_initialization() {