fix!: reject WIF keys without `0x01` flag, reject sighash types >`0xff`, maintain symmetry in {sign,recover} compact signatures, drop unused divergent segments by kwvg · Pull Request #1144 · dashpay/rust-dashcore

kwvg · 2026-10-08T10:42:12Z

Additional Information

Depends on refactor!: drop dashcore_hashes::hex in favour of hex-conservative, ban hex{,_lit,-literal} with cargo deny #1110
PrivateKey::from_wif acccepts any 34-byte payload as a compressed key, but reference refuses if the last byte isn't 0x01 (source). Attempts to import such keys now fail with key::Error::InvalidWifCompressionFlag.
During code cleanup in preparation for ECDSA integration with dash-pkc, some definitions were removed as they deviate with the reference implementation and sit unused.
- PublicKey sorts uncompressed keys first, while CPubKey::operator< compares serializations, placing 0x02 and 0x03 ahead of 0x04 (source). PublicKey::to_sort_key and SortKey were removed to follow.
- ExtendedPubKey's derived ordering starts at network and depth, while CExtPubKey::operator< orders by key, then chain code (source). PubKeyOrAddress inherited its ordering from PublicKey.
- PublicKey::read_from expects a bare key, but reference prefixes a CPubKey with its CompactSize length (source).
signer::CompactSignature duplicated MessageSignature, as both write the header like CKey::SignCompact (source).
- To preserve signer verification behaviour, MessageSignature::from_slice now only accepts the headers SignCompact emits (27 to 34), where it previously accepted anything from 27. Note that this is stricter than the reference implementation, as CPubKey::RecoverCompact masks the header and accepts any value (source).
Private child derivation panicked on an IL of zero, which BIP32 treats as valid (source) and the reference implementation accepts as a zero tweak (source). IL is now parsed as a Scalar, as SecretKey rejects zero.
ExtendedPrivKey::new_master and RootExtendedPrivKey::new_master now share one derivation, mirroring CExtKey::SetSeed (source), where only the latter validated the seed length. Both now return bip32::Error::InvalidSeedLength outside the 128-512 range permitted by BIP32 (source).
transaction_sign_input hashed with all 32 bits of its sighash type (source) but appended only the low byte, which verifiers hash with instead (source), so types above 0xff never verified, matching reference behaviour (source).

PR Hygiene · `cb23ea2`

Bots — coderabbitai ✓
Self-review — posted; again after any push
Build green
Approvals
- files with no dedicated owner (CHANGELOG.md, crypto/src/key.rs, dash/src/blockdata/script/builder.rs and 13 more) — QuantumExplorer or ZocoLini or xdustinface
- key-wallet (key-wallet/src/account/mod.rs, key-wallet/src/bip32.rs, key-wallet/src/wallet/helper.rs and 3 more) — QuantumExplorer or ZocoLini 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
- WIF parsing now rejects compressed-key payloads with an invalid compression flag.
- Message signatures with headers outside the supported range are rejected.
- Transaction signing now rejects sighash types above 0xff; supported one-byte types are handled consistently.
- BIP32 child derivation correctly handles zero-valued derivation components.
Breaking Changes
- Master seeds shorter than 16 bytes or longer than 64 bytes are rejected.
- Several key-ordering and signature-related APIs have been removed.

coderabbitai · 2026-10-08T10:42:20Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: dashpay/rust-dashcore/.coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: c47cda86-d32e-407a-a9f9-0b7bf797b27a

📥 Commits

Reviewing files that changed from the base of the PR and between c19973a and cb23ea2.

📒 Files selected for processing (22)

CHANGELOG.md
crypto/src/key.rs
dash/src/blockdata/script/builder.rs
dash/src/blockdata/witness.rs
dash/src/crypto/key.rs
dash/src/sign_message.rs
dash/src/signer.rs
key-wallet-ffi/Cargo.toml
key-wallet-ffi/FFI_API.md
key-wallet-ffi/src/account_derivation.rs
key-wallet-ffi/src/keys.rs
key-wallet-ffi/src/transaction.rs
key-wallet-ffi/src/tx_decode.rs
key-wallet/src/account/mod.rs
key-wallet/src/bip32.rs
key-wallet/src/wallet/helper.rs
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
key-wallet/src/wallet/root_extended_keys.rs
key-wallet/tests/address_tests.rs
rpc-client/src/error.rs
rpc-integration-test/src/main.rs
rpc-json/src/lib.rs

💤 Files with no reviewable changes (3)

dash/src/blockdata/witness.rs
key-wallet-ffi/src/tx_decode.rs
rpc-client/src/error.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.

📝 Walkthrough

Walkthrough

The changes update key parsing and public APIs, BIP32 seed validation and derivation, message and transaction signing, and wallet key construction. They also remove selected ordering implementations, key and witness methods, and an RPC error variant.

Changes

Cryptography and wallet updates

Layer / File(s)	Summary
Public-key encoding and WIF handling `crypto/src/key.rs`, `dash/src/blockdata/script/builder.rs`, `dash/src/crypto/key.rs`, `CHANGELOG.md`	`PublicKey::with_serialized` is public, and the key-push builder uses it. WIF parsing rejects 34-byte payloads whose final byte is not `0x01`. Public-key ordering, `read_from`, and sort-key APIs are removed.
BIP32 derivation and wallet key construction `key-wallet/src/bip32.rs`, `key-wallet/src/wallet/root_extended_keys.rs`, `key-wallet/src/account/mod.rs`, `key-wallet/src/wallet/helper.rs`, `key-wallet/tests/address_tests.rs`, `key-wallet-ffi/src/account_derivation.rs`, `key-wallet-ffi/src/keys.rs`, `rpc-integration-test/src/main.rs`, `CHANGELOG.md`	`ExtendedPrivKey::new_master` rejects seed lengths outside 16–64 bytes. Private child derivation adds the HMAC left-half scalar to the parent key. Root-key creation delegates to the extended-key constructor. Wallet and test code use `PrivateKey::new`; extended public-key identifiers use the DashCore public-key hash.
Message and wallet signing `dash/src/sign_message.rs`, `dash/src/signer.rs`, `dash/src/blockdata/witness.rs`, `key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`, `CHANGELOG.md`	Message-signature parsing accepts headers 27–34. Signing and verification use `MessageSignature`; wallet transaction signing uses DashCore signature and public-key serialization. `Witness::push_bitcoin_signature` is removed.
FFI transaction signing and scripts `key-wallet-ffi/src/transaction.rs`, `key-wallet-ffi/src/tx_decode.rs`, `key-wallet-ffi/Cargo.toml`, `key-wallet-ffi/FFI_API.md`, `CHANGELOG.md`	Transaction signing rejects sighash types above `0xff` and uses DashCore types to build signatures and P2PKH scripts. Tests cover script bytes and accepted and rejected sighash values. Public-key parsing in transaction decoding relies on `PublicKey::from_slice`.
RPC API and ordering changes `rpc-client/src/error.rs`, `rpc-json/src/lib.rs`, `CHANGELOG.md`	The RPC error type no longer has the secp256k1 variant or conversion. `PubKeyOrAddress` no longer derives ordering traits.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant FFIClient
  participant transaction_sign_input
  participant DashCore
  participant Builder
  FFIClient->>transaction_sign_input: transaction, input, sighash_type
  transaction_sign_input->>DashCore: calculate sighash and serialize signature
  transaction_sign_input->>Builder: push serialized signature and public key
  Builder-->>transaction_sign_input: scriptSig bytes
  transaction_sign_input-->>FFIClient: signing result

Suggested reviewers: quantumexplorer

Merge Risk: ⚪ Minimal · up to cb23e

No identified issue blocks merging after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 70.91% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 55 functions across 16 files. (3 skipped:…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title accurately summarizes the pull request’s main changes: WIF flag validation, sighash width validation, compact-signature symmetry, and removal of unused divergent APIs. It is long but specifi…

Full details: Docstring Coverage

Explanation

Docstring coverage is 70.91% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 55 functions across 16 files. (3 skipped: 3 unsupported.)

Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

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.

❤️ Share

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

codecov · 2026-10-08T10:52:24Z

Codecov Report

❌ Patch coverage is 80.83333% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.44%. Comparing base (c19973a) to head (cb23ea2).

Files with missing lines	Patch %	Lines
key-wallet-ffi/src/transaction.rs	77.77%	16 Missing ⚠️
key-wallet/src/account/mod.rs	0.00%	2 Missing ⚠️
key-wallet/src/bip32.rs	85.71%	2 Missing ⚠️
key-wallet-ffi/src/account_derivation.rs	0.00%	1 Missing ⚠️
key-wallet-ffi/src/keys.rs	0.00%	1 Missing ⚠️
key-wallet/src/wallet/helper.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1144      +/-   ##
==========================================
+ Coverage   78.20%   78.44%   +0.24%     
==========================================
  Files         298      298              
  Lines       78716    78651      -65     
==========================================
+ Hits        61556    61698     +142     
+ Misses      17160    16953     -207

Flag	Coverage Δ
core	`75.70% <100.00%> (+<0.01%)`	⬆️
ffi	`53.71% <75.67%> (+1.81%)`	⬆️
rpc	`49.29% <ø> (+0.08%)`	⬆️
spv	`92.32% <ø> (-0.03%)`	⬇️
wallet	`82.04% <73.68%> (+0.06%)`	⬆️

Files with missing lines	Coverage Δ
dash/src/blockdata/script/builder.rs	`75.34% <100.00%> (ø)`
dash/src/blockdata/witness.rs	`89.95% <ø> (-0.46%)`	⬇️
dash/src/crypto/key.rs	`100.00% <100.00%> (+0.86%)`	⬆️
dash/src/sign_message.rs	`90.00% <100.00%> (+10.89%)`	⬆️
dash/src/signer.rs	`100.00% <100.00%> (+1.73%)`	⬆️
key-wallet-ffi/src/tx_decode.rs	`78.63% <ø> (+0.10%)`	⬆️
.../wallet/managed_wallet_info/transaction_builder.rs	`92.89% <ø> (+0.16%)`	⬆️
key-wallet/src/wallet/root_extended_keys.rs	`57.54% <100.00%> (-1.08%)`	⬇️
rpc-client/src/error.rs	`7.14% <ø> (+0.75%)`	⬆️
rpc-json/src/lib.rs	`81.84% <ø> (ø)`
... and 6 more

... and 19 files with indirect coverage changes

github-actions · 2026-10-08T20:14:05Z

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

github-actions · 2026-10-08T21:59:45Z

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.

github-actions · 2026-10-08T23:29:30Z

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

kwvg · 2026-10-09T06:06:52Z

@coderabbitai review

coderabbitai · 2026-10-09T06:07:01Z

✅ Action performed

Review finished.

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.

github-actions · 2026-10-09T06:09:33Z

@coderabbitai review

No review for cb23ea20 yet, so PR Hygiene is asking once. If nothing arrives, the requirement is dropped for this commit and the pull request is labelled bot-review-skipped.

github-actions · 2026-10-09T06:09:39Z

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.

github-actions · 2026-10-09T06:13:05Z

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

kwvg · 2026-10-09T06:13:13Z

/self-reviewed

github-actions · 2026-10-09T06:14:53Z

Ready for review — files with no dedicated owner: QuantumExplorer or ZocoLini or xdustinface · key-wallet: QuantumExplorer or ZocoLini or xdustinface.
Full checklist in the description.

kwvg mentioned this pull request Oct 8, 2026

refactor!: drop dashcore_hashes::hex in favour of hex-conservative, ban hex{,_lit,-literal} with cargo deny #1110

Merged

3 of 6 tasks

github-actions Bot added the merge-conflict The PR conflicts with the target branch. label Oct 8, 2026

kwvg added 12 commits October 9, 2026 03:20

fix(crypto): reject WIF keys where compression flag isn't 0x01

385163c

refactor!: drop unused key definitions

d619933

refactor: serialize public keys for scripts through with_serialized

8b66611

refactor(dash)!: replace CompactSignature with MessageSignature

ee6a8d2

refactor: encode and hash with existing modules

e91ff65

fix(key-wallet): accept a zero IL in private child derivation

6b73e3e

refactor: build compressed private keys with PrivateKey::new

c53a818

refactor(key-wallet)!: derive BIP32 master keys in one place

c42e16a

refactor!: build P2PKH scripts and scriptSigs with dashcore helpers

589c007

fix(key-wallet): refuse sighash types wider than a byte when signing

d9186dd

refactor(key-wallet): drop the sender key length check in tx_decode

d018a0b

docs: update changelog

cb23ea2

kwvg force-pushed the fix_ecdsa branch from e4f9630 to cb23ea2 Compare October 8, 2026 21:51

kwvg marked this pull request as ready for review October 8, 2026 21:51

github-actions Bot added waiting-bots Waiting for the review bots to report on this head and removed merge-conflict The PR conflicts with the target branch. labels Oct 8, 2026

github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed bot-review-skipped A required review bot did not report; it was skipped by the window or by a person. and removed waiting-bots Waiting for the review bots to report on this head labels Oct 8, 2026

github-actions Bot added waiting-bots Waiting for the review bots to report on this head and removed waiting-self-review Waiting for the author to post /self-reviewed bot-review-skipped A required review bot did not report; it was skipped by the window or by a person. labels Oct 9, 2026

coderabbitai Bot approved these changes Oct 9, 2026

View reviewed changes

github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Oct 9, 2026

github-actions Bot added ready-for-human Bots have reported, the author has self-reviewed, and the build is green: this needs a human. and removed waiting-self-review Waiting for the author to post /self-reviewed labels Oct 9, 2026

github-actions Bot requested review from QuantumExplorer, ZocoLini and xdustinface October 9, 2026 06:14

ZocoLini merged commit 0eaf028 into dashpay:dev Oct 10, 2026
44 of 45 checks passed

kwvg mentioned this pull request Oct 11, 2026

refactor!: pin existing serde layout, abstract EDDSA backend types behind newtypes, switch to dash-pkc, re-export some bitcoin-crypto ECDSA types #1151

Draft

Conversation

kwvg commented Oct 8, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Additional Information

PR Hygiene · cb23ea2

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

codecov Bot commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Oct 8, 2026

Uh oh!

github-actions Bot commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 8, 2026

Uh oh!

kwvg commented Oct 9, 2026

Uh oh!

coderabbitai Bot commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 9, 2026

Uh oh!

github-actions Bot commented Oct 9, 2026

Uh oh!

github-actions Bot commented Oct 9, 2026

Uh oh!

kwvg commented Oct 9, 2026

Uh oh!

github-actions Bot commented Oct 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kwvg commented Oct 8, 2026 •

edited by github-actions Bot

Loading

PR Hygiene · `cb23ea2`

coderabbitai Bot commented Oct 8, 2026 •

edited

Loading

codecov Bot commented Oct 8, 2026 •

edited

Loading

github-actions Bot commented Oct 8, 2026 •

edited

Loading

coderabbitai Bot commented Oct 9, 2026 •

edited

Loading