fix mimblewimble#2510: wallet coin selection respects max_block_weight

Deprecate "soft" max_outputs limit and introduce "hard" max_outputs
limit based on max_block_weight.

src/bin/cmd/wallet_args.rs

@@ -378,9 +378,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,
@@ -390,7 +387,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

@@ -204,7 +204,6 @@ pub struct SendArgs {
 	pub dest: String,
 	pub change_outputs: usize,
 	pub fluff: bool,
-	pub max_outputs: usize,
 }
 
 pub fn send(
@@ -216,7 +215,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

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

@@ -619,7 +619,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 +650,6 @@ where
 			&mut *w,
 			&mut slate,
 			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,
@@ -253,7 +262,7 @@ where
 		amount,
 		current_height,
 		minimum_confirmations,
-		max_outputs,
+		calculate_max_inputs_in_block(change_outputs),
 		selection_strategy_is_use_all,
 		parent_key_id,
 	);
@@ -315,7 +324,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,
 			)
@@ -439,40 +448,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,
@@ -481,7 +470,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 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,
@@ -73,7 +72,6 @@ where
 		wallet,
 		slate,
 		minimum_confirmations,
-		max_outputs,
 		num_change_outputs,
 		selection_strategy_is_use_all,
 		parent_key_id.clone(),

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

@@ -226,7 +226,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,
@@ -248,7 +247,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,
@@ -270,7 +268,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,
@@ -298,7 +295,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,
@@ -234,7 +233,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,
@@ -285,6 +283,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(())
@@ -331,7 +347,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,