Fix valid Gloas bids rejected with InvalidGasLimit after an empty parent

beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs

@@ -15,12 +15,152 @@ use slot_clock::SlotClock;
 use state_processing::signature_sets::{
     execution_payload_bid_signature_set, get_builder_pubkey_from_state,
 };
+use store::iter::ParentRootBlockIterator;
 use tracing::debug;
 use types::{
-    BeaconState, ChainSpec, EthSpec, ExecutionPayloadBid, SignedExecutionPayloadBid,
-    SignedProposerPreferences, Slot, consts::gloas::PAYLOAD_BUILDER_VERSION,
+    BeaconState, ChainSpec, EthSpec, ExecPayload, ExecutionBlockHash, ExecutionPayloadBid, Hash256,
+    SignedBlindedBeaconBlock, SignedExecutionPayloadBid, SignedProposerPreferences, Slot,
+    consts::gloas::PAYLOAD_BUILDER_VERSION,
 };
 
+pub(super) enum ParentExecutionPayloadLocation {
+    /// The beacon block identifying the parent execution payload is available in fork choice.
+    BeaconBlock(Hash256),
+    /// Continue the canonical ancestry search in the database from this beacon block root.
+    SearchStoreFrom(Hash256),
+}
+
+/// Locate the beacon block that identifies the bid's parent execution payload in fork choice.
+pub(super) fn locate_parent_execution_payload_in_fork_choice<T: BeaconChainTypes>(
+    fork_choice_read: &ForkChoiceReadGuard<'_, T>,
+    bid_parent_beacon_block_root: Hash256,
+    parent_execution_block_hash: ExecutionBlockHash,
+) -> ParentExecutionPayloadLocation {
+    let mut block_root = bid_parent_beacon_block_root;
+    while let Some(block) = fork_choice_read.get_block(&block_root) {
+        // Gloas genesis has no payload envelope. Its bid retains the EL genesis hash in
+        // `parent_block_hash`, while `block_hash` is zero to represent an empty payload.
+        if block.slot == Slot::new(0)
+            && block.execution_payload_parent_hash == Some(parent_execution_block_hash)
+        {
+            return ParentExecutionPayloadLocation::BeaconBlock(block_root);
+        }
+
+        if let Some(block_hash) = block.execution_payload_block_hash {
+            // Payload receipt is only recorded after the Gloas envelope has been validated.
+            if fork_choice_read.is_payload_received(&block_root)
+                && block_hash == parent_execution_block_hash
+            {
+                return ParentExecutionPayloadLocation::BeaconBlock(block_root);
+            }
+        } else if !block.execution_status.is_invalid()
+            && block.execution_status.block_hash() == Some(parent_execution_block_hash)
+        {
+            return ParentExecutionPayloadLocation::BeaconBlock(block_root);
+        }
+
+        let Some(parent_root) = block.parent_root else {
+            break;
+        };
+        block_root = parent_root;
+    }
+
+    ParentExecutionPayloadLocation::SearchStoreFrom(block_root)
+}
+
+/// Return the execution block hash and gas limit represented by a beacon block.
+///
+/// Gloas genesis represents the EL genesis block with `parent_block_hash` because it has no payload
+/// envelope of its own.
+fn execution_payload_hash_and_gas_limit<E: EthSpec>(
+    block: &SignedBlindedBeaconBlock<E>,
+) -> Result<(ExecutionBlockHash, u64), PayloadBidError> {
+    if block.fork_name_unchecked().gloas_enabled() {
+        let bid = &block
+            .message()
+            .body()
+            .signed_execution_payload_bid()?
+            .message;
+        let block_hash = if block.slot() == Slot::new(0) {
+            bid.parent_block_hash
+        } else {
+            bid.block_hash
+        };
+        Ok((block_hash, bid.gas_limit))
+    } else {
+        let payload = block.message().execution_payload()?;
+        Ok((payload.block_hash(), payload.gas_limit()))
+    }
+}
+
+/// Continue searching canonical ancestry in the database after fork choice reaches its finalized
+/// boundary.
+fn find_parent_execution_payload_gas_limit_in_store<T: BeaconChainTypes>(
+    store: &BeaconStore<T>,
+    search_start_beacon_block_root: Hash256,
+    parent_execution_block_hash: ExecutionBlockHash,
+) -> Result<u64, PayloadBidError> {
+    for block_result in ParentRootBlockIterator::new(store.as_ref(), search_start_beacon_block_root)
+    {
+        let (block_root, block) = block_result.map_err(|e| {
+            PayloadBidError::InternalError(format!(
+                "failed to load beacon block while finding execution payload: {e:?}"
+            ))
+        })?;
+
+        let (block_hash, gas_limit) = execution_payload_hash_and_gas_limit(&block)?;
+        if block_hash == parent_execution_block_hash {
+            // Non-genesis Gloas blocks only carry an execution payload after its envelope arrives.
+            // Compare the hash first so unrelated ancestors do not require another database read.
+            if block.fork_name_unchecked().gloas_enabled() && block.slot() != Slot::new(0) {
+                let payload_received = store.payload_envelope_exists(&block_root).map_err(|e| {
+                    PayloadBidError::InternalError(format!(
+                        "failed to check payload envelope for block {block_root:?}: {e:?}"
+                    ))
+                })?;
+                if payload_received {
+                    return Ok(gas_limit);
+                }
+            } else {
+                return Ok(gas_limit);
+            }
+        }
+    }
+
+    Err(PayloadBidError::ParentExecutionPayloadUnknown {
+        parent_block_hash: parent_execution_block_hash,
+    })
+}
+
+/// Load the gas limit for `parent_execution_block_hash` from the beacon block previously identified
+/// as representing that execution payload.
+fn get_parent_execution_payload_gas_limit_from_beacon_block<T: BeaconChainTypes>(
+    store: &BeaconStore<T>,
+    parent_execution_payload_beacon_block_root: Hash256,
+    parent_execution_block_hash: ExecutionBlockHash,
+) -> Result<u64, PayloadBidError> {
+    let block = store
+        .get_blinded_block(&parent_execution_payload_beacon_block_root)
+        .map_err(|e| {
+            PayloadBidError::InternalError(format!(
+                "failed to load execution payload block {parent_execution_payload_beacon_block_root:?}: {e:?}"
+            ))
+        })?
+        .ok_or_else(|| {
+            PayloadBidError::InternalError(format!(
+                "execution payload block {parent_execution_payload_beacon_block_root:?} unavailable"
+            ))
+        })?;
+
+    let (block_hash, gas_limit) = execution_payload_hash_and_gas_limit(&block)?;
+    if block_hash != parent_execution_block_hash {
+        return Err(PayloadBidError::InternalError(format!(
+            "execution payload hash mismatch for block {parent_execution_payload_beacon_block_root:?}"
+        )));
+    }
+    Ok(gas_limit)
+}
+
 /// Verify that an execution payload bid is consistent with the current chain state
 /// and proposer preferences.
 pub(crate) fn verify_bid_consistency<E: EthSpec>(
@@ -302,24 +442,6 @@ impl<E: EthSpec> GossipVerifiedPayloadBid<E> {
             });
         }
 
-        // TODO(gloas): [IGNORE] bid.parent_block_hash is the block hash of a known execution
-        // payload in fork choice.
-
-        // TODO(gloas): This uses head state's bid gas_limit as parent_gas_limit, which is only
-        // correct when the bid's parent is the head. If the parent is an ancestor further back
-        // this check may be inaccurate. Fixing this requires storing
-        // gas_limit in fork choice or looking it up from the store by parent_block_hash. Taking the above
-        // TODO into consideration maybe should persist parent block hash and gas limit in fork choice?
-        if let Ok(parent_bid) = head_state.latest_execution_payload_bid()
-            && !is_gas_limit_target_compatible(
-                parent_bid.gas_limit,
-                signed_bid.message.gas_limit,
-                proposer_preferences.message.target_gas_limit,
-            )?
-        {
-            return Err(PayloadBidError::InvalidGasLimit);
-        }
-
         drop(fork_choice);
 
         verify_bid_consistency(
@@ -330,7 +452,26 @@ impl<E: EthSpec> GossipVerifiedPayloadBid<E> {
             ctx.spec,
         )?;
 
-        // Verify signature
+        let check_parent_gas_limit = |parent_gas_limit| {
+            if is_gas_limit_target_compatible(
+                parent_gas_limit,
+                signed_bid.message.gas_limit,
+                proposer_preferences.message.target_gas_limit,
+            )? {
+                Ok(())
+            } else {
+                Err(PayloadBidError::InvalidGasLimit)
+            }
+        };
+
+        let cached_parent_gas_limit = ctx
+            .gossip_verified_payload_bid_cache
+            .get_parent_gas_limit(bid_slot, signed_bid.message.parent_block_hash);
+        if let Some(parent_gas_limit) = cached_parent_gas_limit {
+            check_parent_gas_limit(parent_gas_limit)?;
+        }
+
+        // Verify the signature before falling back to a historical database scan.
         execution_payload_bid_signature_set(
             head_state,
             |i| get_builder_pubkey_from_state(head_state, i),
@@ -343,6 +484,38 @@ impl<E: EthSpec> GossipVerifiedPayloadBid<E> {
         .then_some(())
         .ok_or(PayloadBidError::BadSignature)?;
 
+        if cached_parent_gas_limit.is_none() {
+            // Ancestor lookup can walk fork choice and the database, so only perform it for a bid
+            // whose builder and signature have already been verified.
+            let fork_choice = ctx.canonical_head.fork_choice_read_lock();
+            let location = locate_parent_execution_payload_in_fork_choice(
+                &fork_choice,
+                signed_bid.message.parent_block_root,
+                signed_bid.message.parent_block_hash,
+            );
+            drop(fork_choice);
+
+            let gas_limit = match location {
+                ParentExecutionPayloadLocation::BeaconBlock(beacon_block_root) => {
+                    get_parent_execution_payload_gas_limit_from_beacon_block::<T>(
+                        ctx.store,
+                        beacon_block_root,
+                        signed_bid.message.parent_block_hash,
+                    )?
+                }
+                ParentExecutionPayloadLocation::SearchStoreFrom(beacon_block_root) => {
+                    find_parent_execution_payload_gas_limit_in_store::<T>(
+                        ctx.store,
+                        beacon_block_root,
+                        signed_bid.message.parent_block_hash,
+                    )?
+                }
+            };
+            ctx.gossip_verified_payload_bid_cache
+                .insert_parent_gas_limit(bid_slot, signed_bid.message.parent_block_hash, gas_limit);
+            check_parent_gas_limit(gas_limit)?;
+        }
+
         let gossip_verified_bid = GossipVerifiedPayloadBid { signed_bid };
 
         ctx.gossip_verified_payload_bid_cache

beacon_node/beacon_chain/src/payload_bid_verification/mod.rs

@@ -10,7 +10,7 @@
 //!    GossipVerifiedPayloadBid -------> Insert into GossipVerifiedPayloadBidCache
 //! ```
 
-use types::{BeaconStateError, Hash256, Slot};
+use types::{BeaconStateError, ExecutionBlockHash, Hash256, Slot};
 
 pub mod gossip_verified_bid;
 pub mod payload_bid_cache;
@@ -22,6 +22,10 @@ mod tests;
 pub enum PayloadBidError {
     /// The bid's parent block root is unknown.
     ParentBlockRootUnknown { parent_block_root: Hash256 },
+    /// The bid's parent execution payload is unknown.
+    ParentExecutionPayloadUnknown {
+        parent_block_hash: ExecutionBlockHash,
+    },
     /// 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.

beacon_node/beacon_chain/src/payload_bid_verification/payload_bid_cache.rs

@@ -27,16 +27,24 @@ impl BidParent {
 
 type HighestBidMap<E> = BTreeMap<Slot, HashMap<BidParent, GossipVerifiedPayloadBid<E>>>;
 
+#[derive(Clone, Copy)]
+struct CachedParentGasLimit {
+    gas_limit: u64,
+    last_referenced_bid_slot: Slot,
+}
+
 pub struct GossipVerifiedPayloadBidCache<E: EthSpec> {
     highest_bid: RwLock<HighestBidMap<E>>,
     seen_builder_bids: RwLock<BTreeMap<Slot, HashSet<(BidParent, BuilderIndex)>>>,
+    parent_gas_limits: RwLock<HashMap<ExecutionBlockHash, CachedParentGasLimit>>,
 }
 
 impl<E: EthSpec> Default for GossipVerifiedPayloadBidCache<E> {
     fn default() -> Self {
         Self {
             highest_bid: RwLock::new(BTreeMap::new()),
             seen_builder_bids: RwLock::new(BTreeMap::new()),
+            parent_gas_limits: RwLock::new(HashMap::new()),
         }
     }
 }
@@ -94,6 +102,42 @@ impl<E: EthSpec> GossipVerifiedPayloadBidCache<E> {
             ));
     }
 
+    /// Get the gas limit of a parent execution payload and record the latest bid slot that
+    /// referenced it.
+    pub(crate) fn get_parent_gas_limit(
+        &self,
+        bid_slot: Slot,
+        parent_execution_block_hash: ExecutionBlockHash,
+    ) -> Option<u64> {
+        self.parent_gas_limits
+            .write()
+            .get_mut(&parent_execution_block_hash)
+            .map(|cached| {
+                cached.last_referenced_bid_slot = cached.last_referenced_bid_slot.max(bid_slot);
+                cached.gas_limit
+            })
+    }
+
+    /// Cache the gas limit of the parent execution payload identified by its execution block hash.
+    pub(crate) fn insert_parent_gas_limit(
+        &self,
+        bid_slot: Slot,
+        parent_execution_block_hash: ExecutionBlockHash,
+        gas_limit: u64,
+    ) {
+        self.parent_gas_limits
+            .write()
+            .entry(parent_execution_block_hash)
+            .and_modify(|cached| {
+                cached.gas_limit = gas_limit;
+                cached.last_referenced_bid_slot = cached.last_referenced_bid_slot.max(bid_slot);
+            })
+            .or_insert(CachedParentGasLimit {
+                gas_limit,
+                last_referenced_bid_slot: bid_slot,
+            });
+    }
+
     /// Prune anything before `current_slot`
     pub fn prune(&self, current_slot: Slot) {
         self.highest_bid
@@ -103,6 +147,14 @@ impl<E: EthSpec> GossipVerifiedPayloadBidCache<E> {
         self.seen_builder_bids
             .write()
             .retain(|&slot, _| slot >= current_slot);
+
+        self.parent_gas_limits
+            .write()
+            // Pruning runs before bids arrive for `current_slot`. Keep parents referenced in the
+            // preceding slot so a continuing empty-payload chain can refresh the entry.
+            .retain(|_, cached| {
+                cached.last_referenced_bid_slot.saturating_add(1u64) >= current_slot
+            });
     }
 }
 
@@ -221,6 +273,32 @@ mod tests {
         assert_eq!(highest_b.message.builder_index, 3);
     }
 
+    #[test]
+    fn parent_gas_limit_is_reused_across_slots_and_pruned_by_latest_reference() {
+        let cache = GossipVerifiedPayloadBidCache::<E>::default();
+        let parent_block_hash = ExecutionBlockHash::repeat_byte(0x01);
+
+        assert_eq!(
+            cache.get_parent_gas_limit(Slot::new(1), parent_block_hash),
+            None
+        );
+        cache.insert_parent_gas_limit(Slot::new(1), parent_block_hash, 30_000_000);
+
+        // Slot pruning runs before bids arrive for the new slot, so an entry used in the
+        // preceding slot must survive long enough to be reused and refreshed.
+        cache.prune(Slot::new(2));
+        assert_eq!(
+            cache.get_parent_gas_limit(Slot::new(2), parent_block_hash),
+            Some(30_000_000)
+        );
+
+        cache.prune(Slot::new(4));
+        assert_eq!(
+            cache.get_parent_gas_limit(Slot::new(4), parent_block_hash),
+            None
+        );
+    }
+
     #[test]
     fn prune_removes_old_retains_current() {
         let cache = GossipVerifiedPayloadBidCache::<E>::default();