Skip to content

SNARK-friendly STM: Remove basic verifier #2845

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
Open
curiecrypt wants to merge 1 commit into
base: main
Choose a base branch
from
Open

SNARK-friendly STM: Remove basic verifier #2845

curiecrypt wants to merge 1 commit into from

Conversation

@curiecrypt
Copy link
Collaborator

@curiecrypt curiecrypt commented Dec 5, 2025 •
edited
Loading

Content

This PR removes the intermediate basic_verifier layer. The actual functions handle its functionality.

Pre-submit checklist

  • Branch
    • Crates versions are updated (if relevant)
    • CHANGELOG file is updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • All check jobs of the CI have succeeded
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested
  • Documentation
    • No new TODOs introduced

Comments

The basic_verifier functionality can be accessed from the branch: curiecrypt/basic-verifier if it is needed in the future.

Issue(s)

Relates to #2794

github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 5, 2025
View reviewed changes
@github-actions
Copy link

github-actions bot commented Dec 5, 2025 •
edited
Loading

Test Results

    4 files  ±0    169 suites   - 3   23m 45s ⏱️ +18s
2 271 tests  - 3  2 271 ✅  - 3  0 💤 ±0  0 ❌ ±0 
7 128 runs   - 9  7 128 ✅  - 9  0 💤 ±0  0 ❌ ±0 

Results for commit b9dcde2. ± Comparison against base commit b3772ac.

This pull request removes 3 tests. 
mithril-stm ‑ protocol::aggregate_signature::tests::test_core_verifier
mithril-stm ‑ protocol::aggregate_signature::tests::test_total_stake_core_verifier
mithril-stm::stm_basic ‑ test_core_verifier

♻️ This comment has been updated with latest results.

@curiecrypt curiecrypt requested a review from jpraynaud December 8, 2025 14:16
@curiecrypt curiecrypt force-pushed the curiecrypt/msnark/remove-basic-verifier branch from bad4030 to fef6ca4 Compare December 9, 2025 12:54
@curiecrypt curiecrypt force-pushed the curiecrypt/msnark/remove-basic-verifier branch from fef6ca4 to ab19294 Compare December 9, 2025 13:12
@curiecrypt curiecrypt marked this pull request as ready for review December 9, 2025 13:13
jpraynaud
jpraynaud reviewed Dec 11, 2025
View reviewed changes
mithril-stm/src/proof_system/concatenation.rs
Comment on lines +114 to +120
// BasicVerifier::preliminary_verify(
// &avk.get_total_stake(),
// &self.signatures,
// parameters,
// &msgp,
// )
// .with_context(|| "Preliminary verification of aggregate signatures failed.")?;
Copy link
Member

@jpraynaud jpraynaud Dec 11, 2025

Choose a reason for hiding this comment

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

You forgot to remove this one.

mithril-stm/src/protocol/aggregate_signature/clerk.rs Outdated
Comment on lines 5 to 9
// #[cfg(feature = "future_proof_system")]
// use anyhow::anyhow;

#[cfg(feature = "future_proof_system")]
use crate::AggregationError;
// #[cfg(feature = "future_proof_system")]
// use crate::AggregationError;
Copy link
Member

@jpraynaud jpraynaud Dec 11, 2025

Choose a reason for hiding this comment

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

These ones also can be removed

mithril-stm/src/protocol/aggregate_signature/clerk.rs
let mut removal_idx_by_vk: HashMap<&SingleSignatureWithRegisteredParty, Vec<Index>> =
HashMap::new();

let avk = self.compute_aggregate_verification_key();
Copy link
Member

@jpraynaud jpraynaud Dec 11, 2025

Choose a reason for hiding this comment

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

The computation could be done externally so that we keep the function pure and easy to test.
Maybe you can create a function that does the deduplication and which is pure and call this function in the select_valid_signatures_for_k_indices.

Also is it possible to move the unit tests for deduplication in this module?

@curiecrypt curiecrypt force-pushed the curiecrypt/msnark/remove-basic-verifier branch from ab19294 to b9dcde2 Compare December 15, 2025 14:13
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 15, 2025
View reviewed changes
mithril-stm/src/protocol/aggregate_signature/clerk.rs
@@ -1,5 +1,6 @@
use anyhow::Context;
use anyhow::{Context, anyhow};

Check failure

Code scanning / cargo-doc

cannot determine resolution for the import Error

cannot determine resolution for the import
mithril-stm/src/protocol/aggregate_signature/clerk.rs
@@ -1,5 +1,6 @@
use anyhow::Context;
use anyhow::{Context, anyhow};

Check failure

Code scanning / cargo-doc

unresolved imports anyhow::Context, anyhow::anyhow, anyhow::anyhow Error

unresolved imports anyhow::Context, anyhow::anyhow, anyhow::anyhow
mithril-stm/src/protocol/aggregate_signature/clerk.rs
@@ -1,5 +1,6 @@
use anyhow::Context;
use anyhow::{Context, anyhow};

Check failure

Code scanning / cargo-doc

cannot determine resolution for the import Error

cannot determine resolution for the import
mithril-stm/src/protocol/aggregate_signature/clerk.rs
@@ -1,5 +1,6 @@
use anyhow::Context;
use anyhow::{Context, anyhow};

Check failure

Code scanning / cargo-doc

unresolved imports anyhow::Context, anyhow::anyhow, anyhow::anyhow Error

unresolved imports anyhow::Context, anyhow::anyhow, anyhow::anyhow
mithril-stm/src/protocol/aggregate_signature/clerk.rs
ClosedKeyRegistration, Index, Parameters, Signer, SingleSignature, Stake, StmResult,
VerificationKey, proof_system::ConcatenationProof,
AggregationError, ClosedKeyRegistration, Index, Parameters, Signer, SingleSignature,
SingleSignatureWithRegisteredParty, Stake, StmResult, VerificationKey,

Check failure

Code scanning / cargo-doc

the name AggregationError is defined multiple times Error

the name AggregationError is defined multiple times
mithril-stm/src/protocol/aggregate_signature/clerk.rs
ClosedKeyRegistration, Index, Parameters, Signer, SingleSignature, Stake, StmResult,
VerificationKey, proof_system::ConcatenationProof,
AggregationError, ClosedKeyRegistration, Index, Parameters, Signer, SingleSignature,
SingleSignatureWithRegisteredParty, Stake, StmResult, VerificationKey,

Check warning

Code scanning / cargo-doc

unused import: AggregationError Warning

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

Reviewers

@jpraynaud jpraynaud jpraynaud left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@curiecrypt @jpraynaud