From aa4f44b79abefd50e1959e17e900e6fe3d996c93 Mon Sep 17 00:00:00 2001 From: Ivan Sorokin Date: Wed, 13 Feb 2019 00:06:25 +0100 Subject: [PATCH] fix: wallet coin selection respects max_block_weight (#2546) * fix #2510: wallet coin selection respects max_block_weight Deprecate "soft" max_outputs limit and introduce "hard" max_outputs limit based on max_block_weight. * Fix tests --- servers/tests/framework.rs | 2 - servers/tests/simulnet.rs | 1 - src/bin/cmd/wallet_args.rs | 4 -- wallet/src/command.rs | 3 -- wallet/src/controller.rs | 1 - wallet/src/libwallet/api.rs | 19 +------- wallet/src/libwallet/internal/selection.rs | 51 ++++++++-------------- wallet/src/libwallet/internal/tx.rs | 4 -- wallet/src/libwallet/types.rs | 2 - wallet/src/test_framework/mod.rs | 1 - wallet/tests/accounts.rs | 1 - wallet/tests/check.rs | 1 - wallet/tests/file.rs | 1 - wallet/tests/repost.rs | 2 - wallet/tests/restore.rs | 4 -- wallet/tests/self_send.rs | 1 - wallet/tests/transaction.rs | 23 +++++++--- 17 files changed, 39 insertions(+), 82 deletions(-) diff --git a/servers/tests/framework.rs b/servers/tests/framework.rs index 6ae39835..d0bc36d7 100644 --- a/servers/tests/framework.rs +++ b/servers/tests/framework.rs @@ -378,7 +378,6 @@ impl LocalServerContainer { let client_n = HTTPNodeClient::new(&config.check_node_api_http_addr, None); let client_w = HTTPWalletCommAdapter::new(); - let max_outputs = 500; let change_outputs = 1; let mut wallet = LMDBBackend::new(config.clone(), "", client_n) @@ -389,7 +388,6 @@ impl LocalServerContainer { None, amount, minimum_confirmations, - max_outputs, change_outputs, selection_strategy == "all", None, diff --git a/servers/tests/simulnet.rs b/servers/tests/simulnet.rs index 214549c3..424ab0f0 100644 --- a/servers/tests/simulnet.rs +++ b/servers/tests/simulnet.rs @@ -963,7 +963,6 @@ fn replicate_tx_fluff_failure() { let (mut slate, lock_fn) = api.initiate_tx( None, amount, // amount 2, // minimum confirmations - 500, // max outputs 1000, // num change outputs true, // select all outputs None, diff --git a/src/bin/cmd/wallet_args.rs b/src/bin/cmd/wallet_args.rs index 4ae2f19d..c6fe2b49 100644 --- a/src/bin/cmd/wallet_args.rs +++ b/src/bin/cmd/wallet_args.rs @@ -389,9 +389,6 @@ pub fn parse_send_args(args: &ArgMatches) -> Result Result, amount: u64, minimum_confirmations: u64, - max_outputs: usize, num_change_outputs: usize, selection_strategy_is_use_all: bool, message: Option, @@ -651,7 +644,6 @@ where &mut *w, &mut slate, minimum_confirmations, - max_outputs, num_change_outputs, selection_strategy_is_use_all, &parent_key_id, @@ -681,15 +673,10 @@ where /// * `amount` - The amount to send, in nanogrins. (`1 G = 1_000_000_000nG`) /// * `minimum_confirmations` - The minimum number of confirmations an output /// should have in order to be included in the transaction. - /// * `max_outputs` - By default, the wallet selects as many inputs as possible in a - /// transaction, to reduce the Output set and the fees. The wallet will attempt to spend - /// include up to `max_outputs` in a transaction, however if this is not enough to cover - /// the whole amount, the wallet will include more outputs. This parameter should be considered - /// a soft limit. /// * `num_change_outputs` - The target number of change outputs to create in the transaction. /// The actual number created will be `num_change_outputs` + whatever remainder is needed. /// * `selection_strategy_is_use_all` - If `true`, attempt to use up as many outputs as - /// possible to create the transaction, up the 'soft limit' of `max_outputs`. This helps + /// possible to estimate the transaction. This helps /// to reduce the size of the UTXO set and the amount of data stored in the wallet, and /// minimizes fees. This will generally result in many inputs and a large change output(s), /// usually much larger than the amount being sent. If `false`, the transaction will include @@ -706,7 +693,6 @@ where src_acct_name: Option<&str>, amount: u64, minimum_confirmations: u64, - max_outputs: usize, num_change_outputs: usize, selection_strategy_is_use_all: bool, ) -> Result< @@ -732,7 +718,6 @@ where &mut *w, amount, minimum_confirmations, - max_outputs, num_change_outputs, selection_strategy_is_use_all, &parent_key_id, diff --git a/wallet/src/libwallet/internal/selection.rs b/wallet/src/libwallet/internal/selection.rs index b02cd896..ab76c944 100644 --- a/wallet/src/libwallet/internal/selection.rs +++ b/wallet/src/libwallet/internal/selection.rs @@ -16,11 +16,13 @@ use crate::core::core::{amount_to_hr_string, Transaction}; use crate::core::libtx::{build, tx_fee}; +use crate::core::{consensus, global}; use crate::keychain::{Identifier, Keychain}; use crate::libwallet::error::{Error, ErrorKind}; use crate::libwallet::internal::keys; use crate::libwallet::slate::Slate; use crate::libwallet::types::*; +use std::cmp::min; use std::collections::HashMap; use std::marker::PhantomData; @@ -33,7 +35,6 @@ pub fn build_send_tx( wallet: &mut T, slate: &mut Slate, minimum_confirmations: u64, - max_outputs: usize, change_outputs: usize, selection_strategy_is_use_all: bool, parent_key_id: Identifier, @@ -49,7 +50,6 @@ where slate.height, minimum_confirmations, slate.lock_height, - max_outputs, change_outputs, selection_strategy_is_use_all, &parent_key_id, @@ -220,6 +220,16 @@ where Ok((key_id, context, Box::new(wallet_add_fn))) } +/// Calculate maximal amount of inputs in transaction given amount of outputs +fn calculate_max_inputs_in_block(num_outputs: usize) -> usize { + let coinbase_weight = consensus::BLOCK_OUTPUT_WEIGHT + consensus::BLOCK_KERNEL_WEIGHT; + global::max_block_weight().saturating_sub( + coinbase_weight + + consensus::BLOCK_OUTPUT_WEIGHT.saturating_mul(num_outputs) + + consensus::BLOCK_KERNEL_WEIGHT, + ) / consensus::BLOCK_INPUT_WEIGHT +} + /// Builds a transaction to send to someone from the HD seed associated with the /// wallet and the amount to send. Handles reading through the wallet data file, /// selecting outputs to spend and building the change. @@ -229,7 +239,6 @@ pub fn select_send_tx( current_height: u64, minimum_confirmations: u64, lock_height: u64, - max_outputs: usize, change_outputs: usize, selection_strategy_is_use_all: bool, parent_key_id: &Identifier, @@ -252,7 +261,6 @@ where amount, current_height, minimum_confirmations, - max_outputs, change_outputs, selection_strategy_is_use_all, &parent_key_id, @@ -275,7 +283,6 @@ pub fn select_coins_and_fee( amount: u64, current_height: u64, minimum_confirmations: u64, - max_outputs: usize, change_outputs: usize, selection_strategy_is_use_all: bool, parent_key_id: &Identifier, @@ -299,7 +306,7 @@ where amount, current_height, minimum_confirmations, - max_outputs, + calculate_max_inputs_in_block(change_outputs), selection_strategy_is_use_all, parent_key_id, ); @@ -361,7 +368,7 @@ where amount_with_fee, current_height, minimum_confirmations, - max_outputs, + calculate_max_inputs_in_block(num_outputs), selection_strategy_is_use_all, parent_key_id, ) @@ -476,40 +483,20 @@ where }) .collect::>(); - let max_available = eligible.len(); + // max_available can not be bigger than max_outputs + let max_available = min(eligible.len(), max_outputs); // sort eligible outputs by increasing value eligible.sort_by_key(|out| out.value); // use a sliding window to identify potential sets of possible outputs to spend - // Case of amount > total amount of max_outputs(500): - // The limit exists because by default, we always select as many inputs as - // possible in a transaction, to reduce both the Output set and the fees. - // But that only makes sense up to a point, hence the limit to avoid being too - // greedy. But if max_outputs(500) is actually not enough to cover the whole - // amount, the wallet should allow going over it to satisfy what the user - // wants to send. So the wallet considers max_outputs more of a soft limit. - if eligible.len() > max_outputs { - for window in eligible.windows(max_outputs) { + if max_available > 0 { + for window in eligible.windows(max_available) { let windowed_eligibles = window.iter().cloned().collect::>(); if let Some(outputs) = select_from(amount, select_all, windowed_eligibles) { return (max_available, outputs); } } - // Not exist in any window of which total amount >= amount. - // Then take coins from the smallest one up to the total amount of selected - // coins = the amount. - if let Some(outputs) = select_from(amount, false, eligible.clone()) { - debug!( - "Extending maximum number of outputs. {} outputs selected.", - outputs.len() - ); - return (max_available, outputs); - } - } else { - if let Some(outputs) = select_from(amount, select_all, eligible.clone()) { - return (max_available, outputs); - } } // we failed to find a suitable set of outputs to spend, @@ -518,7 +505,7 @@ where eligible.reverse(); ( max_available, - eligible.iter().take(max_outputs).cloned().collect(), + eligible.iter().take(max_available).cloned().collect(), ) } diff --git a/wallet/src/libwallet/internal/tx.rs b/wallet/src/libwallet/internal/tx.rs index 683a369f..6d720108 100644 --- a/wallet/src/libwallet/internal/tx.rs +++ b/wallet/src/libwallet/internal/tx.rs @@ -47,7 +47,6 @@ pub fn estimate_send_tx( wallet: &mut T, amount: u64, minimum_confirmations: u64, - max_outputs: usize, num_change_outputs: usize, selection_strategy_is_use_all: bool, parent_key_id: &Identifier, @@ -80,7 +79,6 @@ where amount, current_height, minimum_confirmations, - max_outputs, num_change_outputs, selection_strategy_is_use_all, parent_key_id, @@ -93,7 +91,6 @@ pub fn add_inputs_to_slate( wallet: &mut T, slate: &mut Slate, minimum_confirmations: u64, - max_outputs: usize, num_change_outputs: usize, selection_strategy_is_use_all: bool, parent_key_id: &Identifier, @@ -119,7 +116,6 @@ where wallet, slate, minimum_confirmations, - max_outputs, num_change_outputs, selection_strategy_is_use_all, parent_key_id.clone(), diff --git a/wallet/src/libwallet/types.rs b/wallet/src/libwallet/types.rs index 81ef38f9..cef7298e 100644 --- a/wallet/src/libwallet/types.rs +++ b/wallet/src/libwallet/types.rs @@ -710,8 +710,6 @@ pub struct SendTXArgs { pub method: String, /// destination url pub dest: String, - /// Max number of outputs - pub max_outputs: usize, /// Number of change outputs to generate pub num_change_outputs: usize, /// whether to use all outputs (combine) diff --git a/wallet/src/test_framework/mod.rs b/wallet/src/test_framework/mod.rs index 029dff57..d4ef26f8 100644 --- a/wallet/src/test_framework/mod.rs +++ b/wallet/src/test_framework/mod.rs @@ -198,7 +198,6 @@ where None, // account amount, // amount 2, // minimum confirmations - 500, // max outputs 1, // num change outputs true, // select all outputs None, diff --git a/wallet/tests/accounts.rs b/wallet/tests/accounts.rs index 98340c0d..1967f23a 100644 --- a/wallet/tests/accounts.rs +++ b/wallet/tests/accounts.rs @@ -180,7 +180,6 @@ fn accounts_test_impl(test_dir: &str) -> Result<(), libwallet::Error> { let (mut slate, lock_fn) = api.initiate_tx( None, reward, // amount 2, // minimum confirmations - 500, // max outputs 1, // num change outputs true, // select all outputs None, diff --git a/wallet/tests/check.rs b/wallet/tests/check.rs index 8e46d70c..d0747047 100644 --- a/wallet/tests/check.rs +++ b/wallet/tests/check.rs @@ -158,7 +158,6 @@ fn check_repair_impl(test_dir: &str) -> Result<(), libwallet::Error> { None, reward * 2, // amount cm, // minimum confirmations - 500, // max outputs 1, // num change outputs true, // select all outputs None, // optional message diff --git a/wallet/tests/file.rs b/wallet/tests/file.rs index 925f7cca..5c51f38a 100644 --- a/wallet/tests/file.rs +++ b/wallet/tests/file.rs @@ -105,7 +105,6 @@ fn file_exchange_test_impl(test_dir: &str) -> Result<(), libwallet::Error> { Some("mining"), reward * 2, // amount 2, // minimum confirmations - 500, // max outputs 1, // num change outputs true, // select all outputs Some(message.to_owned()), // optional message diff --git a/wallet/tests/repost.rs b/wallet/tests/repost.rs index e24cd6f6..8de39495 100644 --- a/wallet/tests/repost.rs +++ b/wallet/tests/repost.rs @@ -104,7 +104,6 @@ fn file_repost_test_impl(test_dir: &str) -> Result<(), libwallet::Error> { Some("mining"), reward * 2, // amount 2, // minimum confirmations - 500, // max outputs 1, // num change outputs true, // select all outputs None, @@ -200,7 +199,6 @@ fn file_repost_test_impl(test_dir: &str) -> Result<(), libwallet::Error> { None, amount * 2, // amount 2, // minimum confirmations - 500, // max outputs 1, // num change outputs true, // select all outputs None, diff --git a/wallet/tests/restore.rs b/wallet/tests/restore.rs index 9f15a72b..7ff64f07 100644 --- a/wallet/tests/restore.rs +++ b/wallet/tests/restore.rs @@ -237,7 +237,6 @@ fn setup_restore(test_dir: &str) -> Result<(), libwallet::Error> { let (slate_i, lock_fn) = sender_api.initiate_tx( None, amount, // amount 2, // minimum confirmations - 500, // max outputs 1, // num change outputs true, // select all outputs None, @@ -259,7 +258,6 @@ fn setup_restore(test_dir: &str) -> Result<(), libwallet::Error> { None, amount * 2, // amount 2, // minimum confirmations - 500, // max outputs 1, // num change outputs true, // select all outputs None, @@ -281,7 +279,6 @@ fn setup_restore(test_dir: &str) -> Result<(), libwallet::Error> { None, amount * 3, // amount 2, // minimum confirmations - 500, // max outputs 1, // num change outputs true, // select all outputs None, @@ -309,7 +306,6 @@ fn setup_restore(test_dir: &str) -> Result<(), libwallet::Error> { None, amount * 3, // amount 2, // minimum confirmations - 500, // max outputs 1, // num change outputs true, // select all outputs None, diff --git a/wallet/tests/self_send.rs b/wallet/tests/self_send.rs index 54cab307..88e98eee 100644 --- a/wallet/tests/self_send.rs +++ b/wallet/tests/self_send.rs @@ -88,7 +88,6 @@ fn self_send_test_impl(test_dir: &str) -> Result<(), libwallet::Error> { Some("mining"), reward * 2, // amount 2, // minimum confirmations - 500, // max outputs 1, // num change outputs true, // select all outputs None, diff --git a/wallet/tests/transaction.rs b/wallet/tests/transaction.rs index ac22461f..5031f6cd 100644 --- a/wallet/tests/transaction.rs +++ b/wallet/tests/transaction.rs @@ -99,7 +99,6 @@ fn basic_transaction_api(test_dir: &str) -> Result<(), libwallet::Error> { let (slate_i, lock_fn) = sender_api.initiate_tx( None, amount, // amount 2, // minimum confirmations - 500, // max outputs 1, // num change outputs true, // select all outputs None, @@ -232,7 +231,6 @@ fn basic_transaction_api(test_dir: &str) -> Result<(), libwallet::Error> { None, amount * 2, // amount 2, // minimum confirmations - 500, // max outputs 1, // num change outputs true, // select all outputs )?; @@ -243,7 +241,6 @@ fn basic_transaction_api(test_dir: &str) -> Result<(), libwallet::Error> { None, amount * 2, // amount 2, // minimum confirmations - 500, // max outputs 1, // num change outputs false, // select the smallest amount of outputs )?; @@ -261,7 +258,6 @@ fn basic_transaction_api(test_dir: &str) -> Result<(), libwallet::Error> { None, amount * 2, // amount 2, // minimum confirmations - 500, // max outputs 1, // num change outputs true, // select all outputs None, @@ -312,6 +308,24 @@ fn basic_transaction_api(test_dir: &str) -> Result<(), libwallet::Error> { Ok(()) })?; + // Initiate transaction with weight more that + // core::global::max_block_weight() should fail + let invalid_amount_of_outputs = + core::global::max_block_weight() / core::consensus::BLOCK_OUTPUT_WEIGHT + 1; + let res = wallet::controller::owner_single_use(wallet1.clone(), |sender_api| { + // note this will increment the block count as part of the transaction "posting" + sender_api.initiate_tx( + None, + amount, // amount + 2, // minimum confirmations + invalid_amount_of_outputs, // num change outputs + true, // select all outputs + None, + )?; + Ok(()) + }); + assert!(res.is_err()); + // let logging finish thread::sleep(Duration::from_millis(200)); Ok(()) @@ -358,7 +372,6 @@ fn tx_rollback(test_dir: &str) -> Result<(), libwallet::Error> { let (slate_i, lock_fn) = sender_api.initiate_tx( None, amount, // amount 2, // minimum confirmations - 500, // max outputs 1, // num change outputs true, // select all outputs None,