From: Matt Corallo <649246+TheBlueMatt@users.noreply.github.com> Date: Fri, 14 Sep 2018 00:18:21 +0000 (-0400) Subject: Merge pull request #177 from TheBlueMatt/2018-09-163-cleanups X-Git-Tag: v0.0.12~311 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=e323c13a59a1d2ce9f0f1c727ee7c51b448e4ab5;hp=01c8e4f56fed86fdca842a03968741549b336bc1;p=rust-lightning Merge pull request #177 from TheBlueMatt/2018-09-163-cleanups Optimize some ChannelMonitor stuff after #163 --- diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index e9f49bfbd..a2b3b242b 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); diff --git a/src/ln/channelmonitor.rs b/src/ln/channelmonitor.rs index 6de0a6360..12c26322b 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 { @@ -1356,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() { @@ -1369,8 +1370,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() {