Move to a Monitor-Update return from copying around ChannelMonitors #489

TheBlueMatt · 2020-02-10T19:07:00Z

This tackles #387, migrating us to a much more robust way of handling ChannelMonitors. I still need to rewrite the chanmon_consistency fuzz target based on this, plus maybe rebase it onto master instead of the ever-growing Tower of Babylon. It doesn't quite remove the local ChannelMonitor copy from Channel, which is definitely an important next step, but it gets us most of the way there.

TheBlueMatt · 2020-02-20T20:39:39Z

Resolved fuzz issues and rebased to strip out most other dependencies.

TheBlueMatt · 2020-02-20T20:40:43Z

Now based on: #510, #474 and the top commit from #441 (which should be fine to go in with this).

TheBlueMatt · 2020-02-21T19:05:10Z

Now based directly on master :)

ariard

Okay overall, I really like the direction of changes though need to clarify some changes.

ariard · 2020-02-24T20:01:35Z

lightning/src/ln/channelmanager.rs

+// before we forward it.
+//
+// We will then use HTLCForwardInfo's PendingHTLCInfo to construct an outbound HTLC, with a
+// relevant HTLCSource::PreviousHopData filled in to indicate where it came from (which we can use


d1feefd

Note: information in both HTLCForwardInfo and PreviousHopData are quite overlapping, can't we use same struct at least for metadata and while pending on ChannelManager just include it in a clean action-to-commit+tracking-data new struct (but overall fine with this commit but feel we should rework documentation of HTLC-tracking in a separate PR)

Right, this should make more sense in #441. I played with it a bunch there and really didn't come up with anything good, but we can revisit after #441.

lightning/src/ln/chan_utils.rs

ariard · 2020-02-24T20:17:41Z

lightning/src/ln/channelmonitor.rs

@@ -1021,7 +1013,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 			our_to_self_delay: our_to_self_delay,
 			their_to_self_delay: None,

-			old_secrets: [([0; 32], 1 << 48); 49],
+			commitment_secrets: CounterpartyCommitmentSecrets::new(),


0103d12

I like this change, but even go further later we can pour CounterPartyCommmitmentSecrets behind KeyInterface after key derivation is moved in OnChainTxHandler. That way we can remove revocation secret holder from ChannelMonitor. get_min_seen_secret may be wrap into a is_revoked method, and either accessing revocation secret for local detection or try to decrypt pre-signed justice tx for watchtowers.

Hmm, I dunno about KeyInterface - its a "user provides us static data" trait, not an "updated dynamically as Channel changes" - right now we only have one thing that the user has to be careful about persisting to disk at the appropriate time, adding more sounds super complicated.

No need to do anything now but would revisit when work on watchtowers. In case of leak of commitment_secret + revocation_basepoint_secret, a misbehaving remote peer would be able to alleviate its punishment so an implementor should be able to get it out-of-memory. Not necessarily behind KeyInterface.

ariard · 2020-02-24T20:22:55Z

lightning/src/ln/channel.rs

@@ -348,6 +348,7 @@ pub(super) struct Channel<ChanSigner: ChannelKeys> {
 	their_shutdown_scriptpubkey: Option<Script>,

 	channel_monitor: ChannelMonitor<ChanSigner>,
+	commitment_secrets: CounterpartyCommitmentSecrets,


d905268

Same here, I think that's temporary before to abstract it behind KeyInterface. Revocation secret by themselves are useless if you can't get access to the local revocation_basepoint_secret but I would rather keep them out of memory.

Same here - this is a constantly-changing struct.

ariard · 2020-02-24T20:33:01Z

lightning/src/ln/channelmonitor.rs

-	/// avoid this (or call unset_funding_info) on a monitor you wish to send to a watchtower as it
-	/// provides slightly better privacy.
+	/// avoid this on a monitor you wish to send to a watchtower as it provides slightly better
+	/// privacy.


6cf279b

"(at the price of more computation for watchtower implementation and though privacy enhancement is lost in case of remote broadcast of revoked commitment transaction)"

Ehh, hopefully it doesn't cause more CPU just more blocks to fetch. Note that this is an internal function and, three commits later, this and a bunch of the Option<>-setting set_* fns in ChannelMonitor are removed wholesale.

Hmmm I think it's more CPU because you have to try to decrypt every blob_punishment_txn instead of just looking for a spend of the funding_txo. No need to add comment like you said it's internal function. Watchtowers trade-off should be documented on a higher-level.

ariard · 2020-02-24T22:18:58Z

lightning/src/ln/channel.rs

+								Ok((update_fulfill_msg_option, additional_monitor_update_opt)) => {
+									update_fulfill_htlcs.push(update_fulfill_msg_option.unwrap());
+									if let Some(mut additional_monitor_update) = additional_monitor_update_opt {
+										monitor_update.updates.append(&mut additional_monitor_update.updates);


14d5b97

Shouldn't you decrement latest_monitor_update_id here too given it possible increase in get_update_fulfill_htlc ? (Or rather drop the decrement for next send_commitment_no_status_check as updates_id of monitor_update has already been incremented?)

Hmm, AFAICT we should fix it later in the fn in any case. I don't see a control flow path where we don't, or were you just saying for belt-and-suspenders?

Offline discussion turned up the issue here is that the comments all just refer to send_commitment_no_status_check (even though many other fns may increment it during the runtime here) but the code is correct. Comments have been updated.

ariard · 2020-02-24T22:24:24Z

lightning/src/ln/channelmanager.rs

@@ -2090,7 +2090,16 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
 					if chan.get().get_their_node_id() != *their_node_id {
 						return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!", msg.channel_id));
 					}
-					let monitor_update = try_chan_entry!(self, chan.get_mut().funding_signed(&msg), channel_state, chan);
+					let monitor_update = match chan.get_mut().funding_signed(&msg) {


14d5b97

This part shouldn't be in previous commit or is there anything new in this one which prevent its?

The difference is we're not allowed to lose a monitor update anymore - previously if we were waiting on the user to finish updating a monitor we could just return ChannelError::Ignore("Previous monitor update failure ..."))) as we'd provide them a full copy again later, but now we have to make sure any monitor updates generated are returned even with an Err.

lightning/src/ln/channelmonitor.rs

ariard · 2020-02-24T22:51:34Z

lightning/src/ln/channel.rs

+		                                          self.logger.clone()));
+
+		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());
+		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();


710520b

Can't we just store what we need for broadcast of latest local state and drop ChannelMonitor out of Channel ? States will be only provided with new ChannelMonitorUpdates interfaces? (maybe need to cache basic info though). Also getting the minimum operation state from the Channel at anytime would let you spawn new ChannelMonitor at any-time in channel lifecycle (can we do this right now?)

Can't we just store what we need for broadcast of latest local state and drop ChannelMonitor out of Channel ?

Yep, thats the next step! I don't have a commit for it yet cause it requires some refactoring of ChannelMonitor and I didn't want to step on your toes that much :p.

States will be only provided with new ChannelMonitorUpdates interfaces? (maybe need to cache basic info though). Also getting the minimum operation state from the Channel at anytime would let you spawn new ChannelMonitor at any-time in channel lifecycle (can we do this right now?)

Not sure what you meant by the first part, but, at least in my head, there would be no way to build a ChannelMonitor from a Channel after initialization - the docs for ManyChannelMonitor and ChannelMonitorUpdateErr always require a local store of ChannelMonitors and I don't know that there is much use in trying to get around that.

I think we are in-sync on this, let do this after my ChannelMonitor refactor.

ariard · 2020-02-24T22:53:31Z

lightning/src/ln/channel.rs

@@ -3146,7 +3145,9 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		}
 		if header.bitcoin_hash() != self.last_block_connected {
 			self.last_block_connected = header.bitcoin_hash();
-			self.channel_monitor.last_block_hash = self.last_block_connected;
+			if let Some(channel_monitor) = self.channel_monitor.as_mut() {
+				channel_monitor.last_block_hash = self.last_block_connected;


710520b

I found this a bit confusing given that you already feed ChannelMonitor with block in any ManyChannelMonitor implementation (and our API should allow block skew for watchtowers having a different chain access than client)

This should just be dropped when we remove Channel's local ChannelMonitor copy, its just a historical quirk (and note that the internal copy is never used for anything other than local transaction creation now).

This also renames PendingForwardHTLCInfo to PendingHTLCInfo since it now also encompasses Pending *Received* HTLCs.

In order to drop the ChannelMonitor from Channel, we need to track remote per_commitment_secrets outside of the monitor to validate new ones as they come in. This just moves the current code from ChannelMonitor into a new CounterpartyCommitmentSecrets struct in chan_utils.

In the process of removing a local ChannelMonitor in each Channel, we need to track our counterpartys' commitment secrets so that we can check them locally instead of calling our channel monitor to do that work for us.

Currently Channel relies on its own internal channel_monitor copy to keep track of funding_txo information, which is both a bit awkward and not ideal if we want to get rid of the ChannelMonitor copy in Channel. Instead, just duplicate it (its small) and keep it directly in Channel, allowing us to remove the (super awkward) ChannelMonitor::unset_funding_txo().

ariard · 2020-02-26T23:37:23Z

ACK ddb82e2 minus #489 (comment) after offline discussion

TheBlueMatt · 2020-02-27T00:13:32Z

Diff is:

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 72561313..94e4ae6e 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -1794,11 +1794,12 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref> ChannelManager<ChanSigner, M,
        /// exists largely only to prevent races between this and concurrent update_monitor calls.
        ///
        /// Thus, the anticipated use is, at a high level:
-       ///  1) You register a ManyChannelMonitor with this ChannelManager.
+       ///  1) You register a ManyChannelMonitor with this ChannelManager,
        ///  2) it stores each update to disk, and begins updating any remote (eg watchtower) copies of
        ///     said ChannelMonitors as it can, returning ChannelMonitorUpdateErr::TemporaryFailures
        ///     any time it cannot do so instantly,
-       ///  3) once all remote copies are updated, you call this function with the update_id that
+       ///  3) update(s) are applied to each remote copy of a ChannelMonitor,
+       ///  4) once all remote copies are updated, you call this function with the update_id that
        ///     completed, and once it is the latest the Channel will be re-enabled.
        pub fn channel_monitor_updated(&self, funding_txo: &OutPoint, highest_applied_update_id: u64) {
                let _ = self.total_consistency_lock.read().unwrap();

This is the first step in migrating ChannelMonitor updating logic to use incremental Update objects instead of copying the ChannelMonitors themselves and insert_combine()ing them. This adds most of the scaffolding and updates relevant comments to refer to the new architecture, without changing how any actual updates occur.

This is the first of several steps to update ChannelMonitor updates to use the new ChannelMonitorUpdate objects, demonstrating how the new flow works in Channel.

There is little risk of misusing this as there's not much in the way of other ways you may want to serialize bitcoin::Transaction

This is a rather big step towards using the new ChannelMonitorUpdate flow, using it in the various commitment signing and commitment update message processing functions in Channel. Becase they all often call each other, they all have to be updated as a group, resulting in the somewhat large diff in this commit. In order to keep the update_ids strictly increasing by one for ease of use on the user end, we have to play some games with the latest_monitor_update_id field, though its generally still pretty readable, and the pattern of "get an update_id at the start, and use the one we got at the start when returning, irrespective of what other calls into the Channel during that time did" is relatively straightforward.

This prepares for only creating the ChannelMonitor on funding by removing any channel_monitor calls from Channel open/accept-time to funding-signed time.

This is a rather huge diff, almost entirely due to removing the type parameter from ChannelError which was added in c20e930 due to holding the ChannelKeys in ChannelMonitors.

This removes most of the reliance on ChannelMonitor Clone, creating them in Channel only at the time when we need to start monitoring the chain.

This removes the ability to merge ChannelMonitors in favor of explicit ChannelMonitorUpdates. It further removes ChannelManager::test_restore_channel_monitor in favor of the new ChannelManager::channel_monitor_updated method, which explicitly confirms a set of updates instead of providing the latest copy of each ChannelMonitor to the user. This removes almost all need for Channels to have the latest channel_monitor, except for broadcasting the latest local state.

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.

TheBlueMatt · 2020-02-27T01:03:25Z

Gonna merge to get it off the plate. If you're not happy with any of the docs, zero harm in followup PRs :).

TheBlueMatt force-pushed the 2020-02-chan-updates branch 3 times, most recently from 3fa0e35 to 37beb4c Compare February 13, 2020 04:57

TheBlueMatt added this to the 0.0.10 milestone Feb 19, 2020

TheBlueMatt force-pushed the 2020-02-chan-updates branch 2 times, most recently from 4dfe046 to b70e716 Compare February 20, 2020 20:39

TheBlueMatt marked this pull request as ready for review February 20, 2020 20:39

TheBlueMatt requested a review from ariard February 20, 2020 20:40

TheBlueMatt force-pushed the 2020-02-chan-updates branch 2 times, most recently from 10e52bd to f960afe Compare February 21, 2020 19:05

TheBlueMatt force-pushed the 2020-02-chan-updates branch from f960afe to 5afb7b5 Compare February 21, 2020 22:12

ariard reviewed Feb 24, 2020

View reviewed changes

TheBlueMatt force-pushed the 2020-02-chan-updates branch 2 times, most recently from 104775f to 0541124 Compare February 26, 2020 03:56

TheBlueMatt modified the milestones: 0.0.10, 0.0.11 Feb 26, 2020

TheBlueMatt added 4 commits February 26, 2020 17:48

Clarify the in-flight HTLC state-tracking structs a bit.

72e32e7

This also renames PendingForwardHTLCInfo to PendingHTLCInfo since it now also encompasses Pending *Received* HTLCs.

Track counterparty's commitment secrets in Channel directly.

6296eb1

In the process of removing a local ChannelMonitor in each Channel, we need to track our counterpartys' commitment secrets so that we can check them locally instead of calling our channel monitor to do that work for us.

TheBlueMatt force-pushed the 2020-02-chan-updates branch from 0541124 to ddb82e2 Compare February 26, 2020 23:04

TheBlueMatt force-pushed the 2020-02-chan-updates branch from ddb82e2 to c1aebf7 Compare February 27, 2020 00:13

TheBlueMatt added 3 commits February 26, 2020 19:15

Update Channel::funding_signed to use ChannelMonitorUpdate

8c69bb1

This is the first of several steps to update ChannelMonitor updates to use the new ChannelMonitorUpdate objects, demonstrating how the new flow works in Channel.

Impl (de)serialization for bitcoin::Transaction.

569f903

There is little risk of misusing this as there's not much in the way of other ways you may want to serialize bitcoin::Transaction

TheBlueMatt added 7 commits February 26, 2020 19:15

Set ChannelMonitor basic_channel_info on funding, not on accept

537bd35

This prepares for only creating the ChannelMonitor on funding by removing any channel_monitor calls from Channel open/accept-time to funding-signed time.

Use ChannelMonitorUpdate in fallen-behind handling during reestablish

f930fc1

This is a rather huge diff, almost entirely due to removing the type parameter from ChannelError which was added in c20e930 due to holding the ChannelKeys in ChannelMonitors.

Create ChannelMonitors with basic_channel_info and funding_info set

6caed7d

This removes most of the reliance on ChannelMonitor Clone, creating them in Channel only at the time when we need to start monitoring the chain.

Drop TODO which was implemented long ago

08db88c

TheBlueMatt force-pushed the 2020-02-chan-updates branch from c1aebf7 to 08db88c Compare February 27, 2020 00:16

TheBlueMatt merged commit 030c49c into lightningdevkit:master Feb 27, 2020

Move to a Monitor-Update return from copying around ChannelMonitors #489

Move to a Monitor-Update return from copying around ChannelMonitors #489

Uh oh!

Conversation

TheBlueMatt commented Feb 10, 2020

Uh oh!

TheBlueMatt commented Feb 20, 2020

Uh oh!

TheBlueMatt commented Feb 20, 2020

Uh oh!

TheBlueMatt commented Feb 21, 2020

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Feb 26, 2020

Uh oh!

TheBlueMatt commented Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Feb 27, 2020

Uh oh!

Uh oh!

TheBlueMatt Feb 25, 2020 •

edited

Loading

TheBlueMatt Feb 25, 2020 •

edited

Loading

TheBlueMatt commented Feb 27, 2020 •

edited

Loading