Contracts: replace reachable panics with errors

De-panic the contract paths flagged in review plus several found alongside them:
- new: use unsigned_abs (abs() panics on i64::MIN).
- proofs: surface InvoiceProof serialization errors instead of expect(); bounds-
  check the participant index before indexing participant_data.
- utils/slate: propagate the error when a context input is missing from the db
  instead of unwrapping.
- revoke: return an error when a locked input has no cached commitment.
- selection: checked i64/u64 conversions so large amounts can't wrap into a
  wrong (possibly negative) change.
- types: checked sum of output amounts.

Addresses PR #754 review (new.rs, proofs.rs, utils.rs, selection.rs).

libwallet/src/contract/actions/new.rs

@@ -69,7 +69,8 @@ where
 	let num_participants = setup_args.num_participants;
 	let mut slate = Slate::blank(num_participants, net_change > 0);
 	// We set slate.amount to contain the _positive_ net_change for the other party so they can derive expectations.
-	slate.amount = net_change.abs() as u64;
+	// unsigned_abs avoids the i64::MIN overflow panic of abs().
+	slate.amount = net_change.unsigned_abs();
 	debug!("contract::new => slate amount: {}", slate.amount);
 
 	// Perform setup for the slate

libwallet/src/contract/actions/revoke.rs

@@ -84,7 +84,9 @@ where
 	if my_contributed_inputs.len() == 0 {
 		return Ok(None);
 	}
-	let input_commit = my_contributed_inputs[0].commit.as_ref().unwrap();
+	let input_commit = my_contributed_inputs[0].commit.as_ref().ok_or_else(|| {
+		Error::GenericError("Locked input has no cached commitment".to_string())
+	})?;
 	// Account label for the self-spend, so recovered funds return to the inputs' account.
 	let src_acct_name = w
 		.acct_path_iter()?

libwallet/src/contract/proofs.rs

@@ -197,6 +197,15 @@ impl InvoiceProof {
 		participant_index: usize,
 		sender_address: Option<DalekPublicKey>,
 	) -> Result<Self, Error> {
+		// Bounds-check the participant index before indexing participant_data, so a
+		// malformed slate returns an error rather than panicking.
+		if participant_index >= slate.participant_data.len() {
+			return Err(Error::GenericError(format!(
+				"Participant index {} out of range for slate with {} participant(s)",
+				participant_index,
+				slate.participant_data.len()
+			)));
+		}
 		// Sender address is either provided or in slate (or error)
 		let sender_address = match sender_address {
 			Some(a) => a,
@@ -257,8 +266,8 @@ impl InvoiceProof {
 			secret: d_skey,
 		};
 		let mut sig_data_bin = Vec::new();
-		let _ = grin_ser::serialize_default(&mut sig_data_bin, &InvoiceProofBin(self.clone()))
-			.expect("serialization failed");
+		grin_ser::serialize_default(&mut sig_data_bin, &InvoiceProofBin(self.clone()))
+			.map_err(|e| Error::GenericError(format!("InvoiceProof serialization failed: {}", e)))?;
 
 		Ok((keypair.sign(&sig_data_bin), pub_key))
 	}
@@ -276,8 +285,8 @@ impl InvoiceProof {
 
 		// Rebuild message
 		let mut sig_data_bin = Vec::new();
-		let _ = grin_ser::serialize_default(&mut sig_data_bin, &InvoiceProofBin(self.clone()))
-			.expect("serialization failed");
+		grin_ser::serialize_default(&mut sig_data_bin, &InvoiceProofBin(self.clone()))
+			.map_err(|e| Error::GenericError(format!("InvoiceProof serialization failed: {}", e)))?;
 
 		if recipient_address
 			.verify(&sig_data_bin, self.promise_signature.as_ref().unwrap())

libwallet/src/contract/selection.rs

@@ -142,9 +142,10 @@ where
 	// or a single one if we're the receiver doing a payjoin.
 	// If we are the receiver, we pretend we have a virtual input from the sender (for which we don't pay the fees) so we can easily
 	// test that lhs >= rhs and see if we will be able to satisfy equation. We simulate this by starting with lhs = net_change.
-	let mut lhs = 0;
+	let mut lhs: u64 = 0;
 	if net_change > 0 {
-		lhs = net_change as u64; // TODO: check bounds
+		lhs = u64::try_from(net_change)
+			.map_err(|_| Error::GenericError(format!("net change {} exceeds u64", net_change)))?;
 	}
 	// We want to count how many inputs we've picked _so far_. This is used to prevent picking
 	// all 0*H +r*G outputs when we call with min_input_amount=0 and want just a payjoin.
@@ -262,9 +263,16 @@ fn build_output_amount_list(
 	let custom_outputs_sum = my_output_amounts.iter().sum::<u64>();
 	// 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)
-	// TODO: check bounds when casting.
-	let my_change_output_amount =
-		(my_input_sum - custom_outputs_sum) as i64 + expected_net_change - my_fee_cost as i64;
+	// 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 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;
 	// 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!(

libwallet/src/contract/slate.rs

@@ -103,7 +103,7 @@ where
 	let batch = w.batch(keychain_mask)?;
 	for (key_id, mmr_index, _) in context.get_inputs() {
 		// We have no information if the input is a coinbase or not, so we fetch the data from DB
-		let coin = batch.get(&key_id, &mmr_index).unwrap();
+		let coin = batch.get(&key_id, &mmr_index)?;
 		if coin.is_coinbase {
 			slate.add_transaction_elements(
 				&keychain,

libwallet/src/contract/types.rs

@@ -55,7 +55,10 @@ impl OutputSelectionArgs {
 	}
 	/// Returns the sum of our output amounts
 	pub fn sum_output_amounts(&self) -> Result<u64, Error> {
-		Ok(self.output_amounts()?.iter().sum())
+		self.output_amounts()?
+			.iter()
+			.try_fold(0u64, |acc, v| acc.checked_add(*v))
+			.ok_or_else(|| Error::GenericError("output amounts sum overflow".to_string()))
 	}
 	/// Returns the number of custom outputs
 	pub fn num_custom_outputs(&self) -> usize {

libwallet/src/contract/utils.rs

@@ -266,7 +266,7 @@ where
 		}
 		// Lock inputs
 		for id in context.get_inputs() {
-			let mut coin = batch.get(&id.0, &id.1).unwrap();
+			let mut coin = batch.get(&id.0, &id.1)?;
 			// At this point we already have context.log_id set
 			coin.tx_log_entry = context.log_id;
 			batch.lock_output(&mut coin)?;