Add executable gossip validation functions for gloas#5294
Conversation
| def validate_execution_payload_bid_gossip( | ||
| seen: Seen, | ||
| store: Store, | ||
| state: BeaconState, |
There was a problem hiding this comment.
what's the state here? eg. for bid validation we need to use the state of the parent block advanced to the bid slot via process_slots to make sure is_active_builder / can_builder_cover_bid are properly evaluated
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
The correct state is process_slots(head_state, bid.slot)
using the head state is not correct, the state needs to be the post-state of the parent block advanced to the bid slot
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
@nflaig @jihoonsong what do you think about this? 58452be
There was a problem hiding this comment.
I think we need to come up with a ground rule. Currently p2p functions accept various state, which is confusing. In some cases, it's takes the head state, in others it's the parent's post-state, in the rest it doesn't accept any state.
One way is to make it the head state always if it receives state, and derive state otherwise. For instance, we can do the following inside validate_execution_payload_bid_gossip().
state = store.block_states[bid.parent_block_root].copy()
process_slots(state, bid.slot)
Another way is making a note comment on which state above each function, allowing different state per function. In this way, we can also make variable name descriptive such as head_state, parent_post_state, etc.
Adding an ad-hoc function like get_validation_state could be a solution, but then we will have such function per p2p function with non-head state.
There was a problem hiding this comment.
I've asked my agent to look into each CL client to see which state they use for validate_execution_payload_bid_gossip() on glamsterdam-devnet-4 branch. Interestingly, it seems most clients use the head state. Perhaps we need to add some test cases that fail when you use the wrong state per p2p validation?
| Client | State used | Validation function |
|---|---|---|
| Lighthouse | Head state | verify_bid_consistency — beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs |
| Prysm | Head state | validateExecutionPayloadBidGossip — beacon-chain/sync/validate_execution_payload_bid.go |
| Teku | Parent post-state | ExecutionPayloadBidGossipValidator — statetransition/validation/ExecutionPayloadBidGossipValidator.java |
| Nimbus | Head state | validateExecutionPayloadBid — gossip_processing/gossip_validation.nim |
| Lodestar | Head state | validateGossipExecutionPayloadBid — chain/validation/executionPayloadBid.ts |
| Grandine | Parent post-state | validate_execution_payload_bid — fork_choice_store/src/store.rs |
| Erigon/Caplin | Head state | executionPayloadBidService.validateAndStoreBid — phase1/network/services/execution_payload_bid_service.go |
There was a problem hiding this comment.
this is a bug in lodestar, I have a open PR to fix it ChainSafe/lodestar#9409, curious why lighthouse and prysm use head state, that seems definitely wrong but maybe they have a good reason for it
also worth checking not only which state, head vs. parent block, but also if clients dial it to the bid slot or not, that is also relevant
generally, it would be good to get input from other clients teams as well
| parent_state = store.block_states[bid.parent_block_root] | ||
| dependent_root = get_proposer_dependent_root(parent_state, proposal_epoch) |
There was a problem hiding this comment.
| parent_state = store.block_states[bid.parent_block_root] | |
| dependent_root = get_proposer_dependent_root(parent_state, proposal_epoch) | |
| dependent_root = get_dependent_root(store, bid.parent_block_root) |
Can we remove get_proposer_dependent_root and use get_dependent_root instead like this? Related to #5196 and #5306. cc @nflaig
There was a problem hiding this comment.
I haven't followed up on that other PR but it should be equivalent
There was a problem hiding this comment.
I'd suggest using get_dependent_root() then. The other PR introduces it for PB conditions check and I think we can use it here, too.
This PR adds executable gossip message validation specs for Gloas.
Here are the reference tests for all Gloas gossip tests: