Request missing payload envelopes for index-1 attestation#4939
Request missing payload envelopes for index-1 attestation#4939terencechain wants to merge 7 commits intoethereum:masterfrom
Conversation
terencechain
force-pushed
the
gossip-att-unknown-parent-hash
branch
from
89e0e47 to
6a3e5e9
Compare
terencechain
changed the title
ensi321
left a comment
ensi321
left a comment
There was a problem hiding this comment.
I think we should also update this statement:
consensus-specs/specs/gloas/p2p-interface.md
Lines 558 to 560 in f73487b
potuz
left a comment
potuz
left a comment
There was a problem hiding this comment.
I think we need to be explicit that if the payload was invalid the attestation needs to be rejected rather than ignored. Change the language to the usual double negation on p2p (my changes are just a quick suggestion, feel free to change accordingly). Also I believe this PR requires #4918 otherwise this pr forces clients to ignore attestations that should be processed.
Co-authored-by: Potuz <potuz@potuz.net>
Co-authored-by: Potuz <potuz@potuz.net>
specs/gloas/p2p-interface.md
Outdated
| block), the execution payload for `block` has been seen. Client that haven't | ||
| seen the payload MAY queue the attestation and SHOULD request the payload | ||
| envelope by root from the block's bid (e.g. | ||
| `block.body.signed_execution_payload_bid.message.block_hash`) using |
There was a problem hiding this comment.
Why does it say block_hash instead of something like attestation.data.beacon_block_root?
There was a problem hiding this comment.
Hmm you're right. ExecutionPayloadEnvelopesByRoot expects a beacon block root, not an execution payload (block) hash.
consensus-specs/specs/gloas/p2p-interface.md
Lines 550 to 554 in ee78d22
specs/gloas/p2p-interface.md
Outdated
| - _[REJECT]_ If `attestation.data.index == 1` (payload present for a past | ||
| block), the execution payload for `block` passes validation. | ||
| - _[IGNORE]_ If `attestation.data.index == 1` (payload present for a past | ||
| block), the execution payload for `block` has been seen. Client that haven't |
There was a problem hiding this comment.
Language here is a bit weird & inconsistent. You should fix this in both sections. And do not just apply the suggestion, it will fail linting.
| block), the execution payload for `block` has been seen. Client that haven't | |
| block), the execution payload for `block` has been seen. A client that has not |
This PR Implements the gossip validation rules from consensus-specs [#4939](ethereum/consensus-specs#4939) That is: - [REJECT] attestations with `CommitteeIndex == 1` (payload-present vote) when the execution payload for the attested block is known invalid - [IGNORE] attestations with `CommitteeIndex == 1` when the execution payload has not been seen, and request the payload envelope via `ExecutionPayloadEnvelopesByRoot`
jtraglia
requested review from
ensi321,
jihoonsong and
potuz
and removed request for
ensi321 and
potuz
5 participants
Add guidance to request missing execution payload envelopes when index-1 attestations indicate payload present for past blocks in
beacon_aggregate_and_proofandbeacon_attestation_{subnet_id}