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

1. 
   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!

2. 
   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. A summary of reviews will appear here.

3. DrahtBot added the label Tests on Apr 30, 2025
4. 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.
5. stratospher force-pushed on May 5, 2025
6. 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
7. stratospher force-pushed on May 5, 2025

Contributors
stratospher DrahtBot laanwj

Labels
Tests