Skip to content

Add more vectors coverage#46

Merged
siv2r merged 4 commits into
masterfrom
polish-vectors
May 19, 2026
Merged

Add more vectors coverage#46
siv2r merged 4 commits into
masterfrom
polish-vectors

Conversation

@siv2r
Copy link
Copy Markdown
Owner

@siv2r siv2r commented May 13, 2026

Address bitcoin/bips#2070 (comment), except different $(t, n)$ suggestion

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d8c2b64702

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread python/gen_vectors.py
Comment thread python/gen_vectors.py
@siv2r siv2r force-pushed the polish-vectors branch from 79bf23e to 5228668 Compare May 13, 2026 15:51
Comment thread python/gen_vectors.py
Comment thread python/gen_vectors.py Outdated
Comment thread python/gen_vectors.py
Comment thread python/gen_vectors.py
Comment thread python/gen_vectors.py
@claude
Copy link
Copy Markdown

claude Bot commented May 13, 2026

Architectural Review - Summary

This PR significantly expands test vector coverage for FROST signing, adding approximately 500 lines with new error cases and improved documentation.

Strengths:

  1. Expanded Error Coverage: Adds important edge cases for out-of-range identifiers, zero-valued secret shares, secnonce validation, and point-at-infinity tweaking
  2. Improved Error Messages: More descriptive messages aid debugging
  3. Better Documentation: Test vectors summary is valuable

Concerns:

  1. Removed Test Cases: Several validation tests were removed including checks for mismatched counts and pubshare validation. Question: Are these now redundant or moved elsewhere?
  2. Test Maintenance Complexity: Vector generation has grown more complex with conditional overrides and computed tweaks. Consider adding more inline documentation.
  3. Vector File Format: JSON formatting changes may affect machine parsing - ensure tools are tested.

Recommendations:

  • Verify test coverage to ensure removed cases don't leave gaps
  • Document test categories and their BIP specification references
  • Consider organizing vectors into subcategories for scalability

Verdict: Valuable improvements pending fixes to inline issues (particularly reconstruct_thresh_sk validation) and clarification on removed test cases.

@siv2r siv2r changed the title Add more vectors coverage Add more vectors coverage and fix BIP text & python inconsistencies May 19, 2026
@siv2r siv2r changed the title Add more vectors coverage and fix BIP text & python inconsistencies Add more vectors coverage May 19, 2026
@siv2r siv2r merged commit 6dbfff6 into master May 19, 2026
12 of 13 checks passed
@siv2r siv2r deleted the polish-vectors branch May 19, 2026 23:04
@mllwchrry
Copy link
Copy Markdown

I reviewed the test vectors, and everything looks good. Do you plan to add coverage for different threshold settings in another PR?

@siv2r
Copy link
Copy Markdown
Owner Author

siv2r commented May 21, 2026

Thanks for the review. Yes, I'm currently looking into it.

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.

2 participants