tests: Add witness commitment if we have a witness transaction in FullBlockTest.update_block() #31823

1. 
   Christewart commented at 6:05 pm on February 7, 2025: contributor

   This is useful for test cases where we want to test logic invalid blocks that contain witness transactions. If we don’t add the witness commitment as per BIP141, blocks will be rejected with the error Block mutated.

   This change was needed in https://github.com/ajtowns/bitcoin/pull/13 which is a soft fork proposal to disallow 64 byte transactions. We want to test that 64 byte transactions serialized without the witness are invalid. If we do not have this change, we cannot directly test the logic that rejects 64 byte transactions.

   I decided to PR this upstream as many soft fork proposals may not see the light of day, but this functionality seems strictly additive to the test framework.

2. 
   DrahtBot commented at 6:05 pm on February 7, 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/31823.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	kevkevinpal

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

3. DrahtBot added the label Tests on Feb 7, 2025
4. 
   rishkwal commented at 11:13 pm on February 8, 2025: none

   Hi, looking at feature_block.py — it doesn’t seem to contain any SegWit-specific validation tests. They remain in feature_segwit.py, p2p_segwit.py, etc; although I’m not completely confident about the scope of this file. I have a couple of questions(avoid if irrelevant):

   1. Is there any test case that can be covered in the current implementation that needs this change to be implemented?
   2. Wouldn’t it be better to create a new test file like feature_<soft_fork>.py to test the consensus changes made in a new soft fork, following the convention(feature_segwit,feature_taproot,etc)? Also, why disallow 64 byte transactions? Thanks
5. 
   Christewart commented at 6:15 pm on February 9, 2025: contributor

   1. Is there any test case that can be covered in the current implementation that needs this change to be implemented?

   There is not currently. This short coming in the test framework was discovered while working on test cases for disallowing 64 byte transactions in the bitcoin blockchain. This python test case would not fail correctly if this change wasn’t added to the test framework: https://github.com/ajtowns/bitcoin/pull/13/files#diff-d390e9e12bfa34a462fdff9f1894d7dc3edb45c26cf55df486d92864a3052fafR139

   2. Wouldn't it be better to create a new test file like `feature_<soft_fork>.py` to test the consensus changes made in a new soft fork, following the convention(`feature_segwit`,`feature_taproot`,etc)?
   

   This PR is not proposing a soft fork. This PR makes the update_block function BIP141 compliant.

   Also, why disallow 64 byte transactions?

   Here are some resources on why 64 byte transactions are problematic.

   • https://github.com/Christewart/bips/blob/2024-12-20-64bytetxs/bip-XXXX.mediawiki
   • https://bitslog.com/2018/06/09/leaf-node-weakness-in-bitcoin-merkle-tree-design/
   • https://delvingbitcoin.org/t/great-consensus-cleanup-revival/710/41?u=chris_stewart_5
   • https://github.com/Christewart/bips/blob/2024-12-20-64bytetxs/bip-XXXX/2-BitcoinMerkle.pdf
6. in test/functional/feature_block.py:1406 in d39e7287e8 outdated

   1400@@ -1400,6 +1401,9 @@ def update_block(self, block_number, new_transactions):
   1401         self.add_transactions_to_block(block, new_transactions)
   1402         old_sha256 = block.sha256
   1403         block.hashMerkleRoot = block.calc_merkle_root()
   1404+        has_witness_tx = any(not tx.wit.is_null() for tx in block.vtx)
   1405+        if has_witness_tx:
   1406+            add_witness_commitment(block)
   

   ---

   ---

   

   kevkevinpal commented at 2:44 am on February 16, 2025:

   Right now this code would not be executed at all, do you think it makes sense to add a new invalid tx with witness data to invalid_txs.py?

   Similar to what you had here this should execute add_witness_commitment

   ---

   

   Christewart commented at 3:03 pm on February 27, 2025:

   Hi @kevkevinpal !

   I took your suggestion and added the test in 6109bc5

7. tests: Add witness commitment if we have a witness transaction in FullBlockTest.update_block() 6109bc529e
8. Christewart force-pushed on Feb 27, 2025
9. Christewart requested review from kevkevinpal on Feb 27, 2025
10. 
    kevkevinpal commented at 5:36 pm on February 27, 2025: contributor

    ACK 6109bc529e234b464d406495bb9644254ca7fc9e
11. 
    kevkevinpal commented at 5:44 pm on February 27, 2025: contributor

    The CI failure looks unrelated to your change
12. 
    Christewart commented at 6:08 pm on February 27, 2025: contributor

    The CI failure looks unrelated to your change

    Maybe the gods (AKA bitcoin core maintainers 👼 📿 ) will restart the build for me :pray: 😄

13. in test/functional/data/invalid_txs.py:140 in 6109bc529e

    135+# but doesnt spend a segwit output
    136+class ExtraWitness(BadTxTemplate):
    137+    expect_disconnect = False
    138+    valid_in_block = False
    139+    block_reject_reason = "mandatory-script-verify-flag-failed (Witness provided for non-witness script)"
    140+
    

    ---

    ---

    

    maflcko commented at 8:44 am on February 28, 2025:

    Missing reject_reason field?

    ---

    

    Christewart commented at 5:19 pm on February 28, 2025:

    It is my understanding that the feature_block.py test suites turns on -acceptnonstdtxn=1 which turns off this policy check. Thus we don’t receive any error message when submitting the transaction to the mempool.

    https://github.com/bitcoin/bitcoin/blob/3c1f72a36700271c7c1293383549c3be29f28edb/src/validation.cpp#L886

14. Christewart requested review from maflcko on Mar 3, 2025

Contributors
Christewart DrahtBot rishkwal kevkevinpal maflcko

Review Requested
maflcko kevkevinpal

Labels
Tests