diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index ccb12a353da..f618cf63217 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4175,7 +4175,14 @@ impl BeaconChain { }; // Read the cached head prior to taking the fork choice lock to avoid potential deadlocks. - let old_head_slot = self.canonical_head.cached_head().head_slot(); + let cached_head = self.canonical_head.cached_head(); + let old_head_slot = cached_head.head_slot(); + + // Compute the expected proposer for `current_slot` on the canonical chain. This is used by + // `on_block` to gate proposer boost on the block's proposer matching the canonical proposer + // (per spec `update_proposer_boost_root` added in v1.7.0-alpha.5). + let canonical_head_proposer_index = + self.canonical_head_proposer_index(current_slot, &cached_head)?; // Take an upgradable read lock on fork choice so we can check if this block has already // been imported. We don't want to repeat work importing a block that is already imported. @@ -4208,6 +4215,7 @@ impl BeaconChain { block_delay, &state, payload_verification_status, + canonical_head_proposer_index, &self.spec, ) .map_err(|e| BlockError::BeaconChainError(Box::new(e.into())))?; @@ -4950,6 +4958,42 @@ impl BeaconChain { })) } + /// Compute the expected beacon proposer for `slot` on the canonical chain extending `cached_head`. + /// + /// Uses the beacon proposer cache to avoid recomputing the shuffling on every block import. + /// + /// This is used by `update_proposer_boost_root` to gate proposer boost on the block's proposer + /// matching the canonical proposer, per consensus-specs v1.7.0-alpha.5. + /// + /// This function should never error unless there is some corruption of the head state. If a + /// state advance is needed, it will be handled by the proposer cache. + pub fn canonical_head_proposer_index( + &self, + slot: Slot, + cached_head: &CachedHead, + ) -> Result { + let proposal_epoch = slot.epoch(T::EthSpec::slots_per_epoch()); + let head_block_root = cached_head.head_block_root(); + let head_state = &cached_head.snapshot.beacon_state; + + let shuffling_decision_root = head_state.proposer_shuffling_decision_root_at_epoch( + proposal_epoch, + head_block_root, + &self.spec, + )?; + + self.with_proposer_cache::<_, Error>( + shuffling_decision_root, + proposal_epoch, + |proposers| { + proposers + .get_slot::(slot) + .map(|p| p.index as u64) + }, + || Ok((cached_head.head_state_root(), head_state.clone())), + ) + } + pub fn get_expected_withdrawals( &self, forkchoice_update_params: &ForkchoiceUpdateParameters, diff --git a/beacon_node/beacon_chain/src/block_production/gloas.rs b/beacon_node/beacon_chain/src/block_production/gloas.rs index a02963c3586..6510c20ba7b 100644 --- a/beacon_node/beacon_chain/src/block_production/gloas.rs +++ b/beacon_node/beacon_chain/src/block_production/gloas.rs @@ -78,6 +78,16 @@ pub struct ExecutionPayloadData { pub blobs_and_proofs: (types::BlobsList, types::KzgProofs), } +/// The result of a local payload build, used to decide whether to include a builder bid +/// from the gossip cache or fall back to self-build. +pub struct LocalBuildResult { + pub payload_data: ExecutionPayloadData, + /// EL block value (in wei) of the locally-built payload. + pub payload_value: types::Uint256, + /// `true` if the EL signaled `engine_getPayload`'s `shouldOverrideBuilder` flag. + pub should_override_builder: bool, +} + impl BeaconChain { pub async fn produce_block_with_verification_gloas( self: &Arc, @@ -85,7 +95,7 @@ impl BeaconChain { slot: Slot, graffiti_settings: GraffitiSettings, verification: ProduceBlockVerification, - _builder_boost_factor: Option, + builder_boost_factor: Option, ) -> Result, BlockProductionError> { metrics::inc_counter(&metrics::BLOCK_PRODUCTION_REQUESTS); let _complete_timer = metrics::start_timer(&metrics::BLOCK_PRODUCTION_TIMES); @@ -121,11 +131,11 @@ impl BeaconChain { randao_reveal, graffiti_settings, verification, + builder_boost_factor, ) .await } - // TODO(gloas) need to implement builder boost factor logic #[instrument(level = "debug", skip_all)] #[allow(clippy::too_many_arguments)] pub async fn produce_block_on_state_gloas( @@ -138,6 +148,7 @@ impl BeaconChain { randao_reveal: Signature, graffiti_settings: GraffitiSettings, verification: ProduceBlockVerification, + builder_boost_factor: Option, ) -> Result, BlockProductionError> { // Extract the parent's execution requests from the envelope (if parent was full). let parent_execution_requests = if parent_payload_status == PayloadStatus::Full { @@ -179,10 +190,10 @@ impl BeaconChain { // Part 2/3 (async) // - // Produce the execution payload bid. - // TODO(gloas) this is strictly for building local bids - // We'll need to build out trustless/trusted bid paths. - let (execution_payload_bid, state, payload_data) = self + // Produce a local execution payload bid, then select between it and any cached + // gossip-verified builder bid using `builder_boost_factor`. + // TODO(gloas) build out trustless/trusted bid paths. + let (local_signed_bid, state, local_build) = self .clone() .produce_execution_payload_bid( state, @@ -194,6 +205,9 @@ impl BeaconChain { ) .await?; + let (execution_payload_bid, payload_data) = + self.select_payload_bid(local_signed_bid, local_build, builder_boost_factor); + // Part 3/3 (blocking) // // Complete the block with the execution payload bid. @@ -679,16 +693,13 @@ impl BeaconChain { Ok((block, state, consensus_block_value)) } - // TODO(gloas) introduce `ProposerPreferences` so we can build out trustless - // bid building. Right now this only works for local building. - /// Produce an `ExecutionPayloadBid` for some `slot` upon the given `state`. + /// Produce a self-build `ExecutionPayloadBid` for some `slot` upon the given `state`. /// This function assumes we've already advanced `state`. /// - /// Returns the signed bid, the state, and optionally the payload data needed to construct - /// the `ExecutionPayloadEnvelope` after the beacon block is created. - /// - /// For local building, payload data is always returned (`Some`). - /// For trustless building, the builder provides the envelope separately, so `None` is returned. + /// Returns the signed bid, the state, and a `LocalBuildResult` carrying the payload + /// data needed to construct the `ExecutionPayloadEnvelope` after the beacon block is + /// created, plus the EL block value and `should_override_builder` flag used by the + /// caller to compare against any cached p2p builder bid. #[allow(clippy::type_complexity)] #[instrument(level = "debug", skip_all)] pub async fn produce_execution_payload_bid( @@ -703,7 +714,7 @@ impl BeaconChain { ( SignedExecutionPayloadBid, BeaconState, - Option>, + LocalBuildResult, ), BlockProductionError, > { @@ -775,10 +786,11 @@ impl BeaconChain { let BlockProposalContentsGloas { payload, - payload_value: _, + payload_value, execution_requests, blob_kzg_commitments, blobs_and_proofs, + should_override_builder, } = block_proposal_contents; // TODO(gloas) since we are defaulting to local building, execution payment is 0 @@ -807,19 +819,115 @@ impl BeaconChain { blobs_and_proofs, }; - // TODO(gloas) this is only local building - // we'll need to implement builder signature for the trustless path Ok(( SignedExecutionPayloadBid { message: bid, signature: Signature::infinity().map_err(BlockProductionError::BlsError)?, }, state, - // Local building always returns payload data. - // Trustless building would return None here. - Some(payload_data), + LocalBuildResult { + payload_data, + payload_value, + should_override_builder, + }, )) } + + /// Look up the highest gossip-verified bid for the `(slot, parent_block_hash, + /// parent_block_root)` of the local bid, then choose the winner. + fn select_payload_bid( + &self, + local_signed_bid: SignedExecutionPayloadBid, + local_build: LocalBuildResult, + builder_boost_factor: Option, + ) -> ( + SignedExecutionPayloadBid, + Option>, + ) { + 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, + ); + select_payload_bid_pure( + local_signed_bid, + local_build, + cached_bid, + builder_boost_factor, + ) + } +} + +/// Pure local-vs-cached selection logic, factored out for unit testing. +/// +/// Selection rule (mirrors the pre-Gloas builder/local race in `execution_layer`): +/// - `boosted_bid = (cached_bid.value / 100) * builder_boost_factor` (raw value when `None`) +/// - if `local_value_wei >= boosted_bid_wei` → keep local +/// - if the EL signaled `should_override_builder` → keep local +/// - otherwise → use the cached builder bid and drop local payload data +/// (the builder is responsible for revealing the envelope). +/// +/// `cached_bid.value` is in gwei (`u64`); `payload_value` is in wei (`Uint256`); compared in wei. +pub(crate) fn select_payload_bid_pure( + local_signed_bid: SignedExecutionPayloadBid, + local_build: LocalBuildResult, + cached_bid: Option>>, + builder_boost_factor: Option, +) -> ( + SignedExecutionPayloadBid, + Option>, +) { + let LocalBuildResult { + payload_data, + payload_value, + should_override_builder, + } = local_build; + + let Some(cached_bid) = cached_bid else { + return (local_signed_bid, Some(payload_data)); + }; + + let slot = local_signed_bid.message.slot; + + if should_override_builder { + debug!( + %slot, + cached_bid_value = cached_bid.message.value, + "Using local payload because EL signaled shouldOverrideBuilder" + ); + return (local_signed_bid, Some(payload_data)); + } + + // Convert bid value (gwei) to wei for comparison with `payload_value` (wei). + let bid_value_wei = types::Uint256::from(cached_bid.message.value) + .saturating_mul(types::Uint256::from(1_000_000_000u64)); + let boosted_bid_wei = match builder_boost_factor { + Some(factor) => { + (bid_value_wei / types::Uint256::from(100)).saturating_mul(types::Uint256::from(factor)) + } + None => bid_value_wei, + }; + + if payload_value >= boosted_bid_wei { + debug!( + %slot, + %payload_value, + cached_bid_value_gwei = cached_bid.message.value, + ?builder_boost_factor, + "Local payload is more profitable than cached builder bid" + ); + (local_signed_bid, Some(payload_data)) + } else { + debug!( + %slot, + %payload_value, + cached_bid_value_gwei = cached_bid.message.value, + cached_bid_builder_index = cached_bid.message.builder_index, + ?builder_boost_factor, + "Including cached builder bid" + ); + ((*cached_bid).clone(), None) + } } /// Gets an execution payload for inclusion in a block. @@ -1151,4 +1259,111 @@ mod tests { assert_eq!(exits.len(), 2); } + + // ---- select_payload_bid_pure ---- + + const REMOTE_BUILDER: BuilderIndex = 999; + + fn gwei(n: u64) -> types::Uint256 { + types::Uint256::from(n).saturating_mul(types::Uint256::from(1_000_000_000u64)) + } + + fn local_bid() -> SignedExecutionPayloadBid { + SignedExecutionPayloadBid { + message: ExecutionPayloadBid { + builder_index: BUILDER_INDEX_SELF_BUILD, + ..Default::default() + }, + signature: Signature::empty(), + } + } + + fn cached_bid(value_gwei: u64) -> Arc> { + Arc::new(SignedExecutionPayloadBid { + message: ExecutionPayloadBid { + builder_index: REMOTE_BUILDER, + value: value_gwei, + ..Default::default() + }, + signature: Signature::empty(), + }) + } + + fn local_build(payload_gwei: u64, should_override_builder: bool) -> LocalBuildResult { + LocalBuildResult { + payload_data: ExecutionPayloadData { + payload: types::ExecutionPayloadGloas::default(), + execution_requests: ExecutionRequests::default(), + builder_index: BUILDER_INDEX_SELF_BUILD, + slot: Slot::new(0), + blobs_and_proofs: (VariableList::empty(), VariableList::empty()), + }, + payload_value: gwei(payload_gwei), + should_override_builder, + } + } + + const LOCAL: BuilderIndex = BUILDER_INDEX_SELF_BUILD; + const REMOTE: BuilderIndex = REMOTE_BUILDER; + + /// Run `select_payload_bid_pure` and return `(winning_builder_index, has_payload_data)`. + /// + /// Args (positional, mirror `select_payload_bid_pure`): + /// - `local_payload_gwei`: local payload value, in gwei. + /// - `should_override`: EL's `shouldOverrideBuilder` flag. + /// - `cached_gwei`: `Some(g)` ⇒ seed the cache with a bid of `g` gwei. + /// - `boost`: `None` = neutral, `Some(0)` = always local, `Some(>100)` = boost bid. + fn pick( + local_payload_gwei: u64, + should_override: bool, + cached_gwei: Option, + boost: Option, + ) -> (BuilderIndex, bool) { + let build = local_build(local_payload_gwei, should_override); + let cache = cached_gwei.map(cached_bid); + let (out, data) = select_payload_bid_pure::(local_bid(), build, cache, boost); + (out.message.builder_index, data.is_some()) + } + + #[test] + fn select_empty_cache_keeps_local() { + assert_eq!(pick(0, false, None, Some(u64::MAX)), (LOCAL, true)); + } + + #[test] + fn select_el_override_beats_any_cached_bid() { + // `shouldOverrideBuilder` short-circuits regardless of cache or boost. + assert_eq!(pick(0, true, Some(u64::MAX), Some(u64::MAX)), (LOCAL, true)); + } + + #[test] + fn select_boost_zero_always_keeps_local() { + // boost=0 deflates the bid to 0 ⇒ local always wins. + assert_eq!(pick(0, false, Some(u64::MAX), Some(0)), (LOCAL, true)); + } + + #[test] + fn select_neutral_boost_picks_higher_bid() { + // 5 gwei bid > 1 gwei local, neutral compare ⇒ bid. + assert_eq!(pick(1, false, Some(5), None), (REMOTE, false)); + } + + #[test] + fn select_local_strictly_higher_keeps_local() { + assert_eq!(pick(10, false, Some(5), None), (LOCAL, true)); + } + + #[test] + fn select_tie_goes_to_local() { + // `>=` ⇒ local wins ties. + assert_eq!(pick(5, false, Some(5), None), (LOCAL, true)); + } + + #[test] + fn select_boost_factor_amplifies_bid() { + // 5 gwei local vs 3 gwei bid: raw ⇒ local. + assert_eq!(pick(5, false, Some(3), None), (LOCAL, true)); + // boost=200 ⇒ bid scaled to 6 gwei ⇒ bid wins. + assert_eq!(pick(5, false, Some(3), Some(200)), (REMOTE, false)); + } } diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index f67b5015c5c..f61a7abbe69 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -1186,6 +1186,7 @@ where randao_reveal, graffiti_settings, ProduceBlockVerification::VerifyRandao, + None, ) .await .unwrap(); diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index 38d4f4c47e1..be85fc2245b 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -1093,6 +1093,7 @@ async fn invalid_parent() { Duration::from_secs(0), &state, PayloadVerificationStatus::Optimistic, + block.message().proposer_index(), &rig.harness.chain.spec, ), Err(ForkChoiceError::ProtoArrayStringError(message)) diff --git a/beacon_node/beacon_chain/tests/prepare_payload.rs b/beacon_node/beacon_chain/tests/prepare_payload.rs index 549f15a13f9..47dd1ef517e 100644 --- a/beacon_node/beacon_chain/tests/prepare_payload.rs +++ b/beacon_node/beacon_chain/tests/prepare_payload.rs @@ -632,6 +632,7 @@ async fn gloas_block_production_caches_blobs_for_column_publishing() { randao_reveal, graffiti_settings, ProduceBlockVerification::VerifyRandao, + None, ) .await .unwrap(); diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index 4146543fd56..b2dabb7c018 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -205,6 +205,7 @@ pub struct BlockProposalContentsGloas { pub blob_kzg_commitments: KzgCommitments, pub blobs_and_proofs: (BlobsList, KzgProofs), pub execution_requests: ExecutionRequests, + pub should_override_builder: bool, } impl From> for BlockProposalContentsGloas { @@ -215,6 +216,7 @@ impl From> for BlockProposalContentsGloas blob_kzg_commitments: response.blobs_bundle.commitments, blobs_and_proofs: (response.blobs_bundle.blobs, response.blobs_bundle.proofs), execution_requests: response.requests, + should_override_builder: response.should_override_builder, } } } diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index b8326f4495c..56835da4598 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -3450,17 +3450,20 @@ impl ApiTester { .unwrap() .unwrap_or(self.chain.head_beacon_block_root()); - // Presently, the beacon chain harness never runs the code that primes the proposer - // cache. If this changes in the future then we'll need some smarter logic here, but - // this is succinct and effective for the time being. - assert!( - self.chain - .beacon_proposer_cache - .lock() - .get_epoch::(dependent_root, epoch) - .is_none(), - "the proposer cache should miss initially" - ); + // Block import primes the proposer cache for each epoch it runs through (to gate + // proposer boost), so epochs `<= current_epoch` are already cached. The only epoch + // for which we can observe the endpoint's own caching behaviour is + // `current_epoch + 1`, which no block import has touched yet. + if epoch == current_epoch + 1 { + assert!( + self.chain + .beacon_proposer_cache + .lock() + .get_epoch::(dependent_root, epoch) + .is_none(), + "the proposer cache should miss initially for the next epoch" + ); + } let result = self .client @@ -3468,8 +3471,9 @@ impl ApiTester { .await .unwrap(); - // Check that current-epoch requests prime the proposer cache, whilst non-current - // requests don't. + // A current-epoch request should leave the cache primed (block import already did so, + // but this is still a useful end-to-end check). A request for `current_epoch + 1` + // should not prime the cache. if epoch == current_epoch { assert!( self.chain @@ -3477,16 +3481,16 @@ impl ApiTester { .lock() .get_epoch::(dependent_root, epoch) .is_some(), - "a current-epoch request should prime the proposer cache" + "the proposer cache should be primed for the current epoch" ); - } else { + } else if epoch == current_epoch + 1 { assert!( self.chain .beacon_proposer_cache .lock() .get_epoch::(dependent_root, epoch) .is_none(), - "a non-current-epoch request should not prime the proposer cache" + "a request for the next epoch should not prime the proposer cache" ); } diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index a9e62dbe946..477d1fa3b4e 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -756,6 +756,7 @@ where block_delay: Duration, state: &BeaconState, payload_verification_status: PayloadVerificationStatus, + canonical_head_proposer_index: u64, spec: &ChainSpec, ) -> Result<(), Error> { let _timer = metrics::start_timer(&metrics::FORK_CHOICE_ON_BLOCK_TIMES); @@ -820,16 +821,18 @@ where let attestation_threshold = spec.get_attestation_due::(block.slot()); - // Add proposer score boost if the block is timely. - // TODO(gloas): the spec's `update_proposer_boost_root` additionally checks that - // `block.proposer_index == get_beacon_proposer_index(head_state)` — i.e. that - // the block's proposer matches the expected proposer on the canonical chain. - // This requires calling `get_head` and advancing the head state to the current - // slot, which is expensive. Implement once we have a cached proposer index. + // Add proposer score boost if the block is the first timely block for this slot and its + // proposer matches the expected proposer on the canonical chain (per spec + // `update_proposer_boost_root`, introduced in v1.7.0-alpha.5). let is_before_attesting_interval = block_delay < attestation_threshold; let is_first_block = self.fc_store.proposer_boost_root().is_zero(); - if current_slot == block.slot() && is_before_attesting_interval && is_first_block { + let is_canonical_proposer = block.proposer_index() == canonical_head_proposer_index; + if current_slot == block.slot() + && is_before_attesting_interval + && is_first_block + && is_canonical_proposer + { self.fc_store.set_proposer_boost_root(block_root); } diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index d6f937c0ca9..353893026bf 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -316,6 +316,7 @@ impl ForkChoiceTest { Duration::from_secs(0), &state, PayloadVerificationStatus::Verified, + block.message().proposer_index(), &self.harness.chain.spec, ) .unwrap(); @@ -359,6 +360,7 @@ impl ForkChoiceTest { Duration::from_secs(0), &state, PayloadVerificationStatus::Verified, + block.message().proposer_index(), &self.harness.chain.spec, ) .expect_err("on_block did not return an error"); diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 2af205ee471..8b0b74d2564 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -335,15 +335,6 @@ impl Case for ForkChoiceTest { } fn result(&self, _case_index: usize, fork_name: ForkName) -> Result<(), Error> { - // TODO(gloas): We have not implemented this change to fork choice/proposer boost yet. - // https://github.com/sigp/lighthouse/issues/8689 - if self.description == "voting_source_beyond_two_epoch" - || self.description == "justified_update_not_realized_finality" - || self.description == "justified_update_always_if_better" - { - return Err(Error::SkippedKnownFailure); - } - let tester = Tester::new(self, testing_spec::(fork_name))?; for step in &self.steps { @@ -791,6 +782,7 @@ impl Tester { block_delay, &state, PayloadVerificationStatus::Irrelevant, + block.message().proposer_index(), &self.harness.chain.spec, );