fix: is_ddust_tx sighash check for P2WSH multisig inputs

[SIDHARTH20K4]

fixes #45

what was wrong
input.witness.nth(0) always grabs the empty OP_0 item for P2WSH inputs, so the sighash check was failing for valid P2WSH and P2SH-P2WSH multisig transactions.

what I changed:
loop through all witness items, skip the last item (witness script) and check each signature's sighash byte individually.

[Copilot]

Pull request overview

Fixes is_ddust_tx so it no longer falsely rejects P2WSH (and P2SH-P2WSH) multisig inputs, where witness.nth(0) is the empty OP_0 placeholder rather than a signature. The new logic walks every witness item, skips the trailing item (assumed to be the witness script), and verifies the sighash byte of each remaining signature.

Changes:

• Replace single nth(0) sighash check with a loop over all witness items.
• Skip the last witness item (treated as the witness script) and tolerate items of unrecognized lengths (_ => continue instead of return false).
• Combine 64- and 65-byte cases into a single Taproot match arm.

Comments suppressed due to low confidence (3)

src/main.rs:680

• Changing the fallback arm from return false to _ => continue silently relaxes validation. Previously, any unrecognized witness item shape on a segwit input caused the transaction to be rejected as not matching the ddust pattern; now any item whose length is outside 64/65/70..=73 is ignored. While this is necessary to skip the empty OP_0 placeholder in P2WSH multisig and the witness script itself, in combination with skipping only the last item it also means signatures with unexpected lengths (e.g., truncated/odd encodings) are no longer treated as a mismatch. Consider explicitly skipping only known non-signature items (empty pushes, the witness script/control block) and keeping the strict default for everything else, or at minimum document why permissive behavior is now safe.

                    _ => continue,

src/main.rs:678

• A 64-byte Taproot signature has no appended sighash byte and implies SIGHASH_DEFAULT (semantically ALL, not ALL|ANYONECANPAY). Including 64 in this match arm and then comparing the last byte of the signature against TapSighashType::AllPlusAnyoneCanPay treats the final byte of the schnorr signature as if it were a sighash flag. While this will almost always still result in return false (because that byte is effectively random), the logic is misleading: such inputs should be unconditionally rejected as not matching ALL|ANYONECANPAY rather than checked byte-wise. Consider matching only 65 here and rejecting bare 64-byte schnorr sigs explicitly.

                    // Taproot signature
                    64 | 65 => {
                        if *item.last().unwrap() != TapSighashType::AllPlusAnyoneCanPay as u8 {
                            return false;
                        }

src/main.rs:696

• No tests appear to accompany this fix. The PR addresses a bug specifically demonstrated by a 2-of-2 P2WSH multisig transaction (per the linked issue), but no regression test for P2WSH multisig (or for P2WPKH and Taproot inputs that previously worked) is added. Consider adding unit tests for is_ddust_tx covering: P2WPKH, P2WSH single-sig, P2WSH multisig with OP_0, P2SH-P2WSH multisig, Taproot key-path, and Taproot script-path — both with and without ALL|ANYONECANPAY — to lock in the intended behavior and guard against regressions like the key-path skip described above.

    for input in &tx.input {
        if !input.witness.is_empty() {
            // segwit input: check sighash byte of each signature in the witness stack
            // skip the last item (witness script) and empty OP_0
            let witness_len = input.witness.len();
            for (i, item) in input.witness.iter().enumerate() {
                if i == witness_len - 1 {
                    continue;
                }
                match item.len() {
                    // ECDSA signature
                    70..=73 => {
                        if *item.last().unwrap() != EcdsaSighashType::AllPlusAnyoneCanPay as u8 {
                            return false;
                        }
                    }
                    // Taproot signature
                    64 | 65 => {
                        if *item.last().unwrap() != TapSighashType::AllPlusAnyoneCanPay as u8 {
                            return false;
                        }
                    }
                    _ => continue,
                }
            }
        }
        // legacy input: check sighash byte from scriptSig
        else if input.script_sig.is_p2pkh() || input.script_sig.is_p2sh() {
            for instruction in input.script_sig.instructions() {
                if let Ok(Instruction::PushBytes(data)) = instruction
                    && let Ok(sig) = Signature::from_slice(data.as_bytes())
                    && sig.sighash_type != EcdsaSighashType::AllPlusAnyoneCanPay
                {
                    return false;
                }
            }
        }
    }
    true

[SIDHARTH20K4]

Squashed to single commit. The taproot fix was a Copilot suggestion I verified and combined both related fixes for cleaner history.

[SIDHARTH20K4]

All requested changes complete:

• Fixed formatting
• Added tests for 2-of-2 and 2-of-3 P2WSH multisig

Run cargo test test_spend_p2wsh -- --nocapture to verify.

Ready for review.

[harismuzaffer]

utACK. I reviewed briefly, looks good, would like to test it locally and re-ack

[harismuzaffer]

Thanks for the work, lets address the pending comments

[harismuzaffer]

Looks good to me. @SIDHARTH20K4 create a separate issue for P2tr script path or feel free to fix it as part of #22

[bubb1es71]

@SIDHARTH20K4 please sign your commit and make sure it is rebased on the latest commits from the main branch.

[SIDHARTH20K4]

@bubb1es71 done, rebased on latest main and signed the commit.

[harismuzaffer]

@bubb1es71 done, rebased on latest main and signed the commit.

@SIDHARTH20K4 the commit is unverified, please check why it is so

[bubb1es71]

@bubb1es71 done, rebased on latest main and signed the commit.

I don't think you rebased this correctly, make sure your local main branch is updated to the current bip451:main branch before rebasing your branch on it.

[SIDHARTH20K4]

@bubb1es71 done, rebased on latest main and signed the commit.

I don't think you rebased this correctly, make sure your local main branch is updated to the current bip451:main branch before rebasing your branch on it.

rebased on bip451:main, force pushed.

[bubb1es71]

I ran this commit through Claude and it found some problems. Please take a look and see if these are real issues you should fix:

1. Correctness ⚠️
   Issues Found:
   P2WSH witness structure misunderstanding (src/main.rs:675): The code assumes P2WSH has the witness script as the last item and skips it, but this is incorrect. For P2WSH multisig (m-of-n):
   Witness structure: <0> ...
   The first item is OP_0 (empty push for CHECKMULTISIG bug)
   Signatures follow
   Witness script is last
   The current code skips the witness script but also tries to validate the empty OP_0 push and the witness script itself as signatures.
   Line 674: if i == witness_len - 1 { continue; } correctly skips the witness script, but the loop also processes i == 0 which is the empty dummy element, not a signature.
   Line 683-686: Checking for 65-byte taproot signatures in P2WSH witness is incorrect. P2WSH uses ECDSA signatures (70-73 bytes), not Schnorr signatures. A 65-byte item in P2WSH context would be invalid.
   Correct P2WSH validation should:

   // Skip first (dummy OP_0) and last (witness script)
   if i == 0 || i == witness_len - 1 {
       continue;
   }

2. Security ⚠️
   Vulnerabilities:
   False positives: The current code may incorrectly validate P2WSH transactions as dust transactions when they're not, because it checks the empty dummy element and potentially the witness script.
   Missing validation (src/main.rs:707): The new else block returns false for inputs with empty witness AND empty scriptSig. While this prevents completely empty inputs, it doesn't validate P2WPKH inputs, which have an empty scriptSig but should have a 2-item witness (signature, pubkey). The old code assumed P2WPKH would be caught by witness_len == 1, but P2WPKH actually has witness_len == 2.
   No malleability issues introduced by this change since it only validates existing signatures.

3. Bitcoin Protocol Compliance ⚠️
   BIP 141 (SegWit) Issues:
   P2WPKH not handled: P2WPKH has 2 witness items , not 1. The code treats witness_len == 1 as taproot-only.
   P2WSH parsing incorrect: See Add inputs to unconfirmed dust spend transaction #1 above.
   BIP 341 (Taproot) Compliance:
   ✅ Correctly identifies 65-byte sigs with explicit sighash
   ✅ Correctly rejects 64-byte SIGHASH_DEFAULT sigs
   ✅ Taproot key-path with witness_len == 1 is correct

4. Edge Cases ⚠️
   Handled:
   ✅ Empty witness items (line 680: 0 => continue)
   ✅ Empty inputs now rejected (line 708-709)
   Not Handled:
   ❌ P2WPKH: 2-item witness falls into the P2WSH branch
   ❌ P2WSH-wrapped P2WPKH: Would be mishandled
   ❌ Mixed input types: Code doesn't validate that all inputs use consistent ANYONECANPAY logic across different script types
   ❌ Non-standard witness sizes: What if someone creates a weird witness with unexpected item counts?

5. Code Quality ⚠️
   Issues:
   Line 683-686: Dead code path - 65-byte taproot sig check inside P2WSH branch is logically unreachable for valid Bitcoin transactions
   Line 698: Comment says "legacy input" but the condition !input.script_sig.is_empty() catches both legacy (P2PKH, P2SH) and P2SH-wrapped segwit
   No explicit P2WPKH handling: P2WPKH (the most common segwit type) isn't explicitly handled
   Removed old checks: Old code had is_p2pkh() and is_p2sh() checks which were more explicit
   Suggestions:

   // Better structure:
   match witness_len {
       0 => { /* legacy scriptSig validation */ },
       1 => { /* Taproot key-path */ },
       2 => { /* P2WPKH: validate sig at index 0 */ },
       _ => { /* P2WSH: skip index 0 and last, validate rest */ },
   }

6. Test Coverage ⚠️
   Good:
   ✅ Added 2-of-2 P2WSH multisig test
   ✅ Added 2-of-3 P2WSH multisig test
   Missing:
   ❌ P2WPKH test: Most common segwit type not tested
   ❌ Taproot key-path test: Despite fixing it, no test validates taproot
   ❌ Negative tests: No tests for rejecting wrong sighash types
   ❌ Mixed input test: Transaction with both P2WPKH and P2WSH inputs
   ❌ Taproot script-path: Not tested (though may not be relevant for this use case)
   ❌ Edge case test: Empty witness, malformed signatures
   Recommended tests:

   #[test]
   fn test_spend_p2wpkh_dust()  // Single-sig segwit
   
   #[test]
   fn test_spend_taproot_keypath_dust()  // Taproot key-path
   
   #[test]
   fn test_reject_wrong_sighash()  // Should return false
   
   #[test]
   fn test_mixed_input_types()  // P2WPKH + P2WSH in same tx

Summary & Recommendations

Critical Issues:

• Fix P2WSH validation logic to skip both dummy element (index 0) AND witness script (last index)
• Add explicit P2WPKH handling for 2-item witnesses
• Remove unreachable taproot check inside P2WSH branch (line 683-686)
• Suggested fix for src/main.rs:657-710:

  match witness_len {
      1 => {
          // Taproot key-path spend
          let sig = input.witness.nth(0).unwrap();
          match sig.len() {
              65 => {
                  if sig[64] != TapSighashType::AllPlusAnyoneCanPay as u8 {
                      return false;
                  }
              }
              _ => return false,  // Reject SIGHASH_DEFAULT
          }
      }
      2 => {
          // P2WPKH: <signature> <pubkey>
          let sig = input.witness.nth(0).unwrap();
          if sig.len() < 70 || sig.len() > 73 {
              return false;
          }
          if *sig.last().unwrap() != EcdsaSighashType::AllPlusAnyoneCanPay as u8 {
              return false;
          }
      }
      _ => {
          // P2WSH: <OP_0> <sig1> ... <sigN> <witnessScript>
          for (i, item) in input.witness.iter().enumerate() {
              if i == 0 || i == witness_len - 1 {
                  continue;  // Skip dummy element and witness script
              }
              if item.is_empty() {
                  continue;  // Skip empty items (for m-of-n where m < n)
              }
              if item.len() < 70 || item.len() > 73 {
                  return false;
              }
              if *item.last().unwrap() != EcdsaSighashType::AllPlusAnyoneCanPay as u8 {
                  return false;
              }
          }
      }
  }

[SIDHARTH20K4]

thanks for the detailed review, this is really helping me understand the witness structure better. will fix these issues and update the PR.
@bubb1es71

[SIDHARTH20K4]

added test_spend_p2wsh_2of2_multisig which triggers the bug. the remaining multisig tests will come in a separate PR as discussed.

[bubb1es71]

ACK 7b76208

I added an additional commit to cleanup signature parsing with some TODOs for future work. If this looks good to you I'll merge it.

[SIDHARTH20K4]

@bubb1es71
looks good, feel free to merge. thanks for the cleanup!