Skip to content

Conversation

@chong-he
Copy link
Member

@chong-he chong-he commented Feb 20, 2025

Issue Addressed

Proposed Changes

  • Add beacon_committee_selections endpoint
  • Test beacon committee aggregator and confirmed working
  • Add sync_committee_selections endpoint
  • Test sync committee aggregator and confirmed working

Additional Info

Thank you @michaelsproul and @macladson for the help and guidance

@chong-he chong-he added work-in-progress PR is a work-in-progress skip-ci Don't run the `test-suite` dvt Distributed validator technology e.g. SSV, Obol and removed skip-ci Don't run the `test-suite` labels Feb 20, 2025
@chong-he chong-he mentioned this pull request Feb 27, 2025
@chong-he chong-he changed the title Add selections API endpoints to support DVT middleware Implement selections Beacon API endpoints to support DVT middleware Mar 13, 2025
@chong-he
Copy link
Member Author

chong-he commented Mar 20, 2025

With the commit 8b2f058 where VC sends partial signatures in parallel for all validators for a slot, I believe the aggregated attestations are successful (during testing) now, thank you @michaelsproul !

Lighthouse VC logs:

{"log":"Mar 20 06:36:24.046 INFO Successfully published attestation      type: aggregated, slot: 37, committee_index: 0, head_block: 0xdb186315afecb3d1134d03eb2d5b04b83a21ede56be6563e3bff304b103bf4b6, signatures: 24, aggregator: 58, service: attestation\n","stream":"stderr","time":"2025-03-20T06:36:24.04690072Z"}

for every slot now

Charon logs:

{"log":"06:36:24.074 INFO bcast      Successfully submitted attestation aggregations to beacon node {\"delay\": \"8.074945477s\", \"duty\": \"37/aggregator\", \"peer\": \"twinkling-pen\", \"protocol\": \"/charon/parsigex/2.0.0\"}\n","stream":"stderr","time":"2025-03-20T06:36:24.074999639Z"}

for every slot now too

@KaloyanTanev could you give it a try and confirm that the beacon committee selection endpoint is working as intended?

Thanks

macladson added a commit that referenced this pull request May 13, 2025
@dknopik
Copy link
Member

dknopik commented May 21, 2025

I am still confused about the slot - 1. But when removing it, this PR looks good to me from the perspective of the functionality needed by Anchor. @michaelsproul do you know why that is there? By my understanding of the Altair spec, the proof for the previous slot is not at all relevant for the current slot.

@mergify
Copy link

mergify bot commented Jul 2, 2025

Some required checks have failed. Could you please take a look @chong-he? 🙏

@mergify mergify bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jul 2, 2025
@chong-he chong-he added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jul 2, 2025
Copy link
Member

@dknopik dknopik left a comment

Choose a reason for hiding this comment

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

Now that the off-by-one is fixed, this is good to go from an Anchor point of view :)

Thank you @chong-he!

@michaelsproul can you please take a look when you have time?

@michaelsproul michaelsproul self-requested a review July 10, 2025 05:14
@michaelsproul michaelsproul added the v8.0.0-rc.0 Q3 2025 release for Fusaka on Holesky label Jul 10, 2025
jimmygchen added a commit that referenced this pull request Jul 24, 2025
Comment on lines +604 to +608
// For distributed mode
if duties_service
.sync_duties
.selection_proof_config
.parallel_sign
Copy link
Member

Choose a reason for hiding this comment

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

I think we could think about re-combining these two cases in future, as the only substantial difference is the usage of FuturesUnordered in the non-distributed case. The non-distributed case is still parallel but it waits for all of the proofs to come in before writing them to the in-memory map. Whereas in the distributed case we add proofs as they become available. FuturesUnordered would likely be fine for both cases, and would save us a lot of duplication of logic.

I've opened an issue:

Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

I've pushed two commits to address my review comments. The most important change was removing .data[0] which can panic.

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Sep 3, 2025
@mergify mergify bot merged commit a93cafe into sigp:unstable Sep 3, 2025
37 checks passed
jtraglia pushed a commit to jtraglia/lighthouse that referenced this pull request Sep 16, 2025
…sigp#7016)

* sigp#6610


  - [x] Add `beacon_committee_selections` endpoint
- [x] Test beacon committee aggregator and confirmed working
- [x] Add `sync_committee_selections` endpoint
- [x] Test sync committee aggregator and confirmed working
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dvt Distributed validator technology e.g. SSV, Obol ready-for-merge This PR is ready to merge. v8.0.0-rc.0 Q3 2025 release for Fusaka on Holesky

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants