tests: Update more tests to work with descriptor wallets #18788

achow101 · 2020-04-27T19:57:57Z

I went through all the tests and checked whether they passed with descriptor wallets. This partially informed some changes in #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.

DrahtBot · 2020-04-27T23:03:06Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: ignore (but warn) on duplicate -wallet parameters #20199 (wallet: ignore (but warn) on duplicate -wallet parameters by jonasschnelli)
wallet: Unify wallet directory lock error message #20094 (wallet: Unify wallet directory lock error message by hebasto)
Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks #19502 (Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks by luke-jr)
Refactor: Improve setup_clean_chain semantics #19168 (Refactor: Improve setup_clean_chain semantics by fjahr)
test: add v0.20.1, v0.21.0 and v22.0 to backwards compatibility test #19013 (test: add v0.20.1 to backwards compatibility test by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko · 2020-04-28T12:32:49Z

Concept ACK, but I'd prefer if we got the current tests green first: #18800

Sjors · 2020-05-04T09:27:13Z

Concept ACK. Needs rebase, now that #18805 landed, which fixed #18800.

Sjors · 2020-05-15T19:20:34Z

Tests pass on macOS. The commit messages could be more descriptive than "Fix wallet_importprunedfunds", if only to aid the lazy reviewer in understanding what problems you're solving.

maflcko · 2020-05-19T00:09:14Z

Concept ACK

achow101 · 2020-05-26T17:04:40Z

I've updated the commit messages to be more descriptive.

Sjors

f57cacf looks good, except one issue, see inline.

0472e9f: the commit message is confusing. Why is this code being ~duplicated?

Sjors · 2020-06-01T14:41:18Z

test/functional/rpc_fundrawtransaction.py

@@ -480,22 +490,39 @@ def test_spend_2of2(self):
        oldBalance = self.nodes[1].getbalance()
        inputs = []
        outputs = {self.nodes[1].getnewaddress():1.1}
-        rawtx = self.nodes[2].createrawtransaction(inputs, outputs)


In e3dc503 it seems problematic to replace createrawtransaction and fundrawtransaction with PSBT methods in rpc_fundrawtransaction.py.

I think it's fine in some instances (like this one) because the funding code is the same and the goal is to check that the funding works.

Do we still sufficiently test the createrawtransaction and fundrawtransaction RPC elsewhere? Those are rather ancient, with all cool kids using PSBT it might be a while before we see a regression.

IMO we do. createrawtransaction and fundrawtransaction are still being tested in rpc_fundrawtransaction.py as well is in other tests. Additionally, fundrawtransaction basically only calls FundTransaction after a bit of options parsing, and walletcreatefundedpsbt calls this function as well, right after transaction creation.

Descriptor wallets output slightly different information in the wallet tool, so check that output when in descriptor wallet mode.

dumpprivkey and watchonly behavior don't work with descriptor wallets. Test for multisigs is modified to not rely on watchonly behavior for those multisigs. This has a side effect of removing listunspent, but that's not the target of this test, so that's fine.

…wallet.py sethdseed and importmulti are not available for descriptor wallets, so when doing descriptor wallet tests, use importdescriptors instead. Also changes some output to match what descriptor wallets will return.

…ypes.py addmultisigaddress is not available in descriptor wallets, so only run these when testing legacy wallets

The traditional multisig workflow doesn't work with descriptor wallets so make these tests legacy wallet only.

Some tests are legacy wallet only (and make legacy wallets) so they shouldn't be run when doing descriptor wallet tests.

…ptors Although legacy wallet is still the default, for future use, add a --legacy-wallet option to the test framework. Additional tests for descriptor wallets have been enabled with the --descriptors option. Tests that must be legacy wallet only are being started with --legacy-wallet. Even though this option does not currently do anything, this will be helpful in the future when descriptor wallets become the default.

maflcko · 2020-11-02T10:00:24Z

review ACK c7b7e0a 🎿

Show signature and timestamp

Signature:

Timestamp of file with hash 4756f18231367024c68d5255b8972d1d0c9ad9f91dd41296c63c8c42b777c68b -

laanwj · 2020-11-02T17:50:19Z

ACK c7b7e0a

… wallets …

…P2{PKH,SH,WPKH,WSH} scripts 905d672 test: use script_util helpers for creating P2W{PKH,SH} scripts (Sebastian Falbesoner) 285a65c test: use script_util helpers for creating P2SH scripts (Sebastian Falbesoner) b57b633 test: use script_util helpers for creating P2PKH scripts (Sebastian Falbesoner) 61b6a01 test: wallet util: fix multisig P2SH-P2WSH script creation (Sebastian Falbesoner) Pull request description: PR #18788 (commit 08067ae) introduced functions to generate output scripts for various types. This PR replaces all manual CScript creations in the P2PKH, P2SH, P2WPKH, P2WSH formats with those helpers in order to increase readability and maintainability over the functional test codebase. The first commit fixes a bug in the wallet_util helper module w.r.t. to P2SH-P2WSH script creation (the result is not used in any test so far, hence it can still be seen as refactoring). The following table shows a summary of the output script patterns tackled in this PR: | Type | master branch | PR branch | | ---------- | ------------- | ------------- | | P2PKH | `CScript([OP_DUP, OP_HASH160, hash160(key), OP_EQUALVERIFY, OP_CHECKSIG])` | `key_to_p2pkh_script(key)` | | | `CScript([OP_DUP, OP_HASH160, keyhash, OP_EQUALVERIFY, OP_CHECKSIG])` | `keyhash_to_p2pkh_script(keyhash)` | | P2SH | `CScript([OP_HASH160, hash160(script), OP_EQUAL])` | `script_to_p2sh_script(script)` | | P2WPKH | `CScript([OP_0, hash160(key)])` | `key_to_p2wpkh_script(key)` | | P2WSH | `CScript([OP_0, sha256(script)])` | `script_to_p2wsh_script(script)` | Note that the `key_to_...` helpers can't be used if an invalid key size (not 33 or 65 bytes) is passed, which is the case in some rare instances where the scripts still have to be created manually. Possible follow-up ideas: * further simplify by identifying P2SH-wrapped scripts and using `key_to_p2sh_p2wpkh_script()` and `script_to_p2sh_p2wsh_script()` helpers * introduce and use `key_to_p2pk_script()` helper for P2PK scripts ACKs for top commit: rajarshimaitra: tACK 905d672 LarryRuane: tACK 905d672 0xB10C: ACK 905d672 MarcoFalke: review ACK 905d672 🕹 Tree-SHA512: 7ccfe69699bc81168ac122b03536720013355c1b2fbb088355b616015318644c4d1cd27e20c4f56c89ad083ae609add4bc838cf6316794d0edb0ce9cf7fa0fd8

Summary: Removes the use of dumpprivkey so that descriptor wallets can pass on this. Also does a few descriptor wallet specific changes due to different `IsMine` semantics. PR description: > 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 wallet_watchonly.py, wallet_import_with_label.py. Note: this is a PR with many commits, but the commits are mostly independant and can be merged as soon as they are reviewed. This is a backport of [[bitcoin/bitcoin#18788 | core#18788]] [1/16] bitcoin/bitcoin@a357111 Test Plan: ``` test/functional/test_runner.py wallet_importprunedfunds ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10653

Summary: Import things into a separate watchonly wallet to work with descriptor wallets. This is a backport of [[bitcoin/bitcoin#18788 | core#18788]] [2/16] bitcoin/bitcoin@dc81418 Test Plan: `test/functional/test_runner.py rpc_fundrawtransaction` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10654

Summary: Existing import* RPCs are disabled for descriptor wallets, so only do these tests for legacy wallets. This is a backport of [[bitcoin/bitcoin#18788 | core#18788]] [3/16] bitcoin/bitcoin@553dbf9 Test Plan: `test/functional/test_runner.py wallet_listtransactions` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10655

Summary: Generate a privkey in the test framework instead of using dumpprivkey so that descriptor wallets work in this test. This is a backport of [[bitcoin/bitcoin#18788 | core#18788]] [4/16] & [[bitcoin/bitcoin#20322 | core#20322]] (adds a `sync_all`) bitcoin/bitcoin@c2711e4 bitcoin/bitcoin@4444128 I also had to add a `key_to_p2pkh_cashaddr` function to get the address in the correct format. This has the added benefit of adding coverage for `cashaddr.encode[_full]` which was unused before this commit, as far as I can tell. Test Plan: `test/functional/test_runner.py wallet_listsinceblock` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10656

…lance.py Summary: Imports and watchonly behavior are legacy wallet only, so make them only run when the test is in legacy wallet mode. This is a backport of [[bitcoin/bitcoin#18788 | core#18788]] [5/16] bitcoin/bitcoin@a42652e Test Plan: `test/functional/test_runner.py wallet_balance` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10657

Summary: This test works without any changes, for Bitcoin ABC. For Bitcoin Core, there were some changes in segwit related tests. This is a backport of [[bitcoin/bitcoin#18788 | core#18788]] [6/15] bitcoin/bitcoin@0bd1860 Test Plan: `test/functional/test_runner.py rpc_signrawtransaction` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10659

…wallet.py Summary: sethdseed and importmulti are not available for descriptor wallets, so when doing descriptor wallet tests, use importdescriptors instead The keypool size when using descriptor wallets is unaffected for Bitcoin ABC, because our descriptors wallets have only the p2pkh keys. This is a backport of [[bitcoin/bitcoin#18788 | core#18788]] [7/15] bitcoin/bitcoin@25bc5dc Test Plan: `test/functional/test_runner.py wallet_createwallet` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10669

Summary: The traditional multisig workflow doesn't work with descriptor wallets so make these tests legacy wallet only. This is a backport of [[bitcoin/bitcoin#18788 | core#18788]] [9/15] bitcoin/bitcoin@47d3243 Notes for reviewers: - the sanity checks introduced in D2546 belong to the indented code, so I had to move them up (move & indent only) - the "RAW TX MULTISIG TESTS 2of2 tests" are indented only. The only changes are code formatting to keep lines below 80 characters. Test Plan: `test/functional/test_runner.py wallet_createwallet` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10671

…ypes.py Summary: addmultisigaddress is not available in descriptor wallets, so only run these when testing legacy wallets This is a backport of [[bitcoin/bitcoin#18788 | core#18788]] [8/15] bitcoin/bitcoin@59d3da5 Test Plan: `test/functional/test_runner.py wallet_address_types` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10664

Summary: Some tests are legacy wallet only (and make legacy wallets) so they shouldn't be run when doing descriptor wallet tests. This is a backport of [[bitcoin/bitcoin#18788 | core#18788]] [10/15] bitcoin/bitcoin@388053e Test Plan: `test/functional/test_runner.py tool_wallet` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10672

fanquake added Tests Wallet labels Apr 27, 2020

DrahtBot added the Needs rebase label Apr 28, 2020

achow101 force-pushed the desc-wallet-tests branch from fa6ca74 to f98e7d6 Compare April 28, 2020 19:02

DrahtBot added Needs rebase and removed Needs rebase labels Apr 28, 2020

achow101 force-pushed the desc-wallet-tests branch from f98e7d6 to 367cef4 Compare May 4, 2020 16:39

DrahtBot removed the Needs rebase label May 4, 2020

achow101 force-pushed the desc-wallet-tests branch from 367cef4 to 8266e43 Compare May 12, 2020 18:02

DrahtBot mentioned this pull request May 16, 2020

tests: Add capability to disable RPC timeout in functional tests #18986

Merged

DrahtBot added the Needs rebase label May 19, 2020

achow101 force-pushed the desc-wallet-tests branch from 8266e43 to 5cbebc0 Compare May 19, 2020 20:26

DrahtBot removed the Needs rebase label May 19, 2020

DrahtBot mentioned this pull request May 20, 2020

walletdb: Don't remove database transaction logs and instead error #18907

Merged

achow101 force-pushed the desc-wallet-tests branch from 5cbebc0 to f57cacf Compare May 26, 2020 17:04

DrahtBot mentioned this pull request May 28, 2020

test: msg_mempool, fRelay, and other bloomfilter tests #19083

Merged

Sjors reviewed Jun 1, 2020

View reviewed changes

achow101 added 12 commits November 1, 2020 17:54

Add descriptor wallet output to tool_wallet.py

8696888

Descriptor wallets output slightly different information in the wallet tool, so check that output when in descriptor wallet mode.

Add script equivalent of functions in address.py

08067ae

Do addmultisigaddress tests in legacy wallet mode in wallet_address_t…

59d3da5

…ypes.py addmultisigaddress is not available in descriptor wallets, so only run these when testing legacy wallets

Make raw multisig tests legacy wallet only in rpc_rawtransaction.py

47d3243

The traditional multisig workflow doesn't work with descriptor wallets so make these tests legacy wallet only.

Disable some tests for tool_wallet when descriptors

388053e

Some tests are legacy wallet only (and make legacy wallets) so they shouldn't be run when doing descriptor wallet tests.

Update wallet_labels.py to not require descriptors=False

9a4c631

Update feature_backwards_compatibility for descriptor wallets

6c9c12b

Avoid creating legacy wallets in wallet_importdescriptors.py

d4b67ad

tests: Make only desc wallets for wallet_multwallet.py --descriptors

c7b7e0a

achow101 force-pushed the desc-wallet-tests branch from a62855c to c7b7e0a Compare November 1, 2020 22:54

laanwj merged commit ef4c7c4 into bitcoin:master Nov 2, 2020

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 3, 2020

Merge bitcoin#18788: tests: Update more tests to work with descriptor…

571786b

… wallets …

theStack mentioned this pull request Jun 28, 2021

test: refactor: use script_util helpers for creating P2{PKH,SH,WPKH,WSH} scripts #22363

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

tests: Update more tests to work with descriptor wallets #18788

tests: Update more tests to work with descriptor wallets #18788

Uh oh!

achow101 commented Apr 27, 2020

Uh oh!

DrahtBot commented Apr 27, 2020 •

edited

Loading

Uh oh!

maflcko commented Apr 28, 2020

Uh oh!

Sjors commented May 4, 2020

Uh oh!

Sjors commented May 15, 2020

Uh oh!

maflcko commented May 19, 2020

Uh oh!

achow101 commented May 26, 2020

Uh oh!

Sjors left a comment

Uh oh!

Sjors Jun 1, 2020

Uh oh!

achow101 Jun 1, 2020

Uh oh!

Sjors Jun 1, 2020

Uh oh!

achow101 Jun 1, 2020

Uh oh!

maflcko commented Nov 2, 2020

Uh oh!

laanwj commented Nov 2, 2020

Uh oh!

Uh oh!

tests: Update more tests to work with descriptor wallets #18788

tests: Update more tests to work with descriptor wallets #18788

Uh oh!

Conversation

achow101 commented Apr 27, 2020

Uh oh!

DrahtBot commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Apr 28, 2020

Uh oh!

Sjors commented May 4, 2020

Uh oh!

Sjors commented May 15, 2020

Uh oh!

maflcko commented May 19, 2020

Uh oh!

achow101 commented May 26, 2020

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

Sjors Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 2, 2020

Uh oh!

laanwj commented Nov 2, 2020

Uh oh!

Uh oh!

DrahtBot commented Apr 27, 2020 •

edited

Loading