Gloas modify process_attestations #8283
Conversation
shane-moore
force-pushed
the
epbs-process-attestation
branch
from
2611ffc to
def8e8e
Compare
shane-moore
force-pushed
the
epbs-process-attestation
branch
from
def8e8e to
9d93170
Compare
eserilev
left a comment
eserilev
left a comment
There was a problem hiding this comment.
I haven't done a review against the spec yet, just some nits to hopefully make this PR a little bit easier to review
| @@ -0,0 +1,322 @@ | |||
| use super::*; | |||
There was a problem hiding this comment.
could we keep this stuff in process_operations? Makes the PR a bit easier to review if the diff here is minimized. The changes here are a bit sensitive and I don't want to miss anything
There was a problem hiding this comment.
@eserilev, I feel you on that! moved the logic over to process_operations
| let is_matching_payload = if is_attestation_same_slot(state, data)? { | ||
| // For same-slot attestations, data.index must be 0 | ||
| if data.index != 0 { | ||
| // TODO(EIP7732): consider if we want to use a different error type, since this is more of an overloaded data index scenario as opposed to the InvalidCommitteeIndex previous error. It's more like `AttestationInvalid::BadOverloadedDataIndex` |
There was a problem hiding this comment.
I think it'd be nice to introduce a new error variant
There was a problem hiding this comment.
for sure, I updated to the following but am happy to name it something else if another preference
if data.index != 0 {
return Err(Error::BadOverloadedDataIndex(data.index));
}
| if state.fork_name_unchecked().altair_enabled() { | ||
| initialize_progressive_balances_cache(state, spec)?; | ||
| altair_deneb::process_attestation( | ||
| altair_gloas::altair::process_attestation( |
There was a problem hiding this comment.
we'll write a test case for the gloas case in a subequent PR I assume?
There was a problem hiding this comment.
yep! added a TODO to write some test cases for this. we'll be able to write tests for gloas features once we can properly produce gloas blocks and payloads.
probably within the next couple weeks I would think. At that point, I'll go back and create tests for this and my other recent PR's, which also have TODO's for testing
shane-moore
force-pushed
the
epbs-process-attestation
branch
2 times, most recently
from
d08908b to
72fec1d
Compare
shane-moore
changed the title
| use types::{AttestationData, BeaconState, BeaconStateError, EthSpec}; | ||
|
|
||
| /// Checks if the attestation was for the block proposed at the attestation slot. | ||
| pub fn is_attestation_same_slot<E: EthSpec>( |
There was a problem hiding this comment.
this seems like something that might just be a function on the BeaconState itself rather than in its own file?
shane-moore
force-pushed
the
epbs-process-attestation
branch
from
72fec1d to
4d199a5
Compare
| && data.target.root == *state.get_block_root_at_epoch(data.target.epoch)?; | ||
| let is_matching_head = | ||
|
|
||
| let is_matching_blockroot = |
There was a problem hiding this comment.
Honestly i feel this code would be much easier to check if you just assign a variable to head_root_matches like the spec does. The spec may over-assign variables but I don't think that matters terribly for performance but it's just much easier to compare to the spec then.
There was a problem hiding this comment.
ah yah @ethDreamer , looks like head_root_matches came out of a more recent PR than the v1.6.0-beta.1 release, which I thought was devnet-0 target. i asked in epbs channel if that's still the target, and they seem like they're open to devnet-0 being based on at least the latest release, v1.6.1.
i've pushed commit to handle all the relevant changes to this function per latest spec.
Additionally, this week, I'll attempt to go through all the spec changes from v1.6.0-beta.1 til recent as to handle anything we'll need for self-building
4 participants
Changes per spec
process_attestationsmodifiedis_attestation_same_slotaddedget_attestation_participation_flag_indicesmodifiedHow This Works
The fast path for converting a slot's
builder_pending_paymentto abuilder_pending_withdrawaltriggers when the execution payload envelope for that slot is received and verified. In that case, we immediately queue the withdrawal corresponding to the CL block that carried the bid.However, the envelope may never be released. As a fallback, we want to convert the
builder_pending_paymentif enough validators attested on time to the block of that slot.What this PR Changes
Therefore, the
builder_pending_paymenthas aweightfield. During block processing, this PR modifiesprocess_attestationto updates this weight only for same slot attestations, adding the attester’s balance once per validator. (Note that same slot means that theattestation.data.beacon_block_rootcorresponds to thestate.get_block_root(data.slot)).In a future PR
Later, during epoch processing, we'll check if the weight is above quorum and if so, convert the
builder_pending_paymentto abuilder_pending_withdrawal, which will have awithdrawable_epoch.@ethDreamer