You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(dashcore): reject high-S signatures in verify_data_signature - #1379
Since the Base SDK bump in #1214, EcdsaPublicKey::verify normalizes S before verifying, so verify_data_signature silently started accepting high-S ECDSA signatures. A high-S signature is a malleable twin of the low-S one, and callers relying on the previous strict behavior lost it without notice.
verify_data_signature now checks is_low_s before verifying and rejects high-S signatures, restoring the behavior from before the bump.
Bots — coderabbitai skipped after its own rate limit
Self-review — post /self-reviewed
Build running
Approvals
files with no dedicated owner (dash/src/signer.rs) — QuantumExplorer or xdustinface
When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.
Summary by CodeRabbit
Bug Fixes
Signatures with high S values are now rejected, preventing acceptance of malleable signature variants.
Since the Base SDK bump in #1214, `EcdsaPublicKey::verify` normalizes S before verifying, so `verify_data_signature` accepted high-S signatures it refused before. A high-S signature is a malleable twin of the low-S one.
`verify_data_signature` now checks `is_low_s` before verifying and returns an error for a high-S signature, as it did before the bump.
Reviewing files that changed from the base of the PR and between 352f99c and 9b1df99.
Files selected for processing: 1
dash/src/signer.rs
No actionable comments were generated in the recent review.
Walkthrough
verify_data_signature now rejects signatures with a high S value before public-key verification. Its documentation describes the rejection as protection against malleable twins of low-S signatures.
Changes: Signature verification
Layer / File(s)
Summary
Document and enforce low-S verification dash/src/signer.rs
The function documentation now states that high-S signatures are rejected. The implementation returns "the signature has a high S value" for a high-S signature and verifies low-S signatures as before.
dashpay/rust-dashcore#1214: Changed the Base SDK-backed verification behavior to accept high-S signatures and exposed the low-S check used here.
Suggested reviewers:xdustinface, quantumexplorer
Merge Risk: Minimal · previous review 352f9
The verifier rejects high-S signatures as intended; no merge-blocking issue was identified.
Pre-merge checks: 5
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly summarizes the main change: rejecting high-S signatures in verify_data_signature.
Linked Issues check
Issue #1249 requests restoring strict low-S verification or documenting the changed behavior. verify_data_signature now checks sig.is_low_s() and returns an error before public-key verification fo…
Out of Scope Changes check
The whole-PR diff changes only dash/src/signer.rs. The low-S check and its documentation directly address issue #1249. No unrelated changes are present.
Docstring Coverage
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Full details: Linked Issues check
Explanation
Issue #1249 requests restoring strict low-S verification or documenting the changed behavior. verify_data_signature now checks sig.is_low_s() and returns an error before public-key verification for high-S signatures. Its documentation also states that high-S signatures are rejected. The change implements the strict-verification option. The issue does not require a new automated test.
Full details: Docstring Coverage
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 1 files.
Finishing Touches
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Reviewing files that changed from the base of the PR and between ee50584 and 352f99c.
Files selected for processing: 1
dash/src/signer.rs
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.
✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.67%. Comparing base (ee50584) to head (9b1df99). ⚠️ Report is 12 commits behind head on dev.
Team: Claude Sonnet 5.5 · high, Claude Opus 5.5 · high
Memory: enabled (central)
Account limits:
Claude subscription: weekly 82% (resets 2026-11-17 05:00 UTC)
Judge decisions:
Kept: "No regression test for the high-S rejection" (suggestion, high confidence): "suggestion. Missing test coverage is a suggestion under the rubric, even though AGENTS.md asks every fix to come with a regression test that fails without it. The claim holds: the diff adds only the check at dash/src/signer.rs:28 and no test. The listed uses at dash/src/signer.rs:124-186 all verify signatures produced by the normal signing path, which yields low-S, so deleting line 28 would leave them passing."
The reason will be displayed to describe this comment to others. Learn more.
🔴 Changes requested: 1 blocker.
Caution
Claude subscription has used 82% of its weekly limit, past the 80% mark. It resets 2026-11-17 05:00 UTC.
Once a limit is used up, reviews on that account stop until it resets.
What blocks:verify_data_signature now refuses signatures it used to accept, but no test passes it a high-S signature. Nothing shows the new check fires.
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.
The reason will be displayed to describe this comment to others. Learn more.
✅ Approved.
What this does: Signature checking accepted a modified copy of a valid signature (the "high-S" form). Both forms are valid for the same data, so a signature could be altered without the key.
What to test
The review model wrote these steps. Check what a step runs before you follow it.
Sign data with sign and check it with verify_data_signature using the compressed public key. Expect Ok(()).
Replace S in that signature with n - S, the curve order minus S. Pass it to verify_data_signature. Expect an error containing the signature has a high S value.
Check a normal low-S signature against an uncompressed public key. Expect Ok(()).
Check a valid low-S signature against different data. Expect signature verification failed, not the high-S error.
Pass a 64-byte signature with no recovery byte. Expect length not 65 bytes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bot-review-skippedA required review bot did not report; it was skipped by the window or by a person.waiting-self-reviewWaiting for the author to post /self-reviewed
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
github-actions
Bot
added
the
waiting-bots
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![manki-review[bot]](https://avatars.githubusercontent.com/in/3200573?s=60&v=4){kind=small}
Since the Base SDK bump in #1214,
EcdsaPublicKey::verifynormalizes S before verifying, soverify_data_signaturesilently started accepting high-S ECDSA signatures. A high-S signature is a malleable twin of the low-S one, and callers relying on the previous strict behavior lost it without notice.verify_data_signaturenow checksis_low_sbefore verifying and rejects high-S signatures, restoring the behavior from before the bump.Closes #1249.
PR Hygiene ·
9b1df99/self-revieweddash/src/signer.rs) — QuantumExplorer or xdustinfaceWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.Summary by CodeRabbit