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.

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

[consensus] Concurrent Message Validation #614

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Conversation

patrick-ogrady
Copy link
Contributor

@patrick-ogrady patrick-ogrady commented Mar 16, 2025

Related: #434

return;
}
// Verify signature
self.context.with_label("nullify").spawn({
Copy link
Contributor Author

@patrick-ogrady patrick-ogrady Mar 16, 2025

Choose a reason for hiding this comment

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

While I originally thought we should do this as a "stream wrapper", I found that the optimal approach was deep within consensus (after we filtered things we would otherwise never want to verify -> like useless notarization/finalization messages).

Copy link
Contributor Author

@patrick-ogrady patrick-ogrady Mar 16, 2025

Choose a reason for hiding this comment

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

This change isn't without tradeoffs, however. It is now possible that we may verify duplicate messages from the same user (whereas we otherwise would have updated the map upon successful processing) or verify unnecessary notarization/nullification/finalization messages (when we have already seen one).

This should only happen if messages are queued very close together but the point still stands.

@@ -268,8 +268,8 @@ impl<C: Element> Poly<C> {
}

/// Returns the public key of the polynomial (constant term).
pub fn public(public: &Public) -> group::Public {
*public.constant()
pub fn public(public: &Public) -> &group::Public {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In other places in the codebase, we now return a reference where it makes sense (leaving it up to the caller whether or not they should clone.

Copy link

codecov bot commented Mar 16, 2025

Codecov Report

Attention: Patch coverage is 92.69006% with 25 lines in your changes missing coverage. Please review.

Project coverage is 89.01%. Comparing base (f7bf94b) to head (51b16de).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...sensus/src/threshold_simplex/actors/voter/actor.rs 92.64% 17 Missing ⚠️
...sus/src/threshold_simplex/actors/resolver/actor.rs 50.00% 6 Missing ⚠️
cryptography/src/bls12381/primitives/ops.rs 96.96% 2 Missing ⚠️
@@            Coverage Diff             @@
##             main     #614      +/-   ##
==========================================
+ Coverage   88.96%   89.01%   +0.04%     
==========================================
  Files         137      137              
  Lines       35287    35418     +131     
==========================================
+ Hits        31393    31526     +133     
+ Misses       3894     3892       -2     
Files with missing lines Coverage Δ
broadcast/src/linked/engine.rs 90.05% <100.00%> (ø)
broadcast/src/linked/mod.rs 99.28% <100.00%> (ø)
consensus/src/threshold_simplex/mod.rs 93.48% <100.00%> (+<0.01%) ⬆️
consensus/src/threshold_simplex/prover.rs 98.44% <100.00%> (ø)
consensus/src/threshold_simplex/verifier.rs 70.90% <100.00%> (+1.50%) ⬆️
cryptography/src/bls12381/dkg/mod.rs 100.00% <100.00%> (ø)
cryptography/src/bls12381/mod.rs 100.00% <100.00%> (ø)
cryptography/src/bls12381/primitives/poly.rs 91.04% <100.00%> (ø)
cryptography/src/bls12381/primitives/ops.rs 98.56% <96.96%> (-0.16%) ⬇️
...sus/src/threshold_simplex/actors/resolver/actor.rs 87.98% <50.00%> (-1.68%) ⬇️
... and 1 more

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7bf94b...51b16de. Read the comment docs.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Successfully merging this pull request may close these issues.

1 participant