diff --git a/libwallet/src/contract/actions/view.rs b/libwallet/src/contract/actions/view.rs index aa6d913..1c3ff60 100644 --- a/libwallet/src/contract/actions/view.rs +++ b/libwallet/src/contract/actions/view.rs @@ -72,7 +72,7 @@ where // If we have a local context for this slate we've agreed on a net change; surface it. let agreed_net_change = match w.get_private_context(keychain_mask, slate.id.as_bytes()) { - Ok(ctx) => Some(ctx.get_net_change()), + Ok(ctx) => Some(ctx.get_net_change()?), Err(Error::NotFoundErr(_)) => None, Err(e) => return Err(e), }; diff --git a/libwallet/src/contract/context.rs b/libwallet/src/contract/context.rs index a3d806c..d838299 100644 --- a/libwallet/src/contract/context.rs +++ b/libwallet/src/contract/context.rs @@ -94,7 +94,9 @@ where updater::refresh_outputs(w, keychain_mask, parent_key_id, false)?; // Fee contribution estimation - let net_change = setup_args.net_change.unwrap(); + let net_change = setup_args.net_change.ok_or_else(|| { + Error::GenericError("Contract requires a net change (--send or --receive)".to_string()) + })?; // select inputs to estimate fee cost let (inputs, _, my_fee) = prepare_outputs(w, &parent_key_id, current_height, &setup_args, None)?; @@ -129,7 +131,7 @@ where context.setup_args = Some(setup_args.clone()); debug!( "Setting Context.net_change as: {}", - context.get_net_change() + context.get_net_change()? ); Ok(context) @@ -152,7 +154,10 @@ where debug!("contract::utils::add_outputs => outputs have already been added, returning."); return Ok(()); } - let setup_args = context.setup_args.as_ref().unwrap(); + let setup_args = context + .setup_args + .as_ref() + .ok_or_else(|| Error::GenericError("Context carries no contract setup args".to_string()))?; debug!("contract::utils::add_outputs => adding outputs"); let current_height = w.w2n_client().get_chain_tip()?.0; let parent_key_id = &context.parent_key_id; @@ -165,7 +170,19 @@ where &setup_args, context.fee, )?; - assert_eq!(my_fee.fee(), context.fee.unwrap().fee(), "my_fee!=ctx.fee"); + // An old or damaged context, or a future change to the fee schedule, can reach this + // with a stored fee that no longer matches what we just recomputed. Report both + // values instead of terminating the wallet. + let ctx_fee = context + .fee + .ok_or_else(|| Error::GenericError("Context carries no fee".to_string()))?; + if my_fee.fee() != ctx_fee.fee() { + return Err(Error::GenericError(format!( + "Recomputed fee {} does not match the fee stored in the context {}", + my_fee.fee(), + ctx_fee.fee() + ))); + } // Add selected/created inputs/outputs to the context add_inputs_to_ctx(context, &inputs)?; add_outputs_to_ctx(w, keychain_mask, context, my_output_amounts)?; diff --git a/libwallet/src/contract/selection.rs b/libwallet/src/contract/selection.rs index e2e6bfb..edeab0e 100644 --- a/libwallet/src/contract/selection.rs +++ b/libwallet/src/contract/selection.rs @@ -90,15 +90,25 @@ where // For either party, the following MUST hold for the inputs the function returns: // Σmy_inputs >= Σmy_outputs + my_fee_cost // Each party later balances the equation by adding an additional output (change output or receiver output) - let net_change = setup_args.net_change.unwrap(); + let net_change = setup_args.net_change.ok_or_else(|| { + Error::GenericError("Contract requires a net change (--send or --receive)".to_string()) + })?; let custom_outputs_amount_sum = setup_args.selection_args.sum_output_amounts()?; + // unsigned_abs, as abs() panics on i64::MIN let pay_amount = if net_change < 0 { - net_change.abs() as u64 + net_change.unsigned_abs() } else { 0 }; // Add the amount we pay and the custom outputs to rhs of the equation - let rhs = pay_amount + custom_outputs_amount_sum; + let rhs = pay_amount + .checked_add(custom_outputs_amount_sum) + .ok_or_else(|| { + Error::GenericError(format!( + "Amount overflow: pay amount {} plus custom outputs {}", + pay_amount, custom_outputs_amount_sum + )) + })?; let required_inputs = setup_args.selection_args.required_inputs(); let is_payjoin = setup_args.selection_args.is_payjoin(); let is_self_spend = setup_args.num_participants == 1; @@ -258,26 +268,41 @@ fn build_output_amount_list( my_fee_cost: u64, setup_args: &ContractSetupArgsAPI, ) -> Result, Error> { - let expected_net_change = setup_args.net_change.unwrap(); + let expected_net_change = setup_args.net_change.ok_or_else(|| { + Error::GenericError("Contract requires a net change (--send or --receive)".to_string()) + })?; let mut my_output_amounts = setup_args.selection_args.output_amounts()?; - let custom_outputs_sum = my_output_amounts.iter().sum::(); - // We know that `Σmy_inputs >= Σmy_outputs + my_fee_cost` holds so we balance the equation by adding - // an additional output holding the missing amount (change output or receiver output) - // Checked conversions so a very large amount can't wrap into a wrong (possibly negative) change. - let input_minus_outputs = i64::try_from(my_input_sum - custom_outputs_sum).map_err(|_| { - Error::GenericError(format!( - "input sum {} exceeds i64", - my_input_sum - custom_outputs_sum - )) + let custom_outputs_sum = my_output_amounts + .iter() + .try_fold(0u64, |acc, v| acc.checked_add(*v)) + .ok_or_else(|| Error::GenericError("Custom output amounts overflow".to_string()))?; + // We balance `Σmy_inputs >= Σmy_outputs + my_fee_cost` by adding an output holding the + // missing amount (change output or receiver output). A receiver that is not doing a + // payjoin contributes no inputs while --make-outputs is still allowed, so my_input_sum + // can be below custom_outputs_sum: the whole equation is evaluated in checked i64 so + // that case is reported rather than wrapping. + let input_sum = i64::try_from(my_input_sum) + .map_err(|_| Error::GenericError(format!("input sum {} exceeds i64", my_input_sum)))?; + let outputs_sum = i64::try_from(custom_outputs_sum).map_err(|_| { + Error::GenericError(format!("output sum {} exceeds i64", custom_outputs_sum)) })?; let fee_cost = i64::try_from(my_fee_cost) .map_err(|_| Error::GenericError(format!("fee cost {} exceeds i64", my_fee_cost)))?; - let my_change_output_amount = input_minus_outputs + expected_net_change - fee_cost; + let my_change_output_amount = input_sum + .checked_sub(outputs_sum) + .and_then(|v| v.checked_add(expected_net_change)) + .and_then(|v| v.checked_sub(fee_cost)) + .ok_or_else(|| { + Error::GenericError(format!( + "contract::selection::build_output_amount_list => amount overflow. Values: my_input_sum: {}, custom_outputs_sum: {}, expected_net_change: {}, my_fee_cost: {}", + input_sum, outputs_sum, expected_net_change, fee_cost + )) + })?; // A negative change output would mean the selection math is off; reject it explicitly. if my_change_output_amount < 0 { return Err(Error::GenericError(format!( - "contract::selection::build_output_amount_list => negative change output. Values: my_input_sum: {}, expected_net_change: {}, my_fee_cost: {}", - my_input_sum as i64, expected_net_change, my_fee_cost as i64 + "contract::selection::build_output_amount_list => negative change output. Values: my_input_sum: {}, custom_outputs_sum: {}, expected_net_change: {}, my_fee_cost: {}", + input_sum, outputs_sum, expected_net_change, fee_cost ))); } // Add our change/receiver output (which can be a zero-value output) to the list of outputs @@ -338,6 +363,39 @@ mod tests { rv } + // A receiver that is not doing a payjoin contributes no inputs, while --make-outputs + // is still allowed, so the output balance is computed from a zero input sum against a + // non-zero custom output sum. That has to be reported, not wrapped around. + #[test] + fn receiver_no_payjoin_with_custom_outputs() { + let setup_args = ContractSetupArgsAPI { + net_change: Some(5_000_000_000), + selection_args: OutputSelectionArgs { + // no use_inputs, so is_payjoin() is false and no inputs are selected + make_outputs: Some(vec![1_000_000_000]), + ..Default::default() + }, + ..Default::default() + }; + // Receiving more than the custom outputs and the fee leaves a valid balance + let my_fee = my_fee_contribution(0, 2, 1, 2).unwrap(); + let amounts = build_output_amount_list(0, my_fee.fee(), &setup_args).unwrap(); + assert_eq!(amounts.len(), 2); + assert_eq!(amounts[0], 1_000_000_000); + assert_eq!(amounts[1], 5_000_000_000 - 1_000_000_000 - my_fee.fee()); + + // Custom outputs beyond what we receive are rejected rather than underflowing + let setup_args = ContractSetupArgsAPI { + net_change: Some(1_000_000_000), + selection_args: OutputSelectionArgs { + make_outputs: Some(vec![5_000_000_000]), + ..Default::default() + }, + ..Default::default() + }; + assert!(build_output_amount_list(0, my_fee.fee(), &setup_args).is_err()); + } + #[test] fn sender_no_inputs() { // net_change=-1, no inputs, no fee committed => NotEnoughFunds diff --git a/libwallet/src/contract/utils.rs b/libwallet/src/contract/utils.rs index aa03241..439cd0d 100644 --- a/libwallet/src/contract/utils.rs +++ b/libwallet/src/contract/utils.rs @@ -150,18 +150,19 @@ where debug!("contract::sign => context found"); // We have a context so we must have agreed on a certain net_change value in Context.net_change. // If we have both Context.net_change and setup_args.net_change, then they must be equal. + let ctx_net_change = ctx.get_net_change()?; match expected_net_change { Some(args_net_change) => { - if ctx.get_net_change() != args_net_change { + if ctx_net_change != args_net_change { return Err(Error::GenericError(format!( "Expected net change mismatch! Context.net_change: {}, setup_args.net_change: {}", - ctx.get_net_change(), args_net_change + ctx_net_change, args_net_change ))); } } None => (), } - expected_net_change = Some(ctx.get_net_change()); + expected_net_change = Some(ctx_net_change); } Err(Error::NotFoundErr(_)) => { debug!("contract::utils::get_net_change => context not found") @@ -212,7 +213,7 @@ where let mut tx_log_entry = { if !context.log_id.is_some() { // We create a new entry with log_id=0 and but replace it with the real id before committing - create_tx_log_entry(slate, context.get_net_change(), parent_key_id.clone(), 0)? + create_tx_log_entry(slate, context.get_net_change()?, parent_key_id.clone(), 0)? } else { w.get_tx_log_entry(parent_key_id.clone(), context.log_id.unwrap())? .unwrap() diff --git a/libwallet/src/types.rs b/libwallet/src/types.rs index d3a6b46..8bfe295 100644 --- a/libwallet/src/types.rs +++ b/libwallet/src/types.rs @@ -455,9 +455,16 @@ impl Context { } impl Context { - /// Returns net_change for the contract - pub fn get_net_change(&self) -> i64 { - self.setup_args.as_ref().unwrap().net_change.unwrap() + /// Returns net_change for the contract. Context is shared with the standard + /// transaction flows, where setup_args is None, so a context that does not belong to + /// a contract is reported rather than unwrapped. + pub fn get_net_change(&self) -> Result { + self.setup_args + .as_ref() + .and_then(|args| args.net_change) + .ok_or_else(|| { + Error::GenericError("Context carries no contract net change".to_string()) + }) } /// Tracks an output contributing to my excess value (if it needs to