Skip to content

Commit f960afe

Browse files
TheBlueMattTheBlueMatt
committed
Drop Clone from ChannelMonitor.
This removes the somewhat-easy-to-misuse Clone from ChannelMonitors, opening us up to being able to track Events in ChannelMonitors with less risk of misuse. Sadly it doesn't remove the Clone requirement for ChannelKeys, though gets us much closer - we now just need to request a second copy once when we go to create the ChannelMonitors.
1 parent 8ec16e1 commit f960afe

File tree

5 files changed

+51
-32
lines changed

5 files changed

+51
-32
lines changed

‎lightning/src/chain/keysinterface.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,8 @@ pub trait KeysInterface: Send + Sync {
135135
/// (TODO: We shouldn't require that, and should have an API to get them at deser time, due mostly
136136
/// to the possibility of reentrancy issues by calling the user's code during our deserialization
137137
/// routine).
138-
/// TODO: remove Clone once we start returning ChannelUpdate objects instead of copying ChannelMonitor
138+
/// TODO: We should remove Clone by instead requesting a new ChannelKeys copy when we create
139+
/// ChannelMonitors instead of expecting to clone the one out of the Channel into the monitors.
139140
pub trait ChannelKeys : Send+Clone {
140141
/// Gets the private key for the anchor tx
141142
fn funding_key<'a>(&'a self) -> &'a SecretKey;

‎lightning/src/ln/channel.rs‎

Lines changed: 39 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1488,16 +1488,25 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
14881488
let their_pubkeys = self.their_pubkeys.as_ref().unwrap();
14891489
let funding_redeemscript = self.get_funding_redeemscript();
14901490
let funding_txo_script = funding_redeemscript.to_v0_p2wsh();
1491-
self.channel_monitor = Some(ChannelMonitor::new(self.local_keys.clone(),
1492-
&self.shutdown_pubkey, self.our_to_self_delay,
1493-
&self.destination_script, (funding_txo, funding_txo_script),
1494-
&their_pubkeys.htlc_basepoint, &their_pubkeys.delayed_payment_basepoint,
1495-
self.their_to_self_delay, funding_redeemscript, self.channel_value_satoshis,
1496-
self.get_commitment_transaction_number_obscure_factor(),
1497-
self.logger.clone()));
1498-
1499-
self.channel_monitor.as_mut().unwrap().provide_latest_remote_commitment_tx_info(&remote_initial_commitment_tx, Vec::new(), self.cur_remote_commitment_transaction_number, self.their_cur_commitment_point.unwrap());
1500-
self.channel_monitor.as_mut().unwrap().provide_latest_local_commitment_tx_info(local_initial_commitment_tx, local_keys, self.feerate_per_kw, Vec::new()).unwrap();
1491+
macro_rules! create_monitor {
1492+
() => { {
1493+
let mut channel_monitor = ChannelMonitor::new(self.local_keys.clone(),
1494+
&self.shutdown_pubkey, self.our_to_self_delay,
1495+
&self.destination_script, (funding_txo, funding_txo_script.clone()),
1496+
&their_pubkeys.htlc_basepoint, &their_pubkeys.delayed_payment_basepoint,
1497+
self.their_to_self_delay, funding_redeemscript.clone(), self.channel_value_satoshis,
1498+
self.get_commitment_transaction_number_obscure_factor(),
1499+
self.logger.clone());
1500+
1501+
channel_monitor.provide_latest_remote_commitment_tx_info(&remote_initial_commitment_tx, Vec::new(), self.cur_remote_commitment_transaction_number, self.their_cur_commitment_point.unwrap());
1502+
channel_monitor.provide_latest_local_commitment_tx_info(local_initial_commitment_tx.clone(), local_keys.clone(), self.feerate_per_kw, Vec::new()).unwrap();
1503+
channel_monitor
1504+
} }
1505+
}
1506+
1507+
self.channel_monitor = Some(create_monitor!());
1508+
let channel_monitor = create_monitor!();
1509+
15011510
self.channel_state = ChannelState::FundingSent as u32;
15021511
self.channel_id = funding_txo.to_channel_id();
15031512
self.cur_remote_commitment_transaction_number -= 1;
@@ -1506,7 +1515,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
15061515
Ok((msgs::FundingSigned {
15071516
channel_id: self.channel_id,
15081517
signature: our_signature
1509-
}, self.channel_monitor.as_ref().unwrap().clone()))
1518+
}, channel_monitor))
15101519
}
15111520

15121521
/// Handles a funding_signed message from the remote end.
@@ -3330,15 +3339,24 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
33303339
let their_pubkeys = self.their_pubkeys.as_ref().unwrap();
33313340
let funding_redeemscript = self.get_funding_redeemscript();
33323341
let funding_txo_script = funding_redeemscript.to_v0_p2wsh();
3333-
self.channel_monitor = Some(ChannelMonitor::new(self.local_keys.clone(),
3334-
&self.shutdown_pubkey, self.our_to_self_delay,
3335-
&self.destination_script, (funding_txo, funding_txo_script),
3336-
&their_pubkeys.htlc_basepoint, &their_pubkeys.delayed_payment_basepoint,
3337-
self.their_to_self_delay, funding_redeemscript, self.channel_value_satoshis,
3338-
self.get_commitment_transaction_number_obscure_factor(),
3339-
self.logger.clone()));
3340-
3341-
self.channel_monitor.as_mut().unwrap().provide_latest_remote_commitment_tx_info(&commitment_tx, Vec::new(), self.cur_remote_commitment_transaction_number, self.their_cur_commitment_point.unwrap());
3342+
macro_rules! create_monitor {
3343+
() => { {
3344+
let mut channel_monitor = ChannelMonitor::new(self.local_keys.clone(),
3345+
&self.shutdown_pubkey, self.our_to_self_delay,
3346+
&self.destination_script, (funding_txo, funding_txo_script.clone()),
3347+
&their_pubkeys.htlc_basepoint, &their_pubkeys.delayed_payment_basepoint,
3348+
self.their_to_self_delay, funding_redeemscript.clone(), self.channel_value_satoshis,
3349+
self.get_commitment_transaction_number_obscure_factor(),
3350+
self.logger.clone());
3351+
3352+
channel_monitor.provide_latest_remote_commitment_tx_info(&commitment_tx, Vec::new(), self.cur_remote_commitment_transaction_number, self.their_cur_commitment_point.unwrap());
3353+
channel_monitor
3354+
} }
3355+
}
3356+
3357+
self.channel_monitor = Some(create_monitor!());
3358+
let channel_monitor = create_monitor!();
3359+
33423360
self.channel_state = ChannelState::FundingCreated as u32;
33433361
self.channel_id = funding_txo.to_channel_id();
33443362
self.cur_remote_commitment_transaction_number -= 1;
@@ -3348,7 +3366,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
33483366
funding_txid: funding_txo.txid,
33493367
funding_output_index: funding_txo.index,
33503368
signature: our_signature
3351-
}, self.channel_monitor.as_ref().unwrap().clone()))
3369+
}, channel_monitor))
33523370
}
33533371

33543372
/// Gets an UnsignedChannelAnnouncement, as well as a signature covering it using our

‎lightning/src/ln/channelmonitor.rs‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -381,7 +381,6 @@ pub(crate) const LATENCY_GRACE_PERIOD_BLOCKS: u32 = 3;
381381
/// keeping bumping another claim tx to solve the outpoint.
382382
pub(crate) const ANTI_REORG_DELAY: u32 = 6;
383383

384-
#[derive(Clone)]
385384
enum Storage<ChanSigner: ChannelKeys> {
386385
Local {
387386
keys: ChanSigner,
@@ -773,7 +772,6 @@ impl<R: ::std::io::Read> Readable<R> for ChannelMonitorUpdateStep {
773772
///
774773
/// You MUST ensure that no ChannelMonitors for a given channel anywhere contain out-of-date
775774
/// information and are actively monitoring the chain.
776-
#[derive(Clone)]
777775
pub struct ChannelMonitor<ChanSigner: ChannelKeys> {
778776
latest_update_id: u64,
779777
commitment_transaction_number_obscure_factor: u64,

‎lightning/src/ln/functional_tests.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6494,7 +6494,7 @@ fn test_data_loss_protect() {
64946494
}).unwrap().1
64956495
};
64966496
nodes[0].node = &node_state_0;
6497-
assert!(monitor.add_monitor(OutPoint { txid: chan.3.txid(), index: 0 }, chan_monitor.clone()).is_ok());
6497+
assert!(monitor.add_monitor(OutPoint { txid: chan.3.txid(), index: 0 }, chan_monitor).is_ok());
64986498
nodes[0].chan_monitor = &monitor;
64996499
nodes[0].chain_monitor = chain_monitor;
65006500

‎lightning/src/util/test_utils.rs‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -66,13 +66,14 @@ impl channelmonitor::ManyChannelMonitor<EnforcingChannelKeys> for TestChannelMon
6666
// to a watchtower and disk...
6767
let mut w = TestVecWriter(Vec::new());
6868
monitor.write_for_disk(&mut w).unwrap();
69-
assert!(<(Sha256dHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::read(
70-
&mut ::std::io::Cursor::new(&w.0), Arc::new(TestLogger::new())).unwrap().1 == monitor);
69+
let new_monitor = <(Sha256dHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::read(
70+
&mut ::std::io::Cursor::new(&w.0), Arc::new(TestLogger::new())).unwrap().1;
71+
assert!(new_monitor == monitor);
7172
w.0.clear();
7273
monitor.write_for_watchtower(&mut w).unwrap(); // This at least shouldn't crash...
73-
self.added_monitors.lock().unwrap().push((funding_txo, monitor.clone()));
7474
self.latest_monitor_update_id.lock().unwrap().insert(funding_txo.to_channel_id(), (funding_txo, monitor.get_latest_update_id()));
75-
assert!(self.simple_monitor.add_monitor(funding_txo, monitor).is_ok());
75+
self.added_monitors.lock().unwrap().push((funding_txo, monitor));
76+
assert!(self.simple_monitor.add_monitor(funding_txo, new_monitor).is_ok());
7677
self.update_ret.lock().unwrap().clone()
7778
}
7879

@@ -91,11 +92,12 @@ impl channelmonitor::ManyChannelMonitor<EnforcingChannelKeys> for TestChannelMon
9192
let monitor = monitors.get(&funding_txo).unwrap();
9293
w.0.clear();
9394
monitor.write_for_disk(&mut w).unwrap();
94-
assert!(<(Sha256dHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::read(
95-
&mut ::std::io::Cursor::new(&w.0), Arc::new(TestLogger::new())).unwrap().1 == *monitor);
95+
let new_monitor = <(Sha256dHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::read(
96+
&mut ::std::io::Cursor::new(&w.0), Arc::new(TestLogger::new())).unwrap().1;
97+
assert!(new_monitor == *monitor);
9698
w.0.clear();
9799
monitor.write_for_watchtower(&mut w).unwrap(); // This at least shouldn't crash...
98-
self.added_monitors.lock().unwrap().push((funding_txo, monitor.clone()));
100+
self.added_monitors.lock().unwrap().push((funding_txo, new_monitor));
99101
self.update_ret.lock().unwrap().clone()
100102
}
101103

0 commit comments

Comments
 (0)