From 1cd21bfba584fa7d886f13684f34c71931eddf4d Mon Sep 17 00:00:00 2001 From: koe Date: Sat, 14 May 2022 17:07:47 -0500 Subject: [PATCH] add an option to force-update multisig key exchange under some circumstances --- src/gen_multisig/gen_multisig.cpp | 12 +- src/multisig/multisig_account.cpp | 7 +- src/multisig/multisig_account.h | 12 +- src/multisig/multisig_account_kex_impl.cpp | 151 +++++++++++++------ src/simplewallet/simplewallet.cpp | 28 ++-- src/simplewallet/simplewallet.h | 2 +- src/wallet/api/wallet.cpp | 4 +- src/wallet/api/wallet.h | 2 +- src/wallet/api/wallet2_api.h | 3 +- src/wallet/wallet2.cpp | 31 ++-- src/wallet/wallet2.h | 3 +- src/wallet/wallet_rpc_server.cpp | 9 +- src/wallet/wallet_rpc_server_commands_defs.h | 4 +- tests/unit_tests/multisig.cpp | 88 ++++++++--- utils/python-rpc/framework/wallet.py | 3 +- 15 files changed, 244 insertions(+), 115 deletions(-) diff --git a/src/gen_multisig/gen_multisig.cpp b/src/gen_multisig/gen_multisig.cpp index f13e74b0f..eedd1511d 100644 --- a/src/gen_multisig/gen_multisig.cpp +++ b/src/gen_multisig/gen_multisig.cpp @@ -50,7 +50,6 @@ using namespace std; using namespace epee; using namespace cryptonote; -using boost::lexical_cast; namespace po = boost::program_options; #undef MONERO_DEFAULT_LOG_CATEGORY @@ -84,6 +83,9 @@ static bool generate_multisig(uint32_t threshold, uint32_t total, const std::str try { + if (total == 0) + throw std::runtime_error("Signer group of size 0 is not allowed."); + // create M wallets first std::vector> wallets(total); for (size_t n = 0; n < total; ++n) @@ -118,13 +120,17 @@ static bool generate_multisig(uint32_t threshold, uint32_t total, const std::str ss << " " << name << std::endl; } - //exchange keys unless exchange_multisig_keys returns no extra info - while (!kex_msgs_intermediate[0].empty()) + // exchange keys until the wallets are done + bool ready{false}; + wallets[0]->multisig(&ready); + while (!ready) { for (size_t n = 0; n < total; ++n) { kex_msgs_intermediate[n] = wallets[n]->exchange_multisig_keys(pwd_container->password(), kex_msgs_intermediate); } + + wallets[0]->multisig(&ready); } std::string address = wallets[0]->get_account().get_public_address_str(wallets[0]->nettype()); diff --git a/src/multisig/multisig_account.cpp b/src/multisig/multisig_account.cpp index f3e78da18..401c6f1fe 100644 --- a/src/multisig/multisig_account.cpp +++ b/src/multisig/multisig_account.cpp @@ -175,19 +175,20 @@ namespace multisig // only mutate account if update succeeds multisig_account temp_account{*this}; temp_account.set_multisig_config(threshold, std::move(signers)); - temp_account.kex_update_impl(expanded_msgs_rnd1); + temp_account.kex_update_impl(expanded_msgs_rnd1, false); *this = std::move(temp_account); } //---------------------------------------------------------------------------------------------------------------------- // multisig_account: EXTERNAL //---------------------------------------------------------------------------------------------------------------------- - void multisig_account::kex_update(const std::vector &expanded_msgs) + void multisig_account::kex_update(const std::vector &expanded_msgs, + const bool force_update_use_with_caution /*= false*/) { CHECK_AND_ASSERT_THROW_MES(account_is_active(), "multisig account: tried to update kex, but kex isn't initialized yet."); CHECK_AND_ASSERT_THROW_MES(!multisig_is_ready(), "multisig account: tried to update kex, but kex is already complete."); multisig_account temp_account{*this}; - temp_account.kex_update_impl(expanded_msgs); + temp_account.kex_update_impl(expanded_msgs, force_update_use_with_caution); *this = std::move(temp_account); } //---------------------------------------------------------------------------------------------------------------------- diff --git a/src/multisig/multisig_account.h b/src/multisig/multisig_account.h index 7beb594b4..9cd0942d4 100644 --- a/src/multisig/multisig_account.h +++ b/src/multisig/multisig_account.h @@ -169,12 +169,20 @@ namespace multisig * - The main interface for multisig key exchange, this handles all the work of processing input messages, * creating new messages for new rounds, and finalizing the multisig shared public key when kex is complete. * param: expanded_msgs - kex messages corresponding to the account's 'in progress' round + * param: force_update_use_with_caution - try to force the account to update with messages from an incomplete signer set. + * - If this is the post-kex verification round, only require one input message. + * - Force updating here should only be done if we can safely assume an honest signer subgroup of size 'threshold' + * will complete the account. + * - If this is an intermediate round, only require messages from 'num signers - 1 - (round - 1)' other signers. + * - If force updating with maliciously-crafted messages, the resulting account will be invalid (either unable + * to complete signatures, or a 'hostage' to the malicious signer [i.e. can't sign without his participation]). */ - void kex_update(const std::vector &expanded_msgs); + void kex_update(const std::vector &expanded_msgs, + const bool force_update_use_with_caution = false); private: // implementation of kex_update() (non-transactional) - void kex_update_impl(const std::vector &expanded_msgs); + void kex_update_impl(const std::vector &expanded_msgs, const bool incomplete_signer_set); /** * brief: initialize_kex_update - Helper for kex_update_impl() * - Collect the local signer's shared keys to ignore in incoming messages, build the aggregate ancillary key diff --git a/src/multisig/multisig_account_kex_impl.cpp b/src/multisig/multisig_account_kex_impl.cpp index 443e84631..243058b81 100644 --- a/src/multisig/multisig_account_kex_impl.cpp +++ b/src/multisig/multisig_account_kex_impl.cpp @@ -181,7 +181,8 @@ namespace multisig * Key aggregation via aggregation coefficients prevents key cancellation attacks. * See: https://www.getmonero.org/resources/research-lab/pubs/MRL-0009.pdf * param: final_keys - address components (public keys) obtained from other participants (not shared with local) - * param: privkeys_inout - private keys of address components known by local; each key will be multiplied by an aggregation coefficient (return by reference) + * param: privkeys_inout - private keys of address components known by local; each key will be multiplied by an aggregation + * coefficient (return by reference) * return: final multisig public spend key for the account */ //---------------------------------------------------------------------------------------------------------------------- @@ -199,7 +200,8 @@ namespace multisig for (std::size_t multisig_keys_index{0}; multisig_keys_index < privkeys_inout.size(); ++multisig_keys_index) { crypto::public_key pubkey; - CHECK_AND_ASSERT_THROW_MES(crypto::secret_key_to_public_key(privkeys_inout[multisig_keys_index], pubkey), "Failed to derive public key"); + CHECK_AND_ASSERT_THROW_MES(crypto::secret_key_to_public_key(privkeys_inout[multisig_keys_index], pubkey), + "Failed to derive public key"); own_keys_mapping[pubkey] = multisig_keys_index; @@ -307,8 +309,7 @@ namespace multisig * INTERNAL * * brief: multisig_kex_msgs_sanitize_pubkeys - Sanitize multisig kex messages. - * - Removes duplicates from msg pubkeys, ignores pubkeys equal to the local account's signing key, - * ignores messages signed by the local account, ignores keys found in input 'exclusion set', + * - Removes duplicates from msg pubkeys, ignores keys found in input 'exclusion set', * constructs map of pubkey:origins. * - Requires that all input msgs have the same round number. * @@ -316,15 +317,13 @@ namespace multisig * * - If the messages' round numbers are all '1', then only the message signing pubkey is considered * 'recommended'. Furthermore, the 'exclusion set' is ignored. - * param: own_pubkey - local account's signing key (key used to sign multisig messages) * param: expanded_msgs - set of multisig kex messages to process * param: exclude_pubkeys - pubkeys to exclude from output set * outparam: sanitized_pubkeys_out - processed pubkeys obtained from msgs, mapped to their origins * return: round number shared by all input msgs */ //---------------------------------------------------------------------------------------------------------------------- - static std::uint32_t multisig_kex_msgs_sanitize_pubkeys(const crypto::public_key &own_pubkey, - const std::vector &expanded_msgs, + static std::uint32_t multisig_kex_msgs_sanitize_pubkeys(const std::vector &expanded_msgs, const std::vector &exclude_pubkeys, multisig_keyset_map_memsafe_t &sanitized_pubkeys_out) { @@ -339,10 +338,6 @@ namespace multisig // - origins = all the signing pubkeys that recommended a given msg pubkey for (const auto &expanded_msg : expanded_msgs) { - // ignore messages from self - if (expanded_msg.get_signing_pubkey() == own_pubkey) - continue; - // in round 1, only the signing pubkey is treated as a msg pubkey if (round == 1) { @@ -355,10 +350,6 @@ namespace multisig // copy all pubkeys from message into list for (const auto &pubkey : expanded_msg.get_msg_pubkeys()) { - // ignore own pubkey - if (pubkey == own_pubkey) - continue; - // ignore pubkeys in 'ignore' set if (std::find(exclude_pubkeys.begin(), exclude_pubkeys.end(), pubkey) != exclude_pubkeys.end()) continue; @@ -375,6 +366,31 @@ namespace multisig /** * INTERNAL * + * brief: remove_key_from_mapped_sets - Remove a specified key from the mapped sets in a multisig keyset map. + * param: key_to_remove - specified key to remove + * inoutparam: keyset_inout - keyset to update + */ + //---------------------------------------------------------------------------------------------------------------------- + static void remove_key_from_mapped_sets(const crypto::public_key &key_to_remove, + multisig_keyset_map_memsafe_t &keyset_inout) + { + // remove specified key from each mapped set + for (auto keyset_it = keyset_inout.begin(); keyset_it != keyset_inout.end();) + { + // remove specified key from this set + keyset_it->second.erase(key_to_remove); + + // remove empty keyset positions or increment iterator + if (keyset_it->second.size() == 0) + keyset_it = keyset_inout.erase(keyset_it); + else + ++keyset_it; + } + } + //---------------------------------------------------------------------------------------------------------------------- + /** + * INTERNAL + * * brief: evaluate_multisig_kex_round_msgs - Evaluate pubkeys from a kex round in order to prepare for the next round. * - Sanitizes input msgs. * - Require uniqueness in: 'exclude_pubkeys'. @@ -392,6 +408,8 @@ namespace multisig * param: signers - expected participants in multisig kex * param: expanded_msgs - set of multisig kex messages to process * param: exclude_pubkeys - derivations held by the local account corresponding to round 'expected_round' + * param: incomplete_signer_set - only require the minimum number of signers to complete this round + * minimum = num_signers - (round num - 1) (including local signer) * return: fully sanitized and validated pubkey:origins map for building the account's next kex round message */ //---------------------------------------------------------------------------------------------------------------------- @@ -400,7 +418,8 @@ namespace multisig const std::uint32_t expected_round, const std::vector &signers, const std::vector &expanded_msgs, - const std::vector &exclude_pubkeys) + const std::vector &exclude_pubkeys, + const bool incomplete_signer_set) { // exclude_pubkeys should all be unique for (auto it = exclude_pubkeys.begin(); it != exclude_pubkeys.end(); ++it) @@ -410,21 +429,31 @@ namespace multisig } // sanitize input messages - multisig_keyset_map_memsafe_t pubkey_origins_map; - const std::uint32_t round = multisig_kex_msgs_sanitize_pubkeys(base_pubkey, expanded_msgs, exclude_pubkeys, pubkey_origins_map); + multisig_keyset_map_memsafe_t pubkey_origins_map; //map: [pubkey : [origins]] + const std::uint32_t round = multisig_kex_msgs_sanitize_pubkeys(expanded_msgs, exclude_pubkeys, pubkey_origins_map); CHECK_AND_ASSERT_THROW_MES(round == expected_round, "Kex messages were for round [" << round << "], but expected round is [" << expected_round << "]"); + // remove the local signer from each origins set in the sanitized pubkey map + // note: intermediate kex rounds only need keys from other signers to make progress (keys from self are useless) + remove_key_from_mapped_sets(base_pubkey, pubkey_origins_map); + // evaluate pubkeys collected - std::unordered_map> origin_pubkeys_map; + std::unordered_map> origin_pubkeys_map; //map: [origin: [pubkeys]] // 1. each pubkey should be recommended by a precise number of signers + const std::size_t num_recommendations_per_pubkey_required{ + incomplete_signer_set + ? 1 + : round + }; + for (const auto &pubkey_and_origins : pubkey_origins_map) { // expected amount = round_num // With each successive round, pubkeys are shared by incrementally larger groups, // starting at 1 in round 1 (i.e. the local multisig key to start kex with). - CHECK_AND_ASSERT_THROW_MES(pubkey_and_origins.second.size() == round, + CHECK_AND_ASSERT_THROW_MES(pubkey_and_origins.second.size() >= num_recommendations_per_pubkey_required, "A pubkey recommended by multisig kex messages had an unexpected number of recommendations."); // map (sanitized) pubkeys back to origins @@ -433,8 +462,18 @@ namespace multisig } // 2. the number of unique signers recommending pubkeys should equal the number of signers passed in (minus the local signer) - CHECK_AND_ASSERT_THROW_MES(origin_pubkeys_map.size() == signers.size() - 1, - "Number of unique other signers does not equal number of other signers that recommended pubkeys."); + // - if an incomplete set is allowed, then we need at least one signer to represent each subgroup in this round that + // doesn't include the local signer + const std::size_t num_signers_required{ + incomplete_signer_set + ? signers.size() - 1 - (round - 1) + : signers.size() - 1 + }; + + CHECK_AND_ASSERT_THROW_MES(origin_pubkeys_map.size() >= num_signers_required, + "Number of unique other signers recommending pubkeys does not equal number of required other signers " + "(kex round: " << round << ", num signers found: " << origin_pubkeys_map.size() << ", num signers required: " << + num_signers_required << ")."); // 3. each origin should recommend a precise number of pubkeys @@ -461,19 +500,20 @@ namespace multisig // other signers: (N - 2) choose (msg_round_num - 1) // - Each signer recommends keys they share with other signers. - // - In each round, a signer shares a key with 'round num - 1' other signers. - // - Since 'origins pubkey map' excludes keys shared with the local account, - // only keys shared with participants 'other than local and self' will be in the map (e.g. N - 2 signers). - // - So other signers will recommend (N - 2) choose (msg_round_num - 1) pubkeys (after removing keys shared with local). - // - Each origin should have a shared key with each group of size 'round - 1'. - // Note: Keys shared with local are ignored to facilitate kex round boosting, where one or more signers may + // - In each round, every group of size 'round num' will have a key. From a single signer's perspective, + // they will share a key with every group of size 'round num - 1' of other signers. + // - Since 'origins pubkey map' excludes keys shared with the local account, only keys shared with participants + // 'other than local and self' will be in the map (e.g. N - 2 signers). + // - Other signers will recommend (N - 2) choose (msg_round_num - 1) pubkeys (after removing keys shared with local). + // Note: Keys shared with local are filtered out to facilitate kex round boosting, where one or more signers may // have boosted the local signer (implying they didn't have access to the local signer's previous round msg). const std::uint32_t expected_recommendations_others = n_choose_k_f(signers.size() - 2, round - 1); // local: (N - 1) choose (msg_round_num - 1) const std::uint32_t expected_recommendations_self = n_choose_k_f(signers.size() - 1, round - 1); - // note: expected_recommendations_others would be 0 in the last round of 1-of-N, but we return early for that case + // note: expected_recommendations_others would be 0 in the last round of 1-of-N, but we don't call this function for + // that case CHECK_AND_ASSERT_THROW_MES(expected_recommendations_self > 0 && expected_recommendations_others > 0, "Bad num signers or round num (possibly numerical limits exceeded)."); @@ -485,7 +525,7 @@ namespace multisig for (const auto &origin_and_pubkeys : origin_pubkeys_map) { CHECK_AND_ASSERT_THROW_MES(origin_and_pubkeys.second.size() == expected_recommendations_others, - "A pubkey recommended by multisig kex messages had an unexpected number of recommendations."); + "A multisig signer recommended an unexpected number of pubkeys."); // 2 (continued). only expected signers should be recommending keys CHECK_AND_ASSERT_THROW_MES(std::find(signers.begin(), signers.end(), origin_and_pubkeys.first) != signers.end(), @@ -507,6 +547,7 @@ namespace multisig * param: expected_round - expected kex round of input messages * param: signers - expected participants in multisig kex * param: expanded_msgs - set of multisig kex messages to process + * param: incomplete_signer_set - only require the minimum amount of messages to complete this round (1 message) * return: sanitized and validated pubkey:origins map */ //---------------------------------------------------------------------------------------------------------------------- @@ -514,15 +555,20 @@ namespace multisig const crypto::public_key &base_pubkey, const std::uint32_t expected_round, const std::vector &signers, - const std::vector &expanded_msgs) + const std::vector &expanded_msgs, + const bool incomplete_signer_set) { // sanitize input messages const std::vector dummy; - multisig_keyset_map_memsafe_t pubkey_origins_map; - const std::uint32_t round = multisig_kex_msgs_sanitize_pubkeys(base_pubkey, expanded_msgs, dummy, pubkey_origins_map); + multisig_keyset_map_memsafe_t pubkey_origins_map; //map: [pubkey : [origins]] + const std::uint32_t round = multisig_kex_msgs_sanitize_pubkeys(expanded_msgs, dummy, pubkey_origins_map); CHECK_AND_ASSERT_THROW_MES(round == expected_round, "Kex messages were for round [" << round << "], but expected round is [" << expected_round << "]"); + // note: do NOT remove the local signer from the pubkey origins map, since the post-kex round can be force-updated with + // just the local signer's post-kex message (if the local signer were removed, then the post-kex message's pubkeys + // would be completely lost) + // evaluate pubkeys collected // 1) there should only be two pubkeys @@ -533,17 +579,26 @@ namespace multisig CHECK_AND_ASSERT_THROW_MES(pubkey_origins_map.begin()->second == (++(pubkey_origins_map.begin()))->second, "Multisig post-kex round messages from other signers did not all recommend the same pubkey pair."); - // 3) all signers should be present in the recommendation list + // 3) all signers should be present in the recommendation list (unless an incomplete list is permitted) auto origins = pubkey_origins_map.begin()->second; - origins.insert(base_pubkey); //add self + origins.insert(base_pubkey); //add self if missing - CHECK_AND_ASSERT_THROW_MES(origins.size() == signers.size(), - "Multisig post-kex round message origins don't line up with multisig signer set."); + const std::size_t num_signers_required{ + incomplete_signer_set + ? 1 + : signers.size() + }; + + CHECK_AND_ASSERT_THROW_MES(origins.size() >= num_signers_required, + "Multisig post-kex round message origins don't line up with multisig signer set " + "(num signers found: " << origins.size() << ", num signers required: " << num_signers_required << ")."); - for (const crypto::public_key &signer : signers) + for (const crypto::public_key &origin : origins) { - CHECK_AND_ASSERT_THROW_MES(origins.find(signer) != origins.end(), - "Could not find an expected signer in multisig post-kex round messages (all signers expected)."); + // note: if num_signers_required == signers.size(), then this test will ensure all signers are present in 'origins', + // which contains only unique pubkeys + CHECK_AND_ASSERT_THROW_MES(std::find(signers.begin(), signers.end(), origin) != signers.end(), + "An unknown origin recommended a multisig post-kex verification messsage."); } return pubkey_origins_map; @@ -564,6 +619,7 @@ namespace multisig * param: expanded_msgs - set of multisig kex messages to process * param: exclude_pubkeys - keys held by the local account corresponding to round 'current_round' * - If 'current_round' is the final round, these are the local account's shares of the final aggregate key. + * param: incomplete_signer_set - allow messages from an incomplete signer set * outparam: keys_to_origins_map_out - map between round keys and identity keys * - If in the final round, these are key shares recommended by other signers for the final aggregate key. * - Otherwise, these are the local account's DH derivations for the next round. @@ -578,6 +634,7 @@ namespace multisig const std::vector &signers, const std::vector &expanded_msgs, const std::vector &exclude_pubkeys, + const bool incomplete_signer_set, multisig_keyset_map_memsafe_t &keys_to_origins_map_out) { check_multisig_config(current_round, threshold, signers.size()); @@ -598,7 +655,8 @@ namespace multisig current_round, signers, expanded_msgs, - exclude_pubkeys); + exclude_pubkeys, + incomplete_signer_set); } else //(current_round == kex_rounds_required + 1) { @@ -606,7 +664,8 @@ namespace multisig evaluated_pubkeys = evaluate_multisig_post_kex_round_msgs(base_pubkey, current_round, signers, - expanded_msgs); + expanded_msgs, + incomplete_signer_set); } // prepare keys-to-origins map for updating the multisig account @@ -693,9 +752,9 @@ namespace multisig { // post-kex verification round: check that the multisig pubkey and common pubkey were recommended by other signers CHECK_AND_ASSERT_THROW_MES(result_keys_to_origins_map.count(m_multisig_pubkey) > 0, - "Multisig post-kex round: expected multisig pubkey wasn't found in other signers' messages."); + "Multisig post-kex round: expected multisig pubkey wasn't found in input messages."); CHECK_AND_ASSERT_THROW_MES(result_keys_to_origins_map.count(m_common_pubkey) > 0, - "Multisig post-kex round: expected common pubkey wasn't found in other signers' messages."); + "Multisig post-kex round: expected common pubkey wasn't found in input messages."); // save keys that should be recommended to other signers // - for convenience, re-recommend the post-kex verification message once an account is complete @@ -790,7 +849,8 @@ namespace multisig //---------------------------------------------------------------------------------------------------------------------- // multisig_account: INTERNAL //---------------------------------------------------------------------------------------------------------------------- - void multisig_account::kex_update_impl(const std::vector &expanded_msgs) + void multisig_account::kex_update_impl(const std::vector &expanded_msgs, + const bool incomplete_signer_set) { // check messages are for the expected kex round check_messages_round(expanded_msgs, m_kex_rounds_complete + 1); @@ -816,6 +876,7 @@ namespace multisig m_signers, expanded_msgs, exclude_pubkeys, + incomplete_signer_set, result_keys_to_origins_map); // finish account update diff --git a/src/simplewallet/simplewallet.cpp b/src/simplewallet/simplewallet.cpp index 860c3f0b0..96dd2dd2f 100644 --- a/src/simplewallet/simplewallet.cpp +++ b/src/simplewallet/simplewallet.cpp @@ -243,7 +243,7 @@ namespace const char* USAGE_IMPORT_OUTPUTS("import_outputs "); const char* USAGE_SHOW_TRANSFER("show_transfer "); const char* USAGE_MAKE_MULTISIG("make_multisig [...]"); - const char* USAGE_EXCHANGE_MULTISIG_KEYS("exchange_multisig_keys [...]"); + const char* USAGE_EXCHANGE_MULTISIG_KEYS("exchange_multisig_keys [force-update-use-with-caution] [...]"); const char* USAGE_EXPORT_MULTISIG_INFO("export_multisig_info "); const char* USAGE_IMPORT_MULTISIG_INFO("import_multisig_info [...]"); const char* USAGE_SIGN_MULTISIG("sign_multisig "); @@ -1138,11 +1138,22 @@ bool simple_wallet::make_multisig_main(const std::vector &args, boo bool simple_wallet::exchange_multisig_keys(const std::vector &args) { CHECK_MULTISIG_ENABLED(); - exchange_multisig_keys_main(args, false); + bool force_update_use_with_caution = false; + + auto local_args = args; + if (args.size() >= 1 && local_args[0] == "force-update-use-with-caution") + { + force_update_use_with_caution = true; + local_args.erase(local_args.begin()); + } + + exchange_multisig_keys_main(local_args, force_update_use_with_caution, false); return true; } -bool simple_wallet::exchange_multisig_keys_main(const std::vector &args, bool called_by_mms) { +bool simple_wallet::exchange_multisig_keys_main(const std::vector &args, + const bool force_update_use_with_caution, + const bool called_by_mms) { CHECK_MULTISIG_ENABLED(); bool ready; if (m_wallet->key_on_device()) @@ -1168,15 +1179,9 @@ bool simple_wallet::exchange_multisig_keys_main(const std::vector & return false; } - if (args.size() < 1) - { - PRINT_USAGE(USAGE_EXCHANGE_MULTISIG_KEYS); - return false; - } - try { - std::string multisig_extra_info = m_wallet->exchange_multisig_keys(orig_pwd_container->password(), args); + std::string multisig_extra_info = m_wallet->exchange_multisig_keys(orig_pwd_container->password(), args, force_update_use_with_caution); bool ready; m_wallet->multisig(&ready); if (!ready) @@ -11176,7 +11181,8 @@ void simple_wallet::mms_next(const std::vector &args) mms::message m = ms.get_message_by_id(data.message_ids[i]); sig_args[i] = m.content; } - command_successful = exchange_multisig_keys_main(sig_args, true); + // todo: update mms to enable 'key exchange force updating' + command_successful = exchange_multisig_keys_main(sig_args, false, true); break; } diff --git a/src/simplewallet/simplewallet.h b/src/simplewallet/simplewallet.h index 0f2fe7bc6..7c45d45e8 100644 --- a/src/simplewallet/simplewallet.h +++ b/src/simplewallet/simplewallet.h @@ -235,7 +235,7 @@ namespace cryptonote bool make_multisig(const std::vector& args); bool make_multisig_main(const std::vector& args, bool called_by_mms); bool exchange_multisig_keys(const std::vector &args); - bool exchange_multisig_keys_main(const std::vector &args, bool called_by_mms); + bool exchange_multisig_keys_main(const std::vector &args, const bool force_update_use_with_caution, const bool called_by_mms); bool export_multisig(const std::vector& args); bool export_multisig_main(const std::vector& args, bool called_by_mms); bool import_multisig(const std::vector& args); diff --git a/src/wallet/api/wallet.cpp b/src/wallet/api/wallet.cpp index 470206bc5..5b9d398a9 100644 --- a/src/wallet/api/wallet.cpp +++ b/src/wallet/api/wallet.cpp @@ -1396,12 +1396,12 @@ string WalletImpl::makeMultisig(const vector& info, const uint32_t thres return string(); } -std::string WalletImpl::exchangeMultisigKeys(const std::vector &info) { +std::string WalletImpl::exchangeMultisigKeys(const std::vector &info, const bool force_update_use_with_caution /*= false*/) { try { clearStatus(); checkMultisigWalletNotReady(m_wallet); - return m_wallet->exchange_multisig_keys(epee::wipeable_string(m_password), info); + return m_wallet->exchange_multisig_keys(epee::wipeable_string(m_password), info, force_update_use_with_caution); } catch (const exception& e) { LOG_ERROR("Error on exchanging multisig keys: " << e.what()); setStatusError(string(tr("Failed to exchange multisig keys: ")) + e.what()); diff --git a/src/wallet/api/wallet.h b/src/wallet/api/wallet.h index 018b2a0ed..ec2d7e9b3 100644 --- a/src/wallet/api/wallet.h +++ b/src/wallet/api/wallet.h @@ -146,7 +146,7 @@ public: MultisigState multisig() const override; std::string getMultisigInfo() const override; std::string makeMultisig(const std::vector& info, uint32_t threshold) override; - std::string exchangeMultisigKeys(const std::vector &info) override; + std::string exchangeMultisigKeys(const std::vector &info, const bool force_update_use_with_caution = false) override; bool exportMultisigImages(std::string& images) override; size_t importMultisigImages(const std::vector& images) override; bool hasMultisigPartialKeyImages() const override; diff --git a/src/wallet/api/wallet2_api.h b/src/wallet/api/wallet2_api.h index b67bce60c..0ae84adb9 100644 --- a/src/wallet/api/wallet2_api.h +++ b/src/wallet/api/wallet2_api.h @@ -796,9 +796,10 @@ struct Wallet /** * @brief exchange_multisig_keys - provides additional key exchange round for arbitrary multisig schemes (like N-1/N, M/N) * @param info - base58 encoded key derivations returned by makeMultisig or exchangeMultisigKeys function call + * @param force_update_use_with_caution - force multisig account to update even if not all signers contribute round messages * @return new info string if more rounds required or an empty string if wallet creation is done */ - virtual std::string exchangeMultisigKeys(const std::vector &info) = 0; + virtual std::string exchangeMultisigKeys(const std::vector &info, const bool force_update_use_with_caution) = 0; /** * @brief exportMultisigImages - exports transfers' key images * @param images - output paramter for hex encoded array of images diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index 588ddd572..e6bd3df59 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -5126,12 +5126,11 @@ std::string wallet2::make_multisig(const epee::wipeable_string &password, } //---------------------------------------------------------------------------------------------------- std::string wallet2::exchange_multisig_keys(const epee::wipeable_string &password, - const std::vector &kex_messages) + const std::vector &kex_messages, + const bool force_update_use_with_caution /*= false*/) { bool ready{false}; CHECK_AND_ASSERT_THROW_MES(multisig(&ready), "The wallet is not multisig"); - CHECK_AND_ASSERT_THROW_MES(!ready, "Multisig wallet creation process has already been finished"); - CHECK_AND_ASSERT_THROW_MES(kex_messages.size() > 0, "No key exchange messages passed in."); // decrypt account keys epee::misc_utils::auto_scope_leave_caller keys_reencryptor; @@ -5150,13 +5149,6 @@ std::string wallet2::exchange_multisig_keys(const epee::wipeable_string &passwor ); } - // open kex messages - std::vector expanded_msgs; - expanded_msgs.reserve(kex_messages.size()); - - for (const auto &msg : kex_messages) - expanded_msgs.emplace_back(msg); - // reconstruct multisig account multisig::multisig_keyset_map_memsafe_t kex_origins_map; @@ -5177,8 +5169,25 @@ std::string wallet2::exchange_multisig_keys(const epee::wipeable_string &passwor "" }; + // KLUDGE: early return if there are no kex messages and main kex is complete (will return the post-kex verification round + // message) (it's a kludge because this behavior would be more appropriate for a standalone wallet method) + if (kex_messages.size() == 0) + { + CHECK_AND_ASSERT_THROW_MES(multisig_account.main_kex_rounds_done(), + "Exchange multisig keys: there are no kex messages but the main kex rounds are not done."); + + return multisig_account.get_next_kex_round_msg(); + } + + // open kex messages + std::vector expanded_msgs; + expanded_msgs.reserve(kex_messages.size()); + + for (const auto &msg : kex_messages) + expanded_msgs.emplace_back(msg); + // update multisig kex - multisig_account.kex_update(expanded_msgs); + multisig_account.kex_update(expanded_msgs, force_update_use_with_caution); // update wallet state diff --git a/src/wallet/wallet2.h b/src/wallet/wallet2.h index 115651e3b..3fcefd16f 100644 --- a/src/wallet/wallet2.h +++ b/src/wallet/wallet2.h @@ -886,7 +886,8 @@ private: * to other participants */ std::string exchange_multisig_keys(const epee::wipeable_string &password, - const std::vector &kex_messages); + const std::vector &kex_messages, + const bool force_update_use_with_caution = false); /*! * \brief Get initial message to start multisig key exchange (before 'make_multisig()' is called) * \return string to send to other participants diff --git a/src/wallet/wallet_rpc_server.cpp b/src/wallet/wallet_rpc_server.cpp index 1f0a1371f..964945175 100644 --- a/src/wallet/wallet_rpc_server.cpp +++ b/src/wallet/wallet_rpc_server.cpp @@ -4146,13 +4146,6 @@ namespace tools er.message = "This wallet is not multisig"; return false; } - - if (ready) - { - er.code = WALLET_RPC_ERROR_CODE_ALREADY_MULTISIG; - er.message = "This wallet is multisig, and already finalized"; - return false; - } CHECK_MULTISIG_ENABLED(); if (req.multisig_info.size() + 1 < total) @@ -4164,7 +4157,7 @@ namespace tools try { - res.multisig_info = m_wallet->exchange_multisig_keys(req.password, req.multisig_info); + res.multisig_info = m_wallet->exchange_multisig_keys(req.password, req.multisig_info, req.force_update_use_with_caution); m_wallet->multisig(&ready); if (ready) { diff --git a/src/wallet/wallet_rpc_server_commands_defs.h b/src/wallet/wallet_rpc_server_commands_defs.h index 2cca323ee..60df6296f 100644 --- a/src/wallet/wallet_rpc_server_commands_defs.h +++ b/src/wallet/wallet_rpc_server_commands_defs.h @@ -47,7 +47,7 @@ // advance which version they will stop working with // Don't go over 32767 for any of these #define WALLET_RPC_VERSION_MAJOR 1 -#define WALLET_RPC_VERSION_MINOR 25 +#define WALLET_RPC_VERSION_MINOR 26 #define MAKE_WALLET_RPC_VERSION(major,minor) (((major)<<16)|(minor)) #define WALLET_RPC_VERSION MAKE_WALLET_RPC_VERSION(WALLET_RPC_VERSION_MAJOR, WALLET_RPC_VERSION_MINOR) namespace tools @@ -2535,10 +2535,12 @@ namespace wallet_rpc { std::string password; std::vector multisig_info; + bool force_update_use_with_caution; BEGIN_KV_SERIALIZE_MAP() KV_SERIALIZE(password) KV_SERIALIZE(multisig_info) + KV_SERIALIZE_OPT(force_update_use_with_caution, false) END_KV_SERIALIZE_MAP() }; typedef epee::misc_utils::struct_init request; diff --git a/tests/unit_tests/multisig.cpp b/tests/unit_tests/multisig.cpp index 8f8ad52e1..3b3c4197c 100644 --- a/tests/unit_tests/multisig.cpp +++ b/tests/unit_tests/multisig.cpp @@ -95,9 +95,47 @@ static std::vector exchange_round(std::vector& wall std::vector new_infos; new_infos.reserve(infos.size()); + for (size_t i = 0; i < wallets.size(); ++i) + new_infos.push_back(wallets[i].exchange_multisig_keys("", infos)); + + return new_infos; +} + +static std::vector exchange_round_force_update(std::vector& wallets, + const std::vector& infos, + const std::size_t round_in_progress) +{ + EXPECT_TRUE(wallets.size() == infos.size()); + std::vector new_infos; + std::vector temp_force_update_infos; + new_infos.reserve(infos.size()); + + // when force-updating, we only need at most 'num_signers - 1 - (round - 1)' messages from other signers + size_t num_other_messages_required{wallets.size() - 1 - (round_in_progress - 1)}; + if (num_other_messages_required > wallets.size()) + num_other_messages_required = 0; //overflow case for post-kex verification round of 1-of-N + for (size_t i = 0; i < wallets.size(); ++i) { - new_infos.push_back(wallets[i].exchange_multisig_keys("", infos)); + temp_force_update_infos.clear(); + temp_force_update_infos.reserve(num_other_messages_required + 1); + temp_force_update_infos.push_back(infos[i]); //always include the local signer's message for this round + + size_t infos_collected{0}; + for (size_t wallet_index = 0; wallet_index < wallets.size(); ++wallet_index) + { + // skip the local signer's message + if (wallet_index == i) + continue; + + temp_force_update_infos.push_back(infos[wallet_index]); + ++infos_collected; + + if (infos_collected == num_other_messages_required) + break; + } + + new_infos.push_back(wallets[i].exchange_multisig_keys("", temp_force_update_infos, true)); } return new_infos; @@ -105,7 +143,7 @@ static std::vector exchange_round(std::vector& wall static void check_results(const std::vector &intermediate_infos, std::vector& wallets, - std::uint32_t M) + const std::uint32_t M) { // check results std::unordered_set unique_privkeys; @@ -167,8 +205,9 @@ static void check_results(const std::vector &intermediate_infos, wallets[0].encrypt_keys(""); } -static void make_wallets(std::vector& wallets, unsigned int M) +static void make_wallets(const unsigned int M, const unsigned int N, const bool force_update) { + std::vector wallets(N); ASSERT_TRUE(wallets.size() > 1 && wallets.size() <= KEYS_COUNT); ASSERT_TRUE(M <= wallets.size()); std::uint32_t total_rounds_required = multisig::multisig_setup_rounds_required(wallets.size(), M); @@ -207,9 +246,12 @@ static void make_wallets(std::vector& wallets, unsigned int M) wallets[0].multisig(&ready); while (!ready) { - intermediate_infos = exchange_round(wallets, intermediate_infos); - wallets[0].multisig(&ready); + if (force_update) + intermediate_infos = exchange_round_force_update(wallets, intermediate_infos, rounds_complete + 1); + else + intermediate_infos = exchange_round(wallets, intermediate_infos); + wallets[0].multisig(&ready); ++rounds_complete; } @@ -220,38 +262,38 @@ static void make_wallets(std::vector& wallets, unsigned int M) TEST(multisig, make_1_2) { - std::vector wallets(2); - make_wallets(wallets, 1); + make_wallets(1, 2, false); + make_wallets(1, 2, true); } TEST(multisig, make_1_3) { - std::vector wallets(3); - make_wallets(wallets, 1); + make_wallets(1, 3, false); + make_wallets(1, 3, true); } TEST(multisig, make_2_2) { - std::vector wallets(2); - make_wallets(wallets, 2); + make_wallets(2, 2, false); + make_wallets(2, 2, true); } TEST(multisig, make_3_3) { - std::vector wallets(3); - make_wallets(wallets, 3); + make_wallets(3, 3, false); + make_wallets(3, 3, true); } TEST(multisig, make_2_3) { - std::vector wallets(3); - make_wallets(wallets, 2); + make_wallets(2, 3, false); + make_wallets(2, 3, true); } TEST(multisig, make_2_4) { - std::vector wallets(4); - make_wallets(wallets, 2); + make_wallets(2, 4, false); + make_wallets(2, 4, true); } TEST(multisig, multisig_kex_msg) @@ -272,9 +314,7 @@ TEST(multisig, multisig_kex_msg) signing_skey = rct::rct2sk(rct::skGen()); } - crypto::secret_key ancillary_skey = rct::rct2sk(rct::skGen()); - while (ancillary_skey == crypto::null_skey) - ancillary_skey = rct::rct2sk(rct::skGen()); + const crypto::secret_key ancillary_skey{rct::rct2sk(rct::skGen())}; // misc. edge cases EXPECT_NO_THROW((multisig_kex_msg{})); @@ -312,8 +352,8 @@ TEST(multisig, multisig_kex_msg) // test that keys can be recovered if stored in a message and the message's reverse // round 1 - multisig_kex_msg msg_rnd1{1, signing_skey, std::vector{pubkey1}, ancillary_skey}; - multisig_kex_msg msg_rnd1_reverse{msg_rnd1.get_msg()}; + const multisig_kex_msg msg_rnd1{1, signing_skey, std::vector{pubkey1}, ancillary_skey}; + const multisig_kex_msg msg_rnd1_reverse{msg_rnd1.get_msg()}; EXPECT_EQ(msg_rnd1.get_round(), 1); EXPECT_EQ(msg_rnd1.get_round(), msg_rnd1_reverse.get_round()); EXPECT_EQ(msg_rnd1.get_signing_pubkey(), signing_pubkey); @@ -324,8 +364,8 @@ TEST(multisig, multisig_kex_msg) EXPECT_EQ(msg_rnd1.get_msg_privkey(), msg_rnd1_reverse.get_msg_privkey()); // round 2 - multisig_kex_msg msg_rnd2{2, signing_skey, std::vector{pubkey1, pubkey2}, ancillary_skey}; - multisig_kex_msg msg_rnd2_reverse{msg_rnd2.get_msg()}; + const multisig_kex_msg msg_rnd2{2, signing_skey, std::vector{pubkey1, pubkey2}, ancillary_skey}; + const multisig_kex_msg msg_rnd2_reverse{msg_rnd2.get_msg()}; EXPECT_EQ(msg_rnd2.get_round(), 2); EXPECT_EQ(msg_rnd2.get_round(), msg_rnd2_reverse.get_round()); EXPECT_EQ(msg_rnd2.get_signing_pubkey(), signing_pubkey); diff --git a/utils/python-rpc/framework/wallet.py b/utils/python-rpc/framework/wallet.py index f2618b0a8..0f6db76bd 100644 --- a/utils/python-rpc/framework/wallet.py +++ b/utils/python-rpc/framework/wallet.py @@ -524,12 +524,13 @@ class Wallet(object): } return self.rpc.send_json_rpc_request(finalize_multisig) - def exchange_multisig_keys(self, multisig_info, password = ''): + def exchange_multisig_keys(self, multisig_info, password = '', force_update_use_with_caution = False): exchange_multisig_keys = { 'method': 'exchange_multisig_keys', 'params' : { 'multisig_info': multisig_info, 'password': password, + 'force_update_use_with_caution': force_update_use_with_caution, }, 'jsonrpc': '2.0', 'id': '0'