-
Notifications
You must be signed in to change notification settings - Fork 678
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix failing attestation tests and misc electra attestation cleanup #5810
Fix failing attestation tests and misc electra attestation cleanup #5810
Conversation
We should probably make these changes directly to |
i'll work on rebasing to |
51355e8
to
fdd7d16
Compare
- observed attestations are now keyed off of data + committee index - rename op pool attestationref to compactattestationref - remove unwraps in agg pool and use options instead - cherry pick some changes from ef-tests-electra
fdd7d16
to
ef7db9a
Compare
@@ -1222,13 +1221,13 @@ async fn attesting_to_optimistic_head() { | |||
let get_aggregated = || { | |||
rig.harness | |||
.chain | |||
.get_aggregated_attestation(attestation.data()) | |||
.get_aggregated_attestation(&attestation.to_ref()) | |||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things that accept &ObjectRef
should be changed to accept ObjectRef
since ObjectRef
is already Copy
. We should make a lint for this at some point..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 7328924
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Just small things and we should probably wait for electra_attestation_changes
to be cleaned up (some of the PRs during interop were merged to the wrong branches) before merging this.
e340998
into
sigp:electra_attestation_changes
NOTE: this PR includes changes from currently open attestation related PR's. In terms of the "conga line" this should probably be last
committee_index
We added the function
committee_index
to bothAttestation
andAttestationRef
to fetch the committee index across both attestation variants (base & electra). Previously the function was returning a u64. Its been updates to return aOption<u64>
since the electra variant can technically returnNone
if no committee_bits are set. IfNone
is returned for an attestations committee_index received over gossip, this violates the P2P rules and will result in the peer being down scored. The relevant logic for that has been added by returning aNotExactlyOneCommitteeBitSet
error inverify_early_checks
.get_indexed_attestation_from_signed_aggregate
When fetching an indexed attestation via a
signed_aggregate
for the base variant we make the following checksThese checks didn't exist for the electra variant, which caused inconsistencies in our testing infra. This PR adds a new function
get_indexed_attestation_from_signed_aggregate
for electra variant signed aggregates and adds the checks i've described above. Theres some additional cleanup that needs to happen here, i've added additional info about that in some newly included TODOsAggregation pool
Several unwraps were removed in favor of
Option
IndexedAggregatedAttestation
Now expectes a
observed_attestation_key_root
instead ofattestation_data_root
. The new key is generated by hashing the following structThis fixes an issue where the
observed_attestation
map was keyed off ofattestation_data_root
and therefore not effectively tracking unique attestations for electra. Weirdly enough, even without this change the local devnets durings interop seemed to be working fine, but I still believe this change is probably needed.Rename
AttestationRef
In order to reduce confusion between the superstruct
AttestationRef
and the op poolAttestationRef
, I've renamed the op pool struct toCompactAttestationRef
Test fixes