Contracts: document the stored tx window and record the tx location

save_step writes the signed transaction after the wallet state batch has
committed, so the write can fail with the tx log entry and the input
locks already persisted. store_tx writes a file outside LMDB, as it does
for every transaction in the wallet, so the two cannot share a commit
without moving stored transactions into the database, which backend.rs
already carries a TODO for. Cancelling is the way out: it releases the
inputs without reading the stored tx, and nothing has been broadcast
because the error returns before the caller posts. Tested by removing
the file and cancelling.

Also set stored_tx on the entry, as internal::selection does for a
standard send. It was left unset, so txs reported no transaction data
for every contract even though the transaction was stored.

controller/tests/contract_self_spend_cancel.rs

@@ -97,6 +97,130 @@ fn contract_self_spend_cancel_impl(test_dir: &'static str) -> Result<(), libwall
 	Ok(())
 }
 
+/// Find the stored transaction a signed contract wrote
+fn find_stored_tx(test_dir: &str) -> std::path::PathBuf {
+	fn walk(dir: &std::path::Path, out: &mut Vec<std::path::PathBuf>) {
+		if let Ok(entries) = std::fs::read_dir(dir) {
+			for e in entries.filter_map(|e| e.ok()) {
+				let p = e.path();
+				if p.is_dir() {
+					walk(&p, out);
+				} else if p.extension().map(|x| x == "grintx").unwrap_or(false) {
+					out.push(p);
+				}
+			}
+		}
+	}
+	let mut found = vec![];
+	walk(std::path::Path::new(test_dir), &mut found);
+	assert_eq!(found.len(), 1, "expected one stored tx under {}", test_dir);
+	found.pop().unwrap()
+}
+
+/// save_step writes the signed transaction to a file after the wallet state batch has
+/// committed, so that write can fail with the tx log entry and the input locks already
+/// persisted. The documented way out is to cancel, which does not read the stored tx;
+/// this checks that holds by removing the file and cancelling.
+fn contract_self_spend_cancel_missing_stored_tx_impl(
+	test_dir: &'static str,
+) -> Result<(), libwallet::Error> {
+	let (wallets, _chain, stopper, _bh) =
+		create_wallets(vec![vec![("default", 4)]], test_dir).unwrap();
+	let send_wallet = wallets[0].0.clone();
+	let send_mask = wallets[0].1.as_ref();
+
+	let mut slate = Slate::blank(0, true);
+	wallet::controller::owner_single_use(
+		send_wallet.clone(),
+		send_mask,
+		PathBuf::from(test_dir),
+		|api, m| {
+			let args = &ContractNewArgsAPI {
+				setup_args: ContractSetupArgsAPI {
+					net_change: Some(0),
+					num_participants: 1,
+					..Default::default()
+				},
+				..Default::default()
+			};
+			slate = api.contract_new(m, args)?;
+			Ok(())
+		},
+	)?;
+
+	// Signing a self-spend completes the slate, so the transaction is stored
+	wallet::controller::owner_single_use(
+		send_wallet.clone(),
+		send_mask,
+		PathBuf::from(test_dir),
+		|api, m| {
+			let args = &ContractSetupArgsAPI {
+				..Default::default()
+			};
+			slate = api.contract_sign(m, &slate, args)?;
+			Ok(())
+		},
+	)?;
+	assert_eq!(slate.state, SlateState::Standard2);
+
+	// Signing locked our inputs, so the cancel below has something to release, and the
+	// entry records where the transaction was stored, as a standard send does
+	wallet::controller::owner_single_use(
+		send_wallet.clone(),
+		send_mask,
+		PathBuf::from(test_dir),
+		|api, m| {
+			let (_, info) = api.retrieve_summary_info(m, true, 1)?;
+			assert!(info.amount_locked > 0);
+			let (_, txs) = api.retrieve_txs(m, true, None, Some(slate.id), None)?;
+			assert_eq!(
+				txs.last().unwrap().stored_tx,
+				Some(format!("{}.grintx", slate.id))
+			);
+			Ok(())
+		},
+	)?;
+
+	// Stand in for the file write having failed. The transaction has not been posted:
+	// contract_sign returns before the caller broadcasts.
+	let stored = find_stored_tx(test_dir);
+	std::fs::remove_file(&stored).unwrap();
+
+	// Cancelling still releases the inputs
+	wallet::controller::owner_single_use(
+		send_wallet.clone(),
+		send_mask,
+		PathBuf::from(test_dir),
+		|api, m| {
+			api.cancel_tx(m, None, Some(slate.id))?;
+			Ok(())
+		},
+	)?;
+
+	wallet::controller::owner_single_use(
+		send_wallet.clone(),
+		send_mask,
+		PathBuf::from(test_dir),
+		|api, m| {
+			let query_args = libwallet::RetrieveTxQueryArgs {
+				exclude_cancelled: Some(false),
+				..Default::default()
+			};
+			let (_, txs) = api.retrieve_txs(m, true, None, None, Some(query_args))?;
+			let tx_log = txs.last().unwrap();
+			assert_eq!(tx_log.tx_type, TxLogEntryType::TxSelfSpendCancelled);
+			// The spendable balance is back to the mined total
+			let (_, info) = api.retrieve_summary_info(m, true, 1)?;
+			assert_eq!(info.amount_locked, 0);
+			Ok(())
+		},
+	)?;
+
+	stopper.store(false, Ordering::Relaxed);
+	thread::sleep(Duration::from_millis(200));
+	Ok(())
+}
+
 #[test]
 fn wallet_contract_self_spend_cancel() -> Result<(), libwallet::Error> {
 	let test_dir = "test_output/contract_self_spend_cancel";
@@ -105,3 +229,12 @@ fn wallet_contract_self_spend_cancel() -> Result<(), libwallet::Error> {
 	clean_output_dir(test_dir);
 	Ok(())
 }
+
+#[test]
+fn wallet_contract_self_spend_cancel_missing_stored_tx() -> Result<(), libwallet::Error> {
+	let test_dir = "test_output/contract_self_spend_cancel_missing_stored_tx";
+	setup(test_dir);
+	contract_self_spend_cancel_missing_stored_tx_impl(test_dir)?;
+	clean_output_dir(test_dir);
+	Ok(())
+}

libwallet/src/contract/utils.rs

@@ -44,8 +44,7 @@ pub fn create_tx_log_entry(
 		}
 	};
 	let mut t = TxLogEntry::new(parent_key_id.clone(), log_type, log_id);
-	// stored_tx on the entry is left unset; contracts persist the tx separately via
-	// store_tx in save_step.
+	// stored_tx is set in save_step, once we have signed and the transaction is written.
 
 	t.tx_slate_id = Some(slate.id);
 	if net_change > 0 {
@@ -223,6 +222,10 @@ 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)?;
+		// 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.
+		tx_log_entry.stored_tx = Some(format!("{}.grintx", slate.id));
 	}
 	// If we added outputs in this step, we have to create OutputData here because 'batch'
 	// takes the mutable ref and we can no longer call calc_commit_for_cache for output
@@ -308,6 +311,15 @@ where
 	// Store the signed transaction only after the wallet-state batch has committed (and
 	// the signing context is confirmed gone), so a DB failure can't leave the stored tx
 	// out of sync with wallet state. Matches the core convention (internal::selection).
+	//
+	// store_tx writes a file outside LMDB, so this cannot be part of the batch above and
+	// a window remains: the write can fail with the tx log entry and the input locks
+	// already committed. Making it atomic would mean holding stored transactions in the
+	// database, which is how every transaction in the wallet is kept, not just contracts.
+	// Until then the way out is to cancel the transaction, which releases the inputs
+	// without reading the stored tx. Nothing has been broadcast at this point: the error
+	// returns to the caller before it posts. Covered by
+	// wallet_contract_self_spend_cancel_missing_stored_tx.
 	if is_signed {
 		w.store_tx(&format!("{}", slate.id), slate.tx_or_err()?)?;
 	}