derive contract signing state

controller/tests/contract_srs.rs

@@ -20,6 +20,7 @@ extern crate log;
 use grin_wallet_libwallet as libwallet;
 
 use grin_core::core::transaction::{CommitWrapper, Input, Inputs, OutputFeatures, Transaction};
+use grin_util::secp::Signature;
 use impls::test_framework::{self};
 use libwallet::contract::my_fee_contribution;
 use libwallet::contract::types::{ContractNewArgsAPI, ContractSetupArgsAPI, OwnCommitmentStatus};
@@ -94,6 +95,30 @@ fn contract_srs_tx_impl(test_dir: &'static str) -> Result<(), libwallet::Error>
 			signatures: 0,
 		},
 	);
+	wallet::controller::owner_single_use(
+		send_wallet.clone(),
+		send_mask,
+		PathBuf::from(test_dir),
+		|api, m| {
+			let progress = common::wallet_progress(api, m)?;
+			let mut invalid = slate.clone();
+			invalid.participant_data[0].part_sig =
+				Some(Signature::from_raw_data(&[0; 64]).unwrap());
+			let setup_args = ContractSetupArgsAPI {
+				selection_args: common::contract_selection_args(),
+				net_change: Some(-5_000_000_000),
+				..Default::default()
+			};
+			let err = {
+				let mut owner = api.wallet_inst.lock();
+				let wallet = owner.lc_provider()?.wallet_inst()?;
+				libwallet::contract::setup(wallet, m, &invalid, &setup_args).unwrap_err()
+			};
+			assert!(matches!(err, libwallet::Error::LibTX(_)));
+			assert_eq!(common::wallet_progress(api, m)?, progress);
+			Ok(())
+		},
+	)?;
 
 	wallet::controller::owner_single_use(
 		recv_wallet.clone(),
@@ -192,6 +217,18 @@ fn contract_srs_tx_impl(test_dir: &'static str) -> Result<(), libwallet::Error>
 					if msg == &format!("Slate with id:{} has already been signed.", incoming.id)
 			));
 			assert_eq!(common::wallet_progress(api, m)?, progress);
+
+			let err = {
+				let mut owner = api.wallet_inst.lock();
+				let wallet = owner.lc_provider()?.wallet_inst()?;
+				libwallet::contract::setup(wallet, m, &slate, &args).unwrap_err()
+			};
+			assert!(matches!(
+				err,
+				libwallet::Error::GenericError(ref msg)
+					if msg == &format!("Slate with id:{} has already been signed.", slate.id)
+			));
+			assert_eq!(common::wallet_progress(api, m)?, progress);
 			Ok(())
 		},
 	)?;

libwallet/src/contract/actions/README.md

@@ -65,16 +65,12 @@ Separating side effects until the 'save_step' part would make these functions mu
  - Decide whether `contract_accounts_switch.rs` is a legitimate scenario, or whether
    signing under a different account than setup should error.
 
-#### save_step
-
-    // TODO:
-    //  - is_signed should be derived from the slate
-    //  - Consider taking ownership of Context here. It should not be used after this is called.
-
 ### Side-effects
 
 #### Setup
+	// Persisting setup::compute() requires verify_not_signed first
 	// Side-effects:
+	//  - contract_utils::verify_not_signed -> tx_log_iter
 	//  - height = w.w2n_client().get_chain_tip()?.0;
 	//  - maybe_context = w.get_private_context(keychain_mask, sl.id.as_bytes())
 	//  - create_contract_ctx -> updater::refresh_outputs(wallet, keychain_mask, parent_key_id, false)?;

libwallet/src/contract/actions/new.rs

@@ -39,17 +39,10 @@ where
 	K: Keychain,
 {
 	// Compute state for 'new'
-	let (slate, mut context) = compute(w, keychain_mask, setup_args, slate_id)?;
+	let (slate, context) = compute(w, keychain_mask, setup_args, slate_id)?;
 
 	// Atomically commit state
-	contract::utils::save_step(
+	contract::utils::save_step(w, keychain_mask, &slate, context, setup_args.add_outputs)?;
-		w,
-		keychain_mask,
-		&slate,
-		&mut context,
-		setup_args.add_outputs,
-		false,
-	)?;
 
 	Ok(slate)
 }

libwallet/src/contract/actions/setup.rs

@@ -35,18 +35,12 @@ where
 	C: NodeClient,
 	K: Keychain,
 {
+	contract::utils::verify_not_signed(w, slate.id)?;
 	// Compute state for 'setup'
-	let (slate, mut context) = compute(w, keychain_mask, slate, setup_args)?;
+	let (slate, context) = compute(w, keychain_mask, slate, setup_args)?;
 
 	// Atomically commit state
-	contract::utils::save_step(
+	contract::utils::save_step(w, keychain_mask, &slate, context, setup_args.add_outputs)?;
-		w,
-		keychain_mask,
-		&slate,
-		&mut context,
-		setup_args.add_outputs,
-		false,
-	)?;
 
 	Ok(slate)
 }

libwallet/src/contract/actions/sign.rs

@@ -70,7 +70,7 @@ where
 		.map(|context| !context.has_inputs_or_outputs())
 		.unwrap_or(true);
 	// Compute state for 'sign'
-	let (sl, mut context) = compute(
+	let (sl, context) = compute(
 		w,
 		keychain_mask,
 		slate,
@@ -79,7 +79,7 @@ where
 	)?;
 
 	// Atomically commit state
-	contract::utils::save_step(w, keychain_mask, &sl, &mut context, will_add_outputs, true)?;
+	contract::utils::save_step(w, keychain_mask, &sl, context, will_add_outputs)?;
 
 	Ok(sl)
 }

libwallet/src/contract/slate.rs

@@ -465,9 +465,8 @@ pub(super) fn transition_state(slate: &mut Slate) -> Result<(), Error> {
 
 /// Add partial signature to the slate.
 // Nonce reuse is prevented by forgetting the signing context after a signed step:
-// save_step deletes the private context (which holds sec_key/sec_nonce) once is_signed
+// save_step deletes sec_key/sec_nonce after signing, except for step2 contract view
-// (except the deliberately-retained step2 context used for safe cancel). The context is
+// Contexts are keyed by slate.id so a nonce is only used for one message
-// keyed by slate.id, so a given nonce is only ever used for one message.
 pub fn sign<C, K>(
 	w: &mut WalletBackend<C, K>,
 	keychain_mask: Option<&SecretKey>,
@@ -490,6 +489,8 @@ where
 		)));
 	}
 	let keychain = w.keychain(keychain_mask)?;
+	// Do not let fill_round_2 skip a missing participant
+	slate.find_index_matching_context(&keychain, context)?;
 	slate.fill_round_2(&keychain, &context.sec_key, &context.sec_nonce)?;
 	debug!(
 		"contract::sign => signed for slate fees: {}",

libwallet/src/contract/utils.rs

@@ -60,12 +60,14 @@ pub fn create_tx_log_entry(
 	Ok(t)
 }
 
-/// Updates TxLogEntry for a contract with information available in the 'sign' step
+/// Update TxLogEntry with data from the sign step
+/// `participant_index` is our entry in `slate.participant_data`
 pub fn update_tx_log_entry<C, K>(
 	wallet: &mut WalletBackend<C, K>,
-	keychain_mask: Option<&SecretKey>,
+	keychain: &K,
 	slate: &Slate,
 	context: &Context,
+	participant_index: usize,
 	tx_log_entry: &mut TxLogEntry,
 ) -> Result<(), Error>
 where
@@ -73,7 +75,6 @@ where
 	K: Keychain,
 {
 	// This is expected to be called when we are signing the contract and have already contributed inputs & outputs
-	let keychain = wallet.keychain(keychain_mask)?;
 	let parent_key_id = context.parent_key_id.clone();
 	let current_height = wallet.w2n_client().get_chain_tip()?.0;
 	// We have already contributed inputs and outputs so we know how much of each we contribute
@@ -93,7 +94,7 @@ where
 			// note we only use a single path for now
 			let sender_address_path = 0u32;
 			let sender_key = address::address_from_derivation_path(
-				&keychain,
+				keychain,
 				&parent_key_id,
 				sender_address_path,
 			)?;
@@ -102,14 +103,20 @@ where
 			// We're looking for the OTHER party here, the recipient. The 'xor 1' pairing
 			// only holds for a two-party slate, so check that before indexing: a slate
 			// with a shorter participant list would otherwise panic here.
-			let sender_index = slate.find_index_matching_context(&keychain, context)?;
 			if slate.participant_data.len() != 2 {
 				return Err(Error::GenericError(format!(
 					"Expected 2 participants for a payment proof, found {}",
 					slate.participant_data.len()
 				)));
 			}
-			let recipient_index = sender_index ^ 1;
+			let sender = slate
+				.participant_data
+				.get(participant_index)
+				.ok_or(Error::ContextToIndex)?;
+			let recipient = slate
+				.participant_data
+				.get(participant_index ^ 1)
+				.ok_or(Error::ContextToIndex)?;
 
 			tx_log_entry.payment_proof = Some(StoredProofInfo {
 				receiver_address: p.receiver_address,
@@ -120,14 +127,12 @@ where
 				// Filled as separate steps for now; could be merged into a general case
 				// once we know which nonces here belong to the recipient.
 				proof_type: Some(p.proof_type.as_u8()),
-				receiver_public_nonce: Some(slate.participant_data[recipient_index].public_nonce),
+				receiver_public_nonce: Some(recipient.public_nonce),
-				receiver_public_excess: Some(
+				receiver_public_excess: Some(recipient.public_blind_excess),
-					slate.participant_data[recipient_index].public_blind_excess,
-				),
 				timestamp: Some(timestamp),
 				memo: p.memo.clone(),
 				promise_signature: p.promise_signature,
-				sender_part_sig: slate.participant_data[sender_index].part_sig,
+				sender_part_sig: sender.part_sig,
 			});
 		}
 	}
@@ -178,15 +183,15 @@ pub fn get_net_change(
 	Ok(expected_net_change.unwrap())
 }
 
-/// Atomically locks the inputs and saves the changes of Context, TxLogEntry and OutputData.
+/// Lock inputs and store the Context, TxLogEntry and OutputData atomically
-/// Additionally, the transaction is saved in a file in case we signed it.
+/// Consumes the context and derives the signed state from our participant data
+/// Signed transactions are written outside the database batch
 pub fn save_step<C, K>(
 	w: &mut WalletBackend<C, K>,
 	keychain_mask: Option<&SecretKey>,
 	slate: &Slate,
-	context: &mut Context,
+	mut context: Context,
 	step_added_outputs: bool,
-	is_signed: bool,
 ) -> Result<(), Error>
 where
 	C: NodeClient,
@@ -198,9 +203,16 @@ where
 	);
 	// Phase 1 - precompute the data needed for atomic update
 	let parent_key_id = &context.parent_key_id;
+	let keychain = w.keychain(keychain_mask)?;
+	let participant_index = slate.find_index_matching_context(&keychain, &context)?;
+	let is_signed = slate.participant_data[participant_index].is_complete();
+	if is_signed {
+		// Verify part_sig before using it as signed state
+		slate.verify_part_sigs(keychain.secp())?;
+	}
 	let current_height = w.w2n_client().get_chain_tip()?.0;
 	// We are at step2 if we don't have context.log_id and we have signed the slate
-	let is_step2 = !context.log_id.is_some() && is_signed;
+	let is_step2 = context.log_id.is_none() && is_signed;
 
 	let mut tx_log_entry = {
 		if context.log_id.is_none() {
@@ -215,7 +227,14 @@ where
 
 	// Update TxLogEntry if we have signed the contract (we have data about the kernel)
 	if is_signed {
-		update_tx_log_entry(w, keychain_mask, &slate, &context, &mut tx_log_entry)?;
+		update_tx_log_entry(
+			w,
+			&keychain,
+			slate,
+			&context,
+			participant_index,
+			&mut tx_log_entry,
+		)?;
 		// Record where the transaction is stored, as internal::selection does for a
 		// standard send. store_tx below writes it under this name, and 'txs' reads the
 		// field to report whether the transaction data is held.
@@ -252,7 +271,7 @@ where
 	let mut batch = w.batch(keychain_mask)?;
 
 	// Update TxLogEntry
-	if !context.log_id.is_some() {
+	if context.log_id.is_none() {
 		// If we just created the TxLogEntry, we have to assign it an id
 		let log_id = batch.next_tx_log_id(&parent_key_id)?;
 		tx_log_entry.id = log_id;
@@ -279,8 +298,7 @@ where
 	if is_signed && !is_step2 {
 		// NOTE: We MUST forget the context when we sign. Ideally, these two would be atomic or perhaps
 		// when we call slate::sigadd_partial_signaturen we could swap the secret key with a temporary one just to be safe.
-		// The reason we don't delete if we are at step2 is because in case we want to do safe cancel,
+		// Keep the step2 context for contract view
-		// we need to know which inputs are in the context to know which input we have to double-spend.
 		batch.delete_private_context(slate.id.as_bytes())?;
 	} else {
 		batch.save_private_context(slate.id.as_bytes(), &context)?;
@@ -288,10 +306,7 @@ where
 
 	batch.commit()?;
 
-	// Defense in depth: once we have signed (and are past the step2 context that is kept
+	// Confirm sec_key/sec_nonce are gone after signing, except for step2
-	// deliberately for safe cancel), the signing context holding sec_key/sec_nonce must be
-	// gone so the nonce can never be reused. It is deleted in the is_signed && !is_step2
-	// branch above; verify the deletion actually took effect.
 	if is_signed && !is_step2 {
 		match w.get_private_context(keychain_mask, slate.id.as_bytes()) {
 			Err(Error::NotFoundErr(_)) => {}

libwallet/src/slate.rs

@@ -704,7 +704,7 @@ impl Slate {
 	}
 
 	/// Verifies all of the partial signatures in the Slate are valid
-	fn verify_part_sigs(&self, secp: &secp::Secp256k1) -> Result<(), Error> {
+	pub(crate) fn verify_part_sigs(&self, secp: &secp::Secp256k1) -> Result<(), Error> {
 		// collect public nonces
 		for p in self.participant_data.iter() {
 			if p.is_complete() {