Merge remote-tracking branch 'origin/gloas-fix-proposer-pref-gossip-verification' into glamsterdam-devnet-6

beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs

@@ -142,9 +142,18 @@ impl<T: BeaconChainTypes> GossipVerifiedPayloadBid<T> {
             .ok_or(PayloadBidError::UnableToReadSlot)?;
         let head_state = &cached_head.snapshot.beacon_state;
 
+        // Look up the preferences keyed by the dependent root that is canonical from our head's
+        // perspective, so we don't pick up preferences cached for a competing branch's proposer.
+        let proposal_epoch = bid_slot.epoch(T::EthSpec::slots_per_epoch());
+        let dependent_root = head_state.proposer_shuffling_decision_root_at_epoch(
+            proposal_epoch,
+            cached_head.head_block_root(),
+            ctx.spec,
+        )?;
+
         let Some(proposer_preferences) = ctx
             .gossip_verified_proposer_preferences_cache
-            .get_preferences(&bid_slot)
+            .get_preferences(&bid_slot, dependent_root)
         else {
             return Err(PayloadBidError::NoProposerPreferences { slot: bid_slot });
         };

beacon_node/beacon_chain/src/payload_bid_verification/tests.rs

@@ -264,10 +264,11 @@ fn make_signed_preferences(
     validator_index: u64,
     fee_recipient: Address,
     target_gas_limit: u64,
+    dependent_root: Hash256,
 ) -> Arc<SignedProposerPreferences> {
     Arc::new(SignedProposerPreferences {
         message: ProposerPreferences {
-            dependent_root: Hash256::ZERO,
+            dependent_root,
             proposal_slot,
             validator_index,
             fee_recipient,
@@ -278,8 +279,25 @@ fn make_signed_preferences(
 }
 
 fn seed_preferences(ctx: &TestContext, slot: Slot, fee_recipient: Address, gas_limit: u64) {
+    // Key the preferences by the same dependent root that gossip verification will compute from
+    // the head state, otherwise the lookup misses and verification returns `NoProposerPreferences`.
+    let cached_head = ctx.canonical_head.cached_head();
+    let head_state = &cached_head.snapshot.beacon_state;
+    let dependent_root = head_state
+        .proposer_shuffling_decision_root_at_epoch(
+            slot.epoch(E::slots_per_epoch()),
+            cached_head.head_block_root(),
+            &ctx.spec,
+        )
+        .expect("should compute proposer shuffling decision root");
     let prefs = GossipVerifiedProposerPreferences {
-        signed_preferences: make_signed_preferences(slot, 0, fee_recipient, gas_limit),
+        signed_preferences: make_signed_preferences(
+            slot,
+            0,
+            fee_recipient,
+            gas_limit,
+            dependent_root,
+        ),
     };
     ctx.preferences_cache.insert_preferences(prefs);
 }

beacon_node/beacon_chain/src/proposer_preferences_verification/gossip_verified_proposer_preferences.rs

@@ -61,7 +61,6 @@ impl GossipVerifiedProposerPreferences {
         let proposal_slot = signed_preferences.message.proposal_slot;
         let dependent_root = signed_preferences.message.dependent_root;
         let validator_index = signed_preferences.message.validator_index;
-        let cached_head = ctx.canonical_head.cached_head();
         let current_slot = ctx
             .slot_clock
             .now()
@@ -83,15 +82,13 @@ impl GossipVerifiedProposerPreferences {
             ctx.spec,
         )?;
 
-        // Get the block at dependent_root from fork choice to verify canonicity and get state_root
+        // Get the block at dependent_root from fork choice to fetch its state_root. The block
+        // need not be canonical: preferences for a non-canonical branch are still verifiable, and
+        // the dependent state lives in the hot DB.
         let fork_choice = ctx.canonical_head.fork_choice_read_lock();
         let dependent_block = fork_choice
             .get_block(&dependent_root)
             .ok_or(ProposerPreferencesError::DependentRootUnknown { dependent_root })?;
-        let head_root = cached_head.head_block_root();
-        if !fork_choice.is_descendant(dependent_root, head_root) {
-            return Err(ProposerPreferencesError::DependentRootNotCanonical { dependent_root });
-        }
         let dependent_state_root = dependent_block.state_root;
         drop(fork_choice);
 

beacon_node/beacon_chain/src/proposer_preferences_verification/mod.rs

@@ -40,8 +40,6 @@ pub enum ProposerPreferencesError {
     UnableToReadSlot,
     /// The block with root `dependent_root` has not been seen.
     DependentRootUnknown { dependent_root: Hash256 },
-    /// The block with root `dependent_root` is not canonical.
-    DependentRootNotCanonical { dependent_root: Hash256 },
     /// A valid message from this validator for this slot has already been seen.
     AlreadySeen {
         validator_index: u64,

beacon_node/beacon_chain/src/proposer_preferences_verification/proposer_preference_cache.rs

@@ -8,7 +8,9 @@ use parking_lot::RwLock;
 use types::{Hash256, SignedProposerPreferences, Slot};
 
 pub struct GossipVerifiedProposerPreferenceCache {
-    preferences: RwLock<BTreeMap<Slot, GossipVerifiedProposerPreferences>>,
+    /// Mapping of `(proposal_slot, dependent_root)` to `GossipVerifiedProposerPreferences`
+    preferences: RwLock<BTreeMap<(Slot, Hash256), GossipVerifiedProposerPreferences>>,
+    /// Mapping of `proposal_slot` to `(dependent_root, validator_index)`
     seen: RwLock<BTreeMap<Slot, HashSet<(Hash256, u64)>>>,
 }
 
@@ -22,16 +24,23 @@ impl Default for GossipVerifiedProposerPreferenceCache {
 }
 
 impl GossipVerifiedProposerPreferenceCache {
-    pub fn get_preferences(&self, slot: &Slot) -> Option<Arc<SignedProposerPreferences>> {
+    pub fn get_preferences(
+        &self,
+        slot: &Slot,
+        dependent_root: Hash256,
+    ) -> Option<Arc<SignedProposerPreferences>> {
         self.preferences
             .read()
-            .get(slot)
+            .get(&(*slot, dependent_root))
             .map(|p| p.signed_preferences.clone())
     }
 
     pub fn insert_preferences(&self, preferences: GossipVerifiedProposerPreferences) {
         let slot = preferences.signed_preferences.message.proposal_slot;
-        self.preferences.write().insert(slot, preferences);
+        let dependent_root = preferences.signed_preferences.message.dependent_root;
+        self.preferences
+            .write()
+            .insert((slot, dependent_root), preferences);
     }
 
     pub fn get_seen_validator(
@@ -60,7 +69,7 @@ impl GossipVerifiedProposerPreferenceCache {
     pub fn prune(&self, current_slot: Slot) {
         self.preferences
             .write()
-            .retain(|&slot, _| slot >= current_slot);
+            .retain(|&(slot, _), _| slot >= current_slot);
         self.seen.write().retain(|&slot, _| slot >= current_slot);
     }
 }
@@ -108,15 +117,39 @@ mod tests {
         cache.prune(Slot::new(8));
 
         for slot in [1, 2, 3, 7] {
-            assert!(cache.get_preferences(&Slot::new(slot)).is_none());
+            assert!(cache.get_preferences(&Slot::new(slot), root).is_none());
             assert!(!cache.get_seen_validator(&Slot::new(slot), root, slot));
         }
         for slot in [8, 9, 10] {
-            assert!(cache.get_preferences(&Slot::new(slot)).is_some());
+            assert!(cache.get_preferences(&Slot::new(slot), root).is_some());
             assert!(cache.get_seen_validator(&Slot::new(slot), root, slot));
         }
     }
 
+    #[test]
+    fn different_dependent_roots_not_overwritten() {
+        let cache = GossipVerifiedProposerPreferenceCache::default();
+        let slot = Slot::new(5);
+        let root_a = Hash256::repeat_byte(0xaa);
+        let root_b = Hash256::repeat_byte(0xbb);
+
+        // Two competing branches each have a (different) proposer for the same slot.
+        let verified_a = make_gossip_verified(slot, 1, root_a);
+        let verified_b = make_gossip_verified(slot, 2, root_b);
+        cache.insert_preferences(verified_a);
+        cache.insert_preferences(verified_b);
+
+        // Neither branch's preferences overwrite the other; each is retrievable by its dependent root.
+        let prefs_a = cache
+            .get_preferences(&slot, root_a)
+            .expect("root_a present");
+        let prefs_b = cache
+            .get_preferences(&slot, root_b)
+            .expect("root_b present");
+        assert_eq!(prefs_a.message.validator_index, 1);
+        assert_eq!(prefs_b.message.validator_index, 2);
+    }
+
     #[test]
     fn different_dependent_roots_not_deduped() {
         let cache = GossipVerifiedProposerPreferenceCache::default();

beacon_node/beacon_chain/src/proposer_preferences_verification/tests.rs

@@ -256,7 +256,11 @@ fn correct_proposer_bad_signature() {
         !ctx.preferences_cache
             .get_seen_validator(&slot, ctx.head_block_root, actual_proposer)
     );
-    assert!(ctx.preferences_cache.get_preferences(&slot).is_none());
+    assert!(
+        ctx.preferences_cache
+            .get_preferences(&slot, ctx.head_block_root)
+            .is_none()
+    );
 }
 
 #[test]

beacon_node/network/src/network_beacon_processor/gossip_methods.rs

@@ -4081,8 +4081,7 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
                 | ProposerPreferencesError::BeaconChainError(_)
                 | ProposerPreferencesError::BeaconStateError(_)
                 | ProposerPreferencesError::UnableToReadSlot
-                | ProposerPreferencesError::DependentRootUnknown { .. }
+                | ProposerPreferencesError::DependentRootUnknown { .. },
-                | ProposerPreferencesError::DependentRootNotCanonical { .. },
             ) => {
                 self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore);
             }