test: MiniWallet: add P2TR support and use it per default #23371

pull theStack wants to merge 2 commits into bitcoin:master from theStack:202110-test-MiniWallet-change_P2WSH_to_P2TR changing 7 files +52 −28
  1. theStack commented at 10:29 am on October 27, 2021: member

    Taproot activates in about 19 days (2716 blocks), and it’d be nice if we set a good example and also support it in our MiniWallet. This PR changes the default mode from P2WSH (segwit v0 output, bech32 address) to P2TR (segwit v1 output, bech32m address) transactions type with the anyone-can-spend policy, i.e. a witness script of OP_TRUE. The transition is actually quite painless, one only needs one extra piece in the form of a internal public key that is passed in the control block on the witness stack, in order to trigger script-path spending. To keep things simple, the lowest possible valid x-only-public key with the value of 1 was chosen as internal key. Since many tests expect to find outputs for the default scriptPubKey of MiniWallet in the pre-mined chain of the test framework, the generation address is also changed from ADDRESS_BCRT1_P2WSH_OP_TRUE to create_deterministic_address_bcrt1_p2tr_op_true()[0] accordingly (see method BitcoinTestFramework._initialize_chain(...)). Note that the pre-mined chain is cached locally, so you probably have to delete the ./test/cache folder first for the tests to pass again.

    In order to avoid unnecessary renames, the import of ADDRESS_BCRT1_P2WSH_OP_TRUE is eliminated in rpc_blockchain.py by generating blocks directly to the MiniWallet address by using the self.generate(self.wallet, ...) interface (see first commit).

  2. fanquake added the label Tests on Oct 27, 2021
  3. in test/functional/feature_rbf.py:50 in 682005b5f9 outdated
    46@@ -47,7 +47,7 @@ def set_test_params(self):
    47     def run_test(self):
    48         self.wallet = MiniWallet(self.nodes[0])
    49         # the pre-mined test framework chain contains coinbase outputs to the
    50-        # MiniWallet's default address ADDRESS_BCRT1_P2WSH_OP_TRUE in blocks
    51+        # MiniWallet's default address ADDRESS_BCRT1_P2TR_OP_TRUE in blocks
    


    MarcoFalke commented at 10:37 am on October 27, 2021:
    0        # MiniWallet's default address in blocks
    

    nit: Maybe remove, since it is not needed for context?


    theStack commented at 11:52 am on October 27, 2021:
    Thanks, done.
  4. theStack force-pushed on Oct 27, 2021
  5. in test/functional/rpc_blockchain.py:28 in 27c3783940 outdated
    24@@ -25,7 +25,7 @@
    25 import os
    26 import subprocess
    27 
    28-from test_framework.address import ADDRESS_BCRT1_P2WSH_OP_TRUE
    29+from test_framework.address import ADDRESS_BCRT1_P2TR_OP_TRUE
    


    MarcoFalke commented at 10:39 am on October 27, 2021:
    Maybe remove this and replace it with miniwallet.address() for clarity?

    theStack commented at 11:54 am on October 27, 2021:
    Good idea. I added another commit (before the P2TR introduction) that generates directly to the MiniWallet in rpc_blockchain.py, using self.generate(self.wallet, ....).
  6. MarcoFalke commented at 10:44 am on October 27, 2021: member
    nice
  7. in test/functional/test_framework/address.py:23 in 27c3783940 outdated
    16@@ -16,8 +17,11 @@
    17 
    18 ADDRESS_BCRT1_UNSPENDABLE = 'bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3xueyj'
    19 ADDRESS_BCRT1_UNSPENDABLE_DESCRIPTOR = 'addr(bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3xueyj)#juyq9d97'
    20-# Coins sent to this address can be spent with a witness stack of just OP_TRUE
    21+# Coins sent to this addresses can be spent with a witness stack of just OP_TRUE
    22+# (and the control block with internal public key, in case of P2TR)
    23 ADDRESS_BCRT1_P2WSH_OP_TRUE = 'bcrt1qft5p2uhsdcdc3l2ua4ap5qqfg4pjaqlp250x7us7a8qqhrxrxfsqseac85'
    24+ADDRESS_BCRT1_P2TR_OP_TRUE = 'bcrt1p9yfmy5h72durp7zrhlw9lf7jpwjgvwdg0jr0lqmmjtgg83266lqsekaqka'
    


    MarcoFalke commented at 10:47 am on October 27, 2021:

    nit: Maybe move the comment on how to create the address here?

    Or alternatively simply make this a method that can be called to generate the address on demand?


    theStack commented at 11:57 am on October 27, 2021:
    Thanks, done. Added a method create_deterministic_address_bcrt1_p2tr_op_true that returns both address and internal key, in order to avoid another import from the address module.
  8. theStack force-pushed on Oct 27, 2021
  9. theStack commented at 12:03 pm on October 27, 2021: member
    Force-pushed with suggestions by MarcoFalke (https://github.com/bitcoin/bitcoin/pull/23371#discussion_r737334936, #23371 (review), #23371 (review)) and also adapted the PR description accordingly.
  10. in test/functional/rpc_blockchain.py:430 in 047e185d8f outdated
    430-
    431         fee_per_byte = Decimal('0.00000010')
    432         fee_per_kb = 1000 * fee_per_byte
    433 
    434-        miniwallet.send_self_transfer(fee_rate=fee_per_kb, from_node=node)
    435+        self.wallet.rescan_utxos()
    


    MarcoFalke commented at 12:13 pm on October 27, 2021:
    I think this is done by generate, so can be dropped?

    theStack commented at 12:18 pm on October 27, 2021:
    Turned out that without the UTXOs from the pre-mined chain, the coinbases can’t be spent due to not being mature yet ('reject-reason': 'bad-txns-premature-spend-of-coinbase').

    theStack commented at 12:23 pm on October 27, 2021:
    Hm, maybe that’s due to a coin selection issue in MiniWallet – currently it only selects UTXOs based on value, maybe it should also take age as second criteria, i.e. oldest ones first. Will take a closer look.

    theStack commented at 4:05 pm on October 27, 2021:
    Opened a separate PR for this issue, since it isn’t directly related to the P2TR introduction: #23375. After merging this, the rescan_utxos call can be removed from the rpc_blockchain.py test. Probably there are also other places where extra generate calls can be removed, though I didn’t check out yet.
  11. laanwj commented at 12:23 pm on October 27, 2021: member
    Concept ACK, nice
  12. DrahtBot commented at 2:35 pm on October 27, 2021: member

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

    Conflicts

    No conflicts as of last run.

  13. mjdietzx commented at 2:59 pm on October 27, 2021: contributor
    Great idea, Concept and Approach ACK
  14. theStack force-pushed on Oct 28, 2021
  15. theStack commented at 5:48 pm on October 28, 2021: member
    Force-pushed with a fix for the failing test mempool_compatibility.py. On the previous release node (v0.19.1), the transaction from the newer node can’t be added to the mempool, as CheckInputs() fails with non-mandatory-script-verify-flag (Witness version reserved for soft-fork upgrades). This is due to the script verification flag SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM being set. I.e. for this test, we can’t use the default MiniWallet mode (P2TR), so it is changed to the raw P2PK mode instead.
  16. MarcoFalke referenced this in commit 8bac3b1096 on Oct 29, 2021
  17. in test/functional/rpc_blockchain.py:379 in c026917a8a outdated
    376         assert_equal(self.nodes[0].getblockcount(), HEIGHT + 6)
    377         self.log.debug('Node should not stop at this height')
    378         assert_raises(subprocess.TimeoutExpired, lambda: self.nodes[0].process.wait(timeout=3))
    379         try:
    380-            self.generatetoaddress(self.nodes[0], 1, ADDRESS_BCRT1_P2WSH_OP_TRUE)
    381+            self.generate(self.wallet, 1)
    


    MarcoFalke commented at 11:02 am on October 29, 2021:

    I think this can’t use the miniwallet because the node will shut down while generate. Alternative would be to add the exception below to the pass:

    0                                   test_framework.authproxy.JSONRPCException: non-JSON HTTP response with '503 Service Unavailable' from server (-342)
    

    theStack commented at 12:15 pm on October 29, 2021:
    Thanks, tried to the decipher the failing CI log yesterday without success, now it makes more sense. Decided to go the easy route by simply keeping the generatetoaddress call to the MiniWallet address (i.e. coinbase is not added to its UTXO set, but we don’t need it).
  18. theStack force-pushed on Oct 29, 2021
  19. theStack commented at 12:19 pm on October 29, 2021: member
    Rebased on master (after the merge of #23375, the call to rescan_utxos is not needed anymore in rpc_blockchain.py) and fixed the cause for the failing CI log due to node shutdown while generate (https://github.com/bitcoin/bitcoin/pull/23371#discussion_r739139394). All CI instances should (hopefully) pass now.
  20. sidhujag referenced this in commit 3624bcd739 on Oct 29, 2021
  21. DrahtBot added the label Needs rebase on Nov 9, 2021
  22. test: generate blocks to MiniWallet address in rpc_blockchain.py 4a2edf2bf7
  23. test: MiniWallet: add P2TR support and use it per default 041abfebe4
  24. theStack force-pushed on Nov 9, 2021
  25. theStack commented at 11:35 am on November 9, 2021: member
    Rebased on master.
  26. DrahtBot removed the label Needs rebase on Nov 9, 2021
  27. laanwj commented at 10:49 am on November 10, 2021: member
    Code review ACK 041abfebe49ae5e3e882c00cc5caea1365a27a49
  28. laanwj referenced this in commit 2539980e1d on Nov 10, 2021
  29. DrahtBot added the label Needs rebase on Nov 10, 2021
  30. DrahtBot commented at 11:52 am on November 10, 2021: member

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

  31. MarcoFalke closed this on Nov 10, 2021

  32. theStack deleted the branch on Nov 10, 2021
  33. sidhujag referenced this in commit b15206f609 on Nov 10, 2021
  34. michaelfolkson commented at 12:43 pm on November 11, 2021: contributor
    Why was this closed?
  35. fanquake commented at 12:48 pm on November 11, 2021: member

    Why was this closed?

    Because it was merged.

  36. michaelfolkson commented at 12:53 pm on November 11, 2021: contributor
    Ok I must be being dumb :) But this is a PR that is marked as closed on GitHub rather than marked as merged. It isn’t an issue that can be closed when a separate PR fixes the issue. It is a PR and PRs are marked as merged rather than closed if they are merged…
  37. fanquake commented at 1:01 pm on November 11, 2021: member
    Sometimes GitHub just doesn’t work properly, and PRs that have been merged via the merge script don’t actually get displayed as “merged” on GitHub. It’s not entirely clear why this happens, and iirc there is an issue open for it on GitHubs support forum. This has happened number of time over the last few months.
  38. DrahtBot locked this on Nov 11, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-29 01:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me