Contracts: cancel every tx log entry for a slate in one batch

cancel_tx committed each entry separately, so a failure part way through
left some entries cancelled and the rest untouched, and a retry then
stopped on the entry that was already cancelled. Collect the entries and
their outputs first and cancel them in a single commit, so the partial
state cannot arise.

Cancelling an already cancelled transaction still reports an error, as
db_wallet_tx_rollback expects.

libwallet/src/contract/slate.rs

@@ -296,7 +296,7 @@ where
 #[cfg(test)]
 mod tests {
 	use super::*;
-	use crate::grin_keychain::{ExtKeychain, Keychain as _, SwitchCommitmentType};
+	use crate::grin_keychain::{ExtKeychain, SwitchCommitmentType};
 	use crate::slate::ParticipantData;
 
 	fn keychain_and_context() -> (ExtKeychain, Context) {

libwallet/src/internal/tx.rs

@@ -354,6 +354,9 @@ where
 	if tx_vec.len() == 0 {
 		return Err(Error::TransactionDoesntExist(tx_id_string));
 	}
+	// Collect the entries and their outputs first: the batch that cancels them borrows the
+	// wallet, and a slate can have several entries which must all be cancelled together.
+	let mut to_cancel = vec![];
 	for tx in tx_vec {
 		debug!("cancel_tx: tx: {}", tx.tx_type);
 		match tx.tx_type {
@@ -375,9 +378,9 @@ where
 			Some(&parent_key_id),
 		)?;
 		let outputs = res.iter().map(|m| m.output.clone()).collect();
-		updater::cancel_tx_and_outputs(wallet, keychain_mask, tx, outputs, parent_key_id)?;
+		to_cancel.push((tx, outputs));
 	}
-	Ok(())
+	updater::cancel_txs_and_outputs(wallet, keychain_mask, to_cancel, parent_key_id)
 }
 
 /// Update the stored transaction (this update needs to happen when the TX is finalised)

libwallet/src/internal/updater.rs

@@ -484,38 +484,42 @@ where
 }
 
 /// Cancel transaction and associated outputs
-pub fn cancel_tx_and_outputs<C, K>(
+pub fn cancel_txs_and_outputs<C, K>(
 	wallet: &mut WalletBackend<C, K>,
 	keychain_mask: Option<&SecretKey>,
-	mut tx: TxLogEntry,
-	outputs: Vec<OutputData>,
+	txs: Vec<(TxLogEntry, Vec<OutputData>)>,
 	parent_key_id: &Identifier,
 ) -> Result<(), Error>
 where
 	C: NodeClient,
 	K: Keychain,
 {
+	// A single slate can have several tx log entries, so they are all cancelled in one
+	// batch: committing per entry would let a failure part way through leave some of them
+	// cancelled and the rest untouched.
 	let mut batch = wallet.batch(keychain_mask)?;
 
-	for mut o in outputs {
-		// unlock locked outputs
-		if o.status == OutputStatus::Unconfirmed || o.status == OutputStatus::Reverted {
-			batch.delete(&o.key_id, &o.mmr_index)?;
+	for (mut tx, outputs) in txs {
+		for mut o in outputs {
+			// unlock locked outputs
+			if o.status == OutputStatus::Unconfirmed || o.status == OutputStatus::Reverted {
+				batch.delete(&o.key_id, &o.mmr_index)?;
+			}
+			if o.status == OutputStatus::Locked {
+				o.status = OutputStatus::Unspent;
+				batch.save(o)?;
+			}
 		}
-		if o.status == OutputStatus::Locked {
-			o.status = OutputStatus::Unspent;
-			batch.save(o)?;
+		match tx.tx_type {
+			TxLogEntryType::TxSent => tx.tx_type = TxLogEntryType::TxSentCancelled,
+			TxLogEntryType::TxReceived | TxLogEntryType::TxReverted => {
+				tx.tx_type = TxLogEntryType::TxReceivedCancelled
+			}
+			TxLogEntryType::TxSelfSpend => tx.tx_type = TxLogEntryType::TxSelfSpendCancelled,
+			_ => {}
 		}
+		batch.save_tx_log_entry(tx, parent_key_id)?;
 	}
-	match tx.tx_type {
-		TxLogEntryType::TxSent => tx.tx_type = TxLogEntryType::TxSentCancelled,
-		TxLogEntryType::TxReceived | TxLogEntryType::TxReverted => {
-			tx.tx_type = TxLogEntryType::TxReceivedCancelled
-		}
-		TxLogEntryType::TxSelfSpend => tx.tx_type = TxLogEntryType::TxSelfSpendCancelled,
-		_ => {}
-	}
-	batch.save_tx_log_entry(tx, parent_key_id)?;
 	batch.commit()?;
 	Ok(())
 }