Skip to content

chore(key-wallet): drop the unused vendored PSBT implementation - #1041

Draft
ZocoLini wants to merge 1 commit into
devfrom
chore/drop-unused-psbt-module
Draft

ZocoLini wants to merge 1 commit into
devfrom
chore/drop-unused-psbt-module

Conversation

@ZocoLini

Copy link
Copy Markdown
Collaborator

A security report pointed out that PartiallySignedTransaction::sign() signs each input with whatever sighash type the input declares, with no BIP174 whitelist: a hostile coordinator can set SIGHASH_NONE|ANYONECANPAY (0x82) on an input and the victim's signature is then valid for any other transaction spending that same input.

The claim is accurate. sighash_ecdsa() only checked that the value was one of the six standard types, and 0x82 is standard. The behaviour is not ours, though: the whole module is a verbatim copy of rust-bitcoin's psbt (same sign(), same sighash_ecdsa(), same doc comments as bitcoin 0.32.5), which leaves the check to the BIP174 Signer role.

Rather than add the whitelist, the module goes away, because nothing used it:

  • No other module in this workspace referenced key_wallet::psbt. TransactionBuilder does its own signing and hardcodes EcdsaSighashType::All.
  • key-wallet-ffi and dash-spv-ffi expose no PSBT symbol, so the mobile wallets could never reach it.
  • dashpay/platform, the one repository that depends on key-wallet by git, has zero occurrences of psbt in any .rs/.toml/.ts/.js file.
  • The two examples that did use it, ecdsa-psbt and taproot-psbt, declared required-features = ["bitcoinconsensus"] — a feature that does not exist in dash/Cargo.toml — so they have never once compiled.

Most of it was meaningless here anyway: output_type() resolved inputs to Wpkh, Wsh, ShWpkh and Tr, computing segwit and taproot sighashes for a chain that has neither. Only Bare and Sh were reachable.

Removed with it: the BIP174 vector test and its fixtures, the two examples plus the dash -> key-wallet dev-dependency cycle they created, the dash_deserialize_psbt fuzz target and key-wallet as a dash-fuzz dependency, key-wallet's now-unused optional base64 dependency, and ScriptBuf::p2wpkh_script_code() with Script::v0_p2wpkh(), whose only callers in the workspace were the PSBT signer and each other.

walletcreatefundedpsbt and friends in rpc-client are Dash Core RPC methods, not this code, and stay.

Verified: cargo build --workspace --all-targets, cargo clippy --workspace --all-targets --all-features with no warnings, cargo fmt --check, cargo test -p key-wallet --all-features (646 passed), cargo test -p dashcore --all-features (628 + 37 + 12 + 1 passed), cargo doc with no broken intra-doc links.

@coderabbitai

coderabbitai Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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

codecov Bot commented Sep 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.14%. Comparing base (859f0ac) to head (1fba9b1).
⚠️ Report is 1 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1041      +/-   ##
==========================================
+ Coverage   76.02%   77.14%   +1.11%     
==========================================
  Files         255      320      +65     
  Lines       58413    81657   +23244     
==========================================
+ Hits        44410    62995   +18585     
- Misses      14003    18662    +4659     
Flag Coverage Δ
core 78.28% <ø> (+0.04%) ⬆️
ffi 50.52% <ø> (ø)
rpc 20.00% <ø> (ø)
spv 92.13% <ø> (+<0.01%) ⬆️
wallet 79.89% <ø> (?)
Files with missing lines Coverage Δ
dash/src/blockdata/script/borrowed.rs 76.12% <ø> (+1.67%) ⬆️
dash/src/blockdata/script/owned.rs 69.83% <ø> (+4.04%) ⬆️
dash/src/blockdata/transaction/mod.rs 86.76% <ø> (ø)

... and 68 files with indirect coverage changes

@github-actions github-actions Bot added the merge-conflict The PR conflicts with the target branch. label Sep 22, 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.

A security report pointed out that `PartiallySignedTransaction::sign()`
signs each input with whatever sighash type the input declares, with no
BIP174 whitelist: a hostile coordinator can set SIGHASH_NONE|ANYONECANPAY
(0x82) on an input and the victim's signature is then valid for any other
transaction spending that same input.

The claim is accurate. `sighash_ecdsa()` only checked that the value was
one of the six standard types, and 0x82 is standard. The behaviour is not
ours, though: the whole module is a verbatim copy of rust-bitcoin's psbt
(same `sign()`, same `sighash_ecdsa()`, same doc comments as bitcoin
0.32.5), which leaves the check to the BIP174 Signer role.

Rather than add the whitelist, the module goes away, because nothing used
it:

- No other module in this workspace referenced `key_wallet::psbt`.
  `TransactionBuilder` does its own signing and hardcodes
  `EcdsaSighashType::All`.
- `key-wallet-ffi` and `dash-spv-ffi` expose no PSBT symbol, so the mobile
  wallets could never reach it.
- dashpay/platform, the one repository that depends on key-wallet by git,
  has zero occurrences of `psbt` in any .rs/.toml/.ts/.js file.
- The two examples that did use it, `ecdsa-psbt` and `taproot-psbt`,
  declared `required-features = ["bitcoinconsensus"]` — a feature that
  does not exist in dash/Cargo.toml — so they have never once compiled.

Most of it was meaningless here anyway: `output_type()` resolved inputs to
`Wpkh`, `Wsh`, `ShWpkh` and `Tr`, computing segwit and taproot sighashes
for a chain that has neither. Only `Bare` and `Sh` were reachable.

Removed with it: the BIP174 vector test and its fixtures, the two
examples plus the dash -> key-wallet dev-dependency cycle they created,
the `dash_deserialize_psbt` fuzz target and key-wallet as a dash-fuzz
dependency, key-wallet's now-unused optional `base64` dependency, and
`ScriptBuf::p2wpkh_script_code()` with `Script::v0_p2wpkh()`, whose only
callers in the workspace were the PSBT signer and each other.

`walletcreatefundedpsbt` and friends in rpc-client are Dash Core RPC
methods, not this code, and stay.

Verified: cargo build --workspace --all-targets, cargo clippy --workspace
--all-targets --all-features with no warnings, cargo fmt --check,
cargo test -p key-wallet --all-features (646 passed), cargo test -p
dashcore --all-features (628 + 37 + 12 + 1 passed), cargo doc with no
broken intra-doc links.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ZocoLini
ZocoLini force-pushed the chore/drop-unused-psbt-module branch from c8240d7 to 1fba9b1 Compare September 22, 2026 16:05
@github-actions github-actions Bot removed the merge-conflict The PR conflicts with the target branch. label Sep 22, 2026
@ZocoLini ZocoLini closed this Sep 22, 2026
@ZocoLini ZocoLini reopened this Sep 22, 2026
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.

1 participant