Tx (change) output splitter (#1369)

* wip - split change into many outputs for testing

* rustfmt

* add change_outputs param to wallet send
default to 1

* commit

* rustfmt

* cleanup

* fixup servers tests

servers/tests/framework/mod.rs

@@ -332,7 +332,9 @@ impl LocalServerContainer {
 			.expect("Failed to derive keychain from seed file and passphrase.");
 
 		let client = HTTPWalletClient::new(&config.check_node_api_http_addr);
+
 		let max_outputs = 500;
+		let change_outputs = 1;
 
 		let mut wallet = FileWallet::new(config.clone(), "", client)
 			.unwrap_or_else(|e| panic!("Error creating wallet: {:?} Config: {:?}", e, config));
@@ -344,6 +346,7 @@ impl LocalServerContainer {
 					minimum_confirmations,
 					dest,
 					max_outputs,
+					change_outputs,
 					selection_strategy == "all",
 				);
 				match result {

src/bin/cmd/wallet.rs

@@ -170,6 +170,11 @@ pub fn wallet_command(wallet_args: &ArgMatches, global_config: GlobalConfig) {
 				let dest = send_args
 					.value_of("dest")
 					.expect("Destination wallet address required");
+				let change_outputs = send_args
+					.value_of("change_outputs")
+					.unwrap()
+					.parse()
+					.expect("Failed to parse number of change outputs.");
 				let fluff = send_args.is_present("fluff");
 				let max_outputs = 500;
 				if dest.starts_with("http") {
@@ -178,6 +183,7 @@ pub fn wallet_command(wallet_args: &ArgMatches, global_config: GlobalConfig) {
 						minimum_confirmations,
 						dest,
 						max_outputs,
+						change_outputs,
 						selection_strategy == "all",
 					);
 					let slate = match result {
@@ -223,6 +229,7 @@ pub fn wallet_command(wallet_args: &ArgMatches, global_config: GlobalConfig) {
 						minimum_confirmations,
 						dest,
 						max_outputs,
+						change_outputs,
 						selection_strategy == "all",
 					).expect("Send failed");
 					Ok(())

src/bin/grin.rs

@@ -201,6 +201,12 @@ fn main() {
 				.possible_values(&["all", "smallest"])
 				.default_value("all")
 				.takes_value(true))
+			.arg(Arg::with_name("change_outputs")
+				.help("Number of change outputs to generate (mainly for testing).")
+				.short("o")
+				.long("change_outputs")
+				.default_value("1")
+				.takes_value(true))
 			.arg(Arg::with_name("dest")
 				.help("Send the transaction to the provided server")
 				.short("d")

wallet/src/libtx/build.rs

@@ -93,10 +93,9 @@ where
 {
 	Box::new(
 		move |build, (tx, kern, sum)| -> (Transaction, TxKernel, BlindSum) {
-			debug!(LOGGER, "Building an output: {}, {}", value, key_id,);
-
 			let commit = build.keychain.commit(value, &key_id).unwrap();
-			trace!(LOGGER, "Builder - Pedersen Commit is: {:?}", commit,);
+
+			debug!(LOGGER, "Building an output: {}, {:?}", value, commit);
 
 			let rproof = proof::create(build.keychain, value, &key_id, commit, None).unwrap();
 

wallet/src/libtx/slate.rs

@@ -153,6 +153,7 @@ impl Slate {
 		K: Keychain,
 	{
 		self.check_fees()?;
+
 		self.verify_part_sigs(keychain.secp())?;
 		let sig_part = aggsig::calculate_partial_sig(
 			keychain.secp(),

wallet/src/libwallet/api.rs

@@ -139,6 +139,7 @@ where
 		minimum_confirmations: u64,
 		dest: &str,
 		max_outputs: usize,
+		num_change_outputs: usize,
 		selection_strategy_is_use_all: bool,
 	) -> Result<Slate, Error> {
 		let mut w = self.wallet.lock().unwrap();
@@ -154,6 +155,7 @@ where
 			amount,
 			minimum_confirmations,
 			max_outputs,
+			num_change_outputs,
 			selection_strategy_is_use_all,
 		)?;
 
@@ -186,6 +188,7 @@ where
 		minimum_confirmations: u64,
 		dest: &str,
 		max_outputs: usize,
+		num_change_outputs: usize,
 		selection_strategy_is_use_all: bool,
 	) -> Result<(), Error> {
 		let mut w = self.wallet.lock().unwrap();
@@ -196,6 +199,7 @@ where
 			amount,
 			minimum_confirmations,
 			max_outputs,
+			num_change_outputs,
 			selection_strategy_is_use_all,
 		)?;
 

wallet/src/libwallet/controller.rs

@@ -275,6 +275,7 @@ where
 				args.minimum_confirmations,
 				&args.dest,
 				args.max_outputs,
+				args.num_change_outputs,
 				args.selection_strategy_is_use_all,
 			)
 		}))

wallet/src/libwallet/internal/selection.rs

@@ -35,6 +35,7 @@ pub fn build_send_tx_slate<T: ?Sized, C, K>(
 	minimum_confirmations: u64,
 	lock_height: u64,
 	max_outputs: usize,
+	change_outputs: usize,
 	selection_strategy_is_use_all: bool,
 ) -> Result<
 	(
@@ -49,13 +50,14 @@ where
 	C: WalletClient,
 	K: Keychain,
 {
-	let (elems, inputs, change, change_derivation, amount, fee) = select_send_tx(
+	let (elems, inputs, change_amounts_derivations, amount, fee) = select_send_tx(
 		wallet,
 		amount,
 		current_height,
 		minimum_confirmations,
 		lock_height,
 		max_outputs,
+		change_outputs,
 		selection_strategy_is_use_all,
 	)?;
 
@@ -70,6 +72,7 @@ where
 	let keychain = wallet.keychain().clone();
 
 	let blinding = slate.add_transaction_elements(&keychain, elems)?;
+
 	// Create our own private context
 	let mut context = sigcontext::Context::new(
 		wallet.keychain().secp(),
@@ -81,9 +84,9 @@ where
 		context.add_input(&input.key_id);
 	}
 
-	// Store change output
+	// Store change output(s)
-	if change_derivation.is_some() {
+	for (_, derivation) in &change_amounts_derivations {
-		let change_id = keychain.derive_key_id(change_derivation.unwrap()).unwrap();
+		let change_id = keychain.derive_key_id(derivation.clone()).unwrap();
 		context.add_output(&change_id);
 	}
 
@@ -108,18 +111,19 @@ where
 			amount_debited = amount_debited + coin.value;
 			batch.lock_output(&mut coin)?;
 		}
+
 		t.amount_debited = amount_debited;
 
 		// write the output representing our change
-		if let Some(d) = change_derivation {
+		for (change_amount, change_derivation) in &change_amounts_derivations {
-			let change_id = keychain.derive_key_id(change_derivation.unwrap()).unwrap();
+			let change_id = keychain.derive_key_id(change_derivation.clone()).unwrap();
-			t.amount_credited = change as u64;
+			t.num_outputs += 1;
-			t.num_outputs = 1;
+			t.amount_credited += change_amount;
 			batch.save(OutputData {
-				root_key_id: root_key_id,
+				root_key_id: root_key_id.clone(),
 				key_id: change_id.clone(),
-				n_child: d,
+				n_child: change_derivation.clone(),
-				value: change as u64,
+				value: change_amount.clone(),
 				status: OutputStatus::Unconfirmed,
 				height: current_height,
 				lock_height: 0,
@@ -215,15 +219,15 @@ pub fn select_send_tx<T: ?Sized, C, K>(
 	minimum_confirmations: u64,
 	lock_height: u64,
 	max_outputs: usize,
+	change_outputs: usize,
 	selection_strategy_is_use_all: bool,
 ) -> Result<
 	(
 		Vec<Box<build::Append<K>>>,
 		Vec<OutputData>,
-		u64,         //change
+		Vec<(u64, u32)>, // change amounts and derivations
-		Option<u32>, //change derivation
+		u64,             // amount
-		u64,         // amount
+		u64,             // fee
-		u64,         // fee
 	),
 	Error,
 >
@@ -233,7 +237,7 @@ where
 	K: Keychain,
 {
 	// select some spendable coins from the wallet
-	let (max_outputs, mut coins) = select_coins(
+	let (max_outputs, coins) = select_coins(
 		wallet,
 		amount,
 		current_height,
@@ -245,9 +249,12 @@ where
 	// sender is responsible for setting the fee on the partial tx
 	// recipient should double check the fee calculation and not blindly trust the
 	// sender
-	let mut fee;
+
+	// TODO - Is it safe to spend without a change output? (1 input -> 1 output)
+	// TODO - Does this not potentially reveal the senders private key?
+	//
 	// First attempt to spend without change
-	fee = tx_fee(coins.len(), 1, None);
+	let mut fee = tx_fee(coins.len(), 1, None);
 	let mut total: u64 = coins.iter().map(|c| c.value).sum();
 	let mut amount_with_fee = amount + fee;
 
@@ -266,9 +273,11 @@ where
 		})?;
 	}
 
+	let num_outputs = change_outputs + 1;
+
 	// We need to add a change address or amount with fee is more than total
 	if total != amount_with_fee {
-		fee = tx_fee(coins.len(), 2, None);
+		fee = tx_fee(coins.len(), num_outputs, None);
 		amount_with_fee = amount + fee;
 
 		// Here check if we have enough outputs for the amount including fee otherwise
@@ -283,28 +292,29 @@ where
 			}
 
 			// select some spendable coins from the wallet
-			coins = select_coins(
+			let (_, coins) = select_coins(
 				wallet,
 				amount_with_fee,
 				current_height,
 				minimum_confirmations,
 				max_outputs,
 				selection_strategy_is_use_all,
-			).1;
+			);
-			fee = tx_fee(coins.len(), 2, None);
+			fee = tx_fee(coins.len(), num_outputs, None);
 			total = coins.iter().map(|c| c.value).sum();
 			amount_with_fee = amount + fee;
 		}
 	}
 
 	// build transaction skeleton with inputs and change
-	let (mut parts, change, change_derivation) = inputs_and_change(&coins, wallet, amount, fee)?;
+	let (mut parts, change_amounts_derivations) =
+		inputs_and_change(&coins, wallet, amount, fee, change_outputs)?;
 
 	// This is more proof of concept than anything but here we set lock_height
 	// on tx being sent (based on current chain height via api).
 	parts.push(build::with_lock_height(lock_height));
 
-	Ok((parts, coins, change, change_derivation, amount, fee))
+	Ok((parts, coins, change_amounts_derivations, amount, fee))
 }
 
 /// Selects inputs and change for a transaction
@@ -313,7 +323,8 @@ pub fn inputs_and_change<T: ?Sized, C, K>(
 	wallet: &mut T,
 	amount: u64,
 	fee: u64,
-) -> Result<(Vec<Box<build::Append<K>>>, u64, Option<u32>), Error>
+	num_change_outputs: usize,
+) -> Result<(Vec<Box<build::Append<K>>>, Vec<(u64, u32)>), Error>
 where
 	T: WalletBackend<C, K>,
 	C: WalletClient,
@@ -340,17 +351,42 @@ where
 			parts.push(build::input(coin.value, key_id));
 		}
 	}
-	let mut change_derivation = None;
-	if change != 0 {
-		let keychain = wallet.keychain().clone();
-		let root_key_id = keychain.root_key_id();
-		change_derivation = Some(wallet.next_child(root_key_id.clone()).unwrap());
-		let change_k = keychain.derive_key_id(change_derivation.unwrap()).unwrap();
 
-		parts.push(build::output(change, change_k.clone()));
+	let mut change_amounts_derivations = vec![];
+
+	if change == 0 {
+		debug!(
+			LOGGER,
+			"No change (sending exactly amount + fee), no change outputs to build"
+		);
+	} else {
+		debug!(
+			LOGGER,
+			"Building change outputs: total change: {} ({} outputs)", change, num_change_outputs
+		);
+
+		let part_change = change / num_change_outputs as u64;
+		let remainder_change = change % part_change;
+
+		for x in 0..num_change_outputs {
+			// n-1 equal change_outputs and a final one accounting for any remainder
+			let change_amount = if x == (num_change_outputs - 1) {
+				part_change + remainder_change
+			} else {
+				part_change
+			};
+
+			let keychain = wallet.keychain().clone();
+			let root_key_id = keychain.root_key_id();
+			let change_derivation = wallet.next_child(root_key_id.clone()).unwrap();
+			let change_key = keychain.derive_key_id(change_derivation).unwrap();
+
+			change_amounts_derivations.push((change_amount, change_derivation));
+			parts.push(build::output(change_amount, change_key));
+		}
 	}
 
-	Ok((parts, change, change_derivation))
+	Ok((parts, change_amounts_derivations))
 }
 
 /// Select spendable coins from a wallet.

wallet/src/libwallet/internal/tx.rs

@@ -59,6 +59,7 @@ pub fn create_send_tx<T: ?Sized, C, K>(
 	amount: u64,
 	minimum_confirmations: u64,
 	max_outputs: usize,
+	num_change_outputs: usize,
 	selection_strategy_is_use_all: bool,
 ) -> Result<
 	(
@@ -95,6 +96,7 @@ where
 		minimum_confirmations,
 		lock_height,
 		max_outputs,
+		num_change_outputs,
 		selection_strategy_is_use_all,
 	)?;
 
@@ -190,7 +192,9 @@ where
 	debug!(LOGGER, "selected some coins - {}", coins.len());
 
 	let fee = tx_fee(coins.len(), 2, None);
-	let (mut parts, _, _) = selection::inputs_and_change(&coins, wallet, amount, fee)?;
+	let num_change_outputs = 1;
+	let (mut parts, _) =
+		selection::inputs_and_change(&coins, wallet, amount, fee, num_change_outputs)?;
 
 	//TODO: If we end up using this, create change output here
 

wallet/src/libwallet/types.rs

@@ -553,6 +553,8 @@ pub struct SendTXArgs {
 	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)
 	pub selection_strategy_is_use_all: bool,
 	/// dandelion control

wallet/tests/transaction.rs

@@ -123,6 +123,7 @@ fn basic_transaction_api(
 			2,         // minimum confirmations
 			"wallet2", // dest
 			500,       // max outputs
+			1,         // num change outputs
 			true,      // select all outputs
 		)?;
 		Ok(())
@@ -298,6 +299,7 @@ fn tx_rollback(test_dir: &str, backend_type: common::BackendType) -> Result<(),
 			2,         // minimum confirmations
 			"wallet2", // dest
 			500,       // max outputs
+			1,         // num change outputs
 			true,      // select all outputs
 		)?;
 		Ok(())