Skip to content

Conversation

@andresilva
Copy link
Collaborator

@andresilva andresilva commented Dec 18, 2025

This PR refactors the certificate scheme API to store the namespace in the scheme at construction time rather than passing it on every sign/verify call. This avoids redundant namespace derivation and simplifies the signing API.

The previous API required passing namespace bytes to every sign(), verify_attestation(), and verify_attestations() call. For protocols like simplex with multiple message types (notarize, nullify, finalize, seed), this meant recomputing derived namespace bytes on every operation.

The new design introduces a Namespace trait for types that can derive themselves from a base namespace. The Subject trait now has an associated Namespace type, and the scheme computes this namespace once at construction. Sign and verify methods no longer need the namespace parameter since it's already stored in the scheme.

Closes #2521.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Dec 18, 2025

Deploying monorepo with  Cloudflare Pages  Cloudflare Pages

Latest commit: 924dd81
Status: ✅  Deploy successful!
Preview URL: https://50485770.monorepo-eu0.pages.dev
Branch Preview URL: https://andre-consensus-avoid-namesp.monorepo-eu0.pages.dev

View logs

@andresilva andresilva force-pushed the andre/consensus-avoid-namespace-recomputation branch from 399896d to dee840b Compare December 19, 2025 14:48
@andresilva andresilva self-assigned this Dec 19, 2025
@andresilva andresilva moved this to Ready for Review in Tracker Dec 19, 2025
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jan 3, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
commonware-mcp 924dd81 Jan 05 2026, 06:54 AM

Copy link
Contributor

@patrick-ogrady patrick-ogrady left a comment

Choose a reason for hiding this comment

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

Things are looking pretty good, just making my way through!

/// This type encapsulates the pre-computed namespace bytes used for signing and
/// verifying acks.
#[derive(Clone, Debug)]
pub struct Namespace(Vec<u8>);
Copy link
Contributor

Choose a reason for hiding this comment

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

Considered suggesting we enshrine this type in cryptography but think that is out of scope/separate PR-worthy)

///
/// This struct holds the pre-computed namespace bytes for each vote type.
#[derive(Clone, Debug)]
pub struct Namespace {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, now I see why this isn't enshrined.

Copy link
Contributor

@patrick-ogrady patrick-ogrady left a comment

Choose a reason for hiding this comment

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

Making some small tweaks in #2688 then should be g2g

@patrick-ogrady patrick-ogrady merged commit 48e07c4 into main Jan 5, 2026
127 checks passed
@patrick-ogrady patrick-ogrady deleted the andre/consensus-avoid-namespace-recomputation branch January 5, 2026 09:01
@github-project-automation github-project-automation bot moved this from Ready for Review to Done in Tracker Jan 5, 2026
@codecov
Copy link

codecov bot commented Jan 5, 2026

Codecov Report

❌ Patch coverage is 99.63843% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.77%. Comparing base (6776470) to head (924dd81).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
consensus/src/simplex/types.rs 95.58% 6 Missing ⚠️
cryptography/src/certificate.rs 83.33% 1 Missing ⚠️
@@            Coverage Diff             @@
##             main    #2559      +/-   ##
==========================================
- Coverage   92.77%   92.77%   -0.01%     
==========================================
  Files         362      362              
  Lines      105115   106061     +946     
==========================================
+ Hits        97525    98398     +873     
- Misses       7590     7663      +73     
Files with missing lines Coverage Δ
consensus/src/aggregation/engine.rs 84.93% <100.00%> (-0.07%) ⬇️
consensus/src/aggregation/mocks/reporter.rs 90.00% <100.00%> (-0.10%) ⬇️
consensus/src/aggregation/mod.rs 98.73% <100.00%> (-0.05%) ⬇️
consensus/src/aggregation/types.rs 98.68% <100.00%> (+0.03%) ⬆️
consensus/src/marshal/actor.rs 92.77% <100.00%> (-0.02%) ⬇️
consensus/src/marshal/mod.rs 95.28% <100.00%> (-0.01%) ⬇️
consensus/src/ordered_broadcast/ack_manager.rs 99.32% <100.00%> (ø)
consensus/src/ordered_broadcast/engine.rs 85.95% <100.00%> (-0.14%) ⬇️
consensus/src/ordered_broadcast/mocks/reporter.rs 85.82% <100.00%> (ø)
consensus/src/ordered_broadcast/mod.rs 99.06% <100.00%> (+0.01%) ⬆️
... and 38 more

... and 11 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 6776470...924dd81. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[consensus] Avoid Dynamic Computation of Namespace Helpers

3 participants