Skip to content

Commit e323c13

Browse files
TheBlueMattTheBlueMatt
authored
Merge pull request #177 from TheBlueMatt/2018-09-163-cleanups
Optimize some ChannelMonitor stuff after #163
2 parents 01c8e4f + 66d5d76 commit e323c13

File tree

2 files changed

+76
-61
lines changed

2 files changed

+76
-61
lines changed

‎src/ln/channelmanager.rs

+57-45
Original file line numberDiff line numberDiff line change
@@ -3093,32 +3093,41 @@ mod tests {
30933093

30943094
#[derive(PartialEq)]
30953095
enum HTLCType { NONE, TIMEOUT, SUCCESS }
3096-
#[derive(PartialEq)]
3097-
enum PenaltyType { NONE, HTLC }
3098-
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> {
3096+
/// Tests that the given node has broadcast transactions for the given Channel
3097+
///
3098+
/// First checks that the latest local commitment tx has been broadcast, unless an explicit
3099+
/// commitment_tx is provided, which may be used to test that a remote commitment tx was
3100+
/// broadcast and the revoked outputs were claimed.
3101+
///
3102+
/// Next tests that there is (or is not) a transaction that spends the commitment transaction
3103+
/// that appears to be the type of HTLC transaction specified in has_htlc_tx.
3104+
///
3105+
/// All broadcast transactions must be accounted for in one of the above three types of we'll
3106+
/// also fail.
3107+
fn test_txn_broadcast(node: &Node, chan: &(msgs::ChannelUpdate, msgs::ChannelUpdate, [u8; 32], Transaction), commitment_tx: Option<Transaction>, has_htlc_tx: HTLCType) -> Vec<Transaction> {
30993108
let mut node_txn = node.tx_broadcaster.txn_broadcasted.lock().unwrap();
3100-
assert!(node_txn.len() >= if has_htlc_tx == HTLCType::NONE { 0 } else { 1 } + if has_penalty_tx == PenaltyType::NONE { 0 } else { 1 });
3109+
assert!(node_txn.len() >= if commitment_tx.is_some() { 0 } else { 1 } + if has_htlc_tx == HTLCType::NONE { 0 } else { 1 });
31013110

31023111
let mut res = Vec::with_capacity(2);
3103-
3104-
if let Some(explicit_tx) = commitment_tx {
3105-
res.push(explicit_tx.clone());
3106-
} else {
3107-
for tx in node_txn.iter() {
3108-
if tx.input.len() == 1 && tx.input[0].previous_output.txid == chan.3.txid() {
3109-
let mut funding_tx_map = HashMap::new();
3110-
funding_tx_map.insert(chan.3.txid(), chan.3.clone());
3111-
tx.verify(&funding_tx_map).unwrap();
3112+
node_txn.retain(|tx| {
3113+
if tx.input.len() == 1 && tx.input[0].previous_output.txid == chan.3.txid() {
3114+
let mut funding_tx_map = HashMap::new();
3115+
funding_tx_map.insert(chan.3.txid(), chan.3.clone());
3116+
tx.verify(&funding_tx_map).unwrap();
3117+
if commitment_tx.is_none() {
31123118
res.push(tx.clone());
31133119
}
3114-
}
3115-
}
3116-
if !revoked_tx.is_some() && !(has_penalty_tx == PenaltyType::HTLC) {
3117-
assert_eq!(res.len(), 1);
3120+
false
3121+
} else { true }
3122+
});
3123+
if let Some(explicit_tx) = commitment_tx {
3124+
res.push(explicit_tx.clone());
31183125
}
31193126

3127+
assert_eq!(res.len(), 1);
3128+
31203129
if has_htlc_tx != HTLCType::NONE {
3121-
for tx in node_txn.iter() {
3130+
node_txn.retain(|tx| {
31223131
if tx.input.len() == 1 && tx.input[0].previous_output.txid == res[0].txid() {
31233132
let mut funding_tx_map = HashMap::new();
31243133
funding_tx_map.insert(res[0].txid(), res[0].clone());
@@ -3129,29 +3138,32 @@ mod tests {
31293138
assert!(tx.lock_time == 0);
31303139
}
31313140
res.push(tx.clone());
3132-
break;
3133-
}
3134-
}
3141+
false
3142+
} else { true }
3143+
});
31353144
assert_eq!(res.len(), 2);
31363145
}
31373146

3138-
if has_penalty_tx == PenaltyType::HTLC {
3139-
let revoked_tx = revoked_tx.unwrap();
3140-
for tx in node_txn.iter() {
3141-
if tx.input.len() == 1 && tx.input[0].previous_output.txid == revoked_tx.txid() {
3142-
let mut funding_tx_map = HashMap::new();
3143-
funding_tx_map.insert(revoked_tx.txid(), revoked_tx.clone());
3144-
tx.verify(&funding_tx_map).unwrap();
3145-
res.push(tx.clone());
3146-
break;
3147-
}
3148-
}
3149-
assert_eq!(res.len(), 1);
3150-
}
3151-
node_txn.clear();
3147+
assert!(node_txn.is_empty());
31523148
res
31533149
}
31543150

3151+
/// Tests that the given node has broadcast a claim transaction against the provided revoked
3152+
/// HTLC transaction.
3153+
fn test_revoked_htlc_claim_txn_broadcast(node: &Node, revoked_tx: Transaction) {
3154+
let mut node_txn = node.tx_broadcaster.txn_broadcasted.lock().unwrap();
3155+
assert_eq!(node_txn.len(), 1);
3156+
node_txn.retain(|tx| {
3157+
if tx.input.len() == 1 && tx.input[0].previous_output.txid == revoked_tx.txid() {
3158+
let mut funding_tx_map = HashMap::new();
3159+
funding_tx_map.insert(revoked_tx.txid(), revoked_tx.clone());
3160+
tx.verify(&funding_tx_map).unwrap();
3161+
false
3162+
} else { true }
3163+
});
3164+
assert!(node_txn.is_empty());
3165+
}
3166+
31553167
fn check_preimage_claim(node: &Node, prev_txn: &Vec<Transaction>) -> Vec<Transaction> {
31563168
let mut node_txn = node.tx_broadcaster.txn_broadcasted.lock().unwrap();
31573169

@@ -3225,10 +3237,10 @@ mod tests {
32253237
// Simple case with no pending HTLCs:
32263238
nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), true);
32273239
{
3228-
let mut node_txn = test_txn_broadcast(&nodes[1], &chan_1, None, None, HTLCType::NONE, PenaltyType::NONE);
3240+
let mut node_txn = test_txn_broadcast(&nodes[1], &chan_1, None, HTLCType::NONE);
32293241
let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
32303242
nodes[0].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![node_txn.drain(..).next().unwrap()] }, 1);
3231-
test_txn_broadcast(&nodes[0], &chan_1, None, None, HTLCType::NONE, PenaltyType::NONE);
3243+
test_txn_broadcast(&nodes[0], &chan_1, None, HTLCType::NONE);
32323244
}
32333245
get_announce_close_broadcast_events(&nodes, 0, 1);
32343246
assert_eq!(nodes[0].node.list_channels().len(), 0);
@@ -3240,10 +3252,10 @@ mod tests {
32403252
// Simple case of one pending HTLC to HTLC-Timeout
32413253
nodes[1].node.peer_disconnected(&nodes[2].node.get_our_node_id(), true);
32423254
{
3243-
let mut node_txn = test_txn_broadcast(&nodes[1], &chan_2, None, None, HTLCType::TIMEOUT, PenaltyType::NONE);
3255+
let mut node_txn = test_txn_broadcast(&nodes[1], &chan_2, None, HTLCType::TIMEOUT);
32443256
let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
32453257
nodes[2].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![node_txn.drain(..).next().unwrap()] }, 1);
3246-
test_txn_broadcast(&nodes[2], &chan_2, None, None, HTLCType::NONE, PenaltyType::NONE);
3258+
test_txn_broadcast(&nodes[2], &chan_2, None, HTLCType::NONE);
32473259
}
32483260
get_announce_close_broadcast_events(&nodes, 1, 2);
32493261
assert_eq!(nodes[1].node.list_channels().len(), 0);
@@ -3277,7 +3289,7 @@ mod tests {
32773289
// HTLC-Timeout and a nodes[3] claim against it (+ its own announces)
32783290
nodes[2].node.peer_disconnected(&nodes[3].node.get_our_node_id(), true);
32793291
{
3280-
let node_txn = test_txn_broadcast(&nodes[2], &chan_3, None, None, HTLCType::TIMEOUT, PenaltyType::NONE);
3292+
let node_txn = test_txn_broadcast(&nodes[2], &chan_3, None, HTLCType::TIMEOUT);
32813293

32823294
// Claim the payment on nodes[3], giving it knowledge of the preimage
32833295
claim_funds!(nodes[3], nodes[2], payment_preimage_1);
@@ -3302,7 +3314,7 @@ mod tests {
33023314
nodes[3].chain_monitor.block_connected_checked(&header, i, &Vec::new()[..], &[0; 0]);
33033315
}
33043316

3305-
let node_txn = test_txn_broadcast(&nodes[3], &chan_4, None, None, HTLCType::TIMEOUT, PenaltyType::NONE);
3317+
let node_txn = test_txn_broadcast(&nodes[3], &chan_4, None, HTLCType::TIMEOUT);
33063318

33073319
// Claim the payment on nodes[4], giving it knowledge of the preimage
33083320
claim_funds!(nodes[4], nodes[3], payment_preimage_2);
@@ -3314,7 +3326,7 @@ mod tests {
33143326
nodes[4].chain_monitor.block_connected_checked(&header, i, &Vec::new()[..], &[0; 0]);
33153327
}
33163328

3317-
test_txn_broadcast(&nodes[4], &chan_4, None, None, HTLCType::SUCCESS, PenaltyType::NONE);
3329+
test_txn_broadcast(&nodes[4], &chan_4, None, HTLCType::SUCCESS);
33183330

33193331
header = BlockHeader { version: 0x20000000, prev_blockhash: header.bitcoin_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
33203332
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 {
33493361
node_txn[0].verify(&funding_tx_map).unwrap();
33503362
node_txn.swap_remove(0);
33513363
}
3352-
test_txn_broadcast(&nodes[1], &chan_5, None, None, HTLCType::NONE, PenaltyType::NONE);
3364+
test_txn_broadcast(&nodes[1], &chan_5, None, HTLCType::NONE);
33533365

33543366
nodes[0].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![revoked_local_txn[0].clone()] }, 1);
3355-
let node_txn = test_txn_broadcast(&nodes[0], &chan_5, Some(revoked_local_txn[0].clone()), None, HTLCType::TIMEOUT, PenaltyType::NONE);
3367+
let node_txn = test_txn_broadcast(&nodes[0], &chan_5, Some(revoked_local_txn[0].clone()), HTLCType::TIMEOUT);
33563368
header = BlockHeader { version: 0x20000000, prev_blockhash: header.bitcoin_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
33573369
nodes[1].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![node_txn[1].clone()] }, 1);
3358-
test_txn_broadcast(&nodes[1], &chan_5, None, Some(node_txn[1].clone()), HTLCType::NONE, PenaltyType::HTLC);
3370+
test_revoked_htlc_claim_txn_broadcast(&nodes[1], node_txn[1].clone());
33593371
}
33603372
get_announce_close_broadcast_events(&nodes, 0, 1);
33613373
assert_eq!(nodes[0].node.list_channels().len(), 0);

‎src/ln/channelmonitor.rs

+19-16
Original file line numberDiff line numberDiff line change
@@ -1210,35 +1210,32 @@ impl ChannelMonitor {
12101210
}
12111211

12121212
/// Attempst to claim a remote HTLC-Success/HTLC-Timeout s outputs using the revocation key
1213-
fn check_spend_remote_htlc(&self, tx: &Transaction, commitment_number: u64) -> Vec<Transaction> {
1214-
let mut txn_to_broadcast = Vec::new();
1215-
1213+
fn check_spend_remote_htlc(&self, tx: &Transaction, commitment_number: u64) -> Option<Transaction> {
12161214
let htlc_txid = tx.txid(); //TODO: This is gonna be a performance bottleneck for watchtowers!
12171215

12181216
macro_rules! ignore_error {
12191217
( $thing : expr ) => {
12201218
match $thing {
12211219
Ok(a) => a,
1222-
Err(_) => return txn_to_broadcast
1220+
Err(_) => return None
12231221
}
12241222
};
12251223
}
12261224

12271225
let secret = self.get_secret(commitment_number).unwrap();
12281226
let per_commitment_key = ignore_error!(SecretKey::from_slice(&self.secp_ctx, &secret));
1227+
let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key);
12291228
let revocation_pubkey = match self.key_storage {
12301229
KeyStorage::PrivMode { ref revocation_base_key, .. } => {
1231-
let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key);
12321230
ignore_error!(chan_utils::derive_public_revocation_key(&self.secp_ctx, &per_commitment_point, &PublicKey::from_secret_key(&self.secp_ctx, &revocation_base_key)))
12331231
},
12341232
KeyStorage::SigsMode { ref revocation_base_key, .. } => {
1235-
let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key);
12361233
ignore_error!(chan_utils::derive_public_revocation_key(&self.secp_ctx, &per_commitment_point, &revocation_base_key))
12371234
},
12381235
};
12391236
let delayed_key = match self.their_delayed_payment_base_key {
1240-
None => return txn_to_broadcast,
1241-
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)),
1237+
None => return None,
1238+
Some(their_delayed_payment_base_key) => ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, &per_commitment_point, &their_delayed_payment_base_key)),
12421239
};
12431240
let redeemscript = chan_utils::get_revokeable_redeemscript(&revocation_pubkey, self.their_to_self_delay.unwrap(), &delayed_key);
12441241
let revokeable_p2wsh = redeemscript.to_v0_p2wsh();
@@ -1290,9 +1287,8 @@ impl ChannelMonitor {
12901287
spend_tx.input[0].witness.push(vec!(1));
12911288
spend_tx.input[0].witness.push(redeemscript.into_bytes());
12921289

1293-
txn_to_broadcast.push(spend_tx);
1294-
}
1295-
txn_to_broadcast
1290+
Some(spend_tx)
1291+
} else { None }
12961292
}
12971293

12981294
fn broadcast_by_local_state(&self, local_tx: &LocalSignedTx) -> Vec<Transaction> {
@@ -1356,9 +1352,14 @@ impl ChannelMonitor {
13561352
fn block_connected(&self, txn_matched: &[&Transaction], height: u32, broadcaster: &BroadcasterInterface)-> Vec<(Sha256dHash, Vec<TxOut>)> {
13571353
let mut watch_outputs = Vec::new();
13581354
for tx in txn_matched {
1359-
let mut txn: Vec<Transaction> = Vec::new();
1360-
for txin in tx.input.iter() {
1361-
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) {
1355+
if tx.input.len() == 1 {
1356+
// Assuming our keys were not leaked (in which case we're screwed no matter what),
1357+
// commitment transactions and HTLC transactions will all only ever have one input,
1358+
// which is an easy way to filter out any potential non-matching txn for lazy
1359+
// filters.
1360+
let prevout = &tx.input[0].previous_output;
1361+
let mut txn: Vec<Transaction> = Vec::new();
1362+
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) {
13621363
let (remote_txn, new_outputs) = self.check_spend_remote_transaction(tx, height);
13631364
txn = remote_txn;
13641365
if !new_outputs.1.is_empty() {
@@ -1369,8 +1370,10 @@ impl ChannelMonitor {
13691370
}
13701371
} else {
13711372
let remote_commitment_txn_on_chain = self.remote_commitment_txn_on_chain.lock().unwrap();
1372-
for commitment_number in remote_commitment_txn_on_chain.get(&txin.previous_output.txid) {
1373-
txn = self.check_spend_remote_htlc(tx, *commitment_number);
1373+
if let Some(commitment_number) = remote_commitment_txn_on_chain.get(&prevout.txid) {
1374+
if let Some(tx) = self.check_spend_remote_htlc(tx, *commitment_number) {
1375+
txn.push(tx);
1376+
}
13741377
}
13751378
}
13761379
for tx in txn.iter() {

0 commit comments

Comments
 (0)