Replace reachable panics in contract flow with typed errors

Contract setup/sign and output selection used panic! on
protocol-reachable invariant violations: receiver fee exceeding
net_change, net_change / num_participants mismatch between the setup and
sign phases, a negative change output, conflicting selection args, and a
failed kernel-excess computation. A crafted or inconsistent counterparty
slate could abort the wallet. Return Error::GenericError (or propagate the
underlying error) instead; build_output_amount_list now returns Result.

libwallet/src/contract/context.rs

@@ -95,11 +95,11 @@ where
 	// Make sure `my_fee < net_change` holds for the receiver. This can't be true for a self-spend, because nobody
 	// has a net_change > 0 which makes a self-spend ok to be a net negative when fees are included.
 	if net_change > 0 && my_fee.fee() > net_change.abs() as u64 {
-		panic!(
+		return Err(Error::GenericError(format!(
 			"My contribution as a receiver would be net negative. my_fee: {}, net_change: {}",
 			my_fee.fee(),
 			net_change
-		);
+		)));
 	}
 	// Add my share of fee contribution to the slate fees
 	slate.fee_fields = FeeFields::new(0, slate.fee_fields.fee() + my_fee.fee())?;

libwallet/src/contract/selection.rs

@@ -72,7 +72,7 @@ where
 		inputs.clone().iter().map(|out| out.value).sum::<u64>(),
 		fee.fee(),
 		setup_args,
-	);
+	)?;
 	Ok((inputs, output_amounts, fee))
 }
 
@@ -258,7 +258,7 @@ fn build_output_amount_list(
 	my_input_sum: u64,
 	my_fee_cost: u64,
 	setup_args: &ContractSetupArgsAPI,
-) -> Vec<u64> {
+) -> Result<Vec<u64>, Error> {
 	let expected_net_change = setup_args.net_change.unwrap();
 	let mut my_output_amounts = setup_args.selection_args.output_amounts();
 	let custom_outputs_sum = my_output_amounts.iter().sum::<u64>();
@@ -269,10 +269,10 @@ fn build_output_amount_list(
 		(my_input_sum - custom_outputs_sum) as i64 + expected_net_change - my_fee_cost as i64;
 	// TODO: Check if it's even possible for change output to be negative (it shouldn't be if the equation is correct)
 	if my_change_output_amount < 0 {
-		panic!(
-			"contract::selection::build_output_amount_list => ERROR: This should never happen!!! Values: my_input_sum: {}, expected_net_change: {}, my_fee_cost: {}",
+		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
-		);
+		)));
 	}
 	// Add our change/receiver output (which can be a zero-value output) to the list of outputs
 	my_output_amounts.push(my_change_output_amount as u64);
@@ -280,7 +280,7 @@ fn build_output_amount_list(
 		"contract::selection::build_output_amount_list => inputs sum: {}, my_output_amounts:{:#?}",
 		my_input_sum, my_output_amounts
 	);
-	my_output_amounts
+	Ok(my_output_amounts)
 }
 
 /// Compares the output selection args provided at call with those from Context and checks whether they conflict
@@ -292,7 +292,10 @@ pub fn verify_selection_consistency(
 	// default or exactly the same strategy we defined when doing the setup phase.
 	// TODO: Test that this works. Perhaps we'd have to define how to compare the two?
 	if cur_args != ctx_args && cur_args != &OutputSelectionArgs::default() {
-		panic!("Can't define selection args now because we've already done the setup phase. ctx_selection_args:{:#?}, cur_selection_args:{:#?}", ctx_args, cur_args);
+		return Err(Error::GenericError(format!(
+			"Can't define selection args now because we've already done the setup phase. ctx_selection_args:{:#?}, cur_selection_args:{:#?}",
+			ctx_args, cur_args
+		)));
 	}
 	// NOTE: The logic above isn't perfect. This is because the user could define arguments that are the default. In this case
 	// we'd simply silently use the arguments provided in the setup phase. This could be confusing for the user.

libwallet/src/contract/utils.rs

@@ -82,10 +82,7 @@ where
 	tx_log_entry.num_inputs = context.input_ids.len();
 	tx_log_entry.fee = context.fee;
 	// Set kernel information
-	match slate.calc_excess(keychain.secp()) {
-		Ok(e) => tx_log_entry.kernel_excess = Some(e),
-		Err(_) => panic!("We can't update tx log entry. Excess could not be computed."),
-	};
+	tx_log_entry.kernel_excess = Some(slate.calc_excess(keychain.secp())?);
 	tx_log_entry.kernel_lookup_min_height = Some(current_height);
 
 	// If we're sending and there's payment proof info in the slate added by recipient, store as well
@@ -151,10 +148,10 @@ where
 			match expected_net_change {
 				Some(args_net_change) => {
 					if ctx.get_net_change() != args_net_change {
-						panic!(
+						return Err(Error::GenericError(format!(
 							"Expected net change mismatch! Context.net_change: {}, setup_args.net_change: {}",
 							ctx.get_net_change(), args_net_change
-						);
+						)));
 					}
 				}
 				None => (),
@@ -349,18 +346,18 @@ pub fn verify_setup_args_consistency(
 ) -> Result<(), Error> {
 	// Compare net_change
 	if ctx_setup_args.net_change.unwrap() != cur_setup_args.net_change.unwrap() {
-		panic!(
+		return Err(Error::GenericError(format!(
 			"Inconsistent net change. Ctx net_change:{}, Current net_change: {}",
 			ctx_setup_args.net_change.unwrap(),
 			cur_setup_args.net_change.unwrap()
-		);
+		)));
 	}
 	// Compare num_participants
 	if ctx_setup_args.num_participants != cur_setup_args.num_participants {
-		panic!(
+		return Err(Error::GenericError(format!(
 			"Inconsistent num_participants. Ctx num_participants:{}, Current num_participants: {}",
 			ctx_setup_args.num_participants, cur_setup_args.num_participants
-		);
+		)));
 	}
 	// TODO: Should we verify add_outputs?
 	// TODO: verify that the parent_key_id is consistent, perhaps even with the active_account set?