Fork choice compliance test integration, "Production" part #9790

ericsson49 · 2025-08-21T13:36:41Z

PR Description

This PR includes three commits from the draft #8419 PR, which affect code run in production.

Documentation

I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

I thought about adding a changelog entry, and added one if I deemed necessary.

github-actions · 2025-08-21T13:36:50Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

ericsson49 · 2025-08-21T13:37:03Z

@lucassaldanha @mkalinin

ericsson49 · 2025-08-21T13:46:18Z

I have read the CLA Document and I hereby sign the CLA

mkalinin · 2025-08-21T14:22:34Z

I have read the CLA Document and I hereby sign the CLA

…erSlashing

This flag allows to run spec tests with BLS disabled flag. The flat is particularly utilized in the following places: * AbstractBlockProcessor.processAndValidateBlock when processing a block * ForkChoiceUtil.validate when validating an attestation * BeaconStateAccessorsAltair.getNextSyncCommitteeIndices when computing a sync committee aggregate pubkey The decision to put it this way is mainly because of a lot of boilerplate code that would be required to propagate signature verifier to the downstream method calls. Instead of propagating it is read from the spec config instance available at hand. TestDefinition exposes an interface to create a spec with blsDisabled flag.

This method returns viable fork choice heads as per the spec. For instance, getChainHeads(/*includeNonViableHeads=*/ false) would return heads that aren't necesserily justified root descendants which isn't spec compliant.

cursor · 2025-08-22T07:41:59Z

eth-tests/src/main/java/tech/pegasys/teku/ethtests/finder/TestDefinition.java

+  public Spec getSpec(final Boolean blsDisabled) {
    if (spec == null) {
-      createSpec();
+      createSpec(blsDisabled);


Bug: Test Caching Ignores BLS Configuration

The TestDefinition class caches the spec field, but the caching logic doesn't account for the blsDisabled parameter. This means different blsDisabled configurations can return the same cached spec instance, potentially causing tests to run with unintended BLS settings.

This caching mismatch seems to be a potential cause of problems. I am not sure of the impact but maybe we need to be mindful of caching two versions of the spec (with and without blsDisabled) and returning the appropriate one?

lucassaldanha

Overall changes are ok. Just added a few comments for discussion. I'll also ask others in the team to take a look.

lucassaldanha · 2025-08-25T05:04:10Z

ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigPhase0.java

      final Bytes4 fuluForkVersion,
-      final UInt64 fuluForkEpoch) {
+      final UInt64 fuluForkEpoch,
+      final boolean blsDisabled) {


Maybe we can have a constructor without the blsDisabled parameter, and one constructor overload with the parameter and annotated with @VisibleForTests or something to make it clear that it is only used on tests?

This way we also don't need to change existing calls to the constructor.

We can also add a unit test to check that the default constructor (without the paramater) will always create a spec with blsDisabled = false.

lucassaldanha · 2025-08-25T05:07:01Z

ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfig.java

  int getReorgParentWeightThreshold();

+  // Handle spec tests with BLS disabled
+  boolean isBlsDisabled();


NIT: Can the double-negative be misleading? Usually we default to isSomethingEnabled. Not sure what is the answer in this case, just thought I'd raise the question :)

I've got a fairly strong dislike of this kind of code where theres 'test only' stuff in production code. i think i'd make a test class that disables bls in preference to changing a production class to not use BLS.

should be largely addressed by #9805 if looking for the pr i merged

lucassaldanha · 2025-08-25T05:09:00Z

ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfig.java


  int getReorgParentWeightThreshold();

+  // Handle spec tests with BLS disabled


I think we need to make it more explicit that this is only used when testing.

rolfyone · 2025-08-25T06:01:19Z

...in/java/tech/pegasys/teku/spec/logic/versions/altair/helpers/BeaconStateAccessorsAltair.java

+    // Copy the previous aggregatePubkey if BLS is disabled
+    if (altairConfig.isBlsDisabled()) {
+      aggregatePubkey =
+          BeaconStateAltair.required(state)
+              .getNextSyncCommittee()
+              .getAggregatePubkey()
+              .getBLSPublicKey();


do we really need special logic here?

The problem is that aggregatePubkey should be computed regardless of whether BLS disabled or not. Probably, we should define a SignatureVerifier that does fake signature verification only but aggregates pubkeys as usual

rolfyone · 2025-08-26T20:35:34Z

Hey, I updated SpecConfig to be the place that signature verifier is coming from and the associated tests which should be a better way of doing a chunk of this #9805

Will need to think about BeaconStateAccessorsAltair. Did we want to rebase this PR? (cc @mkalinin)

mkalinin · 2025-08-27T03:49:00Z

Hey, I updated SpecConfig to be the place that signature verifier is coming from and the associated tests which should be a better way of doing a chunk of this #9805

Will need to think about BeaconStateAccessorsAltair. Did we want to rebase this PR? (cc @mkalinin)

Thanks a lot @rolfyone! let us rebase this one

github-actions bot added a commit that referenced this pull request Aug 21, 2025

@mkalinin has signed the CLA in #9790

2152518

lucassaldanha self-requested a review August 21, 2025 22:25

mkalinin added 3 commits August 22, 2025 11:38

Pass signatureVerifier from processAndValidateBlock to validateAttest…

c3ced5d

…erSlashing

Implement getViableChainHeads method

150e262

This method returns viable fork choice heads as per the spec. For instance, getChainHeads(/*includeNonViableHeads=*/ false) would return heads that aren't necesserily justified root descendants which isn't spec compliant.

ericsson49 force-pushed the fc-integration-pr1 branch from 3f4ac2a to 150e262 Compare August 22, 2025 07:38

cursor bot reviewed Aug 22, 2025

View reviewed changes

lucassaldanha reviewed Aug 25, 2025

View reviewed changes

rolfyone reviewed Aug 25, 2025

View reviewed changes

ericsson49 closed this Oct 17, 2025

github-actions bot locked and limited conversation to collaborators Oct 17, 2025


		int getReorgParentWeightThreshold();

		// Handle spec tests with BLS disabled

Fork choice compliance test integration, "Production" part #9790

Fork choice compliance test integration, "Production" part #9790

Uh oh!

Conversation

ericsson49 commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

Documentation

Changelog

Uh oh!

github-actions bot commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericsson49 commented Aug 21, 2025

Uh oh!

ericsson49 commented Aug 21, 2025

Uh oh!

mkalinin commented Aug 21, 2025

Uh oh!

cursor bot Aug 22, 2025

Choose a reason for hiding this comment

Bug: Test Caching Ignores BLS Configuration

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucassaldanha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rolfyone commented Aug 26, 2025

Uh oh!

mkalinin commented Aug 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ericsson49 commented Aug 21, 2025 •

edited

Loading

github-actions bot commented Aug 21, 2025 •

edited

Loading