Uh oh!
There was an error while loading. Please reload this page.
Generate latest local commitment transactions via monitor avoiding Channel's copy - #551
Conversation
6555da7 to
20725f3Compare
ariard
left a comment
There was a problem hiding this comment.
Mostly good, just some function descriptions to update.
| &HTLCUpdateAwaitingACK::ClaimHTLC { htlc_id, .. } => { | ||
| if htlc_id_arg == htlc_id { | ||
| // Make sure we don't leave latest_monitor_update_id incremented here: | ||
| self.latest_monitor_update_id -= 1; |
There was a problem hiding this comment.
Hitting this case would indicate a programming error right ? If so I think function description doesn't match anymore what we really do given we don't return IgnoreError anymore here. You should add a debug_assert too?
There was a problem hiding this comment.
Good call. I fixed the underlying duplicate events, added the debug_assertions, and added a comment noting that it is possible to hit them in some reorg cases.
| log_trace!(self, "Broadcast onchain {}", log_tx!(tx)); | ||
| self.tx_broadcaster.broadcast_transaction(&tx); | ||
| if let Some(funding_txo) = funding_txo_option { | ||
| // XXX: Add comment for why this is OK. |
There was a problem hiding this comment.
"ChannelMonitor tracks the whole channel state, if requested to do so, it will broadcast local commitment transaction and any associated HTLC transactions for which we have a valid witness"
| @@ -3477,9 +3471,9 @@ impl<'a, ChanSigner: ChannelKeys + Readable, M: Deref, T: Deref, K: Deref, F: De | |||
| channel.get_revoked_remote_commitment_transaction_number() != monitor.get_min_seen_secret() || | |||
| channel.get_cur_remote_commitment_transaction_number() != monitor.get_cur_remote_commitment_number() || | |||
| channel.get_latest_monitor_update_id() != monitor.get_latest_update_id() { | |||
There was a problem hiding this comment.
Hmmm I know that's already current behavior but here that means if channel deserialization doesn't match monitor one, we force-close, isn't this an issue in case of monitor being older than channel ? In that case, we're fallen-behind and should wait?
There was a problem hiding this comment.
The monitor should never be behind the channel unless the many-monitor was mis-implemented. The docs are (hopefully) pretty clear in this regard - monitor updates must happen in-sync, channel[manager] updates happen in the background, and only afterwards.
There was a problem hiding this comment.
Can we be defensive here and assert in case of many-monitor being misimplemented we catch issue at ChannelManager deserialization?
| if should_broadcast { | ||
| self.broadcast_latest_local_commitment_txn(broadcaster); | ||
| } else { | ||
| log_error!(self, "You have a toxic local commitment transaction avaible in channel monitor, read comment in ChannelMonitor::get_latest_local_commitment_txn to be informed of manual action to take"); |
There was a problem hiding this comment.
Update get_latest_local_commitment_txn documentation, it's not called anymore by ChannelManager.
There was a problem hiding this comment.
Hmm. It still is, just indirectly (via broadcast_latest_local_commitment_txn). I'll let you update the comment when you refactor this further.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
20725f3 to
42eccfaCompareTheBlueMatt
commented
Mar 19, 2020
I believe all our comments have been addressed. Thanks @ariard! |
ariard
commented
Mar 19, 2020
If we call get_update_fulfill_htlc (in this case via ChannelManager::claim_funds_internal -> Channel::get_update_fulfill_htlc_and_commit) and it finds that we already have a holding-cell pending HTLC claim, it will return no monitor update but leave latest_monitor_update_id incremented. If we later go and add a new monitor update we'll panic as the updates appear to have been applied out-of-order.
This avoids calling get_update_fulfill_htlc_and_commit twice for the same HTLC if we have to rescan a block.
This makes it easier to swap out how we fetch the latest local commitment txn in testing (which we use to check or broadcast old states).
Eventually, we want to remove the Channel's copy of its own ChannelMonitor, reducing memory footprint and complexity of ChannelManager greatly. This removes the last uses of said ChannelMonitor for latest local commitment transactions (though it is still used for would_broadcast_at_height(), which is the last remaining use).
TheBlueMatt
commented
Mar 19, 2020
Only change is I fixed the comment to not refer to IgnoreError, so will merge after travis passes. |
42eccfa to
feca83aCompare
One more step towards removing the Channel-internal channel_monitor, having the ChannelMonitor that is external to the Channel generate/broadcast the latest local commitment transaction on force-close via an update instead of doing it in Channel/Manager. This should make #540 much simpler.