Skip to content

feat: periodically return attestation with updated signatures #1497

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
Merged
merged 54 commits into from
May 2, 2025
Merged

feat: periodically return attestation with updated signatures #1497

merged 54 commits into from
May 2, 2025

Conversation

litt3
Copy link
Contributor

@litt3 litt3 commented Apr 24, 2025

  • closes EGDA-1008

@litt3 litt3 self-assigned this Apr 24, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 24, 2025 •
edited
Loading

The latest Buf updates on your PR. Results from workflow Buf Proto / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedApr 25, 2025, 3:17 PM

litt3 added 2 commits April 24, 2025 13:36
@litt3
Clean up
bc85930 
Signed-off-by: litt3 <[email protected]>
@litt3
Use proper string builder
0bd99d5 
Signed-off-by: litt3 <[email protected]>
@litt3 litt3 changed the title Periodically return attestation with updated signatures feat: periodically return attestation with updated signatures Apr 24, 2025
@litt3 litt3 requested a review from cody-littley April 25, 2025 12:49
@litt3
Clean up TODOs
56f1c05 
Signed-off-by: litt3 <[email protected]>
cody-littley
cody-littley reviewed Apr 25, 2025
View reviewed changes
disperser/controller/dispatcher.go Outdated
Comment on lines 352 to 353
// this error isn't fatal: a subsequent PutAttestation attempt might succeed
// TODO: this used to cause the HandleSignatures method to fail entirely. Is it ok to continue trying here?
Copy link
Contributor

Choose a reason for hiding this comment

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

reminder to resolve this TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure the answer here yet. Let's leave this conversation open until I can get to the bottom it it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

cody-littley
cody-littley reviewed Apr 25, 2025
View reviewed changes
@litt3 litt3 requested a review from cody-littley April 28, 2025 18:53
@litt3 litt3 marked this pull request as ready for review April 28, 2025 20:45
@litt3 litt3 requested review from ian-shim and anupsv April 28, 2025 20:45
@litt3 litt3 requested review from cody-littley, anupsv and samlaf April 30, 2025 14:17
cody-littley
cody-littley approved these changes Apr 30, 2025
View reviewed changes
disperser/controller/dispatcher.go Outdated
"err", err,
"batchHeaderHash", batchHeaderHash)
}
// TODO: I removed the initial empty attestation update. Is that ok?
Copy link
Contributor

Choose a reason for hiding this comment

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

reminder to resolve TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

ian-shim
ian-shim reviewed Apr 30, 2025
View reviewed changes
Copy link
Contributor

@ian-shim ian-shim left a comment

Choose a reason for hiding this comment

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

looks good to me! few questions

api/clients/v2/payloaddispersal/payload_disperser.go
attestation := blobStatusReply.GetSignedBatch().GetAttestation()
for index, quorumNumber := range attestation.GetQuorumNumbers() {
quorumPercentagesBuilder.WriteString(
fmt.Sprintf("quorum_%d: %d%%, ", quorumNumber, attestation.GetQuorumSignedPercentages()[index]))
Copy link
Contributor

Choose a reason for hiding this comment

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

should we check len(attestation.GetQuorumSignedPercentages()) before taking the index?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've asserted that the quorum count and signed percentage have the same length in pollBlobStatusUntilSigned, so currently it would be safe to assume the length here.

I added an additional length check in 7639906, though, just in case

disperser/controller/dispatcher.go Outdated
}

// TODO(ian-shim): periodically update the attestation with intermediate quorum results
// TODO: I removed the initial empty attestation update. Is that ok?
Copy link
Contributor

Choose a reason for hiding this comment

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

The empty attestation is created here so that an attestation is available for query "immediately" after the blob is marked as GatheringSignatures considering that GetSignedBatch will return an attestation for the signed batch.
If we remove this empty attestation, we should update GetSignedBatch and behavior in get_blob_status_v2.go to make sure it returns empty attestation upon GetBlobStatus call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the GetSignedBatch in dynamo_metadata_store.go to return an empty attestation if none is found

46c76cb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After struggling with CI for a while, I decided to just keep the empty attestation put for now, to unblock this PR.

disperser/controller/signature_receiver.go Outdated
}

if seen := sr.signatureMessageReceived[signingMessage.Operator]; seen {
sr.logger.Warn("duplicate message from operator",
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be Error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 4aa8441

disperser/controller/signature_receiver.go Outdated

indexedOperatorInfo, found := sr.indexedOperatorState.IndexedOperators[signingMessage.Operator]
if !found {
sr.logger.Warn("operator not found in state",
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be Error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 4aa8441

disperser/controller/signature_receiver.go

operatorPubkey := indexedOperatorInfo.PubkeyG2
if !signingMessage.Signature.Verify(operatorPubkey, sr.batchHeaderHash) {
return fmt.Errorf("signature verification with pubkey %s", hex.EncodeToString(operatorPubkey.Serialize()))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: signature verification "failed" with pubkey...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samlaf has been driving an effort to shorten error messages by omitting prefixed like "failed to". See this conversation above

disperser/controller/signature_receiver.go

ok, err := aggregateSignersG1PubKey.VerifyEquivalence(sr.aggregateSignersG2PubKeys[quorumID])
if err != nil {
return nil, fmt.Errorf("verify aggregate G1 and G2 pubkey equivalence: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: failed to verify...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above

disperser/controller/signature_receiver.go
// attestation is yielded. This is used to track how long it takes to yield each attestation.
attestationUpdateStart time.Time

// significantSigningThresholdPercentage is a configurable "important" signing threshold. Right now, it's being
Copy link
Contributor

Choose a reason for hiding this comment

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

what does "important" and "special handling" mean?
looks like its there to record time at which the threshold is met. how is that timestamp used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Important" for now means "whatever the default signing threshold is", so the current default is 55.

Special handling in the future could be something like immediately emitting an attestation once quorums have reached the configured threshold. This is an optimization that isn't necessary now, but could shave off a couple hundred milliseconds of dispersal latency in the future, once we've improved performance enough for this delta to be significant.

Right now, this value is only being used to understand the profile of the signature gathering phase. i.e. how long after crossing the 'standard' signing threshold do we keep on gathering signatures?

@litt3 litt3 requested review from ian-shim and cody-littley May 1, 2025 17:21
@litt3
Remove TODOs
7b0103d 
Signed-off-by: litt3 <[email protected]>
ian-shim
ian-shim approved these changes May 1, 2025
View reviewed changes
Copy link
Contributor

@ian-shim ian-shim left a comment

Choose a reason for hiding this comment

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

lgtm, please resolve the ci failures

@litt3 litt3 merged commit fc9a971 into master May 2, 2025
15 checks passed
@litt3 litt3 deleted the periodic-signature-updates branch May 2, 2025 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cody-littley cody-littley cody-littley approved these changes

@ian-shim ian-shim ian-shim approved these changes

@anupsv anupsv Awaiting requested review from anupsv

@samlaf samlaf Awaiting requested review from samlaf

Assignees

@litt3 litt3

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@litt3 @anupsv @samlaf @cody-littley @ian-shim