Skip to content

Commit f0c4af9

Browse files
wpaulinowpaulino
committed
Send splice_locked before reestablish commitment
If we have pending updates to send to our counterparty on reestablishment, while also pending a `splice_locked` send, then we must send our `splice_locked` first as the pending updates are considering the post-splice-locked state.
1 parent 294fbba commit f0c4af9

2 files changed

Lines changed: 163 additions & 16 deletions

File tree

‎lightning/src/ln/channelmanager.rs‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11052,6 +11052,19 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1105211052
}
1105311053
}
1105411054

11055+
if let Some(funding_tx_signed) = funding_tx_signed.as_ref() {
11056+
// These [`FundingTxSigned`] fields are only expected as a result of calling
11057+
// [`ChannelManager::funding_transaction_signed`].
11058+
debug_assert!(funding_tx_signed.commitment_signed.is_none());
11059+
debug_assert!(funding_tx_signed.counterparty_initial_commitment_signed_result.is_none());
11060+
}
11061+
if let Some(msg) = funding_tx_signed.as_mut().and_then(|v| v.splice_locked.take()) {
11062+
pending_msg_events.push(MessageSendEvent::SendSpliceLocked {
11063+
node_id: counterparty_node_id,
11064+
msg,
11065+
});
11066+
}
11067+
1105511068
macro_rules! handle_cs { () => {
1105611069
if let Some(update) = commitment_update {
1105711070
pending_msg_events.push(MessageSendEvent::UpdateHTLCs {
@@ -11080,12 +11093,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1108011093
},
1108111094
}
1108211095

11083-
if let Some(funding_tx_signed) = funding_tx_signed.as_ref() {
11084-
// These [`FundingTxSigned`] fields are only expected as a result of calling
11085-
// [`ChannelManager::funding_transaction_signed`].
11086-
debug_assert!(funding_tx_signed.commitment_signed.is_none());
11087-
debug_assert!(funding_tx_signed.counterparty_initial_commitment_signed_result.is_none());
11088-
}
1108911096
if let Some(msg) = funding_tx_signed.as_mut().and_then(|v| v.tx_signatures.take()) {
1109011097
pending_msg_events.push(MessageSendEvent::SendTxSignatures {
1109111098
node_id: counterparty_node_id,
@@ -11111,13 +11118,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
1111111118
});
1111211119
}
1111311120
}
11114-
11115-
if let Some(msg) = funding_tx_signed.as_mut().and_then(|v| v.splice_locked.take()) {
11116-
pending_msg_events.push(MessageSendEvent::SendSpliceLocked {
11117-
node_id: counterparty_node_id,
11118-
msg,
11119-
});
11120-
}
1112111121
} else if let Some(msg) = channel_ready {
1112211122
self.send_channel_ready(pending_msg_events, channel, msg);
1112311123
}

‎lightning/src/ln/splicing_tests.rs‎

Lines changed: 150 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2709,6 +2709,23 @@ fn test_promoted_splice_locked_sent_after_channel_reestablish() {
27092709
initiate_splice_out(&nodes[0], &nodes[1], channel_id, outputs).unwrap();
27102710
let (splice_tx, _) = splice_channel(&nodes[0], &nodes[1], channel_id, funding_contribution);
27112711

2712+
// Send a payment from node 0 to node 1 but don't fully commit it to make sure node 1
2713+
// sends `splice_locked` first when it responds.
2714+
let payment_amount = 1_000_000;
2715+
let (route, payment_hash, _payment_preimage, payment_secret) =
2716+
get_route_and_payment_hash!(nodes[0], nodes[1], payment_amount);
2717+
let onion = RecipientOnionFields::secret_only(payment_secret, payment_amount);
2718+
let payment_id = PaymentId(payment_hash.0);
2719+
nodes[0].node.send_payment_with_route(route, payment_hash, onion, payment_id).unwrap();
2720+
let update = get_htlc_update_msgs(&nodes[0], &node_id_1);
2721+
check_added_monitors(&nodes[0], 1);
2722+
2723+
nodes[1].node.handle_update_add_htlc(node_id_0, &update.update_add_htlcs[0]);
2724+
nodes[1].node.handle_commitment_signed_batch_test(node_id_0, &update.commitment_signed);
2725+
check_added_monitors(&nodes[1], 1);
2726+
let (_dropped_raa, dropped_commitment_signed) = get_revoke_commit_msgs(&nodes[1], &node_id_0);
2727+
assert!(dropped_commitment_signed.len() > 1, "{dropped_commitment_signed:?}");
2728+
27122729
// Confirm the splice for node 0 first. This should result in them sending `splice_locked`, but
27132730
// node 1 should not send it back yet as it hasn't seen the confirmation.
27142731
confirm_transaction(&nodes[0], &splice_tx);
@@ -2740,14 +2757,25 @@ fn test_promoted_splice_locked_sent_after_channel_reestablish() {
27402757

27412758
nodes[1].node.handle_channel_reestablish(node_id_0, &reestablish_0);
27422759
let msg_events = nodes[1].node.get_and_clear_pending_msg_events();
2743-
assert_eq!(msg_events.len(), 3, "{msg_events:?}");
2760+
assert_eq!(msg_events.len(), 5, "{msg_events:?}");
27442761
assert!(matches!(&msg_events[0], MessageSendEvent::SendAnnouncementSignatures { .. }));
27452762
let splice_locked_1 = if let MessageSendEvent::SendSpliceLocked { msg, .. } = &msg_events[1] {
27462763
msg
27472764
} else {
2748-
panic!("Unexpected event {:?}", msg_events[0]);
2765+
panic!("Unexpected event {:?}", msg_events[1]);
2766+
};
2767+
let revoke_and_ack = if let MessageSendEvent::SendRevokeAndACK { msg, .. } = &msg_events[2] {
2768+
msg
2769+
} else {
2770+
panic!("Unexpected event {:?}", msg_events[2]);
2771+
};
2772+
let commit_sig = if let MessageSendEvent::UpdateHTLCs { updates, .. } = &msg_events[3] {
2773+
assert_eq!(updates.commitment_signed.len(), 1);
2774+
updates.commitment_signed.first().unwrap()
2775+
} else {
2776+
panic!("Unexpected event {:?}", msg_events[3]);
27492777
};
2750-
assert!(matches!(&msg_events[2], MessageSendEvent::SendChannelUpdate { .. }));
2778+
assert!(matches!(&msg_events[4], MessageSendEvent::SendChannelUpdate { .. }));
27512779

27522780
// Deliver node 1's reestablish to node 0. Since it was generated prior to the splice
27532781
// confirmation, it should not promote the splice for node 0 yet.
@@ -2765,6 +2793,21 @@ fn test_promoted_splice_locked_sent_after_channel_reestablish() {
27652793
if funding_txo == Some(new_funding_txo)
27662794
));
27672795

2796+
// And finally, deliver the remaining messages to fully commit the sent HTLC.
2797+
nodes[0].node.handle_revoke_and_ack(node_id_1, revoke_and_ack);
2798+
check_added_monitors(&nodes[0], 1);
2799+
nodes[0].node.handle_commitment_signed(node_id_1, commit_sig);
2800+
check_added_monitors(&nodes[0], 1);
2801+
2802+
let revoke_and_ack = get_event_msg!(&nodes[0], MessageSendEvent::SendRevokeAndACK, node_id_1);
2803+
nodes[1].node.handle_revoke_and_ack(node_id_0, &revoke_and_ack);
2804+
check_added_monitors(&nodes[1], 1);
2805+
nodes[1].node.process_pending_htlc_forwards();
2806+
expect_payment_claimable!(&nodes[1], payment_hash, payment_secret, payment_amount);
2807+
2808+
// We should be able to send payments again now that the state is fully committed.
2809+
send_payment(&nodes[0], &[&nodes[1]], payment_amount);
2810+
27682811
for node in [&nodes[0], &nodes[1]] {
27692812
node.chain_source.remove_watched_by_txid(prev_funding_txo.txid);
27702813
}
@@ -2841,6 +2884,110 @@ fn test_splice_reestablish_waits_for_holder_tx_signatures_before_commitment_sign
28412884
expect_splice_pending_event(&nodes[1], &node_id_0);
28422885
}
28432886

2887+
#[test]
2888+
fn test_splice_reestablish_sends_commitment_signed_before_tx_signatures() {
2889+
let chanmon_cfgs = create_chanmon_cfgs(2);
2890+
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
2891+
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
2892+
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
2893+
2894+
let node_id_0 = nodes[0].node.get_our_node_id();
2895+
let node_id_1 = nodes[1].node.get_our_node_id();
2896+
2897+
let initial_channel_value_sat = 100_000;
2898+
let (_, _, channel_id, _) =
2899+
create_announced_chan_between_nodes_with_value(&nodes, 0, 1, initial_channel_value_sat, 0);
2900+
2901+
let outputs = vec![TxOut {
2902+
value: Amount::from_sat(initial_channel_value_sat / 4),
2903+
script_pubkey: nodes[0].wallet_source.get_change_script().unwrap(),
2904+
}];
2905+
let initiator_contribution =
2906+
initiate_splice_out(&nodes[0], &nodes[1], channel_id, outputs).unwrap();
2907+
negotiate_splice_tx(&nodes[0], &nodes[1], channel_id, initiator_contribution);
2908+
2909+
let signing_event = get_event!(nodes[0], Event::FundingTransactionReadyForSigning);
2910+
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
2911+
2912+
// Drop node 1's initial `commitment_signed` so node 0 requests it on reconnect.
2913+
let acceptor_commit_sig = get_htlc_update_msgs(&nodes[1], &node_id_0);
2914+
assert_eq!(acceptor_commit_sig.commitment_signed.len(), 1);
2915+
2916+
let unsigned_transaction = if let Event::FundingTransactionReadyForSigning {
2917+
unsigned_transaction,
2918+
..
2919+
} = signing_event
2920+
{
2921+
unsigned_transaction
2922+
} else {
2923+
panic!("Expected FundingTransactionReadyForSigning event");
2924+
};
2925+
let tx = nodes[0].wallet_source.sign_tx(unsigned_transaction).unwrap();
2926+
nodes[0].node.funding_transaction_signed(&channel_id, &node_id_1, tx).unwrap();
2927+
check_added_monitors(&nodes[0], 0);
2928+
2929+
let initiator_commit_sig = get_htlc_update_msgs(&nodes[0], &node_id_1);
2930+
nodes[1]
2931+
.node
2932+
.handle_commitment_signed_batch_test(node_id_0, &initiator_commit_sig.commitment_signed);
2933+
check_added_monitors(&nodes[1], 1);
2934+
2935+
// Drop node 1's `tx_signatures`. At this point node 0 has not received node 1's
2936+
// `commitment_signed`, while node 1 has its `tx_signatures` ready, so one
2937+
// `channel_reestablish` should trigger both retransmissions.
2938+
let _ = get_event_msg!(&nodes[1], MessageSendEvent::SendTxSignatures, node_id_0);
2939+
2940+
nodes[0].node.peer_disconnected(node_id_1);
2941+
nodes[1].node.peer_disconnected(node_id_0);
2942+
connect_nodes(&nodes[0], &nodes[1]);
2943+
let reestablish_0 =
2944+
get_event_msg!(nodes[0], MessageSendEvent::SendChannelReestablish, node_id_1);
2945+
let _reestablish_1 =
2946+
get_event_msg!(nodes[1], MessageSendEvent::SendChannelReestablish, node_id_0);
2947+
let next_funding = reestablish_0.next_funding.as_ref().expect("next_funding should be set");
2948+
assert!(next_funding.should_retransmit(msgs::NextFundingFlag::CommitmentSigned));
2949+
2950+
nodes[1].node.handle_channel_reestablish(node_id_0, &reestablish_0);
2951+
let msg_events = nodes[1].node.get_and_clear_pending_msg_events();
2952+
let commitment_update_idx = msg_events
2953+
.iter()
2954+
.position(|event| {
2955+
matches!(event, MessageSendEvent::UpdateHTLCs { updates, .. }
2956+
if updates.commitment_signed.len() == 1)
2957+
})
2958+
.expect("commitment_signed should be retransmitted");
2959+
let tx_signatures_idx = msg_events
2960+
.iter()
2961+
.position(|event| matches!(event, MessageSendEvent::SendTxSignatures { .. }))
2962+
.expect("tx_signatures should be retransmitted");
2963+
assert!(
2964+
commitment_update_idx < tx_signatures_idx,
2965+
"commitment_signed should be retransmitted before tx_signatures: {msg_events:?}"
2966+
);
2967+
2968+
let commitment_signed =
2969+
if let MessageSendEvent::UpdateHTLCs { updates, .. } = &msg_events[commitment_update_idx] {
2970+
updates.commitment_signed.clone()
2971+
} else {
2972+
panic!("Expected UpdateHTLCs");
2973+
};
2974+
let tx_signatures =
2975+
if let MessageSendEvent::SendTxSignatures { msg, .. } = &msg_events[tx_signatures_idx] {
2976+
msg.clone()
2977+
} else {
2978+
panic!("Expected SendTxSignatures");
2979+
};
2980+
nodes[0].node.handle_commitment_signed_batch_test(node_id_1, &commitment_signed);
2981+
check_added_monitors(&nodes[0], 1);
2982+
nodes[0].node.handle_tx_signatures(node_id_1, &tx_signatures);
2983+
let initiator_tx_signatures =
2984+
get_event_msg!(nodes[0], MessageSendEvent::SendTxSignatures, node_id_1);
2985+
nodes[1].node.handle_tx_signatures(node_id_0, &initiator_tx_signatures);
2986+
2987+
expect_splice_pending_event(&nodes[0], &node_id_1);
2988+
expect_splice_pending_event(&nodes[1], &node_id_0);
2989+
}
2990+
28442991
#[test]
28452992
fn test_splice_confirms_on_both_sides_while_disconnected() {
28462993
// Regression test: when a splice transaction confirms on both sides while peers are

0 commit comments

Comments
 (0)