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

@@ -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,

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,

src/bin/cmd/wallet_args.rs

@@ -389,9 +389,6 @@ pub fn parse_send_args(args: &ArgMatches) -> Result<command::SendArgs, ParseErro
 	// fluff
 	let fluff = args.is_present("fluff");
 
-	// max_outputs
-	let max_outputs = 500;
-
 	Ok(command::SendArgs {
 		amount: amount,
 		message: message,
@@ -402,7 +399,6 @@ pub fn parse_send_args(args: &ArgMatches) -> Result<command::SendArgs, ParseErro
 		dest: dest.to_owned(),
 		change_outputs: change_outputs,
 		fluff: fluff,
-		max_outputs: max_outputs,
 	})
 }
 

wallet/src/command.rs

@@ -205,7 +205,6 @@ pub struct SendArgs {
 	pub dest: String,
 	pub change_outputs: usize,
 	pub fluff: bool,
-	pub max_outputs: usize,
 }
 
 pub fn send(
@@ -223,7 +222,6 @@ pub fn send(
 							None,
 							args.amount,
 							args.minimum_confirmations,
-							args.max_outputs,
 							args.change_outputs,
 							strategy == "all",
 						)
@@ -237,7 +235,6 @@ pub fn send(
 				None,
 				args.amount,
 				args.minimum_confirmations,
-				args.max_outputs,
 				args.change_outputs,
 				args.selection_strategy == "all",
 				args.message.clone(),

wallet/src/controller.rs

@@ -325,7 +325,6 @@ where
 				None,
 				args.amount,
 				args.minimum_confirmations,
-				args.max_outputs,
 				args.num_change_outputs,
 				args.selection_strategy_is_use_all,
 				args.message,

wallet/src/libwallet/api.rs

@@ -535,15 +535,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 create 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
@@ -600,7 +595,6 @@ where
 	///		None,
 	///		amount,     // amount
 	///		10,         // minimum confirmations
-	///		500,        // max outputs
 	///		1,          // num change outputs
 	///		true,       // select all outputs
 	///		Some("Have some Grins. Love, Yeastplume".to_owned()),
@@ -619,7 +613,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,
 		message: Option<String>,
@@ -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,

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<T: ?Sized, C, K>(
 	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<T: ?Sized, C, K>(
 	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<T: ?Sized, C, K>(
 	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::<Vec<OutputData>>();
 
-	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::<Vec<_>>();
 			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(),
 	)
 }
 

wallet/src/libwallet/internal/tx.rs

@@ -47,7 +47,6 @@ pub fn estimate_send_tx<T: ?Sized, C, K>(
 	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<T: ?Sized, C, K>(
 	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(),

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)

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,

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,

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

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

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,

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,

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,

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,