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: batch validation for electra attestations #6788

Merged
merged 12 commits into from
May 16, 2024

Conversation

twoeths
Copy link
Contributor

@twoeths twoeths commented May 16, 2024

Motivation

In order to do batch validation, we extract AttestationData base64 from its serialized data but it's not enough for electra. We need to also extract committeeBits instead

This issue would lead to invalid aggregated attestations when there are more than committee indices per slot

Description

  • Based on fork, extract either AttestationData (for phase0) or AttestationData + CommitteeBits (electra)
  • This is used for SeenAttestationDatas cache and IndexedGossipQueue
  • After batch gossip validation, we should have committeeIndex so that we don't extract again in attestation pool
  • Also fixed the bug we saw yesterday regarding undefined committeeBits

Need some more testing for this PR

const attestation: allForks.Attestation = attestationOrCache.attestation
? attestationOrCache.attestation
: {
aggregationBits,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

miss committeeBits yesterday which caused the serialization issue @nflaig

Copy link
Member

Choose a reason for hiding this comment

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

is there a way to make this more typesafe?
I'll make an issue for later, but there are some patterns in our code that lead to this kind of bug (that we should clean up later).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah need to review carefully and address when we merge electra to unstable I guess @wemeetagain

@twoeths
Copy link
Contributor Author

twoeths commented May 16, 2024

was able to reproduce the issue if I use "minimal", 64 validators x 4 clients, which make it 256 validators. With 8 slots per epoch we have 32 validators per slot, with TARGET_COMMITTEE_SIZE = 4 for minimal we have 32 : 4 = 8 committees per slot

got this error

[cl-1-lodestar-nethermind] May-16 07:49:32.307[api]             error: Error on publishAggregateAndProofs [0] slot=8, index=0, code=ATTESTATION_ERROR_INVALID_SIGNATURE
[cl-1-lodestar-nethermind] Error: ATTESTATION_ERROR_INVALID_SIGNATURE
[cl-1-lodestar-nethermind]     at validateAggregateAndProof (file:///usr/app/packages/beacon-node/src/chain/validation/aggregateAndProof.ts:230:11)
[cl-1-lodestar-nethermind]     at validateGossipFnRetryUnknownRoot (file:///usr/app/packages/beacon-node/src/network/processor/gossipHandlers.ts:749:14)
[cl-1-lodestar-nethermind]     at file:///usr/app/packages/beacon-node/src/api/impl/validator/index.ts:1106:76
[cl-1-lodestar-nethermind]     at async Promise.all (index 0)
[cl-1-lodestar-nethermind]     at Object.publishAggregateAndProofs (file:///usr/app/packages/beacon-node/src/api/impl/validator/index.ts:1096:7)
[cl-1-lodestar-nethermind]     at Object.handler (file:///usr/app/packages/api/src/utils/server/genericJsonServer.ts:45:23)

@twoeths twoeths force-pushed the te/batch_validation_electra_attestations branch 2 times, most recently from fff8e70 to 6f3171b Compare May 16, 2024 14:48
@twoeths twoeths force-pushed the te/batch_validation_electra_attestations branch from 6f3171b to 48b3b73 Compare May 16, 2024 15:35
@twoeths
Copy link
Contributor Author

twoeths commented May 16, 2024

ATTESTATION_ERROR_INVALID_SIGNATURE was gone as confirm in @ensi321 environment

@twoeths twoeths marked this pull request as ready for review May 16, 2024 16:26
@twoeths twoeths requested a review from a team as a code owner May 16, 2024 16:26
@twoeths twoeths merged commit 764bcc8 into electra-fork May 16, 2024
12 of 17 checks passed
@twoeths twoeths deleted the te/batch_validation_electra_attestations branch May 16, 2024 18:09
g11tech pushed a commit to g11tech/lodestar that referenced this pull request Jun 19, 2024
@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.

3 participants