wallet: try to save large outputs when using an unneeded second input

When a single input is enough to satisfy a transfer, the code would
previously try to add a second input, to match the "canonical" makeup
of a transaction with two inputs and two outputs. This would cause
wallets to slowly merge outputs till all the monero ends up in a
single output, which causes trouble when making two transactions
one after the other, since change is locked for 10 blocks, and an
increasing portion of the remaining balance would end up locked on
each transaction.

There are two new settings (min-output-count and min-output-value)
which can control when to stop adding such unneeded second outputs.
The idea is that small "dust" outputs will still get added, but
larger ones will not.

Enable with, eg:

set min-output-count 10
set min-output-value 30

to avoid using an unneeded second output of 30 monero or more, if
there would be less than 10 such outputs left.

This does not invalidate any other reason why such outputs would
be used (ie, when they're really needed to satisfy a transfer, or
when randomly picked in the normal course of selection). This may
be improved in the future.

4 files changed, 106 insertions, 4 deletions

diff --git a/src/simplewallet/simplewallet.cpp b/src/simplewallet/simplewallet.cpp
index 6cbdf1a1e..9c58d71d4 100644
--- a/src/simplewallet/simplewallet.cpp
+++ b/src/simplewallet/simplewallet.cpp
@@ -615,6 +615,42 @@ bool simple_wallet::set_unit(const std::vector<std::string> &args/* = std::vecto
   return true;
 }
 
+bool simple_wallet::set_min_output_count(const std::vector<std::string> &args/* = std::vector<std::string>()*/)
+{
+  uint32_t count;
+  if (!string_tools::get_xtype_from_string(count, args[1]))
+  {
+    fail_msg_writer() << tr("invalid count: must be an unsigned integer");
+    return true;
+  }
+
+  const auto pwd_container = get_and_verify_password();
+  if (pwd_container)
+  {
+    m_wallet->set_min_output_count(count);
+    m_wallet->rewrite(m_wallet_file, pwd_container->password());
+  }
+  return true;
+}
+
+bool simple_wallet::set_min_output_value(const std::vector<std::string> &args/* = std::vector<std::string>()*/)
+{
+  uint64_t value;
+  if (!cryptonote::parse_amount(value, args[1]))
+  {
+    fail_msg_writer() << tr("invalid value");
+    return true;
+  }
+
+  const auto pwd_container = get_and_verify_password();
+  if (pwd_container)
+  {
+    m_wallet->set_min_output_value(value);
+    m_wallet->rewrite(m_wallet_file, pwd_container->password());
+  }
+  return true;
+}
+
 bool simple_wallet::help(const std::vector<std::string> &args/* = std::vector<std::string>()*/)
 {
   success_msg_writer() << get_commands_str();
@@ -654,7 +690,7 @@ simple_wallet::simple_wallet()
   m_cmd_binder.set_handler("viewkey", boost::bind(&simple_wallet::viewkey, this, _1), tr("Display private view key"));
   m_cmd_binder.set_handler("spendkey", boost::bind(&simple_wallet::spendkey, this, _1), tr("Display private spend key"));
   m_cmd_binder.set_handler("seed", boost::bind(&simple_wallet::seed, this, _1), tr("Display Electrum-style mnemonic seed"));
-  m_cmd_binder.set_handler("set", boost::bind(&simple_wallet::set_variable, this, _1), tr("Available options: seed language - set wallet seed language; always-confirm-transfers <1|0> - whether to confirm unsplit txes; print-ring-members <1|0> - whether to print detailed information about ring members during confirmation; store-tx-info <1|0> - whether to store outgoing tx info (destination address, payment ID, tx secret key) for future reference; default-mixin <n> - set default mixin (default is 4); auto-refresh <1|0> - whether to automatically sync new blocks from the daemon; refresh-type <full|optimize-coinbase|no-coinbase|default> - set wallet refresh behaviour; priority [0|1|2|3|4] - default/unimportant/normal/elevated/priority fee; confirm-missing-payment-id <1|0>; ask-password <1|0>; unit <monero|millinero|micronero|nanonero|piconero> - set default monero (sub-)unit"));
+  m_cmd_binder.set_handler("set", boost::bind(&simple_wallet::set_variable, this, _1), tr("Available options: seed language - set wallet seed language; always-confirm-transfers <1|0> - whether to confirm unsplit txes; print-ring-members <1|0> - whether to print detailed information about ring members during confirmation; store-tx-info <1|0> - whether to store outgoing tx info (destination address, payment ID, tx secret key) for future reference; default-mixin <n> - set default mixin (default is 4); auto-refresh <1|0> - whether to automatically sync new blocks from the daemon; refresh-type <full|optimize-coinbase|no-coinbase|default> - set wallet refresh behaviour; priority [0|1|2|3|4] - default/unimportant/normal/elevated/priority fee; confirm-missing-payment-id <1|0>; ask-password <1|0>; unit <monero|millinero|micronero|nanonero|piconero> - set default monero (sub-)unit; min-output-count [n] - try to keep at least that many outputs of value at least min-output-value; min-output-value [n] - try to keep at least min-output-count outputs of at least that value"));
   m_cmd_binder.set_handler("rescan_spent", boost::bind(&simple_wallet::rescan_spent, this, _1), tr("Rescan blockchain for spent outputs"));
   m_cmd_binder.set_handler("get_tx_key", boost::bind(&simple_wallet::get_tx_key, this, _1), tr("Get transaction key (r) for a given <txid>"));
   m_cmd_binder.set_handler("check_tx_key", boost::bind(&simple_wallet::check_tx_key, this, _1), tr("Check amount going to <address> in <txid>"));
@@ -690,6 +726,8 @@ bool simple_wallet::set_variable(const std::vector<std::string> &args)
     success_msg_writer() << "confirm-missing-payment-id = " << m_wallet->confirm_missing_payment_id();
     success_msg_writer() << "ask-password = " << m_wallet->ask_password();
     success_msg_writer() << "unit = " << cryptonote::get_unit(m_wallet->get_default_decimal_point());
+    success_msg_writer() << "min-outputs-count = " << m_wallet->get_min_output_count();
+    success_msg_writer() << "min-outputs-value = " << cryptonote::print_money(m_wallet->get_min_output_value());
     return true;
   }
   else
@@ -838,6 +876,32 @@ bool simple_wallet::set_variable(const std::vector<std::string> &args)
         return true;
       }
     }
+    else if (args[0] == "min-outputs-count")
+    {
+      if (args.size() <= 1)
+      {
+        fail_msg_writer() << tr("set min-outputs-count: needs an argument (unsigned integer)");
+        return true;
+      }
+      else
+      {
+        set_min_output_count(args);
+        return true;
+      }
+    }
+    else if (args[0] == "min-outputs-value")
+    {
+      if (args.size() <= 1)
+      {
+        fail_msg_writer() << tr("set min-outputs-value: needs an argument (unsigned integer)");
+        return true;
+      }
+      else
+      {
+        set_min_output_value(args);
+        return true;
+      }
+    }
   }
   fail_msg_writer() << tr("set: unrecognized argument(s)");
   return true;
diff --git a/src/simplewallet/simplewallet.h b/src/simplewallet/simplewallet.h
index c7d65633f..bb668b123 100644
--- a/src/simplewallet/simplewallet.h
+++ b/src/simplewallet/simplewallet.h
@@ -117,6 +117,8 @@ namespace cryptonote
     bool set_confirm_missing_payment_id(const std::vector<std::string> &args = std::vector<std::string>());
     bool set_ask_password(const std::vector<std::string> &args = std::vector<std::string>());
     bool set_unit(const std::vector<std::string> &args = std::vector<std::string>());
+    bool set_min_output_count(const std::vector<std::string> &args = std::vector<std::string>());
+    bool set_min_output_value(const std::vector<std::string> &args = std::vector<std::string>());
     bool help(const std::vector<std::string> &args = std::vector<std::string>());
     bool start_mining(const std::vector<std::string> &args);
     bool stop_mining(const std::vector<std::string> &args);
diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index b3791b99c..09df30e66 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -1918,6 +1918,12 @@ bool wallet2::store_keys(const std::string& keys_file_name, const std::string& p
   value2.SetInt(m_ask_password ? 1 :0);
   json.AddMember("ask_password", value2, json.GetAllocator());
 
+  value2.SetUint(m_min_output_count);
+  json.AddMember("min_output_count", value2, json.GetAllocator());
+
+  value2.SetUint64(m_min_output_value);
+  json.AddMember("min_output_value", value2, json.GetAllocator());
+
   value2.SetInt(cryptonote::get_default_decimal_point());
   json.AddMember("default_decimal_point", value2, json.GetAllocator());
 
@@ -1989,6 +1995,8 @@ bool wallet2::load_keys(const std::string& keys_file_name, const std::string& pa
     m_refresh_type = RefreshType::RefreshDefault;
     m_confirm_missing_payment_id = true;
     m_ask_password = true;
+    m_min_output_count = 0;
+    m_min_output_value = 0;
   }
   else
   {
@@ -2053,6 +2061,10 @@ bool wallet2::load_keys(const std::string& keys_file_name, const std::string& pa
     m_ask_password = field_ask_password;
     GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, default_decimal_point, int, Int, false, CRYPTONOTE_DISPLAY_DECIMAL_POINT);
     cryptonote::set_default_decimal_point(field_default_decimal_point);
+    GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, min_output_count, uint32_t, Uint, false, 0);
+    m_min_output_count = field_min_output_count;
+    GET_FIELD_FROM_JSON_RETURN_ON_ERROR(json, min_output_value, uint64_t, Uint64, false, 0);
+    m_min_output_value = field_min_output_value;
   }
 
   const cryptonote::account_keys& keys = m_account.get_keys();
@@ -4189,6 +4201,15 @@ std::vector<size_t> wallet2::get_only_rct(const std::vector<size_t> &unused_dust
   return indices;
 }
 
+static uint32_t get_count_above(const std::vector<wallet2::transfer_details> &transfers, const std::vector<size_t> &indices, uint64_t threshold)
+{
+  uint32_t count = 0;
+  for (size_t idx: indices)
+    if (transfers[idx].amount() >= threshold)
+      ++count;
+  return count;
+}
+
 // Another implementation of transaction creation that is hopefully better
 // While there is anything left to pay, it goes through random outputs and tries
 // to fill the next destination/amount. If it fully fills it, it will use the
@@ -4339,12 +4360,20 @@ std::vector<wallet2::pending_tx> wallet2::create_transactions_2(std::vector<cryp
       std::vector<size_t> indices = get_only_rct(unused_dust_indices, unused_transfers_indices);
       idx = pop_best_value(indices, tx.selected_transfers, true);
 
+      // we might not want to add it if it's a large output and we don't have many left
+      if (m_transfers[idx].amount() >= m_min_output_value) {
+        if (get_count_above(m_transfers, unused_transfers_indices, m_min_output_value) < m_min_output_count) {
+          LOG_PRINT_L2("Second output was not strictly needed, and we're running out of outputs above " << print_money(m_min_output_value) << ", not adding");
+          break;
+        }
+      }
+
       // since we're trying to add a second output which is not strictly needed,
       // we only add it if it's unrelated enough to the first one
       float relatedness = get_output_relatedness(m_transfers[idx], m_transfers[tx.selected_transfers.front()]);
       if (relatedness > SECOND_OUTPUT_RELATEDNESS_THRESHOLD)
       {
-        LOG_PRINT_L2("Second outout was not strictly needed, and relatedness " << relatedness << ", not adding");
+        LOG_PRINT_L2("Second output was not strictly needed, and relatedness " << relatedness << ", not adding");
         break;
       }
       pop_if_present(unused_transfers_indices, idx);
diff --git a/src/wallet/wallet2.h b/src/wallet/wallet2.h
index 565cd9870..d0d2cda3c 100644
--- a/src/wallet/wallet2.h
+++ b/src/wallet/wallet2.h
@@ -105,7 +105,7 @@ namespace tools
     };
 
   private:
-    wallet2(const wallet2&) : m_run(true), m_callback(0), m_testnet(false), m_always_confirm_transfers(true), m_print_ring_members(false), m_store_tx_info(true), m_default_mixin(0), m_default_priority(0), m_refresh_type(RefreshOptimizeCoinbase), m_auto_refresh(true), m_refresh_from_block_height(0), m_confirm_missing_payment_id(true), m_ask_password(true), m_node_rpc_proxy(m_http_client, m_daemon_rpc_mutex) {}
+    wallet2(const wallet2&) : m_run(true), m_callback(0), m_testnet(false), m_always_confirm_transfers(true), m_print_ring_members(false), m_store_tx_info(true), m_default_mixin(0), m_default_priority(0), m_refresh_type(RefreshOptimizeCoinbase), m_auto_refresh(true), m_refresh_from_block_height(0), m_confirm_missing_payment_id(true), m_ask_password(true), m_min_output_count(0), m_min_output_value(0), m_node_rpc_proxy(m_http_client, m_daemon_rpc_mutex) {}
 
   public:
     static const char* tr(const char* str);
@@ -126,7 +126,8 @@ namespace tools
     //! Uses stdin and stdout. Returns a wallet2 and password for wallet with no file if no errors.
     static std::pair<std::unique_ptr<wallet2>, password_container> make_new(const boost::program_options::variables_map& vm);
 
-    wallet2(bool testnet = false, bool restricted = false) : m_run(true), m_callback(0), m_testnet(testnet), m_always_confirm_transfers(true), m_print_ring_members(false), m_store_tx_info(true), m_default_mixin(0), m_default_priority(0), m_refresh_type(RefreshOptimizeCoinbase), m_auto_refresh(true), m_refresh_from_block_height(0), m_confirm_missing_payment_id(true), m_ask_password(true), m_restricted(restricted), is_old_file_format(false), m_node_rpc_proxy(m_http_client, m_daemon_rpc_mutex) {}
+    wallet2(bool testnet = false, bool restricted = false) : m_run(true), m_callback(0), m_testnet(testnet), m_always_confirm_transfers(true), m_print_ring_members(false), m_store_tx_info(true), m_default_mixin(0), m_default_priority(0), m_refresh_type(RefreshOptimizeCoinbase), m_auto_refresh(true), m_refresh_from_block_height(0), m_confirm_missing_payment_id(true), m_ask_password(true), m_min_output_count(0), m_min_output_value(0), m_restricted(restricted), is_old_file_format(false), m_node_rpc_proxy(m_http_client, m_daemon_rpc_mutex) {}
+
     struct transfer_details
     {
       uint64_t m_block_height;
@@ -526,6 +527,10 @@ namespace tools
     void ask_password(bool always) { m_ask_password = always; }
     void set_default_decimal_point(unsigned int decimal_point);
     unsigned int get_default_decimal_point() const;
+    void set_min_output_count(uint32_t count) { m_min_output_count = count; }
+    uint32_t get_min_output_count() const { return m_min_output_count; }
+    void set_min_output_value(uint64_t value) { m_min_output_value = value; }
+    uint64_t get_min_output_value() const { return m_min_output_value; }
 
     bool get_tx_key(const crypto::hash &txid, crypto::secret_key &tx_key) const;
 
@@ -682,6 +687,8 @@ namespace tools
     uint64_t m_refresh_from_block_height;
     bool m_confirm_missing_payment_id;
     bool m_ask_password;
+    uint32_t m_min_output_count;
+    uint64_t m_min_output_value;
     NodeRPCProxy m_node_rpc_proxy;
     std::unordered_set<crypto::hash> m_scanned_pool_txs[2];
   };