Skip to content

Optimize some ChannelMonitor stuff after #163 #177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 14, 2018
Merged

Optimize some ChannelMonitor stuff after #163 #177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
27d5a3a
Optimize check_spend_remote HTLC a tad by avoiding indirections
TheBlueMatt Sep 13, 2018
e9e27f2
There can only be one input in matched txn in ChannelMonitor
TheBlueMatt Sep 13, 2018
66d5d76
Clean up and clarify tx broadcast checks in channelmonitor tests
TheBlueMatt Sep 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 57 additions & 45 deletions src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Transaction>, revoked_tx: Option<Transaction>, has_htlc_tx: HTLCType, has_penalty_tx: PenaltyType) -> Vec<Transaction> {
/// 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<Transaction>, has_htlc_tx: HTLCType) -> Vec<Transaction> {
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());
Expand All @@ -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<Transaction>) -> Vec<Transaction> {
let mut node_txn = node.tx_broadcaster.txn_broadcasted.lock().unwrap();

Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
35 changes: 19 additions & 16 deletions src/ln/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Transaction> {
let mut txn_to_broadcast = Vec::new();

fn check_spend_remote_htlc(&self, tx: &Transaction, commitment_number: u64) -> Option<Transaction> {
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();
Expand Down Expand Up @@ -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<Transaction> {
Expand Down Expand Up @@ -1356,9 +1352,14 @@ impl ChannelMonitor {
fn block_connected(&self, txn_matched: &[&Transaction], height: u32, broadcaster: &BroadcasterInterface)-> Vec<(Sha256dHash, Vec<TxOut>)> {
let mut watch_outputs = Vec::new();
for tx in txn_matched {
let mut txn: Vec<Transaction> = 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<Transaction> = 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() {
Expand All @@ -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() {
Expand Down