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

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

Merged
merged 27 commits into from
May 8, 2024
Merged

Conversation

nazarhussain
Copy link
Contributor

@nazarhussain nazarhussain commented May 7, 2024

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

@nazarhussain nazarhussain requested a review from a team as a code owner May 7, 2024 13:57
@@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

@g11tech is this correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ensi321 ensi321 May 8, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ensi321 ensi321 May 8, 2024

Choose a reason for hiding this comment

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

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

@nazarhussain nazarhussain changed the title Enable spec tests related to eip-7549 test: enable spec tests related to eip-7549 May 7, 2024
Base automatically changed from nc/7549 to electra-fork May 8, 2024 11:00
ensi321 and others added 12 commits May 8, 2024 16:23
* 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>
@@ -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;
Copy link
Contributor

@ensi321 ensi321 May 8, 2024

Choose a reason for hiding this comment

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

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.

@@ -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;
Copy link
Contributor

@ensi321 ensi321 May 8, 2024

Choose a reason for hiding this comment

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

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

@nazarhussain
Copy link
Contributor Author

nazarhussain commented May 8, 2024

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

Copy link
Contributor

@ensi321 ensi321 left a comment

Choose a reason for hiding this comment

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

looks good overall minor comment

if (opts?.skippedForks?.includes(forkStr)) {
if (
opts?.skippedForks?.includes(forkStr) ||
(process.env.SPEC_FILTER_FORK && forkStr !== process.env.SPEC_FILTER_FORK)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@nazarhussain nazarhussain May 8, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ensi321 ensi321 left a comment

Choose a reason for hiding this comment

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

lgtm

@ensi321 ensi321 merged commit 1945fd2 into electra-fork May 8, 2024
6 checks passed
@ensi321 ensi321 deleted the nh/5749-spec-tests branch May 8, 2024 15:14
g11tech pushed a commit that referenced this pull request May 24, 2024
* 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>
g11tech pushed a commit to g11tech/lodestar that referenced this pull request Jun 19, 2024
* 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 (ChainSafe#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>
g11tech pushed a commit that referenced this pull request Jun 25, 2024
* 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>
g11tech pushed a commit that referenced this pull request Jun 25, 2024
* 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>
g11tech pushed a commit that referenced this pull request Jul 1, 2024
* 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>
g11tech pushed a commit that referenced this pull request Jul 30, 2024
* 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>
g11tech pushed a commit that referenced this pull request Jul 31, 2024
* 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>
g11tech pushed a commit that referenced this pull request Aug 2, 2024
* 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>
g11tech pushed a commit that referenced this pull request Aug 9, 2024
* 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>
g11tech pushed a commit that referenced this pull request Aug 9, 2024
* 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>
g11tech pushed a commit that referenced this pull request Aug 23, 2024
* 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>
g11tech pushed a commit that referenced this pull request Aug 27, 2024
* 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>
philknows pushed a commit that referenced this pull request Sep 3, 2024
* 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>
@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