Skip to content

Drop the redundant/broken ChannelMonitor::get_monitored_outpoints #722

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 5, 2020
Merged

Drop the redundant/broken ChannelMonitor::get_monitored_outpoints #722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a162840
Drop the redundant/broken `ChannelMonitor::get_monitored_outpoints`
TheBlueMatt Sep 27, 2020
8e99800
Drop now-unused Vec of outpoints in remote-commitment-tx-tracking
TheBlueMatt Sep 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 13 additions & 32 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,7 @@ pub struct ChannelMonitor<ChanSigner: ChannelKeys> {
/// spending. Thus, in order to claim them via revocation key, we track all the counterparty
/// commitment transactions which we find on-chain, mapping them to the commitment number which
/// can be used to derive the revocation key and claim the transactions.
counterparty_commitment_txn_on_chain: HashMap<Txid, (u64, Vec<Script>)>,
counterparty_commitment_txn_on_chain: HashMap<Txid, u64>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this map be dropped now? The commitment number can be decoded from the commitment TX via:

        let commitment_number = (((commitment_tx.input[0].sequence as u64 & 0xffffff) << 3 * 8)
            | (commitment_tx.lock_time as u64 & 0xffffff))
            ^ obscure_factor;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I guess this is for sending watchtowers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its used to figure out the commitment number given an HTLC tx (ie by looking at the previous output spent), which I don't believe is otherwise possible. Any suggestions for updating the comment to make that more clear?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must be missing something. We cannot see HTLC transactions on-chain before we see the commitment tx on-chain and there can only be one of those. Once we see the commitment tx, we can decode and stash the commitment number in variable in this struct, so we have it for the HTLC tx. So I still don't see the need for a map.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, there was some Slack discussion on this, but, yes, ultimately we could track only the latest remote commitment tx we saw on-chain (because to invalidate that we'd need a reorg, invalidating child HTLC txn too). The code to do that would likely be somewhat simpler, but I'd really like more testing in ChannelMonitor, so am ~0/-0 on such a change, at least not gonna jump to implement it myself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for summarizing the discussion, I might give it a try at some point.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi @devrandom this map was introduced before we had any serious reorg-handling in ChannelMonitor and thus it was simpler at that time to just have a dumb map instead of stashing reorg'ed commitment.

A real improvement would be instead to store a tuple of (commitment_txid, per_commitment_point, per_commitment_key) as it would avoid to fetch the secret holder and re-derive a per-commitment each time we see a revoked child HTLC. It could be done once for all when we see the commitment. I think.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I think you can remove the first line of comment "We cannot identify HTLC-Success or HTLC-Timeout transactions by themselves on the chain." In fact we should be able to do so if we watch well commitment outputs. There is a line of further parsing in check_spend_counterparty_htlc to qualify them well as HTLCs.

Copy link
Collaborator Author

@TheBlueMatt TheBlueMatt Oct 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused by the second comment there? Its true we can't identify them "by themselves", that's why we have the map, no? (and check_spend_counterparty_htlc is only called after looking up in the map).

/// Cache used to make pruning of payment_preimages faster.
/// Maps payment_hash values to commitment numbers for counterparty transactions for non-revoked
/// counterparty transactions (ie should remain pretty small).
Expand Down Expand Up @@ -824,13 +824,9 @@ impl<ChanSigner: ChannelKeys + Writeable> ChannelMonitor<ChanSigner> {
}

writer.write_all(&byte_utils::be64_to_array(self.counterparty_commitment_txn_on_chain.len() as u64))?;
for (ref txid, &(commitment_number, ref txouts)) in self.counterparty_commitment_txn_on_chain.iter() {
for (ref txid, commitment_number) in self.counterparty_commitment_txn_on_chain.iter() {
writer.write_all(&txid[..])?;
writer.write_all(&byte_utils::be48_to_array(commitment_number))?;
(txouts.len() as u64).write(writer)?;
for script in txouts.iter() {
script.write(writer)?;
}
writer.write_all(&byte_utils::be48_to_array(*commitment_number))?;
}

writer.write_all(&byte_utils::be64_to_array(self.counterparty_hash_commitment_number.len() as u64))?;
Expand Down Expand Up @@ -1214,23 +1210,13 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
///
/// (C-not exported) because we have no HashMap bindings
pub fn get_outputs_to_watch(&self) -> &HashMap<Txid, Vec<Script>> {
&self.outputs_to_watch
}

/// Gets the sets of all outpoints which this ChannelMonitor expects to hear about spends of.
/// Generally useful when deserializing as during normal operation the return values of
/// block_connected are sufficient to ensure all relevant outpoints are being monitored (note
/// that the get_funding_txo outpoint and transaction must also be monitored for!).
///
/// (C-not exported) as there is no practical way to track lifetimes of returned values.
pub fn get_monitored_outpoints(&self) -> Vec<(Txid, u32, &Script)> {
let mut res = Vec::with_capacity(self.counterparty_commitment_txn_on_chain.len() * 2);
for (ref txid, &(_, ref outputs)) in self.counterparty_commitment_txn_on_chain.iter() {
for (idx, output) in outputs.iter().enumerate() {
res.push(((*txid).clone(), idx as u32, output));
}
// If we've detected a counterparty commitment tx on chain, we must include it in the set
// of outputs to watch for spends of, otherwise we're likely to lose user funds. Because
// its trivial to do, double-check that here.
for (txid, _) in self.counterparty_commitment_txn_on_chain.iter() {
self.outputs_to_watch.get(txid).expect("Counterparty commitment txn which have been broadcast should have outputs registered");
}
res
&self.outputs_to_watch
}

/// Get the list of HTLCs who's status has been updated on chain. This should be called by
Expand Down Expand Up @@ -1334,7 +1320,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
// We're definitely a counterparty commitment transaction!
log_trace!(logger, "Got broadcast of revoked counterparty commitment transaction, going to generate general spend tx with {} inputs", claimable_outpoints.len());
watch_outputs.append(&mut tx.output.clone());
self.counterparty_commitment_txn_on_chain.insert(commitment_txid, (commitment_number, tx.output.iter().map(|output| { output.script_pubkey.clone() }).collect()));
self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number);

macro_rules! check_htlc_fails {
($txid: expr, $commitment_tx: expr) => {
Expand Down Expand Up @@ -1381,7 +1367,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
// not being generated by the above conditional. Thus, to be safe, we go ahead and
// insert it here.
watch_outputs.append(&mut tx.output.clone());
self.counterparty_commitment_txn_on_chain.insert(commitment_txid, (commitment_number, tx.output.iter().map(|output| { output.script_pubkey.clone() }).collect()));
self.counterparty_commitment_txn_on_chain.insert(commitment_txid, commitment_number);

log_trace!(logger, "Got broadcast of non-revoked counterparty commitment transaction {}", commitment_txid);

Expand Down Expand Up @@ -1719,7 +1705,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
claimable_outpoints.append(&mut new_outpoints);
}
} else {
if let Some(&(commitment_number, _)) = self.counterparty_commitment_txn_on_chain.get(&prevout.txid) {
if let Some(&commitment_number) = self.counterparty_commitment_txn_on_chain.get(&prevout.txid) {
let (mut new_outpoints, new_outputs_option) = self.check_spend_counterparty_htlc(&tx, commitment_number, height, &logger);
claimable_outpoints.append(&mut new_outpoints);
if let Some(new_outputs) = new_outputs_option {
Expand Down Expand Up @@ -2211,12 +2197,7 @@ impl<ChanSigner: ChannelKeys + Readable> Readable for (BlockHash, ChannelMonitor
for _ in 0..counterparty_commitment_txn_on_chain_len {
let txid: Txid = Readable::read(reader)?;
let commitment_number = <U48 as Readable>::read(reader)?.0;
let outputs_count = <u64 as Readable>::read(reader)?;
let mut outputs = Vec::with_capacity(cmp::min(outputs_count as usize, MAX_ALLOC_SIZE / 8));
for _ in 0..outputs_count {
outputs.push(Readable::read(reader)?);
}
if let Some(_) = counterparty_commitment_txn_on_chain.insert(txid, (commitment_number, outputs)) {
if let Some(_) = counterparty_commitment_txn_on_chain.insert(txid, commitment_number) {
return Err(DecodeError::InvalidValue);
}
}
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3730,7 +3730,7 @@ impl<ChanSigner: ChannelKeys + Writeable, M: Deref, T: Deref, K: Deref, F: Deref
/// This may result in closing some Channels if the ChannelMonitor is newer than the stored
/// ChannelManager state to ensure no loss of funds. Thus, transactions may be broadcasted.
/// 3) Register all relevant ChannelMonitor outpoints with your chain watch mechanism using
/// ChannelMonitor::get_monitored_outpoints and ChannelMonitor::get_funding_txo().
/// ChannelMonitor::get_outputs_to_watch() and ChannelMonitor::get_funding_txo().
/// 4) Reconnect blocks on your ChannelMonitors.
/// 5) Move the ChannelMonitors into your local chain::Watch.
/// 6) Disconnect/connect blocks on the ChannelManager.
Expand Down