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

Jump to bottom

Integrate fork choice compliance test suite #8419

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Draft

Integrate fork choice compliance test suite #8419

wants to merge 11 commits into from

Conversation

mkalinin
Copy link

@mkalinin mkalinin commented Jul 3, 2024

PR Description

Adds support of the fork choice compliance test suite, the source of which can be found in the following spec PR:

Running tests

To run tests one need to download one of the test corpus (differentiated by a number of tests):

Extract with command:

tar xvf tst_standard.tar

And use ManualReferenceTestRunner with -Dteku.ref-test-module.override-root=~/Downloads/tst_standard/ to specify the directory with the test vectors.

It is required to expand the consensus-spec-tests suites to build the module where ManualReferenceTestRunner is located so running the new tests this way looks more like a temporary solution and potentially a separate test runner is required.

Implementation detail

The new test suite is based on the fork choice test suite and extends the fork choice test format with only one check, namely viable_for_head_roots_and_weights, so this PR leverages on the existing ForkChoiceTestExecutor implementation and extend it to adopt the suite. However, existing ForkChoiceTestExecutor implementation and the suite design details required a more extensive changes to the codebase, the following list might not be exhaustive:

  • Read blsSetting from meta.yaml and disable BLS for block and attestation processing if that is required by the setting, likewise, do fake pubkey aggregation computation in the sync committee updates during epoch transition. For performance reasons the new suite generates tests with fake BLS that should run with BLS disabled. The support is added via provisioning SpecConfig with the BLS setting, this is done to avoid a ton of boilerplate that would be required in the next sync committee update case.
  • Add an option to discard deferred attestations and turn this option on in the fork choice test runner. The new suite shuffles messages and expects that if the attestation comes earlier it is deemed invalid and ins’t applied in the future. We were debating internally on how to properly design this particular part, and come to the conclusion that for simplicity we don’t expect anything is deferred. But this part of the suite can be revisited after feedback from client developers gets collected.
  • Attestations can be invalid hence valid flag support for attestations is added.
  • Support viable_for_head_roots_and_weights check — this entailed implementation of getViableChainHeads method.

Known issues

A few tests (around 10) from the tst_standard corpus fail due to the following reasons:

  • Teku does not process attestations voting for a non-viable head, results into an UNKNOWN_BLOCK attestation status
  • Teku does not process attestation if it attests to a block that is older than two epochs with the following message in the exception: Committee information must be derived from a state no older than the previous epoch

The above looks like fair optimisation measure protecting from spamming nodes with the old attestations that have almost no value.

A couple of tests from the tst_standard corpus fail due to the following spec issue:

  • If attestation from the current epoch is included in the block which parent is two epochs old then due to different shuffling seed aggregation bits of these attestations would be resolved differently depending on the state that is used. Teku caches validator indices from the block processing and uses them when attestations are applied to the fork choice which is a fair optimisation. While the spec resolves those indices based on the state to which attestations were attesting to. Anyway, when spec processes such an attestation it uses wrong indices for activity and rewards mechanism.

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.

@mkalinin
Pass signatureVerifier from processAndValidateBlock to validateAttest…
32802b4 
…erSlashing
@mkalinin
Implement BLS disabled flag for the SpecConfig
f06d43a 
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.
@mkalinin
Implement getViableChainHeads method
3e76607 
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.
@mkalinin
Add flag to discard deferred attestations
2c63a67 
Some spec tests does not expect early attestations to be deferred
and applied to the fork chocie in the future slot,
a flag allows to handle such cases.
@mkalinin
Implement fork_choice_generated spec test suite
70b110c 
This test suite extends the fork_choice one. Thus, the implementation
leverages on the existing ForkChoiceTestExecutor and extends it with the following:
* Add fork_choice_generated suite tests
* Load meta.yaml to read bls setting and pass the setting to the test spec builder
* Discard deferred attestations when calling onTick as the test generator does not defer them
* Read "valid" flag for attestation and check the status of attestation import accordingly
* Implement "viable_for_head_roots_and_weights" check introduced by the fork_choice_generated suite
@CLAassistant
Copy link

CLAassistant commented Jul 3, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@lucassaldanha lucassaldanha added the DO NOT MERGE Not ready to merge label Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
DO NOT MERGE Not ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mkalinin @CLAassistant @lucassaldanha