backport: bitcoin#18788, #18974, #19230, #19239, #19441, #19568, #19668, #20226, #21049, #22446 (descriptor wallets part II) #5911
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
knst
force-pushed
the
bp-descriptors-2
branch
from
e9029bf to
924ea39
Compare
|
Any objects to delaying this to post v20.1? |
no, it can't be merged before #5579 anyway. Has no critical fixes/improvements, just steady improvement of 5579 |
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
bp-descriptors-2
branch
2 times, most recently
from
0c56480 to
e03cfce
Compare
knst
changed the title
knst
changed the title
|
This pull request has conflicts, please rebase. |
…n not be imported facede1 test: Check that invalid witness destinations can not be imported (MarcoFalke) Pull request description: ACKs for top commit: practicalswift: ACK facede1 -- thanks for adding! Tree-SHA512: 87000606fac2e6f2780ca75cdeeb2dc1f0528d9b8f13e4156e8304ce7a6b1eb014781b6f0c59d11544bf360ba3dc5f99549470b0876132e189b9107f2c6bb38d
…lar dependency c75de5d [TESTS] Move base58 to own module to break circular dependency (Pieter Wuille) Pull request description: I encountered difficulties with the test framework in bitcoin#17977. This fixes them, and I think the change is generally useful. ACKs for top commit: laanwj: Code review ACK c75de5d MarcoFalke: ACK c75de5d according to --color-moved=dimmed-zebra this is a move-only apart from the imports 👒 Tree-SHA512: 9e0493de3e279074f0c70e92c959b73ae30479ad6f2083a3c6bbf4b0191d65ef94854559a5b7c904f5dadc5e93129ed00f6dc0a8ccce6ba7921cd45f7119f74b
3a83a01 [tests] move generate_wif_key to wallet_util.py (John Newbery) b216b0b [tests] sort imports in rpc_createmultisig.py (John Newbery) e380818 Revert "[TESTS] Move base58 to own module to break circular dependency" (John Newbery) Pull request description: generate_wif_key is a wallet utility function. Move it from the EC key module to the wallet util module. This fixes the circular dependency issue in bitcoin#17977 ACKs for top commit: MarcoFalke: ACK 3a83a01 🍪 Tree-SHA512: 24985dffb75202721ccc0c6c5b52f1fa5d1ce7963bccde24389feb913cab4dad0c265274ca67892c46c8b64e6a065a0f23263a89be4fb9134dfefbdbe5c7238a
…tiple cache entries a66a7a1 walletdb: don't reinitialize desc cache with multiple cache entries (Andrew Chow) Pull request description: When loading descriptor caches, we would accidentally reinitialize the descriptor cache when seeing that one already exists. This should have only been initializing the cache when one does not exist. However this code itself is unnecessary as the act of looking up the cache to add to it will initialize it if it didn't already exist. This issue could be hit by trying to load a wallet that had imported a multisig descriptor. The wallet would fail to load. A test has been added to wallet_importdescriptors.py to catch this case. Another test case has also been added to check that loading a wallet with only single key descriptors works. ACKs for top commit: hugohn: tACK [a66a7a1](bitcoin@a66a7a1) jonatack: ACK a66a7a1 meshcollider: Code review ACK a66a7a1 Tree-SHA512: 3df746421a008708eaa3bbbdd12b9ddd3e2ec111d54625a212dca7414b971cc1f6e2b1757b3232c31a2f637d1b1ef43bf3ffa4ac4216646cf1e92db5f79954f1
e7448d6 wallet: Don't override signing errors (Fabian Jahr) Pull request description: While reviewing bitcoin#17204 I noticed that the errors in `input_errors` from `::SignTransaction` where being overridden by `CWallet::SignTransaction`. For example, a Script related error led to incomplete signature data which led to `CWallet::SignTransaction` reporting that keys were missing, which was a less precise error than the original one. Additionally, the error `"Input not found or already spent"` is [duplicated in `sign.cpp`](https://github.com/bitcoin/bitcoin/blob/c7b4968552c704f1e2e9a046911e1207f5af5fe0/src/script/sign.cpp#L481), so the error here is redundant at the moment. So technically the whole error block could be removed, I think. However, this code is affected by the ongoing work on the wallet so there might be a reason why these errors are here. But even if there is a reason to keep them, I don't think existing, potentially more precise errors should be overridden here unless we want to hide them from the users. I am looking for feedback if this is a work in progress state where these errors could be more useful in the future or if they can be removed. On testing: even though [the errors in `CWallet` are covered](https://marcofalke.github.io/btc_cov/total.coverage/src/wallet/wallet.cpp.gcov.html), all tests still pass after removing them. I am not sure if there is a desire to cover these specific error messages, tests in `test/functional/rpc_signrawtransaction.py` seem to aim for a more generic approach. ACKs for top commit: achow101: ACK e7448d6 meshcollider: Code review ACK e7448d6 Tree-SHA512: 3e2bc11d05379d2aef87b093a383d1b044787efc70e35955b2f8ecd028b6acef02f386180566af6a1a63193635f5d685466e2f6141c96326c49ffc5c81ca3e23
647b81b wallet, rpc: add listdescriptors command (Ivan Metlushko) Pull request description: Looking for concept ACKs **Rationale**: allow users to inspect the contents of their newly created descriptor wallets. Currently the command only returns xpubs which is not very useful in itself, but there are multiples ways to extend it: * add an option to export xprv * with bitcoin#19136 it'll be possible to return normalised descriptors suitable for a watch-only purposes The output is compatible with `importdescriptors` command so it could be easily used for backup/recover purposes. **Output example:** ```json [ { "desc": "wpkh(tpubD6NzVbkrYhZ4WW6E2ZETFyNfq2hfF23SKxqSGFvUpPAY58jmmuBybwqwFihAyQPk9KnwTt5516NDZRJ7k5QPeKjy7wuVd5WvXNxwwAs5tUD/*)#nhavpr5h", "timestamp": 1296688602, "active": false, "range": [ 0, 999 ], "next": 0 } ] ``` ACKs for top commit: jonatack: re-ACK 647b81b rebased to master, debug builds cleanly, reviewed diff since last review, tested with a descriptor wallet (and with a legacy wallet) achow101: re-ACK 647b81b Tree-SHA512: 51a3620bb17c836c52cecb066d4fa9d5ff418af56809046eaee0528c4dc240a4e90fff5711ba96e399c6664e00b9ee8194e33852b1b9e75af18061296e19a8a7
51f3752 Add release notes for listdescriptors RPC (Ivan Metlushko) Pull request description: Original PR is bitcoin#20226 ACKs for top commit: jonatack: ACK 51f3752 jonasschnelli: ACK 51f3752 Tree-SHA512: e8091d01b99a3effcd6c1738e7363a44858ba9bcf6bd99bf60f2025a25db83fc8d61354ab2002365b56071b9f3693c7d534153a259b5ebc91cbcf8d13f6555f1
… wallets c7b7e0a tests: Make only desc wallets for wallet_multwallet.py --descriptors (Andrew Chow) d4b67ad Avoid creating legacy wallets in wallet_importdescriptors.py (Andrew Chow) 6c9c12b Update feature_backwards_compatibility for descriptor wallets (Andrew Chow) 9a4c631 Update wallet_labels.py to not require descriptors=False (Andrew Chow) 242aed7 tests: Add a --legacy-wallet that is mutually exclusive with --descriptors (Andrew Chow) 388053e Disable some tests for tool_wallet when descriptors (Andrew Chow) 47d3243 Make raw multisig tests legacy wallet only in rpc_rawtransaction.py (Andrew Chow) 59d3da5 Do addmultisigaddress tests in legacy wallet mode in wallet_address_types.py (Andrew Chow) 25bc5dc Use importdescriptors when in descriptor wallet mode in wallet_createwallet.py (Andrew Chow) 0bd1860 Avoid dumpprivkey and watchonly behavior in rpc_signrawtransaction.py (Andrew Chow) 08067ae Add script equivalent of functions in address.py (Andrew Chow) 8696888 Add descriptor wallet output to tool_wallet.py (Andrew Chow) 3457679 Use separate watchonly wallet for multisig in feature_nulldummy.py (Andrew Chow) a42652e Move import and watchonly tests to be legacy wallet only in wallet_balance.py (Andrew Chow) 4b87190 Use importdescriptors for descriptor wallets in wallet_bumpfee.py (Andrew Chow) c2711e4 Avoid dumpprivkey in wallet_listsinceblock.py (Andrew Chow) 553dbf9 Make import tests in wallet_listtransactions.py legacy wallet only (Andrew Chow) dc81418 Use a separate watchonly wallet in rpc_fundrawtransaction.py (Andrew Chow) a357111 Update wallet_importprunedfunds to avoid dumpprivkey (Andrew Chow) Pull request description: I went through all the tests and checked whether they passed with descriptor wallets. This partially informed some changes in bitcoin#16528. Some tests needed changes to work with descriptor wallets. These were primarily due to import and watchonly behavior. There are some tests and test cases that only test legacy wallet behavior so those tests won't be run with descriptor wallets. This PR updates more tests to have to the `--descriptors` switch in `test_runner.py`. Additionally a mutually exclusive `--legacy-wallet` option has been added to force legacy wallets. This does nothing currently but will be useful in the future when descriptor wallets are the default. For the tests that rely on legacy wallet behavior, this option is being set so that we don't forget in the future. Those tests are `feature_segwit.py`, `wallet_watchonly.py`, `wallet_implicitsegwit.py`, `wallet_import_with_label.py`, and `wallet_import_with_label.py`. If you invert the `--descriptors`/`--legacy-wallet` default so that descriptor wallets are the default, all tests (besides the legacy wallet specific ones) will pass. ACKs for top commit: MarcoFalke: review ACK c7b7e0a 🎿 laanwj: ACK c7b7e0a Tree-SHA512: 2f4e87815005d1d0a2543ea7947f7cd7593d8cf5312228ef85f8e096f19739b225769961943049cb44f6f07a35b8de988e2246ab9aca5bb5a0b2e62694d5637d
…t compiled 0c845e3 test: Fix wallet_listdescriptors.py if bdb is not compiled (Hennadii Stepanov) Pull request description: If build system is configured `--without-bdb`, the `wallet_listdescriptors.py` fails: ``` $ test/functional/wallet_listdescriptors.py --descriptors 2021-07-14T13:20:52.931000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_02p7o1c9 2021-07-14T13:21:23.377000Z TestFramework (INFO): Test that the command is not available for legacy wallets. 2021-07-14T13:21:23.381000Z TestFramework (ERROR): JSONRPC error Traceback (most recent call last): File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/test_framework.py", line 128, in main self.run_test() File "test/functional/wallet_listdescriptors.py", line 34, in run_test node.createwallet(wallet_name='w1', descriptors=False) File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/test_node.py", line 685, in createwallet return self.__getattr__('createwallet')(wallet_name, disable_private_keys, blank, passphrase, avoid_reuse, descriptors, load_on_startup, external_signer) File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__ return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs) File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/authproxy.py", line 146, in __call__ raise JSONRPCException(response['error'], status) test_framework.authproxy.JSONRPCException: Compiled without bdb support (required for legacy wallets) (-4) 2021-07-14T13:21:23.436000Z TestFramework (INFO): Stopping nodes 2021-07-14T13:21:24.092000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_02p7o1c9 2021-07-14T13:21:24.092000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_02p7o1c9/test_framework.log 2021-07-14T13:21:24.092000Z TestFramework (ERROR): 2021-07-14T13:21:24.092000Z TestFramework (ERROR): Hint: Call /home/hebasto/GitHub/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_02p7o1c9' to consolidate all logs 2021-07-14T13:21:24.092000Z TestFramework (ERROR): 2021-07-14T13:21:24.092000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log. 2021-07-14T13:21:24.092000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues 2021-07-14T13:21:24.092000Z TestFramework (ERROR): ``` This PR fixes this issue. Also see bitcoin#20267. ACKs for top commit: achow101: ACK 0c845e3 Tree-SHA512: d7116a9ae30c7b7e3f55f55d2eea66f9e293c38d6757ed66d0477e4256ff5fedca5ddedafa0ef0c09f4dc1f7f973163e5a46090da26b067fdddbd9ea2ee76633
Comment on lines
+101
to
+108
| # Check persistence of data and that loading works correctly | ||
| w1.unloadwallet() | ||
| self.nodes[1].loadwallet('w1') | ||
| test_address(w1, | ||
| key.p2pkh_addr, | ||
| solvable=True, | ||
| ismine=True, | ||
| labels=["Descriptor import test"]) |
There was a problem hiding this comment.
why is this ordering different?
There was a problem hiding this comment.
The main idea of this test is to test this scenario: import+test+unload+load and retest address again.
We don't have required code snippet with first part (import + test) due to segwit DNM.
So, I re-implemented that test (unload / load & re-test) for another imported descriptor.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
This PR continues supporting of Descriptor Wallets, see #5579 for prior work.
See https://github.com/dashpay/dash-issues/issues/59 for Check-list issue
What was done?
Particularly:
Beside that fixed:
How Has This Been Tested?
Run unit + functional test. Backport bitcoin#18788 enables descriptor wallets for multiple functional tests.
Breaking Changes
N/A
Checklist: