From 177c15d2f7cd5406ddbce8217fc023057539b828 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 4 Jun 2021 16:37:41 -0400 Subject: [PATCH 1/6] Limit LegacyScriptPubKeyMan address types Make sure that LegacyScriptPubKeyMan can only be used for legacy, p2sh-segwit, and bech32 address types. --- src/wallet/scriptpubkeyman.cpp | 9 +++++++++ src/wallet/scriptpubkeyman.h | 7 +++++++ src/wallet/wallet.cpp | 2 +- 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index c8baa0665e..4212b6f34a 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -22,6 +22,11 @@ const uint32_t BIP32_HARDENED_KEY_LIMIT = 0x80000000; bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error) { + if (LEGACY_OUTPUT_TYPES.count(type) == 0) { + error = _("Error: Legacy wallets only support the \"legacy\", \"p2sh-segwit\", and \"bech32\" address types").translated; + return false; + } + LOCK(cs_KeyStore); error.clear(); @@ -291,6 +296,10 @@ bool LegacyScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, WalletBat bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) { + if (LEGACY_OUTPUT_TYPES.count(type) == 0) { + return false; + } + LOCK(cs_KeyStore); if (!CanGetAddresses(internal)) { return false; diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 3c4603608c..3c6a29e5d1 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -254,6 +254,13 @@ public: boost::signals2::signal NotifyCanGetAddressesChanged; }; +/** OutputTypes supported by the LegacyScriptPubKeyMan */ +static const std::unordered_set LEGACY_OUTPUT_TYPES { + OutputType::LEGACY, + OutputType::P2SH_SEGWIT, + OutputType::BECH32, +}; + class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProvider { private: diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 256faf2b23..63d0f4cf41 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3033,7 +3033,7 @@ void CWallet::SetupLegacyScriptPubKeyMan() } auto spk_manager = std::unique_ptr(new LegacyScriptPubKeyMan(*this)); - for (const auto& type : OUTPUT_TYPES) { + for (const auto& type : LEGACY_OUTPUT_TYPES) { m_internal_spk_managers[type] = spk_manager.get(); m_external_spk_managers[type] = spk_manager.get(); } From 0262536c34567743e527dad46912c9ba493252cd Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 4 Jun 2021 16:38:47 -0400 Subject: [PATCH 2/6] Add OutputType::BECH32M Bech32m addresses need their own OutputType We are not ready to create DescriptorScriptPubKeyMans which produce bech32m addresses. So don't allow generating them. --- src/outputtype.cpp | 7 +++++++ src/outputtype.h | 2 ++ src/wallet/scriptpubkeyman.cpp | 7 +++++++ src/wallet/wallet.cpp | 5 +++++ 4 files changed, 21 insertions(+) diff --git a/src/outputtype.cpp b/src/outputtype.cpp index d96fb282c5..9748fe24c7 100644 --- a/src/outputtype.cpp +++ b/src/outputtype.cpp @@ -18,6 +18,7 @@ static const std::string OUTPUT_TYPE_STRING_LEGACY = "legacy"; static const std::string OUTPUT_TYPE_STRING_P2SH_SEGWIT = "p2sh-segwit"; static const std::string OUTPUT_TYPE_STRING_BECH32 = "bech32"; +static const std::string OUTPUT_TYPE_STRING_BECH32M = "bech32m"; bool ParseOutputType(const std::string& type, OutputType& output_type) { @@ -30,6 +31,9 @@ bool ParseOutputType(const std::string& type, OutputType& output_type) } else if (type == OUTPUT_TYPE_STRING_BECH32) { output_type = OutputType::BECH32; return true; + } else if (type == OUTPUT_TYPE_STRING_BECH32M) { + output_type = OutputType::BECH32M; + return true; } return false; } @@ -40,6 +44,7 @@ const std::string& FormatOutputType(OutputType type) case OutputType::LEGACY: return OUTPUT_TYPE_STRING_LEGACY; case OutputType::P2SH_SEGWIT: return OUTPUT_TYPE_STRING_P2SH_SEGWIT; case OutputType::BECH32: return OUTPUT_TYPE_STRING_BECH32; + case OutputType::BECH32M: return OUTPUT_TYPE_STRING_BECH32M; } // no default case, so the compiler can warn about missing cases assert(false); } @@ -59,6 +64,7 @@ CTxDestination GetDestinationForKey(const CPubKey& key, OutputType type) return witdest; } } + case OutputType::BECH32M: {} // This function should never be used with BECH32M, so let it assert } // no default case, so the compiler can warn about missing cases assert(false); } @@ -98,6 +104,7 @@ CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore, return ScriptHash(witprog); } } + case OutputType::BECH32M: {} // This function should not be used for BECH32M, so let it assert } // no default case, so the compiler can warn about missing cases assert(false); } diff --git a/src/outputtype.h b/src/outputtype.h index 88422e5824..8727d3f543 100644 --- a/src/outputtype.h +++ b/src/outputtype.h @@ -18,12 +18,14 @@ enum class OutputType { LEGACY, P2SH_SEGWIT, BECH32, + BECH32M, }; static constexpr auto OUTPUT_TYPES = std::array{ OutputType::LEGACY, OutputType::P2SH_SEGWIT, OutputType::BECH32, + OutputType::BECH32M, }; [[nodiscard]] bool ParseOutputType(const std::string& str, OutputType& output_type); diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 4212b6f34a..dedd40694e 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1889,6 +1889,12 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_key, OutputType addr_type) { + if (addr_type == OutputType::BECH32M) { + // Don't allow setting up taproot descriptors yet + // TODO: Allow setting up taproot descriptors + return false; + } + LOCK(cs_desc_man); assert(m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); @@ -1918,6 +1924,7 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_ desc_prefix = "wpkh(" + xpub + "/84'"; break; } + case OutputType::BECH32M: assert(false); // TODO: Setup taproot descriptor } // no default case, so the compiler can warn about missing cases assert(!desc_prefix.empty()); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 63d0f4cf41..fbda77ed62 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3086,6 +3086,11 @@ void CWallet::SetupDescriptorScriptPubKeyMans() for (bool internal : {false, true}) { for (OutputType t : OUTPUT_TYPES) { + if (t == OutputType::BECH32M) { + // Skip taproot (bech32m) for now + // TODO: Setup taproot (bech32m) descriptors by default + continue; + } auto spk_manager = std::unique_ptr(new DescriptorScriptPubKeyMan(*this, internal)); if (IsCrypted()) { if (IsLocked()) { From 699dfcd8ad9487a4e04c1ffc68211e84e126b3d2 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 4 Jun 2021 16:40:48 -0400 Subject: [PATCH 3/6] Opportunistically use bech32m change addresses if available If a transaction as a segwit output, use a bech32m change address if they are available. If not, fallback to bech32. If bech32 change addresses are unavailable, fallback to the default address type. --- src/wallet/wallet.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index fbda77ed62..0f69302697 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1909,7 +1909,13 @@ OutputType CWallet::TransactionChangeType(const std::optional& chang int witnessversion = 0; std::vector witnessprogram; if (recipient.scriptPubKey.IsWitnessProgram(witnessversion, witnessprogram)) { - return OutputType::BECH32; + if (GetScriptPubKeyMan(OutputType::BECH32M, true)) { + return OutputType::BECH32M; + } else if (GetScriptPubKeyMan(OutputType::BECH32, true)) { + return OutputType::BECH32; + } else { + return m_default_address_type; + } } } From 6dbe4d10728f882986ed0d9ed77bc736f051c662 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 4 Jun 2021 16:42:33 -0400 Subject: [PATCH 4/6] Use BECH32M for tr() desc, WitV1Taproot, and WitUnknown CTxDests The tr() descriptor, WitnessV1Taproot CTxDestination, and WitnessUnknown CTxDestination are OutputType::BECH32M so they should report as such. --- src/script/descriptor.cpp | 10 ++++++---- test/functional/wallet_taproot.py | 6 +++--- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index 84a8b06c5c..5da249b7f9 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -646,11 +646,13 @@ static std::optional OutputTypeFromDestination(const CTxDestination& return OutputType::LEGACY; } if (std::holds_alternative(dest) || - std::holds_alternative(dest) || - std::holds_alternative(dest) || - std::holds_alternative(dest)) { + std::holds_alternative(dest)) { return OutputType::BECH32; } + if (std::holds_alternative(dest) || + std::holds_alternative(dest)) { + return OutputType::BECH32M; + } return std::nullopt; } @@ -874,7 +876,7 @@ public: { assert(m_subdescriptor_args.size() == m_depths.size()); } - std::optional GetOutputType() const override { return OutputType::BECH32; } + std::optional GetOutputType() const override { return OutputType::BECH32M; } bool IsSingleType() const final { return true; } }; diff --git a/test/functional/wallet_taproot.py b/test/functional/wallet_taproot.py index 1547a90125..b085a6eab6 100755 --- a/test/functional/wallet_taproot.py +++ b/test/functional/wallet_taproot.py @@ -226,7 +226,7 @@ class WalletTaprootTest(BitcoinTestFramework): result = self.addr_gen.importdescriptors([{"desc": desc_pub, "active": True, "timestamp": "now"}]) assert(result[0]['success']) for i in range(4): - addr_g = self.addr_gen.getnewaddress(address_type='bech32') + addr_g = self.addr_gen.getnewaddress(address_type='bech32m') if treefn is not None: addr_r = self.make_addr(treefn, keys, i) assert_equal(addr_g, addr_r) @@ -259,7 +259,7 @@ class WalletTaprootTest(BitcoinTestFramework): result = self.rpc_online.importdescriptors([{"desc": desc_change, "active": True, "timestamp": "now", "internal": True}]) assert(result[0]['success']) for i in range(4): - addr_g = self.rpc_online.getnewaddress(address_type='bech32') + addr_g = self.rpc_online.getnewaddress(address_type='bech32m') if treefn is not None: addr_r = self.make_addr(treefn, keys_pay, i) assert_equal(addr_g, addr_r) @@ -290,7 +290,7 @@ class WalletTaprootTest(BitcoinTestFramework): result = self.psbt_offline.importdescriptors([{"desc": desc_change, "active": True, "timestamp": "now", "internal": True}]) assert(result[0]['success']) for i in range(4): - addr_g = self.psbt_online.getnewaddress(address_type='bech32') + addr_g = self.psbt_online.getnewaddress(address_type='bech32m') if treefn is not None: addr_r = self.make_addr(treefn, keys_pay, i) assert_equal(addr_g, addr_r) From 87a0e7a3b7c0ffd545e537bd497cdc3e67d045f6 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 4 Jun 2021 17:35:47 -0400 Subject: [PATCH 5/6] Disallow bech32m addresses for legacy wallet things We don't want the legacy wallet to ever have bech32m addresses so don't allow importing them. This includes addmultisigaddress as that is a legacy wallet only RPC Additionally, bech32m multisigs are not available yet, so disallow them in createmultisig. --- src/outputtype.cpp | 16 ++++++++ src/outputtype.h | 3 ++ src/rpc/misc.cpp | 3 ++ src/script/descriptor.cpp | 16 -------- src/wallet/rpcdump.cpp | 9 +++++ src/wallet/rpcwallet.cpp | 9 +++++ src/wallet/scriptpubkeyman.cpp | 5 +++ test/functional/rpc_createmultisig.py | 7 ++++ test/functional/wallet_address_types.py | 10 +++++ test/functional/wallet_basic.py | 3 ++ test/functional/wallet_importmulti.py | 21 ++++++++++ test/functional/wallet_labels.py | 52 +++++++++++++------------ 12 files changed, 113 insertions(+), 41 deletions(-) diff --git a/src/outputtype.cpp b/src/outputtype.cpp index 9748fe24c7..8ede7b9974 100644 --- a/src/outputtype.cpp +++ b/src/outputtype.cpp @@ -108,3 +108,19 @@ CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore, } // no default case, so the compiler can warn about missing cases assert(false); } + +std::optional OutputTypeFromDestination(const CTxDestination& dest) { + if (std::holds_alternative(dest) || + std::holds_alternative(dest)) { + return OutputType::LEGACY; + } + if (std::holds_alternative(dest) || + std::holds_alternative(dest)) { + return OutputType::BECH32; + } + if (std::holds_alternative(dest) || + std::holds_alternative(dest)) { + return OutputType::BECH32M; + } + return std::nullopt; +} diff --git a/src/outputtype.h b/src/outputtype.h index 8727d3f543..2b83235cd0 100644 --- a/src/outputtype.h +++ b/src/outputtype.h @@ -47,4 +47,7 @@ std::vector GetAllDestinationsForKey(const CPubKey& key); */ CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore, const CScript& script, OutputType); +/** Get the OutputType for a CTxDestination */ +std::optional OutputTypeFromDestination(const CTxDestination& dest); + #endif // BITCOIN_OUTPUTTYPE_H diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp index ab239fe79c..5178ce60e8 100644 --- a/src/rpc/misc.cpp +++ b/src/rpc/misc.cpp @@ -131,6 +131,9 @@ static RPCHelpMan createmultisig() if (!ParseOutputType(request.params[2].get_str(), output_type)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[2].get_str())); } + if (output_type == OutputType::BECH32M) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "createmultisig cannot create bech32m multisig addresses"); + } } // Construct using pay-to-script-hash: diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index 5da249b7f9..d796ed26aa 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -640,22 +640,6 @@ public: std::optional GetOutputType() const override { return std::nullopt; } }; -static std::optional OutputTypeFromDestination(const CTxDestination& dest) { - if (std::holds_alternative(dest) || - std::holds_alternative(dest)) { - return OutputType::LEGACY; - } - if (std::holds_alternative(dest) || - std::holds_alternative(dest)) { - return OutputType::BECH32; - } - if (std::holds_alternative(dest) || - std::holds_alternative(dest)) { - return OutputType::BECH32M; - } - return std::nullopt; -} - /** A parsed addr(A) descriptor. */ class AddressDescriptor final : public DescriptorImpl { diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 4e9ba83ead..35649ab02c 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -286,6 +286,9 @@ RPCHelpMan importaddress() if (fP2SH) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Cannot use the p2sh flag with an address - use a script instead"); } + if (OutputTypeFromDestination(dest) == OutputType::BECH32M) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Bech32m addresses cannot be imported into legacy wallets"); + } pwallet->MarkDirty(); @@ -962,6 +965,9 @@ static UniValue ProcessImportLegacy(ImportData& import_data, std::mapGetOutputType() == OutputType::BECH32M) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Bech32m descriptors cannot be imported into legacy wallets"); + } have_solving_data = parsed_desc->IsSolvable(); const bool watch_only = data.exists("watchonly") ? data["watchonly"].get_bool() : false; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 8b1b6c6d95..951f575c2f 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -269,6 +269,9 @@ static RPCHelpMan getnewaddress() if (!ParseOutputType(request.params[1].get_str(), output_type)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[1].get_str())); } + if (output_type == OutputType::BECH32M && pwallet->GetLegacyScriptPubKeyMan()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Legacy wallets cannot provide bech32m addresses"); + } } CTxDestination dest; @@ -313,6 +316,9 @@ static RPCHelpMan getrawchangeaddress() if (!ParseOutputType(request.params[0].get_str(), output_type)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[0].get_str())); } + if (output_type == OutputType::BECH32M && pwallet->GetLegacyScriptPubKeyMan()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Legacy wallets cannot provide bech32m addresses"); + } } CTxDestination dest; @@ -1004,6 +1010,9 @@ static RPCHelpMan addmultisigaddress() if (!ParseOutputType(request.params[3].get_str(), output_type)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[3].get_str())); } + if (output_type == OutputType::BECH32M) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Bech32m multisig addresses cannot be created with legacy wallets"); + } } // Construct using pay-to-script-hash: diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index dedd40694e..2f7729a901 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -26,6 +26,7 @@ bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestinat error = _("Error: Legacy wallets only support the \"legacy\", \"p2sh-segwit\", and \"bech32\" address types").translated; return false; } + assert(type != OutputType::BECH32M); LOCK(cs_KeyStore); error.clear(); @@ -299,6 +300,7 @@ bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool i if (LEGACY_OUTPUT_TYPES.count(type) == 0) { return false; } + assert(type != OutputType::BECH32M); LOCK(cs_KeyStore); if (!CanGetAddresses(internal)) { @@ -1303,6 +1305,7 @@ void LegacyScriptPubKeyMan::AddKeypoolPubkeyWithDB(const CPubKey& pubkey, const void LegacyScriptPubKeyMan::KeepDestination(int64_t nIndex, const OutputType& type) { + assert(type != OutputType::BECH32M); // Remove from key pool WalletBatch batch(m_storage.GetDatabase()); batch.ErasePool(nIndex); @@ -1336,6 +1339,7 @@ void LegacyScriptPubKeyMan::ReturnDestination(int64_t nIndex, bool fInternal, co bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, const OutputType type, bool internal) { + assert(type != OutputType::BECH32M); if (!CanGetAddresses(internal)) { return false; } @@ -1404,6 +1408,7 @@ bool LegacyScriptPubKeyMan::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& key void LegacyScriptPubKeyMan::LearnRelatedScripts(const CPubKey& key, OutputType type) { + assert(type != OutputType::BECH32M); if (key.IsCompressed() && (type == OutputType::P2SH_SEGWIT || type == OutputType::BECH32)) { CTxDestination witdest = WitnessV0KeyHash(key.GetID()); CScript witprog = GetScriptForDestination(witdest); diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index af515f3a27..816ec67492 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -97,6 +97,9 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): sorted_key_desc = descsum_create('sh(multi(2,{}))'.format(sorted_key_str)) assert_equal(self.nodes[0].deriveaddresses(sorted_key_desc)[0], t['address']) + # Check that bech32m is currently not allowed + assert_raises_rpc_error(-5, "createmultisig cannot create bech32m multisig addresses", self.nodes[0].createmultisig, 2, self.pub, "bech32m") + def check_addmultisigaddress_errors(self): if self.options.descriptors: return @@ -108,6 +111,10 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): self.nodes[0].importaddress(a) assert_raises_rpc_error(-5, 'no full public key for address', lambda: self.nodes[0].addmultisigaddress(nrequired=1, keys=addresses)) + # Bech32m address type is disallowed for legacy wallets + pubs = [self.nodes[1].getaddressinfo(addr)["pubkey"] for addr in addresses] + assert_raises_rpc_error(-5, "Bech32m multisig addresses cannot be created with legacy wallets", self.nodes[0].addmultisigaddress, 2, pubs, "", "bech32m") + def checkbalances(self): node0, node1, node2 = self.nodes node0.generate(COINBASE_MATURITY) diff --git a/test/functional/wallet_address_types.py b/test/functional/wallet_address_types.py index 6d93cf412f..b05fedcfc7 100755 --- a/test/functional/wallet_address_types.py +++ b/test/functional/wallet_address_types.py @@ -373,5 +373,15 @@ class AddressTypeTest(BitcoinTestFramework): self.test_address(4, self.nodes[4].getrawchangeaddress(), multisig=False, typ='p2sh-segwit') self.test_address(4, self.nodes[4].getrawchangeaddress('bech32'), multisig=False, typ='bech32') + if self.options.descriptors: + self.log.info("Descriptor wallets do not have bech32m addreses by default yet") + # TODO: Remove this when they do + assert_raises_rpc_error(-12, "Error: No bech32m addresses available", self.nodes[0].getnewaddress, "", "bech32m") + assert_raises_rpc_error(-12, "Error: Keypool ran out, please call keypoolrefill first", self.nodes[0].getrawchangeaddress, "bech32m") + else: + self.log.info("Legacy wallets cannot make bech32m addresses") + assert_raises_rpc_error(-8, "Legacy wallets cannot provide bech32m addresses", self.nodes[0].getnewaddress, "", "bech32m") + assert_raises_rpc_error(-8, "Legacy wallets cannot provide bech32m addresses", self.nodes[0].getrawchangeaddress, "bech32m") + if __name__ == '__main__': AddressTypeTest().main() diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index a052ec7477..b5afc3785e 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -420,6 +420,9 @@ class WalletTest(BitcoinTestFramework): # This will raise an exception for importing an invalid pubkey assert_raises_rpc_error(-5, "Pubkey is not a valid public key", self.nodes[0].importpubkey, "5361746f736869204e616b616d6f746f") + # Bech32m addresses cannot be imported into a legacy wallet + assert_raises_rpc_error(-5, "Bech32m addresses cannot be imported into legacy wallets", self.nodes[0].importaddress, "bcrt1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqc8gma6") + # Import address and private key to check correct behavior of spendable unspents # 1. Send some coins to generate new UTXO address_to_import = self.nodes[2].getnewaddress() diff --git a/test/functional/wallet_importmulti.py b/test/functional/wallet_importmulti.py index 0a00c5eed9..baeac655df 100755 --- a/test/functional/wallet_importmulti.py +++ b/test/functional/wallet_importmulti.py @@ -746,6 +746,27 @@ class ImportMultiTest(BitcoinTestFramework): assert 'hdmasterfingerprint' not in pub_import_info assert 'hdkeypath' not in pub_import_info + # Bech32m addresses and descriptors cannot be imported + self.log.info("Bech32m addresses and descriptors cannot be imported") + self.test_importmulti( + { + "scriptPubKey": {"address": "bcrt1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqc8gma6"}, + "timestamp": "now", + }, + success=False, + error_code=-5, + error_message="Bech32m addresses cannot be imported into legacy wallets", + ) + self.test_importmulti( + { + "desc": descsum_create("tr({})".format(pub)), + "timestamp": "now", + }, + success=False, + error_code=-5, + error_message="Bech32m descriptors cannot be imported into legacy wallets", + ) + # Import some public keys to the keypool of a no privkey wallet self.log.info("Adding pubkey to keypool of disableprivkey wallet") self.nodes[1].createwallet(wallet_name="noprivkeys", disable_private_keys=True) diff --git a/test/functional/wallet_labels.py b/test/functional/wallet_labels.py index 2d792bac52..a571454acf 100755 --- a/test/functional/wallet_labels.py +++ b/test/functional/wallet_labels.py @@ -135,31 +135,33 @@ class WalletLabelsTest(BitcoinTestFramework): # in the label. This is a no-op. change_label(node, labels[2].addresses[0], labels[2], labels[2]) - self.log.info('Check watchonly labels') - node.createwallet(wallet_name='watch_only', disable_private_keys=True) - wallet_watch_only = node.get_wallet_rpc('watch_only') - BECH32_VALID = { - '✔️_VER15_PROG40': 'bcrt10qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqxkg7fn', - '✔️_VER16_PROG03': 'bcrt1sqqqqq8uhdgr', - '✔️_VER16_PROB02': 'bcrt1sqqqq4wstyw', - } - BECH32_INVALID = { - '❌_VER15_PROG41': 'bcrt1sqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqajlxj8', - '❌_VER16_PROB01': 'bcrt1sqq5r4036', - } - for l in BECH32_VALID: - ad = BECH32_VALID[l] - wallet_watch_only.importaddress(label=l, rescan=False, address=ad) - node.generatetoaddress(1, ad) - assert_equal(wallet_watch_only.getaddressesbylabel(label=l), {ad: {'purpose': 'receive'}}) - assert_equal(wallet_watch_only.getreceivedbylabel(label=l), 0) - for l in BECH32_INVALID: - ad = BECH32_INVALID[l] - assert_raises_rpc_error( - -5, - "Address is not valid" if self.options.descriptors else "Invalid Bitcoin address or script", - lambda: wallet_watch_only.importaddress(label=l, rescan=False, address=ad), - ) + if self.options.descriptors: + # This is a descriptor wallet test because of segwit v1+ addresses + self.log.info('Check watchonly labels') + node.createwallet(wallet_name='watch_only', disable_private_keys=True) + wallet_watch_only = node.get_wallet_rpc('watch_only') + BECH32_VALID = { + '✔️_VER15_PROG40': 'bcrt10qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqxkg7fn', + '✔️_VER16_PROG03': 'bcrt1sqqqqq8uhdgr', + '✔️_VER16_PROB02': 'bcrt1sqqqq4wstyw', + } + BECH32_INVALID = { + '❌_VER15_PROG41': 'bcrt1sqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqajlxj8', + '❌_VER16_PROB01': 'bcrt1sqq5r4036', + } + for l in BECH32_VALID: + ad = BECH32_VALID[l] + wallet_watch_only.importaddress(label=l, rescan=False, address=ad) + node.generatetoaddress(1, ad) + assert_equal(wallet_watch_only.getaddressesbylabel(label=l), {ad: {'purpose': 'receive'}}) + assert_equal(wallet_watch_only.getreceivedbylabel(label=l), 0) + for l in BECH32_INVALID: + ad = BECH32_INVALID[l] + assert_raises_rpc_error( + -5, + "Address is not valid" if self.options.descriptors else "Invalid Bitcoin address or script", + lambda: wallet_watch_only.importaddress(label=l, rescan=False, address=ad), + ) class Label: From 754f134a50cc56cdf0baf996d909c992770fcc97 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Sat, 12 Jun 2021 21:36:05 -0400 Subject: [PATCH 6/6] wallet: Add error message to GetReservedDestination Adds an error output parameter to all GetReservedDestination functions so that callers can get the actual reason that a change address could not be fetched. This more closely matches GetNewDestination. This allows for more granular error messages, such as one that indicates that bech32m addresses cannot be generated yet. --- src/wallet/scriptpubkeyman.cpp | 8 +++++--- src/wallet/scriptpubkeyman.h | 6 +++--- src/wallet/spend.cpp | 5 +++-- src/wallet/wallet.cpp | 10 +++++----- src/wallet/wallet.h | 2 +- test/functional/rpc_fundrawtransaction.py | 2 +- test/functional/wallet_address_types.py | 4 ++-- test/functional/wallet_keypool.py | 2 +- 8 files changed, 21 insertions(+), 18 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 2f7729a901..44c3912544 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -295,19 +295,22 @@ bool LegacyScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, WalletBat return true; } -bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) +bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool, std::string& error) { if (LEGACY_OUTPUT_TYPES.count(type) == 0) { + error = _("Error: Legacy wallets only support the \"legacy\", \"p2sh-segwit\", and \"bech32\" address types").translated; return false; } assert(type != OutputType::BECH32M); LOCK(cs_KeyStore); if (!CanGetAddresses(internal)) { + error = _("Error: Keypool ran out, please call keypoolrefill first").translated; return false; } if (!ReserveKeyFromKeyPool(index, keypool, internal)) { + error = _("Error: Keypool ran out, please call keypoolrefill first").translated; return false; } address = GetDestinationForKey(keypool.vchPubKey, type); @@ -1720,10 +1723,9 @@ bool DescriptorScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, Walle return true; } -bool DescriptorScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) +bool DescriptorScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool, std::string& error) { LOCK(cs_desc_man); - std::string error; bool result = GetNewDestination(type, address, error); index = m_wallet_descriptor.next_index - 1; return result; diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 3c6a29e5d1..b2ca354b0a 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -181,7 +181,7 @@ public: virtual bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) { return false; } virtual bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) { return false; } - virtual bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) { return false; } + virtual bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool, std::string& error) { return false; } virtual void KeepDestination(int64_t index, const OutputType& type) {} virtual void ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) {} @@ -364,7 +364,7 @@ public: bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) override; bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) override; - bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) override; + bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool, std::string& error) override; void KeepDestination(int64_t index, const OutputType& type) override; void ReturnDestination(int64_t index, bool internal, const CTxDestination&) override; @@ -573,7 +573,7 @@ public: bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) override; bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) override; - bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) override; + bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool, std::string& error) override; void ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) override; // Tops up the descriptor cache and m_map_script_pub_keys. The cache is stored in the wallet file diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index c8ded4c51e..6a8df437ae 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -618,8 +618,9 @@ bool CWallet::CreateTransactionInternal( // Reserve a new key pair from key pool. If it fails, provide a dummy // destination in case we don't need change. CTxDestination dest; - if (!reservedest.GetReservedDestination(dest, true)) { - error = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first."); + std::string dest_err; + if (!reservedest.GetReservedDestination(dest, true, dest_err)) { + error = strprintf(_("Transaction needs a change address, but we can't generate it. %s"), dest_err); } scriptChange = GetScriptForDestination(dest); // A valid destination implies a change script (and diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0f69302697..c2586b60b8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2118,7 +2118,7 @@ bool CWallet::GetNewDestination(const OutputType type, const std::string label, spk_man->TopUp(); result = spk_man->GetNewDestination(type, dest, error); } else { - error = strprintf("Error: No %s addresses available.", FormatOutputType(type)); + error = strprintf(_("Error: No %s addresses available."), FormatOutputType(type)).translated; } if (result) { SetAddressBook(dest, label, "receive"); @@ -2133,8 +2133,7 @@ bool CWallet::GetNewChangeDestination(const OutputType type, CTxDestination& des error.clear(); ReserveDestination reservedest(this, type); - if (!reservedest.GetReservedDestination(dest, true)) { - error = _("Error: Keypool ran out, please call keypoolrefill first").translated; + if (!reservedest.GetReservedDestination(dest, true, error)) { return false; } @@ -2181,10 +2180,11 @@ std::set CWallet::GetLabelAddresses(const std::string& label) co return result; } -bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool internal) +bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool internal, std::string& error) { m_spk_man = pwallet->GetScriptPubKeyMan(type, internal); if (!m_spk_man) { + error = strprintf(_("Error: No %s addresses available."), FormatOutputType(type)).translated; return false; } @@ -2194,7 +2194,7 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool inter m_spk_man->TopUp(); CKeyPool keypool; - if (!m_spk_man->GetReservedDestination(type, internal, address, nIndex, keypool)) { + if (!m_spk_man->GetReservedDestination(type, internal, address, nIndex, keypool, error)) { return false; } fInternal = keypool.fInternal; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 66f39edb4d..b63938c5f1 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -181,7 +181,7 @@ public: } //! Reserve an address - bool GetReservedDestination(CTxDestination& pubkey, bool internal); + bool GetReservedDestination(CTxDestination& pubkey, bool internal, std::string& error); //! Return reserved address void ReturnDestination(); //! Keep the address. Do not return it's key to the keypool when this object goes out of scope diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 4b07a32c54..fa98c44152 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -551,7 +551,7 @@ class RawTransactionsTest(BitcoinTestFramework): # creating the key must be impossible because the wallet is locked outputs = {self.nodes[0].getnewaddress():1.1} rawtx = self.nodes[1].createrawtransaction(inputs, outputs) - assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.", self.nodes[1].fundrawtransaction, rawtx) + assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it.", self.nodes[1].fundrawtransaction, rawtx) # Refill the keypool. self.nodes[1].walletpassphrase("test", 100) diff --git a/test/functional/wallet_address_types.py b/test/functional/wallet_address_types.py index b05fedcfc7..9b97d08424 100755 --- a/test/functional/wallet_address_types.py +++ b/test/functional/wallet_address_types.py @@ -374,10 +374,10 @@ class AddressTypeTest(BitcoinTestFramework): self.test_address(4, self.nodes[4].getrawchangeaddress('bech32'), multisig=False, typ='bech32') if self.options.descriptors: - self.log.info("Descriptor wallets do not have bech32m addreses by default yet") + self.log.info("Descriptor wallets do not have bech32m addresses by default yet") # TODO: Remove this when they do assert_raises_rpc_error(-12, "Error: No bech32m addresses available", self.nodes[0].getnewaddress, "", "bech32m") - assert_raises_rpc_error(-12, "Error: Keypool ran out, please call keypoolrefill first", self.nodes[0].getrawchangeaddress, "bech32m") + assert_raises_rpc_error(-12, "Error: No bech32m addresses available", self.nodes[0].getrawchangeaddress, "bech32m") else: self.log.info("Legacy wallets cannot make bech32m addresses") assert_raises_rpc_error(-8, "Legacy wallets cannot provide bech32m addresses", self.nodes[0].getnewaddress, "", "bech32m") diff --git a/test/functional/wallet_keypool.py b/test/functional/wallet_keypool.py index 3fe6adeebc..28bfc9116f 100755 --- a/test/functional/wallet_keypool.py +++ b/test/functional/wallet_keypool.py @@ -161,7 +161,7 @@ class KeyPoolTest(BitcoinTestFramework): # Using a fee rate (10 sat / byte) well above the minimum relay rate # creating a 5,000 sat transaction with change should not be possible - assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.", w2.walletcreatefundedpsbt, inputs=[], outputs=[{addr.pop(): 0.00005000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.00010}) + assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it.", w2.walletcreatefundedpsbt, inputs=[], outputs=[{addr.pop(): 0.00005000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.00010}) # creating a 10,000 sat transaction without change, with a manual input, should still be possible res = w2.walletcreatefundedpsbt(inputs=w2.listunspent(), outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.00010})