test: add test for malleated transaction with valid witness

stratospher commented at 8:15 am on April 30, 2025: contributor

create_malleated_version() function in the functional tests currently creates transactions with same txid, different wtxid but the witness is invalid.
it is useful to have transactions with same txid, different wtxid and valid witness for better coverage and to test how these transactions are relayed over different kinds of P2P connections (ex: see #29415 (review))

Ideally, I would like to extend create_malleated_version() function so that it can be directly used in private broadcast PR. However for these kind of malleations, I think we need to specially craft a pair of transactions. If anyone has ideas on how to better modularise this so that it can be reused in other tests, I would love to hear them!

DrahtBot commented at 8:15 am on April 30, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32385.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	vasild, theStack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Apr 30, 2025

in test/functional/test_framework/key.py:189 in e2f06b222d outdated

180@@ -181,6 +181,8 @@ def sign_ecdsa(self, msg, low_s=True, rfc6979=False):
181         s = (pow(k, -1, ORDER) * (z + self.secret * r)) % ORDER
182         if low_s and s > secp256k1.GE.ORDER_HALF:

laanwj commented at 7:52 am on May 1, 2025:

Could use elif, but as the effect is the same, better to roll this into one if, i think:

0if (low_s and s > secp256k1.GE.ORDER_HALF) or (not low_s and s < secp256k1.GE.ORDER_HALF):
1    s = ORDER - s

All in all this defines not low_s as “high_s” instead of “any s”. This doesn’t cause problems as the test framework doesn’t use low_s=False anywhere else. Still, it might be clearer to add a new flag?

stratospher commented at 9:00 am on May 5, 2025:

Could use elif, but as the effect is the same, better to roll this into one if, i think:

nice, I’ve used this.

All in all this defines not low_s as “high_s” instead of “any s”. This doesn’t cause problems as the test framework doesn’t use low_s=False anywhere else. Still, it might be clearer to add a new flag?

oh right - just added a comment for now, since the low_s=False option isn’t used elsewhere and can’t think of a scenario where non-determinism from allowing “any s” (returning high s sometimes, low s other times) would be needed. here’s the diff.

laanwj commented at 9:12 am on May 5, 2025:

Right it’s fine, it’s documented well enough now, if anyone needs that functionality in the future they can add it then.

stratospher force-pushed on May 5, 2025

test: add test for malleated transaction with valid witness

- create_malleated_version() function in the functional tests
  creates transactions with same txid, different wtxid but the
  witness is invalid
- it is useful in the tests to have transactions with same
  txid, different wtxid and valid witness for better coverage
  and (in other PRs) to test how these transactions are
  relayed in different kinds of P2P connections
  (ex: private broadcast connections)

2c668bd502

stratospher force-pushed on May 5, 2025

in test/functional/p2p_segwit.py:2111 in 2c668bd502

2106+
2107+        # 2. Create and sign a transaction spending from the P2WPKH output (bad high-S signature)
2108+        spending_tx = create_tx(funding_p2wpkh_tx.sha256, 0, amount - 1000, script_pubkey)
2109+        pubkey_hash = hash160(pubkey)
2110+        p2pkh_script = keyhash_to_p2pkh_script(pubkey_hash)
2111+        sign_witness_input(spending_tx, 0, p2pkh_script, amount, low_s=False)

vasild commented at 6:04 pm on May 5, 2025:

I can imagine that malleating a transaction does not require the private key and a re-sign.

Currently there is this function:

0def create_malleated_version(self, tx)

which fills garbage. Could it be extended to take a boolean argument, indicating whether to fill with garbage like now, or flip s to ORDER - s? For this it would have to extract s like done on line 97 here:

https://github.com/bitcoin/bitcoin/blob/baa848b8d38187ce6b24a57cfadf1ea180209711/test/functional/test_framework/key.py#L59-L97

flip it to ORDER - s and re-do the DER encoding like:

https://github.com/bitcoin/bitcoin/blob/baa848b8d38187ce6b24a57cfadf1ea180209711/test/functional/test_framework/key.py#L184-L189

For the caller it would be useful to know if the new flipped s is low or high. I guess this can be indicated somehow by create_malleated_version() or there could be a separate function which takes a signed transaction and returns whether s is low or high.

in test/functional/p2p_segwit.py:2128 in 2c668bd502

2123+        assert_not_equal(prev_wtxid, spending_tx.getwtxid())
2124+        # Transaction should now be accepted
2125+        test_transaction_acceptance(self.nodes[1], self.test_node, spending_tx, with_witness=True, accepted=True)
2126+        self.generate(self.nodes[0], 1)
2127+
2128+        # Example 2: P2WSH 1-of-2 multisig transaction where each signature applied to tx results in different wtxid

vasild commented at 6:08 pm on May 5, 2025:

Isn’t multisig kind of out-of-scope for creating a valid malleated transaction? I don’t mind some extra tests, but those might make this PR more difficult to review. Maybe omit those if there is no review interest for some time.

stratospher commented at 6:37 pm on May 5, 2025:

oh included it because high-s transactions are non-standard and I was worried they wouldn’t be relayed and we couldn’t test private broadcast behaviour.

vasild commented at 5:05 am on May 6, 2025:

Now I see! Multisig is another way to create “same_txid, different_wtxid” even though it does not fiddle with the s value. And so, both transactions are relayable. Cool. Best would be to have a function that produces those two transactions. That would need the private keys. I guess better be separate from create_malleated_version().

vasild commented at 6:09 pm on May 5, 2025: contributor

Concept ACK, thank you for looking into this!

theStack commented at 12:57 pm on May 13, 2025: contributor

Concept ACK

Seems useful to create a pair of valid “same-txid-different-wtxid” transactions for testing purposes.

Didn’t try that yet, but I think another possibility to create these without having to involve signatures at all would be a “trivial math puzzle”-like locking script of e.g. “OP_ADD 5 OP_EQUAL” (in a p2wsh or p2tr script-path) and swapped witness data elements ([1,4] and [4,1]) in the spending txs.

test: add test for malleated transaction with valid witness #32385

Code Coverage & Benchmarks

Reviews