Skip to content

Commit 66d5d76

Browse files
TheBlueMattTheBlueMatt
committed
Clean up and clarify tx broadcast checks in channelmonitor tests
This effecitlvey reverts the refactors in 383bd90, 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.
1 parent e9e27f2 commit 66d5d76

File tree

1 file changed

+57
-45
lines changed

1 file changed

+57
-45
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);

0 commit comments

Comments
 (0)