Merge pull request #36 from j-berman/tie-outs-to-mix-outs

Re-use same outs & chosen mix outs across tx construction attempts

README.md

@@ -294,7 +294,7 @@ Useful for displaying an estimated fee – To obtain exact fees, see "Creating a
 
 As mentioned, implementing the Send procedure without making use of one of our existing libraries or examples involves two bridge calls surrounded by server API calls, and mandatory reconstruction logic, and is simplified by various opportunities to pass values directly between the steps.
 
-The values which must be passed between functions have (almost entirely) consistent names, simplifying integration. The only current exception is the name of the explicit `fee_actually_needed` which should be passed to step1 as the optional `passedIn_attemptAt_fee` after being received by calling step2 (see below).
+The values which must be passed between functions have (almost entirely) consistent names, simplifying integration. The only current exceptions are the names of the explicit `fee_actually_needed` and `outs_to_mix_outs`, which should be passed to step1 as the optional `prior_attempt_size_calcd_fee` and `prior_attempt_unspent_outs_to_mix_outs` respectively after calling step2, when the transaction must be reconstructed (see below).
 
 ##### Examples
 * [JS implementation of SendFunds](https://github.com/mymonero/mymonero-core-js/blob/master/monero_utils/monero_sendingFunds_utils.js#L100)
@@ -323,7 +323,8 @@ The values which must be passed between functions have (almost entirely) consist
 	* `fork_version: UInt8String`
 	* `unspent_outs: [UnspentOutput]` - fully parsed server response
 	* `payment_id_string: Optional<String>`
-	* `passedIn_attemptAt_fee: Optional<UInt64String>`
+	* `prior_attempt_size_calcd_fee: Optional<UInt64String>`
+	* `prior_attempt_unspent_outs_to_mix_outs: Optional<Map<String, [RandomAmountOutput]>>` - map of output public keys to mix outs, explained below
 
 * Returns: 
 
@@ -345,6 +346,28 @@ The values which must be passed between functions have (almost entirely) consist
 	* `using_outs: [UnspentOutput]` passable directly to step2
 	* `final_total_wo_fee: UInt64String`
 
+##### `pre_step2_tie_unspent_outs_to_mix_outs_for_all_future_tx_attempts`
+
+`prior_attempt_unspent_outs_to_mix_outs` functions as a cache, tying used outputs to a constant set of mix outs across construction attempts. If the transaction construction steps need to be repated, you should re-use the same outs and their respective selected mix outs as used in prior attempts. This has 2 benefits: (1) it ensures the fee calculation is done correctly to prevent leaving transactions on chain that are fingerprintable by their fee, (2) no need to keep re-querying the server for decoys. This step should be called *after* receiving `mix_outs` from an API call, and before step2. The resulting `prior_attempt_unspent_outs_to_mix_outs_new` should become the new `prior_attempt_unspent_outs_to_mix_outs` in future tx construction attempts.
+
+* Args:
+	* `using_outs: [UnspentOutput]` returned by step1
+	* `mix_outs_from_server: [MixAmountAndOuts]` defined below
+	* `prior_attempt_unspent_outs_to_mix_outs: Optional<Map<String, [RandomAmountOutput]>>`
+
+* Returns:
+
+	* `err_code: CreateTransactionErrorCode`==`notEnoughUsableDecoysFound(22)`
+
+	*OR*
+
+	* `err_code: CreateTransactionErrorCode`==`tooManyDecoysRemaining(23)`
+
+	*OR*
+
+	* `mix_outs: [MixAmountAndOuts]` passable directly to step2
+	* `prior_attempt_unspent_outs_to_mix_outs_new: Map<String, [RandomAmountOutput]>`
+
 
 ##### `send_step2__try_create_transaction`
 
@@ -366,7 +389,7 @@ The values which must be passed between functions have (almost entirely) consist
 	* `nettype_string: NettypeString`
 	* `payment_id_string: Optional<String>`
 
-		* `MixAmountAndOuts: Dictionary` decoys obtained from API call with
+		* `MixAmountAndOuts: Dictionary` decoys obtained from API call with, or from `pre_step2_tie_unspent_outs_to_mix_outs_for_all_future_tx_attempts`
 			* `amount: UInt64String`
 			* `outputs: [MixOut]` where
 				* `MixOut: Dictionary` with 
@@ -377,7 +400,7 @@ The values which must be passed between functions have (almost entirely) consist
 	* Returns: 
 
 		* `tx_must_be_reconstructed: BoolString`==`true`
-		* `fee_actually_needed: UInt64String` pass this back to step1 as `passedIn_attemptAt_fee`
+		* `fee_actually_needed: UInt64String` pass this back to step1 as `prior_attempt_size_calcd_fee`
 
 		*OR*
 

src/monero_send_routine.cpp

@@ -86,10 +86,24 @@ LightwalletAPI_Req_GetUnspentOuts monero_send_routine::new__req_params__get_unsp
 	};
 }
 LightwalletAPI_Req_GetRandomOuts monero_send_routine::new__req_params__get_random_outs(
-	vector<SpendableOutput> &step1__using_outs
+	const vector<SpendableOutput> &step1__using_outs,
+	const optional<SpendableOutputToRandomAmountOutputs> &prior_attempt_unspent_outs_to_mix_outs
 ) {
+	// request decoys for any newly selected inputs
+	std::vector<SpendableOutput> decoy_requests;
+	if (prior_attempt_unspent_outs_to_mix_outs) {
+		for (size_t i = 0; i < step1__using_outs.size(); ++i) {
+			// only need to request decoys for outs that were not already passed in
+			if (prior_attempt_unspent_outs_to_mix_outs->find(step1__using_outs[i].public_key) == prior_attempt_unspent_outs_to_mix_outs->end()) {
+				decoy_requests.push_back(step1__using_outs[i]);
+			}
+		}
+	} else {
+		decoy_requests = step1__using_outs;
+	}
+
 	vector<string> decoy_req__amounts;
-	BOOST_FOREACH(SpendableOutput &using_out, step1__using_outs)
+	BOOST_FOREACH(SpendableOutput &using_out, decoy_requests)
 	{
 		if (using_out.rct != none && (*(using_out.rct)).size() > 0) {
 			decoy_req__amounts.push_back("0");
@@ -320,15 +334,17 @@ struct _SendFunds_ConstructAndSendTx_Args
 	const secret_key &sec_viewKey;
 	const secret_key &sec_spendKey;
 	//
-	optional<uint64_t> passedIn_attemptAt_fee;
+	optional<uint64_t> prior_attempt_size_calcd_fee;
+	optional<SpendableOutputToRandomAmountOutputs> prior_attempt_unspent_outs_to_mix_outs;
 	size_t constructionAttempt;
 };
 void _reenterable_construct_and_send_tx(
 	const _SendFunds_ConstructAndSendTx_Args &args,
 	//
 	// re-entry params
-	optional<uint64_t> passedIn_attemptAt_fee						= none,
-	size_t constructionAttempt 										= 0
+	optional<uint64_t> prior_attempt_size_calcd_fee								= none,
+	optional<SpendableOutputToRandomAmountOutputs> prior_attempt_unspent_outs_to_mix_outs		= none,
+	size_t constructionAttempt												= 0
 ) {
 	args.status_update_fn(calculatingFee);
 	//
@@ -347,7 +363,8 @@ void _reenterable_construct_and_send_tx(
 		args.fee_per_b,
 		args.fee_quantization_mask,
 		//
-		passedIn_attemptAt_fee // use this for passing step2 "must-reconstruct" return values back in, i.e. re-entry; when nil, defaults to attempt at network min
+		prior_attempt_size_calcd_fee, // use this for passing step2 "must-reconstruct" return values back in, i.e. re-entry; when nil, defaults to attempt at network min
+		prior_attempt_unspent_outs_to_mix_outs // on re-entry, re-use the same outs and requested decoys, in order to land on the correct calculated fee
 	);
 	if (step1_retVals.errCode != noError) {
 		SendFunds_Error_RetVals error_retVals;
@@ -360,18 +377,38 @@ void _reenterable_construct_and_send_tx(
 	api_fetch_cb_fn get_random_outs_fn__cb_fn = [
 		args,
 		step1_retVals,
+		prior_attempt_unspent_outs_to_mix_outs,
 		constructionAttempt,
 		use_fork_rules
 	] (
 		const property_tree::ptree &res
 	) -> void {
-		auto parsed_res = new__parsed_res__get_random_outs(res);
+		auto parsed_res = (res != boost::property_tree::ptree{})
+			? new__parsed_res__get_random_outs(res)
+			: LightwalletAPI_Res_GetRandomOuts{ boost::none/*err_msg*/, vector<RandomAmountOutputs>{}/*mix_outs*/ };
 		if (parsed_res.err_msg != none) {
 			SendFunds_Error_RetVals error_retVals;
 			error_retVals.explicit_errMsg = std::move(*(parsed_res.err_msg));
 			args.error_cb_fn(error_retVals);
 			return;
 		}
+
+		Tie_Outs_to_Mix_Outs_RetVals tie_outs_to_mix_outs_retVals;
+		monero_transfer_utils::pre_step2_tie_unspent_outs_to_mix_outs_for_all_future_tx_attempts(
+			tie_outs_to_mix_outs_retVals,
+			//
+			step1_retVals.using_outs,
+			*(parsed_res.mix_outs),
+			//
+			prior_attempt_unspent_outs_to_mix_outs
+		);
+		if (tie_outs_to_mix_outs_retVals.errCode != noError) {
+			SendFunds_Error_RetVals error_retVals;
+			error_retVals.errCode = tie_outs_to_mix_outs_retVals.errCode;
+			args.error_cb_fn(error_retVals);
+			return;
+		}
+
 		Send_Step2_RetVals step2_retVals;
 		monero_transfer_utils::send_step2__try_create_transaction(
 			step2_retVals,
@@ -388,7 +425,7 @@ void _reenterable_construct_and_send_tx(
 			step1_retVals.using_outs,
 			args.fee_per_b,
 			args.fee_quantization_mask,
-			*(parsed_res.mix_outs),
+			tie_outs_to_mix_outs_retVals.mix_outs,
 			std::move(use_fork_rules),
 			args.unlock_time,
 			args.nettype
@@ -411,7 +448,8 @@ void _reenterable_construct_and_send_tx(
 			_reenterable_construct_and_send_tx(
 				args,
 				//
-				step2_retVals.fee_actually_needed, // -> reconstruction attempt's step1's passedIn_attemptAt_fee
+				step2_retVals.fee_actually_needed, // -> reconstruction attempt's step1's prior_attempt_size_calcd_fee
+				tie_outs_to_mix_outs_retVals.prior_attempt_unspent_outs_to_mix_outs_new,
 				constructionAttempt+1
 			);
 			return;
@@ -462,10 +500,16 @@ void _reenterable_construct_and_send_tx(
 	//
 	args.status_update_fn(fetchingDecoyOutputs);
 	//
-	args.get_random_outs_fn(
-		new__req_params__get_random_outs(step1_retVals.using_outs),
-		get_random_outs_fn__cb_fn
+	// we won't need to make request for random outs every tx construction attempt, if already passed in out for all outs
+	auto req_params = new__req_params__get_random_outs(
+		step1_retVals.using_outs,
+		prior_attempt_unspent_outs_to_mix_outs
 	);
+	if (req_params.amounts.size() > 0) {
+		args.get_random_outs_fn(req_params, get_random_outs_fn__cb_fn);
+	} else {
+		get_random_outs_fn__cb_fn(boost::property_tree::ptree{});
+	}
 }
 //
 //

src/monero_send_routine.hpp

@@ -115,7 +115,8 @@ namespace monero_send_routine
 		return req_params_ss.str();
 	}
 	LightwalletAPI_Req_GetRandomOuts new__req_params__get_random_outs( // used internally and by emscr async send impl
-		vector<SpendableOutput> &step1__using_outs
+		const vector<SpendableOutput> &step1__using_outs,
+		const optional<SpendableOutputToRandomAmountOutputs> &prior_attempt_unspent_outs_to_mix_outs
 	);
 	typedef std::function<void(
 		LightwalletAPI_Req_GetRandomOuts, // req_params - use these for making the request

src/monero_transfer_utils.cpp

@@ -238,7 +238,8 @@ void monero_transfer_utils::send_step1__prepare_params_for_get_decoys(
 	uint64_t fee_per_b, // per v8
 	uint64_t fee_quantization_mask,
 	//
-	optional<uint64_t> passedIn_attemptAt_fee
+	optional<uint64_t> prior_attempt_size_calcd_fee,
+	optional<SpendableOutputToRandomAmountOutputs> prior_attempt_unspent_outs_to_mix_outs
 ) {
 	retVals = {};
 	//
@@ -274,12 +275,12 @@ void monero_transfer_utils::send_step1__prepare_params_for_get_decoys(
 	const uint64_t fee_multiplier = get_fee_multiplier(simple_priority, default_priority(), get_fee_algorithm(use_fork_rules_fn), use_fork_rules_fn);
 	//
 	uint64_t attempt_at_min_fee;
-	if (passedIn_attemptAt_fee == none) {
+	if (prior_attempt_size_calcd_fee == none) {
 		attempt_at_min_fee = estimate_fee(true/*use_per_byte_fee*/, true/*use_rct*/, 1/*est num inputs*/, fake_outs_count, 2, extra.size(), bulletproof, clsag, base_fee, fee_multiplier, fee_quantization_mask);
 		// use a minimum viable estimate_fee() with 1 input. It would be better to under-shoot this estimate, and then need to use a higher fee  from calculate_fee() because the estimate is too low,
 		// versus the worse alternative of over-estimating here and getting stuck using too high of a fee that leads to fingerprinting
 	} else {
-		attempt_at_min_fee = *passedIn_attemptAt_fee;
+		attempt_at_min_fee = *prior_attempt_size_calcd_fee;
 	}
 	struct Total
 	{
@@ -299,6 +300,20 @@ void monero_transfer_utils::send_step1__prepare_params_for_get_decoys(
 	// Gather outputs and amount to use for getting decoy outputs…
 	uint64_t using_outs_amount = 0;
 	vector<SpendableOutput>  remaining_unusedOuts = unspent_outs; // take copy so not to modify original
+
+	// start by using all the passed in outs that were selected in a prior tx construction attempt
+	if (prior_attempt_unspent_outs_to_mix_outs != none) {
+		for (size_t i = 0; i < remaining_unusedOuts.size(); ++i) {
+			SpendableOutput &out = remaining_unusedOuts[i];
+
+			// search for out by public key to see if it should be re-used in an attempt
+			if (prior_attempt_unspent_outs_to_mix_outs->find(out.public_key) != prior_attempt_unspent_outs_to_mix_outs->end()) {
+				using_outs_amount += out.amount;
+				retVals.using_outs.push_back(std::move(pop_index(remaining_unusedOuts, i)));
+			}
+		}
+	}
+
 	// TODO: factor this out to get spendable balance for display in the MM wallet:
 	while (using_outs_amount < potential_total && remaining_unusedOuts.size() > 0) {
 		auto out = pop_random_value(remaining_unusedOuts);
@@ -328,7 +343,7 @@ void monero_transfer_utils::send_step1__prepare_params_for_get_decoys(
 		bulletproof, clsag, base_fee, fee_multiplier, fee_quantization_mask
 	);
 	// if newNeededFee < neededFee, use neededFee instead (should only happen on the 2nd or later times through (due to estimated fee being too low))
-	if (passedIn_attemptAt_fee != none && needed_fee < attempt_at_min_fee) {
+	if (prior_attempt_size_calcd_fee != none && needed_fee < attempt_at_min_fee) {
 		needed_fee = attempt_at_min_fee;
 	}
 	//
@@ -392,6 +407,75 @@ void monero_transfer_utils::send_step1__prepare_params_for_get_decoys(
 //		// TODO?
 //	}
 }
+//
+//
+void monero_transfer_utils::pre_step2_tie_unspent_outs_to_mix_outs_for_all_future_tx_attempts(
+	Tie_Outs_to_Mix_Outs_RetVals &retVals,
+	//
+	const vector<SpendableOutput> &using_outs,
+	vector<RandomAmountOutputs> mix_outs_from_server,
+	//
+	const optional<SpendableOutputToRandomAmountOutputs> &prior_attempt_unspent_outs_to_mix_outs
+) {
+	retVals.errCode = noError;
+	//
+	// combine newly requested mix outs returned from the server, with the already known decoys from prior tx construction attempts,
+	// so that the same decoys will be re-used with the same outputs in all tx construction attempts. This ensures fee returned
+	// by calculate_fee() will be correct in the final tx, and also reduces number of needed trips to the server during tx construction.
+	SpendableOutputToRandomAmountOutputs prior_attempt_unspent_outs_to_mix_outs_new;
+	if (prior_attempt_unspent_outs_to_mix_outs) {
+		prior_attempt_unspent_outs_to_mix_outs_new = *prior_attempt_unspent_outs_to_mix_outs;
+	}
+
+	std::vector<RandomAmountOutputs> mix_outs;
+	mix_outs.reserve(using_outs.size());
+
+	for (size_t i = 0; i < using_outs.size(); ++i) {
+		auto out = using_outs[i];
+
+		// if we don't already know of a particular out's mix outs (from a prior attempt),
+		// then tie out to a set of mix outs retrieved from the server
+		if (prior_attempt_unspent_outs_to_mix_outs_new.find(out.public_key) == prior_attempt_unspent_outs_to_mix_outs_new.end()) {
+			for (size_t j = 0; j < mix_outs_from_server.size(); ++j) {
+				if ((out.rct != none && mix_outs_from_server[j].amount != 0) ||
+					(out.rct == none && mix_outs_from_server[j].amount != out.amount)) {
+					continue;
+				}
+
+				RandomAmountOutputs output_mix_outs = pop_index(mix_outs_from_server, j);
+
+				// if we need to retry constructing tx, will remember to use same mix outs for this out on subsequent attempt(s)
+				prior_attempt_unspent_outs_to_mix_outs_new[out.public_key] = output_mix_outs.outputs;
+				mix_outs.push_back(std::move(output_mix_outs));
+
+				break;
+			}
+		} else {
+			RandomAmountOutputs output_mix_outs;
+			output_mix_outs.outputs = prior_attempt_unspent_outs_to_mix_outs_new[out.public_key];
+			output_mix_outs.amount = out.amount;
+			mix_outs.push_back(std::move(output_mix_outs));
+		}
+	}
+
+	// we expect to have a set of mix outs for every output in the tx
+	if (mix_outs.size() != using_outs.size()) {
+		retVals.errCode = notEnoughUsableDecoysFound;
+		return;
+	}
+
+	// we expect to use up all mix outs returned by the server
+	if (!mix_outs_from_server.empty()) {
+		retVals.errCode = tooManyDecoysRemaining;
+		return;
+	}
+
+	retVals.mix_outs = std::move(mix_outs);
+	retVals.prior_attempt_unspent_outs_to_mix_outs_new = std::move(prior_attempt_unspent_outs_to_mix_outs_new);
+}
+//
+//
+//
 void monero_transfer_utils::send_step2__try_create_transaction(
 	Send_Step2_RetVals &retVals,
 	//