diff --git a/beacon_node/beacon_chain/src/beacon_block_reward.rs b/beacon_node/beacon_chain/src/beacon_block_reward.rs index d345e6f5599..7eaaa3da702 100644 --- a/beacon_node/beacon_chain/src/beacon_block_reward.rs +++ b/beacon_node/beacon_chain/src/beacon_block_reward.rs @@ -82,7 +82,7 @@ impl BeaconChain { BeaconChainError::BlockRewardAttestationError })? } else { - self.compute_beacon_block_attestation_reward_altair_deneb(block, state) + self.compute_beacon_block_attestation_reward_altair_and_later(block, state) .map_err(|e| { error!( error = ?e, @@ -249,7 +249,7 @@ impl BeaconChain { Ok(block_reward) } - fn compute_beacon_block_attestation_reward_altair_deneb< + fn compute_beacon_block_attestation_reward_altair_and_later< Payload: AbstractExecPayload, >( &self, @@ -267,13 +267,20 @@ impl BeaconChain { let mut previous_epoch_participation = state.previous_epoch_participation()?.to_owned_list(); + let parent_slot = state + .latest_execution_payload_bid() + .ok() + .map(|bid| bid.slot); + for attestation in block.body().attestations() { let data = attestation.data(); let inclusion_delay = state.slot().safe_sub(data.slot)?.as_u64(); + // [Modified in Deneb:EIP7045] let participation_flag_indices = get_attestation_participation_flag_indices( state, data, + parent_slot, inclusion_delay, &self.spec, )?; diff --git a/beacon_node/beacon_chain/src/block_production/gloas.rs b/beacon_node/beacon_chain/src/block_production/gloas.rs index 80895ebbcb8..655db71ea18 100644 --- a/beacon_node/beacon_chain/src/block_production/gloas.rs +++ b/beacon_node/beacon_chain/src/block_production/gloas.rs @@ -39,6 +39,7 @@ use types::{ Withdrawals, }; +use crate::payload_bid_verification::payload_bid_cache::BidParent; use crate::pending_payload_envelopes::PendingEnvelopeData; use crate::{ BeaconChain, BeaconChainError, BeaconChainTypes, BlockProductionError, @@ -924,8 +925,7 @@ impl BeaconChain { ) -> WinningBid { let cached_bid = self.gossip_verified_payload_bid_cache.get_highest_bid( local_signed_bid.message.slot, - local_signed_bid.message.parent_block_hash, - local_signed_bid.message.parent_block_root, + BidParent::from_bid(&local_signed_bid.message), ); select_payload_bid_pure( local_signed_bid, diff --git a/beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs b/beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs index 1e2f779163a..00168a58c9f 100644 --- a/beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs +++ b/beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs @@ -1,8 +1,12 @@ use std::sync::Arc; use crate::{ - BeaconChain, BeaconChainTypes, CanonicalHead, - payload_bid_verification::{PayloadBidError, payload_bid_cache::GossipVerifiedPayloadBidCache}, + BeaconChain, BeaconChainTypes, BeaconStore, CachedHead, CanonicalHead, + canonical_head::ForkChoiceReadGuard, + payload_bid_verification::{ + PayloadBidError, + payload_bid_cache::{BidParent, GossipVerifiedPayloadBidCache}, + }, proposer_preferences_verification::proposer_preference_cache::GossipVerifiedProposerPreferenceCache, }; use educe::Educe; @@ -82,12 +86,84 @@ pub(crate) fn verify_bid_consistency( Ok(()) } +/// Checks if `bid` is compatible with the head branch +pub(crate) fn is_bid_compatible_with_head( + cached_head: &CachedHead, + fork_choice_read: &ForkChoiceReadGuard<'_, T>, + bid: &ExecutionPayloadBid, + spec: &ChainSpec, +) -> Result { + let head_block_root = cached_head.head_block_root(); + + let head_block = fork_choice_read + .get_block(&head_block_root) + .ok_or_else(|| { + PayloadBidError::InternalError(format!( + "head block {head_block_root:?} not found in fork choice" + )) + })?; + + // TODO(post-gloas) this can be removed after the gloas fork + let head_is_pre_gloas = !spec + .fork_name_at_slot::(head_block.slot) + .gloas_enabled(); + + let (head_bid_parent_block_hash, head_bid_block_hash) = if head_is_pre_gloas { + let parent_payload_hash = head_block + .parent_root + .and_then(|parent_root| fork_choice_read.get_block(&parent_root)) + .and_then(|parent| parent.execution_status.block_hash()); + ( + parent_payload_hash, + head_block.execution_status.block_hash(), + ) + } else { + ( + head_block.execution_payload_parent_hash, + head_block.execution_payload_block_hash, + ) + }; + + let builds_on_parent_block = Some(bid.parent_block_root) == head_block.parent_root; + let builds_on_parent_payload = Some(bid.parent_block_hash) == head_bid_parent_block_hash; + + if builds_on_parent_block && builds_on_parent_payload { + return Ok(true); + } + + if bid.parent_block_root != head_block.root { + return Ok(false); + } + + let builds_on_head_payload = Some(bid.parent_block_hash) == head_bid_block_hash; + + if head_is_pre_gloas { + return Ok(builds_on_head_payload); + } + + if fork_choice_read + .should_build_on_full( + &head_block_root, + cached_head.head_payload_status(), + bid.slot, + ) + .map_err(|e| { + PayloadBidError::InternalError(format!("should_build_on_full failed: {e:?}")) + })? + { + return Ok(builds_on_head_payload); + } + + Ok(builds_on_parent_payload) +} + pub struct GossipVerificationContext<'a, T: BeaconChainTypes> { pub canonical_head: &'a CanonicalHead, pub gossip_verified_payload_bid_cache: &'a GossipVerifiedPayloadBidCache, pub gossip_verified_proposer_preferences_cache: &'a GossipVerifiedProposerPreferenceCache, pub slot_clock: &'a T::SlotClock, pub spec: &'a ChainSpec, + pub store: &'a BeaconStore, } /// A wrapper around a `SignedExecutionPayloadBid` that indicates it has been approved for re-gossiping on @@ -107,13 +183,13 @@ impl GossipVerifiedPayloadBid { T: BeaconChainTypes, { let bid_slot = signed_bid.message.slot; - let bid_parent_block_hash = signed_bid.message.parent_block_hash; + let bid_parent = BidParent::from_bid(&signed_bid.message); let bid_parent_block_root = signed_bid.message.parent_block_root; let bid_value = signed_bid.message.value; if ctx .gossip_verified_payload_bid_cache - .seen_builder_index(&bid_slot, signed_bid.message.builder_index) + .seen_builder_bid_for_parent(&bid_slot, bid_parent, signed_bid.message.builder_index) { return Err(PayloadBidError::BuilderAlreadySeen { builder_index: signed_bid.message.builder_index, @@ -123,11 +199,10 @@ impl GossipVerifiedPayloadBid { // TODO(gloas): Extract into `bid_value_over_threshold` on the bid cache and potentially // make this more sophisticate than just a <= check. - if let Some(cached_bid) = ctx.gossip_verified_payload_bid_cache.get_highest_bid( - bid_slot, - bid_parent_block_hash, - bid_parent_block_root, - ) && bid_value <= cached_bid.message.value + if let Some(cached_bid) = ctx + .gossip_verified_payload_bid_cache + .get_highest_bid(bid_slot, bid_parent) + && bid_value <= cached_bid.message.value { return Err(PayloadBidError::BidValueBelowCached { cached_value: cached_bid.message.value, @@ -140,7 +215,43 @@ impl GossipVerifiedPayloadBid { .slot_clock .now() .ok_or(PayloadBidError::UnableToReadSlot)?; - let head_state = &cached_head.snapshot.beacon_state; + let snapshot_state = &cached_head.snapshot.beacon_state; + + // At the Gloas fork boundary the head snapshot is still a pre-Gloas state, so we must + // use the advanced state instead. + // TODO(post-gloas) this can be removed after the gloas fork + let advanced_state; + let head_state = if ctx + .spec + .fork_name_at_slot::(bid_slot) + .gloas_enabled() + && !snapshot_state.fork_name_unchecked().gloas_enabled() + { + let (_, state) = ctx + .store + .get_advanced_hot_state( + cached_head.head_block_root(), + bid_slot, + cached_head.head_state_root(), + ) + .map_err(|e| { + PayloadBidError::InternalError(format!( + "failed to load advanced head state: {e:?}" + )) + })? + .ok_or_else(|| { + PayloadBidError::InternalError("advanced head state unavailable".to_string()) + })?; + if !state.fork_name_unchecked().gloas_enabled() { + return Err(PayloadBidError::InternalError( + "head state not yet advanced to Gloas".to_string(), + )); + } + advanced_state = state; + &advanced_state + } else { + snapshot_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. @@ -183,10 +294,10 @@ impl GossipVerifiedPayloadBid { return Err(PayloadBidError::InvalidPrevRandao { slot: bid_slot }); } - // TODO(gloas) reprocess bids whose parent_block_root becomes canonical after a reorg. - let head_root = cached_head.head_block_root(); - if !fork_choice.is_descendant(bid_parent_block_root, head_root) { - return Err(PayloadBidError::ParentBlockRootNotCanonical { + // TODO(gloas) should we reprocess a dropped bid when the head changes to its parent? + if !is_bid_compatible_with_head(&cached_head, &fork_choice, &signed_bid.message, ctx.spec)? + { + return Err(PayloadBidError::BidNotCompatibleWithHead { parent_block_root: bid_parent_block_root, }); } @@ -235,7 +346,7 @@ impl GossipVerifiedPayloadBid { let gossip_verified_bid = GossipVerifiedPayloadBid { signed_bid }; ctx.gossip_verified_payload_bid_cache - .insert_seen_builder(&gossip_verified_bid); + .insert_seen_builder_bid(&gossip_verified_bid); ctx.gossip_verified_payload_bid_cache .insert_highest_bid(gossip_verified_bid.clone()); @@ -254,6 +365,7 @@ impl BeaconChain { .gossip_verified_proposer_preferences_cache, slot_clock: &self.slot_clock, spec: &self.spec, + store: &self.store, } } diff --git a/beacon_node/beacon_chain/src/payload_bid_verification/mod.rs b/beacon_node/beacon_chain/src/payload_bid_verification/mod.rs index a5453d0c5bb..afa9805fad1 100644 --- a/beacon_node/beacon_chain/src/payload_bid_verification/mod.rs +++ b/beacon_node/beacon_chain/src/payload_bid_verification/mod.rs @@ -22,8 +22,8 @@ mod tests; pub enum PayloadBidError { /// The bid's parent block root is unknown. ParentBlockRootUnknown { parent_block_root: Hash256 }, - /// The bid's parent block root is known but not on the canonical chain. - ParentBlockRootNotCanonical { parent_block_root: Hash256 }, + /// The bid does not build on the head block or on the head block's parent. + BidNotCompatibleWithHead { parent_block_root: Hash256 }, /// The signature is invalid. BadSignature, /// A bid for this builder at this slot has already been seen. diff --git a/beacon_node/beacon_chain/src/payload_bid_verification/payload_bid_cache.rs b/beacon_node/beacon_chain/src/payload_bid_verification/payload_bid_cache.rs index 22e21bd57e9..060a5a965ee 100644 --- a/beacon_node/beacon_chain/src/payload_bid_verification/payload_bid_cache.rs +++ b/beacon_node/beacon_chain/src/payload_bid_verification/payload_bid_cache.rs @@ -4,46 +4,60 @@ use std::{ collections::{BTreeMap, HashMap, HashSet}, sync::Arc, }; -use types::{BuilderIndex, EthSpec, ExecutionBlockHash, Hash256, SignedExecutionPayloadBid, Slot}; +use types::{ + BuilderIndex, EthSpec, ExecutionBlockHash, ExecutionPayloadBid, Hash256, + SignedExecutionPayloadBid, Slot, +}; + +/// The parent a bid builds on: which beacon block, and which payload state of it. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub struct BidParent { + pub parent_block_hash: ExecutionBlockHash, + pub parent_block_root: Hash256, +} + +impl BidParent { + pub fn from_bid(bid: &ExecutionPayloadBid) -> Self { + Self { + parent_block_hash: bid.parent_block_hash, + parent_block_root: bid.parent_block_root, + } + } +} -type HighestBidMap = - BTreeMap>>; +type HighestBidMap = BTreeMap>>; pub struct GossipVerifiedPayloadBidCache { highest_bid: RwLock>, - seen_builder: RwLock>>, + seen_builder_bids: RwLock>>, } impl Default for GossipVerifiedPayloadBidCache { fn default() -> Self { Self { highest_bid: RwLock::new(BTreeMap::new()), - seen_builder: RwLock::new(BTreeMap::new()), + seen_builder_bids: RwLock::new(BTreeMap::new()), } } } impl GossipVerifiedPayloadBidCache { - /// Get the cached bid for the tuple `(slot, parent_block_hash, parent_block_root)`. + /// Get the cached bid for `(slot, bid_parent)`. pub fn get_highest_bid( &self, slot: Slot, - parent_block_hash: ExecutionBlockHash, - parent_block_root: Hash256, + bid_parent: BidParent, ) -> Option>> { - self.highest_bid.read().get(&slot).and_then(|map| { - map.get(&(parent_block_hash, parent_block_root)) - .map(|b| b.signed_bid.clone()) - }) + self.highest_bid + .read() + .get(&slot) + .and_then(|map| map.get(&bid_parent).map(|b| b.signed_bid.clone())) } - /// Insert a bid for the tuple `(slot, parent_block_hash, parent_block_root)` only if - /// its value is higher than the currently cached bid for that tuple. + /// Insert a bid for `(slot, bid_parent)` only if its value is higher than the + /// currently cached bid for that key. pub fn insert_highest_bid(&self, bid: GossipVerifiedPayloadBid) { - let key = ( - bid.signed_bid.message.parent_block_hash, - bid.signed_bid.message.parent_block_root, - ); + let key = BidParent::from_bid(&bid.signed_bid.message); let mut highest_bid = self.highest_bid.write(); let slot_map = highest_bid.entry(bid.signed_bid.message.slot).or_default(); @@ -55,21 +69,29 @@ impl GossipVerifiedPayloadBidCache { slot_map.insert(key, bid); } - /// A gossip verified bid for `BuilderIndex` already exists at `slot` - pub fn seen_builder_index(&self, slot: &Slot, builder_index: BuilderIndex) -> bool { - self.seen_builder + /// A gossip verified bid for `BuilderIndex` already exists for `(slot, bid_parent)`. + pub fn seen_builder_bid_for_parent( + &self, + slot: &Slot, + bid_parent: BidParent, + builder_index: BuilderIndex, + ) -> bool { + self.seen_builder_bids .read() .get(slot) - .is_some_and(|seen_builders| seen_builders.contains(&builder_index)) + .is_some_and(|seen_builders| seen_builders.contains(&(bid_parent, builder_index))) } /// Insert a builder into the seen cache. - pub fn insert_seen_builder(&self, bid: &GossipVerifiedPayloadBid) { - let mut seen_builder = self.seen_builder.write(); - seen_builder + pub fn insert_seen_builder_bid(&self, bid: &GossipVerifiedPayloadBid) { + let mut seen_builder_bids = self.seen_builder_bids.write(); + seen_builder_bids .entry(bid.signed_bid.message.slot) .or_default() - .insert(bid.signed_bid.message.builder_index); + .insert(( + BidParent::from_bid(&bid.signed_bid.message), + bid.signed_bid.message.builder_index, + )); } /// Prune anything before `current_slot` @@ -78,7 +100,7 @@ impl GossipVerifiedPayloadBidCache { .write() .retain(|&slot, _| slot >= current_slot); - self.seen_builder + self.seen_builder_bids .write() .retain(|&slot, _| slot >= current_slot); } @@ -94,7 +116,7 @@ mod tests { SignedExecutionPayloadBid, Slot, }; - use super::GossipVerifiedPayloadBidCache; + use super::{BidParent, GossipVerifiedPayloadBidCache}; use crate::payload_bid_verification::gossip_verified_bid::GossipVerifiedPayloadBid; type E = MinimalEthSpec; @@ -121,15 +143,97 @@ mod tests { } } + #[test] + fn seen_builder_for_parent() { + let cache = GossipVerifiedPayloadBidCache::::default(); + let slot = Slot::new(1); + let parent_a = BidParent { + parent_block_hash: ExecutionBlockHash::zero(), + parent_block_root: Hash256::ZERO, + }; + let parent_b = BidParent { + parent_block_hash: ExecutionBlockHash::from_root(Hash256::repeat_byte(0x01)), + parent_block_root: Hash256::repeat_byte(0x02), + }; + + let verified = make_gossip_verified( + slot, + 0, + parent_a.parent_block_hash, + parent_a.parent_block_root, + 100, + ); + cache.insert_seen_builder_bid(&verified); + + // Seen only for the exact (slot, parent tuple, builder) combination. + assert!(cache.seen_builder_bid_for_parent(&slot, parent_a, 0)); + assert!(!cache.seen_builder_bid_for_parent(&slot, parent_b, 0)); + assert!(!cache.seen_builder_bid_for_parent(&slot, parent_a, 1)); + assert!(!cache.seen_builder_bid_for_parent(&Slot::new(2), parent_a, 0)); + } + + #[test] + fn highest_bid_for_parent() { + let cache = GossipVerifiedPayloadBidCache::::default(); + let slot = Slot::new(1); + let hash_a = ExecutionBlockHash::zero(); + let root_a = Hash256::ZERO; + let hash_b = ExecutionBlockHash::from_root(Hash256::repeat_byte(0x01)); + let root_b = Hash256::repeat_byte(0x02); + let parent_a = BidParent { + parent_block_hash: hash_a, + parent_block_root: root_a, + }; + let parent_b = BidParent { + parent_block_hash: hash_b, + parent_block_root: root_b, + }; + + cache.insert_highest_bid(make_gossip_verified(slot, 0, hash_a, root_a, 100)); + cache.insert_highest_bid(make_gossip_verified(slot, 1, hash_b, root_b, 50)); + + // Each parent tuple keeps its own highest bid. + assert_eq!( + cache.get_highest_bid(slot, parent_a).unwrap().message.value, + 100 + ); + assert_eq!( + cache.get_highest_bid(slot, parent_b).unwrap().message.value, + 50 + ); + + // A lower bid does not replace the cached bid for its tuple, and does + // not touch the other tuple. + cache.insert_highest_bid(make_gossip_verified(slot, 2, hash_a, root_a, 60)); + assert_eq!( + cache.get_highest_bid(slot, parent_a).unwrap().message.value, + 100 + ); + assert_eq!( + cache.get_highest_bid(slot, parent_b).unwrap().message.value, + 50 + ); + + // A higher bid replaces the cached bid for its tuple. + cache.insert_highest_bid(make_gossip_verified(slot, 3, hash_b, root_b, 70)); + let highest_b = cache.get_highest_bid(slot, parent_b).unwrap(); + assert_eq!(highest_b.message.value, 70); + assert_eq!(highest_b.message.builder_index, 3); + } + #[test] fn prune_removes_old_retains_current() { let cache = GossipVerifiedPayloadBidCache::::default(); let hash = ExecutionBlockHash::zero(); let root = Hash256::ZERO; + let bid_parent = BidParent { + parent_block_hash: hash, + parent_block_root: root, + }; for slot in [1, 2, 3, 7, 8, 9, 10] { let verified = make_gossip_verified(Slot::new(slot), slot, hash, root, slot * 100); - cache.insert_seen_builder(&verified); + cache.insert_seen_builder_bid(&verified); cache.insert_highest_bid(verified); } @@ -137,13 +241,13 @@ mod tests { // Slots 1-7 pruned from both maps. for slot in [1, 2, 3, 7] { - assert!(cache.get_highest_bid(Slot::new(slot), hash, root).is_none()); - assert!(!cache.seen_builder_index(&Slot::new(slot), slot)); + assert!(cache.get_highest_bid(Slot::new(slot), bid_parent).is_none()); + assert!(!cache.seen_builder_bid_for_parent(&Slot::new(slot), bid_parent, slot)); } // Slots 8-10 retained in both maps. for slot in [8, 9, 10] { - assert!(cache.get_highest_bid(Slot::new(slot), hash, root).is_some()); - assert!(cache.seen_builder_index(&Slot::new(slot), slot)); + assert!(cache.get_highest_bid(Slot::new(slot), bid_parent).is_some()); + assert!(cache.seen_builder_bid_for_parent(&Slot::new(slot), bid_parent, slot)); } } } diff --git a/beacon_node/beacon_chain/src/payload_bid_verification/tests.rs b/beacon_node/beacon_chain/src/payload_bid_verification/tests.rs index 62da420f9d4..d76138bf76c 100644 --- a/beacon_node/beacon_chain/src/payload_bid_verification/tests.rs +++ b/beacon_node/beacon_chain/src/payload_bid_verification/tests.rs @@ -18,7 +18,7 @@ use types::{ consts::gloas::PAYLOAD_BUILDER_VERSION, }; -use proto_array::{Block as ProtoBlock, ExecutionStatus, PayloadStatus}; +use proto_array::{Block as ProtoBlock, ExecutionStatus}; use types::AttestationShufflingId; use crate::{ @@ -29,7 +29,7 @@ use crate::{ payload_bid_verification::{ PayloadBidError, gossip_verified_bid::{GossipVerificationContext, GossipVerifiedPayloadBid}, - payload_bid_cache::GossipVerifiedPayloadBidCache, + payload_bid_cache::{BidParent, GossipVerifiedPayloadBidCache}, }, proposer_preferences_verification::{ gossip_verified_proposer_preferences::GossipVerifiedProposerPreferences, @@ -57,6 +57,7 @@ struct TestContext { spec: ChainSpec, genesis_block_root: Hash256, inactive_builder_index: u64, + store: crate::BeaconStore, } fn builder_withdrawal_credentials(pubkey: &bls::PublicKey, spec: &ChainSpec) -> Hash256 { @@ -143,14 +144,18 @@ impl TestContext { let fc_store = BeaconForkChoiceStore::get_forkchoice_store(store.clone(), snapshot.clone()) .expect("should create fork choice store"); - let fork_choice = + let mut fork_choice = ForkChoice::from_anchor(fc_store, block_root, &signed_block, &state, None, &spec) .expect("should create fork choice"); + let (_, head_payload_status) = fork_choice + .get_head(Slot::new(0), &spec) + .expect("should run get_head"); + let canonical_head = CanonicalHead::new( fork_choice, Arc::new(snapshot), - PayloadStatus::Pending, + head_payload_status, FastConfirmationMode::Disabled, &store, &spec, @@ -172,6 +177,7 @@ impl TestContext { spec, genesis_block_root: block_root, inactive_builder_index, + store, } } @@ -199,6 +205,7 @@ impl TestContext { gossip_verified_proposer_preferences_cache: &self.preferences_cache, slot_clock: &self.slot_clock, spec: &self.spec, + store: &self.store, } } @@ -358,7 +365,7 @@ fn builder_already_seen_for_slot() { let verified = GossipVerifiedPayloadBid { signed_bid: bid.clone(), }; - ctx.bid_cache.insert_seen_builder(&verified); + ctx.bid_cache.insert_seen_builder_bid(&verified); let result = GossipVerifiedPayloadBid::new(bid, &gossip); assert!(matches!( @@ -370,6 +377,46 @@ fn builder_already_seen_for_slot() { )); } +#[test] +fn same_builder_new_parent_tuple_not_blocked() { + if !fork_name_from_env().is_some_and(|f| f.gloas_enabled()) { + return; + } + let ctx = TestContext::new(); + let gossip = ctx.gossip_ctx(); + let slot = Slot::new(1); + seed_preferences(&ctx, slot, Address::ZERO, 30_000_000); + + let bid = ctx.sign_bid(ExecutionPayloadBid { + slot, + builder_index: 0, + fee_recipient: Address::ZERO, + gas_limit: 30_000_000, + value: 0, + parent_block_root: ctx.genesis_block_root, + prev_randao: ctx.expected_prev_randao(), + ..ExecutionPayloadBid::default() + }); + let result = GossipVerifiedPayloadBid::new(bid, &gossip); + assert!( + result.is_ok(), + "first bid should pass: {:?}", + result.unwrap_err() + ); + + // The same builder bids again at the same slot with a different parent tuple. + // The seen-builder rule is per (slot, parent_block_hash, parent_block_root), + // so this bid gets past that check and fails later on the unknown parent root. + let unknown_root = Hash256::repeat_byte(0xff); + let bid_2 = ctx.make_signed_bid(slot, 0, Address::ZERO, 30_000_000, 0, unknown_root); + let result_2 = GossipVerifiedPayloadBid::new(bid_2, &gossip); + let err = result_2.expect_err("second bid should fail on the unknown parent root"); + assert!( + matches!(err, PayloadBidError::ParentBlockRootUnknown { .. }), + "expected ParentBlockRootUnknown (not BuilderAlreadySeen), got: {err:?}" + ); +} + #[test] fn bid_value_below_cached() { if !fork_name_from_env().is_some_and(|f| f.gloas_enabled()) { @@ -609,8 +656,8 @@ fn parent_block_root_not_canonical() { assert!(result.is_err(), "expected error, got Ok"); let err = result.unwrap_err(); assert!( - matches!(err, PayloadBidError::ParentBlockRootNotCanonical { .. }), - "expected ParentBlockRootNotCanonical, got: {err:?}" + matches!(err, PayloadBidError::BidNotCompatibleWithHead { .. }), + "expected BidNotCompatibleWithHead, got: {err:?}" ); } @@ -703,12 +750,15 @@ fn bad_signature() { ); let result = GossipVerifiedPayloadBid::new(bid, &gossip); assert!(matches!(result, Err(PayloadBidError::BadSignature))); - assert!(!ctx.bid_cache.seen_builder_index(&slot, 0)); + let bid_parent = BidParent { + parent_block_hash: ExecutionBlockHash::zero(), + parent_block_root: ctx.genesis_block_root, + }; assert!( - ctx.bid_cache - .get_highest_bid(slot, ExecutionBlockHash::zero(), ctx.genesis_block_root) - .is_none() + !ctx.bid_cache + .seen_builder_bid_for_parent(&slot, bid_parent, 0) ); + assert!(ctx.bid_cache.get_highest_bid(slot, bid_parent).is_none()); } #[test] @@ -785,12 +835,22 @@ fn two_builders_coexist_in_cache() { ); // Both builders should be seen. - assert!(ctx.bid_cache.seen_builder_index(&slot, 0)); - assert!(ctx.bid_cache.seen_builder_index(&slot, 1)); + let bid_parent = BidParent { + parent_block_hash: ExecutionBlockHash::zero(), + parent_block_root: ctx.genesis_block_root, + }; + assert!( + ctx.bid_cache + .seen_builder_bid_for_parent(&slot, bid_parent, 0) + ); + assert!( + ctx.bid_cache + .seen_builder_bid_for_parent(&slot, bid_parent, 1) + ); let highest = ctx .bid_cache - .get_highest_bid(slot, ExecutionBlockHash::zero(), ctx.genesis_block_root) + .get_highest_bid(slot, bid_parent) .expect("should have highest bid"); assert_eq!(highest.message.value, 1); assert_eq!(highest.message.builder_index, 1); diff --git a/beacon_node/beacon_chain/src/validator_monitor.rs b/beacon_node/beacon_chain/src/validator_monitor.rs index 294f160b821..8cca7a416ad 100644 --- a/beacon_node/beacon_chain/src/validator_monitor.rs +++ b/beacon_node/beacon_chain/src/validator_monitor.rs @@ -728,10 +728,16 @@ impl ValidatorMonitor { let data = unaggregated_attestation.data(); + let parent_slot = state + .latest_execution_payload_bid() + .ok() + .map(|bid| bid.slot); + // Get the reward indices for the unaggregated attestation or log an error match get_attestation_participation_flag_indices( state, unaggregated_attestation.data(), + parent_slot, inclusion_delay, spec, ) { diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs index 3480cf80653..1b5f7fd7650 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -4233,7 +4233,7 @@ impl NetworkBeaconProcessor { | PayloadBidError::BuilderAlreadySeen { .. } | PayloadBidError::BidValueBelowCached { .. } | PayloadBidError::ParentBlockRootUnknown { .. } - | PayloadBidError::ParentBlockRootNotCanonical { .. } + | PayloadBidError::BidNotCompatibleWithHead { .. } | PayloadBidError::BuilderCantCoverBid { .. } | PayloadBidError::InvalidFeeRecipient | PayloadBidError::InvalidGasLimit diff --git a/beacon_node/operation_pool/src/attestation.rs b/beacon_node/operation_pool/src/attestation.rs index 7b6985b4c33..60bd2f2df9d 100644 --- a/beacon_node/operation_pool/src/attestation.rs +++ b/beacon_node/operation_pool/src/attestation.rs @@ -81,9 +81,19 @@ impl<'a, E: EthSpec> AttMaxCover<'a, E> { let att_data = att.attestation_data(); let inclusion_delay = state.slot().as_u64().checked_sub(att_data.slot.as_u64())?; - let att_participation_flags = - get_attestation_participation_flag_indices(state, &att_data, inclusion_delay, spec) - .ok()?; + let parent_slot = state + .latest_execution_payload_bid() + .ok() + .map(|bid| bid.slot); + + let att_participation_flags = get_attestation_participation_flag_indices( + state, + &att_data, + parent_slot, + inclusion_delay, + spec, + ) + .ok()?; let fresh_validators_rewards = att .indexed diff --git a/consensus/state_processing/src/common/get_attestation_participation.rs b/consensus/state_processing/src/common/get_attestation_participation.rs index 8f1f000f401..ac27579d058 100644 --- a/consensus/state_processing/src/common/get_attestation_participation.rs +++ b/consensus/state_processing/src/common/get_attestation_participation.rs @@ -2,7 +2,7 @@ use integer_sqrt::IntegerSquareRoot; use safe_arith::SafeArith; use smallvec::SmallVec; use types::{ - AttestationData, BeaconState, BeaconStateError as Error, ChainSpec, EthSpec, + AttestationData, BeaconState, BeaconStateError as Error, ChainSpec, EthSpec, Slot, consts::altair::{ NUM_FLAG_INDICES, TIMELY_HEAD_FLAG_INDEX, TIMELY_SOURCE_FLAG_INDEX, TIMELY_TARGET_FLAG_INDEX, @@ -21,6 +21,7 @@ use types::{ pub fn get_attestation_participation_flag_indices( state: &BeaconState, data: &AttestationData, + parent_slot: Option, inclusion_delay: u64, spec: &ChainSpec, ) -> Result, Error> { @@ -37,6 +38,8 @@ pub fn get_attestation_participation_flag_indices( // [New in Gloas:EIP7732] let payload_matches = if state.fork_name_unchecked().gloas_enabled() { + let parent_slot = parent_slot.ok_or(Error::MissingParentSlot)?; + if state.is_attestation_same_slot(data)? { // For same-slot attestations, data.index must be 0 if data.index != 0 { @@ -45,8 +48,7 @@ pub fn get_attestation_participation_flag_indices( true } else { // For non same-slot attestations, check execution payload availability - let slot_index = data - .slot + let slot_index = parent_slot .as_usize() .safe_rem(E::slots_per_historical_root())?; let payload_index = state diff --git a/consensus/state_processing/src/per_block_processing.rs b/consensus/state_processing/src/per_block_processing.rs index 22ec9a9ed47..2895ab756a9 100644 --- a/consensus/state_processing/src/per_block_processing.rs +++ b/consensus/state_processing/src/per_block_processing.rs @@ -185,6 +185,7 @@ pub fn per_block_processing>( state.build_committee_cache(RelativeEpoch::Previous, spec)?; state.build_committee_cache(RelativeEpoch::Current, spec)?; + let mut parent_slot = None; // The call to the `process_execution_payload` must happen before the call to the // `process_randao` as the former depends on the `randao_mix` computed with the reveal of the // previous block. @@ -193,6 +194,7 @@ pub fn per_block_processing>( if state.fork_name_unchecked().gloas_enabled() { withdrawals::gloas::process_withdrawals::(state, spec)?; let signed_bid = block.body().signed_execution_payload_bid()?; + parent_slot = Some(state.latest_execution_payload_bid()?.slot); process_execution_payload_bid(state, signed_bid, verify_signatures, spec)?; } else { if state.fork_name_unchecked().capella_enabled() { @@ -208,7 +210,14 @@ pub fn per_block_processing>( process_randao(state, block, verify_randao, ctxt, spec)?; process_eth1_data(state, block.body().eth1_data())?; - process_operations(state, block.body(), verify_signatures, ctxt, spec)?; + process_operations( + state, + block.body(), + verify_signatures, + parent_slot, + ctxt, + spec, + )?; if let Ok(sync_aggregate) = block.body().sync_aggregate() { process_sync_aggregate( diff --git a/consensus/state_processing/src/per_block_processing/process_operations.rs b/consensus/state_processing/src/per_block_processing/process_operations.rs index b2d2d49a83b..1b8af3d09e7 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -17,6 +17,7 @@ pub fn process_operations>( state: &mut BeaconState, block_body: BeaconBlockBodyRef, verify_signatures: VerifySignatures, + parent_slot: Option, ctxt: &mut ConsensusContext, spec: &ChainSpec, ) -> Result<(), BlockProcessingError> { @@ -40,7 +41,14 @@ pub fn process_operations>( ctxt, spec, )?; - process_attestations(state, block_body, verify_signatures, ctxt, spec)?; + process_attestations( + state, + block_body, + verify_signatures, + parent_slot, + ctxt, + spec, + )?; process_deposits(state, &block_body.deposits().to_cow_slice(), spec)?; process_exits( state, @@ -247,7 +255,7 @@ pub mod altair_deneb { let data = attestation.data(); let inclusion_delay = state.slot().safe_sub(data.slot)?.as_u64(); let participation_flag_indices = - get_attestation_participation_flag_indices(state, data, inclusion_delay, spec)?; + get_attestation_participation_flag_indices(state, data, None, inclusion_delay, spec)?; // Update epoch participation flags. let mut proposer_reward_numerator = 0; @@ -304,6 +312,7 @@ pub mod gloas { state: &mut BeaconState, attestations: I, verify_signatures: VerifySignatures, + parent_slot: Option, ctxt: &mut ConsensusContext, spec: &ChainSpec, ) -> Result<(), BlockProcessingError> @@ -311,7 +320,15 @@ pub mod gloas { I: Iterator>, { attestations.enumerate().try_for_each(|(i, attestation)| { - process_attestation(state, attestation, i, ctxt, verify_signatures, spec) + process_attestation( + state, + attestation, + i, + verify_signatures, + parent_slot, + ctxt, + spec, + ) }) } @@ -319,8 +336,9 @@ pub mod gloas { state: &mut BeaconState, attestation: AttestationRef, att_index: usize, - ctxt: &mut ConsensusContext, verify_signatures: VerifySignatures, + parent_slot: Option, + ctxt: &mut ConsensusContext, spec: &ChainSpec, ) -> Result<(), BlockProcessingError> { let proposer_index = ctxt.get_proposer_index(state, spec)?; @@ -339,8 +357,13 @@ pub mod gloas { // Matching roots, participation flag indices let data = attestation.data(); let inclusion_delay = state.slot().safe_sub(data.slot)?.as_u64(); - let participation_flag_indices = - get_attestation_participation_flag_indices(state, data, inclusion_delay, spec)?; + let participation_flag_indices = get_attestation_participation_flag_indices( + state, + data, + parent_slot, + inclusion_delay, + spec, + )?; // [New in EIP-7732] let current_epoch_target = data.target.epoch == state.current_epoch(); @@ -540,6 +563,7 @@ pub fn process_attestations>( state: &mut BeaconState, block_body: BeaconBlockBodyRef, verify_signatures: VerifySignatures, + parent_slot: Option, ctxt: &mut ConsensusContext, spec: &ChainSpec, ) -> Result<(), BlockProcessingError> { @@ -548,6 +572,7 @@ pub fn process_attestations>( state, block_body.attestations(), verify_signatures, + parent_slot, ctxt, spec, )?; diff --git a/consensus/state_processing/src/per_block_processing/tests.rs b/consensus/state_processing/src/per_block_processing/tests.rs index 63f33548fb2..4b21c108963 100644 --- a/consensus/state_processing/src/per_block_processing/tests.rs +++ b/consensus/state_processing/src/per_block_processing/tests.rs @@ -441,10 +441,15 @@ async fn invalid_attestation_no_committee_for_index() { .into_data_mut() .index += 1; let mut ctxt = ConsensusContext::new(state.slot()); + let parent_slot = state + .latest_execution_payload_bid() + .ok() + .map(|bid| bid.slot); let result = process_operations::process_attestations( &mut state, head_block.body(), VerifySignatures::True, + parent_slot, &mut ctxt, &spec, ); @@ -491,10 +496,15 @@ async fn invalid_attestation_wrong_justified_checkpoint() { .source = new_justified_checkpoint; let mut ctxt = ConsensusContext::new(state.slot()); + let parent_slot = state + .latest_execution_payload_bid() + .ok() + .map(|bid| bid.slot); let result = process_operations::process_attestations( &mut state, head_block.body(), VerifySignatures::True, + parent_slot, &mut ctxt, &spec, ); @@ -545,10 +555,15 @@ async fn invalid_attestation_bad_aggregation_bitfield_len() { } let mut ctxt = ConsensusContext::new(state.slot()); + let parent_slot = state + .latest_execution_payload_bid() + .ok() + .map(|bid| bid.slot); let result = process_operations::process_attestations( &mut state, head_block.body(), VerifySignatures::True, + parent_slot, &mut ctxt, &spec, ); @@ -585,10 +600,15 @@ async fn invalid_attestation_bad_signature() { .into_signature_mut() = AggregateSignature::empty(); let mut ctxt = ConsensusContext::new(state.slot()); + let parent_slot = state + .latest_execution_payload_bid() + .ok() + .map(|bid| bid.slot); let result = process_operations::process_attestations( &mut state, head_block.body(), VerifySignatures::True, + parent_slot, &mut ctxt, &spec, ); @@ -629,10 +649,15 @@ async fn invalid_attestation_included_too_early() { .slot = new_attesation_slot; let mut ctxt = ConsensusContext::new(state.slot()); + let parent_slot = state + .latest_execution_payload_bid() + .ok() + .map(|bid| bid.slot); let result = process_operations::process_attestations( &mut state, head_block.body(), VerifySignatures::True, + parent_slot, &mut ctxt, &spec, ); @@ -680,10 +705,15 @@ async fn invalid_attestation_target_epoch_slot_mismatch() { .epoch += Epoch::new(1); let mut ctxt = ConsensusContext::new(state.slot()); + let parent_slot = state + .latest_execution_payload_bid() + .ok() + .map(|bid| bid.slot); let result = process_operations::process_attestations( &mut state, head_block.body(), VerifySignatures::True, + parent_slot, &mut ctxt, &spec, ); diff --git a/consensus/state_processing/src/upgrade/altair.rs b/consensus/state_processing/src/upgrade/altair.rs index 66bdb1af250..887fc100022 100644 --- a/consensus/state_processing/src/upgrade/altair.rs +++ b/consensus/state_processing/src/upgrade/altair.rs @@ -25,7 +25,7 @@ pub fn translate_participation( // Translate attestation inclusion info to flag indices. let participation_flag_indices = - get_attestation_participation_flag_indices(state, data, inclusion_delay, spec)?; + get_attestation_participation_flag_indices(state, data, None, inclusion_delay, spec)?; // Apply flags to all attesting validators. let committee = state.get_beacon_committee(data.slot, data.index)?; diff --git a/consensus/types/src/state/beacon_state.rs b/consensus/types/src/state/beacon_state.rs index b70b1c5b21e..84e3b303a16 100644 --- a/consensus/types/src/state/beacon_state.rs +++ b/consensus/types/src/state/beacon_state.rs @@ -237,6 +237,7 @@ pub enum BeaconStateError { InvalidIndicesCount, InvalidBuilderPendingPaymentsIndex(usize), InvalidExecutionPayloadAvailabilityIndex(usize), + MissingParentSlot, /// Merkle proofs against the `BeaconState` and `BeaconBlockBody` use progressive-container /// generalized indices from Gloas (EIP-7688) onwards, which are not implemented yet. ProgressiveMerkleProofNotSupported, diff --git a/testing/ef_tests/src/cases/operations.rs b/testing/ef_tests/src/cases/operations.rs index 350d52dcf2a..c72e4a2256d 100644 --- a/testing/ef_tests/src/cases/operations.rs +++ b/testing/ef_tests/src/cases/operations.rs @@ -131,12 +131,14 @@ impl Operation for Attestation { initialize_progressive_balances_cache(state, spec)?; let mut ctxt = ConsensusContext::new(state.slot()); if state.fork_name_unchecked().gloas_enabled() { + let parent_slot = Some(state.latest_execution_payload_bid()?.slot); gloas::process_attestation( state, self.to_ref(), 0, - &mut ctxt, VerifySignatures::True, + parent_slot, + &mut ctxt, spec, ) } else if state.fork_name_unchecked().altair_enabled() {