Claim HTLC output on-chain if preimage is recv'd after force-close

If we receive a preimage for an outgoing HTLC that solves an output on a
backwards force-closed channel, we need to claim the output on-chain.

Note that this commit also gets rid of the channel monitor redundantly setting
`self.counterparty_payment_script` in `check_spend_counterparty_transaction`.

Co-authored-by: Antoine Riard <[email protected]>
Co-authored-by: Valentine Wallace <[email protected]>

Author: valentinewallace

lightning/src/chain/channelmonitor.rs

@@ -1165,6 +1165,41 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 		    L::Target: Logger,
 	{
 		self.payment_preimages.insert(payment_hash.clone(), payment_preimage.clone());
+
+		// If the channel is force closed, try to claim the output from this preimage.
+		// First check if a counterparty commitment transaction has been broadcasted:
+		macro_rules! claim_htlcs {
+			($commitment_number: expr, $txid: expr) => {
+				let htlc_claim_reqs = self.get_counterparty_htlc_output_claim_reqs($commitment_number, $txid, None);
+				self.onchain_tx_handler.update_claims_view(&Vec::new(), htlc_claim_reqs, None, broadcaster, fee_estimator, logger);
+			}
+		}
+		if let Some(txid) = self.current_counterparty_commitment_txid {
+			if let Some(commitment_number) = self.counterparty_commitment_txn_on_chain.get(&txid) {
+				claim_htlcs!(*commitment_number, txid);
+				return;
+			}
+		}
+		if let Some(txid) = self.prev_counterparty_commitment_txid {
+			if let Some(commitment_number) = self.counterparty_commitment_txn_on_chain.get(&txid) {
+				claim_htlcs!(*commitment_number, txid);
+				return;
+			}
+		}
+
+		// Then if a holder commitment transaction has been seen on-chain, broadcast transactions
+		// claiming the HTLC output from each of the holder commitment transactions.
+		// Note that we can't just use `self.holder_tx_signed`, because that only covers the case where
+		// *we* sign a holder commitment transaction, not when e.g. a watchtower broadcasts one of our
+		// holder commitment transactions.
+		if self.broadcasted_holder_revokable_script.is_some() {
+			let (claim_reqs, _) = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx);
+			self.onchain_tx_handler.update_claims_view(&Vec::new(), claim_reqs, None, broadcaster, fee_estimator, logger);
+			if let Some(ref tx) = self.prev_holder_signed_commitment_tx {
+				let (claim_reqs, _) = self.get_broadcasted_holder_claims(&tx);
+				self.onchain_tx_handler.update_claims_view(&Vec::new(), claim_reqs, None, broadcaster, fee_estimator, logger);
+			}
+		}
 	}
 
 	pub(crate) fn broadcast_latest_holder_commitment_txn<B: Deref, L: Deref>(&mut self, broadcaster: &B, logger: &L)
@@ -1463,39 +1498,55 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 				check_htlc_fails!(txid, "previous", 'prev_loop);
 			}
 
+			let htlc_claim_reqs = self.get_counterparty_htlc_output_claim_reqs(commitment_number, commitment_txid, Some(tx));
+			for req in htlc_claim_reqs {
+				claimable_outpoints.push(req);
+			}
+
+		}
+		(claimable_outpoints, (commitment_txid, watch_outputs))
+	}
+
+	fn get_counterparty_htlc_output_claim_reqs(&self, commitment_number: u64, commitment_txid: Txid, tx: Option<&Transaction>) -> Vec<ClaimRequest> {
+		let mut claims = Vec::new();
+		if let Some(htlc_outputs) = self.counterparty_claimable_outpoints.get(&commitment_txid) {
 			if let Some(revocation_points) = self.their_cur_revocation_points {
 				let revocation_point_option =
+					// If the counterparty commitment tx is the latest valid state, use their latest
+					// per-commitment point
 					if revocation_points.0 == commitment_number { Some(&revocation_points.1) }
 					else if let Some(point) = revocation_points.2.as_ref() {
+						// If counterparty commitment tx is the state previous to the latest valid state, use
+						// their previous per-commitment point (non-atomicity of revocation means it's valid for
+						// them to temporarily have two valid commitment txns from our viewpoint)
 						if revocation_points.0 == commitment_number + 1 { Some(point) } else { None }
 					} else { None };
 				if let Some(revocation_point) = revocation_point_option {
-					self.counterparty_payment_script = {
-						// Note that the Network here is ignored as we immediately drop the address for the
-						// script_pubkey version
-						let payment_hash160 = WPubkeyHash::hash(&self.keys.pubkeys().payment_point.serialize());
-						Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0).push_slice(&payment_hash160[..]).into_script()
-					};
-
-					// Then, try to find htlc outputs
-					for (_, &(ref htlc, _)) in per_commitment_data.iter().enumerate() {
+					for (_, &(ref htlc, _)) in htlc_outputs.iter().enumerate() {
 						if let Some(transaction_output_index) = htlc.transaction_output_index {
-							if transaction_output_index as usize >= tx.output.len() ||
-									tx.output[transaction_output_index as usize].value != htlc.amount_msat / 1000 {
-								return (claimable_outpoints, (commitment_txid, watch_outputs)); // Corrupted per_commitment_data, fuck this user
+							if let Some(transaction) = tx {
+								if transaction_output_index as usize >= transaction.output.len() ||
+									transaction.output[transaction_output_index as usize].value != htlc.amount_msat / 1000 {
+										return claims; // Corrupted per_commitment_data, fuck this user
+									}
 							}
-							let preimage = if htlc.offered { if let Some(p) = self.payment_preimages.get(&htlc.payment_hash) { Some(*p) } else { None } } else { None };
+							let preimage =
+								if htlc.offered {
+									if let Some(p) = self.payment_preimages.get(&htlc.payment_hash) {
+										Some(*p)
+									} else { None }
+								} else { None };
 							let aggregable = if !htlc.offered { false } else { true };
 							if preimage.is_some() || !htlc.offered {
 								let witness_data = InputMaterial::CounterpartyHTLC { per_commitment_point: *revocation_point, counterparty_delayed_payment_base_key: self.counterparty_tx_cache.counterparty_delayed_payment_base_key, counterparty_htlc_base_key: self.counterparty_tx_cache.counterparty_htlc_base_key, preimage, htlc: htlc.clone() };
-								claimable_outpoints.push(ClaimRequest { absolute_timelock: htlc.cltv_expiry, aggregable, outpoint: BitcoinOutPoint { txid: commitment_txid, vout: transaction_output_index }, witness_data });
+								claims.push(ClaimRequest { absolute_timelock: htlc.cltv_expiry, aggregable, outpoint: BitcoinOutPoint { txid: commitment_txid, vout: transaction_output_index }, witness_data });
 							}
 						}
 					}
 				}
 			}
 		}
-		(claimable_outpoints, (commitment_txid, watch_outputs))
+		claims
 	}
 
 	/// Attempts to claim a counterparty HTLC-Success/HTLC-Timeout's outputs using the revocation key
@@ -1816,7 +1867,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 			}
 		}
 
-		self.onchain_tx_handler.block_connected(&txn_matched, claimable_outpoints, height, &*broadcaster, &*fee_estimator, &*logger);
+		self.onchain_tx_handler.update_claims_view(&txn_matched, claimable_outpoints, Some(height), &&*broadcaster, &&*fee_estimator, &&*logger);
 		self.last_block_hash = block_hash;
 
 		// Determine new outputs to watch by comparing against previously known outputs to watch,

lightning/src/ln/functional_tests.rs

@@ -8503,3 +8503,187 @@ fn test_htlc_no_detection() {
         connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1, 201, true, header_201.block_hash());
         expect_payment_failed!(nodes[0], our_payment_hash, true);
 }
+
+fn do_test_onchain_htlc_settlement_after_close(broadcast_alice: bool, go_onchain_before_fulfill: bool) {
+	// If we route an HTLC, then learn the HTLC's preimage after the upstream channel has been
+	// force-closed, we must claim that HTLC on-chain. (Given an HTLC forwarded from Alice --> Bob -->
+	// Carol, Alice would be the upstream node, and Carol the downstream.)
+	//
+	// Steps of the test:
+	// 1) Alice sends a HTLC to Carol through Bob.
+	// 2) Carol doesn't settle the HTLC.
+	// 3) If broadcast_alice is true, Alice force-closes her channel with Bob. Else Bob force closes.
+	// Steps 4 and 5 may be reordered depending on go_onchain_before_fulfill.
+	// 4) Bob sees the Alice's commitment on his chain or vice versa. An offered output is present
+	//    but can't be claimed as Bob doesn't have yet knowledge of the preimage.
+	// 5) Carol release the preimage to Bob off-chain.
+	// 6) Bob claims the offered output on the broadcasted commitment.
+	let chanmon_cfgs = create_chanmon_cfgs(3);
+	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
+	let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+
+	// Create some initial channels
+	let chan_ab = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 10001, InitFeatures::known(), InitFeatures::known());
+	create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 100000, 10001, InitFeatures::known(), InitFeatures::known());
+
+	// Steps (1) and (2):
+	// Send an HTLC Alice --> Bob --> Carol, but Carol doesn't settle the HTLC back.
+	let (payment_preimage, _payment_hash) = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2]), 3_000_000);
+
+	// Check that Alice's commitment transaction now contains an output for this HTLC.
+	let alice_txn = get_local_commitment_txn!(nodes[0], chan_ab.2);
+	check_spends!(alice_txn[0], chan_ab.3);
+	assert_eq!(alice_txn[0].output.len(), 2);
+	check_spends!(alice_txn[1], alice_txn[0]); // 2nd transaction is a non-final HTLC-timeout
+	assert_eq!(alice_txn[1].input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
+	assert_eq!(alice_txn.len(), 2);
+
+	// Steps (3) and (4):
+	// If `go_onchain_before_fufill`, broadcast the relevant commitment transaction and check that Bob
+	// responds by (1) broadcasting a channel update and (2) adding a new ChannelMonitor.
+	let mut force_closing_node = 0; // Alice force-closes
+	if !broadcast_alice { force_closing_node = 1; } // Bob force-closes
+	nodes[force_closing_node].node.force_close_channel(&chan_ab.2);
+	check_closed_broadcast!(nodes[force_closing_node], false);
+	check_added_monitors!(nodes[force_closing_node], 1);
+	if go_onchain_before_fulfill {
+		let txn_to_broadcast = match broadcast_alice {
+			true => alice_txn.clone(),
+			false => get_local_commitment_txn!(nodes[1], chan_ab.2)
+		};
+		let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42};
+		connect_block(&nodes[1], &Block { header, txdata: vec![txn_to_broadcast[0].clone()]}, 1);
+		let mut bob_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
+		if broadcast_alice {
+			check_closed_broadcast!(nodes[1], false);
+			check_added_monitors!(nodes[1], 1);
+		}
+		assert_eq!(bob_txn.len(), 1);
+		check_spends!(bob_txn[0], chan_ab.3);
+	}
+
+	// Step (5):
+	// Carol then claims the funds and sends an update_fulfill message to Bob, and they go through the
+	// process of removing the HTLC from their commitment transactions.
+	assert!(nodes[2].node.claim_funds(payment_preimage, &None, 3_000_000));
+	check_added_monitors!(nodes[2], 1);
+	let carol_updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
+	assert!(carol_updates.update_add_htlcs.is_empty());
+	assert!(carol_updates.update_fail_htlcs.is_empty());
+	assert!(carol_updates.update_fail_malformed_htlcs.is_empty());
+	assert!(carol_updates.update_fee.is_none());
+	assert_eq!(carol_updates.update_fulfill_htlcs.len(), 1);
+
+	nodes[1].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &carol_updates.update_fulfill_htlcs[0]);
+	// If Alice broadcasted but Bob doesn't know yet, here he prepares to tell her about the preimage.
+	if !go_onchain_before_fulfill && broadcast_alice {
+		let events = nodes[1].node.get_and_clear_pending_msg_events();
+		assert_eq!(events.len(), 1);
+		match events[0] {
+			MessageSendEvent::UpdateHTLCs { ref node_id, .. } => {
+				assert_eq!(*node_id, nodes[0].node.get_our_node_id());
+			},
+			_ => panic!("Unexpected event"),
+		};
+	}
+	nodes[1].node.handle_commitment_signed(&nodes[2].node.get_our_node_id(), &carol_updates.commitment_signed);
+	// One monitor update for the preimage to update the Bob<->Alice channel, one monitor update
+	// Carol<->Bob's updated commitment transaction info.
+	check_added_monitors!(nodes[1], 2);
+
+	let events = nodes[1].node.get_and_clear_pending_msg_events();
+	assert_eq!(events.len(), 2);
+	let bob_revocation = match events[0] {
+		MessageSendEvent::SendRevokeAndACK { ref node_id, ref msg } => {
+			assert_eq!(*node_id, nodes[2].node.get_our_node_id());
+			(*msg).clone()
+		},
+		_ => panic!("Unexpected event"),
+	};
+	let bob_updates = match events[1] {
+		MessageSendEvent::UpdateHTLCs { ref node_id, ref updates } => {
+			assert_eq!(*node_id, nodes[2].node.get_our_node_id());
+			(*updates).clone()
+		},
+		_ => panic!("Unexpected event"),
+	};
+
+	nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bob_revocation);
+	check_added_monitors!(nodes[2], 1);
+	nodes[2].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bob_updates.commitment_signed);
+	check_added_monitors!(nodes[2], 1);
+
+	let events = nodes[2].node.get_and_clear_pending_msg_events();
+	assert_eq!(events.len(), 1);
+	let carol_revocation = match events[0] {
+		MessageSendEvent::SendRevokeAndACK { ref node_id, ref msg } => {
+			assert_eq!(*node_id, nodes[1].node.get_our_node_id());
+			(*msg).clone()
+		},
+		_ => panic!("Unexpected event"),
+	};
+	nodes[1].node.handle_revoke_and_ack(&nodes[2].node.get_our_node_id(), &carol_revocation);
+	check_added_monitors!(nodes[1], 1);
+
+	// If this test requires the force-closed channel to not be on-chain until after the fulfill,
+	// here's where we put said channel's commitment tx on-chain.
+	let mut txn_to_broadcast = alice_txn.clone();
+	if !broadcast_alice { txn_to_broadcast = get_local_commitment_txn!(nodes[1], chan_ab.2); }
+	if !go_onchain_before_fulfill {
+		let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42};
+		connect_block(&nodes[1], &Block { header, txdata: vec![txn_to_broadcast[0].clone()]}, 1);
+		// If Bob was the one to force-close, he will have already passed these checks earlier.
+		if broadcast_alice {
+			check_closed_broadcast!(nodes[1], false);
+			check_added_monitors!(nodes[1], 1);
+		}
+		let mut bob_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
+		if broadcast_alice {
+			// In `connect_block()`, the ChainMonitor and ChannelManager are separately notified about a
+			// new block being connected. The ChannelManager being notified triggers a monitor update,
+			// which triggers broadcasting our commitment tx and an HTLC-claiming tx. The ChainMonitor
+			// being notified triggers the HTLC-claiming tx redundantly, resulting in 3 total txs being
+			// broadcasted.
+			assert_eq!(bob_txn.len(), 3);
+			check_spends!(bob_txn[1], chan_ab.3);
+		} else {
+			assert_eq!(bob_txn.len(), 2);
+			check_spends!(bob_txn[0], chan_ab.3);
+		}
+	}
+
+	// Step (6):
+	// Finally, check that Bob broadcasted a preimage-claiming transaction for the HTLC output on the
+	// broadcasted commitment transaction.
+	{
+		let bob_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().clone();
+		if go_onchain_before_fulfill {
+			// Bob should now have an extra broadcasted tx, for the preimage-claiming transaction.
+			assert_eq!(bob_txn.len(), 2);
+		}
+		let script_weight = match broadcast_alice {
+			true => OFFERED_HTLC_SCRIPT_WEIGHT,
+			false => ACCEPTED_HTLC_SCRIPT_WEIGHT
+		};
+		// If Alice force-closed and Bob didn't receive her commitment transaction until after he
+		// received Carol's fulfill, he broadcasts the HTLC-output-claiming transaction first. Else if
+		// Bob force closed or if he found out about Alice's commitment tx before receiving Carol's
+		// fulfill, then he broadcasts the HTLC-output-claiming transaction second.
+		if broadcast_alice && !go_onchain_before_fulfill {
+			check_spends!(bob_txn[0], txn_to_broadcast[0]);
+			assert_eq!(bob_txn[0].input[0].witness.last().unwrap().len(), script_weight);
+		} else {
+			check_spends!(bob_txn[1], txn_to_broadcast[0]);
+			assert_eq!(bob_txn[1].input[0].witness.last().unwrap().len(), script_weight);
+		}
+	}
+}
+
+#[test]
+fn test_onchain_htlc_settlement_after_close() {
+	do_test_onchain_htlc_settlement_after_close(true, true);
+	do_test_onchain_htlc_settlement_after_close(false, true); // Technically redundant, but may as well
+	do_test_onchain_htlc_settlement_after_close(true, false);
+	do_test_onchain_htlc_settlement_after_close(false, false);
+}

lightning/src/ln/onchaintx.rs

@@ -282,6 +282,8 @@ pub struct OnchainTxHandler<ChanSigner: ChannelKeys> {
 
 	onchain_events_waiting_threshold_conf: HashMap<u32, Vec<OnchainEvent>>,
 
+	latest_height: u32,
+
 	secp_ctx: Secp256k1<secp256k1::All>,
 }
 
@@ -328,6 +330,7 @@ impl<ChanSigner: ChannelKeys + Writeable> OnchainTxHandler<ChanSigner> {
 				}
 			}
 		}
+		self.latest_height.write(writer)?;
 		Ok(())
 	}
 }
@@ -387,6 +390,7 @@ impl<ChanSigner: ChannelKeys + Readable> Readable for OnchainTxHandler<ChanSigne
 			}
 			onchain_events_waiting_threshold_conf.insert(height_target, events);
 		}
+		let latest_height = Readable::read(reader)?;
 
 		Ok(OnchainTxHandler {
 			destination_script,
@@ -399,6 +403,7 @@ impl<ChanSigner: ChannelKeys + Readable> Readable for OnchainTxHandler<ChanSigne
 			claimable_outpoints,
 			pending_claim_requests,
 			onchain_events_waiting_threshold_conf,
+			latest_height,
 			secp_ctx: Secp256k1::new(),
 		})
 	}
@@ -420,6 +425,7 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
 			pending_claim_requests: HashMap::new(),
 			claimable_outpoints: HashMap::new(),
 			onchain_events_waiting_threshold_conf: HashMap::new(),
+			latest_height: 0,
 
 			secp_ctx: Secp256k1::new(),
 		}
@@ -471,7 +477,7 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
 
 	/// Lightning security model (i.e being able to redeem/timeout HTLC or penalize coutnerparty onchain) lays on the assumption of claim transactions getting confirmed before timelock expiration
 	/// (CSV or CLTV following cases). In case of high-fee spikes, claim tx may stuck in the mempool, so you need to bump its feerate quickly using Replace-By-Fee or Child-Pay-For-Parent.
-	fn generate_claim_tx<F: Deref, L: Deref>(&mut self, height: u32, cached_claim_datas: &ClaimTxBumpMaterial, fee_estimator: F, logger: L) -> Option<(Option<u32>, u32, Transaction)>
+	fn generate_claim_tx<F: Deref, L: Deref>(&mut self, height: u32, cached_claim_datas: &ClaimTxBumpMaterial, fee_estimator: &F, logger: &L) -> Option<(Option<u32>, u32, Transaction)>
 		where F::Target: FeeEstimator,
 					L::Target: Logger,
 	{
@@ -657,12 +663,20 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
 		None
 	}
 
-	pub(crate) fn block_connected<B: Deref, F: Deref, L: Deref>(&mut self, txn_matched: &[&Transaction], claimable_outpoints: Vec<ClaimRequest>, height: u32, broadcaster: B, fee_estimator: F, logger: L)
+	/// Upon channelmonitor.block_connected(..) or upon provision of a preimage on the forward link
+	/// for this channel, provide new relevant on-chain transactions and/or new claim requests.
+	/// Formerly this was named `block_connected`, but it is now also used for claiming an HTLC output
+	/// if we receive a preimage after force-close.
+	pub(crate) fn update_claims_view<B: Deref, F: Deref, L: Deref>(&mut self, txn_matched: &[&Transaction], claimable_outpoints: Vec<ClaimRequest>, latest_height: Option<u32>, broadcaster: &B, fee_estimator: &F, logger: &L)
 		where B::Target: BroadcasterInterface,
 		      F::Target: FeeEstimator,
 					L::Target: Logger,
 	{
-		log_trace!(logger, "Block at height {} connected with {} claim requests", height, claimable_outpoints.len());
+		let height = match latest_height {
+			Some(h) => h,
+			None => self.latest_height,
+		};
+		log_trace!(logger, "Updating claims view at height {} with {} matched transactions and {} claim requests", height, txn_matched.len(), claimable_outpoints.len());
 		let mut new_claims = Vec::new();
 		let mut aggregated_claim = HashMap::new();
 		let mut aggregated_soonest = ::std::u32::MAX;
@@ -855,7 +869,7 @@ impl<ChanSigner: ChannelKeys> OnchainTxHandler<ChanSigner> {
 			}
 		}
 		for (_, claim_material) in bump_candidates.iter_mut() {
-			if let Some((new_timer, new_feerate, bump_tx)) = self.generate_claim_tx(height, &claim_material, &*fee_estimator, &*logger) {
+			if let Some((new_timer, new_feerate, bump_tx)) = self.generate_claim_tx(height, &claim_material, &&*fee_estimator, &&*logger) {
 				claim_material.height_timer = new_timer;
 				claim_material.feerate_previous = new_feerate;
 				broadcaster.broadcast_transaction(&bump_tx);