From 2c623b4b9332d8b733d32a404d67855d1e5f6cd5 Mon Sep 17 00:00:00 2001 From: Thomas Kerin Date: Tue, 8 Nov 2016 14:36:19 +0000 Subject: [PATCH 1/7] RPC: addmultisigaddress / createmultisig: parameterize _createmultisig_redeemScript to allow sorting of public keys (BIP67) --- src/rpc/client.cpp | 2 + src/rpc/output_script.cpp | 8 +++- src/rpc/util.cpp | 4 +- src/rpc/util.h | 2 +- src/script/standard.cpp | 19 ++++++++-- src/script/standard.h | 2 +- src/wallet/rpc/addresses.cpp | 6 ++- test/functional/rpc_sort_multisig.py | 41 +++++++++++++++++++++ test/functional/test_framework/test_node.py | 5 ++- test/functional/test_runner.py | 1 + test/functional/wallet_labels.py | 22 +++++++++++ 11 files changed, 99 insertions(+), 13 deletions(-) create mode 100755 test/functional/rpc_sort_multisig.py diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 4459dd71aa..62d1ec5f27 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -94,8 +94,10 @@ static const CRPCConvertParam vRPCConvertParams[] = { "scantxoutset", 1, "scanobjects" }, { "addmultisigaddress", 0, "nrequired" }, { "addmultisigaddress", 1, "keys" }, + { "addmultisigaddress", 4, "sort" }, { "createmultisig", 0, "nrequired" }, { "createmultisig", 1, "keys" }, + { "createmultisig", 3, "sort" }, { "listunspent", 0, "minconf" }, { "listunspent", 1, "maxconf" }, { "listunspent", 2, "addresses" }, diff --git a/src/rpc/output_script.cpp b/src/rpc/output_script.cpp index 990ec3ab0c..30f187bd67 100644 --- a/src/rpc/output_script.cpp +++ b/src/rpc/output_script.cpp @@ -90,7 +90,8 @@ static RPCHelpMan createmultisig() { return RPCHelpMan{"createmultisig", "\nCreates a multi-signature address with n signature of m keys required.\n" - "It returns a json object with the address and redeemScript.\n", + "It returns a json object with the address and redeemScript.\n" + "Public keys can be sorted according to BIP67 during the request if required.\n", { {"nrequired", RPCArg::Type::NUM, RPCArg::Optional::NO, "The number of required signatures out of the n keys."}, {"keys", RPCArg::Type::ARR, RPCArg::Optional::NO, "The hex-encoded public keys.", @@ -98,6 +99,7 @@ static RPCHelpMan createmultisig() {"key", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, "The hex-encoded public key"}, }}, {"address_type", RPCArg::Type::STR, RPCArg::Default{"legacy"}, "The address type to use. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\"."}, + {"sort", RPCArg::Type::BOOL, RPCArg::Default{false}, "Whether to sort public keys according to BIP67."}, }, RPCResult{ RPCResult::Type::OBJ, "", "", @@ -121,6 +123,8 @@ static RPCHelpMan createmultisig() { int required = request.params[0].getInt(); + bool sort = request.params.size() > 3 && request.params[3].get_bool(); + // Get the public keys const UniValue& keys = request.params[1].get_array(); std::vector pubkeys; @@ -147,7 +151,7 @@ static RPCHelpMan createmultisig() // Construct using pay-to-script-hash: FillableSigningProvider keystore; CScript inner; - const CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, keystore, inner); + const CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, keystore, inner, sort); // Make the descriptor std::unique_ptr descriptor = InferDescriptor(GetScriptForDestination(dest), keystore); diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index e2fa38ac67..2b27214872 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -206,7 +206,7 @@ CPubKey AddrToPubKey(const FillableSigningProvider& keystore, const std::string& } // Creates a multisig address from a given list of public keys, number of signatures required, and the address type -CTxDestination AddAndGetMultisigDestination(const int required, const std::vector& pubkeys, OutputType type, FillableSigningProvider& keystore, CScript& script_out) +CTxDestination AddAndGetMultisigDestination(const int required, const std::vector& pubkeys, OutputType type, FillableSigningProvider& keystore, CScript& script_out, bool sort) { // Gather public keys if (required < 1) { @@ -219,7 +219,7 @@ CTxDestination AddAndGetMultisigDestination(const int required, const std::vecto throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Number of keys involved in the multisignature address creation > %d\nReduce the number", MAX_PUBKEYS_PER_MULTISIG)); } - script_out = GetScriptForMultisig(required, pubkeys); + script_out = GetScriptForMultisig(required, pubkeys, sort); // Check if any keys are uncompressed. If so, the type is legacy for (const CPubKey& pk : pubkeys) { diff --git a/src/rpc/util.h b/src/rpc/util.h index 99b6b0bb9a..7c22df6f59 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -97,7 +97,7 @@ std::string HelpExampleRpcNamed(const std::string& methodname, const RPCArgList& CPubKey HexToPubKey(const std::string& hex_in); CPubKey AddrToPubKey(const FillableSigningProvider& keystore, const std::string& addr_in); -CTxDestination AddAndGetMultisigDestination(const int required, const std::vector& pubkeys, OutputType type, FillableSigningProvider& keystore, CScript& script_out); +CTxDestination AddAndGetMultisigDestination(const int required, const std::vector& pubkeys, OutputType type, FillableSigningProvider& keystore, CScript& script_out, bool sort); UniValue DescribeAddress(const CTxDestination& dest); diff --git a/src/script/standard.cpp b/src/script/standard.cpp index 7c4a05b6e6..1c684b9c7e 100644 --- a/src/script/standard.cpp +++ b/src/script/standard.cpp @@ -341,13 +341,24 @@ CScript GetScriptForRawPubKey(const CPubKey& pubKey) return CScript() << std::vector(pubKey.begin(), pubKey.end()) << OP_CHECKSIG; } -CScript GetScriptForMultisig(int nRequired, const std::vector& keys) +CScript GetScriptForMultisig(int nRequired, const std::vector& keys, bool fSorted) { - CScript script; + int nEncoded = 0; + std::vector> vEncoded; + vEncoded.resize(keys.size()); + for (const CPubKey& key : keys) { + vEncoded[nEncoded++] = ToByteVector(key); + } + if (fSorted) { + std::sort(vEncoded.begin(), vEncoded.end()); + } + + CScript script; script << nRequired; - for (const CPubKey& key : keys) - script << ToByteVector(key); + for (std::vector bytes : vEncoded) { + script << bytes; + } script << keys.size() << OP_CHECKMULTISIG; return script; diff --git a/src/script/standard.h b/src/script/standard.h index 18cf5c8c88..6ec067bf20 100644 --- a/src/script/standard.h +++ b/src/script/standard.h @@ -193,7 +193,7 @@ CScript GetScriptForRawPubKey(const CPubKey& pubkey); std::optional>>> MatchMultiA(const CScript& script LIFETIMEBOUND); /** Generate a multisig script. */ -CScript GetScriptForMultisig(int nRequired, const std::vector& keys); +CScript GetScriptForMultisig(int nRequired, const std::vector& keys, bool fSorted=false); struct ShortestVectorFirstComparator { diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index a5be0739a9..4735ea5b19 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -219,6 +219,7 @@ RPCHelpMan addmultisigaddress() "This functionality is only intended for use with non-watchonly addresses.\n" "See `importaddress` for watchonly p2sh address support.\n" "If 'label' is specified, assign address to that label.\n" + "Public keys can be sorted according to BIP67 during the request if required.\n" "Note: This command is only compatible with legacy wallets.\n", { {"nrequired", RPCArg::Type::NUM, RPCArg::Optional::NO, "The number of required signatures out of the n keys or addresses."}, @@ -229,6 +230,7 @@ RPCHelpMan addmultisigaddress() }, {"label", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "A label to assign the addresses to."}, {"address_type", RPCArg::Type::STR, RPCArg::DefaultHint{"set by -addresstype"}, "The address type to use. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\"."}, + {"sort", RPCArg::Type::BOOL, RPCArg::Default{false}, "Whether to sort public keys according to BIP67."}, }, RPCResult{ RPCResult::Type::OBJ, "", "", @@ -261,6 +263,8 @@ RPCHelpMan addmultisigaddress() int required = request.params[0].getInt(); + bool sort = request.params.size() > 4 && request.params[4].get_bool(); + // Get the public keys const UniValue& keys_or_addrs = request.params[1].get_array(); std::vector pubkeys; @@ -285,7 +289,7 @@ RPCHelpMan addmultisigaddress() // Construct using pay-to-script-hash: CScript inner; - CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, spk_man, inner); + CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, spk_man, inner, sort); pwallet->SetAddressBook(dest, label, AddressPurpose::SEND); // Make the descriptor diff --git a/test/functional/rpc_sort_multisig.py b/test/functional/rpc_sort_multisig.py new file mode 100755 index 0000000000..9e8d9de654 --- /dev/null +++ b/test/functional/rpc_sort_multisig.py @@ -0,0 +1,41 @@ +#!/usr/bin/env python3 +# Copyright (c) 2014-2016 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +# Exercise the createmultisig API + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, + assert_raises_rpc_error, +) + +class SortMultisigTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 1 + self.extra_args = [[]] + self.setup_clean_chain = True + + def run_test(self): + pub1 = "022df8750480ad5b26950b25c7ba79d3e37d75f640f8e5d9bcd5b150a0f85014da" + pub2 = "03e3818b65bcc73a7d64064106a859cc1a5a728c4345ff0b641209fba0d90de6e9" + pub3 = "021f2f6e1e50cb6a953935c3601284925decd3fd21bc445712576873fb8c6ebc18" + + pubs = [pub1,pub2,pub3] + + default = self.nodes[0].createmultisig(2, pubs) + unsorted = self.nodes[0].createmultisig(2, pubs, None, False) + + assert_equal("2N2BchzwfyuqJep7sKmFfBucfopHZQuPnpt", unsorted["address"]) + assert_equal("5221022df8750480ad5b26950b25c7ba79d3e37d75f640f8e5d9bcd5b150a0f85014da2103e3818b65bcc73a7d64064106a859cc1a5a728c4345ff0b641209fba0d90de6e921021f2f6e1e50cb6a953935c3601284925decd3fd21bc445712576873fb8c6ebc1853ae", unsorted["redeemScript"]) + assert_equal(default["address"], unsorted["address"]) + assert_equal(default["redeemScript"], unsorted["redeemScript"]) + + sorted = self.nodes[0].createmultisig(2, pubs, None, True) + assert_equal("2NFd5JqpwmQNz3gevZJ3rz9ofuHvqaP9Cye", sorted["address"]) + assert_equal("5221021f2f6e1e50cb6a953935c3601284925decd3fd21bc445712576873fb8c6ebc1821022df8750480ad5b26950b25c7ba79d3e37d75f640f8e5d9bcd5b150a0f85014da2103e3818b65bcc73a7d64064106a859cc1a5a728c4345ff0b641209fba0d90de6e953ae", sorted["redeemScript"]) + +if __name__ == '__main__': + SortMultisigTest().main() + diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 56abe5f26a..7fdfc72faa 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -791,10 +791,11 @@ class RPCOverloadWrapper(): if not import_res[0]['success']: raise JSONRPCException(import_res[0]['error']) - def addmultisigaddress(self, nrequired, keys, label=None, address_type=None): + def addmultisigaddress(self, nrequired, keys, label=None, address_type=None, sort=False): wallet_info = self.getwalletinfo() if 'descriptors' not in wallet_info or ('descriptors' in wallet_info and not wallet_info['descriptors']): - return self.__getattr__('addmultisigaddress')(nrequired, keys, label, address_type) + return self.__getattr__('addmultisigaddress')(nrequired, keys, label, address_type, sort) + assert not sort cms = self.createmultisig(nrequired, keys, address_type) req = [{ 'desc': cms['descriptor'], diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index bcedc0c9af..cd8575aae2 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -268,6 +268,7 @@ BASE_SCRIPTS = [ 'feature_nulldummy.py', 'mempool_accept.py', 'mempool_expiry.py', + 'rpc_sort_multisig.py', 'wallet_import_with_label.py --legacy-wallet', 'wallet_importdescriptors.py --descriptors', 'wallet_upgradewallet.py --legacy-wallet', diff --git a/test/functional/wallet_labels.py b/test/functional/wallet_labels.py index f074339a2b..bbc6ac1f65 100755 --- a/test/functional/wallet_labels.py +++ b/test/functional/wallet_labels.py @@ -66,6 +66,25 @@ class WalletLabelsTest(BitcoinTestFramework): for rpc_call in rpc_calls: assert_raises_rpc_error(-11, "Invalid label name", *rpc_call, "*") + def test_sort_multisig(self, node): + node.importprivkey("cSJUMwramrFYHKPfY77FH94bv4Q5rwUCyfD6zX3kLro4ZcWsXFEM") + node.importprivkey("cSpQbSsdKRmxaSWJ3TckCFTrksXNPbh8tfeZESGNQekkVxMbQ77H") + node.importprivkey("cRNbfcJgnvk2QJEVbMsxzoprotm1cy3kVA2HoyjSs3ss5NY5mQqr") + + addresses = [ + "muRmfCwue81ZT9oc3NaepefPscUHtP5kyC", + "n12RzKwqWPPA4cWGzkiebiM7Gu6NXUnDW8", + "n2yWMtx8jVbo8wv9BK2eN1LdbaakgKL3Mt", + ] + + sorted_default = node.addmultisigaddress(2, addresses, "sort-test", 'legacy') + sorted_false = node.addmultisigaddress(2, addresses, "sort-test", 'legacy', False) + sorted_true = node.addmultisigaddress(2, addresses, "sort-test", 'legacy', True) + + assert_equal(sorted_default, sorted_false) + assert_equal("2N6dne8yzh13wsRJxCcMgCYNeN9fxKWNHt8", sorted_default['address']) + assert_equal("2MsJ2YhGewgDPGEQk4vahGs4wRikJXpRRtU", sorted_true['address']) + def run_test(self): # Check that there's no UTXO on the node node = self.nodes[0] @@ -188,6 +207,9 @@ class WalletLabelsTest(BitcoinTestFramework): self.invalid_label_name_test() + if not self.options.descriptors: + self.test_sort_multisig(node) + if self.options.descriptors: # This is a descriptor wallet test because of segwit v1+ addresses self.log.info('Check watchonly labels') From cc4475acb40a31e68b1f8478acce9814eaa7411e Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 4 Feb 2017 20:40:40 +0000 Subject: [PATCH 2/7] RPC: Use options object rather than adding a "sort" boolean for multisig methods --- src/rpc/client.cpp | 4 +- src/rpc/output_script.cpp | 55 +++++++++++++++------ src/wallet/rpc/addresses.cpp | 53 +++++++++++++++++--- test/functional/rpc_sort_multisig.py | 4 +- test/functional/test_framework/test_node.py | 11 +++-- test/functional/wallet_labels.py | 16 ++++-- 6 files changed, 111 insertions(+), 32 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 62d1ec5f27..ee25008638 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -94,10 +94,10 @@ static const CRPCConvertParam vRPCConvertParams[] = { "scantxoutset", 1, "scanobjects" }, { "addmultisigaddress", 0, "nrequired" }, { "addmultisigaddress", 1, "keys" }, - { "addmultisigaddress", 4, "sort" }, + { "addmultisigaddress", 2, "options" }, { "createmultisig", 0, "nrequired" }, { "createmultisig", 1, "keys" }, - { "createmultisig", 3, "sort" }, + { "createmultisig", 2, "options" }, { "listunspent", 0, "minconf" }, { "listunspent", 1, "maxconf" }, { "listunspent", 2, "addresses" }, diff --git a/src/rpc/output_script.cpp b/src/rpc/output_script.cpp index 30f187bd67..bb75d24c4a 100644 --- a/src/rpc/output_script.cpp +++ b/src/rpc/output_script.cpp @@ -98,8 +98,12 @@ static RPCHelpMan createmultisig() { {"key", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, "The hex-encoded public key"}, }}, - {"address_type", RPCArg::Type::STR, RPCArg::Default{"legacy"}, "The address type to use. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\"."}, - {"sort", RPCArg::Type::BOOL, RPCArg::Default{false}, "Whether to sort public keys according to BIP67."}, + {"options|address_type", {RPCArg::Type::OBJ, RPCArg::Type::STR}, RPCArg::Optional::OMITTED, "", + { + {"address_type", RPCArg::Type::STR, RPCArg::Default{"legacy"}, "The address type to use. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\"."}, + {"sort", RPCArg::Type::BOOL, RPCArg::Default{false}, "Whether to sort public keys according to BIP67."}, + }, + RPCArgOptions{.oneline_description="options"}}, }, RPCResult{ RPCResult::Type::OBJ, "", "", @@ -123,7 +127,40 @@ static RPCHelpMan createmultisig() { int required = request.params[0].getInt(); - bool sort = request.params.size() > 3 && request.params[3].get_bool(); + bool sort = false; + OutputType output_type = OutputType::LEGACY; + + if (request.params[2].isStr()) { + // backward compatibility + std::optional parsed = ParseOutputType(request.params[2].get_str()); + if (!parsed) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[2].get_str())); + } + output_type = parsed.value(); + } else if (!request.params[2].isNull()) { + const UniValue& options = request.params[2].get_obj(); + RPCTypeCheckObj(options, + { + {"address_type", UniValueType(UniValue::VSTR)}, + {"sort", UniValueType(UniValue::VBOOL)}, + }, + true, true); + + if (options.exists("address_type")) { + std::optional parsed = ParseOutputType(options["address_type"].get_str()); + if (!parsed) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", options["address_type"].get_str())); + } + output_type = parsed.value(); + } + + if (options.exists("sort")) { + sort = options["sort"].get_bool(); + } + } + if (output_type == OutputType::BECH32M) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "createmultisig cannot create bech32m multisig addresses"); + } // Get the public keys const UniValue& keys = request.params[1].get_array(); @@ -136,18 +173,6 @@ static RPCHelpMan createmultisig() } } - // Get the output type - OutputType output_type = OutputType::LEGACY; - if (!request.params[2].isNull()) { - std::optional parsed = ParseOutputType(request.params[2].get_str()); - if (!parsed) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[2].get_str())); - } else if (parsed.value() == OutputType::BECH32M) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "createmultisig cannot create bech32m multisig addresses"); - } - output_type = parsed.value(); - } - // Construct using pay-to-script-hash: FillableSigningProvider keystore; CScript inner; diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index 4735ea5b19..3782c6c38c 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -228,9 +228,14 @@ RPCHelpMan addmultisigaddress() {"key", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "bitcoin address or hex-encoded public key"}, }, }, - {"label", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "A label to assign the addresses to."}, - {"address_type", RPCArg::Type::STR, RPCArg::DefaultHint{"set by -addresstype"}, "The address type to use. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\"."}, - {"sort", RPCArg::Type::BOOL, RPCArg::Default{false}, "Whether to sort public keys according to BIP67."}, + {"options|label", {RPCArg::Type::OBJ, RPCArg::Type::STR}, RPCArg::Optional::OMITTED, "", + { + {"address_type", RPCArg::Type::STR, RPCArg::DefaultHint{"set by -addresstype"}, "The address type to use. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\"."}, + {"label", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "A label to assign the address to."}, + {"sort", RPCArg::Type::BOOL, RPCArg::Default{false}, "Whether to sort public keys according to BIP67."}, + }, + RPCArgOptions{.oneline_description="\"options\""}}, + {"address_type", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "", RPCArgOptions{.hidden=true}}, }, RPCResult{ RPCResult::Type::OBJ, "", "", @@ -259,11 +264,46 @@ RPCHelpMan addmultisigaddress() LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore); - const std::string label{LabelFromValue(request.params[2])}; - int required = request.params[0].getInt(); - bool sort = request.params.size() > 4 && request.params[4].get_bool(); + std::string label; + OutputType output_type = pwallet->m_default_address_type; + bool sort = false; + + if (!request.params[2].isNull()) { + if (request.params[2].type() == UniValue::VSTR) { + // Backward compatibility + label = LabelFromValue(request.params[2]); + } else { + const UniValue& options = request.params[2]; + RPCTypeCheckObj(options, + { + {"address_type", UniValueType(UniValue::VSTR)}, + {"label", UniValueType(UniValue::VSTR)}, + {"sort", UniValueType(UniValue::VBOOL)}, + }, + true, true); + + if (options.exists("address_type")) { + if (!request.params[3].isNull()) { + throw JSONRPCError(RPC_MISC_ERROR, "address_type provided in both options and 4th parameter"); + } + std::optional parsed = ParseOutputType(options["address_type"].get_str()); + if (!parsed) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", options["address_type"].get_str())); + } + output_type = parsed.value(); + } + + if (options.exists("label")) { + label = LabelFromValue(options["label"]); + } + + if (options.exists("sort")) { + sort = options["sort"].get_bool(); + } + } + } // Get the public keys const UniValue& keys_or_addrs = request.params[1].get_array(); @@ -276,7 +316,6 @@ RPCHelpMan addmultisigaddress() } } - OutputType output_type = pwallet->m_default_address_type; if (!request.params[3].isNull()) { std::optional parsed = ParseOutputType(request.params[3].get_str()); if (!parsed) { diff --git a/test/functional/rpc_sort_multisig.py b/test/functional/rpc_sort_multisig.py index 9e8d9de654..673003ad36 100755 --- a/test/functional/rpc_sort_multisig.py +++ b/test/functional/rpc_sort_multisig.py @@ -25,14 +25,14 @@ class SortMultisigTest(BitcoinTestFramework): pubs = [pub1,pub2,pub3] default = self.nodes[0].createmultisig(2, pubs) - unsorted = self.nodes[0].createmultisig(2, pubs, None, False) + unsorted = self.nodes[0].createmultisig(2, pubs, {"sort": False}) assert_equal("2N2BchzwfyuqJep7sKmFfBucfopHZQuPnpt", unsorted["address"]) assert_equal("5221022df8750480ad5b26950b25c7ba79d3e37d75f640f8e5d9bcd5b150a0f85014da2103e3818b65bcc73a7d64064106a859cc1a5a728c4345ff0b641209fba0d90de6e921021f2f6e1e50cb6a953935c3601284925decd3fd21bc445712576873fb8c6ebc1853ae", unsorted["redeemScript"]) assert_equal(default["address"], unsorted["address"]) assert_equal(default["redeemScript"], unsorted["redeemScript"]) - sorted = self.nodes[0].createmultisig(2, pubs, None, True) + sorted = self.nodes[0].createmultisig(2, pubs, {"sort": True}) assert_equal("2NFd5JqpwmQNz3gevZJ3rz9ofuHvqaP9Cye", sorted["address"]) assert_equal("5221021f2f6e1e50cb6a953935c3601284925decd3fd21bc445712576873fb8c6ebc1821022df8750480ad5b26950b25c7ba79d3e37d75f640f8e5d9bcd5b150a0f85014da2103e3818b65bcc73a7d64064106a859cc1a5a728c4345ff0b641209fba0d90de6e953ae", sorted["redeemScript"]) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 7fdfc72faa..e9d439bcf1 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -791,11 +791,16 @@ class RPCOverloadWrapper(): if not import_res[0]['success']: raise JSONRPCException(import_res[0]['error']) - def addmultisigaddress(self, nrequired, keys, label=None, address_type=None, sort=False): + def addmultisigaddress(self, nrequired, keys, label=None, address_type=None): wallet_info = self.getwalletinfo() if 'descriptors' not in wallet_info or ('descriptors' in wallet_info and not wallet_info['descriptors']): - return self.__getattr__('addmultisigaddress')(nrequired, keys, label, address_type, sort) - assert not sort + return self.__getattr__('addmultisigaddress')(nrequired, keys, label, address_type) + if isinstance(label, dict): + options = dict(label) # copy, so we can pop and check for emptiness + assert address_type is None + address_type = options.pop('address_type', None) + label = options.pop('label', None) + assert not options cms = self.createmultisig(nrequired, keys, address_type) req = [{ 'desc': cms['descriptor'], diff --git a/test/functional/wallet_labels.py b/test/functional/wallet_labels.py index bbc6ac1f65..b54260d2ac 100755 --- a/test/functional/wallet_labels.py +++ b/test/functional/wallet_labels.py @@ -77,14 +77,24 @@ class WalletLabelsTest(BitcoinTestFramework): "n2yWMtx8jVbo8wv9BK2eN1LdbaakgKL3Mt", ] - sorted_default = node.addmultisigaddress(2, addresses, "sort-test", 'legacy') - sorted_false = node.addmultisigaddress(2, addresses, "sort-test", 'legacy', False) - sorted_true = node.addmultisigaddress(2, addresses, "sort-test", 'legacy', True) + sorted_default = node.addmultisigaddress(2, addresses, None, 'legacy') + sorted_false = node.addmultisigaddress(2, addresses, {"sort": False}, 'legacy') + sorted_true = node.addmultisigaddress(2, addresses, {"sort": True}, 'legacy') assert_equal(sorted_default, sorted_false) assert_equal("2N6dne8yzh13wsRJxCcMgCYNeN9fxKWNHt8", sorted_default['address']) assert_equal("2MsJ2YhGewgDPGEQk4vahGs4wRikJXpRRtU", sorted_true['address']) + sorted_default = node.addmultisigaddress(2, addresses, {'address_type': 'legacy'}) + sorted_false = node.addmultisigaddress(2, addresses, {'address_type': 'legacy', "sort": False}) + sorted_true = node.addmultisigaddress(2, addresses, {'address_type': 'legacy', "sort": True}) + + assert_equal(sorted_default, sorted_false) + assert_equal("2N6dne8yzh13wsRJxCcMgCYNeN9fxKWNHt8", sorted_default['address']) + assert_equal("2MsJ2YhGewgDPGEQk4vahGs4wRikJXpRRtU", sorted_true['address']) + + assert_raises_rpc_error(-1, "address_type provided in both options and 4th parameter", node.addmultisigaddress, 2, addresses, {"address_type": 'legacy'}, 'bech32') + def run_test(self): # Check that there's no UTXO on the node node = self.nodes[0] From cab9446fbb5af50ca16471a81b101e716bf390d3 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 4 Feb 2017 20:43:46 +0000 Subject: [PATCH 3/7] script: GetScriptForRawPubKey: More efficient key-encoding loop --- src/script/standard.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/script/standard.cpp b/src/script/standard.cpp index 1c684b9c7e..1de0feaa76 100644 --- a/src/script/standard.cpp +++ b/src/script/standard.cpp @@ -343,11 +343,10 @@ CScript GetScriptForRawPubKey(const CPubKey& pubKey) CScript GetScriptForMultisig(int nRequired, const std::vector& keys, bool fSorted) { - int nEncoded = 0; std::vector> vEncoded; - vEncoded.resize(keys.size()); + vEncoded.reserve(keys.size()); for (const CPubKey& key : keys) { - vEncoded[nEncoded++] = ToByteVector(key); + vEncoded.emplace_back(ToByteVector(key)); } if (fSorted) { From 766ffa62d65fa32574fb567cc74995f57640ac83 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sat, 4 Feb 2017 20:46:28 +0000 Subject: [PATCH 4/7] doc/bips: Add BIP 67 --- doc/bips.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/bips.md b/doc/bips.md index 1d5c91b8bd..ab49a9310e 100644 --- a/doc/bips.md +++ b/doc/bips.md @@ -21,6 +21,7 @@ BIPs that are implemented by Bitcoin Core (up-to-date up to **v24.0**): * [`BIP 61`](https://github.com/bitcoin/bips/blob/master/bip-0061.mediawiki): The 'reject' protocol message (and the protocol version bump to 70002) was added in **v0.9.0** ([PR #3185](https://github.com/bitcoin/bitcoin/pull/3185)). Starting **v0.17.0**, whether to send reject messages can be configured with the `-enablebip61` option, and support is deprecated (disabled by default) as of **v0.18.0**. Support was removed in **v0.20.0** ([PR #15437](https://github.com/bitcoin/bitcoin/pull/15437)). * [`BIP 65`](https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki): The CHECKLOCKTIMEVERIFY softfork was merged in **v0.12.0** ([PR #6351](https://github.com/bitcoin/bitcoin/pull/6351)), and backported to **v0.11.2** and **v0.10.4**. Mempool-only CLTV was added in [PR #6124](https://github.com/bitcoin/bitcoin/pull/6124). * [`BIP 66`](https://github.com/bitcoin/bips/blob/master/bip-0066.mediawiki): The strict DER rules and associated version 3 blocks have been implemented since **v0.10.0** ([PR #5713](https://github.com/bitcoin/bitcoin/pull/5713)). +* [`BIP 67`](https://github.com/bitcoin/bips/blob/master/bip-0067.mediawiki): Sorting multisig keys according to BIP 67 was merged in **v0.15.1** ([PR #8751](https://github.com/bitcoin/bitcoin/pull/8751)). * [`BIP 68`](https://github.com/bitcoin/bips/blob/master/bip-0068.mediawiki): Sequence locks have been implemented as of **v0.12.1** ([PR #7184](https://github.com/bitcoin/bitcoin/pull/7184)), and have been *buried* since **v0.19.0** ([PR #16060](https://github.com/bitcoin/bitcoin/pull/16060)). * [`BIP 70`](https://github.com/bitcoin/bips/blob/master/bip-0070.mediawiki) [`71`](https://github.com/bitcoin/bips/blob/master/bip-0071.mediawiki) [`72`](https://github.com/bitcoin/bips/blob/master/bip-0072.mediawiki): Payment Protocol support has been available in Bitcoin Core GUI since **v0.9.0** ([PR #5216](https://github.com/bitcoin/bitcoin/pull/5216)). From b22d7eaab5fa8b5aae2f8c118ddcca05262def11 Mon Sep 17 00:00:00 2001 From: Thomas Kerin Date: Sat, 30 Sep 2017 14:02:18 +0200 Subject: [PATCH 5/7] Ensure whilst sorting only compressed public keys are used --- src/rpc/output_script.cpp | 3 +++ src/wallet/rpc/addresses.cpp | 3 +++ 2 files changed, 6 insertions(+) diff --git a/src/rpc/output_script.cpp b/src/rpc/output_script.cpp index bb75d24c4a..c0dcf77789 100644 --- a/src/rpc/output_script.cpp +++ b/src/rpc/output_script.cpp @@ -168,6 +168,9 @@ static RPCHelpMan createmultisig() for (unsigned int i = 0; i < keys.size(); ++i) { if (IsHex(keys[i].get_str()) && (keys[i].get_str().length() == 66 || keys[i].get_str().length() == 130)) { pubkeys.push_back(HexToPubKey(keys[i].get_str())); + if (sort && !pubkeys.back().IsCompressed()) { + throw std::runtime_error(strprintf("Compressed key required for BIP67: %s", keys[i].get_str())); + } } else { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Invalid public key: %s\n.", keys[i].get_str())); } diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index 3782c6c38c..1c5a462864 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -314,6 +314,9 @@ RPCHelpMan addmultisigaddress() } else { pubkeys.push_back(AddrToPubKey(spk_man, keys_or_addrs[i].get_str())); } + if (sort && !pubkeys.back().IsCompressed()) { + throw std::runtime_error(strprintf("Compressed key required for BIP67: %s", keys_or_addrs[i].get_str())); + } } if (!request.params[3].isNull()) { From bcf3cfafd843fb3f1ffd214889f6a9a6f1ecb813 Mon Sep 17 00:00:00 2001 From: Thomas Kerin Date: Sat, 30 Sep 2017 14:52:55 +0200 Subject: [PATCH 6/7] Add more tests to sort_multisig.py / wallet_labels.py sort_multisig test: check uncompressed keys are disallowed sort_multisig: add test demonstrating sorting wallet_labels: test addmultisigaddress fails if sort=true and (wallet) address is uncompressed --- test/functional/rpc_sort_multisig.py | 47 +++++++++++++++++++++++++++- test/functional/wallet_labels.py | 18 +++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/test/functional/rpc_sort_multisig.py b/test/functional/rpc_sort_multisig.py index 673003ad36..e6e030c030 100755 --- a/test/functional/rpc_sort_multisig.py +++ b/test/functional/rpc_sort_multisig.py @@ -17,7 +17,7 @@ class SortMultisigTest(BitcoinTestFramework): self.extra_args = [[]] self.setup_clean_chain = True - def run_test(self): + def run_simple_test(self): pub1 = "022df8750480ad5b26950b25c7ba79d3e37d75f640f8e5d9bcd5b150a0f85014da" pub2 = "03e3818b65bcc73a7d64064106a859cc1a5a728c4345ff0b641209fba0d90de6e9" pub3 = "021f2f6e1e50cb6a953935c3601284925decd3fd21bc445712576873fb8c6ebc18" @@ -36,6 +36,51 @@ class SortMultisigTest(BitcoinTestFramework): assert_equal("2NFd5JqpwmQNz3gevZJ3rz9ofuHvqaP9Cye", sorted["address"]) assert_equal("5221021f2f6e1e50cb6a953935c3601284925decd3fd21bc445712576873fb8c6ebc1821022df8750480ad5b26950b25c7ba79d3e37d75f640f8e5d9bcd5b150a0f85014da2103e3818b65bcc73a7d64064106a859cc1a5a728c4345ff0b641209fba0d90de6e953ae", sorted["redeemScript"]) + def run_demonstrate_sorting(self): + pub1 = "022df8750480ad5b26950b25c7ba79d3e37d75f640f8e5d9bcd5b150a0f85014da" + pub2 = "03e3818b65bcc73a7d64064106a859cc1a5a728c4345ff0b641209fba0d90de6e9" + pub3 = "021f2f6e1e50cb6a953935c3601284925decd3fd21bc445712576873fb8c6ebc18" + + sorted = self.nodes[0].createmultisig(2, [pub3,pub1,pub2,]) + + self.test_if_result_matches(2, [pub1,pub2,pub3], True, sorted["address"]) + self.test_if_result_matches(2, [pub1,pub3,pub2], True, sorted["address"]) + self.test_if_result_matches(2, [pub2,pub3,pub1], True, sorted["address"]) + self.test_if_result_matches(2, [pub2,pub1,pub3], True, sorted["address"]) + self.test_if_result_matches(2, [pub3,pub1,pub2], True, sorted["address"]) + self.test_if_result_matches(2, [pub3,pub2,pub1], True, sorted["address"]) + + self.test_if_result_matches(2, [pub1,pub2,pub3], False, sorted["address"]) + self.test_if_result_matches(2, [pub1,pub3,pub2], False, sorted["address"]) + self.test_if_result_matches(2, [pub2,pub3,pub1], False, sorted["address"]) + self.test_if_result_matches(2, [pub2,pub1,pub3], False, sorted["address"]) + self.test_if_result_matches(2, [pub3,pub2,pub1], False, sorted["address"]) + + def test_if_result_matches(self, m, keys, sort, against): + result = self.nodes[0].createmultisig(m, keys, {"sort": sort}) + assert_equal(sort, result["address"] == against) + + def test_compressed_keys_forbidden(self): + pub1 = "02fdf7e1b65a477a7815effde996a03a7d94cbc46f7d14c05ef38425156fc92e22" + pub2 = "04823336da95f0b4cf745839dff26992cef239ad2f08f494e5b57c209e4f3602d5526bc251d480e3284d129f736441560e17f3a7eb7ed665fdf0158f44550b926c" + rs = "522102fdf7e1b65a477a7815effde996a03a7d94cbc46f7d14c05ef38425156fc92e224104823336da95f0b4cf745839dff26992cef239ad2f08f494e5b57c209e4f3602d5526bc251d480e3284d129f736441560e17f3a7eb7ed665fdf0158f44550b926c52ae" + pubs = [pub1,pub2] + + default = self.nodes[0].createmultisig(2, pubs) + assert_equal(rs, default["redeemScript"]) + + unsorted = self.nodes[0].createmultisig(2, pubs, {"sort": False}) + assert_equal(rs, unsorted["redeemScript"]) + assert_equal(default["address"], unsorted["address"]) + assert_equal(default["redeemScript"], unsorted["redeemScript"]) + + assert_raises_rpc_error(-1, "Compressed key required for BIP67: 04823336da95f0b4cf745839dff26992cef239ad2f08f494e5b57c209e4f3602d5526bc251d480e3284d129f736441560e17f3a7eb7ed665fdf0158f44550b926c", self.nodes[0].createmultisig, 2, pubs, {"sort": True}) + + def run_test(self): + self.run_simple_test() + self.run_demonstrate_sorting() + self.test_compressed_keys_forbidden() + if __name__ == '__main__': SortMultisigTest().main() diff --git a/test/functional/wallet_labels.py b/test/functional/wallet_labels.py index b54260d2ac..6dd249bb20 100755 --- a/test/functional/wallet_labels.py +++ b/test/functional/wallet_labels.py @@ -95,6 +95,23 @@ class WalletLabelsTest(BitcoinTestFramework): assert_raises_rpc_error(-1, "address_type provided in both options and 4th parameter", node.addmultisigaddress, 2, addresses, {"address_type": 'legacy'}, 'bech32') + def test_sort_multisig_with_uncompressed_hash160(self, node): + node.importpubkey("02632b12f4ac5b1d1b72b2a3b508c19172de44f6f46bcee50ba33f3f9291e47ed0") + node.importpubkey("04dd4fe618a8ad14732f8172fe7c9c5e76dd18c2cc501ef7f86e0f4e285ca8b8b32d93df2f4323ebb02640fa6b975b2e63ab3c9d6979bc291193841332442cc6ad") + address = "2MxvEpFdXeEDbnz8MbRwS23kDZC8tzQ9NjK" + + addresses = [ + "msDoRfEfZQFaQNfAEWyqf69H99yntZoBbG", + "myrfasv56W7579LpepuRy7KFhVhaWsJYS8", + ] + default = self.nodes[0].addmultisigaddress(2, addresses, {'address_type': 'legacy'}) + assert_equal(address, default['address']) + + unsorted = self.nodes[0].addmultisigaddress(2, addresses, {'address_type': 'legacy', "sort": False}) + assert_equal(address, unsorted['address']) + + assert_raises_rpc_error(-1, "Compressed key required for BIP67: myrfasv56W7579LpepuRy7KFhVhaWsJYS8", node.addmultisigaddress, 2, addresses, {"sort": True}) + def run_test(self): # Check that there's no UTXO on the node node = self.nodes[0] @@ -219,6 +236,7 @@ class WalletLabelsTest(BitcoinTestFramework): if not self.options.descriptors: self.test_sort_multisig(node) + self.test_sort_multisig_with_uncompressed_hash160(node) if self.options.descriptors: # This is a descriptor wallet test because of segwit v1+ addresses From 87f8bfc283d64c67219d53ad5af268d4d22361d3 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Tue, 10 Oct 2023 01:59:36 +0000 Subject: [PATCH 7/7] script: Optimise GetScriptForMultisig slightly --- src/script/standard.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/script/standard.cpp b/src/script/standard.cpp index 1de0feaa76..1e370d6464 100644 --- a/src/script/standard.cpp +++ b/src/script/standard.cpp @@ -355,7 +355,7 @@ CScript GetScriptForMultisig(int nRequired, const std::vector& keys, bo CScript script; script << nRequired; - for (std::vector bytes : vEncoded) { + for (const std::vector& bytes : vEncoded) { script << bytes; } script << keys.size() << OP_CHECKMULTISIG;