Merge pull request #546 from TheBlueMatt/2020-03-519-nits

Watch revoked HTLC-Success/Timeout outputs

Author: TheBlueMatt

lightning/src/ln/channelmonitor.rs

@@ -1668,22 +1668,22 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 	}
 
 	/// Attempts to claim a remote HTLC-Success/HTLC-Timeout's outputs using the revocation key
-	fn check_spend_remote_htlc(&mut self, tx: &Transaction, commitment_number: u64, height: u32) -> Vec<ClaimRequest> {
-		//TODO: send back new outputs to guarantee pending_claim_request consistency
+	fn check_spend_remote_htlc(&mut self, tx: &Transaction, commitment_number: u64, height: u32) -> (Vec<ClaimRequest>, Option<(Sha256dHash, Vec<TxOut>)>) {
+		let htlc_txid = tx.txid();
 		if tx.input.len() != 1 || tx.output.len() != 1 || tx.input[0].witness.len() != 5 {
-			return Vec::new()
+			return (Vec::new(), None)
 		}
 
 		macro_rules! ignore_error {
 			( $thing : expr ) => {
 				match $thing {
 					Ok(a) => a,
-					Err(_) => return Vec::new()
+					Err(_) => return (Vec::new(), None)
 				}
 			};
 		}
 
-		let secret = if let Some(secret) = self.get_secret(commitment_number) { secret } else { return Vec::new(); };
+		let secret = if let Some(secret) = self.get_secret(commitment_number) { secret } else { return (Vec::new(), None); };
 		let per_commitment_key = ignore_error!(SecretKey::from_slice(&secret));
 		let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key);
 		let (revocation_pubkey, revocation_key) = match self.key_storage {
@@ -1694,16 +1694,15 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 			Storage::Watchtower { .. } => { unimplemented!() }
 		};
 		let delayed_key = match self.their_delayed_payment_base_key {
-			None => return Vec::new(),
+			None => return (Vec::new(), None),
 			Some(their_delayed_payment_base_key) => ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, &per_commitment_point, &their_delayed_payment_base_key)),
 		};
 		let redeemscript = chan_utils::get_revokeable_redeemscript(&revocation_pubkey, self.our_to_self_delay, &delayed_key);
-		let htlc_txid = tx.txid(); //TODO: This is gonna be a performance bottleneck for watchtowers!
 
 		log_trace!(self, "Remote HTLC broadcast {}:{}", htlc_txid, 0);
 		let witness_data = InputMaterial::Revoked { witness_script: redeemscript, pubkey: Some(revocation_pubkey), key: revocation_key, is_htlc: false, amount: tx.output[0].value };
 		let claimable_outpoints = vec!(ClaimRequest { absolute_timelock: height + self.our_to_self_delay as u32, aggregable: true, outpoint: BitcoinOutPoint { txid: htlc_txid, vout: 0}, witness_data });
-		claimable_outpoints
+		(claimable_outpoints, Some((htlc_txid, tx.output.clone())))
 	}
 
 	fn broadcast_by_local_state(&self, local_tx: &LocalSignedTx, delayed_payment_base_key: &SecretKey) -> (Vec<Transaction>, Vec<SpendableOutputDescriptor>, Vec<TxOut>) {
@@ -2019,8 +2018,11 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 					}
 				} else {
 					if let Some(&(commitment_number, _)) = self.remote_commitment_txn_on_chain.get(&prevout.txid) {
-						let mut new_outpoints = self.check_spend_remote_htlc(&tx, commitment_number, height);
+						let (mut new_outpoints, new_outputs_option) = self.check_spend_remote_htlc(&tx, commitment_number, height);
 						claimable_outpoints.append(&mut new_outpoints);
+						if let Some(new_outputs) = new_outputs_option {
+							watch_outputs.push(new_outputs);
+						}
 					}
 				}
 			}

lightning/src/ln/functional_test_utils.rs

@@ -1098,9 +1098,9 @@ pub fn test_txn_broadcast<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, chan: &(msgs::Cha
 /// HTLC transaction.
 pub fn test_revoked_htlc_claim_txn_broadcast<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, revoked_tx: Transaction, commitment_revoked_tx: Transaction)  {
 	let mut node_txn = node.tx_broadcaster.txn_broadcasted.lock().unwrap();
-	// We should issue a 2nd transaction if one htlc is dropped from initial claiming tx
-	// but sometimes not as feerate is too-low
-	if node_txn.len() != 1 && node_txn.len() != 2 { assert!(false); }
+	// We may issue multiple claiming transaction on revoked outputs due to block rescan
+	// for revoked htlc outputs
+	if node_txn.len() != 1 && node_txn.len() != 2 && node_txn.len() != 3 { assert!(false); }
 	node_txn.retain(|tx| {
 		if tx.input.len() == 1 && tx.input[0].previous_output.txid == revoked_tx.txid() {
 			check_spends!(tx, revoked_tx);

lightning/src/ln/functional_tests.rs

@@ -2127,8 +2127,10 @@ fn test_justice_tx() {
 		test_txn_broadcast(&nodes[1], &chan_5, None, HTLCType::NONE);
 
 		nodes[0].block_notifier.block_connected(&Block { header, txdata: vec![revoked_local_txn[0].clone()] }, 1);
+		// Verify broadcast of revoked HTLC-timeout
 		let node_txn = test_txn_broadcast(&nodes[0], &chan_5, Some(revoked_local_txn[0].clone()), HTLCType::TIMEOUT);
 		header = BlockHeader { version: 0x20000000, prev_blockhash: header.bitcoin_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
+		// Broadcast revoked HTLC-timeout on node 1
 		nodes[1].block_notifier.block_connected(&Block { header, txdata: vec![node_txn[1].clone()] }, 1);
 		test_revoked_htlc_claim_txn_broadcast(&nodes[1], node_txn[1].clone(), revoked_local_txn[0].clone());
 	}
@@ -4182,9 +4184,14 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx() {
 	check_closed_broadcast!(nodes[1], false);
 
 	let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
-	assert_eq!(node_txn.len(), 4 ); // ChannelMonitor: justice tx on revoked commitment, justice tx on revoked HTLC-timeout, adjusted justice tx, ChannelManager: local commitment tx
+	assert_eq!(node_txn.len(), 4); // ChannelMonitor: justice tx on revoked commitment, justice tx on revoked HTLC-timeout, adjusted justice tx, ChannelManager: local commitment tx
+	assert_eq!(node_txn[0].input.len(), 2);
+	check_spends!(node_txn[0], revoked_local_txn[0]);
+	check_spends!(node_txn[1], chan_1.3);
 	assert_eq!(node_txn[2].input.len(), 1);
 	check_spends!(node_txn[2], revoked_htlc_txn[0]);
+	assert_eq!(node_txn[3].input.len(), 1);
+	check_spends!(node_txn[3], revoked_local_txn[0]);
 
 	// Check B's ChannelMonitor was able to generate the right spendable output descriptor
 	let spend_txn = check_spendable_outputs!(nodes[1], 1);
@@ -4233,7 +4240,7 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_success_tx() {
 
 	// Check A's ChannelMonitor was able to generate the right spendable output descriptor
 	let spend_txn = check_spendable_outputs!(nodes[0], 1);
-	assert_eq!(spend_txn.len(), 4);
+	assert_eq!(spend_txn.len(), 5); // Duplicated SpendableOutput due to block rescan after revoked htlc output tracking
 	assert_eq!(spend_txn[0], spend_txn[2]);
 	check_spends!(spend_txn[0], revoked_local_txn[0]); // spending to_remote output from revoked local tx
 	check_spends!(spend_txn[1], node_txn[0]); // spending justice tx output from revoked local tx htlc received output
@@ -6993,7 +7000,7 @@ fn test_bump_penalty_txn_on_revoked_htlcs() {
 
 		assert_eq!(node_txn[0].input.len(), 2);
 		check_spends!(node_txn[0], revoked_htlc_txn[0], revoked_htlc_txn[1]);
-		//// Verify bumped tx is different and 25% bump heuristic
+		// Verify bumped tx is different and 25% bump heuristic
 		assert_ne!(first, node_txn[0].txid());
 		let fee_2 = revoked_htlc_txn[0].output[0].value + revoked_htlc_txn[1].output[0].value - node_txn[0].output[0].value;
 		let feerate_2 = fee_2 * 1000 / node_txn[0].get_weight() as u64;
@@ -7008,7 +7015,13 @@ fn test_bump_penalty_txn_on_revoked_htlcs() {
 	connect_blocks(&nodes[0].block_notifier, 20, 145, true, header_145.bitcoin_hash());
 	{
 		let mut node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
-		assert_eq!(node_txn.len(), 1); //TODO: fix check_spend_remote_htlc lack of watch output
+		// We verify than no new transaction has been broadcast because previously
+		// we were buggy on this exact behavior by not tracking for monitoring remote HTLC outputs (see #411)
+		// which means we wouldn't see a spend of them by a justice tx and bumped justice tx
+		// were generated forever instead of safe cleaning after confirmation and ANTI_REORG_SAFE_DELAY blocks.
+		// Enforce spending of revoked htlc output by claiming transaction remove request as expected and dry
+		// up bumped justice generation.
+		assert_eq!(node_txn.len(), 0);
 		node_txn.clear();
 	}
 	check_closed_broadcast!(nodes[0], false);

lightning/src/ln/onchaintx.rs

@@ -482,6 +482,7 @@ impl OnchainTxHandler {
 		where B::Target: BroadcasterInterface,
 		      F::Target: FeeEstimator
 	{
+		log_trace!(self, "Block at height {} connected with {} claim requests", height, claimable_outpoints.len());
 		let mut new_claims = Vec::new();
 		let mut aggregated_claim = HashMap::new();
 		let mut aggregated_soonest = ::std::u32::MAX;
@@ -573,17 +574,21 @@ impl OnchainTxHandler {
 						if set_equality {
 							clean_claim_request_after_safety_delay!();
 						} else { // If false, generate new claim request with update outpoint set
+							let mut at_least_one_drop = false;
 							for input in tx.input.iter() {
 								if let Some(input_material) = claim_material.per_input_material.remove(&input.previous_output) {
 									claimed_outputs_material.push((input.previous_output, input_material));
+									at_least_one_drop = true;
 								}
 								// If there are no outpoints left to claim in this request, drop it entirely after ANTI_REORG_DELAY.
 								if claim_material.per_input_material.is_empty() {
 									clean_claim_request_after_safety_delay!();
 								}
 							}
 							//TODO: recompute soonest_timelock to avoid wasting a bit on fees
-							bump_candidates.insert(first_claim_txid_height.0.clone());
+							if at_least_one_drop {
+								bump_candidates.insert(first_claim_txid_height.0.clone());
+							}
 						}
 						break; //No need to iterate further, either tx is our or their
 					} else {
@@ -634,6 +639,7 @@ impl OnchainTxHandler {
 		}
 
 		// Build, bump and rebroadcast tx accordingly
+		log_trace!(self, "Bumping {} candidates", bump_candidates.len());
 		for first_claim_txid in bump_candidates.iter() {
 			if let Some((new_timer, new_feerate)) = {
 				if let Some(claim_material) = self.pending_claim_requests.get(first_claim_txid) {