test: enable spec tests related to eip-7549 #6741

nazarhussain · 2024-05-07T13:57:24Z

Motivation

Increase spec test coverage for Electra.

Description

Enable the spec tests related to EIP7549.
Add SPEC_FILTER_FORK and SPEC_FILTER_FORK environmental variable which will be read in during spec test
Fix committeeIndex check during processAttestation()

Steps to test or reproduce

Run all tests

wemeetagain · 2024-05-07T14:49:51Z

packages/state-transition/src/block/processAttestationPhase0.ts

@@ -93,7 +93,7 @@ export function validateAttestation(
      throw new Error(`AttestationData.index must be zero: index=${data.index}`);
    }
    const attestationElectra = attestation as electra.Attestation;
-    const committeeBitsLength = attestationElectra.committeeBits.bitLen;
+    const committeeBitsLength = attestationElectra.committeeBits.getTrueBitIndexes().length;


@g11tech is this correct?

The committeeBits is a bit vector so the bitLen will always remain fixed which is MAX_COMMITTEES_PER_SLOT. Hence if a chain is starting with initial validators less than MAX_COMMITTEES_PER_SLOT the attestation check will never pass.

So we actually need to get how many comittees actually participatd using getTrueBitIndexes.

The if(committeeBitsLength > committeeCount) check in electra-fork branch is wrong.

The check is supposedly to be identical to this line of the spec. The reason is although committeeBits has a set length of 64, actual number of committees might be less than that. If the 60th bit is set, but there is only 40 committees, then this check needs to throw error

Here we actually want the position of the last set bit in attestationElectra.committeeBits. If lastCommitteeIndex is greater than committeeCount, then we throw error.

So something like this:

const committeeIndices = attestationElectra.committeeBits.getTrueBitIndexes(); if (committeeIndices.length === 0) { throw Error("Should have at least one committee bit set"); } else { const lastCommitteeIndex = committeeIndices[committeeIndices.length - 1]; if (lastCommitteeIndex >= committeeCount) { throw new Error("Committee index exceeds committee count"); } }

This is beyond the scope of this PR, happy to fix this in another PR. Maybe add a todo here for now. Thanks for catching it @nazarhussain @wemeetagain

* feat: getAttestationsForBlock() for electra * chore: fix lint * fix: MAX_ATTESTATIONS_PER_GROUP_ELECTRA and address PR comments * chore: unit test aggregateConsolidation * Fix rebase mistake * Address my own comment :) --------- Co-authored-by: Navie Chan <naviechan@gmail.com>

packages/beacon-node/test/spec/utils/specTestIterator.ts

ensi321 · 2024-05-08T13:47:01Z

packages/state-transition/src/block/processAttestationPhase0.ts

@@ -93,7 +93,7 @@ export function validateAttestation(
      throw new Error(`AttestationData.index must be zero: index=${data.index}`);
    }
    const attestationElectra = attestation as electra.Attestation;
-    const committeeBitsLength = attestationElectra.committeeBits.bitLen;
+    const committeeBitsLength = attestationElectra.committeeBits.getTrueBitIndexes().length;


The if(committeeBitsLength > committeeCount) check in electra-fork branch is wrong.

The check is supposedly to be identical to this line of the spec. The reason is although committeeBits has a set length of 64, actual number of committees might be less than that. If the 60th bit is set, but there is only 40 committees, then this check needs to throw error

Here we actually want the position of the last set bit in attestationElectra.committeeBits. If lastCommitteeIndex is greater than committeeCount, then we throw error.

ensi321 · 2024-05-08T14:02:07Z

packages/state-transition/src/block/processAttestationPhase0.ts

@@ -93,7 +93,7 @@ export function validateAttestation(
      throw new Error(`AttestationData.index must be zero: index=${data.index}`);
    }
    const attestationElectra = attestation as electra.Attestation;
-    const committeeBitsLength = attestationElectra.committeeBits.bitLen;
+    const committeeBitsLength = attestationElectra.committeeBits.getTrueBitIndexes().length;


So something like this:

const committeeIndices = attestationElectra.committeeBits.getTrueBitIndexes(); if (committeeIndices.length === 0) { throw Error("Should have at least one committee bit set"); } else { const lastCommitteeIndex = committeeIndices[committeeIndices.length - 1]; if (lastCommitteeIndex >= committeeCount) { throw new Error("Committee index exceeds committee count"); } }

This is beyond the scope of this PR, happy to fix this in another PR. Maybe add a todo here for now. Thanks for catching it @nazarhussain @wemeetagain

…to nh/5749-spec-tests

nazarhussain · 2024-05-08T14:17:01Z

@ensi321 As this PR is enabling the spec tests for electra/oeprations/attestations so any fix to make these pass preferrably go with this PR. So I just added a commit with the fix you suggested and it's passing the spec.

ensi321

looks good overall minor comment

ensi321 · 2024-05-08T14:23:17Z

packages/beacon-node/test/spec/utils/specTestIterator.ts

-    if (opts?.skippedForks?.includes(forkStr)) {
+    if (
+      opts?.skippedForks?.includes(forkStr) ||
+      (process.env.SPEC_FILTER_FORK && forkStr !== process.env.SPEC_FILTER_FORK)


Curious why we would want to specify fork that we want to skip in a env variable SPEC_FILTER_FORK. What's the use case for this? I think defaultSkipOpts is serving quite well

Quickly run the spec tests for some forks. Whilde debugging, if we run for all forks it take too much time.

SPEC_FILTER_FORK=electra yarn test:spec:minimal

ensi321

lgtm

* initial commit * Update gossip validation * Update attestation gossip validation * aggregateAndProof validation * Extend spec runner to be more flexible * Add missing state attributes for electra * Fix ss data types for electra spec * Make the spec runner more flexible * Fix the bug in process attestation * Update the sepc test version * clean up * Misc * Fix the build erros * feat: get attestations for electra block (#6732) * feat: getAttestationsForBlock() for electra * chore: fix lint * fix: MAX_ATTESTATIONS_PER_GROUP_ELECTRA and address PR comments * chore: unit test aggregateConsolidation * Fix rebase mistake * Address my own comment :) --------- Co-authored-by: Navie Chan <naviechan@gmail.com> * Fix check-types * Address comments * Fix the build erros * Extend spec runner to be more flexible * Add missing state attributes for electra * Fix ss data types for electra spec * Make the spec runner more flexible * Fix the bug in process attestation * Update the sepc test version * Fix rebase issue * Update committee index count check --------- Co-authored-by: NC <adrninistrator1@protonmail.com> Co-authored-by: Navie Chan <naviechan@gmail.com> Co-authored-by: tuyennhv <tuyen@chainsafe.io>

nazarhussain requested a review from a team as a code owner May 7, 2024 13:57

wemeetagain reviewed May 7, 2024

View reviewed changes

nazarhussain changed the title ~~Enable spec tests related to eip-7549~~ test: enable spec tests related to eip-7549 May 7, 2024

ensi321 force-pushed the nc/7549 branch from 70e770d to b286351 Compare May 7, 2024 19:21

nazarhussain force-pushed the nh/5749-spec-tests branch from 02b5f1c to 7c7bdeb Compare May 8, 2024 08:36

Base automatically changed from nc/7549 to electra-fork May 8, 2024 11:00

nazarhussain force-pushed the nh/5749-spec-tests branch from 84d0950 to 28ae804 Compare May 8, 2024 12:25

ensi321 and others added 10 commits May 8, 2024 15:27

initial commit

6ddf100

Update gossip validation

14e6fd8

Update attestation gossip validation

32e8ae0

aggregateAndProof validation

f491628

Extend spec runner to be more flexible

39b259e

Add missing state attributes for electra

2ba9a22

Fix ss data types for electra spec

8061a4d

Make the spec runner more flexible

c509eb0

Fix the bug in process attestation

c7f4b26

Update the sepc test version

0ff25d9

nazarhussain force-pushed the nh/5749-spec-tests branch from 28ae804 to 0ff25d9 Compare May 8, 2024 12:32

ensi321 and others added 12 commits May 8, 2024 16:23

clean up

cd74406

Misc

a4761e8

Fix the build erros

2cf273d

Fix check-types

295642f

Address comments

13878fa

Fix the build erros

bddcf2b

Extend spec runner to be more flexible

4285f75

Add missing state attributes for electra

8bcc864

Fix ss data types for electra spec

6610dc1

Make the spec runner more flexible

3d88a33

Fix the bug in process attestation

8fc72da

Update the sepc test version

8be57b1

ensi321 force-pushed the nh/5749-spec-tests branch from 0ff25d9 to 8be57b1 Compare May 8, 2024 13:28

Fix rebase issue

323e74c

ensi321 reviewed May 8, 2024

View reviewed changes

nazarhussain added 2 commits May 8, 2024 16:09

Merge branch 'nh/5749-spec-tests' of github.com:ChainSafe/lodestar in…

40b7eb7

…to nh/5749-spec-tests

Update committee index count check

3c6e2e6

nazarhussain requested review from ensi321 and wemeetagain May 8, 2024 14:17

ensi321 reviewed May 8, 2024

View reviewed changes

ensi321 approved these changes May 8, 2024

View reviewed changes

Merge branch 'electra-fork' into nh/5749-spec-tests

ff0e397

ensi321 merged commit 1945fd2 into electra-fork May 8, 2024
6 checks passed

ensi321 deleted the nh/5749-spec-tests branch May 8, 2024 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: enable spec tests related to eip-7549 #6741

test: enable spec tests related to eip-7549 #6741

nazarhussain commented May 7, 2024 •

edited by ensi321

wemeetagain May 7, 2024

nazarhussain May 8, 2024

ensi321 May 8, 2024 •

edited

ensi321 May 8, 2024 •

edited

ensi321 May 8, 2024 •

edited

ensi321 May 8, 2024 •

edited

nazarhussain commented May 8, 2024 •

edited

ensi321 left a comment

ensi321 May 8, 2024

nazarhussain May 8, 2024 •

edited

ensi321 left a comment

test: enable spec tests related to eip-7549 #6741

test: enable spec tests related to eip-7549 #6741

Conversation

nazarhussain commented May 7, 2024 • edited by ensi321

wemeetagain May 7, 2024

Choose a reason for hiding this comment

nazarhussain May 8, 2024

Choose a reason for hiding this comment

ensi321 May 8, 2024 • edited

Choose a reason for hiding this comment

ensi321 May 8, 2024 • edited

Choose a reason for hiding this comment

ensi321 May 8, 2024 • edited

Choose a reason for hiding this comment

ensi321 May 8, 2024 • edited

Choose a reason for hiding this comment

nazarhussain commented May 8, 2024 • edited

ensi321 left a comment

Choose a reason for hiding this comment

ensi321 May 8, 2024

Choose a reason for hiding this comment

nazarhussain May 8, 2024 • edited

Choose a reason for hiding this comment

ensi321 left a comment

Choose a reason for hiding this comment

nazarhussain commented May 7, 2024 •

edited by ensi321

ensi321 May 8, 2024 •

edited

ensi321 May 8, 2024 •

edited

ensi321 May 8, 2024 •

edited

ensi321 May 8, 2024 •

edited

nazarhussain commented May 8, 2024 •

edited

nazarhussain May 8, 2024 •

edited