Fix valid Gloas bids rejected with InvalidGasLimit after an empty parent - #9905
Fix valid Gloas bids rejected with InvalidGasLimit after an empty parent#9905jimmygchen wants to merge 5 commits into
Conversation
eserilev
left a comment
There was a problem hiding this comment.
one small testing nit and maybe opportunity to add a TODO
other than that lgtm
| } | ||
|
|
||
| #[test] | ||
| fn gas_limit_uses_known_execution_parent_after_unreceived_payload() { |
There was a problem hiding this comment.
it would be nice if we could test GossipVerifiedBid::new directly here
There was a problem hiding this comment.
Done. valid_bid now calls GossipVerifiedPayloadBid::new directly with a non-zero EL genesis hash and no stored genesis envelope. The finalized-history regression also goes through the constructor.
| }; | ||
|
|
||
| /// Find the beacon block carrying the known execution payload referenced by a bid. | ||
| pub(crate) fn find_execution_payload_block_root<T: BeaconChainTypes>( |
There was a problem hiding this comment.
just noting that since is_bid_compatible_with_head runs first this while loop is bounded. if we didnt call that check before, we could end up looping all the way back to finalization which in a non finalized network is still probably not a big deal. but nice to keep this bounded i think
There was a problem hiding this comment.
I moved the ancestry lookup after the signature check and cache the gas limit by execution block hash, so we don't repeat the walk across slots.
| }; | ||
|
|
||
| if let Some(block_hash) = block.execution_payload_block_hash { | ||
| if fork_choice_read.is_payload_received(&root) && block_hash == parent_block_hash { |
There was a problem hiding this comment.
right now we are (correctly assuming) that payload received means payload is valid. but with optimistic sync (when we eventually implement it) this might no longer be the case. might be worth adding a TODO here so that we dont forget
feel like theres probably other places in the codebase that could be affected once we impl optimistic sync
|
Tick the box to add this pull request to the merge queue (same as
|
|
Some required checks have failed. Could you please take a look @jimmygchen? 🙏 |
Use the EL genesis hash carried by the genesis bid when no payload envelope exists. Cache resolved parent gas limits per slot so invalid follow-up bids do not repeat historical ancestry scans.
eserilev
left a comment
There was a problem hiding this comment.
a few changes and I think we should be good. most are just comment nits
could we also add one more test that checks the case where the payload isnt in fork choice or the store? would want to ensure we dont iterate to genesis in that case
| 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> { |
There was a problem hiding this comment.
this function could potentially iterate all the way back to genesis? am i wrong about that?
is_bid_compatible_with_head check can still pass even if its payload hasnt been imported yet.
Maybe we should only enter this loop when parent_block_hash == head_state.latest_block_hash() so that we have guarantees that the payload was in fact imported
| if fork_choice_read.is_payload_received(&block_root) | ||
| && block_hash == parent_execution_block_hash |
There was a problem hiding this comment.
i think we could also add an elif block_hash == parent_execution_block_hash && !is_payload_received and return a payload unknown error if true
| PayloadBidError::InternalError(format!( | ||
| "failed to load beacon block while finding execution payload: {e:?}" | ||
| )) |
There was a problem hiding this comment.
not sure this should be an internal error, I think it should be a ParentExecutionPayloadUnknown. If we ever added bid reprocessing we could send the bid to the reprocess queue in this case and trigger a lookup?
| 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), | ||
| } |
There was a problem hiding this comment.
nit: this enum is a bit confusing to me
What if we renamed to something like
enum ForkChoiceLookup {
Found(Hash256),
NotFound { continue_from: Hash256 }
}or something where fork choice found and not found is mentioned explicitly?
| SearchStoreFrom(Hash256), | ||
| } | ||
|
|
||
| /// Locate the beacon block that identifies the bid's parent execution payload in fork choice. |
There was a problem hiding this comment.
nit: maybe add additional info on what happens if its not found in fork choice?
| /// 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. |
There was a problem hiding this comment.
nit
| /// 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. | |
| /// Return the block hash and gas limit this beacon block committed to. | |
| /// | |
| /// Pre-Gloas blocks embed the payload. Gloas blocks commit to a payload via their bid. | |
| /// Gloas genesis is a special case since the block commits to an empty payload. The EL genesis | |
| /// hash comes from the bid's `parent_block_hash` |
| /// Continue searching canonical ancestry in the database after fork choice reaches its finalized | ||
| /// boundary. |
There was a problem hiding this comment.
| /// Continue searching canonical ancestry in the database after fork choice reaches its finalized | |
| /// boundary. | |
| /// Iterate parent roots in the store looking for the block that committed to the parent execution payload. | |
| /// Used when the payload we are looking for has already been pruned by fork choice. |
(this comment is only true if we add the parent_block_hash == head_state.latest_block_hash() check I mentioned below)
| // 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. |
There was a problem hiding this comment.
nit
| // 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. | |
| // For gloas blocks post-genesis, we must check that the payload has been received |
| /// Load the gas limit for `parent_execution_block_hash` from the beacon block previously identified | ||
| /// as representing that execution payload. |
There was a problem hiding this comment.
| /// Load the gas limit for `parent_execution_block_hash` from the beacon block previously identified | |
| /// as representing that execution payload. | |
| /// Return the gas limit committed by the beacon block at `parent_execution_payload_beacon_block_root` | |
| /// for the payload `parent_execution_block_hash`. |
Issue Addressed
After an empty parent, a valid payload bid can be rejected with
InvalidGasLimit. The check currently uses the head state’s latest bid, which can refer to an unreceived payload, instead of the execution payload referenced bybid.parent_block_hash.The spec validates against the known parent execution payload’s gas limit. This bug causes valid bids to be ignored and not propagated.
Proposed Changes
bid.parent_block_hashin the parent branch.