From 7d067a7258e8e47e84b7016130711bb9fff6f1e2 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 27 Jan 2023 14:54:08 +0100 Subject: [PATCH 01/60] Add new finalized checkpoint function --- consensus/proto_array/src/proto_array.rs | 54 ++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index add84f54787..b8a1bb8d7d9 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -990,6 +990,60 @@ impl ProtoArray { .unwrap_or(false) } + /// Returns `true` if the `descendant_root` has an ancestor with `ancestor_root`. Always + /// returns `false` if either input root is unknown. + /// + /// ## Notes + /// + /// Still returns `true` if `ancestor_root` is known and `ancestor_root == descendant_root`. + pub fn is_finalized_descendant(&self, root: Hash256) -> bool { + let finalized_root = self.finalized_checkpoint.root; + let finalized_slot = self + .finalized_checkpoint + .epoch + .start_slot(E::slots_per_epoch()); + + let mut node = if let Some(node) = self + .indices + .get(&root) + .and_then(|index| self.nodes.get(*index)) + { + node + } else { + // An unknown root is not a finalized descendant. This line can only + // be reached if the user supplies a root that is not known to fork + // choice. + return false; + }; + + loop { + // If `node` is less than or equal to the finalized slot then `node` + // must be the finalized block. + if node.slot <= finalized_slot { + return node.root == finalized_root; + } + + // Load the parent from the database since `node` is prior to the + // finalized checkpoint. + let parent = if let Some(parent) = node.parent.and_then(|index| self.nodes.get(index)) { + parent + } else { + // If `node` is not the finalized block and its parent does not + // exist in fork choice, then the parent must have been pruned. + // This indicates that the parent is not in the finalized chain. + return false; + }; + + // If the parent is equal to or less than the finalized slot, then + // it must be the finalized root. + if parent.slot <= finalized_slot { + return node.root == finalized_root; + } + + node = parent + } + } + /// Returns the first *beacon block root* which contains an execution payload with the given /// `block_hash`, if any. pub fn execution_block_hash_to_beacon_block_root( From c82ef87310943c1a4210d30ed88e574a6c1f7506 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 27 Jan 2023 16:05:11 +0100 Subject: [PATCH 02/60] Add test --- consensus/proto_array/src/proto_array.rs | 2 +- .../src/proto_array_fork_choice.rs | 120 ++++++++++++++++++ 2 files changed, 121 insertions(+), 1 deletion(-) diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index b8a1bb8d7d9..9f37eb0530a 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -1037,7 +1037,7 @@ impl ProtoArray { // If the parent is equal to or less than the finalized slot, then // it must be the finalized root. if parent.slot <= finalized_slot { - return node.root == finalized_root; + return parent.root == finalized_root; } node = parent diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index cbd369ae6ec..d952ea5c8ae 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -1004,6 +1004,126 @@ mod test_compute_deltas { assert!(!fc.is_descendant(not_finalized_desc, unknown)); } + #[test] + fn finalized_descendant_edge_case() { + let get_block_root = |i| Hash256::from_low_u64_be(i); + let genesis_slot = Slot::new(0); + let junk_state_root = Hash256::zero(); + let junk_shuffling_id = + AttestationShufflingId::from_components(Epoch::new(0), Hash256::zero()); + let execution_status = ExecutionStatus::irrelevant(); + + let genesis_checkpoint = Checkpoint { + epoch: Epoch::new(0), + root: get_block_root(0), + }; + + let mut fc = ProtoArrayForkChoice::new::( + genesis_slot, + junk_state_root, + genesis_checkpoint, + genesis_checkpoint, + junk_shuffling_id.clone(), + junk_shuffling_id.clone(), + execution_status, + CountUnrealizedFull::default(), + ) + .unwrap(); + + struct TestBlock { + slot: u64, + root: u64, + parent_root: u64, + } + + let mut insert_block = |fc: &mut ProtoArrayForkChoice, block: TestBlock| { + fc.proto_array + .on_block::( + Block { + slot: Slot::from(block.slot), + root: get_block_root(block.root), + parent_root: Some(get_block_root(block.parent_root)), + state_root: Hash256::zero(), + target_root: Hash256::zero(), + current_epoch_shuffling_id: junk_shuffling_id.clone(), + next_epoch_shuffling_id: junk_shuffling_id.clone(), + justified_checkpoint: Checkpoint { + epoch: Epoch::new(0), + root: get_block_root(0), + }, + finalized_checkpoint: genesis_checkpoint, + execution_status, + unrealized_justified_checkpoint: Some(genesis_checkpoint), + unrealized_finalized_checkpoint: Some(genesis_checkpoint), + }, + Slot::from(block.slot), + ) + .unwrap(); + }; + + // Produce the 0th epoch of blocks. They should all form a chain from + // the genesis block. + for i in 1..MainnetEthSpec::slots_per_epoch() { + insert_block( + &mut fc, + TestBlock { + slot: i, + root: i, + parent_root: i - 1, + }, + ) + } + + let last_slot_of_epoch_0 = MainnetEthSpec::slots_per_epoch() - 1; + + // Produce a block that descends from the last block of epoch -. + // + // This block will be non-canonical. + let non_canonical_slot = last_slot_of_epoch_0 + 1; + insert_block( + &mut fc, + TestBlock { + slot: non_canonical_slot, + root: non_canonical_slot, + parent_root: non_canonical_slot - 1, + }, + ); + + // Produce a block that descends from the last block of the 0th epoch, + // that skips the 1st slot of the 1st epoch. + // + // This block will be canonical. + let canonical_slot = last_slot_of_epoch_0 + 2; + insert_block( + &mut fc, + TestBlock { + slot: canonical_slot, + root: canonical_slot, + parent_root: non_canonical_slot - 1, + }, + ); + + // Set the finalized checkpoint to finalize the first slot of epoch 1 on + // the canonical chain. + fc.proto_array.finalized_checkpoint = Checkpoint { + root: get_block_root(last_slot_of_epoch_0), + epoch: Epoch::new(1), + }; + + assert!( + fc.proto_array + .is_finalized_descendant::(get_block_root(canonical_slot)), + "the canonical block is a descendant of the finalized checkpoint" + ); + assert_eq!( + fc.proto_array + .is_finalized_descendant::(get_block_root(non_canonical_slot)), + false, + "although the non-canonical block is a descendant of the finalized block, \ + it's not a descendant of the finalized checkpoint" + ); + } + #[test] fn zero_hash() { let validator_count: usize = 16; From 791cf47de1d9eb88cb255c073710d81d9dbb70a9 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 27 Jan 2023 16:07:37 +0100 Subject: [PATCH 03/60] Tidy --- consensus/proto_array/src/proto_array_fork_choice.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index d952ea5c8ae..03219dbe344 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -1103,10 +1103,12 @@ mod test_compute_deltas { }, ); + let finalized_root = get_block_root(last_slot_of_epoch_0); + // Set the finalized checkpoint to finalize the first slot of epoch 1 on // the canonical chain. fc.proto_array.finalized_checkpoint = Checkpoint { - root: get_block_root(last_slot_of_epoch_0), + root: finalized_root, epoch: Epoch::new(1), }; From bff90287f0c58b5a8ab3c5d47587524effe51774 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 27 Jan 2023 16:20:23 +0100 Subject: [PATCH 04/60] Use new method --- consensus/fork_choice/src/fork_choice.rs | 3 +-- consensus/proto_array/src/proto_array.rs | 1 + consensus/proto_array/src/proto_array_fork_choice.rs | 11 +++++++++++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 290cef78ab5..f6cbc3a3a90 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -1374,8 +1374,7 @@ where /// Return `true` if `block_root` is equal to the finalized root, or a known descendant of it. pub fn is_descendant_of_finalized(&self, block_root: Hash256) -> bool { - self.proto_array - .is_descendant(self.fc_store.finalized_checkpoint().root, block_root) + self.proto_array.is_finalized_descendant::(block_root) } /// Returns `Ok(true)` if `block_root` has been imported optimistically or deemed invalid. diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 9f37eb0530a..f95e6efb74b 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -482,6 +482,7 @@ impl ProtoArray { let latest_valid_ancestor_is_descendant = latest_valid_ancestor_root.map_or(false, |ancestor_root| { self.is_descendant(ancestor_root, head_block_root) + // TODO(paul): should this be updated to the new method? && self.is_descendant(self.finalized_checkpoint.root, ancestor_root) }); diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 03219dbe344..9b9879b0579 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -748,6 +748,12 @@ impl ProtoArrayForkChoice { .is_descendant(ancestor_root, descendant_root) } + /// See `ProtoArray` documentation. + pub fn is_finalized_descendant(&self, descendant_root: Hash256) -> bool { + self.proto_array + .is_finalized_descendant::(descendant_root) + } + pub fn latest_message(&self, validator_index: usize) -> Option<(Hash256, Epoch)> { if validator_index < self.votes.0.len() { let vote = &self.votes.0[validator_index]; @@ -993,6 +999,11 @@ mod test_compute_deltas { assert!(!fc.is_descendant(finalized_root, not_finalized_desc)); assert!(!fc.is_descendant(finalized_root, unknown)); + assert!(fc.is_finalized_descendant::(finalized_root)); + assert!(fc.is_finalized_descendant::(finalized_desc)); + assert!(!fc.is_finalized_descendant::(not_finalized_desc)); + assert!(!fc.is_finalized_descendant::(unknown)); + assert!(!fc.is_descendant(finalized_desc, not_finalized_desc)); assert!(fc.is_descendant(finalized_desc, finalized_desc)); assert!(!fc.is_descendant(finalized_desc, finalized_root)); From a119edc739e9dcefe1cb800a2ce9eb4baab55f20 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 27 Jan 2023 17:35:30 +0100 Subject: [PATCH 05/60] Add descriptive comment --- .../src/proto_array_fork_choice.rs | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 9b9879b0579..342a76403d8 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -1015,6 +1015,36 @@ mod test_compute_deltas { assert!(!fc.is_descendant(not_finalized_desc, unknown)); } + /// This test covers an interesting case where a block can be a descendant + /// of the finalized *block*, but not a descenant of the finalized + /// *checkpoint*. + /// + /// ## Example + /// + /// Consider this block tree which has three blocks (`A`, `B` and `C`): + /// + /// ```ignore + /// [A] <--- [-] <--- [B] + /// | + /// |--[C] + /// ``` + /// + /// - `A` (slot 31) is the common descendant. + /// - `B` (slot 33) descends from `A`, but there is a single skip slot + /// between it and `A`. + /// - `C` (slot 32) descends from `A` and conflicts with `B`. + /// + /// Imagine that the `B` chain is finalized at epoch 1. This means that the + /// finalized checkpoint points to the skipped slot at 32. The root of the + /// finalized checkpoint is `A`. + /// + /// In this scenario, the block `C` has the finalized root (`A`) as an + /// ancestor whilst simultaneously conflicting with the finalized + /// checkpoint. + /// + /// This means that to ensure a block does not conflict with finality we + /// must check to ensure that it's an ancestor of the finalized + /// *checkpoint*, not just the finalized *block*. #[test] fn finalized_descendant_edge_case() { let get_block_root = |i| Hash256::from_low_u64_be(i); @@ -1072,6 +1102,10 @@ mod test_compute_deltas { .unwrap(); }; + /* + * Start of interesting part of tests. + */ + // Produce the 0th epoch of blocks. They should all form a chain from // the genesis block. for i in 1..MainnetEthSpec::slots_per_epoch() { From 6590da58d22d1b33706a785c51c375d68b8832a5 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 27 Jan 2023 17:54:25 +0100 Subject: [PATCH 06/60] Fix clippy lints --- consensus/proto_array/src/proto_array_fork_choice.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 342a76403d8..1c4c5d5a4d9 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -1047,7 +1047,7 @@ mod test_compute_deltas { /// *checkpoint*, not just the finalized *block*. #[test] fn finalized_descendant_edge_case() { - let get_block_root = |i| Hash256::from_low_u64_be(i); + let get_block_root = Hash256::from_low_u64_be; let genesis_slot = Slot::new(0); let junk_state_root = Hash256::zero(); let junk_shuffling_id = @@ -1077,7 +1077,7 @@ mod test_compute_deltas { parent_root: u64, } - let mut insert_block = |fc: &mut ProtoArrayForkChoice, block: TestBlock| { + let insert_block = |fc: &mut ProtoArrayForkChoice, block: TestBlock| { fc.proto_array .on_block::( Block { @@ -1162,10 +1162,9 @@ mod test_compute_deltas { .is_finalized_descendant::(get_block_root(canonical_slot)), "the canonical block is a descendant of the finalized checkpoint" ); - assert_eq!( - fc.proto_array + assert!( + !fc.proto_array .is_finalized_descendant::(get_block_root(non_canonical_slot)), - false, "although the non-canonical block is a descendant of the finalized block, \ it's not a descendant of the finalized checkpoint" ); From c977fab516ed2ef9620d6012ded3f28580efa993 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 27 Jan 2023 17:57:36 +0100 Subject: [PATCH 07/60] Rename function --- beacon_node/beacon_chain/src/beacon_chain.rs | 4 ++-- .../beacon_chain/src/block_verification.rs | 6 +++--- consensus/fork_choice/src/fork_choice.rs | 16 ++++++++------- consensus/proto_array/src/proto_array.rs | 2 +- .../src/proto_array_fork_choice.rs | 20 +++++++++++-------- 5 files changed, 27 insertions(+), 21 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 3366e1364cf..1e3142f6694 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -8,7 +8,7 @@ use crate::beacon_proposer_cache::compute_proposer_duties_from_head; use crate::beacon_proposer_cache::BeaconProposerCache; use crate::block_times_cache::BlockTimesCache; use crate::block_verification::{ - check_block_is_finalized_descendant, check_block_relevancy, get_block_root, + check_block_is_finalized_checkpoint_descendant, check_block_relevancy, get_block_root, signature_verify_chain_segment, BlockError, ExecutionPendingBlock, GossipVerifiedBlock, IntoExecutionPendingBlock, PayloadVerificationOutcome, POS_PANDA_BANNER, }; @@ -2736,7 +2736,7 @@ impl BeaconChain { let mut fork_choice = self.canonical_head.fork_choice_write_lock(); // Do not import a block that doesn't descend from the finalized root. - check_block_is_finalized_descendant(self, &fork_choice, &signed_block)?; + check_block_is_finalized_checkpoint_descendant(self, &fork_choice, &signed_block)?; // Register the new block with the fork choice service. { diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index ad08bd9f4f3..75ebdee4d25 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -744,7 +744,7 @@ impl GossipVerifiedBlock { // Do not process a block that doesn't descend from the finalized root. // // We check this *before* we load the parent so that we can return a more detailed error. - check_block_is_finalized_descendant( + check_block_is_finalized_checkpoint_descendant( chain, &chain.canonical_head.fork_choice_write_lock(), &block, @@ -1564,12 +1564,12 @@ fn check_block_against_finalized_slot( /// ## Warning /// /// Taking a lock on the `chain.canonical_head.fork_choice` might cause a deadlock here. -pub fn check_block_is_finalized_descendant( +pub fn check_block_is_finalized_checkpoint_descendant( chain: &BeaconChain, fork_choice: &BeaconForkChoice, block: &Arc>, ) -> Result<(), BlockError> { - if fork_choice.is_descendant_of_finalized(block.parent_root()) { + if fork_choice.is_descendant_of_finalized_checkpoint(block.parent_root()) { Ok(()) } else { // If fork choice does *not* consider the parent to be a descendant of the finalized block, diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index f6cbc3a3a90..a4a08938a41 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -1282,7 +1282,7 @@ where if store.best_justified_checkpoint().epoch > store.justified_checkpoint().epoch { let store = &self.fc_store; - if self.is_descendant_of_finalized(store.best_justified_checkpoint().root) { + if self.is_descendant_of_finalized_checkpoint(store.best_justified_checkpoint().root) { let store = &mut self.fc_store; store .set_justified_checkpoint(*store.best_justified_checkpoint()) @@ -1323,12 +1323,13 @@ where /// Returns `true` if the block is known **and** a descendant of the finalized root. pub fn contains_block(&self, block_root: &Hash256) -> bool { - self.proto_array.contains_block(block_root) && self.is_descendant_of_finalized(*block_root) + self.proto_array.contains_block(block_root) + && self.is_descendant_of_finalized_checkpoint(*block_root) } /// Returns a `ProtoBlock` if the block is known **and** a descendant of the finalized root. pub fn get_block(&self, block_root: &Hash256) -> Option { - if self.is_descendant_of_finalized(*block_root) { + if self.is_descendant_of_finalized_checkpoint(*block_root) { self.proto_array.get_block(block_root) } else { None @@ -1337,7 +1338,7 @@ where /// Returns an `ExecutionStatus` if the block is known **and** a descendant of the finalized root. pub fn get_block_execution_status(&self, block_root: &Hash256) -> Option { - if self.is_descendant_of_finalized(*block_root) { + if self.is_descendant_of_finalized_checkpoint(*block_root) { self.proto_array.get_block_execution_status(block_root) } else { None @@ -1372,9 +1373,10 @@ where }) } - /// Return `true` if `block_root` is equal to the finalized root, or a known descendant of it. - pub fn is_descendant_of_finalized(&self, block_root: Hash256) -> bool { - self.proto_array.is_finalized_descendant::(block_root) + /// Return `true` if `block_root` is equal to the finalized checkpoint, or a known descendant of it. + pub fn is_descendant_of_finalized_checkpoint(&self, block_root: Hash256) -> bool { + self.proto_array + .is_finalized_checkpoint_descendant::(block_root) } /// Returns `Ok(true)` if `block_root` has been imported optimistically or deemed invalid. diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index f95e6efb74b..3cd3a28cb25 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -997,7 +997,7 @@ impl ProtoArray { /// ## Notes /// /// Still returns `true` if `ancestor_root` is known and `ancestor_root == descendant_root`. - pub fn is_finalized_descendant(&self, root: Hash256) -> bool { + pub fn is_finalized_checkpoint_descendant(&self, root: Hash256) -> bool { let finalized_root = self.finalized_checkpoint.root; let finalized_slot = self .finalized_checkpoint diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 1c4c5d5a4d9..b0b7f1d4614 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -749,9 +749,9 @@ impl ProtoArrayForkChoice { } /// See `ProtoArray` documentation. - pub fn is_finalized_descendant(&self, descendant_root: Hash256) -> bool { + pub fn is_finalized_checkpoint_descendant(&self, descendant_root: Hash256) -> bool { self.proto_array - .is_finalized_descendant::(descendant_root) + .is_finalized_checkpoint_descendant::(descendant_root) } pub fn latest_message(&self, validator_index: usize) -> Option<(Hash256, Epoch)> { @@ -999,10 +999,10 @@ mod test_compute_deltas { assert!(!fc.is_descendant(finalized_root, not_finalized_desc)); assert!(!fc.is_descendant(finalized_root, unknown)); - assert!(fc.is_finalized_descendant::(finalized_root)); - assert!(fc.is_finalized_descendant::(finalized_desc)); - assert!(!fc.is_finalized_descendant::(not_finalized_desc)); - assert!(!fc.is_finalized_descendant::(unknown)); + assert!(fc.is_finalized_checkpoint_descendant::(finalized_root)); + assert!(fc.is_finalized_checkpoint_descendant::(finalized_desc)); + assert!(!fc.is_finalized_checkpoint_descendant::(not_finalized_desc)); + assert!(!fc.is_finalized_checkpoint_descendant::(unknown)); assert!(!fc.is_descendant(finalized_desc, not_finalized_desc)); assert!(fc.is_descendant(finalized_desc, finalized_desc)); @@ -1159,12 +1159,16 @@ mod test_compute_deltas { assert!( fc.proto_array - .is_finalized_descendant::(get_block_root(canonical_slot)), + .is_finalized_checkpoint_descendant::(get_block_root( + canonical_slot + )), "the canonical block is a descendant of the finalized checkpoint" ); assert!( !fc.proto_array - .is_finalized_descendant::(get_block_root(non_canonical_slot)), + .is_finalized_checkpoint_descendant::(get_block_root( + non_canonical_slot + )), "although the non-canonical block is a descendant of the finalized block, \ it's not a descendant of the finalized checkpoint" ); From b306f6751778d78f546cc34975ef6f28811d5de7 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 9 Feb 2023 12:06:58 +1100 Subject: [PATCH 08/60] Rename function, add test condition --- beacon_node/beacon_chain/src/beacon_chain.rs | 4 +-- .../beacon_chain/src/block_verification.rs | 4 +-- consensus/fork_choice/src/fork_choice.rs | 2 +- consensus/proto_array/src/proto_array.rs | 2 +- .../src/proto_array_fork_choice.rs | 25 +++++++++++++------ 5 files changed, 23 insertions(+), 14 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 1e3142f6694..6a67ae71ed2 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -8,7 +8,7 @@ use crate::beacon_proposer_cache::compute_proposer_duties_from_head; use crate::beacon_proposer_cache::BeaconProposerCache; use crate::block_times_cache::BlockTimesCache; use crate::block_verification::{ - check_block_is_finalized_checkpoint_descendant, check_block_relevancy, get_block_root, + check_block_is_finalized_checkpoint_or_descendant, check_block_relevancy, get_block_root, signature_verify_chain_segment, BlockError, ExecutionPendingBlock, GossipVerifiedBlock, IntoExecutionPendingBlock, PayloadVerificationOutcome, POS_PANDA_BANNER, }; @@ -2736,7 +2736,7 @@ impl BeaconChain { let mut fork_choice = self.canonical_head.fork_choice_write_lock(); // Do not import a block that doesn't descend from the finalized root. - check_block_is_finalized_checkpoint_descendant(self, &fork_choice, &signed_block)?; + check_block_is_finalized_checkpoint_or_descendant(self, &fork_choice, &signed_block)?; // Register the new block with the fork choice service. { diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 75ebdee4d25..a0f57d321b4 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -744,7 +744,7 @@ impl GossipVerifiedBlock { // Do not process a block that doesn't descend from the finalized root. // // We check this *before* we load the parent so that we can return a more detailed error. - check_block_is_finalized_checkpoint_descendant( + check_block_is_finalized_checkpoint_or_descendant( chain, &chain.canonical_head.fork_choice_write_lock(), &block, @@ -1564,7 +1564,7 @@ fn check_block_against_finalized_slot( /// ## Warning /// /// Taking a lock on the `chain.canonical_head.fork_choice` might cause a deadlock here. -pub fn check_block_is_finalized_checkpoint_descendant( +pub fn check_block_is_finalized_checkpoint_or_descendant( chain: &BeaconChain, fork_choice: &BeaconForkChoice, block: &Arc>, diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index a4a08938a41..5b283c442e3 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -1376,7 +1376,7 @@ where /// Return `true` if `block_root` is equal to the finalized checkpoint, or a known descendant of it. pub fn is_descendant_of_finalized_checkpoint(&self, block_root: Hash256) -> bool { self.proto_array - .is_finalized_checkpoint_descendant::(block_root) + .is_finalized_checkpoint_or_descendant::(block_root) } /// Returns `Ok(true)` if `block_root` has been imported optimistically or deemed invalid. diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 3cd3a28cb25..39fb33af6a7 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -997,7 +997,7 @@ impl ProtoArray { /// ## Notes /// /// Still returns `true` if `ancestor_root` is known and `ancestor_root == descendant_root`. - pub fn is_finalized_checkpoint_descendant(&self, root: Hash256) -> bool { + pub fn is_finalized_checkpoint_or_descendant(&self, root: Hash256) -> bool { let finalized_root = self.finalized_checkpoint.root; let finalized_slot = self .finalized_checkpoint diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index b0b7f1d4614..0fdebd5d67e 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -749,9 +749,12 @@ impl ProtoArrayForkChoice { } /// See `ProtoArray` documentation. - pub fn is_finalized_checkpoint_descendant(&self, descendant_root: Hash256) -> bool { + pub fn is_finalized_checkpoint_or_descendant( + &self, + descendant_root: Hash256, + ) -> bool { self.proto_array - .is_finalized_checkpoint_descendant::(descendant_root) + .is_finalized_checkpoint_or_descendant::(descendant_root) } pub fn latest_message(&self, validator_index: usize) -> Option<(Hash256, Epoch)> { @@ -999,10 +1002,10 @@ mod test_compute_deltas { assert!(!fc.is_descendant(finalized_root, not_finalized_desc)); assert!(!fc.is_descendant(finalized_root, unknown)); - assert!(fc.is_finalized_checkpoint_descendant::(finalized_root)); - assert!(fc.is_finalized_checkpoint_descendant::(finalized_desc)); - assert!(!fc.is_finalized_checkpoint_descendant::(not_finalized_desc)); - assert!(!fc.is_finalized_checkpoint_descendant::(unknown)); + assert!(fc.is_finalized_checkpoint_or_descendant::(finalized_root)); + assert!(fc.is_finalized_checkpoint_or_descendant::(finalized_desc)); + assert!(!fc.is_finalized_checkpoint_or_descendant::(not_finalized_desc)); + assert!(!fc.is_finalized_checkpoint_or_descendant::(unknown)); assert!(!fc.is_descendant(finalized_desc, not_finalized_desc)); assert!(fc.is_descendant(finalized_desc, finalized_desc)); @@ -1159,14 +1162,20 @@ mod test_compute_deltas { assert!( fc.proto_array - .is_finalized_checkpoint_descendant::(get_block_root( + .is_finalized_checkpoint_or_descendant::(finalized_root), + "the finalized checkpoint is the finalized checkpoint" + ); + + assert!( + fc.proto_array + .is_finalized_checkpoint_or_descendant::(get_block_root( canonical_slot )), "the canonical block is a descendant of the finalized checkpoint" ); assert!( !fc.proto_array - .is_finalized_checkpoint_descendant::(get_block_root( + .is_finalized_checkpoint_or_descendant::(get_block_root( non_canonical_slot )), "although the non-canonical block is a descendant of the finalized block, \ From 95721355b084bf09e3cc83b93a7c3e5b34aa759f Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 9 Feb 2023 12:07:07 +1100 Subject: [PATCH 09/60] Add shortcut method --- consensus/proto_array/src/proto_array.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 39fb33af6a7..9857ba8b070 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -1017,6 +1017,14 @@ impl ProtoArray { return false; }; + // A shortcut method for when we already know that a node is descenant of the finalized checkpoint. + if node + .finalized_checkpoint + .map_or(false, |cp| cp == self.finalized_checkpoint) + { + return true; + } + loop { // If `node` is less than or equal to the finalized slot then `node` // must be the finalized block. From e2312d2680ebde4afcd9724240313ffe1d3cf052 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 9 Feb 2023 12:23:26 +1100 Subject: [PATCH 10/60] Improve loop-shortcutting method --- consensus/proto_array/src/proto_array.rs | 39 ++++++++++--------- .../src/proto_array_fork_choice.rs | 10 ++++- 2 files changed, 29 insertions(+), 20 deletions(-) diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 9857ba8b070..012edcce3a3 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -1017,12 +1017,22 @@ impl ProtoArray { return false; }; - // A shortcut method for when we already know that a node is descenant of the finalized checkpoint. - if node - .finalized_checkpoint - .map_or(false, |cp| cp == self.finalized_checkpoint) - { - return true; + // The finalized and justified checkpoints represent a list of known + // ancestors of `node` that are likely to coincide with the store's + // finalized checkpoint. + // + // Don't continue checking these values for ancestors. If they don't + // match for the child then they're unlikely to start matching for its + // ancestors. + for checkpoint in &[ + node.finalized_checkpoint, + node.justified_checkpoint, + node.unrealized_finalized_checkpoint, + node.unrealized_justified_checkpoint, + ] { + if checkpoint.map_or(false, |cp| cp == self.finalized_checkpoint) { + return true; + } } loop { @@ -1032,24 +1042,17 @@ impl ProtoArray { return node.root == finalized_root; } - // Load the parent from the database since `node` is prior to the - // finalized checkpoint. - let parent = if let Some(parent) = node.parent.and_then(|index| self.nodes.get(index)) { - parent + // Since `node` is from a higher slot that the finalized checkpoint, + // replace `node` with the parent of `node`. + if let Some(parent) = node.parent.and_then(|index| self.nodes.get(index)) { + node = parent } else { // If `node` is not the finalized block and its parent does not // exist in fork choice, then the parent must have been pruned. + // Since fork choice only prunes // This indicates that the parent is not in the finalized chain. return false; }; - - // If the parent is equal to or less than the finalized slot, then - // it must be the finalized root. - if parent.slot <= finalized_slot { - return parent.root == finalized_root; - } - - node = parent } } diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 0fdebd5d67e..69de0c09ebd 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -937,6 +937,10 @@ mod test_compute_deltas { epoch: genesis_epoch, root: finalized_root, }; + let junk_checkpoint = Checkpoint { + epoch: Epoch::new(42), + root: Hash256::repeat_byte(42), + }; let mut fc = ProtoArrayForkChoice::new::( genesis_slot, @@ -982,8 +986,10 @@ mod test_compute_deltas { target_root: finalized_root, current_epoch_shuffling_id: junk_shuffling_id.clone(), next_epoch_shuffling_id: junk_shuffling_id, - justified_checkpoint: genesis_checkpoint, - finalized_checkpoint: genesis_checkpoint, + // Use the junk checkpoint for the next to values to prevent + // the loop-shortcutting mechanism from triggering. + justified_checkpoint: junk_checkpoint, + finalized_checkpoint: junk_checkpoint, execution_status, unrealized_justified_checkpoint: None, unrealized_finalized_checkpoint: None, From 89d7fd377332978398bbab175a4fa2f75d4d9996 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 9 Feb 2023 12:28:21 +1100 Subject: [PATCH 11/60] Update comments --- consensus/proto_array/src/proto_array.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 012edcce3a3..c5305453dfe 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -978,6 +978,12 @@ impl ProtoArray { /// ## Notes /// /// Still returns `true` if `ancestor_root` is known and `ancestor_root == descendant_root`. + /// + /// ## Warning + /// + /// Do not use this function to check if a block is a descendant of the + /// finalized checkpoint. Use `Self::is_finalized_checkpoint_or_descendant` + /// instead. pub fn is_descendant(&self, ancestor_root: Hash256, descendant_root: Hash256) -> bool { self.indices .get(&ancestor_root) @@ -991,12 +997,11 @@ impl ProtoArray { .unwrap_or(false) } - /// Returns `true` if the `descendant_root` has an ancestor with `ancestor_root`. Always - /// returns `false` if either input root is unknown. + /// Returns `true` if `root` is equal to or a descendant of + /// `self.finalized_checkpoint`. /// - /// ## Notes - /// - /// Still returns `true` if `ancestor_root` is known and `ancestor_root == descendant_root`. + /// Notably, this function is checking ancestory of the finalized + /// *checkpoint* not the finalized *block*. pub fn is_finalized_checkpoint_or_descendant(&self, root: Hash256) -> bool { let finalized_root = self.finalized_checkpoint.root; let finalized_slot = self From dfc223cf2482d9de71cdba0474a8d4a396271116 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 9 Feb 2023 12:30:17 +1100 Subject: [PATCH 12/60] Update fork_choice function name --- beacon_node/beacon_chain/src/block_verification.rs | 2 +- consensus/fork_choice/src/fork_choice.rs | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index a0f57d321b4..4f65a05c56b 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -1569,7 +1569,7 @@ pub fn check_block_is_finalized_checkpoint_or_descendant( fork_choice: &BeaconForkChoice, block: &Arc>, ) -> Result<(), BlockError> { - if fork_choice.is_descendant_of_finalized_checkpoint(block.parent_root()) { + if fork_choice.is_finalized_checkpoint_or_descendant(block.parent_root()) { Ok(()) } else { // If fork choice does *not* consider the parent to be a descendant of the finalized block, diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 5b283c442e3..a5198b5f8a4 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -1282,7 +1282,7 @@ where if store.best_justified_checkpoint().epoch > store.justified_checkpoint().epoch { let store = &self.fc_store; - if self.is_descendant_of_finalized_checkpoint(store.best_justified_checkpoint().root) { + if self.is_finalized_checkpoint_or_descendant(store.best_justified_checkpoint().root) { let store = &mut self.fc_store; store .set_justified_checkpoint(*store.best_justified_checkpoint()) @@ -1324,12 +1324,12 @@ where /// Returns `true` if the block is known **and** a descendant of the finalized root. pub fn contains_block(&self, block_root: &Hash256) -> bool { self.proto_array.contains_block(block_root) - && self.is_descendant_of_finalized_checkpoint(*block_root) + && self.is_finalized_checkpoint_or_descendant(*block_root) } /// Returns a `ProtoBlock` if the block is known **and** a descendant of the finalized root. pub fn get_block(&self, block_root: &Hash256) -> Option { - if self.is_descendant_of_finalized_checkpoint(*block_root) { + if self.is_finalized_checkpoint_or_descendant(*block_root) { self.proto_array.get_block(block_root) } else { None @@ -1338,7 +1338,7 @@ where /// Returns an `ExecutionStatus` if the block is known **and** a descendant of the finalized root. pub fn get_block_execution_status(&self, block_root: &Hash256) -> Option { - if self.is_descendant_of_finalized_checkpoint(*block_root) { + if self.is_finalized_checkpoint_or_descendant(*block_root) { self.proto_array.get_block_execution_status(block_root) } else { None @@ -1374,7 +1374,7 @@ where } /// Return `true` if `block_root` is equal to the finalized checkpoint, or a known descendant of it. - pub fn is_descendant_of_finalized_checkpoint(&self, block_root: Hash256) -> bool { + pub fn is_finalized_checkpoint_or_descendant(&self, block_root: Hash256) -> bool { self.proto_array .is_finalized_checkpoint_or_descendant::(block_root) } From 788ecd655eed87d2dd0405e22bfc6b45043d1da7 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 9 Feb 2023 12:36:50 +1100 Subject: [PATCH 13/60] Fix todo --- consensus/fork_choice/src/fork_choice.rs | 2 +- consensus/proto_array/src/fork_choice_test_definition.rs | 2 +- consensus/proto_array/src/proto_array.rs | 5 ++--- consensus/proto_array/src/proto_array_fork_choice.rs | 4 ++-- 4 files changed, 6 insertions(+), 7 deletions(-) diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index a5198b5f8a4..afae7f058b4 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -721,7 +721,7 @@ where op: &InvalidationOperation, ) -> Result<(), Error> { self.proto_array - .process_execution_payload_invalidation(op) + .process_execution_payload_invalidation::(op) .map_err(Error::FailedToProcessInvalidExecutionPayload) } diff --git a/consensus/proto_array/src/fork_choice_test_definition.rs b/consensus/proto_array/src/fork_choice_test_definition.rs index 035fb799eea..68b3fb71981 100644 --- a/consensus/proto_array/src/fork_choice_test_definition.rs +++ b/consensus/proto_array/src/fork_choice_test_definition.rs @@ -273,7 +273,7 @@ impl ForkChoiceTestDefinition { } }; fork_choice - .process_execution_payload_invalidation(&op) + .process_execution_payload_invalidation::(&op) .unwrap() } Operation::AssertWeight { block_root, weight } => assert_eq!( diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index c5305453dfe..36eb81f8556 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -451,7 +451,7 @@ impl ProtoArray { /// Invalidate zero or more blocks, as specified by the `InvalidationOperation`. /// /// See the documentation of `InvalidationOperation` for usage. - pub fn propagate_execution_payload_invalidation( + pub fn propagate_execution_payload_invalidation( &mut self, op: &InvalidationOperation, ) -> Result<(), Error> { @@ -482,8 +482,7 @@ impl ProtoArray { let latest_valid_ancestor_is_descendant = latest_valid_ancestor_root.map_or(false, |ancestor_root| { self.is_descendant(ancestor_root, head_block_root) - // TODO(paul): should this be updated to the new method? - && self.is_descendant(self.finalized_checkpoint.root, ancestor_root) + && self.is_finalized_checkpoint_or_descendant::(ancestor_root) }); // Collect all *ancestors* which were declared invalid since they reside between the diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 69de0c09ebd..0e0d806e76e 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -358,12 +358,12 @@ impl ProtoArrayForkChoice { } /// See `ProtoArray::propagate_execution_payload_invalidation` for documentation. - pub fn process_execution_payload_invalidation( + pub fn process_execution_payload_invalidation( &mut self, op: &InvalidationOperation, ) -> Result<(), String> { self.proto_array - .propagate_execution_payload_invalidation(op) + .propagate_execution_payload_invalidation::(op) .map_err(|e| format!("Failed to process invalid payload: {:?}", e)) } From 0261cdacf71b3a390ae17fb3a163c92efa8d6fc5 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 9 Feb 2023 15:28:02 +1100 Subject: [PATCH 14/60] Fix comment --- consensus/proto_array/src/proto_array.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 36eb81f8556..ba82526499b 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -1053,8 +1053,8 @@ impl ProtoArray { } else { // If `node` is not the finalized block and its parent does not // exist in fork choice, then the parent must have been pruned. - // Since fork choice only prunes - // This indicates that the parent is not in the finalized chain. + // Proto-array only prunes blocks prior to the finalized block, + // so this means the parent conflicts with finality. return false; }; } From f35d44beaf4dda8089ff7c9a8f5b412d7e80166c Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 1 Feb 2023 15:45:00 +1100 Subject: [PATCH 15/60] Add some custom fc tests --- .gitignore | 1 + testing/ef_tests/.gitignore | 1 + testing/ef_tests/Makefile | 17 +++++++++++ testing/ef_tests/src/handler.rs | 14 +++++++-- testing/ef_tests/tests/tests.rs | 54 +++++++++++++++++++++------------ 5 files changed, 65 insertions(+), 22 deletions(-) diff --git a/.gitignore b/.gitignore index ae9f83c46dd..b1fb8faf6cc 100644 --- a/.gitignore +++ b/.gitignore @@ -9,6 +9,7 @@ perf.data* /bin genesis.ssz /clippy.toml +*.DS_Store # IntelliJ /*.iml diff --git a/testing/ef_tests/.gitignore b/testing/ef_tests/.gitignore index 6a2ca1fe754..10d36e13f06 100644 --- a/testing/ef_tests/.gitignore +++ b/testing/ef_tests/.gitignore @@ -1,3 +1,4 @@ /consensus-spec-tests .accessed_file_log.txt /bls12-381-tests +/custom-fc-tests diff --git a/testing/ef_tests/Makefile b/testing/ef_tests/Makefile index b2af490dd0e..faf7a6c9fc6 100644 --- a/testing/ef_tests/Makefile +++ b/testing/ef_tests/Makefile @@ -2,6 +2,18 @@ TESTS_TAG := v1.2.0 TESTS = general minimal mainnet TARBALLS = $(patsubst %,%-$(TESTS_TAG).tar.gz,$(TESTS)) +# Custom fork choice tests from https://github.com/adiasg/consensus-specs +# +# Since these tests are private, we can't download them in this Makefile +# without adding an API key into the process, so we're just downloading +# the files manually. These files can be obtained from: +# https://github.com/adiasg/consensus-specs/pull/18#issuecomment-1411070772 +CUSTOM_FC_TESTS_TAG = 076551b +CUSTOM_FC_TESTS_MAINNET_ARCHIVE = $(CUSTOM_FC_TESTS_TAG)-mainnet.tar.gz +CUSTOM_FC_TESTS_MINIMAL_ARCHIVE = $(CUSTOM_FC_TESTS_TAG)-minimal.tar.gz +CUSTOM_FC_TESTS_REPO_NAME := custom-fc-tests +CUSTOM_FC_TESTS_OUTPUT_DIR := ./$(CUSTOM_FC_TESTS_REPO_NAME) + REPO_NAME := consensus-spec-tests OUTPUT_DIR := ./$(REPO_NAME) BASE_URL := https://github.com/ethereum/$(REPO_NAME)/releases/download/$(TESTS_TAG) @@ -33,6 +45,11 @@ $(BLS_OUTPUT_DIR): %-$(TESTS_TAG).tar.gz: $(WGET) $(BASE_URL)/$*.tar.gz -O $@ +custom-fc-tests: + mkdir $(CUSTOM_FC_TESTS_OUTPUT_DIR) + tar -xzf $(CUSTOM_FC_TESTS_MAINNET_ARCHIVE) -C $(CUSTOM_FC_TESTS_OUTPUT_DIR); + tar -xzf $(CUSTOM_FC_TESTS_MINIMAL_ARCHIVE) -C $(CUSTOM_FC_TESTS_OUTPUT_DIR); + clean-test-files: rm -rf $(OUTPUT_DIR) $(BLS_OUTPUT_DIR) diff --git a/testing/ef_tests/src/handler.rs b/testing/ef_tests/src/handler.rs index 13f70fea716..18d433e8d44 100644 --- a/testing/ef_tests/src/handler.rs +++ b/testing/ef_tests/src/handler.rs @@ -18,6 +18,10 @@ pub trait Handler { fn handler_name(&self) -> String; + fn tests_dir(&self) -> String { + "consensus-spec-tests".to_string() + } + fn is_enabled_for_fork(&self, fork_name: ForkName) -> bool { Self::Case::is_enabled_for_fork(fork_name) } @@ -38,7 +42,7 @@ pub trait Handler { let fork_name_str = fork_name.to_string(); let handler_path = PathBuf::from(env!("CARGO_MANIFEST_DIR")) - .join("consensus-spec-tests") + .join(&self.tests_dir()) .join("tests") .join(Self::config_name()) .join(&fork_name_str) @@ -499,13 +503,15 @@ impl Handler for FinalityHandler { } pub struct ForkChoiceHandler { + tests_dir: String, handler_name: String, _phantom: PhantomData, } impl ForkChoiceHandler { - pub fn new(handler_name: &str) -> Self { + pub fn new(tests_dir: &str, handler_name: &str) -> Self { Self { + tests_dir: tests_dir.into(), handler_name: handler_name.into(), _phantom: PhantomData, } @@ -523,6 +529,10 @@ impl Handler for ForkChoiceHandler { "fork_choice" } + fn tests_dir(&self) -> String { + self.tests_dir.clone() + } + fn handler_name(&self) -> String { self.handler_name.clone() } diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index 87a6bec71b5..d4cb7026807 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -1,8 +1,11 @@ -#![cfg(feature = "ef_tests")] +// #![cfg(feature = "ef_tests")] use ef_tests::*; use types::*; +const CONSENSUS_SPEC_TESTS: &str = "consensus-spec-tests"; +const CUSTOM_FC_TESTS: &str = "custom-fc-tests"; + // Check that the hand-computed multiplications on EthSpec are correctly computed. // This test lives here because one is most likely to muck these up during a spec update. fn check_typenum_values() { @@ -424,30 +427,41 @@ fn finality() { FinalityHandler::::default().run(); } -#[test] -fn fork_choice_get_head() { - ForkChoiceHandler::::new("get_head").run(); - ForkChoiceHandler::::new("get_head").run(); -} +macro_rules! fork_choice_tests { + ($mod: ident, $tests_dir: ident) => { + mod $mod { + use super::*; -#[test] -fn fork_choice_on_block() { - ForkChoiceHandler::::new("on_block").run(); - ForkChoiceHandler::::new("on_block").run(); -} + #[test] + fn fork_choice_get_head() { + ForkChoiceHandler::::new($tests_dir, "get_head").run(); + ForkChoiceHandler::::new($tests_dir, "get_head").run(); + } -#[test] -fn fork_choice_on_merge_block() { - ForkChoiceHandler::::new("on_merge_block").run(); - ForkChoiceHandler::::new("on_merge_block").run(); -} + #[test] + fn fork_choice_on_block() { + ForkChoiceHandler::::new($tests_dir, "on_block").run(); + ForkChoiceHandler::::new($tests_dir, "on_block").run(); + } -#[test] -fn fork_choice_ex_ante() { - ForkChoiceHandler::::new("ex_ante").run(); - ForkChoiceHandler::::new("ex_ante").run(); + #[test] + fn fork_choice_on_merge_block() { + ForkChoiceHandler::::new($tests_dir, "on_merge_block").run(); + ForkChoiceHandler::::new($tests_dir, "on_merge_block").run(); + } + + #[test] + fn fork_choice_ex_ante() { + ForkChoiceHandler::::new($tests_dir, "ex_ante").run(); + ForkChoiceHandler::::new($tests_dir, "ex_ante").run(); + } + } + }; } +fork_choice_tests!(fork_choice_standard, CONSENSUS_SPEC_TESTS); +fork_choice_tests!(fork_choice_custom, CUSTOM_FC_TESTS); + #[test] fn optimistic_sync() { OptimisticSyncHandler::::default().run(); From db112c5a561f977b5bf2c9470c521e1fa924ca44 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 1 Feb 2023 15:48:19 +1100 Subject: [PATCH 16/60] Add re-org test --- testing/ef_tests/tests/tests.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index d4cb7026807..24f16f3b290 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -462,6 +462,12 @@ macro_rules! fork_choice_tests { fork_choice_tests!(fork_choice_standard, CONSENSUS_SPEC_TESTS); fork_choice_tests!(fork_choice_custom, CUSTOM_FC_TESTS); +#[test] +fn fork_choice_custom_reorg() { + ForkChoiceHandler::::new(CUSTOM_FC_TESTS, "reorg").run(); + ForkChoiceHandler::::new(CUSTOM_FC_TESTS, "reorg").run(); +} + #[test] fn optimistic_sync() { OptimisticSyncHandler::::default().run(); From 656f958327208e53fae6f65210756d7c4f1704e2 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 2 Feb 2023 14:31:32 +1100 Subject: [PATCH 17/60] Remove should_update_justified_checkpoint --- consensus/fork_choice/src/fork_choice.rs | 67 ++---------------------- 1 file changed, 3 insertions(+), 64 deletions(-) diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index afae7f058b4..e7aec69cdad 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -653,58 +653,6 @@ where } } - /// Returns `true` if the given `store` should be updated to set - /// `state.current_justified_checkpoint` its `justified_checkpoint`. - /// - /// ## Specification - /// - /// Is equivalent to: - /// - /// https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/fork-choice.md#should_update_justified_checkpoint - fn should_update_justified_checkpoint( - &mut self, - new_justified_checkpoint: Checkpoint, - slots: UpdateJustifiedCheckpointSlots, - spec: &ChainSpec, - ) -> Result> { - self.update_time(slots.current_slot(), spec)?; - - if compute_slots_since_epoch_start::(self.fc_store.get_current_slot()) - < spec.safe_slots_to_update_justified - { - return Ok(true); - } - - let justified_slot = - compute_start_slot_at_epoch::(self.fc_store.justified_checkpoint().epoch); - - // This sanity check is not in the spec, but the invariant is implied. - if let Some(state_slot) = slots.state_slot() { - if justified_slot >= state_slot { - return Err(Error::AttemptToRevertJustification { - store: justified_slot, - state: state_slot, - }); - } - } - - // We know that the slot for `new_justified_checkpoint.root` is not greater than - // `state.slot`, since a state cannot justify its own slot. - // - // We know that `new_justified_checkpoint.root` is an ancestor of `state`, since a `state` - // only ever justifies ancestors. - // - // A prior `if` statement protects against a justified_slot that is greater than - // `state.slot` - let justified_ancestor = - self.get_ancestor(new_justified_checkpoint.root, justified_slot)?; - if justified_ancestor != Some(self.fc_store.justified_checkpoint().root) { - return Ok(false); - } - - Ok(true) - } - /// See `ProtoArrayForkChoice::process_execution_payload_validation` for documentation. pub fn on_valid_execution_payload( &mut self, @@ -1006,23 +954,14 @@ where ) -> Result<(), Error> { // Update justified checkpoint. if justified_checkpoint.epoch > self.fc_store.justified_checkpoint().epoch { - if justified_checkpoint.epoch > self.fc_store.best_justified_checkpoint().epoch { - self.fc_store - .set_best_justified_checkpoint(justified_checkpoint); - } - if self.should_update_justified_checkpoint(justified_checkpoint, slots, spec)? { - self.fc_store - .set_justified_checkpoint(justified_checkpoint) - .map_err(Error::UnableToSetJustifiedCheckpoint)?; - } + self.fc_store + .set_justified_checkpoint(justified_checkpoint) + .map_err(Error::UnableToSetJustifiedCheckpoint)?; } // Update finalized checkpoint. if finalized_checkpoint.epoch > self.fc_store.finalized_checkpoint().epoch { self.fc_store.set_finalized_checkpoint(finalized_checkpoint); - self.fc_store - .set_justified_checkpoint(justified_checkpoint) - .map_err(Error::UnableToSetJustifiedCheckpoint)?; } Ok(()) } From 0652e9b1e41a118f37a6868ca8e6d807bc21ec49 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 2 Feb 2023 15:08:17 +1100 Subject: [PATCH 18/60] Sketch in new filter logic --- consensus/proto_array/src/proto_array.rs | 43 ++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index ba82526499b..4da85f754f7 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -103,6 +103,22 @@ pub struct ProtoNode { pub unrealized_finalized_checkpoint: Option, } +impl ProtoNode { + fn voting_source(&self, current_epoch: Epoch, slots_per_epoch: u64) -> Option { + let node_epoch = self.slot.epoch(slots_per_epoch); + if current_epoch > node_epoch { + self.unrealized_justified_checkpoint + .or(self.justified_checkpoint) + } else { + self.justified_checkpoint + } + } + + fn unrealized_justification(&self) -> Option { + unimplemented!() + } +} + #[derive(PartialEq, Debug, Encode, Decode, Serialize, Deserialize, Copy, Clone)] pub struct ProposerBoost { pub root: Hash256, @@ -900,7 +916,33 @@ impl ProtoArray { } let genesis_epoch = Epoch::new(0); + let current_epoch = current_slot.epoch(E::slots_per_epoch()); + let voting_source = + if let Some(checkpoint) = node.voting_source(current_epoch, E::slots_per_epoch()) { + checkpoint + } else { + // The node does not have any information about the + // justified/finalized checkpoint. This indicates an inconsistent + // proto-array. + return false; + }; + + let mut correct_justified = self.justified_checkpoint.epoch == genesis_epoch + || voting_source.epoch == self.justified_checkpoint.epoch; + + if let Some(unrealized_justification) = node + .unrealized_justification() + .filter(|_| !correct_justified) + { + correct_justified = unrealized_justification.epoch >= self.justified_checkpoint.epoch + && voting_source.epoch + 2 >= current_epoch; + } + // TODO: finalized_checkpoint. + let correct_finalized = true; + + correct_justified && correct_finalized + /* let checkpoint_match_predicate = |node_justified_checkpoint: Checkpoint, node_finalized_checkpoint: Checkpoint| { let correct_justified = node_justified_checkpoint == self.justified_checkpoint @@ -949,6 +991,7 @@ impl ProtoArray { } else { false } + */ } /// Return a reverse iterator over the nodes which comprise the chain ending at `block_root`. From 011d6a814a338c0f186e9ba9b96e4deda3ff1c1b Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 7 Feb 2023 16:52:33 +1100 Subject: [PATCH 19/60] Fix clippy lints from unused safe-slots functions --- beacon_node/beacon_chain/src/beacon_chain.rs | 1 - .../beacon_chain/src/block_verification.rs | 1 - consensus/fork_choice/src/fork_choice.rs | 57 ++----------------- testing/ef_tests/src/cases/fork_choice.rs | 2 +- testing/ef_tests/src/handler.rs | 2 +- 5 files changed, 8 insertions(+), 55 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 6a67ae71ed2..5f31eaf11df 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1861,7 +1861,6 @@ impl BeaconChain { self.slot()?, verified.indexed_attestation(), AttestationFromBlock::False, - &self.spec, ) .map_err(Into::into) } diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 4f65a05c56b..121ad03342e 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -1467,7 +1467,6 @@ impl ExecutionPendingBlock { current_slot, indexed_attestation, AttestationFromBlock::True, - &chain.spec, ) { Ok(()) => Ok(()), // Ignore invalid attestations whilst importing attestations from a block. The diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index e7aec69cdad..e7956712e50 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -206,33 +206,6 @@ impl From for CountUnrealized { } } -#[derive(Copy, Clone)] -enum UpdateJustifiedCheckpointSlots { - OnTick { - current_slot: Slot, - }, - OnBlock { - state_slot: Slot, - current_slot: Slot, - }, -} - -impl UpdateJustifiedCheckpointSlots { - fn current_slot(&self) -> Slot { - match self { - UpdateJustifiedCheckpointSlots::OnTick { current_slot } => *current_slot, - UpdateJustifiedCheckpointSlots::OnBlock { current_slot, .. } => *current_slot, - } - } - - fn state_slot(&self) -> Option { - match self { - UpdateJustifiedCheckpointSlots::OnTick { .. } => None, - UpdateJustifiedCheckpointSlots::OnBlock { state_slot, .. } => Some(*state_slot), - } - } -} - /// Indicates if a block has been verified by an execution payload. /// /// There is no variant for "invalid", since such a block should never be added to fork choice. @@ -532,7 +505,7 @@ where // Provide the slot (as per the system clock) to the `fc_store` and then return its view of // the current slot. The `fc_store` will ensure that the `current_slot` is never // decreasing, a property which we must maintain. - let current_slot = self.update_time(system_time_current_slot, spec)?; + let current_slot = self.update_time(system_time_current_slot)?; let store = &mut self.fc_store; @@ -706,7 +679,7 @@ where // Provide the slot (as per the system clock) to the `fc_store` and then return its view of // the current slot. The `fc_store` will ensure that the `current_slot` is never // decreasing, a property which we must maintain. - let current_slot = self.update_time(system_time_current_slot, spec)?; + let current_slot = self.update_time(system_time_current_slot)?; // Parent block must be known. let parent_block = self @@ -761,17 +734,10 @@ where self.fc_store.set_proposer_boost_root(block_root); } - let update_justified_checkpoint_slots = UpdateJustifiedCheckpointSlots::OnBlock { - state_slot: state.slot(), - current_slot, - }; - // Update store with checkpoints if necessary self.update_checkpoints( state.current_justified_checkpoint(), state.finalized_checkpoint(), - update_justified_checkpoint_slots, - spec, )?; // Update unrealized justified/finalized checkpoints. @@ -853,8 +819,6 @@ where self.update_checkpoints( unrealized_justified_checkpoint, unrealized_finalized_checkpoint, - update_justified_checkpoint_slots, - spec, )?; } @@ -949,8 +913,6 @@ where &mut self, justified_checkpoint: Checkpoint, finalized_checkpoint: Checkpoint, - slots: UpdateJustifiedCheckpointSlots, - spec: &ChainSpec, ) -> Result<(), Error> { // Update justified checkpoint. if justified_checkpoint.epoch > self.fc_store.justified_checkpoint().epoch { @@ -1103,9 +1065,8 @@ where system_time_current_slot: Slot, attestation: &IndexedAttestation, is_from_block: AttestationFromBlock, - spec: &ChainSpec, ) -> Result<(), Error> { - self.update_time(system_time_current_slot, spec)?; + self.update_time(system_time_current_slot)?; // Ignore any attestations to the zero hash. // @@ -1166,16 +1127,12 @@ where /// Call `on_tick` for all slots between `fc_store.get_current_slot()` and the provided /// `current_slot`. Returns the value of `self.fc_store.get_current_slot`. - pub fn update_time( - &mut self, - current_slot: Slot, - spec: &ChainSpec, - ) -> Result> { + pub fn update_time(&mut self, current_slot: Slot) -> Result> { while self.fc_store.get_current_slot() < current_slot { let previous_slot = self.fc_store.get_current_slot(); // Note: we are relying upon `on_tick` to update `fc_store.time` to ensure we don't // get stuck in a loop. - self.on_tick(previous_slot + 1, spec)? + self.on_tick(previous_slot + 1)? } // Process any attestations that might now be eligible. @@ -1191,7 +1148,7 @@ where /// Equivalent to: /// /// https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/fork-choice.md#on_tick - fn on_tick(&mut self, time: Slot, spec: &ChainSpec) -> Result<(), Error> { + fn on_tick(&mut self, time: Slot) -> Result<(), Error> { let store = &mut self.fc_store; let previous_slot = store.get_current_slot(); @@ -1235,8 +1192,6 @@ where self.update_checkpoints( unrealized_justified_checkpoint, unrealized_finalized_checkpoint, - UpdateJustifiedCheckpointSlots::OnTick { current_slot }, - spec, )?; Ok(()) } diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 039efb36845..8560412dfd1 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -377,7 +377,7 @@ impl Tester { .chain .canonical_head .fork_choice_write_lock() - .update_time(slot, &self.spec) + .update_time(slot) .unwrap(); } diff --git a/testing/ef_tests/src/handler.rs b/testing/ef_tests/src/handler.rs index 18d433e8d44..e80b40aa8ac 100644 --- a/testing/ef_tests/src/handler.rs +++ b/testing/ef_tests/src/handler.rs @@ -42,7 +42,7 @@ pub trait Handler { let fork_name_str = fork_name.to_string(); let handler_path = PathBuf::from(env!("CARGO_MANIFEST_DIR")) - .join(&self.tests_dir()) + .join(self.tests_dir()) .join("tests") .join(Self::config_name()) .join(&fork_name_str) From b02753fcee058cf4aee4af2f88b810136239da6a Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 7 Feb 2023 17:15:33 +1100 Subject: [PATCH 20/60] Add more filter logic --- consensus/proto_array/src/proto_array.rs | 25 ++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 4da85f754f7..9b3181ebd67 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -113,10 +113,6 @@ impl ProtoNode { self.justified_checkpoint } } - - fn unrealized_justification(&self) -> Option { - unimplemented!() - } } #[derive(PartialEq, Debug, Encode, Decode, Serialize, Deserialize, Copy, Clone)] @@ -926,20 +922,25 @@ impl ProtoArray { // proto-array. return false; }; + let unrealized_justification = if self.justified_checkpoint.epoch + 1 == current_epoch { + node.unrealized_justified_checkpoint + } else { + None + }; let mut correct_justified = self.justified_checkpoint.epoch == genesis_epoch || voting_source.epoch == self.justified_checkpoint.epoch; - if let Some(unrealized_justification) = node - .unrealized_justification() - .filter(|_| !correct_justified) - { - correct_justified = unrealized_justification.epoch >= self.justified_checkpoint.epoch - && voting_source.epoch + 2 >= current_epoch; + if !correct_justified { + if let Some(unrealized_justification) = unrealized_justification { + correct_justified = unrealized_justification.epoch + >= self.justified_checkpoint.epoch + && voting_source.epoch + 2 >= current_epoch; + } } - // TODO: finalized_checkpoint. - let correct_finalized = true; + let correct_finalized = self.finalized_checkpoint.epoch == genesis_epoch + || self.is_finalized_checkpoint_descendant::(node.root); correct_justified && correct_finalized /* From 0ee032853dd3f50096331139dc34f453dc092a38 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 7 Feb 2023 17:25:05 +1100 Subject: [PATCH 21/60] Update tests commit --- testing/ef_tests/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/ef_tests/Makefile b/testing/ef_tests/Makefile index faf7a6c9fc6..242390df69f 100644 --- a/testing/ef_tests/Makefile +++ b/testing/ef_tests/Makefile @@ -8,7 +8,7 @@ TARBALLS = $(patsubst %,%-$(TESTS_TAG).tar.gz,$(TESTS)) # without adding an API key into the process, so we're just downloading # the files manually. These files can be obtained from: # https://github.com/adiasg/consensus-specs/pull/18#issuecomment-1411070772 -CUSTOM_FC_TESTS_TAG = 076551b +CUSTOM_FC_TESTS_TAG = 8e46b6f CUSTOM_FC_TESTS_MAINNET_ARCHIVE = $(CUSTOM_FC_TESTS_TAG)-mainnet.tar.gz CUSTOM_FC_TESTS_MINIMAL_ARCHIVE = $(CUSTOM_FC_TESTS_TAG)-minimal.tar.gz CUSTOM_FC_TESTS_REPO_NAME := custom-fc-tests From 9554274065be8dd4d81bc6b431d5d13d20c363f5 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 7 Feb 2023 17:25:12 +1100 Subject: [PATCH 22/60] Add withholding tests --- testing/ef_tests/tests/tests.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index 24f16f3b290..5735a5f692b 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -468,6 +468,12 @@ fn fork_choice_custom_reorg() { ForkChoiceHandler::::new(CUSTOM_FC_TESTS, "reorg").run(); } +#[test] +fn fork_choice_custom_withholding() { + ForkChoiceHandler::::new(CUSTOM_FC_TESTS, "withholding").run(); + ForkChoiceHandler::::new(CUSTOM_FC_TESTS, "withholding").run(); +} + #[test] fn optimistic_sync() { OptimisticSyncHandler::::default().run(); From 6af13e06a1911d76c1a6bdc600c82c9c1de556d7 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 7 Feb 2023 18:04:48 +1100 Subject: [PATCH 23/60] Enable CountUnrealized --- testing/ef_tests/src/cases/fork_choice.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 8560412dfd1..cafd5e179d5 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -387,7 +387,7 @@ impl Tester { let result = self.block_on_dangerous(self.harness.chain.process_block( block_root, block.clone(), - CountUnrealized::False, + CountUnrealized::True, NotifyExecutionLayer::Yes, ))?; if result.is_ok() != valid { From 8ad941debd9aded74084551c3dc2830258702b19 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 7 Feb 2023 18:37:12 +1100 Subject: [PATCH 24/60] Track slashed indices --- .../beacon_chain/src/beacon_fork_choice_store.rs | 7 ++++++- consensus/proto_array/src/justified_balances.rs | 14 +++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) 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 0b789b8b615..91ef57d8f50 100644 --- a/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs +++ b/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs @@ -11,6 +11,7 @@ use proto_array::JustifiedBalances; use safe_arith::ArithError; use ssz_derive::{Decode, Encode}; use std::collections::BTreeSet; +use std::iter::FromIterator; use std::marker::PhantomData; use std::sync::Arc; use store::{Error as StoreError, HotColdDB, ItemStore}; @@ -186,6 +187,8 @@ where }; let finalized_checkpoint = justified_checkpoint; let justified_balances = JustifiedBalances::from_justified_state(anchor_state)?; + let equivocating_indices = + BTreeSet::from_iter(justified_balances.slashed_indices.iter().copied()); Ok(Self { store, @@ -198,7 +201,7 @@ where unrealized_justified_checkpoint: justified_checkpoint, unrealized_finalized_checkpoint: finalized_checkpoint, proposer_boost_root: Hash256::zero(), - equivocating_indices: BTreeSet::new(), + equivocating_indices, _phantom: PhantomData, }) } @@ -328,6 +331,8 @@ where .ok_or_else(|| Error::MissingState(justified_block.state_root()))?; self.justified_balances = JustifiedBalances::from_justified_state(&state)?; + self.equivocating_indices + .extend(self.justified_balances.slashed_indices.iter().copied()); } Ok(()) diff --git a/consensus/proto_array/src/justified_balances.rs b/consensus/proto_array/src/justified_balances.rs index 75f6c2f7c80..6adda98f913 100644 --- a/consensus/proto_array/src/justified_balances.rs +++ b/consensus/proto_array/src/justified_balances.rs @@ -1,4 +1,5 @@ use safe_arith::{ArithError, SafeArith}; +use std::collections::HashSet; use types::{BeaconState, EthSpec}; #[derive(Debug, PartialEq, Clone, Default)] @@ -12,6 +13,8 @@ pub struct JustifiedBalances { pub total_effective_balance: u64, /// The number of active validators included in `self.effective_balances`. pub num_active_validators: u64, + /// The set of all slashed validators. + pub slashed_indices: HashSet, } impl JustifiedBalances { @@ -19,11 +22,17 @@ impl JustifiedBalances { let current_epoch = state.current_epoch(); let mut total_effective_balance = 0u64; let mut num_active_validators = 0u64; + let mut slashed_indices = HashSet::new(); let effective_balances = state .validators() .iter() - .map(|validator| { + .enumerate() + .map(|(validator_index, validator)| { + if validator.slashed { + slashed_indices.insert(validator_index as u64); + } + if validator.is_active_at(current_epoch) { total_effective_balance.safe_add_assign(validator.effective_balance)?; num_active_validators.safe_add_assign(1)?; @@ -39,6 +48,7 @@ impl JustifiedBalances { effective_balances, total_effective_balance, num_active_validators, + slashed_indices, }) } @@ -57,6 +67,8 @@ impl JustifiedBalances { effective_balances, total_effective_balance, num_active_validators, + // TODO(paul): is an empty set sensible? + slashed_indices: <_>::default(), }) } } From 7065c49e54716f3db3b031301f64f49878424480 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 8 Feb 2023 11:21:58 +1100 Subject: [PATCH 25/60] Tidy --- consensus/proto_array/src/proto_array.rs | 60 ++++-------------------- 1 file changed, 10 insertions(+), 50 deletions(-) diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 9b3181ebd67..dcfeb1da5c2 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -923,6 +923,16 @@ impl ProtoArray { return false; }; let unrealized_justification = if self.justified_checkpoint.epoch + 1 == current_epoch { + // This `unrealized_justified_checkpoint` is an `Option` that can be + // `None` in the scenario that we're not computing unrealized + // justification (UJ). During sync we avoid this computation as an + // optimisation. + // + // I claim that it is safe to always have `unrealized_justification + // == None` under the following conditions: + // + // 1. All competing heads do not have UJ computed. + // 2. The store is not updated as per UJ values. node.unrealized_justified_checkpoint } else { None @@ -943,56 +953,6 @@ impl ProtoArray { || self.is_finalized_checkpoint_descendant::(node.root); correct_justified && correct_finalized - /* - let checkpoint_match_predicate = - |node_justified_checkpoint: Checkpoint, node_finalized_checkpoint: Checkpoint| { - let correct_justified = node_justified_checkpoint == self.justified_checkpoint - || self.justified_checkpoint.epoch == genesis_epoch; - let correct_finalized = node_finalized_checkpoint == self.finalized_checkpoint - || self.finalized_checkpoint.epoch == genesis_epoch; - correct_justified && correct_finalized - }; - - if let ( - Some(unrealized_justified_checkpoint), - Some(unrealized_finalized_checkpoint), - Some(justified_checkpoint), - Some(finalized_checkpoint), - ) = ( - node.unrealized_justified_checkpoint, - node.unrealized_finalized_checkpoint, - node.justified_checkpoint, - node.finalized_checkpoint, - ) { - let current_epoch = current_slot.epoch(E::slots_per_epoch()); - - // If previous epoch is justified, pull up all tips to at least the previous epoch - if CountUnrealizedFull::True == self.count_unrealized_full - && (current_epoch > genesis_epoch - && self.justified_checkpoint.epoch + 1 == current_epoch) - { - unrealized_justified_checkpoint.epoch + 1 >= current_epoch - // If previous epoch is not justified, pull up only tips from past epochs up to the current epoch - } else { - // If block is from a previous epoch, filter using unrealized justification & finalization information - if node.slot.epoch(E::slots_per_epoch()) < current_epoch { - checkpoint_match_predicate( - unrealized_justified_checkpoint, - unrealized_finalized_checkpoint, - ) - // If block is from the current epoch, filter using the head state's justification & finalization information - } else { - checkpoint_match_predicate(justified_checkpoint, finalized_checkpoint) - } - } - } else if let (Some(justified_checkpoint), Some(finalized_checkpoint)) = - (node.justified_checkpoint, node.finalized_checkpoint) - { - checkpoint_match_predicate(justified_checkpoint, finalized_checkpoint) - } else { - false - } - */ } /// Return a reverse iterator over the nodes which comprise the chain ending at `block_root`. From e6d95dad214b9eb1bced69f09732b1ab811b2825 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 8 Feb 2023 11:38:27 +1100 Subject: [PATCH 26/60] Remove best justified checkpoint --- .../src/beacon_fork_choice_store.rs | 19 +++++++----------- consensus/fork_choice/src/fork_choice.rs | 20 ------------------- .../fork_choice/src/fork_choice_store.rs | 6 ------ consensus/fork_choice/tests/tests.rs | 10 ++++------ testing/ef_tests/src/cases/fork_choice.rs | 15 +++----------- 5 files changed, 14 insertions(+), 56 deletions(-) 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 91ef57d8f50..db3ae484a31 100644 --- a/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs +++ b/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs @@ -21,6 +21,11 @@ use types::{ Hash256, Slot, }; +const JUNK_CHECKPOINT: Checkpoint = Checkpoint { + epoch: Epoch::new(42), + root: Hash256::repeat_byte(42), +}; + #[derive(Debug)] pub enum Error { UnableToReadSlot, @@ -145,7 +150,6 @@ pub struct BeaconForkChoiceStore, Cold: ItemStore< finalized_checkpoint: Checkpoint, justified_checkpoint: Checkpoint, justified_balances: JustifiedBalances, - best_justified_checkpoint: Checkpoint, unrealized_justified_checkpoint: Checkpoint, unrealized_finalized_checkpoint: Checkpoint, proposer_boost_root: Hash256, @@ -197,7 +201,6 @@ where justified_checkpoint, justified_balances, finalized_checkpoint, - best_justified_checkpoint: justified_checkpoint, unrealized_justified_checkpoint: justified_checkpoint, unrealized_finalized_checkpoint: finalized_checkpoint, proposer_boost_root: Hash256::zero(), @@ -215,7 +218,8 @@ where finalized_checkpoint: self.finalized_checkpoint, justified_checkpoint: self.justified_checkpoint, justified_balances: self.justified_balances.effective_balances.clone(), - best_justified_checkpoint: self.best_justified_checkpoint, + // TODO(paul): consider a database migration here. + best_justified_checkpoint: JUNK_CHECKPOINT, unrealized_justified_checkpoint: self.unrealized_justified_checkpoint, unrealized_finalized_checkpoint: self.unrealized_finalized_checkpoint, proposer_boost_root: self.proposer_boost_root, @@ -237,7 +241,6 @@ where finalized_checkpoint: persisted.finalized_checkpoint, justified_checkpoint: persisted.justified_checkpoint, justified_balances, - best_justified_checkpoint: persisted.best_justified_checkpoint, unrealized_justified_checkpoint: persisted.unrealized_justified_checkpoint, unrealized_finalized_checkpoint: persisted.unrealized_finalized_checkpoint, proposer_boost_root: persisted.proposer_boost_root, @@ -280,10 +283,6 @@ where &self.justified_balances } - fn best_justified_checkpoint(&self) -> &Checkpoint { - &self.best_justified_checkpoint - } - fn finalized_checkpoint(&self) -> &Checkpoint { &self.finalized_checkpoint } @@ -338,10 +337,6 @@ where Ok(()) } - fn set_best_justified_checkpoint(&mut self, checkpoint: Checkpoint) { - self.best_justified_checkpoint = checkpoint - } - fn set_unrealized_justified_checkpoint(&mut self, checkpoint: Checkpoint) { self.unrealized_justified_checkpoint = checkpoint; } diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index e7956712e50..507d921cbc7 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -1176,16 +1176,6 @@ where return Ok(()); } - if store.best_justified_checkpoint().epoch > store.justified_checkpoint().epoch { - let store = &self.fc_store; - if self.is_finalized_checkpoint_or_descendant(store.best_justified_checkpoint().root) { - let store = &mut self.fc_store; - store - .set_justified_checkpoint(*store.best_justified_checkpoint()) - .map_err(Error::ForkChoiceStoreError)?; - } - } - // Update store.justified_checkpoint if a better unrealized justified checkpoint is known let unrealized_justified_checkpoint = *self.fc_store.unrealized_justified_checkpoint(); let unrealized_finalized_checkpoint = *self.fc_store.unrealized_finalized_checkpoint(); @@ -1359,16 +1349,6 @@ where *self.fc_store.justified_checkpoint() } - /// Return the best justified checkpoint. - /// - /// ## Warning - /// - /// This is distinct to the "justified checkpoint" or the "current justified checkpoint". This - /// "best justified checkpoint" value should only be used internally or for testing. - pub fn best_justified_checkpoint(&self) -> Checkpoint { - *self.fc_store.best_justified_checkpoint() - } - pub fn unrealized_justified_checkpoint(&self) -> Checkpoint { *self.fc_store.unrealized_justified_checkpoint() } diff --git a/consensus/fork_choice/src/fork_choice_store.rs b/consensus/fork_choice/src/fork_choice_store.rs index 60c58859ed8..749fce2d111 100644 --- a/consensus/fork_choice/src/fork_choice_store.rs +++ b/consensus/fork_choice/src/fork_choice_store.rs @@ -47,9 +47,6 @@ pub trait ForkChoiceStore: Sized { /// Returns balances from the `state` identified by `justified_checkpoint.root`. fn justified_balances(&self) -> &JustifiedBalances; - /// Returns the `best_justified_checkpoint`. - fn best_justified_checkpoint(&self) -> &Checkpoint; - /// Returns the `finalized_checkpoint`. fn finalized_checkpoint(&self) -> &Checkpoint; @@ -68,9 +65,6 @@ pub trait ForkChoiceStore: Sized { /// Sets the `justified_checkpoint`. fn set_justified_checkpoint(&mut self, checkpoint: Checkpoint) -> Result<(), Self::Error>; - /// Sets the `best_justified_checkpoint`. - fn set_best_justified_checkpoint(&mut self, checkpoint: Checkpoint); - /// Sets the `unrealized_justified_checkpoint`. fn set_unrealized_justified_checkpoint(&mut self, checkpoint: Checkpoint); diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 00bd1f763dc..5bd679edd00 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -105,12 +105,10 @@ impl ForkChoiceTest { } /// Assert the epochs match. - pub fn assert_best_justified_epoch(self, epoch: u64) -> Self { - assert_eq!( - self.get(|fc_store| fc_store.best_justified_checkpoint().epoch), - Epoch::new(epoch), - "best_justified_epoch" - ); + /// + /// Note: this test is a no-op since the best-justified-checkpoint was + /// removed by Aditya's PR #18. + pub fn assert_best_justified_epoch(self, _epoch: u64) -> Self { self } diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index cafd5e179d5..b0f15a02148 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -575,21 +575,12 @@ impl Tester { check_equal("finalized_checkpoint", fc_checkpoint, expected_checkpoint) } + /// This test is now a no-op since it was removed by Aditya's PR #18. pub fn check_best_justified_checkpoint( &self, - expected_checkpoint: Checkpoint, + _expected_checkpoint: Checkpoint, ) -> Result<(), Error> { - let best_justified_checkpoint = self - .harness - .chain - .canonical_head - .fork_choice_read_lock() - .best_justified_checkpoint(); - check_equal( - "best_justified_checkpoint", - best_justified_checkpoint, - expected_checkpoint, - ) + Ok(()) } pub fn check_u_justified_checkpoint( From fc1e17fc849433bdcf4ad94a5b2ec0533fb706a3 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 8 Feb 2023 15:27:49 +1100 Subject: [PATCH 27/60] Tidy --- consensus/fork_choice/src/fork_choice.rs | 16 ++++++---- consensus/proto_array/src/proto_array.rs | 38 +++++++++++------------- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 507d921cbc7..8a2157186d9 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -816,10 +816,7 @@ where // If block is from past epochs, try to update store's justified & finalized checkpoints right away if block.slot().epoch(E::slots_per_epoch()) < current_slot.epoch(E::slots_per_epoch()) { - self.update_checkpoints( - unrealized_justified_checkpoint, - unrealized_finalized_checkpoint, - )?; + self.pull_up_store_checkpoints()?; } ( @@ -1176,14 +1173,21 @@ where return Ok(()); } + // Update the justified/finalized checkpoints based upon the + // best-observed unrealized justification/finality. + self.pull_up_store_checkpoints()?; + + Ok(()) + } + + fn pull_up_store_checkpoints(&mut self) -> Result<(), Error> { // Update store.justified_checkpoint if a better unrealized justified checkpoint is known let unrealized_justified_checkpoint = *self.fc_store.unrealized_justified_checkpoint(); let unrealized_finalized_checkpoint = *self.fc_store.unrealized_finalized_checkpoint(); self.update_checkpoints( unrealized_justified_checkpoint, unrealized_finalized_checkpoint, - )?; - Ok(()) + ) } /// Processes and removes from the queue any queued attestations which may now be eligible for diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index dcfeb1da5c2..f9ae5120fe1 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -103,18 +103,6 @@ pub struct ProtoNode { pub unrealized_finalized_checkpoint: Option, } -impl ProtoNode { - fn voting_source(&self, current_epoch: Epoch, slots_per_epoch: u64) -> Option { - let node_epoch = self.slot.epoch(slots_per_epoch); - if current_epoch > node_epoch { - self.unrealized_justified_checkpoint - .or(self.justified_checkpoint) - } else { - self.justified_checkpoint - } - } -} - #[derive(PartialEq, Debug, Encode, Decode, Serialize, Deserialize, Copy, Clone)] pub struct ProposerBoost { pub root: Hash256, @@ -913,15 +901,23 @@ impl ProtoArray { let genesis_epoch = Epoch::new(0); let current_epoch = current_slot.epoch(E::slots_per_epoch()); - let voting_source = - if let Some(checkpoint) = node.voting_source(current_epoch, E::slots_per_epoch()) { - checkpoint - } else { - // The node does not have any information about the - // justified/finalized checkpoint. This indicates an inconsistent - // proto-array. - return false; - }; + let node_epoch = node.slot.epoch(E::slots_per_epoch()); + + let voting_source_opt = if current_epoch > node_epoch { + node.unrealized_justified_checkpoint + .or(node.justified_checkpoint) + } else { + node.justified_checkpoint + }; + let voting_source = if let Some(checkpoint) = voting_source_opt { + checkpoint + } else { + // The node does not have any information about the + // justified/finalized checkpoint. This indicates an inconsistent + // proto-array. + return false; + }; + let unrealized_justification = if self.justified_checkpoint.epoch + 1 == current_epoch { // This `unrealized_justified_checkpoint` is an `Option` that can be // `None` in the scenario that we're not computing unrealized From e7b26f1fa1c6f5f6f4ea870ae6654eee4f70864c Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 8 Feb 2023 17:49:00 +1100 Subject: [PATCH 28/60] Fix test compile errors --- beacon_node/beacon_chain/tests/tests.rs | 4 ++-- consensus/fork_choice/tests/tests.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/beacon_node/beacon_chain/tests/tests.rs b/beacon_node/beacon_chain/tests/tests.rs index d80db132ef9..3be2d468167 100644 --- a/beacon_node/beacon_chain/tests/tests.rs +++ b/beacon_node/beacon_chain/tests/tests.rs @@ -500,7 +500,7 @@ async fn unaggregated_attestations_added_to_fork_choice_some_none() { // Move forward a slot so all queued attestations can be processed. harness.advance_slot(); fork_choice - .update_time(harness.chain.slot().unwrap(), &harness.chain.spec) + .update_time(harness.chain.slot().unwrap()) .unwrap(); let validator_slots: Vec<(usize, Slot)> = (0..VALIDATOR_COUNT) @@ -614,7 +614,7 @@ async fn unaggregated_attestations_added_to_fork_choice_all_updated() { // Move forward a slot so all queued attestations can be processed. harness.advance_slot(); fork_choice - .update_time(harness.chain.slot().unwrap(), &harness.chain.spec) + .update_time(harness.chain.slot().unwrap()) .unwrap(); let validators: Vec = (0..VALIDATOR_COUNT).collect(); diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 5bd679edd00..c4410d2c1a2 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -149,7 +149,7 @@ impl ForkChoiceTest { .chain .canonical_head .fork_choice_write_lock() - .update_time(self.harness.chain.slot().unwrap(), &self.harness.spec) + .update_time(self.harness.chain.slot().unwrap()) .unwrap(); func( self.harness From 13fbfedb7b885d0b7af95f66d9ee6c5aa2400bf8 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 9 Feb 2023 15:36:02 +1100 Subject: [PATCH 29/60] Fix after rebase --- consensus/proto_array/src/proto_array.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index f9ae5120fe1..810096a6641 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -946,7 +946,7 @@ impl ProtoArray { } let correct_finalized = self.finalized_checkpoint.epoch == genesis_epoch - || self.is_finalized_checkpoint_descendant::(node.root); + || self.is_finalized_checkpoint_or_descendant::(node.root); correct_justified && correct_finalized } From 529085c14392afe8cb1c6f3ed34f4ca5c5941c2e Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 9 Feb 2023 15:37:00 +1100 Subject: [PATCH 30/60] Remove .DSStore from git ignore --- .gitignore | 1 - 1 file changed, 1 deletion(-) diff --git a/.gitignore b/.gitignore index b1fb8faf6cc..ae9f83c46dd 100644 --- a/.gitignore +++ b/.gitignore @@ -9,7 +9,6 @@ perf.data* /bin genesis.ssz /clippy.toml -*.DS_Store # IntelliJ /*.iml From 1b4cde85019b9e62ba2b30559ab3a5230fc31afe Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 9 Feb 2023 18:17:33 +1100 Subject: [PATCH 31/60] Refactor filter logic for tidiness --- consensus/proto_array/src/proto_array.rs | 49 +++++++++--------------- 1 file changed, 19 insertions(+), 30 deletions(-) diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 810096a6641..35c62f0cb5d 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -902,44 +902,33 @@ impl ProtoArray { let genesis_epoch = Epoch::new(0); let current_epoch = current_slot.epoch(E::slots_per_epoch()); let node_epoch = node.slot.epoch(E::slots_per_epoch()); + let node_justified_checkpoint = + if let Some(justified_checkpoint) = node.justified_checkpoint { + justified_checkpoint + } else { + // The node does not have any information about the justified + // checkpoint. This indicates an inconsistent proto-array. + return false; + }; - let voting_source_opt = if current_epoch > node_epoch { - node.unrealized_justified_checkpoint - .or(node.justified_checkpoint) - } else { - node.justified_checkpoint - }; - let voting_source = if let Some(checkpoint) = voting_source_opt { - checkpoint - } else { - // The node does not have any information about the - // justified/finalized checkpoint. This indicates an inconsistent - // proto-array. - return false; - }; - - let unrealized_justification = if self.justified_checkpoint.epoch + 1 == current_epoch { - // This `unrealized_justified_checkpoint` is an `Option` that can be - // `None` in the scenario that we're not computing unrealized - // justification (UJ). During sync we avoid this computation as an - // optimisation. - // - // I claim that it is safe to always have `unrealized_justification - // == None` under the following conditions: - // - // 1. All competing heads do not have UJ computed. - // 2. The store is not updated as per UJ values. + let voting_source = if current_epoch > node_epoch { + // The block is from a prior epoch, the voting source will be pulled-up. node.unrealized_justified_checkpoint + // Sometimes we don't track the unrealized justification. In + // that case, just use the fully-realized justified checkpoint. + .unwrap_or(node_justified_checkpoint) } else { - None + // The block is not from a prior epoch, therefore the voting source + // is not pulled up. + node_justified_checkpoint }; let mut correct_justified = self.justified_checkpoint.epoch == genesis_epoch || voting_source.epoch == self.justified_checkpoint.epoch; - if !correct_justified { - if let Some(unrealized_justification) = unrealized_justification { - correct_justified = unrealized_justification.epoch + if let Some(node_unrealized_justified_checkpoint) = node.unrealized_justified_checkpoint { + if !correct_justified && self.justified_checkpoint.epoch + 1 == current_epoch { + correct_justified = node_unrealized_justified_checkpoint.epoch >= self.justified_checkpoint.epoch && voting_source.epoch + 2 >= current_epoch; } From 0b56a7367f8f024b3f29d680a5ca6a9f18a279bb Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 10 Feb 2023 09:31:04 +1100 Subject: [PATCH 32/60] Tidy --- consensus/fork_choice/src/fork_choice.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 8a2157186d9..8b1015f539d 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -922,6 +922,7 @@ where if finalized_checkpoint.epoch > self.fc_store.finalized_checkpoint().epoch { self.fc_store.set_finalized_checkpoint(finalized_checkpoint); } + Ok(()) } From b175574c7d1c7bca10d0fe9c6112b9da2274c381 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 10 Feb 2023 10:28:26 +1100 Subject: [PATCH 33/60] Hide custom FC tests behind flag, disable standard tests --- testing/ef_tests/Cargo.toml | 1 + testing/ef_tests/tests/tests.rs | 82 ++++++++++++++++++--------------- 2 files changed, 45 insertions(+), 38 deletions(-) diff --git a/testing/ef_tests/Cargo.toml b/testing/ef_tests/Cargo.toml index d969c9727d7..379ed90c295 100644 --- a/testing/ef_tests/Cargo.toml +++ b/testing/ef_tests/Cargo.toml @@ -9,6 +9,7 @@ edition = "2021" ef_tests = [] milagro = ["bls/milagro"] fake_crypto = ["bls/fake_crypto"] +fork_choice_custom = [] [dependencies] bls = { path = "../../crypto/bls", default-features = false } diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index 5735a5f692b..5ae542e6f53 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -3,9 +3,6 @@ use ef_tests::*; use types::*; -const CONSENSUS_SPEC_TESTS: &str = "consensus-spec-tests"; -const CUSTOM_FC_TESTS: &str = "custom-fc-tests"; - // Check that the hand-computed multiplications on EthSpec are correctly computed. // This test lives here because one is most likely to muck these up during a spec update. fn check_typenum_values() { @@ -427,51 +424,60 @@ fn finality() { FinalityHandler::::default().run(); } +#[allow(unused_macros)] // TODO(paul): remove `allow` once we have EF test release. macro_rules! fork_choice_tests { - ($mod: ident, $tests_dir: ident) => { - mod $mod { - use super::*; - - #[test] - fn fork_choice_get_head() { - ForkChoiceHandler::::new($tests_dir, "get_head").run(); - ForkChoiceHandler::::new($tests_dir, "get_head").run(); - } + ($tests_dir: ident) => { + #[test] + fn fork_choice_get_head() { + ForkChoiceHandler::::new($tests_dir, "get_head").run(); + ForkChoiceHandler::::new($tests_dir, "get_head").run(); + } - #[test] - fn fork_choice_on_block() { - ForkChoiceHandler::::new($tests_dir, "on_block").run(); - ForkChoiceHandler::::new($tests_dir, "on_block").run(); - } + #[test] + fn fork_choice_on_block() { + ForkChoiceHandler::::new($tests_dir, "on_block").run(); + ForkChoiceHandler::::new($tests_dir, "on_block").run(); + } - #[test] - fn fork_choice_on_merge_block() { - ForkChoiceHandler::::new($tests_dir, "on_merge_block").run(); - ForkChoiceHandler::::new($tests_dir, "on_merge_block").run(); - } + #[test] + fn fork_choice_on_merge_block() { + ForkChoiceHandler::::new($tests_dir, "on_merge_block").run(); + ForkChoiceHandler::::new($tests_dir, "on_merge_block").run(); + } - #[test] - fn fork_choice_ex_ante() { - ForkChoiceHandler::::new($tests_dir, "ex_ante").run(); - ForkChoiceHandler::::new($tests_dir, "ex_ante").run(); - } + #[test] + fn fork_choice_ex_ante() { + ForkChoiceHandler::::new($tests_dir, "ex_ante").run(); + ForkChoiceHandler::::new($tests_dir, "ex_ante").run(); } }; } -fork_choice_tests!(fork_choice_standard, CONSENSUS_SPEC_TESTS); -fork_choice_tests!(fork_choice_custom, CUSTOM_FC_TESTS); +// TODO(paul): The typical fork choice tests from the latest release will not +// pass due to the changes in PR #18. This must be resolved before we merge into +// `unstable`, but we are waiting on new test vectors from the EF. +// +// fork_choice_tests!("consensus_spec_tests"); -#[test] -fn fork_choice_custom_reorg() { - ForkChoiceHandler::::new(CUSTOM_FC_TESTS, "reorg").run(); - ForkChoiceHandler::::new(CUSTOM_FC_TESTS, "reorg").run(); -} +#[cfg(feature = "fork_choice_custom")] +mod fork_choice_custom { + use super::*; -#[test] -fn fork_choice_custom_withholding() { - ForkChoiceHandler::::new(CUSTOM_FC_TESTS, "withholding").run(); - ForkChoiceHandler::::new(CUSTOM_FC_TESTS, "withholding").run(); + const CUSTOM_FC_TESTS: &str = "custom-fc-tests"; + + fork_choice_tests!(CUSTOM_FC_TESTS); + + #[test] + fn fork_choice_custom_reorg() { + ForkChoiceHandler::::new(CUSTOM_FC_TESTS, "reorg").run(); + ForkChoiceHandler::::new(CUSTOM_FC_TESTS, "reorg").run(); + } + + #[test] + fn fork_choice_custom_withholding() { + ForkChoiceHandler::::new(CUSTOM_FC_TESTS, "withholding").run(); + ForkChoiceHandler::::new(CUSTOM_FC_TESTS, "withholding").run(); + } } #[test] From 0bd58ed312958e05cc8d1918068f0bac24975ad6 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 10 Feb 2023 14:33:46 +1100 Subject: [PATCH 34/60] Fix failing test --- consensus/fork_choice/tests/tests.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index c4410d2c1a2..82dce8e5433 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -610,8 +610,9 @@ async fn justified_checkpoint_updates_with_non_descendent_outside_safe_slots_wit .unwrap(); }) .await - .assert_justified_epoch(2) - .assert_best_justified_epoch(3); + // Now that `SAFE_SLOTS_TO_UPDATE_JUSTIFIED` has been removed, the new + // block should have updated the justified checkpoint. + .assert_justified_epoch(3); } /// - The new justified checkpoint **does not** descend from the current. From 2349e80e970bfa6176caaff978cfc8df72f019c1 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 10 Feb 2023 14:37:02 +1100 Subject: [PATCH 35/60] Remove references to best justified checkpoint in FC tests --- consensus/fork_choice/tests/tests.rs | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 82dce8e5433..82bf642f180 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -104,14 +104,6 @@ impl ForkChoiceTest { self } - /// Assert the epochs match. - /// - /// Note: this test is a no-op since the best-justified-checkpoint was - /// removed by Aditya's PR #18. - pub fn assert_best_justified_epoch(self, _epoch: u64) -> Self { - self - } - /// Assert the given slot is greater than the head slot. pub fn assert_finalized_epoch_is_less_than(self, epoch: Epoch) -> Self { assert!(self.harness.finalized_checkpoint().epoch < epoch); @@ -239,6 +231,11 @@ impl ForkChoiceTest { /// /// If the chain is presently in an unsafe period, transition through it and the following safe /// period. + /// + /// Note: the `SAFE_SLOTS_TO_UPDATE_JUSTIFIED` variable has been removed + /// from the fork choice spec in Q1 2023. We're still leaving references to + /// it in our tests because (a) it's easier and (b) it allows us to easily + /// test for the absence of that parameter. pub fn move_to_next_unsafe_period(self) -> Self { self.move_inside_safe_to_update() .move_outside_safe_to_update() @@ -532,7 +529,6 @@ async fn justified_checkpoint_updates_with_descendent_outside_safe_slots() { .unwrap() .move_outside_safe_to_update() .assert_justified_epoch(2) - .assert_best_justified_epoch(2) .apply_blocks(1) .await .assert_justified_epoch(3); @@ -549,11 +545,9 @@ async fn justified_checkpoint_updates_first_justification_outside_safe_to_update .unwrap() .move_to_next_unsafe_period() .assert_justified_epoch(0) - .assert_best_justified_epoch(0) .apply_blocks(1) .await - .assert_justified_epoch(2) - .assert_best_justified_epoch(2); + .assert_justified_epoch(2); } /// - The new justified checkpoint **does not** descend from the current. @@ -581,8 +575,7 @@ async fn justified_checkpoint_updates_with_non_descendent_inside_safe_slots_with .unwrap(); }) .await - .assert_justified_epoch(3) - .assert_best_justified_epoch(3); + .assert_justified_epoch(3); } /// - The new justified checkpoint **does not** descend from the current. @@ -640,8 +633,7 @@ async fn justified_checkpoint_updates_with_non_descendent_outside_safe_slots_wit .unwrap(); }) .await - .assert_justified_epoch(3) - .assert_best_justified_epoch(3); + .assert_justified_epoch(3); } /// Check that the balances are obtained correctly. From 6fbf9e441a9fc86f7504d4f92bc28e0bd1435fe9 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 10 Feb 2023 14:40:58 +1100 Subject: [PATCH 36/60] Remove best just checkpoint from EF tests --- testing/ef_tests/src/cases/fork_choice.rs | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index b0f15a02148..af5d38aaa60 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -45,7 +45,6 @@ pub struct Checks { justified_checkpoint: Option, justified_checkpoint_root: Option, finalized_checkpoint: Option, - best_justified_checkpoint: Option, u_justified_checkpoint: Option, u_finalized_checkpoint: Option, proposer_boost_root: Option, @@ -229,7 +228,6 @@ impl Case for ForkChoiceTest { justified_checkpoint, justified_checkpoint_root, finalized_checkpoint, - best_justified_checkpoint, u_justified_checkpoint, u_finalized_checkpoint, proposer_boost_root, @@ -260,11 +258,6 @@ impl Case for ForkChoiceTest { tester.check_finalized_checkpoint(*expected_finalized_checkpoint)?; } - if let Some(expected_best_justified_checkpoint) = best_justified_checkpoint { - tester - .check_best_justified_checkpoint(*expected_best_justified_checkpoint)?; - } - if let Some(expected_u_justified_checkpoint) = u_justified_checkpoint { tester.check_u_justified_checkpoint(*expected_u_justified_checkpoint)?; } @@ -575,14 +568,6 @@ impl Tester { check_equal("finalized_checkpoint", fc_checkpoint, expected_checkpoint) } - /// This test is now a no-op since it was removed by Aditya's PR #18. - pub fn check_best_justified_checkpoint( - &self, - _expected_checkpoint: Checkpoint, - ) -> Result<(), Error> { - Ok(()) - } - pub fn check_u_justified_checkpoint( &self, expected_checkpoint: Checkpoint, From 1b761d63fab2be3b6a8cc9c725bc0dc88bc25908 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 10 Feb 2023 14:58:28 +1100 Subject: [PATCH 37/60] Remove count-unrealized-full --- beacon_node/beacon_chain/src/beacon_chain.rs | 5 +--- beacon_node/beacon_chain/src/builder.rs | 4 --- .../beacon_chain/src/canonical_head.rs | 15 +++------- beacon_node/beacon_chain/src/chain_config.rs | 5 +--- beacon_node/beacon_chain/src/fork_revert.rs | 3 -- beacon_node/beacon_chain/src/lib.rs | 2 +- beacon_node/src/cli.rs | 2 +- beacon_node/src/config.rs | 10 +++++-- consensus/fork_choice/src/fork_choice.rs | 22 ++++---------- consensus/fork_choice/src/lib.rs | 4 +-- .../src/fork_choice_test_definition.rs | 6 ++-- consensus/proto_array/src/lib.rs | 4 +-- consensus/proto_array/src/proto_array.rs | 19 ------------ .../src/proto_array_fork_choice.rs | 15 +++------- consensus/proto_array/src/ssz_container.rs | 9 ++---- lighthouse/tests/beacon_node.rs | 30 ++++++------------- 16 files changed, 42 insertions(+), 113 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 5f31eaf11df..12561ec3659 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -69,7 +69,7 @@ use itertools::process_results; use itertools::Itertools; use operation_pool::{AttestationRef, OperationPool, PersistedOperationPool}; use parking_lot::{Mutex, RwLock}; -use proto_array::{CountUnrealizedFull, DoNotReOrg, ProposerHeadError}; +use proto_array::{DoNotReOrg, ProposerHeadError}; use safe_arith::SafeArith; use slasher::Slasher; use slog::{crit, debug, error, info, trace, warn, Logger}; @@ -469,7 +469,6 @@ impl BeaconChain { pub fn load_fork_choice( store: BeaconStore, reset_payload_statuses: ResetPayloadStatuses, - count_unrealized_full: CountUnrealizedFull, spec: &ChainSpec, log: &Logger, ) -> Result>, Error> { @@ -486,7 +485,6 @@ impl BeaconChain { persisted_fork_choice.fork_choice, reset_payload_statuses, fc_store, - count_unrealized_full, spec, log, )?)) @@ -2870,7 +2868,6 @@ impl BeaconChain { ResetPayloadStatuses::always_reset_conditionally( self.config.always_reset_payload_statuses, ), - self.config.count_unrealized_full, &self.store, &self.spec, &self.log, diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index 48419d46edb..89dff049544 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -265,7 +265,6 @@ where ResetPayloadStatuses::always_reset_conditionally( self.chain_config.always_reset_payload_statuses, ), - self.chain_config.count_unrealized_full, &self.spec, log, ) @@ -384,7 +383,6 @@ where &genesis.beacon_block, &genesis.beacon_state, current_slot, - self.chain_config.count_unrealized_full, &self.spec, ) .map_err(|e| format!("Unable to initialize ForkChoice: {:?}", e))?; @@ -503,7 +501,6 @@ where &snapshot.beacon_block, &snapshot.beacon_state, current_slot, - self.chain_config.count_unrealized_full, &self.spec, ) .map_err(|e| format!("Unable to initialize ForkChoice: {:?}", e))?; @@ -682,7 +679,6 @@ where Some(current_slot), &self.spec, self.chain_config.count_unrealized.into(), - self.chain_config.count_unrealized_full, )?; } diff --git a/beacon_node/beacon_chain/src/canonical_head.rs b/beacon_node/beacon_chain/src/canonical_head.rs index dd64e02edf7..9db21e0482c 100644 --- a/beacon_node/beacon_chain/src/canonical_head.rs +++ b/beacon_node/beacon_chain/src/canonical_head.rs @@ -45,8 +45,7 @@ use crate::{ }; use eth2::types::{EventKind, SseChainReorg, SseFinalizedCheckpoint, SseHead, SseLateHead}; use fork_choice::{ - CountUnrealizedFull, ExecutionStatus, ForkChoiceView, ForkchoiceUpdateParameters, ProtoBlock, - ResetPayloadStatuses, + ExecutionStatus, ForkChoiceView, ForkchoiceUpdateParameters, ProtoBlock, ResetPayloadStatuses, }; use itertools::process_results; use parking_lot::{Mutex, RwLock, RwLockReadGuard, RwLockWriteGuard}; @@ -274,19 +273,13 @@ impl CanonicalHead { // defensive programming. mut fork_choice_write_lock: RwLockWriteGuard>, reset_payload_statuses: ResetPayloadStatuses, - count_unrealized_full: CountUnrealizedFull, store: &BeaconStore, spec: &ChainSpec, log: &Logger, ) -> Result<(), Error> { - let fork_choice = >::load_fork_choice( - store.clone(), - reset_payload_statuses, - count_unrealized_full, - spec, - log, - )? - .ok_or(Error::MissingPersistedForkChoice)?; + let fork_choice = + >::load_fork_choice(store.clone(), reset_payload_statuses, spec, log)? + .ok_or(Error::MissingPersistedForkChoice)?; let fork_choice_view = fork_choice.cached_fork_choice_view(); let beacon_block_root = fork_choice_view.head_block_root; let beacon_block = store diff --git a/beacon_node/beacon_chain/src/chain_config.rs b/beacon_node/beacon_chain/src/chain_config.rs index cce2fbb971f..1711bbd89a2 100644 --- a/beacon_node/beacon_chain/src/chain_config.rs +++ b/beacon_node/beacon_chain/src/chain_config.rs @@ -1,4 +1,4 @@ -pub use proto_array::{CountUnrealizedFull, ReOrgThreshold}; +pub use proto_array::ReOrgThreshold; use serde_derive::{Deserialize, Serialize}; use std::time::Duration; use types::{Checkpoint, Epoch}; @@ -56,8 +56,6 @@ pub struct ChainConfig { pub always_reset_payload_statuses: bool, /// Whether to apply paranoid checks to blocks proposed by this beacon node. pub paranoid_block_proposal: bool, - /// Whether to strictly count unrealized justified votes. - pub count_unrealized_full: CountUnrealizedFull, /// Optionally set timeout for calls to checkpoint sync endpoint. pub checkpoint_sync_url_timeout: u64, /// The offset before the start of a proposal slot at which payload attributes should be sent. @@ -88,7 +86,6 @@ impl Default for ChainConfig { count_unrealized: true, always_reset_payload_statuses: false, paranoid_block_proposal: false, - count_unrealized_full: CountUnrealizedFull::default(), checkpoint_sync_url_timeout: 60, prepare_payload_lookahead: Duration::from_secs(4), optimistic_finalized_sync: true, diff --git a/beacon_node/beacon_chain/src/fork_revert.rs b/beacon_node/beacon_chain/src/fork_revert.rs index 6d5b5ddc4ae..ef23248aba6 100644 --- a/beacon_node/beacon_chain/src/fork_revert.rs +++ b/beacon_node/beacon_chain/src/fork_revert.rs @@ -1,7 +1,6 @@ use crate::{BeaconForkChoiceStore, BeaconSnapshot}; use fork_choice::{CountUnrealized, ForkChoice, PayloadVerificationStatus}; use itertools::process_results; -use proto_array::CountUnrealizedFull; use slog::{info, warn, Logger}; use state_processing::state_advance::complete_state_advance; use state_processing::{ @@ -102,7 +101,6 @@ pub fn reset_fork_choice_to_finalization, Cold: It current_slot: Option, spec: &ChainSpec, count_unrealized_config: CountUnrealized, - count_unrealized_full_config: CountUnrealizedFull, ) -> Result, E>, String> { // Fetch finalized block. let finalized_checkpoint = head_state.finalized_checkpoint(); @@ -156,7 +154,6 @@ pub fn reset_fork_choice_to_finalization, Cold: It &finalized_snapshot.beacon_block, &finalized_snapshot.beacon_state, current_slot, - count_unrealized_full_config, spec, ) .map_err(|e| format!("Unable to reset fork choice for revert: {:?}", e))?; diff --git a/beacon_node/beacon_chain/src/lib.rs b/beacon_node/beacon_chain/src/lib.rs index 5e75c2a632e..2a7232a5732 100644 --- a/beacon_node/beacon_chain/src/lib.rs +++ b/beacon_node/beacon_chain/src/lib.rs @@ -56,7 +56,7 @@ pub use self::beacon_chain::{ INVALID_JUSTIFIED_PAYLOAD_SHUTDOWN_REASON, MAXIMUM_GOSSIP_CLOCK_DISPARITY, }; pub use self::beacon_snapshot::BeaconSnapshot; -pub use self::chain_config::{ChainConfig, CountUnrealizedFull}; +pub use self::chain_config::ChainConfig; pub use self::errors::{BeaconChainError, BlockProductionError}; pub use self::historical_blocks::HistoricalBlockError; pub use attestation_verification::Error as AttestationError; diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs index b4da83315c8..162f77f2b61 100644 --- a/beacon_node/src/cli.rs +++ b/beacon_node/src/cli.rs @@ -906,7 +906,7 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { Arg::with_name("count-unrealized-full") .long("count-unrealized-full") .hidden(true) - .help("Stricter version of `count-unrealized`.") + .help("This flag is deprecated and has no effect.") .takes_value(true) .default_value("false") ) diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index 726f8368ea4..188cd18eee4 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -719,8 +719,14 @@ pub fn get_config( client_config.chain.count_unrealized = clap_utils::parse_required(cli_args, "count-unrealized")?; - client_config.chain.count_unrealized_full = - clap_utils::parse_required::(cli_args, "count-unrealized-full")?.into(); + + if clap_utils::parse_required::(cli_args, "count-unrealized-full")? { + warn!( + log, + "The flag --count-unrealized-full is deprecated and will be removed"; + "info" => "setting it to `true` has no effect" + ); + } client_config.chain.always_reset_payload_statuses = cli_args.is_present("reset-payload-statuses"); diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 8b1015f539d..aa2134f45d0 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -1,6 +1,6 @@ use crate::{ForkChoiceStore, InvalidationOperation}; use proto_array::{ - Block as ProtoBlock, CountUnrealizedFull, ExecutionStatus, ProposerHeadError, ProposerHeadInfo, + Block as ProtoBlock, ExecutionStatus, ProposerHeadError, ProposerHeadInfo, ProtoArrayForkChoice, ReOrgThreshold, }; use slog::{crit, debug, warn, Logger}; @@ -365,7 +365,6 @@ where anchor_block: &SignedBeaconBlock, anchor_state: &BeaconState, current_slot: Option, - count_unrealized_full_config: CountUnrealizedFull, spec: &ChainSpec, ) -> Result> { // Sanity check: the anchor must lie on an epoch boundary. @@ -412,7 +411,6 @@ where current_epoch_shuffling_id, next_epoch_shuffling_id, execution_status, - count_unrealized_full_config, )?; let mut fork_choice = Self { @@ -1414,13 +1412,11 @@ where pub fn proto_array_from_persisted( persisted: &PersistedForkChoice, reset_payload_statuses: ResetPayloadStatuses, - count_unrealized_full: CountUnrealizedFull, spec: &ChainSpec, log: &Logger, ) -> Result> { - let mut proto_array = - ProtoArrayForkChoice::from_bytes(&persisted.proto_array_bytes, count_unrealized_full) - .map_err(Error::InvalidProtoArrayBytes)?; + let mut proto_array = ProtoArrayForkChoice::from_bytes(&persisted.proto_array_bytes) + .map_err(Error::InvalidProtoArrayBytes)?; let contains_invalid_payloads = proto_array.contains_invalid_payloads(); debug!( @@ -1451,7 +1447,7 @@ where "error" => e, "info" => "please report this error", ); - ProtoArrayForkChoice::from_bytes(&persisted.proto_array_bytes, count_unrealized_full) + ProtoArrayForkChoice::from_bytes(&persisted.proto_array_bytes) .map_err(Error::InvalidProtoArrayBytes) } else { debug!( @@ -1468,17 +1464,11 @@ where persisted: PersistedForkChoice, reset_payload_statuses: ResetPayloadStatuses, fc_store: T, - count_unrealized_full: CountUnrealizedFull, spec: &ChainSpec, log: &Logger, ) -> Result> { - let proto_array = Self::proto_array_from_persisted( - &persisted, - reset_payload_statuses, - count_unrealized_full, - spec, - log, - )?; + let proto_array = + Self::proto_array_from_persisted(&persisted, reset_payload_statuses, spec, log)?; let current_slot = fc_store.get_current_slot(); diff --git a/consensus/fork_choice/src/lib.rs b/consensus/fork_choice/src/lib.rs index b307c66d885..397a2ff8930 100644 --- a/consensus/fork_choice/src/lib.rs +++ b/consensus/fork_choice/src/lib.rs @@ -7,6 +7,4 @@ pub use crate::fork_choice::{ PersistedForkChoice, QueuedAttestation, ResetPayloadStatuses, }; pub use fork_choice_store::ForkChoiceStore; -pub use proto_array::{ - Block as ProtoBlock, CountUnrealizedFull, ExecutionStatus, InvalidationOperation, -}; +pub use proto_array::{Block as ProtoBlock, ExecutionStatus, InvalidationOperation}; diff --git a/consensus/proto_array/src/fork_choice_test_definition.rs b/consensus/proto_array/src/fork_choice_test_definition.rs index 68b3fb71981..157f072ad37 100644 --- a/consensus/proto_array/src/fork_choice_test_definition.rs +++ b/consensus/proto_array/src/fork_choice_test_definition.rs @@ -3,7 +3,6 @@ mod ffg_updates; mod no_votes; mod votes; -use crate::proto_array::CountUnrealizedFull; use crate::proto_array_fork_choice::{Block, ExecutionStatus, ProtoArrayForkChoice}; use crate::{InvalidationOperation, JustifiedBalances}; use serde_derive::{Deserialize, Serialize}; @@ -88,7 +87,6 @@ impl ForkChoiceTestDefinition { junk_shuffling_id.clone(), junk_shuffling_id, ExecutionStatus::Optimistic(ExecutionBlockHash::zero()), - CountUnrealizedFull::default(), ) .expect("should create fork choice struct"); let equivocating_indices = BTreeSet::new(); @@ -307,8 +305,8 @@ fn get_checkpoint(i: u64) -> Checkpoint { fn check_bytes_round_trip(original: &ProtoArrayForkChoice) { let bytes = original.as_bytes(); - let decoded = ProtoArrayForkChoice::from_bytes(&bytes, CountUnrealizedFull::default()) - .expect("fork choice should decode from bytes"); + let decoded = + ProtoArrayForkChoice::from_bytes(&bytes).expect("fork choice should decode from bytes"); assert!( *original == decoded, "fork choice should encode and decode without change" diff --git a/consensus/proto_array/src/lib.rs b/consensus/proto_array/src/lib.rs index f2b29e1c7b2..e84139345ae 100644 --- a/consensus/proto_array/src/lib.rs +++ b/consensus/proto_array/src/lib.rs @@ -6,9 +6,7 @@ mod proto_array_fork_choice; mod ssz_container; pub use crate::justified_balances::JustifiedBalances; -pub use crate::proto_array::{ - calculate_committee_fraction, CountUnrealizedFull, InvalidationOperation, -}; +pub use crate::proto_array::{calculate_committee_fraction, InvalidationOperation}; pub use crate::proto_array_fork_choice::{ Block, DoNotReOrg, ExecutionStatus, ProposerHeadError, ProposerHeadInfo, ProtoArrayForkChoice, ReOrgThreshold, diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 3871d2cde94..2c2514b20e0 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -118,24 +118,6 @@ impl Default for ProposerBoost { } } -/// Indicate whether we should strictly count unrealized justification/finalization votes. -#[derive(Default, PartialEq, Eq, Debug, Serialize, Deserialize, Copy, Clone)] -pub enum CountUnrealizedFull { - True, - #[default] - False, -} - -impl From for CountUnrealizedFull { - fn from(b: bool) -> Self { - if b { - CountUnrealizedFull::True - } else { - CountUnrealizedFull::False - } - } -} - #[derive(PartialEq, Debug, Serialize, Deserialize, Clone)] pub struct ProtoArray { /// Do not attempt to prune the tree unless it has at least this many nodes. Small prunes @@ -146,7 +128,6 @@ pub struct ProtoArray { pub nodes: Vec, pub indices: HashMap, pub previous_proposer_boost: ProposerBoost, - pub count_unrealized_full: CountUnrealizedFull, } impl ProtoArray { diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 0e0d806e76e..eae54e73428 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -1,8 +1,8 @@ use crate::{ error::Error, proto_array::{ - calculate_committee_fraction, CountUnrealizedFull, InvalidationOperation, Iter, - ProposerBoost, ProtoArray, ProtoNode, + calculate_committee_fraction, InvalidationOperation, Iter, ProposerBoost, ProtoArray, + ProtoNode, }, ssz_container::SszContainer, JustifiedBalances, @@ -307,7 +307,6 @@ impl ProtoArrayForkChoice { current_epoch_shuffling_id: AttestationShufflingId, next_epoch_shuffling_id: AttestationShufflingId, execution_status: ExecutionStatus, - count_unrealized_full: CountUnrealizedFull, ) -> Result { let mut proto_array = ProtoArray { prune_threshold: DEFAULT_PRUNE_THRESHOLD, @@ -316,7 +315,6 @@ impl ProtoArrayForkChoice { nodes: Vec::with_capacity(1), indices: HashMap::with_capacity(1), previous_proposer_boost: ProposerBoost::default(), - count_unrealized_full, }; let block = Block { @@ -780,13 +778,10 @@ impl ProtoArrayForkChoice { SszContainer::from(self).as_ssz_bytes() } - pub fn from_bytes( - bytes: &[u8], - count_unrealized_full: CountUnrealizedFull, - ) -> Result { + pub fn from_bytes(bytes: &[u8]) -> Result { let container = SszContainer::from_ssz_bytes(bytes) .map_err(|e| format!("Failed to decode ProtoArrayForkChoice: {:?}", e))?; - (container, count_unrealized_full) + container .try_into() .map_err(|e| format!("Failed to initialize ProtoArrayForkChoice: {e:?}")) } @@ -950,7 +945,6 @@ mod test_compute_deltas { junk_shuffling_id.clone(), junk_shuffling_id.clone(), execution_status, - CountUnrealizedFull::default(), ) .unwrap(); @@ -1076,7 +1070,6 @@ mod test_compute_deltas { junk_shuffling_id.clone(), junk_shuffling_id.clone(), execution_status, - CountUnrealizedFull::default(), ) .unwrap(); diff --git a/consensus/proto_array/src/ssz_container.rs b/consensus/proto_array/src/ssz_container.rs index 1a20ef967ad..ed1efaae1af 100644 --- a/consensus/proto_array/src/ssz_container.rs +++ b/consensus/proto_array/src/ssz_container.rs @@ -1,6 +1,6 @@ use crate::proto_array::ProposerBoost; use crate::{ - proto_array::{CountUnrealizedFull, ProtoArray, ProtoNode}, + proto_array::{ProtoArray, ProtoNode}, proto_array_fork_choice::{ElasticList, ProtoArrayForkChoice, VoteTracker}, Error, JustifiedBalances, }; @@ -43,12 +43,10 @@ impl From<&ProtoArrayForkChoice> for SszContainer { } } -impl TryFrom<(SszContainer, CountUnrealizedFull)> for ProtoArrayForkChoice { +impl TryFrom for ProtoArrayForkChoice { type Error = Error; - fn try_from( - (from, count_unrealized_full): (SszContainer, CountUnrealizedFull), - ) -> Result { + fn try_from(from: SszContainer) -> Result { let proto_array = ProtoArray { prune_threshold: from.prune_threshold, justified_checkpoint: from.justified_checkpoint, @@ -56,7 +54,6 @@ impl TryFrom<(SszContainer, CountUnrealizedFull)> for ProtoArrayForkChoice { nodes: from.nodes, indices: from.indices.into_iter().collect::>(), previous_proposer_boost: from.previous_proposer_boost, - count_unrealized_full, }; Ok(Self { diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index 053a04f879a..67eb9961d96 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -1,4 +1,4 @@ -use beacon_node::{beacon_chain::CountUnrealizedFull, ClientConfig as Config}; +use beacon_node::ClientConfig as Config; use crate::exec::{CommandLineTestExec, CompletedTest}; use beacon_node::beacon_chain::chain_config::{ @@ -232,39 +232,27 @@ fn count_unrealized_true() { fn count_unrealized_full_no_arg() { CommandLineTest::new() .flag("count-unrealized-full", None) + // This flag should be ignored, so there's nothing to test but that the + // client starts with the flag present. .run_with_zero_port() - .with_config(|config| { - assert_eq!( - config.chain.count_unrealized_full, - CountUnrealizedFull::False - ) - }); } #[test] fn count_unrealized_full_false() { CommandLineTest::new() .flag("count-unrealized-full", Some("false")) - .run_with_zero_port() - .with_config(|config| { - assert_eq!( - config.chain.count_unrealized_full, - CountUnrealizedFull::False - ) - }); + // This flag should be ignored, so there's nothing to test but that the + // client starts with the flag present. + .run_with_zero_port(); } #[test] fn count_unrealized_full_true() { CommandLineTest::new() .flag("count-unrealized-full", Some("true")) - .run_with_zero_port() - .with_config(|config| { - assert_eq!( - config.chain.count_unrealized_full, - CountUnrealizedFull::True - ) - }); + // This flag should be ignored, so there's nothing to test but that the + // client starts with the flag present. + .run_with_zero_port(); } #[test] From a3e0f3ef89eff05b2463983c0ee232acdb33dd80 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 13 Feb 2023 14:12:02 +1100 Subject: [PATCH 38/60] Fix test compile error --- lighthouse/tests/beacon_node.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index 67eb9961d96..ee335ace679 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -234,7 +234,7 @@ fn count_unrealized_full_no_arg() { .flag("count-unrealized-full", None) // This flag should be ignored, so there's nothing to test but that the // client starts with the flag present. - .run_with_zero_port() + .run_with_zero_port(); } #[test] From 6972850a6c144da2899fcfb10419a9d04bb5d7d5 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 13 Feb 2023 16:50:02 +1100 Subject: [PATCH 39/60] Use junk justified checkpoint --- beacon_node/beacon_chain/src/beacon_fork_choice_store.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) 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 db3ae484a31..3c8c5b21e78 100644 --- a/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs +++ b/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs @@ -21,9 +21,12 @@ use types::{ Hash256, Slot, }; -const JUNK_CHECKPOINT: Checkpoint = Checkpoint { - epoch: Epoch::new(42), - root: Hash256::repeat_byte(42), +/// Ensure this justified checkpoint has an epoch of 0 so that it is never +/// greater than the justified checkpoint and enshrined as the actual justified +/// checkpoint. +const JUNK_BEST_JUSTIFIED_CHECKPOINT: Checkpoint = Checkpoint { + epoch: Epoch::new(0), + root: Hash256::repeat_byte(0), }; #[derive(Debug)] From e9835aa3255814dce9964818e985399abc723685 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 13 Feb 2023 16:52:52 +1100 Subject: [PATCH 40/60] Skip fork choice tests --- testing/ef_tests/check_all_files_accessed.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/testing/ef_tests/check_all_files_accessed.py b/testing/ef_tests/check_all_files_accessed.py index 892b9a37707..34dafc96c78 100755 --- a/testing/ef_tests/check_all_files_accessed.py +++ b/testing/ef_tests/check_all_files_accessed.py @@ -50,7 +50,11 @@ # some bls tests are not included now "bls12-381-tests/deserialization_G1", "bls12-381-tests/deserialization_G2", - "bls12-381-tests/hash_to_G2" + "bls12-381-tests/hash_to_G2", + # Fork choice (temporarily skipped) + # + # TODO(paul): these tests MUST be re-enabled before merging into unstable! + "tests/.*/.*/fork_choice" ] def normalize_path(path): From dcb8427f4fd71f16516abc0a08d00ab99c45367f Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 13 Feb 2023 16:54:14 +1100 Subject: [PATCH 41/60] Fix compile error --- beacon_node/beacon_chain/src/beacon_fork_choice_store.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 3c8c5b21e78..623df5618e3 100644 --- a/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs +++ b/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs @@ -221,8 +221,7 @@ where finalized_checkpoint: self.finalized_checkpoint, justified_checkpoint: self.justified_checkpoint, justified_balances: self.justified_balances.effective_balances.clone(), - // TODO(paul): consider a database migration here. - best_justified_checkpoint: JUNK_CHECKPOINT, + best_justified_checkpoint: JUNK_BEST_JUSTIFIED_CHECKPOINT, unrealized_justified_checkpoint: self.unrealized_justified_checkpoint, unrealized_finalized_checkpoint: self.unrealized_finalized_checkpoint, proposer_boost_root: self.proposer_boost_root, From dfe73a2752207cc0b2e8b681a2a79e69c1d59c7a Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 13 Feb 2023 17:09:01 +1100 Subject: [PATCH 42/60] Remove commented out cfg flag --- testing/ef_tests/tests/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index 5ae542e6f53..f803c426123 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -1,4 +1,4 @@ -// #![cfg(feature = "ef_tests")] +#![cfg(feature = "ef_tests")] use ef_tests::*; use types::*; From 27966064d688b6a13c9dbf83a0c779d1deddf164 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 13 Feb 2023 17:10:00 +1100 Subject: [PATCH 43/60] Add comment about running tests --- testing/ef_tests/Makefile | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/testing/ef_tests/Makefile b/testing/ef_tests/Makefile index 242390df69f..6797779f72a 100644 --- a/testing/ef_tests/Makefile +++ b/testing/ef_tests/Makefile @@ -4,9 +4,11 @@ TARBALLS = $(patsubst %,%-$(TESTS_TAG).tar.gz,$(TESTS)) # Custom fork choice tests from https://github.com/adiasg/consensus-specs # -# Since these tests are private, we can't download them in this Makefile -# without adding an API key into the process, so we're just downloading -# the files manually. These files can be obtained from: +# Since these tests are private, we can't download them in this Makefile without +# adding an API key into the process, so we're just downloading the files +# manually and putting them in the root of the `testing/ef_tests` directory. +# +# These files can be obtained from: # https://github.com/adiasg/consensus-specs/pull/18#issuecomment-1411070772 CUSTOM_FC_TESTS_TAG = 8e46b6f CUSTOM_FC_TESTS_MAINNET_ARCHIVE = $(CUSTOM_FC_TESTS_TAG)-mainnet.tar.gz From 162e97c87b883be51d67939c216b4a526cf21798 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 16 Feb 2023 10:15:47 +1100 Subject: [PATCH 44/60] Bump test version: --- testing/ef_tests/Makefile | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/testing/ef_tests/Makefile b/testing/ef_tests/Makefile index 6797779f72a..cab7aa6bace 100644 --- a/testing/ef_tests/Makefile +++ b/testing/ef_tests/Makefile @@ -10,7 +10,7 @@ TARBALLS = $(patsubst %,%-$(TESTS_TAG).tar.gz,$(TESTS)) # # These files can be obtained from: # https://github.com/adiasg/consensus-specs/pull/18#issuecomment-1411070772 -CUSTOM_FC_TESTS_TAG = 8e46b6f +CUSTOM_FC_TESTS_TAG = fea20be CUSTOM_FC_TESTS_MAINNET_ARCHIVE = $(CUSTOM_FC_TESTS_TAG)-mainnet.tar.gz CUSTOM_FC_TESTS_MINIMAL_ARCHIVE = $(CUSTOM_FC_TESTS_TAG)-minimal.tar.gz CUSTOM_FC_TESTS_REPO_NAME := custom-fc-tests @@ -52,6 +52,9 @@ custom-fc-tests: tar -xzf $(CUSTOM_FC_TESTS_MAINNET_ARCHIVE) -C $(CUSTOM_FC_TESTS_OUTPUT_DIR); tar -xzf $(CUSTOM_FC_TESTS_MINIMAL_ARCHIVE) -C $(CUSTOM_FC_TESTS_OUTPUT_DIR); +clean-custom-fc-tests: + rm -rf $(CUSTOM_FC_TESTS_OUTPUT_DIR) + clean-test-files: rm -rf $(OUTPUT_DIR) $(BLS_OUTPUT_DIR) From ff76ffa87e5f1870af81bb0ae018b713f1c295e6 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 1 Mar 2023 15:27:02 +1100 Subject: [PATCH 45/60] Switch to unified test format --- testing/ef_tests/Makefile | 29 +------------ testing/ef_tests/src/handler.rs | 14 +----- testing/ef_tests/tests/tests.rs | 76 ++++++++++++--------------------- 3 files changed, 32 insertions(+), 87 deletions(-) diff --git a/testing/ef_tests/Makefile b/testing/ef_tests/Makefile index 5e949c18946..4911d170eac 100644 --- a/testing/ef_tests/Makefile +++ b/testing/ef_tests/Makefile @@ -1,21 +1,7 @@ -TESTS_TAG := v1.3.0-rc.3 -TESTS = general minimal mainnet +TESTS_TAG := v1.3.0-rc.3-pr18-1b09c23 +TESTS = minimal mainnet TARBALLS = $(patsubst %,%-$(TESTS_TAG).tar.gz,$(TESTS)) -# Custom fork choice tests from https://github.com/adiasg/consensus-specs -# -# Since these tests are private, we can't download them in this Makefile without -# adding an API key into the process, so we're just downloading the files -# manually and putting them in the root of the `testing/ef_tests` directory. -# -# These files can be obtained from: -# https://github.com/adiasg/consensus-specs/pull/18#issuecomment-1411070772 -CUSTOM_FC_TESTS_TAG = fea20be -CUSTOM_FC_TESTS_MAINNET_ARCHIVE = $(CUSTOM_FC_TESTS_TAG)-mainnet.tar.gz -CUSTOM_FC_TESTS_MINIMAL_ARCHIVE = $(CUSTOM_FC_TESTS_TAG)-minimal.tar.gz -CUSTOM_FC_TESTS_REPO_NAME := custom-fc-tests -CUSTOM_FC_TESTS_OUTPUT_DIR := ./$(CUSTOM_FC_TESTS_REPO_NAME) - REPO_NAME := consensus-spec-tests OUTPUT_DIR := ./$(REPO_NAME) BASE_URL := https://github.com/ethereum/$(REPO_NAME)/releases/download/$(TESTS_TAG) @@ -44,17 +30,6 @@ $(BLS_OUTPUT_DIR): $(WGET) $(BLS_BASE_URL)/$(BLS_TEST).tar.gz -O $(BLS_TARBALL) tar -xzf $(BLS_TARBALL) -C $(BLS_OUTPUT_DIR) -%-$(TESTS_TAG).tar.gz: - $(WGET) $(BASE_URL)/$*.tar.gz -O $@ - -custom-fc-tests: - mkdir $(CUSTOM_FC_TESTS_OUTPUT_DIR) - tar -xzf $(CUSTOM_FC_TESTS_MAINNET_ARCHIVE) -C $(CUSTOM_FC_TESTS_OUTPUT_DIR); - tar -xzf $(CUSTOM_FC_TESTS_MINIMAL_ARCHIVE) -C $(CUSTOM_FC_TESTS_OUTPUT_DIR); - -clean-custom-fc-tests: - rm -rf $(CUSTOM_FC_TESTS_OUTPUT_DIR) - clean-test-files: rm -rf $(OUTPUT_DIR) $(BLS_OUTPUT_DIR) diff --git a/testing/ef_tests/src/handler.rs b/testing/ef_tests/src/handler.rs index c475a1543b0..c066bdafa48 100644 --- a/testing/ef_tests/src/handler.rs +++ b/testing/ef_tests/src/handler.rs @@ -18,10 +18,6 @@ pub trait Handler { fn handler_name(&self) -> String; - fn tests_dir(&self) -> String { - "consensus-spec-tests".to_string() - } - fn is_enabled_for_fork(&self, fork_name: ForkName) -> bool { Self::Case::is_enabled_for_fork(fork_name) } @@ -47,7 +43,7 @@ pub trait Handler { let fork_name_str = fork_name.to_string(); let handler_path = PathBuf::from(env!("CARGO_MANIFEST_DIR")) - .join(self.tests_dir()) + .join("consensus-spec-tests") .join("tests") .join(Self::config_name()) .join(&fork_name_str) @@ -517,15 +513,13 @@ impl Handler for FinalityHandler { } pub struct ForkChoiceHandler { - tests_dir: String, handler_name: String, _phantom: PhantomData, } impl ForkChoiceHandler { - pub fn new(tests_dir: &str, handler_name: &str) -> Self { + pub fn new(handler_name: &str) -> Self { Self { - tests_dir: tests_dir.into(), handler_name: handler_name.into(), _phantom: PhantomData, } @@ -543,10 +537,6 @@ impl Handler for ForkChoiceHandler { "fork_choice" } - fn tests_dir(&self) -> String { - self.tests_dir.clone() - } - fn handler_name(&self) -> String { self.handler_name.clone() } diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index f828a1d5376..6409d2bccc3 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -479,60 +479,40 @@ fn finality() { FinalityHandler::::default().run(); } -#[allow(unused_macros)] // TODO(paul): remove `allow` once we have EF test release. -macro_rules! fork_choice_tests { - ($tests_dir: ident) => { - #[test] - fn fork_choice_get_head() { - ForkChoiceHandler::::new($tests_dir, "get_head").run(); - ForkChoiceHandler::::new($tests_dir, "get_head").run(); - } - - #[test] - fn fork_choice_on_block() { - ForkChoiceHandler::::new($tests_dir, "on_block").run(); - ForkChoiceHandler::::new($tests_dir, "on_block").run(); - } - - #[test] - fn fork_choice_on_merge_block() { - ForkChoiceHandler::::new($tests_dir, "on_merge_block").run(); - ForkChoiceHandler::::new($tests_dir, "on_merge_block").run(); - } - - #[test] - fn fork_choice_ex_ante() { - ForkChoiceHandler::::new($tests_dir, "ex_ante").run(); - ForkChoiceHandler::::new($tests_dir, "ex_ante").run(); - } - }; +#[test] +fn fork_choice_get_head() { + ForkChoiceHandler::::new("get_head").run(); + ForkChoiceHandler::::new("get_head").run(); } -// TODO(paul): The typical fork choice tests from the latest release will not -// pass due to the changes in PR #18. This must be resolved before we merge into -// `unstable`, but we are waiting on new test vectors from the EF. -// -// fork_choice_tests!("consensus_spec_tests"); - -#[cfg(feature = "fork_choice_custom")] -mod fork_choice_custom { - use super::*; +#[test] +fn fork_choice_on_block() { + ForkChoiceHandler::::new("on_block").run(); + ForkChoiceHandler::::new("on_block").run(); +} - const CUSTOM_FC_TESTS: &str = "custom-fc-tests"; +#[test] +fn fork_choice_on_merge_block() { + ForkChoiceHandler::::new("on_merge_block").run(); + ForkChoiceHandler::::new("on_merge_block").run(); +} - fork_choice_tests!(CUSTOM_FC_TESTS); +#[test] +fn fork_choice_ex_ante() { + ForkChoiceHandler::::new("ex_ante").run(); + ForkChoiceHandler::::new("ex_ante").run(); +} - #[test] - fn fork_choice_custom_reorg() { - ForkChoiceHandler::::new(CUSTOM_FC_TESTS, "reorg").run(); - ForkChoiceHandler::::new(CUSTOM_FC_TESTS, "reorg").run(); - } +#[test] +fn fork_choice_reorg() { + ForkChoiceHandler::::new("reorg").run(); + ForkChoiceHandler::::new("reorg").run(); +} - #[test] - fn fork_choice_custom_withholding() { - ForkChoiceHandler::::new(CUSTOM_FC_TESTS, "withholding").run(); - ForkChoiceHandler::::new(CUSTOM_FC_TESTS, "withholding").run(); - } +#[test] +fn fork_choice_withholding() { + ForkChoiceHandler::::new("withholding").run(); + ForkChoiceHandler::::new("withholding").run(); } #[test] From d4a2b02c708b07fe7ba643cf17b81c0a1ff2cf9e Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 2 Mar 2023 11:30:05 +1100 Subject: [PATCH 46/60] Remove some non-supplied tests --- testing/ef_tests/src/handler.rs | 6 ++++++ testing/ef_tests/tests/tests.rs | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/testing/ef_tests/src/handler.rs b/testing/ef_tests/src/handler.rs index c066bdafa48..58fab1a2d9c 100644 --- a/testing/ef_tests/src/handler.rs +++ b/testing/ef_tests/src/handler.rs @@ -552,6 +552,12 @@ impl Handler for ForkChoiceHandler { return false; } + // Withholding tests were introduced shortly before the Capella fork and + // have not been generated for prior forks. + if self.handler_name == "withholding" && fork_name == ForkName::Base { + return false; + } + // These tests check block validity (which may include signatures) and there is no need to // run them with fake crypto. cfg!(not(feature = "fake_crypto")) diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index 6409d2bccc3..33f8d67ec00 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -506,13 +506,13 @@ fn fork_choice_ex_ante() { #[test] fn fork_choice_reorg() { ForkChoiceHandler::::new("reorg").run(); - ForkChoiceHandler::::new("reorg").run(); + // There is no mainnet variant for this test. } #[test] fn fork_choice_withholding() { ForkChoiceHandler::::new("withholding").run(); - ForkChoiceHandler::::new("withholding").run(); + // There is no mainnet variant for this test. } #[test] From 558fe79ae395b4392e2bc8ed634515e5bc18ea2b Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 20 Mar 2023 15:14:09 +1100 Subject: [PATCH 47/60] Set tests to v1.3.0-rc.4 --- testing/ef_tests/Makefile | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/testing/ef_tests/Makefile b/testing/ef_tests/Makefile index 4911d170eac..f7562f477a2 100644 --- a/testing/ef_tests/Makefile +++ b/testing/ef_tests/Makefile @@ -1,5 +1,5 @@ -TESTS_TAG := v1.3.0-rc.3-pr18-1b09c23 -TESTS = minimal mainnet +TESTS_TAG := v1.3.0-rc.4 +TESTS = general minimal mainnet TARBALLS = $(patsubst %,%-$(TESTS_TAG).tar.gz,$(TESTS)) REPO_NAME := consensus-spec-tests @@ -30,6 +30,9 @@ $(BLS_OUTPUT_DIR): $(WGET) $(BLS_BASE_URL)/$(BLS_TEST).tar.gz -O $(BLS_TARBALL) tar -xzf $(BLS_TARBALL) -C $(BLS_OUTPUT_DIR) +%-$(TESTS_TAG).tar.gz: + $(WGET) $(BASE_URL)/$*.tar.gz -O $@ + clean-test-files: rm -rf $(OUTPUT_DIR) $(BLS_OUTPUT_DIR) From 023524bc393428a60b71c82d43d9f4a3c2cb4629 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 20 Mar 2023 15:22:47 +1100 Subject: [PATCH 48/60] Skip specific phase0 FC tests --- testing/ef_tests/src/handler.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/testing/ef_tests/src/handler.rs b/testing/ef_tests/src/handler.rs index 1c05e375286..87ef7156226 100644 --- a/testing/ef_tests/src/handler.rs +++ b/testing/ef_tests/src/handler.rs @@ -548,8 +548,11 @@ impl Handler for ForkChoiceHandler { } // Withholding tests were introduced shortly before the Capella fork and - // have not been generated for prior forks. - if self.handler_name == "withholding" && fork_name == ForkName::Base { + // have not been generated for prior forks.\ + let no_base_tests = ["ex_ante", "withholding", "on_block", "reorg", "get_head"] + .iter() + .any(|&name| self.handler_name == name); + if no_base_tests && fork_name == ForkName::Base { return false; } From 913abc02047fe0a4e25522ba94917d4e2fc3a740 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 20 Mar 2023 15:23:36 +1100 Subject: [PATCH 49/60] Skip all FC tests with base/phase0 --- testing/ef_tests/src/handler.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/testing/ef_tests/src/handler.rs b/testing/ef_tests/src/handler.rs index 87ef7156226..2ed596e25e4 100644 --- a/testing/ef_tests/src/handler.rs +++ b/testing/ef_tests/src/handler.rs @@ -547,12 +547,8 @@ impl Handler for ForkChoiceHandler { return false; } - // Withholding tests were introduced shortly before the Capella fork and - // have not been generated for prior forks.\ - let no_base_tests = ["ex_ante", "withholding", "on_block", "reorg", "get_head"] - .iter() - .any(|&name| self.handler_name == name); - if no_base_tests && fork_name == ForkName::Base { + // Tests are no longer generated for the base/phase0 specification. + if fork_name == ForkName::Base { return false; } From 42a8f4feb9f497a1ddd2443ed9aafbc1cd523194 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 20 Mar 2023 17:25:58 +1100 Subject: [PATCH 50/60] Remove `slashed_indices` field from `JustifiedBalances` --- .../src/beacon_fork_choice_store.rs | 12 ++-- .../proto_array/src/justified_balances.rs | 57 +++++++++++++------ 2 files changed, 46 insertions(+), 23 deletions(-) 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 13a3e78b54b..0b52a903434 100644 --- a/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs +++ b/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs @@ -193,9 +193,9 @@ where root: anchor_root, }; let finalized_checkpoint = justified_checkpoint; - let justified_balances = JustifiedBalances::from_justified_state(anchor_state)?; - let equivocating_indices = - BTreeSet::from_iter(justified_balances.slashed_indices.iter().copied()); + let (justified_balances, slashed_indices) = + JustifiedBalances::from_justified_state_with_equivocating_indices(anchor_state)?; + let equivocating_indices = BTreeSet::from_iter(slashed_indices.into_iter()); Ok(Self { store, @@ -331,9 +331,11 @@ where .map_err(Error::FailedToReadState)? .ok_or_else(|| Error::MissingState(justified_block.state_root()))?; - self.justified_balances = JustifiedBalances::from_justified_state(&state)?; + let (justified_balances, slashed_indices) = + JustifiedBalances::from_justified_state_with_equivocating_indices(&state)?; + self.justified_balances = justified_balances; self.equivocating_indices - .extend(self.justified_balances.slashed_indices.iter().copied()); + .extend(slashed_indices.into_iter()); } Ok(()) diff --git a/consensus/proto_array/src/justified_balances.rs b/consensus/proto_array/src/justified_balances.rs index 6adda98f913..924efedfc97 100644 --- a/consensus/proto_array/src/justified_balances.rs +++ b/consensus/proto_array/src/justified_balances.rs @@ -1,6 +1,5 @@ use safe_arith::{ArithError, SafeArith}; -use std::collections::HashSet; -use types::{BeaconState, EthSpec}; +use types::{BeaconState, Epoch, EthSpec, Validator}; #[derive(Debug, PartialEq, Clone, Default)] pub struct JustifiedBalances { @@ -13,26 +12,51 @@ pub struct JustifiedBalances { pub total_effective_balance: u64, /// The number of active validators included in `self.effective_balances`. pub num_active_validators: u64, - /// The set of all slashed validators. - pub slashed_indices: HashSet, } impl JustifiedBalances { pub fn from_justified_state(state: &BeaconState) -> Result { - let current_epoch = state.current_epoch(); + Self::from_justified_components(state.current_epoch(), &mut state.validators().iter()) + } + + /// Instantiates `Self` and returns a list of all slashed validator indices + /// in `state`, without performing any additional iterations over the + /// validator set. + pub fn from_justified_state_with_equivocating_indices( + state: &BeaconState, + ) -> Result<(Self, Vec), ArithError> { + let mut equivocating_indices = vec![]; + + let mut iter = state.validators().iter().enumerate().map(|(i, validator)| { + equivocating_indices.push(i as u64); + validator + }); + + let justified_balances = Self::from_justified_components(state.current_epoch(), &mut iter)?; + + // Ensure that the entirety of the iterator has been consumed. This is a + // paranoid check to defend against modifications to + // `Self::from_justified_changes` that might result in the `validators` + // iterator not visiting all validators. + iter.all(|_| true); + + Ok((justified_balances, equivocating_indices)) + } + + /// A generic method for generating `Self` from an iterator over the + /// validator set. + fn from_justified_components<'a, I>( + current_epoch: Epoch, + validators: &mut I, + ) -> Result + where + I: Iterator, + { let mut total_effective_balance = 0u64; let mut num_active_validators = 0u64; - let mut slashed_indices = HashSet::new(); - - let effective_balances = state - .validators() - .iter() - .enumerate() - .map(|(validator_index, validator)| { - if validator.slashed { - slashed_indices.insert(validator_index as u64); - } + let effective_balances = validators + .map(|validator| { if validator.is_active_at(current_epoch) { total_effective_balance.safe_add_assign(validator.effective_balance)?; num_active_validators.safe_add_assign(1)?; @@ -48,7 +72,6 @@ impl JustifiedBalances { effective_balances, total_effective_balance, num_active_validators, - slashed_indices, }) } @@ -67,8 +90,6 @@ impl JustifiedBalances { effective_balances, total_effective_balance, num_active_validators, - // TODO(paul): is an empty set sensible? - slashed_indices: <_>::default(), }) } } From 2dd0f6ce51a1c77d4e10b3616d8bb3e28b5bb681 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 20 Mar 2023 17:40:11 +1100 Subject: [PATCH 51/60] Fix bug with `JustifiedBalances` slashed indices --- consensus/proto_array/src/justified_balances.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/consensus/proto_array/src/justified_balances.rs b/consensus/proto_array/src/justified_balances.rs index 924efedfc97..76c0a592b1f 100644 --- a/consensus/proto_array/src/justified_balances.rs +++ b/consensus/proto_array/src/justified_balances.rs @@ -28,7 +28,9 @@ impl JustifiedBalances { let mut equivocating_indices = vec![]; let mut iter = state.validators().iter().enumerate().map(|(i, validator)| { - equivocating_indices.push(i as u64); + if validator.slashed { + equivocating_indices.push(i as u64); + } validator }); From 3f60f703dbd6dfdd1a7dfc3b0e4a3acbf64f5ebf Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 20 Mar 2023 17:40:33 +1100 Subject: [PATCH 52/60] Un-ignore skipped test files --- testing/ef_tests/check_all_files_accessed.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/testing/ef_tests/check_all_files_accessed.py b/testing/ef_tests/check_all_files_accessed.py index 81d111bee1a..6af8c037633 100755 --- a/testing/ef_tests/check_all_files_accessed.py +++ b/testing/ef_tests/check_all_files_accessed.py @@ -53,10 +53,6 @@ "bls12-381-tests/deserialization_G1", "bls12-381-tests/deserialization_G2", "bls12-381-tests/hash_to_G2", - # Fork choice (temporarily skipped) - # - # TODO(paul): these tests MUST be re-enabled before merging into unstable! - "tests/.*/.*/fork_choice" ] From 43a8405defc3eb3cc54cc62c9753e20891511e1b Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 20 Mar 2023 18:19:31 +1100 Subject: [PATCH 53/60] Deprecate `count-unrealized` flag --- beacon_node/beacon_chain/src/beacon_chain.rs | 2 +- beacon_node/beacon_chain/src/builder.rs | 4 ++-- beacon_node/beacon_chain/src/chain_config.rs | 4 ---- beacon_node/src/cli.rs | 4 +--- beacon_node/src/config.rs | 9 +++++++-- consensus/fork_choice/src/fork_choice.rs | 18 ------------------ lighthouse/tests/beacon_node.rs | 15 +++++++++------ testing/ef_tests/src/cases/fork_choice.rs | 2 +- 8 files changed, 21 insertions(+), 37 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 1bf990c91b0..9ebfc6fc868 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -2835,7 +2835,7 @@ impl BeaconChain { &state, payload_verification_status, &self.spec, - count_unrealized.and(self.config.count_unrealized.into()), + count_unrealized, ) .map_err(|e| BlockError::BeaconChainError(e.into()))?; } diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index 375f5b69ee4..a7f76e33f97 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -18,7 +18,7 @@ use crate::{ }; use eth1::Config as Eth1Config; use execution_layer::ExecutionLayer; -use fork_choice::{ForkChoice, ResetPayloadStatuses}; +use fork_choice::{CountUnrealized, ForkChoice, ResetPayloadStatuses}; use futures::channel::mpsc::Sender; use operation_pool::{OperationPool, PersistedOperationPool}; use parking_lot::RwLock; @@ -678,7 +678,7 @@ where store.clone(), Some(current_slot), &self.spec, - self.chain_config.count_unrealized.into(), + CountUnrealized::True, )?; } diff --git a/beacon_node/beacon_chain/src/chain_config.rs b/beacon_node/beacon_chain/src/chain_config.rs index 69b49dde244..9a29ec6f587 100644 --- a/beacon_node/beacon_chain/src/chain_config.rs +++ b/beacon_node/beacon_chain/src/chain_config.rs @@ -48,9 +48,6 @@ pub struct ChainConfig { pub builder_fallback_epochs_since_finalization: usize, /// Whether any chain health checks should be considered when deciding whether to use the builder API. pub builder_fallback_disable_checks: bool, - /// When set to `true`, weigh the "unrealized" FFG progression when choosing a head in fork - /// choice. - pub count_unrealized: bool, /// When set to `true`, forget any valid/invalid/optimistic statuses in fork choice during start /// up. pub always_reset_payload_statuses: bool, @@ -87,7 +84,6 @@ impl Default for ChainConfig { builder_fallback_skips_per_epoch: 8, builder_fallback_epochs_since_finalization: 3, builder_fallback_disable_checks: false, - count_unrealized: true, always_reset_payload_statuses: false, paranoid_block_proposal: false, checkpoint_sync_url_timeout: 60, diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs index 251b79c9d09..5cc46527a5d 100644 --- a/beacon_node/src/cli.rs +++ b/beacon_node/src/cli.rs @@ -958,10 +958,8 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { Arg::with_name("count-unrealized") .long("count-unrealized") .hidden(true) - .help("Enables an alternative, potentially more performant FFG \ - vote tracking method.") + .help("This flag is deprecated and has no effect.") .takes_value(true) - .default_value("true") ) .arg( Arg::with_name("count-unrealized-full") diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index 4a3a43f0646..16e003b1efe 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -705,8 +705,13 @@ pub fn get_config( client_config.chain.fork_choice_before_proposal_timeout_ms = timeout; } - client_config.chain.count_unrealized = - clap_utils::parse_required(cli_args, "count-unrealized")?; + if cli_args.is_present("count-unrealized") { + warn!( + log, + "The flag --count-unrealized is deprecated and will be removed"; + "info" => "any use of the flag will have no effect" + ); + } if clap_utils::parse_required::(cli_args, "count-unrealized-full")? { warn!( diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 0430c2a1930..8a4e35f454b 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -187,24 +187,6 @@ impl CountUnrealized { pub fn is_true(&self) -> bool { matches!(self, CountUnrealized::True) } - - pub fn and(&self, other: CountUnrealized) -> CountUnrealized { - if self.is_true() && other.is_true() { - CountUnrealized::True - } else { - CountUnrealized::False - } - } -} - -impl From for CountUnrealized { - fn from(count_unrealized: bool) -> Self { - if count_unrealized { - CountUnrealized::True - } else { - CountUnrealized::False - } - } } /// Indicates if a block has been verified by an execution payload. diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index b0c7e87e9ed..2c58b34a9de 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -223,24 +223,27 @@ fn count_unrealized_default() { fn count_unrealized_no_arg() { CommandLineTest::new() .flag("count-unrealized", None) - .run_with_zero_port() - .with_config(|config| assert!(config.chain.count_unrealized)); + // This flag should be ignored, so there's nothing to test but that the + // client starts with the flag present. + .run_with_zero_port(); } #[test] fn count_unrealized_false() { CommandLineTest::new() .flag("count-unrealized", Some("false")) - .run_with_zero_port() - .with_config(|config| assert!(!config.chain.count_unrealized)); + // This flag should be ignored, so there's nothing to test but that the + // client starts with the flag present. + .run_with_zero_port(); } #[test] fn count_unrealized_true() { CommandLineTest::new() .flag("count-unrealized", Some("true")) - .run_with_zero_port() - .with_config(|config| assert!(config.chain.count_unrealized)); + // This flag should be ignored, so there's nothing to test but that the + // client starts with the flag present. + .run_with_zero_port(); } #[test] diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index bf259581bcb..7c3154a3289 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -441,7 +441,7 @@ impl Tester { &state, PayloadVerificationStatus::Irrelevant, &self.harness.chain.spec, - self.harness.chain.config.count_unrealized.into(), + CountUnrealized::True, ); if result.is_ok() { From dd6109ca303f876735df9a56ce2700710ff57274 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 20 Mar 2023 19:00:28 +1100 Subject: [PATCH 54/60] Update equivocating indices in `process_state` --- .../src/beacon_fork_choice_store.rs | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) 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 0b52a903434..193060f827f 100644 --- a/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs +++ b/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs @@ -21,6 +21,8 @@ use types::{ Hash256, Slot, }; +type SlashedIndices = Vec; + /// Ensure this justified checkpoint has an epoch of 0 so that it is never /// greater than the justified checkpoint and enshrined as the actual justified /// checkpoint. @@ -94,7 +96,7 @@ impl BalancesCache { &mut self, block_root: Hash256, state: &BeaconState, - ) -> Result<(), Error> { + ) -> Result, Error> { let epoch = state.current_epoch(); let epoch_boundary_slot = epoch.start_slot(E::slots_per_epoch()); let epoch_boundary_root = if epoch_boundary_slot == state.slot() { @@ -110,10 +112,13 @@ impl BalancesCache { // of a single epoch, so even if the block on the epoch boundary itself is skipped we can // still update its cache entry from any subsequent state in that epoch. if self.position(epoch_boundary_root, epoch).is_none() { + let (justified_balances, slashed_indices) = + JustifiedBalances::from_justified_state_with_equivocating_indices(state)?; + let item = CacheItem { block_root: epoch_boundary_root, epoch, - balances: JustifiedBalances::from_justified_state(state)?.effective_balances, + balances: justified_balances.effective_balances, }; if self.items.len() == MAX_BALANCE_CACHE_SIZE { @@ -121,9 +126,11 @@ impl BalancesCache { } self.items.push(item); - } - Ok(()) + Ok(Some(slashed_indices)) + } else { + Ok(None) + } } fn position(&self, block_root: Hash256, epoch: Epoch) -> Option { @@ -274,7 +281,14 @@ where block_root: Hash256, state: &BeaconState, ) -> Result<(), Self::Error> { - self.balances_cache.process_state(block_root, state) + if let Some(slashed_indices) = self.balances_cache.process_state(block_root, state)? { + // Note the equivocating indices for any state which could + // potentially be a justified state (i.e. an epoch boundary state). + self.equivocating_indices + .extend(slashed_indices.into_iter()) + } + + Ok(()) } fn justified_checkpoint(&self) -> &Checkpoint { From 5377ac612cfa1fc92aad76960c0123029f40c2a7 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 20 Mar 2023 20:07:34 +1100 Subject: [PATCH 55/60] Censor slashed validators from JustifiedBalances --- .../src/beacon_fork_choice_store.rs | 37 ++++----------- .../proto_array/src/justified_balances.rs | 47 +++---------------- 2 files changed, 14 insertions(+), 70 deletions(-) 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 193060f827f..71160fcb638 100644 --- a/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs +++ b/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs @@ -11,7 +11,6 @@ use proto_array::JustifiedBalances; use safe_arith::ArithError; use ssz_derive::{Decode, Encode}; use std::collections::BTreeSet; -use std::iter::FromIterator; use std::marker::PhantomData; use std::sync::Arc; use store::{Error as StoreError, HotColdDB, ItemStore}; @@ -21,8 +20,6 @@ use types::{ Hash256, Slot, }; -type SlashedIndices = Vec; - /// Ensure this justified checkpoint has an epoch of 0 so that it is never /// greater than the justified checkpoint and enshrined as the actual justified /// checkpoint. @@ -96,7 +93,7 @@ impl BalancesCache { &mut self, block_root: Hash256, state: &BeaconState, - ) -> Result, Error> { + ) -> Result<(), Error> { let epoch = state.current_epoch(); let epoch_boundary_slot = epoch.start_slot(E::slots_per_epoch()); let epoch_boundary_root = if epoch_boundary_slot == state.slot() { @@ -112,13 +109,10 @@ impl BalancesCache { // of a single epoch, so even if the block on the epoch boundary itself is skipped we can // still update its cache entry from any subsequent state in that epoch. if self.position(epoch_boundary_root, epoch).is_none() { - let (justified_balances, slashed_indices) = - JustifiedBalances::from_justified_state_with_equivocating_indices(state)?; - let item = CacheItem { block_root: epoch_boundary_root, epoch, - balances: justified_balances.effective_balances, + balances: JustifiedBalances::from_justified_state(state)?.effective_balances, }; if self.items.len() == MAX_BALANCE_CACHE_SIZE { @@ -126,11 +120,9 @@ impl BalancesCache { } self.items.push(item); - - Ok(Some(slashed_indices)) - } else { - Ok(None) } + + Ok(()) } fn position(&self, block_root: Hash256, epoch: Epoch) -> Option { @@ -200,9 +192,7 @@ where root: anchor_root, }; let finalized_checkpoint = justified_checkpoint; - let (justified_balances, slashed_indices) = - JustifiedBalances::from_justified_state_with_equivocating_indices(anchor_state)?; - let equivocating_indices = BTreeSet::from_iter(slashed_indices.into_iter()); + let justified_balances = JustifiedBalances::from_justified_state(anchor_state)?; Ok(Self { store, @@ -214,7 +204,7 @@ where unrealized_justified_checkpoint: justified_checkpoint, unrealized_finalized_checkpoint: finalized_checkpoint, proposer_boost_root: Hash256::zero(), - equivocating_indices, + equivocating_indices: BTreeSet::new(), _phantom: PhantomData, }) } @@ -281,14 +271,7 @@ where block_root: Hash256, state: &BeaconState, ) -> Result<(), Self::Error> { - if let Some(slashed_indices) = self.balances_cache.process_state(block_root, state)? { - // Note the equivocating indices for any state which could - // potentially be a justified state (i.e. an epoch boundary state). - self.equivocating_indices - .extend(slashed_indices.into_iter()) - } - - Ok(()) + self.balances_cache.process_state(block_root, state) } fn justified_checkpoint(&self) -> &Checkpoint { @@ -345,11 +328,7 @@ where .map_err(Error::FailedToReadState)? .ok_or_else(|| Error::MissingState(justified_block.state_root()))?; - let (justified_balances, slashed_indices) = - JustifiedBalances::from_justified_state_with_equivocating_indices(&state)?; - self.justified_balances = justified_balances; - self.equivocating_indices - .extend(slashed_indices.into_iter()); + self.justified_balances = JustifiedBalances::from_justified_state(&state)?; } Ok(()) diff --git a/consensus/proto_array/src/justified_balances.rs b/consensus/proto_array/src/justified_balances.rs index 76c0a592b1f..c8787817f1a 100644 --- a/consensus/proto_array/src/justified_balances.rs +++ b/consensus/proto_array/src/justified_balances.rs @@ -1,5 +1,5 @@ use safe_arith::{ArithError, SafeArith}; -use types::{BeaconState, Epoch, EthSpec, Validator}; +use types::{BeaconState, EthSpec}; #[derive(Debug, PartialEq, Clone, Default)] pub struct JustifiedBalances { @@ -16,50 +16,15 @@ pub struct JustifiedBalances { impl JustifiedBalances { pub fn from_justified_state(state: &BeaconState) -> Result { - Self::from_justified_components(state.current_epoch(), &mut state.validators().iter()) - } - - /// Instantiates `Self` and returns a list of all slashed validator indices - /// in `state`, without performing any additional iterations over the - /// validator set. - pub fn from_justified_state_with_equivocating_indices( - state: &BeaconState, - ) -> Result<(Self, Vec), ArithError> { - let mut equivocating_indices = vec![]; - - let mut iter = state.validators().iter().enumerate().map(|(i, validator)| { - if validator.slashed { - equivocating_indices.push(i as u64); - } - validator - }); - - let justified_balances = Self::from_justified_components(state.current_epoch(), &mut iter)?; - - // Ensure that the entirety of the iterator has been consumed. This is a - // paranoid check to defend against modifications to - // `Self::from_justified_changes` that might result in the `validators` - // iterator not visiting all validators. - iter.all(|_| true); - - Ok((justified_balances, equivocating_indices)) - } - - /// A generic method for generating `Self` from an iterator over the - /// validator set. - fn from_justified_components<'a, I>( - current_epoch: Epoch, - validators: &mut I, - ) -> Result - where - I: Iterator, - { + let current_epoch = state.current_epoch(); let mut total_effective_balance = 0u64; let mut num_active_validators = 0u64; - let effective_balances = validators + let effective_balances = state + .validators() + .iter() .map(|validator| { - if validator.is_active_at(current_epoch) { + if !validator.slashed && validator.is_active_at(current_epoch) { total_effective_balance.safe_add_assign(validator.effective_balance)?; num_active_validators.safe_add_assign(1)?; From d47a3e484dcd585480e6d969d8d9ba128e3d0314 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 20 Mar 2023 20:29:13 +1100 Subject: [PATCH 56/60] Fix test compilation issue --- lighthouse/tests/beacon_node.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index 2c58b34a9de..b2c9df810f5 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -212,13 +212,6 @@ fn paranoid_block_proposal_on() { .with_config(|config| assert!(config.chain.paranoid_block_proposal)); } -#[test] -fn count_unrealized_default() { - CommandLineTest::new() - .run_with_zero_port() - .with_config(|config| assert!(config.chain.count_unrealized)); -} - #[test] fn count_unrealized_no_arg() { CommandLineTest::new() From 77a49708d7150f9808571b07017f9533e3c0ead0 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 21 Mar 2023 10:24:33 +1100 Subject: [PATCH 57/60] Add migration to drop balances cache --- beacon_node/beacon_chain/src/schema_change.rs | 9 ++++ .../src/schema_change/migration_schema_v16.rs | 46 +++++++++++++++++++ beacon_node/store/src/metadata.rs | 2 +- 3 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 beacon_node/beacon_chain/src/schema_change/migration_schema_v16.rs diff --git a/beacon_node/beacon_chain/src/schema_change.rs b/beacon_node/beacon_chain/src/schema_change.rs index 35202a3c5d3..5808e648a2c 100644 --- a/beacon_node/beacon_chain/src/schema_change.rs +++ b/beacon_node/beacon_chain/src/schema_change.rs @@ -3,6 +3,7 @@ mod migration_schema_v12; mod migration_schema_v13; mod migration_schema_v14; mod migration_schema_v15; +mod migration_schema_v16; use crate::beacon_chain::{BeaconChainTypes, ETH1_CACHE_DB_KEY}; use crate::eth1_chain::SszEth1; @@ -132,6 +133,14 @@ pub fn migrate_schema( let ops = migration_schema_v15::downgrade_from_v15::(db.clone(), log)?; db.store_schema_version_atomically(to, ops) } + (SchemaVersion(15), SchemaVersion(16)) => { + let ops = migration_schema_v16::upgrade_to_v16::(db.clone(), log)?; + db.store_schema_version_atomically(to, ops) + } + (SchemaVersion(16), SchemaVersion(15)) => { + let ops = migration_schema_v16::downgrade_from_v16::(db.clone(), log)?; + db.store_schema_version_atomically(to, ops) + } // Anything else is an error. (_, _) => Err(HotColdDBError::UnsupportedSchemaVersion { target_version: to, diff --git a/beacon_node/beacon_chain/src/schema_change/migration_schema_v16.rs b/beacon_node/beacon_chain/src/schema_change/migration_schema_v16.rs new file mode 100644 index 00000000000..230573b0288 --- /dev/null +++ b/beacon_node/beacon_chain/src/schema_change/migration_schema_v16.rs @@ -0,0 +1,46 @@ +use crate::beacon_chain::{BeaconChainTypes, FORK_CHOICE_DB_KEY}; +use crate::persisted_fork_choice::PersistedForkChoiceV11; +use slog::{debug, Logger}; +use std::sync::Arc; +use store::{Error, HotColdDB, KeyValueStoreOp, StoreItem}; + +pub fn upgrade_to_v16( + db: Arc>, + log: Logger, +) -> Result, Error> { + drop_balances_cache::(db, log) +} + +pub fn downgrade_from_v16( + db: Arc>, + log: Logger, +) -> Result, Error> { + drop_balances_cache::(db, log) +} + +/// Drop the balances cache from the fork choice store. +/// +/// There aren't any type-level changes in this schema migration, however the +/// way that we compute the `JustifiedBalances` has changed due to: +/// https://github.com/sigp/lighthouse/pull/3962 +pub fn drop_balances_cache( + db: Arc>, + log: Logger, +) -> Result, Error> { + let mut persisted_fork_choice = db + .get_item::(&FORK_CHOICE_DB_KEY)? + .ok_or_else(|| Error::SchemaMigrationError("fork choice missing from database".into()))?; + + debug!( + log, + "Dropping fork choice balances cache"; + "item_count" => persisted_fork_choice.fork_choice_store.balances_cache.items.len() + ); + + // Drop all items in the balances cache. + persisted_fork_choice.fork_choice_store.balances_cache = <_>::default(); + + let kv_op = persisted_fork_choice.as_kv_store_op(FORK_CHOICE_DB_KEY); + + Ok(vec![kv_op]) +} diff --git a/beacon_node/store/src/metadata.rs b/beacon_node/store/src/metadata.rs index 729b36ff2e6..8e9b3599b14 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(15); +pub const CURRENT_SCHEMA_VERSION: SchemaVersion = SchemaVersion(16); // All the keys that get stored under the `BeaconMeta` column. // From 9216bdaf1f24c31af999cd2496c7bb8560a4ab9e Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 21 Mar 2023 12:22:28 +1100 Subject: [PATCH 58/60] Fix failing CLI test --- beacon_node/src/cli.rs | 1 + beacon_node/src/config.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs index 5cc46527a5d..afb408e5b8f 100644 --- a/beacon_node/src/cli.rs +++ b/beacon_node/src/cli.rs @@ -960,6 +960,7 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { .hidden(true) .help("This flag is deprecated and has no effect.") .takes_value(true) + .default_value("true") ) .arg( Arg::with_name("count-unrealized-full") diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index 16e003b1efe..39905a24e12 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -705,7 +705,7 @@ pub fn get_config( client_config.chain.fork_choice_before_proposal_timeout_ms = timeout; } - if cli_args.is_present("count-unrealized") { + if clap_utils::parse_required::(cli_args, "count-unrealized")? == false { warn!( log, "The flag --count-unrealized is deprecated and will be removed"; From 7cc1ab1acfe3108e15aa199986168559beebc3ef Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 21 Mar 2023 12:55:44 +1100 Subject: [PATCH 59/60] Appease clippy --- beacon_node/src/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index 39905a24e12..6fc730e23db 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -705,7 +705,7 @@ pub fn get_config( client_config.chain.fork_choice_before_proposal_timeout_ms = timeout; } - if clap_utils::parse_required::(cli_args, "count-unrealized")? == false { + if !clap_utils::parse_required::(cli_args, "count-unrealized")? { warn!( log, "The flag --count-unrealized is deprecated and will be removed"; From d2fefc417a4cb07760deb3cb5b59563f7f211362 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 21 Mar 2023 13:53:38 +1100 Subject: [PATCH 60/60] Tidy diff --- testing/ef_tests/.gitignore | 1 - testing/ef_tests/Cargo.toml | 1 - testing/ef_tests/check_all_files_accessed.py | 2 +- 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/testing/ef_tests/.gitignore b/testing/ef_tests/.gitignore index 10d36e13f06..6a2ca1fe754 100644 --- a/testing/ef_tests/.gitignore +++ b/testing/ef_tests/.gitignore @@ -1,4 +1,3 @@ /consensus-spec-tests .accessed_file_log.txt /bls12-381-tests -/custom-fc-tests diff --git a/testing/ef_tests/Cargo.toml b/testing/ef_tests/Cargo.toml index 5ae7ec8cbc0..79664a26228 100644 --- a/testing/ef_tests/Cargo.toml +++ b/testing/ef_tests/Cargo.toml @@ -9,7 +9,6 @@ edition = "2021" ef_tests = [] milagro = ["bls/milagro"] fake_crypto = ["bls/fake_crypto"] -fork_choice_custom = [] [dependencies] bls = { path = "../../crypto/bls", default-features = false } diff --git a/testing/ef_tests/check_all_files_accessed.py b/testing/ef_tests/check_all_files_accessed.py index 6af8c037633..b52d1552244 100755 --- a/testing/ef_tests/check_all_files_accessed.py +++ b/testing/ef_tests/check_all_files_accessed.py @@ -52,7 +52,7 @@ # some bls tests are not included now "bls12-381-tests/deserialization_G1", "bls12-381-tests/deserialization_G2", - "bls12-381-tests/hash_to_G2", + "bls12-381-tests/hash_to_G2" ]