diff --git a/servers/tests/framework/mod.rs b/servers/tests/framework/mod.rs index 46ad5a4c7..77745f160 100644 --- a/servers/tests/framework/mod.rs +++ b/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 { diff --git a/src/bin/cmd/wallet.rs b/src/bin/cmd/wallet.rs index e4b3aae74..6e6a4bba6 100644 --- a/src/bin/cmd/wallet.rs +++ b/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(()) diff --git a/src/bin/grin.rs b/src/bin/grin.rs index 61d326ffd..7539451a7 100644 --- a/src/bin/grin.rs +++ b/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") diff --git a/wallet/src/libtx/build.rs b/wallet/src/libtx/build.rs index 1214693ff..d66c62297 100644 --- a/wallet/src/libtx/build.rs +++ b/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(); diff --git a/wallet/src/libtx/slate.rs b/wallet/src/libtx/slate.rs index 12e5bff88..d988ee867 100644 --- a/wallet/src/libtx/slate.rs +++ b/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(), diff --git a/wallet/src/libwallet/api.rs b/wallet/src/libwallet/api.rs index e1d76e8ea..f0874c2a6 100644 --- a/wallet/src/libwallet/api.rs +++ b/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 { 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, )?; diff --git a/wallet/src/libwallet/controller.rs b/wallet/src/libwallet/controller.rs index e229ea918..df7203e18 100644 --- a/wallet/src/libwallet/controller.rs +++ b/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, ) })) diff --git a/wallet/src/libwallet/internal/selection.rs b/wallet/src/libwallet/internal/selection.rs index 4633f0ab4..8d26e7041 100644 --- a/wallet/src/libwallet/internal/selection.rs +++ b/wallet/src/libwallet/internal/selection.rs @@ -35,6 +35,7 @@ pub fn build_send_tx_slate( 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 - if change_derivation.is_some() { - let change_id = keychain.derive_key_id(change_derivation.unwrap()).unwrap(); + // Store change output(s) + for (_, derivation) in &change_amounts_derivations { + 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 { - let change_id = keychain.derive_key_id(change_derivation.unwrap()).unwrap(); - t.amount_credited = change as u64; - t.num_outputs = 1; + for (change_amount, change_derivation) in &change_amounts_derivations { + let change_id = keychain.derive_key_id(change_derivation.clone()).unwrap(); + 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, - value: change as u64, + n_child: change_derivation.clone(), + value: change_amount.clone(), status: OutputStatus::Unconfirmed, height: current_height, lock_height: 0, @@ -215,15 +219,15 @@ pub fn select_send_tx( minimum_confirmations: u64, lock_height: u64, max_outputs: usize, + change_outputs: usize, selection_strategy_is_use_all: bool, ) -> Result< ( Vec>>, Vec, - u64, //change - Option, //change derivation - u64, // amount - u64, // fee + Vec<(u64, u32)>, // change amounts and derivations + u64, // amount + 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( wallet: &mut T, amount: u64, fee: u64, -) -> Result<(Vec>>, u64, Option), Error> + num_change_outputs: usize, +) -> Result<(Vec>>, Vec<(u64, u32)>), Error> where T: WalletBackend, 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. diff --git a/wallet/src/libwallet/internal/tx.rs b/wallet/src/libwallet/internal/tx.rs index 42a77bb6b..0c9500377 100644 --- a/wallet/src/libwallet/internal/tx.rs +++ b/wallet/src/libwallet/internal/tx.rs @@ -59,6 +59,7 @@ pub fn create_send_tx( 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 diff --git a/wallet/src/libwallet/types.rs b/wallet/src/libwallet/types.rs index 12982caf4..2db62d45e 100644 --- a/wallet/src/libwallet/types.rs +++ b/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 diff --git a/wallet/tests/transaction.rs b/wallet/tests/transaction.rs index aa64b94e4..27c930bc3 100644 --- a/wallet/tests/transaction.rs +++ b/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(())