Skip to content

test(stm): add negative single-signature tests#2985

Merged
jpraynaud merged 5 commits into
IntersectMBO:mainfrom
leepl37:fix/stm-negative-tests
Mar 3, 2026
Merged

test(stm): add negative single-signature tests#2985
jpraynaud merged 5 commits into
IntersectMBO:mainfrom
leepl37:fix/stm-negative-tests

Conversation

@leepl37

@leepl37 leepl37 commented Feb 4, 2026

Copy link
Copy Markdown
Contributor

Content

This PR adds negative tests for SingleSignature::verify in mithril-stm to ensure invalid inputs are rejected:

  • wrong verification key
  • out‑of‑bounds index
  • wrong message

This aligns with the reviewer suggestion from @hjeljeli32 and addresses #2946.
Note: dependency on #2943 is acknowledged; I will rebase/update after it merges if needed.

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • 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
    • Update README file (if relevant)
    • Update documentation website (if relevant)
    • Add dev blog post (if relevant)
    • Add ADR blog post or Dev ADR entry (if relevant)
    • No new TODOs introduced

Comments

This is a Draft PR as discussed with @jpraynaud. I am opening it now to allow for early review of the test logic.

Issue(s)

Relates to #2946
Blocked by #2943 (will rebase if needed)

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds negative unit tests for SingleSignature::verify in mithril-stm to ensure invalid inputs are rejected, addressing #2946.

Changes:

  • Introduce a shared test setup helper to build a signer / AVK context.
  • Add negative verification tests for: wrong verification key, out-of-bounds lottery index, and wrong message.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mithril-stm/src/protocol/single_signature/signature.rs Fixed
Comment thread mithril-stm/src/protocol/single_signature/signature.rs Fixed

@jpraynaud jpraynaud left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @leepl37 for the contribution 👍

I left few comments below.

Comment thread mithril-stm/src/protocol/single_signature/signature.rs Outdated
Comment thread mithril-stm/src/protocol/single_signature/signature.rs Outdated
Comment thread mithril-stm/src/protocol/single_signature/signature.rs Outdated
Comment thread mithril-stm/src/protocol/single_signature/signature.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@curiecrypt curiecrypt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good overall. Here are my suggestions:

  • Use a structure for the test context, I give the details below.
  • Modify build_single_signature_context function to take arguments such as number of signers, or rng. After that, it should return the struct created for test.
  • Another test case would be trying to verify a valid signature for another registration. This would be done with a different avk (in the current version). In the case we have the structure and context builder function, we can use different calls to this function.

Comment thread mithril-stm/src/protocol/single_signature/signature.rs Outdated
Comment thread mithril-stm/src/protocol/single_signature/signature.rs Outdated
Comment thread mithril-stm/src/protocol/single_signature/signature.rs
Comment thread mithril-stm/src/protocol/single_signature/signature.rs Outdated
Comment thread mithril-stm/src/protocol/single_signature/signature.rs Outdated
@leepl37
leepl37 force-pushed the fix/stm-negative-tests branch from 9423053 to a6a6856 Compare February 15, 2026 05:33
Comment thread mithril-stm/src/protocol/single_signature/signature.rs Fixed
Comment thread mithril-stm/src/protocol/single_signature/signature.rs Fixed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mithril-stm/src/protocol/single_signature/signature.rs Outdated

@jpraynaud jpraynaud left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the updates @leepl37 !

Once all comments from @curiecrypt are addressed and the CI is all green, we'll be able to merge 👍

Comment thread mithril-stm/src/protocol/single_signature/signature.rs Outdated
Comment thread mithril-stm/src/protocol/single_signature/signature.rs Outdated
@leepl37

leepl37 commented Feb 24, 2026

Copy link
Copy Markdown
Contributor Author

Hi @curiecrypt and @jpraynaud! I have addressed all the feedback and implemented the new test context structure.
Let me know if you need any other tweaks.

@jpraynaud jpraynaud left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍

Thanks @leepl37 for the contribution!

@curiecrypt curiecrypt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM 🚀
Thank you!

@jpraynaud

Copy link
Copy Markdown
Member

@leepl37 could you bump the version and update the CHANGELOG file in mithril-stm crate?

I'll merge the PR once this is done 🚀

@leepl37
leepl37 force-pushed the fix/stm-negative-tests branch from dbb9202 to 9b64cc9 Compare March 3, 2026 02:28
@leepl37

leepl37 commented Mar 3, 2026

Copy link
Copy Markdown
Contributor Author

Hi @jpraynaud!
I have bumped the version and updated the CHANGELOG in the mithril-stm crate as requested.
This PR is now ready for the final merge. Thanks!

@jpraynaud
jpraynaud merged commit 5875aa6 into IntersectMBO:main Mar 3, 2026
57 checks passed
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.

5 participants