Skip to content

refactor: use miniscript interpreter for input sighash detection - #51

Open
bubb1es71 wants to merge 1 commit into
bip451:mainfrom
bubb1es71:feature/interpreter-sighash-detection
Open

refactor: use miniscript interpreter for input sighash detection#51
bubb1es71 wants to merge 1 commit into
bip451:mainfrom
bubb1es71:feature/interpreter-sighash-detection

Conversation

@bubb1es71

Copy link
Copy Markdown
Collaborator

Authored by: kimi-k3

fixes #47

Summary

Rewrites ddust transaction detection (is_ddust_tx) to classify input signatures with the miniscript interpreter instead of witness-length heuristics, fixing detection of taproot script-path spends and resolving both TODOs in the function.

Background

A witness/scriptSig stack is untyped: whether an element is a signature, pubkey, hash preimage, or script is defined by the script being spent, not by the stack itself. The old code guessed script type from witness length (1 = taproot key-path, 2 = P2WPKH, 3+ = P2WSH multisig), which could not be made correct in general:

  • Taproot script-path spends (≥ 2 witness items + control block) were misclassified as P2WPKH/P2WSH, so ddust txs spending them were silently excluded from mempool batching — including txs this tool creates from tr() descriptors with script trees.
  • wsh(pk(key)) inputs have a 2-item witness <sig> <script> with no OP_0 dummy, so the old multisig logic skipped the signature entirely and accepted any sighash.

Changes

  • is_ddust_tx(tx, rpc_client) now fetches each input's prevout script_pubkey via RPC (cheap OP_RETURN check runs first, so only candidates incur the lookup). Unfetchable prevouts are skipped gracefully rather than panicking.
  • New is_all_anyonecanpay_input() feeds prevout script + scriptSig + witness into miniscript::interpreter::Interpreter::from_txdata and iterates iter_assume_sigs(). The interpreter assigns each stack element its script-defined role — only actual signatures are checked, covering legacy P2PK/P2PKH, P2SH (incl. wrapped segwit), P2WPKH, P2WSH, and taproot key-path and script-path. Signature validity is not verified, only the sighash type, so no prevout amounts or sighash cache are needed.
  • Conservative rejection: inputs that can't be interpreted (non-miniscript scripts, malformed elements, unsatisfied timelocks) or that contain no signatures are not classified as ddust.
  • No new dependency — bdk_wallet re-exports miniscript, so the bitcoin types align with existing code.

Tests

  • New src/test_sighash.rs: 17 fast, node-less unit tests for the pure classification function:
    • Newly supported types: taproot script-path (pk leaf, multi_a 2-of-3 with empty sig slot), wsh(pk), P2SH-P2WPKH
    • Negative cases: plain SIGHASH_ALL, 64-byte SIGHASH_DEFAULT, invalid 65-byte 0x00 sighash, one bad sighash in a multi-sig input, signature-less inputs, empty inputs
  • All 57 existing integration tests pass unchanged (P2PKH, P2WPKH, P2TR key-path, P2WSH/P2SH/P2SH-P2WSH multisig, all batching combinations)
  • cargo fmt --check, cargo clippy --tests -- -D warnings clean; full suite: 74 passed

@SIDHARTH20K4

Copy link
Copy Markdown
Contributor

While going through the files changed tab, I noticed that is_ddust_tx now takes an rpc_client and calls get_raw_transaction to fetch the prevout script for every input. That's a nice fix for the classification bug, but it also means this function now needs to look up potentially old, already-spent transactions by txid.

From what I understand, Bitcoin Core only supports that kind of lookup if the node is running with txindex=1. Most nodes don't have that enabled by default. If that's right, then on a regular node this function might just fail to fetch the prevout for a lot of inputs and quietly return false for them, since the "unfetchable prevout" case is handled by skipping rather than erroring out.

Not saying this is wrong, just wanted to flag it in case it's worth mentioning as a requirement somewhere, or double checking what actually happens on a node without txindex.

@bubb1es71

Copy link
Copy Markdown
Collaborator Author

Good catch. I'll update the README to require this index is enabled.

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.

Improve input script type detection in is_ddust_tx()

2 participants