Skip to content

Commit c36081e

Browse files
wpaulinowpaulino
committed
Use channel ID over funding outpoint to track monitors in ChainMonitor
Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify `ChannelMonitor`s with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime. This commit replaces the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.
1 parent ad462bd commit c36081e

12 files changed

+322
-329
lines changed

‎lightning/src/chain/chainmonitor.rs‎

Lines changed: 78 additions & 82 deletions
Large diffs are not rendered by default.

‎lightning/src/chain/mod.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,7 @@ pub trait Watch<ChannelSigner: EcdsaChannelSigner> {
276276
/// [`get_outputs_to_watch`]: channelmonitor::ChannelMonitor::get_outputs_to_watch
277277
/// [`block_connected`]: channelmonitor::ChannelMonitor::block_connected
278278
/// [`block_disconnected`]: channelmonitor::ChannelMonitor::block_disconnected
279-
fn watch_channel(&self, funding_txo: OutPoint, monitor: ChannelMonitor<ChannelSigner>) -> Result<ChannelMonitorUpdateStatus, ()>;
279+
fn watch_channel(&self, channel_id: ChannelId, monitor: ChannelMonitor<ChannelSigner>) -> Result<ChannelMonitorUpdateStatus, ()>;
280280

281281
/// Updates a channel identified by `funding_txo` by applying `update` to its monitor.
282282
///
@@ -293,7 +293,7 @@ pub trait Watch<ChannelSigner: EcdsaChannelSigner> {
293293
/// [`ChannelMonitorUpdateStatus::UnrecoverableError`], see its documentation for more info.
294294
///
295295
/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
296-
fn update_channel(&self, funding_txo: OutPoint, update: &ChannelMonitorUpdate) -> ChannelMonitorUpdateStatus;
296+
fn update_channel(&self, channel_id: ChannelId, update: &ChannelMonitorUpdate) -> ChannelMonitorUpdateStatus;
297297

298298
/// Returns any monitor events since the last call. Subsequent calls must only return new
299299
/// events.

‎lightning/src/ln/async_signer_tests.rs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -489,8 +489,8 @@ fn do_test_async_raa_peer_disconnect(test_case: UnblockSignerAcrossDisconnectCas
489489
if test_case == UnblockSignerAcrossDisconnectCase::BeforeMonitorRestored {
490490
dst.enable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, block_raa_signer_op);
491491
chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
492-
let (outpoint, latest_update, _) = dst.chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
493-
dst.chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, latest_update);
492+
let (latest_update, _) = dst.chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
493+
dst.chain_monitor.chain_monitor.force_channel_monitor_updated(chan_id, latest_update);
494494
check_added_monitors!(dst, 0);
495495
}
496496

@@ -614,8 +614,8 @@ fn do_test_async_commitment_signature_peer_disconnect(test_case: UnblockSignerAc
614614
if test_case == UnblockSignerAcrossDisconnectCase::BeforeMonitorRestored {
615615
dst.enable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, SignerOp::SignCounterpartyCommitment);
616616
chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
617-
let (outpoint, latest_update, _) = dst.chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
618-
dst.chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, latest_update);
617+
let (latest_update, _) = dst.chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
618+
dst.chain_monitor.chain_monitor.force_channel_monitor_updated(chan_id, latest_update);
619619
check_added_monitors!(dst, 0);
620620
}
621621

@@ -737,8 +737,8 @@ fn do_test_async_commitment_signature_ordering(monitor_update_failure: bool) {
737737

738738
if monitor_update_failure {
739739
chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
740-
let (outpoint, latest_update, _) = nodes[0].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
741-
nodes[0].chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, latest_update);
740+
let (latest_update, _) = nodes[0].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
741+
nodes[0].chain_monitor.chain_monitor.force_channel_monitor_updated(chan_id, latest_update);
742742
check_added_monitors!(nodes[0], 0);
743743
}
744744

‎lightning/src/ln/chanmon_update_fail_tests.rs‎

Lines changed: 61 additions & 63 deletions
Large diffs are not rendered by default.

‎lightning/src/ln/channelmanager.rs‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1697,7 +1697,7 @@ where
16971697
///
16981698
/// // Move the monitors to the ChannelManager's chain::Watch parameter
16991699
/// for monitor in channel_monitors {
1700-
/// chain_monitor.watch_channel(monitor.get_funding_txo().0, monitor);
1700+
/// chain_monitor.watch_channel(monitor.channel_id(), monitor);
17011701
/// }
17021702
/// # Ok(())
17031703
/// # }
@@ -3292,7 +3292,7 @@ macro_rules! handle_new_monitor_update {
32923292
$in_flight_updates.len() - 1
32933293
});
32943294
if $self.background_events_processed_since_startup.load(Ordering::Acquire) {
3295-
let update_res = $self.chain_monitor.update_channel($funding_txo, &$in_flight_updates[$update_idx]);
3295+
let update_res = $self.chain_monitor.update_channel($chan_id, &$in_flight_updates[$update_idx]);
32963296
handle_new_monitor_update!($self, update_res, $logger, $chan_id, _internal, $completed)
32973297
} else {
32983298
// We blindly assume that the ChannelMonitorUpdate will be regenerated on startup if we
@@ -6299,10 +6299,10 @@ where
62996299

63006300
for event in background_events.drain(..) {
63016301
match event {
6302-
BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup((funding_txo, _channel_id, update)) => {
6302+
BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup((_funding_txo, channel_id, update)) => {
63036303
// The channel has already been closed, so no use bothering to care about the
63046304
// monitor updating completing.
6305-
let _ = self.chain_monitor.update_channel(funding_txo, &update);
6305+
let _ = self.chain_monitor.update_channel(channel_id, &update);
63066306
},
63076307
BackgroundEvent::MonitorUpdateRegeneratedOnStartup { counterparty_node_id, funding_txo, channel_id, update } => {
63086308
self.apply_post_close_monitor_update(counterparty_node_id, channel_id, funding_txo, update);
@@ -8108,7 +8108,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
81088108
fail_chan!("The funding_created message had the same funding_txid as an existing channel - funding is not possible");
81098109
},
81108110
hash_map::Entry::Vacant(i_e) => {
8111-
let monitor_res = self.chain_monitor.watch_channel(monitor.get_funding_txo().0, monitor);
8111+
let monitor_res = self.chain_monitor.watch_channel(monitor.channel_id(), monitor);
81128112
if let Ok(persist_state) = monitor_res {
81138113
i_e.insert(chan.context.get_counterparty_node_id());
81148114
mem::drop(outpoint_to_peer_lock);
@@ -8167,7 +8167,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
81678167
chan.funding_signed(&msg, best_block, &self.signer_provider, &&logger);
81688168
match res {
81698169
Ok((mut chan, monitor)) => {
8170-
if let Ok(persist_status) = self.chain_monitor.watch_channel(chan.context.get_funding_txo().unwrap(), monitor) {
8170+
if let Ok(persist_status) = self.chain_monitor.watch_channel(chan.context.channel_id(), monitor) {
81718171
// We really should be able to insert here without doing a second
81728172
// lookup, but sadly rust stdlib doesn't currently allow keeping
81738173
// the original Entry around with the value removed.
@@ -8852,7 +8852,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
88528852
let monitor = try_chan_phase_entry!(
88538853
self, peer_state, chan.commitment_signed_initial_v2(msg, best_block, &self.signer_provider, &&logger),
88548854
chan_phase_entry);
8855-
let monitor_res = self.chain_monitor.watch_channel(monitor.get_funding_txo().0, monitor);
8855+
let monitor_res = self.chain_monitor.watch_channel(monitor.channel_id(), monitor);
88568856
if let Ok(persist_state) = monitor_res {
88578857
handle_new_monitor_update!(self, persist_state, peer_state_lock, peer_state,
88588858
per_peer_state, chan, INITIAL_MONITOR);

‎lightning/src/ln/functional_test_utils.rs‎

Lines changed: 18 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -255,8 +255,8 @@ pub fn connect_block<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, block: &Block)
255255

256256
fn call_claimable_balances<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>) {
257257
// Ensure `get_claimable_balances`' self-tests never panic
258-
for (funding_outpoint, _channel_id) in node.chain_monitor.chain_monitor.list_monitors() {
259-
node.chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances();
258+
for channel_id in node.chain_monitor.chain_monitor.list_monitors() {
259+
node.chain_monitor.chain_monitor.get_monitor(channel_id).unwrap().get_claimable_balances();
260260
}
261261
}
262262

@@ -554,9 +554,7 @@ impl<'a, 'b, 'c> Node<'a, 'b, 'c> {
554554
channel_keys_id = Some(chan.channel_keys_id);
555555
}
556556

557-
let monitor = self.chain_monitor.chain_monitor.list_monitors().into_iter()
558-
.find(|(_, channel_id)| *channel_id == *chan_id)
559-
.and_then(|(funding_txo, _)| self.chain_monitor.chain_monitor.get_monitor(funding_txo).ok());
557+
let monitor = self.chain_monitor.chain_monitor.get_monitor(*chan_id).ok();
560558
if let Some(monitor) = monitor {
561559
monitor.do_mut_signer_call(|signer| {
562560
channel_keys_id = channel_keys_id.or(Some(signer.inner.channel_keys_id()));
@@ -690,9 +688,9 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> {
690688
let feeest = test_utils::TestFeeEstimator::new(253);
691689
let mut deserialized_monitors = Vec::new();
692690
{
693-
for (outpoint, _channel_id) in self.chain_monitor.chain_monitor.list_monitors() {
691+
for channel_id in self.chain_monitor.chain_monitor.list_monitors() {
694692
let mut w = test_utils::TestVecWriter(Vec::new());
695-
self.chain_monitor.chain_monitor.get_monitor(outpoint).unwrap().write(&mut w).unwrap();
693+
self.chain_monitor.chain_monitor.get_monitor(channel_id).unwrap().write(&mut w).unwrap();
696694
let (_, deserialized_monitor) = <(BlockHash, ChannelMonitor<TestChannelSigner>)>::read(
697695
&mut io::Cursor::new(&w.0), (self.keys_manager, self.keys_manager)).unwrap();
698696
deserialized_monitors.push(deserialized_monitor);
@@ -734,8 +732,8 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> {
734732
let chain_source = test_utils::TestChainSource::new(Network::Testnet);
735733
let chain_monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &broadcaster, &self.logger, &feeest, &persister, &self.keys_manager);
736734
for deserialized_monitor in deserialized_monitors.drain(..) {
737-
let funding_outpoint = deserialized_monitor.get_funding_txo().0;
738-
if chain_monitor.watch_channel(funding_outpoint, deserialized_monitor) != Ok(ChannelMonitorUpdateStatus::Completed) {
735+
let channel_id = deserialized_monitor.channel_id();
736+
if chain_monitor.watch_channel(channel_id, deserialized_monitor) != Ok(ChannelMonitorUpdateStatus::Completed) {
739737
panic!();
740738
}
741739
}
@@ -1033,20 +1031,7 @@ macro_rules! get_channel_type_features {
10331031
macro_rules! get_monitor {
10341032
($node: expr, $channel_id: expr) => {
10351033
{
1036-
use bitcoin::hashes::Hash;
1037-
let mut monitor = None;
1038-
// Assume funding vout is either 0 or 1 blindly
1039-
for index in 0..2 {
1040-
if let Ok(mon) = $node.chain_monitor.chain_monitor.get_monitor(
1041-
$crate::chain::transaction::OutPoint {
1042-
txid: bitcoin::Txid::from_slice(&$channel_id.0[..]).unwrap(), index
1043-
})
1044-
{
1045-
monitor = Some(mon);
1046-
break;
1047-
}
1048-
}
1049-
monitor.unwrap()
1034+
$node.chain_monitor.chain_monitor.get_monitor($channel_id).unwrap()
10501035
}
10511036
}
10521037
}
@@ -1170,8 +1155,8 @@ pub fn _reload_node<'a, 'b, 'c>(node: &'a Node<'a, 'b, 'c>, default_config: User
11701155
assert!(node_read.is_empty());
11711156

11721157
for monitor in monitors_read.drain(..) {
1173-
let funding_outpoint = monitor.get_funding_txo().0;
1174-
assert_eq!(node.chain_monitor.watch_channel(funding_outpoint, monitor),
1158+
let channel_id = monitor.channel_id();
1159+
assert_eq!(node.chain_monitor.watch_channel(channel_id, monitor),
11751160
Ok(ChannelMonitorUpdateStatus::Completed));
11761161
check_added_monitors!(node, 1);
11771162
}
@@ -1275,19 +1260,24 @@ pub fn create_dual_funding_utxos_with_prev_txs(
12751260
}
12761261

12771262
pub fn sign_funding_transaction<'a, 'b, 'c>(node_a: &Node<'a, 'b, 'c>, node_b: &Node<'a, 'b, 'c>, channel_value: u64, expected_temporary_channel_id: ChannelId) -> Transaction {
1278-
let (temporary_channel_id, tx, funding_output) = create_funding_transaction(node_a, &node_b.node.get_our_node_id(), channel_value, 42);
1263+
let (temporary_channel_id, tx, _) = create_funding_transaction(node_a, &node_b.node.get_our_node_id(), channel_value, 42);
12791264
assert_eq!(temporary_channel_id, expected_temporary_channel_id);
12801265

12811266
assert!(node_a.node.funding_transaction_generated(temporary_channel_id, node_b.node.get_our_node_id(), tx.clone()).is_ok());
12821267
check_added_monitors!(node_a, 0);
12831268

12841269
let funding_created_msg = get_event_msg!(node_a, MessageSendEvent::SendFundingCreated, node_b.node.get_our_node_id());
12851270
assert_eq!(funding_created_msg.temporary_channel_id, expected_temporary_channel_id);
1271+
1272+
let channel_id = ChannelId::v1_from_funding_txid(
1273+
funding_created_msg.funding_txid.as_byte_array(), funding_created_msg.funding_output_index
1274+
);
1275+
12861276
node_b.node.handle_funding_created(node_a.node.get_our_node_id(), &funding_created_msg);
12871277
{
12881278
let mut added_monitors = node_b.chain_monitor.added_monitors.lock().unwrap();
12891279
assert_eq!(added_monitors.len(), 1);
1290-
assert_eq!(added_monitors[0].0, funding_output);
1280+
assert_eq!(added_monitors[0].0, channel_id);
12911281
added_monitors.clear();
12921282
}
12931283
expect_channel_pending_event(&node_b, &node_a.node.get_our_node_id());
@@ -1296,7 +1286,7 @@ pub fn sign_funding_transaction<'a, 'b, 'c>(node_a: &Node<'a, 'b, 'c>, node_b: &
12961286
{
12971287
let mut added_monitors = node_a.chain_monitor.added_monitors.lock().unwrap();
12981288
assert_eq!(added_monitors.len(), 1);
1299-
assert_eq!(added_monitors[0].0, funding_output);
1289+
assert_eq!(added_monitors[0].0, channel_id);
13001290
added_monitors.clear();
13011291
}
13021292
expect_channel_pending_event(&node_a, &node_b.node.get_our_node_id());

0 commit comments

Comments
 (0)