From 48d0747d005c80fde3837fc4bcdb3eff26907d74 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Sat, 2 Apr 2016 13:06:39 +0100 Subject: [PATCH] wallet: better output selection for transfer/transfer_new This now requests the set of outputs that can be mixed first, to avoid trying non dust but unmixable outputs, which we know will fail. --- src/simplewallet/simplewallet.cpp | 4 +- src/wallet/wallet2.cpp | 89 ++++++++----------- src/wallet/wallet2.h | 30 +++---- src/wallet/wallet_rpc_server.cpp | 6 +- src/wallet/wallet_rpc_server_commands_defs.h | 4 + .../transactions_flow_test.cpp | 2 +- 6 files changed, 61 insertions(+), 74 deletions(-) diff --git a/src/simplewallet/simplewallet.cpp b/src/simplewallet/simplewallet.cpp index aa571755..f4293760 100644 --- a/src/simplewallet/simplewallet.cpp +++ b/src/simplewallet/simplewallet.cpp @@ -2133,9 +2133,9 @@ bool simple_wallet::transfer_main(bool new_algorithm, const std::vector ptx_vector; if (new_algorithm) - ptx_vector = m_wallet->create_transactions_2(dsts, fake_outs_count, 0 /* unlock_time */, 0 /* unused fee arg*/, extra); + ptx_vector = m_wallet->create_transactions_2(dsts, fake_outs_count, 0 /* unlock_time */, 0 /* unused fee arg*/, extra, m_trusted_daemon); else - ptx_vector = m_wallet->create_transactions(dsts, fake_outs_count, 0 /* unlock_time */, 0 /* unused fee arg*/, extra); + ptx_vector = m_wallet->create_transactions(dsts, fake_outs_count, 0 /* unlock_time */, 0 /* unused fee arg*/, extra, m_trusted_daemon); // if more than one tx necessary, prompt user to confirm if (m_wallet->always_confirm_transfers() || ptx_vector.size() > 1) diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index a9a65535..547336cb 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -1749,47 +1749,12 @@ namespace // returns: // direct return: amount of money found // modified reference: selected_transfers, a list of iterators/indices of input sources -uint64_t wallet2::select_transfers(uint64_t needed_money, bool add_dust, uint64_t dust, bool hf2_rules, std::list& selected_transfers) +uint64_t wallet2::select_transfers(uint64_t needed_money, std::vector unused_transfers_indices, std::list& selected_transfers, bool trusted_daemon) { - std::vector unused_transfers_indices; - std::vector unused_dust_indices; - - // aggregate sources available for transfers - // if dust needed, take dust from only one source (so require source has at least dust amount) - for (size_t i = 0; i < m_transfers.size(); ++i) - { - const transfer_details& td = m_transfers[i]; - if (!td.m_spent && is_transfer_unlocked(td)) - { - if (dust < td.amount() && is_valid_decomposed_amount(td.amount())) - unused_transfers_indices.push_back(i); - else - { - // for hf2 rules, we disregard dust, which will be spendable only - // via sweep_dust. If we're asked to add dust, though, we still - // consider them, as this will be a mixin 0 tx (and thus we may - // end up with a tx with one mixable output and N dusty ones). - // This should be made better at some point... - if (!hf2_rules || add_dust) - unused_dust_indices.push_back(i); - } - } - } - - bool select_one_dust = add_dust && !unused_dust_indices.empty(); uint64_t found_money = 0; - while (found_money < needed_money && (!unused_transfers_indices.empty() || !unused_dust_indices.empty())) + while (found_money < needed_money && !unused_transfers_indices.empty()) { - size_t idx; - if (select_one_dust) - { - idx = pop_random_value(unused_dust_indices); - select_one_dust = false; - } - else - { - idx = !unused_transfers_indices.empty() ? pop_random_value(unused_transfers_indices) : pop_random_value(unused_dust_indices); - } + size_t idx = pop_random_value(unused_transfers_indices); transfer_container::iterator it = m_transfers.begin() + idx; selected_transfers.push_back(it); @@ -1811,18 +1776,18 @@ void wallet2::add_unconfirmed_tx(const cryptonote::transaction& tx, const std::v } //---------------------------------------------------------------------------------------------------- -void wallet2::transfer(const std::vector& dsts, size_t fake_outputs_count, - uint64_t unlock_time, uint64_t fee, const std::vector& extra, cryptonote::transaction& tx, pending_tx& ptx) +void wallet2::transfer(const std::vector& dsts, const size_t fake_outs_count, const std::vector &unused_transfers_indices, + uint64_t unlock_time, uint64_t fee, const std::vector& extra, cryptonote::transaction& tx, pending_tx& ptx, bool trusted_daemon) { - transfer(dsts, fake_outputs_count, unlock_time, fee, extra, detail::digit_split_strategy, tx_dust_policy(::config::DEFAULT_DUST_THRESHOLD), tx, ptx); + transfer(dsts, fake_outs_count, unused_transfers_indices, unlock_time, fee, extra, detail::digit_split_strategy, tx_dust_policy(::config::DEFAULT_DUST_THRESHOLD), tx, ptx, trusted_daemon); } //---------------------------------------------------------------------------------------------------- -void wallet2::transfer(const std::vector& dsts, size_t fake_outputs_count, - uint64_t unlock_time, uint64_t fee, const std::vector& extra) +void wallet2::transfer(const std::vector& dsts, const size_t fake_outs_count, const std::vector &unused_transfers_indices, + uint64_t unlock_time, uint64_t fee, const std::vector& extra, bool trusted_daemon) { cryptonote::transaction tx; pending_tx ptx; - transfer(dsts, fake_outputs_count, unlock_time, fee, extra, tx, ptx); + transfer(dsts, fake_outs_count, unused_transfers_indices, unlock_time, fee, extra, tx, ptx, trusted_daemon); } namespace { @@ -2019,8 +1984,9 @@ void wallet2::commit_tx(std::vector& ptx_vector) // // this function will make multiple calls to wallet2::transfer if multiple // transactions will be required -std::vector wallet2::create_transactions(std::vector dsts, const size_t fake_outs_count, const uint64_t unlock_time, const uint64_t fee_UNUSED, const std::vector extra) +std::vector wallet2::create_transactions(std::vector dsts, const size_t fake_outs_count, const uint64_t unlock_time, const uint64_t fee_UNUSED, const std::vector extra, bool trusted_daemon) { + const std::vector unused_transfers_indices = select_available_outputs_from_histogram(fake_outs_count + 1, true, trusted_daemon); // failsafe split attempt counter size_t attempt_count = 0; @@ -2050,7 +2016,7 @@ std::vector wallet2::create_transactions(std::vector wallet2::create_transactions_2(std::vector dsts, const size_t fake_outs_count, const uint64_t unlock_time, const uint64_t fee_UNUSED, const std::vector extra) +std::vector wallet2::create_transactions_2(std::vector dsts, const size_t fake_outs_count, const uint64_t unlock_time, const uint64_t fee_UNUSED, const std::vector extra, bool trusted_daemon) { std::vector unused_transfers_indices; std::vector unused_dust_indices; @@ -2703,9 +2669,8 @@ std::vector wallet2::get_unspent_amounts_vector() return vector; } //---------------------------------------------------------------------------------------------------- -std::vector wallet2::select_available_unmixable_outputs(bool trusted_daemon) +std::vector wallet2::select_available_outputs_from_histogram(uint64_t count, bool atleast, bool trusted_daemon) { - // request all outputs with at least 3 instances, so we can use mixin 2 with epee::json_rpc::request req_t = AUTO_VAL_INIT(req_t); epee::json_rpc::response resp_t = AUTO_VAL_INIT(resp_t); m_daemon_rpc_mutex.lock(); @@ -2714,7 +2679,7 @@ std::vector wallet2::select_available_unmixable_outputs(bool trusted_dae req_t.method = "get_output_histogram"; if (trusted_daemon) req_t.params.amounts = get_unspent_amounts_vector(); - req_t.params.min_count = 3; + req_t.params.min_count = count; req_t.params.max_count = 0; bool r = net_utils::invoke_http_json_remote_command2(m_daemon_address + "/json_rpc", req_t, resp_t, m_http_client); m_daemon_rpc_mutex.unlock(); @@ -2728,14 +2693,32 @@ std::vector wallet2::select_available_unmixable_outputs(bool trusted_dae mixable.insert(i.amount); } - return select_available_outputs([mixable](const transfer_details &td) { + return select_available_outputs([mixable, atleast](const transfer_details &td) { const uint64_t amount = td.amount(); - if (mixable.find(amount) == mixable.end()) - return true; + if (atleast) { + if (mixable.find(amount) != mixable.end()) + return true; + } + else { + if (mixable.find(amount) == mixable.end()) + return true; + } return false; }); } //---------------------------------------------------------------------------------------------------- +std::vector wallet2::select_available_unmixable_outputs(bool trusted_daemon) +{ + // request all outputs with less than 3 instances + return select_available_outputs_from_histogram(3, false, trusted_daemon); +} +//---------------------------------------------------------------------------------------------------- +std::vector wallet2::select_available_mixable_outputs(bool trusted_daemon) +{ + // request all outputs with at least 3 instances, so we can use mixin 2 with + return select_available_outputs_from_histogram(3, true, trusted_daemon); +} +//---------------------------------------------------------------------------------------------------- std::vector wallet2::create_unmixable_sweep_transactions(bool trusted_daemon) { // From hard fork 1, we don't consider small amounts to be dust anymore diff --git a/src/wallet/wallet2.h b/src/wallet/wallet2.h index b6466d3f..179d1553 100644 --- a/src/wallet/wallet2.h +++ b/src/wallet/wallet2.h @@ -274,11 +274,11 @@ namespace tools uint64_t unlocked_balance() const; uint64_t unlocked_dust_balance(const tx_dust_policy &dust_policy) const; template - void transfer(const std::vector& dsts, size_t fake_outputs_count, uint64_t unlock_time, uint64_t fee, const std::vector& extra, T destination_split_strategy, const tx_dust_policy& dust_policy); + void transfer(const std::vector& dsts, const size_t fake_outputs_count, const std::vector &unused_transfers_indices, uint64_t unlock_time, uint64_t fee, const std::vector& extra, T destination_split_strategy, const tx_dust_policy& dust_policy, bool trusted_daemon); template - void transfer(const std::vector& dsts, size_t fake_outputs_count, uint64_t unlock_time, uint64_t fee, const std::vector& extra, T destination_split_strategy, const tx_dust_policy& dust_policy, cryptonote::transaction& tx, pending_tx& ptx); - void transfer(const std::vector& dsts, size_t fake_outputs_count, uint64_t unlock_time, uint64_t fee, const std::vector& extra); - void transfer(const std::vector& dsts, size_t fake_outputs_count, uint64_t unlock_time, uint64_t fee, const std::vector& extra, cryptonote::transaction& tx, pending_tx& ptx); + void transfer(const std::vector& dsts, const size_t fake_outputs_count, const std::vector &unused_transfers_indices, uint64_t unlock_time, uint64_t fee, const std::vector& extra, T destination_split_strategy, const tx_dust_policy& dust_policy, cryptonote::transaction& tx, pending_tx& ptx, bool trusted_daemon); + void transfer(const std::vector& dsts, const size_t fake_outputs_count, const std::vector &unused_transfers_indices, uint64_t unlock_time, uint64_t fee, const std::vector& extra, bool trusted_daemon); + void transfer(const std::vector& dsts, const size_t fake_outputs_count, const std::vector &unused_transfers_indices, uint64_t unlock_time, uint64_t fee, const std::vector& extra, cryptonote::transaction& tx, pending_tx& ptx, bool trusted_daemon); template void transfer_from(const std::vector &outs, size_t num_outputs, uint64_t unlock_time, uint64_t needed_fee, T destination_split_strategy, const tx_dust_policy& dust_policy, const std::vector& extra, cryptonote::transaction& tx, pending_tx &ptx); template @@ -287,8 +287,8 @@ namespace tools void commit_tx(pending_tx& ptx_vector); void commit_tx(std::vector& ptx_vector); - std::vector create_transactions(std::vector dsts, const size_t fake_outs_count, const uint64_t unlock_time, const uint64_t fee, const std::vector extra); - std::vector create_transactions_2(std::vector dsts, const size_t fake_outs_count, const uint64_t unlock_time, const uint64_t fee_UNUSED, const std::vector extra); + std::vector create_transactions(std::vector dsts, const size_t fake_outs_count, const uint64_t unlock_time, const uint64_t fee, const std::vector extra, bool trusted_daemon); + std::vector create_transactions_2(std::vector dsts, const size_t fake_outs_count, const uint64_t unlock_time, const uint64_t fee_UNUSED, const std::vector extra, bool trusted_daemon); std::vector create_unmixable_sweep_transactions(bool trusted_daemon); bool check_connection(); void get_transfers(wallet2::transfer_container& incoming_transfers) const; @@ -389,7 +389,7 @@ namespace tools void pull_blocks(uint64_t start_height, uint64_t& blocks_start_height, const std::list &short_chain_history, std::list &blocks); void pull_next_blocks(uint64_t start_height, uint64_t &blocks_start_height, std::list &short_chain_history, const std::list &prev_blocks, std::list &blocks, bool &error); void process_blocks(uint64_t start_height, const std::list &blocks, uint64_t& blocks_added); - uint64_t select_transfers(uint64_t needed_money, bool add_dust, uint64_t dust, bool hf2_rules, std::list& selected_transfers); + uint64_t select_transfers(uint64_t needed_money, std::vector unused_transfers_indices, std::list& selected_transfers, bool trusted_daemon); bool prepare_file_names(const std::string& file_path); void process_unconfirmed(const cryptonote::transaction& tx, uint64_t height); void process_outgoing(const cryptonote::transaction& tx, uint64_t height, uint64_t spent, uint64_t received); @@ -403,8 +403,10 @@ namespace tools uint64_t get_upper_tranaction_size_limit(); void check_pending_txes(); std::vector get_unspent_amounts_vector(); + std::vector select_available_outputs_from_histogram(uint64_t count, bool atleast, bool trusted_daemon); std::vector select_available_outputs(const std::function &f); std::vector select_available_unmixable_outputs(bool trusted_daemon); + std::vector select_available_mixable_outputs(bool trusted_daemon); cryptonote::account_base m_account; std::string m_daemon_address; @@ -562,17 +564,17 @@ namespace tools } //---------------------------------------------------------------------------------------------------- template - void wallet2::transfer(const std::vector& dsts, size_t fake_outputs_count, - uint64_t unlock_time, uint64_t fee, const std::vector& extra, T destination_split_strategy, const tx_dust_policy& dust_policy) + void wallet2::transfer(const std::vector& dsts, const size_t fake_outs_count, const std::vector &unused_transfers_indices, + uint64_t unlock_time, uint64_t fee, const std::vector& extra, T destination_split_strategy, const tx_dust_policy& dust_policy, bool trusted_daemon) { pending_tx ptx; cryptonote::transaction tx; - transfer(dsts, fake_outputs_count, unlock_time, fee, extra, destination_split_strategy, dust_policy, tx, ptx); + transfer(dsts, fake_outs_count, unused_transfers_indices, unlock_time, fee, extra, destination_split_strategy, dust_policy, tx, ptx, trusted_daemon); } template - void wallet2::transfer(const std::vector& dsts, size_t fake_outputs_count, - uint64_t unlock_time, uint64_t fee, const std::vector& extra, T destination_split_strategy, const tx_dust_policy& dust_policy, cryptonote::transaction& tx, pending_tx &ptx) + void wallet2::transfer(const std::vector& dsts, size_t fake_outputs_count, const std::vector &unused_transfers_indices, + uint64_t unlock_time, uint64_t fee, const std::vector& extra, T destination_split_strategy, const tx_dust_policy& dust_policy, cryptonote::transaction& tx, pending_tx &ptx, bool trusted_daemon) { using namespace cryptonote; // throw if attempting a transaction with no destinations @@ -593,9 +595,7 @@ namespace tools // randomly select inputs for transaction // throw if requested send amount is greater than amount available to send std::list selected_transfers; - bool hf2_rules = use_fork_rules(2); // first fork has version 2 - const bool add_dust = (0 == fake_outputs_count) && hf2_rules; - uint64_t found_money = select_transfers(needed_money, add_dust, dust_policy.dust_threshold, hf2_rules, selected_transfers); + uint64_t found_money = select_transfers(needed_money, unused_transfers_indices, selected_transfers, trusted_daemon); THROW_WALLET_EXCEPTION_IF(found_money < needed_money, error::not_enough_money, found_money, needed_money - fee, fee); typedef COMMAND_RPC_GET_RANDOM_OUTPUTS_FOR_AMOUNTS::out_entry out_entry; diff --git a/src/wallet/wallet_rpc_server.cpp b/src/wallet/wallet_rpc_server.cpp index d7d99c2a..6897c3d4 100644 --- a/src/wallet/wallet_rpc_server.cpp +++ b/src/wallet/wallet_rpc_server.cpp @@ -232,7 +232,7 @@ namespace tools LOG_PRINT_L1("Requested mixin " << req.mixin << " too low for hard fork 2, using 2"); mixin = 2; } - std::vector ptx_vector = m_wallet.create_transactions(dsts, mixin, req.unlock_time, req.fee, extra); + std::vector ptx_vector = m_wallet.create_transactions(dsts, mixin, req.unlock_time, req.fee, extra, req.trusted_daemon); // reject proposed transactions if there are more than one. see on_transfer_split below. if (ptx_vector.size() != 1) @@ -299,9 +299,9 @@ namespace tools } std::vector ptx_vector; if (req.new_algorithm) - ptx_vector = m_wallet.create_transactions_2(dsts, mixin, req.unlock_time, req.fee, extra); + ptx_vector = m_wallet.create_transactions_2(dsts, mixin, req.unlock_time, req.fee, extra, req.trusted_daemon); else - ptx_vector = m_wallet.create_transactions(dsts, mixin, req.unlock_time, req.fee, extra); + ptx_vector = m_wallet.create_transactions(dsts, mixin, req.unlock_time, req.fee, extra, req.trusted_daemon); m_wallet.commit_tx(ptx_vector); diff --git a/src/wallet/wallet_rpc_server_commands_defs.h b/src/wallet/wallet_rpc_server_commands_defs.h index 2c4e2640..2d90bf62 100644 --- a/src/wallet/wallet_rpc_server_commands_defs.h +++ b/src/wallet/wallet_rpc_server_commands_defs.h @@ -115,6 +115,7 @@ namespace wallet_rpc uint64_t unlock_time; std::string payment_id; bool get_tx_key; + bool trusted_daemon; BEGIN_KV_SERIALIZE_MAP() KV_SERIALIZE(destinations) @@ -123,6 +124,7 @@ namespace wallet_rpc KV_SERIALIZE(unlock_time) KV_SERIALIZE(payment_id) KV_SERIALIZE(get_tx_key) + KV_SERIALIZE(trusted_daemon) END_KV_SERIALIZE_MAP() }; @@ -149,6 +151,7 @@ namespace wallet_rpc std::string payment_id; bool new_algorithm; bool get_tx_keys; + bool trusted_daemon; BEGIN_KV_SERIALIZE_MAP() KV_SERIALIZE(destinations) @@ -158,6 +161,7 @@ namespace wallet_rpc KV_SERIALIZE(payment_id) KV_SERIALIZE(new_algorithm) KV_SERIALIZE(get_tx_keys) + KV_SERIALIZE(trusted_daemon) END_KV_SERIALIZE_MAP() }; diff --git a/tests/functional_tests/transactions_flow_test.cpp b/tests/functional_tests/transactions_flow_test.cpp index 3c3cf3a9..159ccfd8 100644 --- a/tests/functional_tests/transactions_flow_test.cpp +++ b/tests/functional_tests/transactions_flow_test.cpp @@ -85,7 +85,7 @@ bool do_send_money(tools::wallet2& w1, tools::wallet2& w2, size_t mix_in_factor, try { tools::wallet2::pending_tx ptx; - w1.transfer(dsts, mix_in_factor, 0, TEST_FEE, std::vector(), tools::detail::null_split_strategy, tools::tx_dust_policy(TEST_DUST_THRESHOLD), tx, ptx); + w1.transfer(dsts, mix_in_factor, 0, TEST_FEE, std::vector(), tools::detail::null_split_strategy, tools::tx_dust_policy(TEST_DUST_THRESHOLD), tx, ptx, true); w1.commit_tx(ptx); return true; }