test: MiniWallet: add P2TR support and use it per default #23371
Conversation
theStack
force-pushed
the
202110-test-MiniWallet-change_P2WSH_to_P2TR
branch
from
682005b
to
27c3783
Compare
theStack
force-pushed
the
202110-test-MiniWallet-change_P2WSH_to_P2TR
branch
from
27c3783
to
047e185
Compare
|
Force-pushed with suggestions by MarcoFalke (#23371 (comment), #23371 (comment), #23371 (comment)) and also adapted the PR description accordingly. |
|
Concept ACK, nice |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
|
Great idea, Concept and Approach ACK |
theStack
force-pushed
the
202110-test-MiniWallet-change_P2WSH_to_P2TR
branch
from
047e185
to
c026917
Compare
|
Force-pushed with a fix for the failing test |
…n selection for coinbase UTXOs (oldest first) d2c4904 test: MiniWallet: more deterministic coin selection for coinbase UTXOs (oldest first) (Sebastian Falbesoner) Pull request description: The coin selection strategy for MiniWallet is quite straight-forward: simply pick a single UTXO with the largest value: https://github.com/bitcoin/bitcoin/blob/ab25ef8c7f767258d5fe44f53b35ad8bd51ed5cd/test/functional/test_framework/wallet.py#L173-L174 If there are several candidates with the same value, however, it is not clear which one is taken. This can be particularly problematic for coinbase outputs with fixed block subsidy, since spending could lead to a `bad-txns-premature-spend-of-coinbase` reject if an UTXO from a too-recent block is picked. Introduce block height as second criteria (saved in `self._utxos` in the methods `generate(...)` and `rescan_utxos(...)`), in order to avoid potential issues with coinbases that are not matured yet. If there is a tie between coinbase UTXOs and non-coinbase UTXOs (the latter are added via `scan_tx(...)`), prefer the non-coinbase UTXOs, since those don't need to mature. The issue came up while refactoring the test rpc_blockchain.py, see bitcoin/bitcoin#23371 (comment) (PR #23371). ACKs for top commit: MarcoFalke: review ACK d2c4904 shaavan: ACK d2c4904 Tree-SHA512: 15d67b42fb8b77fd53022ea2ab8a6ed2b615567f3ce73bab16c06bfcb687c1a04dcb0360d0c2287c526b604cd3ac5eef7b14ce46fc31e23047ce1a3290027306
theStack
force-pushed
the
202110-test-MiniWallet-change_P2WSH_to_P2TR
branch
from
c026917
to
8dfe7d4
Compare
|
Rebased on master (after the merge of #23375, the call to |
…ion for coinbase UTXOs (oldest first) d2c4904 test: MiniWallet: more deterministic coin selection for coinbase UTXOs (oldest first) (Sebastian Falbesoner) Pull request description: The coin selection strategy for MiniWallet is quite straight-forward: simply pick a single UTXO with the largest value: https://github.com/bitcoin/bitcoin/blob/ab25ef8c7f767258d5fe44f53b35ad8bd51ed5cd/test/functional/test_framework/wallet.py#L173-L174 If there are several candidates with the same value, however, it is not clear which one is taken. This can be particularly problematic for coinbase outputs with fixed block subsidy, since spending could lead to a `bad-txns-premature-spend-of-coinbase` reject if an UTXO from a too-recent block is picked. Introduce block height as second criteria (saved in `self._utxos` in the methods `generate(...)` and `rescan_utxos(...)`), in order to avoid potential issues with coinbases that are not matured yet. If there is a tie between coinbase UTXOs and non-coinbase UTXOs (the latter are added via `scan_tx(...)`), prefer the non-coinbase UTXOs, since those don't need to mature. The issue came up while refactoring the test rpc_blockchain.py, see bitcoin#23371 (comment) (PR bitcoin#23371). ACKs for top commit: MarcoFalke: review ACK d2c4904 shaavan: ACK d2c4904 Tree-SHA512: 15d67b42fb8b77fd53022ea2ab8a6ed2b615567f3ce73bab16c06bfcb687c1a04dcb0360d0c2287c526b604cd3ac5eef7b14ce46fc31e23047ce1a3290027306
theStack
force-pushed
the
202110-test-MiniWallet-change_P2WSH_to_P2TR
branch
from
8dfe7d4
to
041abfe
Compare
|
Rebased on master. |
|
Code review ACK 041abfe |
…se it per default 041abfe test: MiniWallet: add P2TR support and use it per default (Sebastian Falbesoner) 4a2edf2 test: generate blocks to MiniWallet address in rpc_blockchain.py (Sebastian Falbesoner) Pull request description: Taproot activates in [about 19 days](https://taproot.watch/) (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). ACKs for top commit: laanwj: Code review ACK 041abfe Tree-SHA512: 876a5b0595333f9c96c68d5ecf2b4530aee2715aebb75a953f4f75ca12258bd7239210fcfa1ae044bee91489804c9c2f2a6a335bd46c3ac701873d32e3a4f49d
|
🐙 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". |
|
Why was this closed? |
Because it was merged. |
|
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... |
|
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. |
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_TRUEtocreate_deterministic_address_bcrt1_p2tr_op_true()[0]accordingly (see methodBitcoinTestFramework._initialize_chain(...)). Note that the pre-mined chain is cached locally, so you probably have to delete the./test/cachefolder first for the tests to pass again.In order to avoid unnecessary renames, the import of
ADDRESS_BCRT1_P2WSH_OP_TRUEis eliminated in rpc_blockchain.py by generating blocks directly to the MiniWallet address by using theself.generate(self.wallet, ...)interface (see first commit).