Skip to content
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

Merged

Conversation

eserilev
Copy link
Collaborator

@eserilev eserilev commented May 17, 2024

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 both Attestation and AttestationRef to fetch the committee index across both attestation variants (base & electra). Previously the function was returning a u64. Its been updates to return a Option<u64> since the electra variant can technically return None if no committee_bits are set. If None 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 a NotExactlyOneCommitteeBitSet error in verify_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 checks

  • selection_proof.is_aggregator returns true
  • committee contains the signed aggregates aggregator index

These 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 TODOs

Aggregation pool

Several unwraps were removed in favor of Option

IndexedAggregatedAttestation

Now expectes a observed_attestation_key_root instead of attestation_data_root. The new key is generated by hashing the following struct

struct ObservedAttestationKey {
    committee_index: u64,
    attestation_data: AttestationData
}

This fixes an issue where the observed_attestation map was keyed off of attestation_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 pool AttestationRef, I've renamed the op pool struct to CompactAttestationRef

Test fixes

  • Fix an issue where beacon chain tests werent compiling
  • All beacon chain tests except one are passing. The one that fails doesnt fail on a different branch of mine that also includes the ef test changes

@eserilev eserilev added electra Required for the Electra/Prague fork work-in-progress PR is a work-in-progress labels May 20, 2024
@realbigsean
Copy link
Member

We should probably make these changes directly to electra_attestation_changes branch so we can keep all the attesation changes together. (we can pull the changes through the conga line after merging this to that branch)

@realbigsean realbigsean added the skip-ci Don't run the `test-suite` label May 20, 2024
@eserilev eserilev changed the base branch from ef-tests-electra to electra_attestation_changes May 20, 2024 21:50
@eserilev eserilev changed the base branch from electra_attestation_changes to ef-tests-electra May 20, 2024 21:54
@eserilev
Copy link
Collaborator Author

i'll work on rebasing to electra_attestation_changes

- 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
@eserilev eserilev changed the base branch from ef-tests-electra to electra_attestation_changes May 22, 2024 14:32
@eserilev eserilev changed the title Ef tests electra cleanup Fix failing attestation tests and misc electra attestation cleanup May 22, 2024
@eserilev eserilev added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels May 22, 2024
@eserilev eserilev removed the skip-ci Don't run the `test-suite` label May 22, 2024
@eserilev eserilev added the skip-ci Don't run the `test-suite` label May 22, 2024
@@ -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())
};
Copy link
Member

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..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 7328924

Copy link
Member

@ethDreamer ethDreamer left a 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.

@realbigsean realbigsean merged commit e340998 into sigp:electra_attestation_changes May 30, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
electra Required for the Electra/Prague fork ready-for-review The code is ready for review skip-ci Don't run the `test-suite`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants