Skip to content

Comments

Refactor/stream vc vote publishing#8880

Open
shane-moore wants to merge 11 commits intosigp:unstablefrom
shane-moore:refactor/stream-vc-vote-publishing
Open

Refactor/stream vc vote publishing#8880
shane-moore wants to merge 11 commits intosigp:unstablefrom
shane-moore:refactor/stream-vc-vote-publishing

Conversation

@shane-moore
Copy link
Member

Proposed Changes

Changes four ValidatorStore batch signing methods to return impl Stream instead of Future. Services consume the stream and publish each batch as it arrives. No behavioral change for lh since LighthouseValidatorStore wraps everything in stream::once

Also replaces anonymous tuples in method signatures with named structs

Motivation

Anchor needs to yield signing results per SSV committee. Each committee runs consensus independently, so streams let faster committees publish without waiting on slower ones.

/// Sign a batch of sync committee contributions and return results as a stream of batches.
fn sign_sync_committee_contributions(
self: &Arc<Self>,
contributions: Vec<ContributionToSign<Self::E>>,
Copy link
Member Author

Choose a reason for hiding this comment

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

@dknopik @michaelsproul, made some helper structs like ContributionToSign to replace inline tuple structs like

(
            u64,
            PublicKeyBytes,
            SyncCommitteeContribution<Self::E>,
            SyncSelectionProof,
        )

wdyt?

.map(|att| att.attester_index)
.collect::<Vec<_>>();
let published_count = single_attestations.len();
Err(e) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I was originally doing return Err('...') in this variant after the log. I think an issue with that is that if we hit an error with one batch, then we'd early return before the rest could be processed

also, propagating the error up just logs an error, which we're doing with the crit! anyways, so seems fine to allow the stream to complete and hit the Ok(), since the logging happens regardless

open to thoughts from @dknopik and otherwise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant