test: refactor mempool_accept_wtxid #33067

1. 
   naiyoma commented at 0:34 am on July 26, 2025: contributor

   This PR improves mempool_accept_wtxid.py by using a pre-mined chain instead of generating new blocks, switching to MiniWallet to avoid RPC calls for signing transactions, replacing class ValidWitnessMalleatedTx with valid_witness_malleate_tx() function, and simplifying the code by using txid.hex directly.
2. DrahtBot added the label Tests on Jul 26, 2025
3. 
   DrahtBot commented at 0:34 am on July 26, 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/33067.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	enirox001, cedwies
   Concept ACK	yuvicc, stratospher

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

4. test: use pre-generated chain 26577cac8b
5. naiyoma force-pushed on Jul 26, 2025
6. 
   cedwies commented at 7:42 pm on July 26, 2025: none

   Tested on MacOS 15.5 (M2, Debug build)

   • mempool_accept_wtxid.py passes
   • Confirmed with grep that build_parent_tx() helper is not referenced anywhere else
   • nit: two spaces sneaked into assert_equal(node.getmempoolentry(…), child_one.wtxid_hex) (double space before child_one)

   ACK 9b37c1f

7. 
   yuvicc commented at 7:58 pm on July 27, 2025: contributor

   Concep ACK This would make the test consistent with mempool_accept
8. 
   maflcko commented at 8:29 am on July 28, 2025: member

   This PR improves mempool_accept_wtxid.py by:

   1. Using a pre-mined chain instead of generating new blocks [522bf76](https://github.com/bitcoin/bitcoin/commit/522bf76d7d8058872a008a721831da264881746d)
   
   2. Using MiniWallet to avoid RPC calls for signing transactions [4ec5ae9](https://github.com/bitcoin/bitcoin/commit/4ec5ae9fe126ffabcd429277092e3b27f483d430)
   
   3. Removing child txid variables and using txid.hex directly [361ebd5](https://github.com/bitcoin/bitcoin/commit/361ebd56eb817b1e054ee1aacc87bf60c06c6b94)
   

   Not sure why the pull description is just a copy-paste of the commit list (with outdated commit ids). It would be better to remove those. If anyone wants to look at the list of commit ids, they can already see them directly, no need to copy them.

9. in test/functional/mempool_accept_wtxid.py:39 in 671e555fb0 outdated

   37         assert_equal(node.getmempoolinfo()['size'], 0)
   38 
   39         self.log.info("Submit parent with multiple script branches to mempool")
   40         txgen = ValidWitnessMalleatedTx()
   41-        parent = txgen.build_parent_tx(txid, 9.99998 * COIN)
   42+        parent = wallet.send_to(from_node=node,scriptPubKey=txgen.get_script_pubkey(),amount=int(9.99998 * COIN),fee=1000)
   

   ---

   ---

   

   stratospher commented at 3:34 pm on July 28, 2025:

   0        parent = wallet.send_to(from_node=node, scriptPubKey=txgen.get_script_pubkey(), amount=int(9.99998 * COIN), fee=1000)
   

   style nit: space after , (here and in other places too)

10. 
    stratospher commented at 5:06 pm on July 28, 2025: contributor

    Concept ACK. I like how you’ve used MiniWallet to remove the signing bits (signrawtransactionwithkey) which needed the private key.

    You can simplify this further. ValidWitnessMalleatedTx returned parent transaction and children transaction in 2 different steps because the children transactions needed the parent’s txid which required signing with private key. Now since we don’t do signrawtransactionwithkey, you can replace class ValidWitnessMalleatedTx with a function valid_witness_malleate_tx() which does everything done previously in the class and return parent, child_one, child_two together. I think this would remove the concern in #32385 (review)!

11. test: replace class ValidWitnessMalleatedTx with function d1a2e27857
12. test: remove child_one/child_two (w)txid variables 0cbcdcaf5a
13. naiyoma force-pushed on Jul 30, 2025
14. 
    enirox001 commented at 4:06 pm on August 1, 2025: contributor

    ACK 0cbcdca

    Solid test refactor with significant performance improvement:

    • Using pre-mined chain reduces runtime from ~36s to ~7-10s (70%+ faster)
    • MiniWallet integration eliminates manual signing and key management
    • Class-to-function conversion appropriate for single-use case
    • Removes unnecessary variable assignments, improves readability

    Test coverage remains identical, CI passes.

15. DrahtBot requested review from cedwies on Aug 1, 2025
16. DrahtBot requested review from stratospher on Aug 1, 2025
17. 
    cedwies commented at 3:44 pm on August 3, 2025: none

    Changes look great.

    Nice to see the valid_witness_malleate_tx helper instead of the ValidWitnessMalleatedTx class. Makes the test’s logic much clearer IMO.

    ACK 0cbcdca

Contributors
naiyoma DrahtBot cedwies yuvicc maflcko stratospher enirox001

Review Requested
stratospher

Labels
Tests