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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
6ddf100
initial commit
ensi321 Apr 16, 2024
14e6fd8
Update gossip validation
ensi321 Apr 24, 2024
32e8ae0
Update attestation gossip validation
ensi321 Apr 25, 2024
f491628
aggregateAndProof validation
ensi321 Apr 26, 2024
39b259e
Extend spec runner to be more flexible
nazarhussain May 7, 2024
2ba9a22
Add missing state attributes for electra
nazarhussain May 7, 2024
8061a4d
Fix ss data types for electra spec
nazarhussain May 7, 2024
c509eb0
Make the spec runner more flexible
nazarhussain May 7, 2024
c7f4b26
Fix the bug in process attestation
nazarhussain May 7, 2024
0ff25d9
Update the sepc test version
nazarhussain May 8, 2024
cd74406
clean up
ensi321 Apr 26, 2024
a4761e8
Misc
ensi321 Apr 29, 2024
2cf273d
Fix the build erros
nazarhussain May 6, 2024
316a500
feat: get attestations for electra block (#6732)
twoeths May 7, 2024
295642f
Fix check-types
ensi321 May 5, 2024
13878fa
Address comments
ensi321 May 7, 2024
bddcf2b
Fix the build erros
nazarhussain May 6, 2024
4285f75
Extend spec runner to be more flexible
nazarhussain May 7, 2024
8bcc864
Add missing state attributes for electra
nazarhussain May 7, 2024
6610dc1
Fix ss data types for electra spec
nazarhussain May 7, 2024
3d88a33
Make the spec runner more flexible
nazarhussain May 7, 2024
8fc72da
Fix the bug in process attestation
nazarhussain May 7, 2024
8be57b1
Update the sepc test version
nazarhussain May 8, 2024
323e74c
Fix rebase issue
ensi321 May 8, 2024
40b7eb7
Merge branch 'nh/5749-spec-tests' of github.com:ChainSafe/lodestar in…
nazarhussain May 8, 2024
3c6e2e6
Update committee index count check
nazarhussain May 8, 2024
ff0e397
Merge branch 'electra-fork' into nh/5749-spec-tests
ensi321 May 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/beacon-node/test/spec/presets/operations.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,8 @@ const operations: TestRunnerFn<OperationsTestCase, BeaconStateAllForks> = (fork,
sszTypes: {
pre: ssz[fork].BeaconState,
post: ssz[fork].BeaconState,
attestation: ssz.phase0.Attestation,
attester_slashing: ssz.phase0.AttesterSlashing,
attestation: fork === ForkName.electra ? ssz.electra.Attestation : ssz.phase0.Attestation,
attester_slashing: fork === ForkName.electra ? ssz.electra.AttesterSlashing : ssz.phase0.AttesterSlashing,
block: ssz[fork].BeaconBlock,
body: ssz[fork].BeaconBlockBody,
deposit: ssz.phase0.Deposit,
Expand Down
2 changes: 1 addition & 1 deletion packages/beacon-node/test/spec/specTestVersioning.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {DownloadTestsOptions} from "@lodestar/spec-test-util/downloadTests";
const __dirname = path.dirname(fileURLToPath(import.meta.url));

export const ethereumConsensusSpecsTests: DownloadTestsOptions = {
specVersion: "v1.5.0-alpha.1",
specVersion: "v1.5.0-alpha.2",
// Target directory is the host package root: 'packages/*/spec-tests'
outputDir: path.join(__dirname, "../../spec-tests"),
specTestsRepoUrl: "https://github.com/ethereum/consensus-spec-tests",
Expand Down
27 changes: 19 additions & 8 deletions packages/beacon-node/test/spec/utils/specTestIterator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ const ARTIFACT_FILENAMES = new Set([
]);

export interface SkipOpts {
skippedPrefixes?: string[];
skippedTestSuites?: RegExp[];
skippedTests?: RegExp[];
skippedForks?: string[];
skippedRunners?: string[];
skippedHandlers?: string[];
Expand Down Expand Up @@ -57,14 +58,17 @@ const coveredTestRunners = [
// ],
// ```
export const defaultSkipOpts: SkipOpts = {
skippedForks: ["electra", "eip7594"],
skippedForks: ["eip7594"],
// TODO: capella
// BeaconBlockBody proof in lightclient is the new addition in v1.3.0-rc.2-hotfix
// Skip them for now to enable subsequently
skippedPrefixes: [
"capella/light_client/single_merkle_proof/BeaconBlockBody",
"deneb/light_client/single_merkle_proof/BeaconBlockBody",
skippedTestSuites: [
/^capella\/light_client\/single_merkle_proof\/BeaconBlockBody.*/,
/^deneb\/light_client\/single_merkle_proof\/BeaconBlockBody.*/,
// /^electra\/(?!operations\/attestations)(?!operations\/attester_slashing)/,
ensi321 marked this conversation as resolved.
Show resolved Hide resolved
/^electra\/(?!operations\/attestation)/,
],
skippedTests: [],
skippedRunners: ["merkle_proof", "networking"],
};

Expand Down Expand Up @@ -100,7 +104,10 @@ export function specTestIterator(
opts: SkipOpts = defaultSkipOpts
): void {
for (const forkStr of readdirSyncSpec(configDirpath)) {
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 •
edited
Loading

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

) {
continue;
}
const fork = forkStr as ForkName;
Expand Down Expand Up @@ -134,7 +141,7 @@ export function specTestIterator(
for (const testSuite of readdirSyncSpec(testHandlerDirpath)) {
const testId = `${fork}/${testRunnerName}/${testHandler}/${testSuite}`;

if (opts?.skippedPrefixes?.some((skippedPrefix) => testId.startsWith(skippedPrefix))) {
if (opts?.skippedTestSuites?.some((skippedMatch) => testId.match(skippedMatch))) {
displaySkipTest(testId);
} else if (fork === undefined) {
displayFailTest(testId, `Unknown fork ${forkStr}`);
Expand All @@ -150,7 +157,11 @@ export function specTestIterator(
// Generic testRunner
else {
const {testFunction, options} = testRunner.fn(fork, testHandler, testSuite);

if (opts.skippedTests && options.shouldSkip === undefined) {
options.shouldSkip = (_testCase: any, name: string, _index: number): boolean => {
return opts?.skippedTests?.some((skippedMatch) => name.match(skippedMatch)) ?? false;
};
}
describeDirectorySpecTest(testId, testSuiteDirpath, testFunction, options);
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/state-transition/src/block/processAttestationPhase0.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export function validateAttestation(
if (fork >= ForkSeq.electra) {
assert.equal(data.index, 0, `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 •
edited
Loading

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 •
edited
Loading

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


if (committeeBitsLength > committeeCount) {
throw new Error(
Expand Down
18 changes: 9 additions & 9 deletions packages/types/src/electra/sszTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ import {
MAX_WITHDRAWAL_REQUESTS_PER_PAYLOAD,
MAX_CONSOLIDATIONS,
PENDING_BALANCE_DEPOSITS_LIMIT,
PENDING_CONSOLIDATIONS_LIMIT,
PENDING_PARTIAL_WITHDRAWALS_LIMIT,
PENDING_CONSOLIDATIONS_LIMIT,
} from "@lodestar/params";
import {ssz as primitiveSsz} from "../primitive/index.js";
import {ssz as phase0Ssz} from "../phase0/index.js";
Expand Down Expand Up @@ -336,14 +336,14 @@ export const BeaconState = new ContainerType(
// Deep history valid from Capella onwards
historicalSummaries: capellaSsz.BeaconState.fields.historicalSummaries,
depositReceiptsStartIndex: UintBn64, // New in ELECTRA
depositBalanceToConsume: Gwei, // [New in Electra]
exitBalanceToConsume: Gwei, // [New in Electra]
earliestExitEpoch: Epoch, // [New in Electra]
consolidationBalanceToConsume: Gwei, // [New in Electra]
earliestConsolidationEpoch: Epoch, // [New in Electra]
pendingBalanceDeposits: new ListCompositeType(PendingBalanceDeposit, PENDING_BALANCE_DEPOSITS_LIMIT), // [New in Electra]
pendingPartialWithdrawals: new ListCompositeType(PartialWithdrawal, PENDING_PARTIAL_WITHDRAWALS_LIMIT), // [New in Electra]
pendingConsolidations: new ListCompositeType(PendingConsolidation, PENDING_CONSOLIDATIONS_LIMIT), // [New in Electra]
depositBalanceToConsume: Gwei, // New in Electra:EIP7251
exitBalanceToConsume: Gwei, // New in Electra:EIP7251
earliestExitEpoch: Epoch, // New in Electra:EIP7251
consolidationBalanceToConsume: Gwei, // New in Electra:EIP7251
earliestConsolidationEpoch: Epoch, // New in Electra:EIP7251
pendingBalanceDeposits: new ListCompositeType(PendingBalanceDeposit, Number(PENDING_BALANCE_DEPOSITS_LIMIT)), // new in electra:eip7251
pendingPartialWithdrawals: new ListCompositeType(PartialWithdrawal, Number(PENDING_PARTIAL_WITHDRAWALS_LIMIT)), // New in Electra:EIP7251
pendingConsolidations: new ListCompositeType(PendingConsolidation, Number(PENDING_CONSOLIDATIONS_LIMIT)), // new in electra:eip7251
},
{typeName: "BeaconState", jsonCase: "eth2"}
);
Expand Down
Loading