From 27d5a3a94f9bac6d88f4ea364142d98a8ff2dc20 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 13 Sep 2018 11:34:26 -0400 Subject: [PATCH 1/3] Optimize check_spend_remote HTLC a tad by avoiding indirections Instead of hopping a pointer, we're only ever going to return one Transaction at max, so skip the Vec. Also avoid re-pubkey-converting the revocation key. --- src/ln/channelmonitor.rs | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/src/ln/channelmonitor.rs b/src/ln/channelmonitor.rs index 6de0a63609a..f037c673079 100644 --- a/src/ln/channelmonitor.rs +++ b/src/ln/channelmonitor.rs @@ -1210,35 +1210,32 @@ impl ChannelMonitor { } /// Attempst to claim a remote HTLC-Success/HTLC-Timeout s outputs using the revocation key - fn check_spend_remote_htlc(&self, tx: &Transaction, commitment_number: u64) -> Vec { - let mut txn_to_broadcast = Vec::new(); - + fn check_spend_remote_htlc(&self, tx: &Transaction, commitment_number: u64) -> Option { let htlc_txid = tx.txid(); //TODO: This is gonna be a performance bottleneck for watchtowers! macro_rules! ignore_error { ( $thing : expr ) => { match $thing { Ok(a) => a, - Err(_) => return txn_to_broadcast + Err(_) => return None } }; } let secret = self.get_secret(commitment_number).unwrap(); let per_commitment_key = ignore_error!(SecretKey::from_slice(&self.secp_ctx, &secret)); + let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key); let revocation_pubkey = match self.key_storage { KeyStorage::PrivMode { ref revocation_base_key, .. } => { - let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key); ignore_error!(chan_utils::derive_public_revocation_key(&self.secp_ctx, &per_commitment_point, &PublicKey::from_secret_key(&self.secp_ctx, &revocation_base_key))) }, KeyStorage::SigsMode { ref revocation_base_key, .. } => { - let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key); ignore_error!(chan_utils::derive_public_revocation_key(&self.secp_ctx, &per_commitment_point, &revocation_base_key)) }, }; let delayed_key = match self.their_delayed_payment_base_key { - None => return txn_to_broadcast, - Some(their_delayed_payment_base_key) => ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, &PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key), &their_delayed_payment_base_key)), + None => return 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.their_to_self_delay.unwrap(), &delayed_key); let revokeable_p2wsh = redeemscript.to_v0_p2wsh(); @@ -1290,9 +1287,8 @@ impl ChannelMonitor { spend_tx.input[0].witness.push(vec!(1)); spend_tx.input[0].witness.push(redeemscript.into_bytes()); - txn_to_broadcast.push(spend_tx); - } - txn_to_broadcast + Some(spend_tx) + } else { None } } fn broadcast_by_local_state(&self, local_tx: &LocalSignedTx) -> Vec { @@ -1369,8 +1365,10 @@ impl ChannelMonitor { } } else { let remote_commitment_txn_on_chain = self.remote_commitment_txn_on_chain.lock().unwrap(); - for commitment_number in remote_commitment_txn_on_chain.get(&txin.previous_output.txid) { - txn = self.check_spend_remote_htlc(tx, *commitment_number); + if let Some(commitment_number) = remote_commitment_txn_on_chain.get(&prevout.txid) { + if let Some(tx) = self.check_spend_remote_htlc(tx, *commitment_number) { + txn.push(tx); + } } } for tx in txn.iter() { From e9e27f277a3330cede4fd79f41923b03d3b872ed Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 13 Sep 2018 11:35:23 -0400 Subject: [PATCH 2/3] There can only be one input in matched txn in ChannelMonitor This lets us simplify a few tidbits of loop. --- src/ln/channelmonitor.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/ln/channelmonitor.rs b/src/ln/channelmonitor.rs index f037c673079..12c26322bf8 100644 --- a/src/ln/channelmonitor.rs +++ b/src/ln/channelmonitor.rs @@ -1352,9 +1352,14 @@ impl ChannelMonitor { fn block_connected(&self, txn_matched: &[&Transaction], height: u32, broadcaster: &BroadcasterInterface)-> Vec<(Sha256dHash, Vec)> { let mut watch_outputs = Vec::new(); for tx in txn_matched { - let mut txn: Vec = Vec::new(); - for txin in tx.input.iter() { - if self.funding_txo.is_none() || (txin.previous_output.txid == self.funding_txo.as_ref().unwrap().0.txid && txin.previous_output.vout == self.funding_txo.as_ref().unwrap().0.index as u32) { + if tx.input.len() == 1 { + // Assuming our keys were not leaked (in which case we're screwed no matter what), + // commitment transactions and HTLC transactions will all only ever have one input, + // which is an easy way to filter out any potential non-matching txn for lazy + // filters. + let prevout = &tx.input[0].previous_output; + let mut txn: Vec = Vec::new(); + if self.funding_txo.is_none() || (prevout.txid == self.funding_txo.as_ref().unwrap().0.txid && prevout.vout == self.funding_txo.as_ref().unwrap().0.index as u32) { let (remote_txn, new_outputs) = self.check_spend_remote_transaction(tx, height); txn = remote_txn; if !new_outputs.1.is_empty() { From 66d5d764aa2a23f846efecdfa8522cd96e2d1aee Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 13 Sep 2018 13:51:23 -0400 Subject: [PATCH 3/3] Clean up and clarify tx broadcast checks in channelmonitor tests This effecitlvey reverts the refactors in 383bd90a481bc146b3a3b1d8, however keeps the actully new test code. It also writes documentation for the super confusing tx test func and makes it a bit less permissive. --- src/ln/channelmanager.rs | 102 ++++++++++++++++++++++----------------- 1 file changed, 57 insertions(+), 45 deletions(-) diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index e9f49bfbd2f..a2b3b242b8a 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -3093,32 +3093,41 @@ mod tests { #[derive(PartialEq)] enum HTLCType { NONE, TIMEOUT, SUCCESS } - #[derive(PartialEq)] - enum PenaltyType { NONE, HTLC } - fn test_txn_broadcast(node: &Node, chan: &(msgs::ChannelUpdate, msgs::ChannelUpdate, [u8; 32], Transaction), commitment_tx: Option, revoked_tx: Option, has_htlc_tx: HTLCType, has_penalty_tx: PenaltyType) -> Vec { + /// Tests that the given node has broadcast transactions for the given Channel + /// + /// First checks that the latest local commitment tx has been broadcast, unless an explicit + /// commitment_tx is provided, which may be used to test that a remote commitment tx was + /// broadcast and the revoked outputs were claimed. + /// + /// Next tests that there is (or is not) a transaction that spends the commitment transaction + /// that appears to be the type of HTLC transaction specified in has_htlc_tx. + /// + /// All broadcast transactions must be accounted for in one of the above three types of we'll + /// also fail. + fn test_txn_broadcast(node: &Node, chan: &(msgs::ChannelUpdate, msgs::ChannelUpdate, [u8; 32], Transaction), commitment_tx: Option, has_htlc_tx: HTLCType) -> Vec { let mut node_txn = node.tx_broadcaster.txn_broadcasted.lock().unwrap(); - assert!(node_txn.len() >= if has_htlc_tx == HTLCType::NONE { 0 } else { 1 } + if has_penalty_tx == PenaltyType::NONE { 0 } else { 1 }); + assert!(node_txn.len() >= if commitment_tx.is_some() { 0 } else { 1 } + if has_htlc_tx == HTLCType::NONE { 0 } else { 1 }); let mut res = Vec::with_capacity(2); - - if let Some(explicit_tx) = commitment_tx { - res.push(explicit_tx.clone()); - } else { - for tx in node_txn.iter() { - if tx.input.len() == 1 && tx.input[0].previous_output.txid == chan.3.txid() { - let mut funding_tx_map = HashMap::new(); - funding_tx_map.insert(chan.3.txid(), chan.3.clone()); - tx.verify(&funding_tx_map).unwrap(); + node_txn.retain(|tx| { + if tx.input.len() == 1 && tx.input[0].previous_output.txid == chan.3.txid() { + let mut funding_tx_map = HashMap::new(); + funding_tx_map.insert(chan.3.txid(), chan.3.clone()); + tx.verify(&funding_tx_map).unwrap(); + if commitment_tx.is_none() { res.push(tx.clone()); } - } - } - if !revoked_tx.is_some() && !(has_penalty_tx == PenaltyType::HTLC) { - assert_eq!(res.len(), 1); + false + } else { true } + }); + if let Some(explicit_tx) = commitment_tx { + res.push(explicit_tx.clone()); } + assert_eq!(res.len(), 1); + if has_htlc_tx != HTLCType::NONE { - for tx in node_txn.iter() { + node_txn.retain(|tx| { if tx.input.len() == 1 && tx.input[0].previous_output.txid == res[0].txid() { let mut funding_tx_map = HashMap::new(); funding_tx_map.insert(res[0].txid(), res[0].clone()); @@ -3129,29 +3138,32 @@ mod tests { assert!(tx.lock_time == 0); } res.push(tx.clone()); - break; - } - } + false + } else { true } + }); assert_eq!(res.len(), 2); } - if has_penalty_tx == PenaltyType::HTLC { - let revoked_tx = revoked_tx.unwrap(); - for tx in node_txn.iter() { - if tx.input.len() == 1 && tx.input[0].previous_output.txid == revoked_tx.txid() { - let mut funding_tx_map = HashMap::new(); - funding_tx_map.insert(revoked_tx.txid(), revoked_tx.clone()); - tx.verify(&funding_tx_map).unwrap(); - res.push(tx.clone()); - break; - } - } - assert_eq!(res.len(), 1); - } - node_txn.clear(); + assert!(node_txn.is_empty()); res } + /// Tests that the given node has broadcast a claim transaction against the provided revoked + /// HTLC transaction. + fn test_revoked_htlc_claim_txn_broadcast(node: &Node, revoked_tx: Transaction) { + let mut node_txn = node.tx_broadcaster.txn_broadcasted.lock().unwrap(); + assert_eq!(node_txn.len(), 1); + node_txn.retain(|tx| { + if tx.input.len() == 1 && tx.input[0].previous_output.txid == revoked_tx.txid() { + let mut funding_tx_map = HashMap::new(); + funding_tx_map.insert(revoked_tx.txid(), revoked_tx.clone()); + tx.verify(&funding_tx_map).unwrap(); + false + } else { true } + }); + assert!(node_txn.is_empty()); + } + fn check_preimage_claim(node: &Node, prev_txn: &Vec) -> Vec { let mut node_txn = node.tx_broadcaster.txn_broadcasted.lock().unwrap(); @@ -3225,10 +3237,10 @@ mod tests { // Simple case with no pending HTLCs: nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), true); { - let mut node_txn = test_txn_broadcast(&nodes[1], &chan_1, None, None, HTLCType::NONE, PenaltyType::NONE); + let mut node_txn = test_txn_broadcast(&nodes[1], &chan_1, None, HTLCType::NONE); let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 }; nodes[0].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![node_txn.drain(..).next().unwrap()] }, 1); - test_txn_broadcast(&nodes[0], &chan_1, None, None, HTLCType::NONE, PenaltyType::NONE); + test_txn_broadcast(&nodes[0], &chan_1, None, HTLCType::NONE); } get_announce_close_broadcast_events(&nodes, 0, 1); assert_eq!(nodes[0].node.list_channels().len(), 0); @@ -3240,10 +3252,10 @@ mod tests { // Simple case of one pending HTLC to HTLC-Timeout nodes[1].node.peer_disconnected(&nodes[2].node.get_our_node_id(), true); { - let mut node_txn = test_txn_broadcast(&nodes[1], &chan_2, None, None, HTLCType::TIMEOUT, PenaltyType::NONE); + let mut node_txn = test_txn_broadcast(&nodes[1], &chan_2, None, HTLCType::TIMEOUT); let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 }; nodes[2].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![node_txn.drain(..).next().unwrap()] }, 1); - test_txn_broadcast(&nodes[2], &chan_2, None, None, HTLCType::NONE, PenaltyType::NONE); + test_txn_broadcast(&nodes[2], &chan_2, None, HTLCType::NONE); } get_announce_close_broadcast_events(&nodes, 1, 2); assert_eq!(nodes[1].node.list_channels().len(), 0); @@ -3277,7 +3289,7 @@ mod tests { // HTLC-Timeout and a nodes[3] claim against it (+ its own announces) nodes[2].node.peer_disconnected(&nodes[3].node.get_our_node_id(), true); { - let node_txn = test_txn_broadcast(&nodes[2], &chan_3, None, None, HTLCType::TIMEOUT, PenaltyType::NONE); + let node_txn = test_txn_broadcast(&nodes[2], &chan_3, None, HTLCType::TIMEOUT); // Claim the payment on nodes[3], giving it knowledge of the preimage claim_funds!(nodes[3], nodes[2], payment_preimage_1); @@ -3302,7 +3314,7 @@ mod tests { nodes[3].chain_monitor.block_connected_checked(&header, i, &Vec::new()[..], &[0; 0]); } - let node_txn = test_txn_broadcast(&nodes[3], &chan_4, None, None, HTLCType::TIMEOUT, PenaltyType::NONE); + let node_txn = test_txn_broadcast(&nodes[3], &chan_4, None, HTLCType::TIMEOUT); // Claim the payment on nodes[4], giving it knowledge of the preimage claim_funds!(nodes[4], nodes[3], payment_preimage_2); @@ -3314,7 +3326,7 @@ mod tests { nodes[4].chain_monitor.block_connected_checked(&header, i, &Vec::new()[..], &[0; 0]); } - test_txn_broadcast(&nodes[4], &chan_4, None, None, HTLCType::SUCCESS, PenaltyType::NONE); + test_txn_broadcast(&nodes[4], &chan_4, None, HTLCType::SUCCESS); header = BlockHeader { version: 0x20000000, prev_blockhash: header.bitcoin_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 }; nodes[4].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![node_txn[0].clone()] }, TEST_FINAL_CLTV - 5); @@ -3349,13 +3361,13 @@ mod tests { node_txn[0].verify(&funding_tx_map).unwrap(); node_txn.swap_remove(0); } - test_txn_broadcast(&nodes[1], &chan_5, None, None, HTLCType::NONE, PenaltyType::NONE); + test_txn_broadcast(&nodes[1], &chan_5, None, HTLCType::NONE); nodes[0].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![revoked_local_txn[0].clone()] }, 1); - let node_txn = test_txn_broadcast(&nodes[0], &chan_5, Some(revoked_local_txn[0].clone()), None, HTLCType::TIMEOUT, PenaltyType::NONE); + 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 }; nodes[1].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![node_txn[1].clone()] }, 1); - test_txn_broadcast(&nodes[1], &chan_5, None, Some(node_txn[1].clone()), HTLCType::NONE, PenaltyType::HTLC); + test_revoked_htlc_claim_txn_broadcast(&nodes[1], node_txn[1].clone()); } get_announce_close_broadcast_events(&nodes, 0, 1); assert_eq!(nodes[0].node.list_channels().len(), 0);