From 534d54de8e2930b514af754463599dbe36006774 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 7 Dec 2021 08:50:59 +0100 Subject: [PATCH 1/3] Merge bitcoin/bitcoin#23686: test: fix `interface_bitcoin_cli.py --descriptors` and add to test runner 035767f54a393ff9c0b4869b3142db12ecacb8e3 test: add interface_bitcoin_cli.py --descriptors to test_runner.py (Sebastian Falbesoner) e4fa28a3228619b58d0177a28dc016aac8a87afe test: fix test interface_bitcoin_cli.py for descriptor wallets (Sebastian Falbesoner) Pull request description: The functional test interface_bitcoin_cli.py currently fails on master branch, if descriptor wallets are used (argument `--descriptors`), see #23684. This is due to the fact that different change output types are used for created transactions (P2WPKH for legacy wallets, P2TR for descriptor wallets; the former doesn't have a ScriptPubKeyMan for bech32m), resulting in different tx sizes and hence also fees. Fix this by explicitely setting the output type via passing both `-addresstype=bech32` and `-changetype=bech32` as argument. The former would not be needed by now, but makes the test more deterministic and avoids a failure if bech32m becomes the default address type. Fixes #23684, should also pave the way for #23682. Top commit has no ACKs. Tree-SHA512: 39b780e25e4c7094cb3378e0f10d4a8aebac1500b7b2d68de47e54e23b9b5efe5afcf8765bb8398eeaf56968e2586a1b294a0f8773c7d90f4188a0f00b8501ff --- test/functional/interface_bitcoin_cli.py | 2 ++ test/functional/test_runner.py | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/test/functional/interface_bitcoin_cli.py b/test/functional/interface_bitcoin_cli.py index d72cd31f577e4..15910b1f5ace6 100755 --- a/test/functional/interface_bitcoin_cli.py +++ b/test/functional/interface_bitcoin_cli.py @@ -149,6 +149,8 @@ def run_test(self): if self.is_wallet_compiled(): self.log.info("Test -getinfo and dash-cli getwalletinfo return expected wallet info") + # Explicitely set the output type in order to have constintent tx vsize / fees + self.restart_node(0, extra_args=["-addresstype=bech32", "-changetype=bech32"]) assert_equal(Decimal(cli_get_info['Balance']), BALANCE) assert 'Balances' not in cli_get_info_string wallet_info = self.nodes[0].getwalletinfo() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index d118bf6fa7663..3431f10c5452d 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -160,7 +160,8 @@ 'interface_zmq_dash.py --legacy-wallet', 'interface_zmq.py', 'rpc_invalid_address_message.py', - 'interface_bitcoin_cli.py', + 'interface_bitcoin_cli.py --legacy-wallet', + 'interface_bitcoin_cli.py --descriptors', 'feature_bind_extra.py', 'mempool_resurrect.py', 'wallet_txn_doublespend.py --mineblock', From 5602c823dde05a8a9a9184ef849082a56cd8241c Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 9 Dec 2021 17:54:15 +0100 Subject: [PATCH 2/3] Merge bitcoin/bitcoin#23725: test: fix `feature_coinstatsindex.py --descriptors` and add to test runner 61fb410c0d9bc9e5fe4e3c52b4c519d49faf15f6 test: add feature_coinstatsindex.py --descriptors to test_runner.py (Sebastian Falbesoner) 50b044a88e3cfa67e28261333e658f4288d54018 test: fix test feature_coinstatsindex.py for descriptor wallets (Sebastian Falbesoner) Pull request description: The functional test feature_coinstatsindex.py currently fails on master branch, if descriptor wallets are used (argument `--descriptors`; or if BDB is not compiled, see https://github.com/bitcoin/bitcoin/pull/23682#issuecomment-989827592). This is due to the fact that different change output types are used for created transactions (P2WPKH for legacy wallets, P2TR for descriptor wallets; the former doesn't have a ScriptPubKeyMan for bech32m), resulting in different tx sizes and hence also fees. Fix this by explicitely setting the output type via passing both `-addresstype=bech32` and `-changetype=bech32` as argument. The former would not be needed by now, but makes the test more deterministic and avoids a failure if bech32m becomes the default address type. Should further pave the way for #23682. ACKs for top commit: MarcoFalke: cr ACK 61fb410c0d9bc9e5fe4e3c52b4c519d49faf15f6 Tree-SHA512: 300a53f539c0b874da5fc1dd1e4e41b9408dc5526c5858c79f0aabf2ab07e57df4c9cc627fafe25246206752754a91a2977a3df8f8b2d98fb98e51c7e4d81633 --- test/functional/feature_coinstatsindex.py | 4 +++- test/functional/interface_bitcoin_cli.py | 2 +- test/functional/test_runner.py | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/test/functional/feature_coinstatsindex.py b/test/functional/feature_coinstatsindex.py index e01af97c1fa32..2e5ab2ae1cb12 100755 --- a/test/functional/feature_coinstatsindex.py +++ b/test/functional/feature_coinstatsindex.py @@ -41,7 +41,9 @@ def set_test_params(self): self.num_nodes = 2 self.supports_cli = False self.extra_args = [ - [], + # Explicitly set the output type in order to have consistent tx vsize / fees + # for both legacy and descriptor wallets (disables the change address type detection algorithm) + ["-addresstype=bech32", "-changetype=bech32"], ["-coinstatsindex"] ] diff --git a/test/functional/interface_bitcoin_cli.py b/test/functional/interface_bitcoin_cli.py index 15910b1f5ace6..1220253a58fe8 100755 --- a/test/functional/interface_bitcoin_cli.py +++ b/test/functional/interface_bitcoin_cli.py @@ -149,7 +149,7 @@ def run_test(self): if self.is_wallet_compiled(): self.log.info("Test -getinfo and dash-cli getwalletinfo return expected wallet info") - # Explicitely set the output type in order to have constintent tx vsize / fees + # Explicitly set the output type in order to have consistent tx vsize / fees self.restart_node(0, extra_args=["-addresstype=bech32", "-changetype=bech32"]) assert_equal(Decimal(cli_get_info['Balance']), BALANCE) assert 'Balances' not in cli_get_info_string diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 3431f10c5452d..3a096a5a03107 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -360,7 +360,8 @@ 'rpc_scantxoutset.py', 'feature_logging.py', 'feature_anchors.py', - 'feature_coinstatsindex.py', + 'feature_coinstatsindex.py --legacy-wallet', + 'feature_coinstatsindex.py --descriptors', 'wallet_orphanedreward.py', 'p2p_node_network_limited.py', 'p2p_node_network_limited.py --v2transport', From fceb8e3e14755d40cf0e38eb126fe016ebf373c8 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 22 Oct 2021 13:30:44 +0200 Subject: [PATCH 3/3] Merge bitcoin/bitcoin#23002: Make descriptor wallets by default 9c1052a5218e191fd23c0d9fc06f2fca34b03411 wallet: Default new wallets to descriptor wallets (Andrew Chow) f19ad404631010a5e2dac2c7cbecd057b005fe2a rpc, wallet: Descriptor wallets are no longer experimental (Andrew Chow) Pull request description: Changes the default wallet type from legacy to descriptors. Descriptor wallets will now by the default type. Additionally, descriptor wallets will no longer be marked as experimental. This follows the timeline proposed in #20160 ACKs for top commit: lsilva01: Tested ACK https://github.com/bitcoin/bitcoin/pull/23002/commits/9c1052a5218e191fd23c0d9fc06f2fca34b03411 on Ubuntu 20.04 prayank23: tACK https://github.com/bitcoin/bitcoin/pull/23002/commits/9c1052a5218e191fd23c0d9fc06f2fca34b03411 meshcollider: Code review ACK 9c1052a5218e191fd23c0d9fc06f2fca34b03411 Tree-SHA512: 834e6fec88e0c18673af7ebe135bd5333694d1be502164eb93a90e3e76c27974165aa4e59426945100c88e4eca07356e16886ef5b05cf789683ecb23fc71a12a --- src/bitcoin-wallet.cpp | 1 + src/qt/forms/createwalletdialog.ui | 3 +++ src/wallet/rpcwallet.cpp | 5 ++--- src/wallet/wallettool.cpp | 18 +++++++++++++++++- test/functional/tool_wallet.py | 4 ++-- 5 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp index 176b455ce90d8..b21b200980483 100644 --- a/src/bitcoin-wallet.cpp +++ b/src/bitcoin-wallet.cpp @@ -31,6 +31,7 @@ static void SetupWalletToolArgs(ArgsManager& argsman) argsman.AddArg("-dumpfile=", "When used with 'dump', writes out the records to this file. When used with 'createfromdump', loads the records into a new wallet.", ArgsManager::ALLOW_STRING, OptionsCategory::OPTIONS); argsman.AddArg("-debug=", "Output debugging information (default: 0).", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); argsman.AddArg("-descriptors", "Create descriptors wallet. Only for 'create'", ArgsManager::ALLOW_BOOL, OptionsCategory::OPTIONS); + argsman.AddArg("-legacy", "Create legacy wallet. Only for 'create'", ArgsManager::ALLOW_BOOL, OptionsCategory::OPTIONS); argsman.AddArg("-format=", "The format of the wallet file to create. Either \"bdb\" or \"sqlite\". Only used with 'createfromdump'", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-printtoconsole", "Send trace/debug info to console (default: 1 when no -debug is true, 0 otherwise).", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); diff --git a/src/qt/forms/createwalletdialog.ui b/src/qt/forms/createwalletdialog.ui index 49f66c25e4182..fb680806eca81 100644 --- a/src/qt/forms/createwalletdialog.ui +++ b/src/qt/forms/createwalletdialog.ui @@ -107,6 +107,9 @@ Descriptor Wallet (EXPERIMENTAL) + + true + diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 4c48f4621d6cd..2c77b0075558b 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2986,7 +2986,7 @@ static RPCHelpMan createwallet() {"blank", RPCArg::Type::BOOL, RPCArg::Default{false}, "Create a blank wallet. A blank wallet has no keys or HD seed. One can be set using upgradetohd (by mnemonic) or sethdseed (WIF private key)."}, {"passphrase", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Encrypt the wallet with this passphrase."}, {"avoid_reuse", RPCArg::Type::BOOL, RPCArg::Default{false}, "Keep track of coin reuse, and treat dirty and clean coins differently with privacy considerations in mind."}, - {"descriptors", RPCArg::Type::BOOL, RPCArg::Default{false}, "Create a native descriptor wallet. The wallet will use descriptors internally to handle address creation. This feature is well-tested but still considered experimental."}, + {"descriptors", RPCArg::Type::BOOL, RPCArg::Default{true}, "Create a native descriptor wallet. The wallet will use descriptors internally to handle address creation. This feature is well-tested but still considered experimental."}, {"load_on_startup", RPCArg::Type::BOOL, RPCArg::Optional::OMITTED_NAMED_ARG, "Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged."}, }, RPCResult{ @@ -3027,7 +3027,7 @@ static RPCHelpMan createwallet() if (!request.params[4].isNull() && request.params[4].get_bool()) { flags |= WALLET_FLAG_AVOID_REUSE; } - if (!request.params[5].isNull() && request.params[5].get_bool()) { + if (request.params[5].isNull() || request.params[5].get_bool()) { #ifndef USE_SQLITE throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without sqlite support (required for descriptor wallets)"); #endif @@ -3035,7 +3035,6 @@ static RPCHelpMan createwallet() throw JSONRPCError(RPC_INVALID_PARAMETER, "The createwallet RPC requires specifying the 'load_on_startup' flag when creating descriptor wallets. Dash Core v21 introduced this requirement due to breaking changes in the createwallet RPC."); } flags |= WALLET_FLAG_DESCRIPTORS; - warnings.emplace_back(Untranslated("Wallet is an experimental descriptor wallet")); } #ifndef USE_BDB diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index 96ca344c4e545..0792c59bd079a 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -128,6 +128,10 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) tfm::format(std::cerr, "The -descriptors option can only be used with the 'create' command.\n"); return false; } + if (args.IsArgSet("-legacy") && command != "create") { + tfm::format(std::cerr, "The -legacy option can only be used with the 'create' command.\n"); + return false; + } if (command == "create" && !args.IsArgSet("-wallet")) { tfm::format(std::cerr, "Wallet name must be provided when creating a new wallet.\n"); return false; @@ -138,7 +142,19 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) if (command == "create") { DatabaseOptions options; options.require_create = true; - if (args.GetBoolArg("-descriptors", false)) { + // If -legacy is set, use it. Otherwise default to false. + bool make_legacy = args.GetBoolArg("-legacy", false); + // If neither -legacy nor -descriptors is set, default to true. If -descriptors is set, use its value. + bool make_descriptors = (!args.IsArgSet("-descriptors") && !args.IsArgSet("-legacy")) || (args.IsArgSet("-descriptors") && args.GetBoolArg("-descriptors", true)); + if (make_legacy && make_descriptors) { + tfm::format(std::cerr, "Only one of -legacy or -descriptors can be set to true, not both\n"); + return false; + } + if (!make_legacy && !make_descriptors) { + tfm::format(std::cerr, "One of -legacy or -descriptors must be set to true (or omitted)\n"); + return false; + } + if (make_descriptors) { options.create_flags |= WALLET_FLAG_DESCRIPTORS; options.require_format = DatabaseFormat::SQLITE; } diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py index 153193930f8e0..ccb9fdd933911 100755 --- a/test/functional/tool_wallet.py +++ b/test/functional/tool_wallet.py @@ -30,8 +30,8 @@ def skip_test_if_missing_module(self): def dash_wallet_process(self, *args): binary = self.config["environment"]["BUILDDIR"] + '/src/dash-wallet' + self.config["environment"]["EXEEXT"] default_args = ['-datadir={}'.format(self.nodes[0].datadir), '-chain=%s' % self.chain] - if self.options.descriptors and 'create' in args: - default_args.append('-descriptors') + if not self.options.descriptors and 'create' in args: + default_args.append('-legacy') return subprocess.Popen([binary] + default_args + list(args), stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)