Avoid combining validators by aggregating committee when using DVT #9357
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Validated the fix using charon via kurtosis setup. Everything seems to be stable now. |
rolfyone
reviewed
Apr 16, 2025
| @@ -290,6 +293,83 @@ public void shouldProduceSingleAggregateAndProofWhenMultipleValidatorsAggregateS | |||
| .record(any(), any(AggregationDuty.class), eq(ValidatorDutyMetricsSteps.SIGN)); | |||
| } | |||
|
|
|||
| @TestTemplate | |||
| public void | |||
| shouldProduceAllAggregateAndProofsWhenValidatorsAggregateSameCommitteeAndUsingUngroupedAggregators() { | |||
There was a problem hiding this comment.
this is probably at the point where a comment would be better than a ridiculous name...
rolfyone
approved these changes
Apr 16, 2025
7 tasks
lucassaldanha
added a commit
that referenced
this pull request
May 12, 2025
* Add read functionality to blobs archiving (#9318) * Schedule Gnosis Pectra hard-fork (#9340) * Updated ref tests v1.5.0-beta.5 (#9341) * Created Fulu skeleton (#9325) * Made checkpoint-state-url and initial-state mutually exclusive (#9342) It's hard to reason on how these two should interact, and logically they're mutually exclusive, so this just solidifies the theory that they shouldn't be used together. Signed-off-by: Paul Harris <[email protected]> * partial 3rd party updates and errorprone updates (#9351) I ended up disabling the instanceof errorprone, as it's very very common for us to be doing it apparently. I started changing things but in the interest of time will raise a ticket Signed-off-by: Paul Harris <[email protected]> * Make builder timeouts only for the HTTP call (#9353) Co-authored-by: Lucas Saldanha <[email protected]> * more 3rd party updates (#9355) Signed-off-by: Paul Harris <[email protected]> * KZG updates from das branch (#9335) * Avoid combining validators by aggregating committee when using DVT (#9357) * builder json format, make media type more compatible (#9360) * refactored expected withdrawals and added test cases (#9361) Signed-off-by: Paul Harris <[email protected]> * Use `URI.create(...).toURL()` removing URL constructor deprecation (#9364) * clear-changelog (#9366) * New infra stream module (#9362) * feat: Add support for reading static peers from file (#9328) Co-authored-by: Paul Harris <[email protected]> * Start cleaning up `api/schema` (#9376) * Start cleaning up `api/schema` - moved `ValidatorStatus` - Removed `Version` - Moved `PublicKeyException` - Moved `EventType` Signed-off-by: Paul Harris <[email protected]> * cleanup the rest of api/schema (#9377) Signed-off-by: Paul Harris <[email protected]> * Added KZG computeCells method (#9375) * Added KZG computeCells method * Moved KZG no-operation logic into testFixture class * Fulu Schemas (ssz + datastructures) - including fulu ssz ref tests (#9363) * Added context to the sync committee duties error (#9380) Signed-off-by: Paul Harris <[email protected]> * Returning custody_group_count on node/identity Beacon API (#9381) * may 3rd party updates (#9388) Signed-off-by: Paul Harris <[email protected]> * Added snakeyaml to allow more complicated config reading (#9390) - added tests to show reading lists of objects (BPO related) - added a testcase to show that hex is read correctly - added testcase to show specConfigReader isnt filling defaults when we read local config files Partially addresses #9365 Signed-off-by: Paul Harris <[email protected]> * Add FULU to the Spec Factory (#9373) * Improve attestation bits aggregator electra (#9393) * added a DeserializableConfigTypeDefinition (#9396) - Tests demonstrate that this should at least be very close to what we require for reading BPO schedule. Partially addresses #9365 Signed-off-by: Paul Harris <[email protected]> * Updating ref test to 1.5.0 stable (#9398) * Adding DataColumnsByRootIdentifier container * PR comments * need more coffee... * uniforms the AggregatingAttestationPool interface (#9401) * Avoids potential mutability of the result of getCommitteeBits (#9402) --------- Signed-off-by: Paul Harris <[email protected]> Co-authored-by: Stefan Bratanov <[email protected]> Co-authored-by: Lucas Saldanha <[email protected]> Co-authored-by: Paul Harris <[email protected]> Co-authored-by: Enrico Del Fante <[email protected]> Co-authored-by: crStiv <[email protected]> Co-authored-by: Mehdi AOUADI <[email protected]> Co-authored-by: Lucas Saldanha <[email protected]>
zilm13
added a commit
that referenced
this pull request
May 20, 2025
* Add read functionality to blobs archiving (#9318) * Schedule Gnosis Pectra hard-fork (#9340) * Updated ref tests v1.5.0-beta.5 (#9341) * Created Fulu skeleton (#9325) * Made checkpoint-state-url and initial-state mutually exclusive (#9342) It's hard to reason on how these two should interact, and logically they're mutually exclusive, so this just solidifies the theory that they shouldn't be used together. Signed-off-by: Paul Harris <[email protected]> * partial 3rd party updates and errorprone updates (#9351) I ended up disabling the instanceof errorprone, as it's very very common for us to be doing it apparently. I started changing things but in the interest of time will raise a ticket Signed-off-by: Paul Harris <[email protected]> * Make builder timeouts only for the HTTP call (#9353) Co-authored-by: Lucas Saldanha <[email protected]> * more 3rd party updates (#9355) Signed-off-by: Paul Harris <[email protected]> * KZG updates from das branch (#9335) * Avoid combining validators by aggregating committee when using DVT (#9357) * builder json format, make media type more compatible (#9360) * refactored expected withdrawals and added test cases (#9361) Signed-off-by: Paul Harris <[email protected]> * Use `URI.create(...).toURL()` removing URL constructor deprecation (#9364) * clear-changelog (#9366) * New infra stream module (#9362) * feat: Add support for reading static peers from file (#9328) Co-authored-by: Paul Harris <[email protected]> * Start cleaning up `api/schema` (#9376) * Start cleaning up `api/schema` - moved `ValidatorStatus` - Removed `Version` - Moved `PublicKeyException` - Moved `EventType` Signed-off-by: Paul Harris <[email protected]> * cleanup the rest of api/schema (#9377) Signed-off-by: Paul Harris <[email protected]> * Added KZG computeCells method (#9375) * Added KZG computeCells method * Moved KZG no-operation logic into testFixture class * Fulu Schemas (ssz + datastructures) - including fulu ssz ref tests (#9363) * Added context to the sync committee duties error (#9380) Signed-off-by: Paul Harris <[email protected]> * Returning custody_group_count on node/identity Beacon API (#9381) * may 3rd party updates (#9388) Signed-off-by: Paul Harris <[email protected]> * Added snakeyaml to allow more complicated config reading (#9390) - added tests to show reading lists of objects (BPO related) - added a testcase to show that hex is read correctly - added testcase to show specConfigReader isnt filling defaults when we read local config files Partially addresses #9365 Signed-off-by: Paul Harris <[email protected]> * Add FULU to the Spec Factory (#9373) * Improve attestation bits aggregator electra (#9393) * added a DeserializableConfigTypeDefinition (#9396) - Tests demonstrate that this should at least be very close to what we require for reading BPO schedule. Partially addresses #9365 Signed-off-by: Paul Harris <[email protected]> * Updating ref test to 1.5.0 stable (#9398) * uniforms the AggregatingAttestationPool interface (#9401) * Avoids potential mutability of the result of getCommitteeBits (#9402) * Adding DataColumnsByRootIdentifier container (#9399) * Added an info message for the highest milestone (#9405) Also added a sanity check that will fail if the specFactory hasn't defined the highest milestone correctly, which has flow on effects. fixes #9400 Signed-off-by: Paul Harris <[email protected]> * Introduce `PooledAttestation` and `PooledAttestationWithData` (#9407) * Improve jdk 24 support and add docker-jdk24 (#9410) * add docker-jdk24 support * solves jdk 24 startup warning * rename AttestationBitsAggregator to AttestationBits (#9412) * Introduce aggregating pool interface (#9414) * [docker-jdk24] SIGHUP handler fix (#9413) * Optionally include validator indices in `PooledAttestation` (#9418) * Improve committeesSize retrieval in `AggregatingAttestationPool` (#9419) * Batch attestation duty scheduling for an epoch (#9374) * Updated MiscHelpersFulu + Added Gossip Logger (#9422) * Updated MiscHelpersFulu * Added GossipLogger and DAS related changes * gossip DAS related changes * fix test * fix runtime with noop fulu managers * bump MAX_SUBSCRIBED_TOPICS for Fulu and further forks * Use master's circle ci config * Remove duplicated workflows * Update test fixtures with random order changes * fix test * fix test * fix GetNewBlockV3Test * fix GetBlockTest * Gossip DAS related changes (#9423) * added bpo parsing to configuration (#9406) Signed-off-by: Paul Harris <[email protected]> * Fix ProduceBlockRequestTest for Fulu and updated order in random * Regenerate test fixtures with the changes in DataStructureUtil call order * Regenerate test fixtures with the changes in DataStructureUtil call order for GetNewBlockV3IntegrationTest * spotless * move GetDataColumnSidecars to tekuv1 space, fix openapi integration test * BPO - remove max blobs from fulu spec (#9427) Signed-off-by: Paul Harris <[email protected]> * Introduces `RewardBasedAttestationSorter` (#9428) * Fix integration tests after master upstream * Fix property tests + refactor some random datastructure utils to avoid fulu duplication * fix assemble * fix test * revert das test coverage changes * DAS storage changes (#9432) * Added config defaulting to the Config loader (#9439) Signed-off-by: Paul Harris <[email protected]> Co-authored-by: Gabriel Fukushima <[email protected]> * Introduce `AttestationMatchingGroupV2` (#9438) * Fix DasSyncAcceptanceTest, fix DSL, remove broken DSL --------- Signed-off-by: Paul Harris <[email protected]> Co-authored-by: Stefan Bratanov <[email protected]> Co-authored-by: Lucas Saldanha <[email protected]> Co-authored-by: Paul Harris <[email protected]> Co-authored-by: Enrico Del Fante <[email protected]> Co-authored-by: crStiv <[email protected]> Co-authored-by: Mehdi AOUADI <[email protected]> Co-authored-by: Lucas Saldanha <[email protected]> Co-authored-by: Gabriel Fukushima <[email protected]>
zilm13
added a commit
that referenced
this pull request
May 21, 2025
* Add read functionality to blobs archiving (#9318) * Schedule Gnosis Pectra hard-fork (#9340) * Updated ref tests v1.5.0-beta.5 (#9341) * Created Fulu skeleton (#9325) * Made checkpoint-state-url and initial-state mutually exclusive (#9342) It's hard to reason on how these two should interact, and logically they're mutually exclusive, so this just solidifies the theory that they shouldn't be used together. Signed-off-by: Paul Harris <[email protected]> * partial 3rd party updates and errorprone updates (#9351) I ended up disabling the instanceof errorprone, as it's very very common for us to be doing it apparently. I started changing things but in the interest of time will raise a ticket Signed-off-by: Paul Harris <[email protected]> * Make builder timeouts only for the HTTP call (#9353) Co-authored-by: Lucas Saldanha <[email protected]> * more 3rd party updates (#9355) Signed-off-by: Paul Harris <[email protected]> * KZG updates from das branch (#9335) * Avoid combining validators by aggregating committee when using DVT (#9357) * builder json format, make media type more compatible (#9360) * refactored expected withdrawals and added test cases (#9361) Signed-off-by: Paul Harris <[email protected]> * Use `URI.create(...).toURL()` removing URL constructor deprecation (#9364) * clear-changelog (#9366) * New infra stream module (#9362) * feat: Add support for reading static peers from file (#9328) Co-authored-by: Paul Harris <[email protected]> * Start cleaning up `api/schema` (#9376) * Start cleaning up `api/schema` - moved `ValidatorStatus` - Removed `Version` - Moved `PublicKeyException` - Moved `EventType` Signed-off-by: Paul Harris <[email protected]> * cleanup the rest of api/schema (#9377) Signed-off-by: Paul Harris <[email protected]> * Added KZG computeCells method (#9375) * Added KZG computeCells method * Moved KZG no-operation logic into testFixture class * Fulu Schemas (ssz + datastructures) - including fulu ssz ref tests (#9363) * Added context to the sync committee duties error (#9380) Signed-off-by: Paul Harris <[email protected]> * Returning custody_group_count on node/identity Beacon API (#9381) * may 3rd party updates (#9388) Signed-off-by: Paul Harris <[email protected]> * Added snakeyaml to allow more complicated config reading (#9390) - added tests to show reading lists of objects (BPO related) - added a testcase to show that hex is read correctly - added testcase to show specConfigReader isnt filling defaults when we read local config files Partially addresses #9365 Signed-off-by: Paul Harris <[email protected]> * Add FULU to the Spec Factory (#9373) * Improve attestation bits aggregator electra (#9393) * added a DeserializableConfigTypeDefinition (#9396) - Tests demonstrate that this should at least be very close to what we require for reading BPO schedule. Partially addresses #9365 Signed-off-by: Paul Harris <[email protected]> * Updating ref test to 1.5.0 stable (#9398) * uniforms the AggregatingAttestationPool interface (#9401) * Avoids potential mutability of the result of getCommitteeBits (#9402) * Adding DataColumnsByRootIdentifier container (#9399) * Added an info message for the highest milestone (#9405) Also added a sanity check that will fail if the specFactory hasn't defined the highest milestone correctly, which has flow on effects. fixes #9400 Signed-off-by: Paul Harris <[email protected]> * Introduce `PooledAttestation` and `PooledAttestationWithData` (#9407) * Improve jdk 24 support and add docker-jdk24 (#9410) * add docker-jdk24 support * solves jdk 24 startup warning * rename AttestationBitsAggregator to AttestationBits (#9412) * Introduce aggregating pool interface (#9414) * [docker-jdk24] SIGHUP handler fix (#9413) * Optionally include validator indices in `PooledAttestation` (#9418) * Improve committeesSize retrieval in `AggregatingAttestationPool` (#9419) * Batch attestation duty scheduling for an epoch (#9374) * Updated MiscHelpersFulu + Added Gossip Logger (#9422) * Updated MiscHelpersFulu * Added GossipLogger and DAS related changes * Gossip DAS related changes (#9423) * added bpo parsing to configuration (#9406) Signed-off-by: Paul Harris <[email protected]> * BPO - remove max blobs from fulu spec (#9427) Signed-off-by: Paul Harris <[email protected]> * Introduces `RewardBasedAttestationSorter` (#9428) * DAS storage changes (#9432) * Added config defaulting to the Config loader (#9439) Signed-off-by: Paul Harris <[email protected]> Co-authored-by: Gabriel Fukushima <[email protected]> * Introduce `AttestationMatchingGroupV2` (#9438) * Introduce `AggregatingAttestationPoolV2` (#9445) * Introduce `AggregatingAttestationPoolProfiler` implementations (#9447) Co-authored-by: Paul Harris <[email protected]> * PeerDAS RPC related changes (#9444) * Added a development flag to enable strict config loading (#9449) Strict loading was the default until yesterday, but it's caused a number of problems because if we push changes too fast etc it causes support issues. The new default permissive loading would mean that if a passed config file is missing an entry, we default it and print a warning. This new flag `--Xstartup-strict-config-loader-enabled` defaults to false, but if turned on will revert to failing on startup if a config being loaded doesn't include all of the expected configuration attributes. We could potentially start using this in AT to specify minimal changes to a configuration for a test, but for now I've left acceptance tests using strict loading. Signed-off-by: Paul Harris <[email protected]> * Updating reference tests to 1.6.0-alpha.0 (#9450) * Updating reference tests to 1.6.0-alpha.0 * Added Fulu networking tests * rename * Refactoring networking ATs * Add peer via api (#9431) * add initial impl of endpoint Signed-off-by: Gabriel Fukushima <[email protected]> * add catch to cover address conversion failure Signed-off-by: Gabriel Fukushima <[email protected]> * add tests for 200, 400 and 500 Signed-off-by: Gabriel Fukushima <[email protected]> * spotless Signed-off-by: Gabriel Fukushima <[email protected]> * add test for when discovery is not enabled Signed-off-by: Gabriel Fukushima <[email protected]> * spotless Signed-off-by: Gabriel Fukushima <[email protected]> * fix name of test after changing exception returned for invalid peer Signed-off-by: Gabriel Fukushima <[email protected]> * add json def to paths Signed-off-by: Gabriel Fukushima <[email protected]> * add changelog Signed-off-by: Gabriel Fukushima <[email protected]> * PR review Signed-off-by: Gabriel Fukushima <[email protected]> * spotless caught me again Signed-off-by: Gabriel Fukushima <[email protected]> * change impl and tests to handle a single peer per api call Signed-off-by: Gabriel Fukushima <[email protected]> * spotless Signed-off-by: Gabriel Fukushima <[email protected]> --------- Signed-off-by: Gabriel Fukushima <[email protected]> * `AggregatingAttestationPool` new CLI params (#9452) * poolV2-CLI * fix arity * getBits method names clearer (#9453) * Validate electra attestationData index received by BN (#9430) * fix integration test --------- Signed-off-by: Paul Harris <[email protected]> Signed-off-by: Gabriel Fukushima <[email protected]> Co-authored-by: Stefan Bratanov <[email protected]> Co-authored-by: Lucas Saldanha <[email protected]> Co-authored-by: Paul Harris <[email protected]> Co-authored-by: Enrico Del Fante <[email protected]> Co-authored-by: crStiv <[email protected]> Co-authored-by: Mehdi AOUADI <[email protected]> Co-authored-by: Gabriel Fukushima <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Description
By default, when we have multiple validators aggregating on the same committee, Teku VC optimizes this by only creating a single
AggregateAndProofthat is sent to the Beacon Node. However, when using DVT, it is important that each validator submits their ownAggregateAndProof, regardless if they are to the exact same slot/committee.In this change,
AggregationDutyhas been updated with an abstraction for the aggregator set calledAggregationDutyAggregators. This interface hides the underlying logic of combining validators in the same committee or not from theAggregationDutylogic.Fixed Issue(s)
fixes #9347
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog