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

feat: beacon node process electra attestations EIP-7549 #6738

Merged
merged 3 commits into from
May 7, 2024

Conversation

ensi321
Copy link
Contributor

@ensi321 ensi321 commented May 5, 2024

  • Update validation for electra attestations from block
  • getBeaconCommittees() to retrieve multiple committees from a single attestation
  • getAttestingIndices() to retrieve a list of indices of validators who attested in the provided attestation

Part of #6341 and #6689

@ensi321 ensi321 requested a review from a team as a code owner May 5, 2024 11:45
@ensi321 ensi321 mentioned this pull request May 5, 2024
12 tasks

const validatorIndices = this.getBeaconCommittees(data.slot, committeeIndices);

const attestingIndices = new Set(aggregationBits.intersectValues(validatorIndices));
Copy link
Contributor

Choose a reason for hiding this comment

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

return the array from intersectValues directly, there should not be any duplicates here
if there is, it'll fail the validation anyway, the created Set does not help

} else {
const {aggregationBits, committeeBits, data} = attestation as electra.Attestation;

// There is a naming conflict on the term `committeeIndices`
Copy link
Contributor

Choose a reason for hiding this comment

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

add TODO: resolve the naming conflicts...

`aggregationBitsLength=${attestation.aggregationBits.bitLen} committeeLength=${committee.length}`
);
if (fork >= ForkSeq.electra) {
if (data.index !== 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

use assert.equal util

.map((committeeIndex) => epochCtx.getBeaconCommittee(data.slot, committeeIndex).length)
.reduce((acc, committeeSize) => acc + committeeSize, 0);

if (attestationElectra.aggregationBits.bitLen !== participantCount) {
Copy link
Contributor

Choose a reason for hiding this comment

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

use assert.equal util

@@ -146,7 +147,7 @@ export async function importBlock(

for (const attestation of attestations) {
try {
const indexedAttestation = postState.epochCtx.getIndexedAttestation(attestation);
const indexedAttestation = postState.epochCtx.getIndexedAttestation(fork, attestation);
Copy link
Contributor

Choose a reason for hiding this comment

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

add TODO: figure out how to reuse the attesting indices computed from state transition

getIndexedAttestationSignatureSet(state, state.epochCtx.getIndexedAttestation(attestation))
getIndexedAttestationSignatureSet(
state,
state.epochCtx.getIndexedAttestation(state.epochCtx.config.getForkSeq(signedBlock.message.slot), attestation)
Copy link
Contributor

Choose a reason for hiding this comment

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

add TODO: figure how to get attesting indices of an attestation once per block processing

@ensi321 ensi321 mentioned this pull request May 7, 2024
@ensi321
Copy link
Contributor Author

ensi321 commented May 7, 2024

@tuyennhv all your feedbacks have been addressed, and the changes are reflected in nc/7549. Please continue the review in #6689 .

@ensi321 ensi321 closed this May 7, 2024
@ensi321 ensi321 reopened this May 7, 2024
@ensi321 ensi321 merged commit 738d708 into electra-fork May 7, 2024
16 of 21 checks passed
@ensi321 ensi321 deleted the nc/7549-1 branch May 7, 2024 19:11
Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm review post merge

g11tech pushed a commit that referenced this pull request May 24, 2024
* Process attestations in block

* Fix check-types

* Address comments
g11tech pushed a commit to g11tech/lodestar that referenced this pull request Jun 19, 2024
* Process attestations in block

* Fix check-types

* Address comments
g11tech pushed a commit that referenced this pull request Jun 25, 2024
* Process attestations in block

* Fix check-types

* Address comments
g11tech pushed a commit that referenced this pull request Jun 25, 2024
* Process attestations in block

* Fix check-types

* Address comments
g11tech pushed a commit that referenced this pull request Jul 1, 2024
* Process attestations in block

* Fix check-types

* Address comments
g11tech pushed a commit that referenced this pull request Jul 30, 2024
* Process attestations in block

* Fix check-types

* Address comments
g11tech pushed a commit that referenced this pull request Jul 31, 2024
* Process attestations in block

* Fix check-types

* Address comments
g11tech pushed a commit that referenced this pull request Aug 2, 2024
* Process attestations in block

* Fix check-types

* Address comments
g11tech pushed a commit that referenced this pull request Aug 9, 2024
* Process attestations in block

* Fix check-types

* Address comments
g11tech pushed a commit that referenced this pull request Aug 9, 2024
* Process attestations in block

* Fix check-types

* Address comments
g11tech pushed a commit that referenced this pull request Aug 23, 2024
* Process attestations in block

* Fix check-types

* Address comments
g11tech pushed a commit that referenced this pull request Aug 27, 2024
* Process attestations in block

* Fix check-types

* Address comments
philknows pushed a commit that referenced this pull request Sep 3, 2024
* Process attestations in block

* Fix check-types

* Address comments
@wemeetagain
Copy link
Member

🎉 This PR is included in v1.22.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants