Skip to content

Commit a162840

Browse files
committed
Drop the redundant/broken ChannelMonitor::get_monitored_outpoints
In review of the final doc changes in #649, I noticed there appeared to be redundant monitored-outpoints function in `ChannelMonitor` - `get_monitored_outpoints()` and `get_outputs_to_watch()`. In 6f08779, get_monitored_outpoints() was added, with its behavior largely the same as today's - only returning the set of remote commitment txn outputs that we've learned about on-chain. This is clearly not sufficient, and in 73dce20, `get_outputs_to_watch` was added which was overly cautious to ensure nothing was missed. Still, the author of 73dce20 (me) seemed entirely unaware of the work in 6f08779 (also me), despite the function being the literal next function in the same file. This is presumably because it was assumed that `get_monitored_outpoints` referred to oupoints for which we should monitor for spends of (which is true), while `get_outputs_to_watch` referred to outpouts which we should monitor for the transaction containing said output (which is not true), or something of that nature. Specifically, it is the expected behavior that the only time we care about `Filter::register_tx` is for the funding transaction (which we aren't aware of the inputs of), but for all other transactions we register interest on the basis of an outpoint in the previous transaction (ie via `Filter::register_output`). Here we drop the broken-on-day-one `get_monitored_outpoints()` version, but assert in testing that the values which it would return are all present in `get_outputs_to_watch()`.
1 parent 45a558b commit a162840

File tree

2 files changed

+10
-16
lines changed

2 files changed

+10
-16
lines changed

lightning/src/chain/channelmonitor.rs

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1214,23 +1214,17 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
12141214
///
12151215
/// (C-not exported) because we have no HashMap bindings
12161216
pub fn get_outputs_to_watch(&self) -> &HashMap<Txid, Vec<Script>> {
1217-
&self.outputs_to_watch
1218-
}
1219-
1220-
/// Gets the sets of all outpoints which this ChannelMonitor expects to hear about spends of.
1221-
/// Generally useful when deserializing as during normal operation the return values of
1222-
/// block_connected are sufficient to ensure all relevant outpoints are being monitored (note
1223-
/// that the get_funding_txo outpoint and transaction must also be monitored for!).
1224-
///
1225-
/// (C-not exported) as there is no practical way to track lifetimes of returned values.
1226-
pub fn get_monitored_outpoints(&self) -> Vec<(Txid, u32, &Script)> {
1227-
let mut res = Vec::with_capacity(self.counterparty_commitment_txn_on_chain.len() * 2);
1228-
for (ref txid, &(_, ref outputs)) in self.counterparty_commitment_txn_on_chain.iter() {
1229-
for (idx, output) in outputs.iter().enumerate() {
1230-
res.push(((*txid).clone(), idx as u32, output));
1217+
// If we've detected a counterparty commitment tx on chain, we must include it in the set
1218+
// of outputs to watch for spends of, otherwise we're likely to lose user funds. Because
1219+
// its trivial to do, double-check that here.
1220+
for (txid, &(_, ref outputs)) in self.counterparty_commitment_txn_on_chain.iter() {
1221+
let watched_outputs = self.outputs_to_watch.get(txid).expect("Counterparty commitment txn which have been broadcast should have outputs registered");
1222+
assert_eq!(watched_outputs.len(), outputs.len());
1223+
for (watched, output) in watched_outputs.iter().zip(outputs.iter()) {
1224+
assert_eq!(watched, output);
12311225
}
12321226
}
1233-
res
1227+
&self.outputs_to_watch
12341228
}
12351229

12361230
/// Get the list of HTLCs who's status has been updated on chain. This should be called by

lightning/src/ln/channelmanager.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3730,7 +3730,7 @@ impl<ChanSigner: ChannelKeys + Writeable, M: Deref, T: Deref, K: Deref, F: Deref
37303730
/// This may result in closing some Channels if the ChannelMonitor is newer than the stored
37313731
/// ChannelManager state to ensure no loss of funds. Thus, transactions may be broadcasted.
37323732
/// 3) Register all relevant ChannelMonitor outpoints with your chain watch mechanism using
3733-
/// ChannelMonitor::get_monitored_outpoints and ChannelMonitor::get_funding_txo().
3733+
/// ChannelMonitor::get_outputs_to_watch() and ChannelMonitor::get_funding_txo().
37343734
/// 4) Reconnect blocks on your ChannelMonitors.
37353735
/// 5) Move the ChannelMonitors into your local chain::Watch.
37363736
/// 6) Disconnect/connect blocks on the ChannelManager.

0 commit comments

Comments
 (0)