Skip to content

fix(cli): use attack-specific recovery failure reasons#27

Open
oritwoen wants to merge 2 commits intomainfrom
fix/generic-recovery-failure-reason
Open

fix(cli): use attack-specific recovery failure reasons#27
oritwoen wants to merge 2 commits intomainfrom
fix/generic-recovery-failure-reason

Conversation

@oritwoen
Copy link
Owner

Recovery failure always reported "all pairs have s1 == s2" regardless of which attack was running. That message only makes sense for nonce-reuse - for polynonce or biased-nonce it's actively misleading.

Now each attack type gets its own reason string when recovery fails.

Closes #21

@coderabbitai
Copy link

coderabbitai bot commented Mar 15, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5a7898cb-e362-4557-b43c-47d33a078189

📥 Commits

Reviewing files that changed from the base of the PR and between 4197b9b and 75c0298.

📒 Files selected for processing (1)
  • src/main.rs
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: test
🧰 Additional context used
📓 Path-based instructions (3)
src/main.rs

📄 CodeRabbit inference engine (AGENTS.md)

src/main.rs: Add new CLI subcommands by extending the enum Command in main.rs
Use exit code 0 for clean runs, 1 for vulnerabilities found, 2 for errors

Files:

  • src/main.rs
src/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.rs: Import PrimeField trait from k256 when using Scalar::from_repr()
Never suppress type errors using as any or equivalent type casts

Files:

  • src/main.rs
src/{math,main}.rs

📄 CodeRabbit inference engine (AGENTS.md)

Do not implement WIF (Wallet Import Format) output functionality

Files:

  • src/main.rs
🧠 Learnings (5)
📓 Common learnings
Learnt from: oritwoen
Repo: oritwoen/vusi PR: 9
File: src/attack/polynonce.rs:0-0
Timestamp: 2026-01-15T17:42:59.187Z
Learning: In the vusi project (src/attack/polynonce.rs), ECDSA signatures are validated at the signature parsing layer before reaching attack implementations, so `s=0` checks are not needed in the polynonce attack code.
📚 Learning: 2026-01-15T17:42:59.187Z
Learnt from: oritwoen
Repo: oritwoen/vusi PR: 9
File: src/attack/polynonce.rs:0-0
Timestamp: 2026-01-15T17:42:59.187Z
Learning: In the vusi project (src/attack/polynonce.rs), ECDSA signatures are validated at the signature parsing layer before reaching attack implementations, so `s=0` checks are not needed in the polynonce attack code.

Applied to files:

  • src/main.rs
📚 Learning: 2026-01-15T18:08:47.413Z
Learnt from: CR
Repo: oritwoen/vusi PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T18:08:47.413Z
Learning: Applies to src/attack/*.rs : Implement `trait Attack` when adding a new attack type for ECDSA vulnerability analysis

Applied to files:

  • src/main.rs
📚 Learning: 2026-01-15T17:42:59.187Z
Learnt from: oritwoen
Repo: oritwoen/vusi PR: 9
File: src/attack/polynonce.rs:0-0
Timestamp: 2026-01-15T17:42:59.187Z
Learning: The polynonce attack implementation in vusi (src/attack/polynonce.rs) uses exactly 4 signatures for the linear case (degree=1) elimination polynomial construction. The `.take(4)` behavior is intentional and documented.

Applied to files:

  • src/main.rs
📚 Learning: 2026-01-15T18:08:47.413Z
Learnt from: CR
Repo: oritwoen/vusi PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T18:08:47.413Z
Learning: Applies to src/math.rs : Implement key recovery using the formula `(s*k - z) / r` in the `recover_private_key` function

Applied to files:

  • src/main.rs
🔇 Additional comments (1)
src/main.rs (1)

224-233: Good fix: failure reason is now attack-aware instead of nonce-reuse-only.

This removes a misleading message path for non-nonce-reuse attacks and keeps a correct generic fallback for unknown types.


📝 Walkthrough

Walkthrough

Error messages on recovery failure now map to attack type instead of hardcoding the nonce-reuse message for all attacks. Uses pattern matching on vuln.attack_type to return contextually accurate failure reasons for polynonce, biased-nonce, and others.

Changes

Cohort / File(s) Summary
Recovery Error Messaging
src/main.rs
Replaced hardcoded "all pairs have s1 == s2" message with attack-type-specific reasons via pattern matching: nonce-reuse → s-value collision, polynonce → invalid polynomial roots, biased-nonce variants → lattice reduction failure, default fallback for unknowns.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

When recovery fails, now you'll know the why— ✨
Nonce-reuse screams "s-values collide!"
Polynonce roots rejected, lattice vectors tried,
Each attack gets its truth, no more lies. 🔍

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Title accurately describes the main change: making recovery failure reasons attack-specific instead of a hardcoded nonce-reuse message.
Description check ✅ Passed Description explains the problem and solution but omits the Testing section with checkboxes from the template.
Linked Issues check ✅ Passed Code maps attack types to contextual failure reasons (nonce-reuse, polynonce, biased-nonce, default) matching issue #21's request for attack-specific messages instead of the hardcoded nonce-reuse message.
Out of Scope Changes check ✅ Passed Changes are confined to the recovery failure path in main.rs, replacing the static message with dynamic attack-type-based reasons as required by issue #21.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/generic-recovery-failure-reason
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch fix/generic-recovery-failure-reason
📝 Coding Plan
  • Generate coding plan for human review comments

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 1 file

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Auto-approved: Low-risk cosmetic change improving CLI error messages for different attack types.

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.

bug: recovery failure reason hardcoded to nonce-reuse message for all attack types

1 participant