Skip to content

fix(ffi): validate account seed lengths - #752

Open
thepastaclaw wants to merge 1 commit into
dashpay:devfrom
thepastaclaw:fix-ffi-seed-length-validation
Open

fix(ffi): validate account seed lengths#752
thepastaclaw wants to merge 1 commit into
dashpay:devfrom
thepastaclaw:fix-ffi-seed-length-validation

Conversation

@thepastaclaw

@thepastaclaw thepastaclaw commented May 10, 2026

Copy link
Copy Markdown
Contributor

FFI account seed-length validation

Summary

  • Add FFI-side seed-length validation for BLS seed derivation helpers before
    slicing or deriving.
  • Add EdDSA seed-length validation at the FFI boundary to mirror the underlying
    SLIP-10 contract.
  • Cover invalid BLS and EdDSA seed lengths with focused regression tests while
    leaving generic secp256k1 account seed behavior unchanged.

Validation

  • cargo fmt -- key-wallet-ffi/src/account_derivation.rs \ key-wallet-ffi/src/account_derivation_tests.rs
  • cargo test -p key-wallet-ffi --features eddsa,bls account_derivation -- \ --nocapture — 6 passed
  • code-review dashpay/rust-dashcore upstream/v0.42-dev \ fix-ffi-seed-length-validation "Validate key-wallet FFI BLS/EdDSA account \ seed lengths before slicing/deriving while preserving secp256k1 generic \ account seed behavior" — no significant issues found; recommendation:
    ship

Notes

Draft until human review marks it ready.

Summary by CodeRabbit

  • Bug Fixes

    • Improved account derivation input validation. BLS derivation now accepts seeds from 16 to 64 bytes, while EdDSA derivation requires at least 16 bytes.
    • Invalid seed lengths now fail safely with an invalid-input error instead of producing an account.
  • Tests

    • Added coverage confirming invalid seed lengths are rejected for both BLS and EdDSA account derivation.

@coderabbitai

coderabbitai Bot commented May 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 846b4717-4806-4372-b84d-f75fc7c86d75

📥 Commits

Reviewing files that changed from the base of the PR and between 0d8f959 and 94918c7.

📒 Files selected for processing (2)
  • key-wallet-ffi/src/account_derivation.rs
  • key-wallet-ffi/src/account_derivation_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • key-wallet-ffi/src/account_derivation.rs

📝 Walkthrough

Walkthrough

This PR adds private seed-length validators, enforces 16-byte minimums for BLS and EdDSA derivation, keeps the BLS maximum at 64 bytes, and adds regression tests for null results and FFIErrorCode::InvalidInput.

Changes

Seed-Length Validation

Layer / File(s) Summary
Validation and derivation guards
key-wallet-ffi/src/account_derivation.rs
Adds BLS and EdDSA validators. BLS accepts 16–64 bytes. EdDSA requires at least 16 bytes. Invalid input sets FFIErrorCode::InvalidInput and returns null.
Regression tests
key-wallet-ffi/src/account_derivation_tests.rs
Adds feature-gated tests for invalid BLS and EdDSA seed lengths.

Estimated code review effort: 2 (Simple) | ~12 minutes

Suggested reviewers: quantumexplorer

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding FFI validation for account seed lengths.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

@thepastaclaw

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 10, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@key-wallet-ffi/src/account_derivation_tests.rs`:
- Around line 282-292: The second negative test reuses the prior FFIError
(error) so a stale error.code can mask a missing error from
bls_account_derive_private_key_from_seed; before calling
bls_account_derive_private_key_from_seed for the long_seed test,
reinitialize/reset error (e.g., create a fresh FFIError/default or zeroed
instance) so the function must set it for the assert to be valid—ensure you
reset the same variable named error used with ffi_account and too_long prior to
the call and then assert error.code == FFIErrorCode::InvalidInput.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1d48f1d3-d750-4723-91a4-211dd0735532

📥 Commits

Reviewing files that changed from the base of the PR and between dca03ce and d60981f.

📒 Files selected for processing (2)
  • key-wallet-ffi/src/account_derivation.rs
  • key-wallet-ffi/src/account_derivation_tests.rs

Comment thread key-wallet-ffi/src/account_derivation_tests.rs
@thepastaclaw
thepastaclaw force-pushed the fix-ffi-seed-length-validation branch 2 times, most recently from b20ebe0 to 0d8f959 Compare May 10, 2026 09:47
@thepastaclaw

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 10, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@codecov

codecov Bot commented May 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 74.72%. Comparing base (9cbe4e7) to head (94918c7).

Files with missing lines Patch % Lines
key-wallet-ffi/src/account_derivation.rs 95.45% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #752      +/-   ##
==========================================
- Coverage   74.76%   74.72%   -0.05%     
==========================================
  Files         328      328              
  Lines       76593    76613      +20     
==========================================
- Hits        57267    57251      -16     
- Misses      19326    19362      +36     
Flag Coverage Δ
core 77.29% <ø> (ø)
ffi 49.13% <95.45%> (-0.59%) ⬇️
rpc 20.00% <ø> (ø)
spv 91.14% <ø> (+0.19%) ⬆️
wallet 75.69% <ø> (ø)
Files with missing lines Coverage Δ
key-wallet-ffi/src/account_derivation.rs 21.52% <95.45%> (+15.12%) ⬆️

... and 21 files with indirect coverage changes

@thepastaclaw

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai[bot]
coderabbitai Bot previously approved these changes May 21, 2026
@thepastaclaw

thepastaclaw commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

✅ Review complete (commit 0d8f959)

@thepastaclaw
thepastaclaw marked this pull request as ready for review June 16, 2026 07:06
@thepastaclaw

Copy link
Copy Markdown
Contributor Author

CI note: the new red evaluate run is not a code/test failure. It ran after the PR was marked ready for review, detected that CodeRabbit approved and all CI checks passed, then failed while trying to add the ready-for-review label because the workflow token lacks label-write permission (GraphQL: Resource not accessible by integration (addLabelsToLabelable)). The Rust/test matrix remains green, so I am leaving the branch unchanged.

@thepastaclaw

Copy link
Copy Markdown
Contributor Author

Ownership sweep follow-up: this PR looks ready for maintainer review. CodeRabbit approved the current head (0d8f959), Codecov/project and patch are green, and the Rust CI matrix is green; the remaining red evaluate job is the known Ready for Review Label workflow permission failure when adding the label, not a code/test failure. Could a maintainer take a pass or final decision when available?

@github-actions github-actions Bot added the merge-conflict The PR conflicts with the target branch. label Jul 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them.

@thepastaclaw
thepastaclaw force-pushed the fix-ffi-seed-length-validation branch from 0d8f959 to 94918c7 Compare August 3, 2026 01:29
@github-actions github-actions Bot removed the merge-conflict The PR conflicts with the target branch. label Aug 3, 2026
@github-actions github-actions Bot added the ready-for-review CodeRabbit has approved this PR label Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review CodeRabbit has approved this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant