Track the full list of outpoints a chanmon wants monitoring for #455

TheBlueMatt · 2020-01-21T03:18:50Z

Upon deserialization/reload we need to be able to register each
outpoint which spends the commitment txo which a channelmonitor
believes to be on chain. While our other internal tracking is
likely sufficient to regenerate these, its much easier to simply
track all outpouts we've ever generated, so we do that here.

arik-so · 2020-02-10T20:43:16Z

lightning/src/ln/channelmonitor.rs

+	// interface knows about the TXOs that we want to be notified of spends of. We could probably
+	// be smart and derive them from the above storage fields, but its much simpler and more
+	// Obviously Correct (tm) if we just keep track of them explicitly.
+	watch_outputs: HashMap<Sha256dHash, Vec<Script>>,


nit: watch_outputs sounds more like a boolean. watched_ or monitored_ might be more explicit

Hmm, I dont want to imply that they are watched, only that they must be watched. Went with outputs_to_watch.

arik-so · 2020-02-10T20:45:39Z

lightning/src/ln/channelmonitor.rs

@@ -966,6 +982,15 @@ impl<ChanSigner: ChannelKeys + Writeable> ChannelMonitor<ChanSigner> {
 			}
 		}

+		(self.watch_outputs.len() as u64).write(writer)?;


this is quite the write function. It might be prudent to break it out into subroutines so it's easier to navigate.

Agreed. We should probably implement serialize on subtypes so that we can just call the write derive macro, but its not particularly worse here, just still-bad.

arik-so · 2020-02-10T20:49:09Z

lightning/src/ln/channelmonitor.rs

@@ -2589,6 +2621,9 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 			}
 		}
 		self.last_block_hash = block_hash.clone();
+		for &(ref txid, ref output_scripts) in watch_outputs.iter() {


block_connected could definitely use some comments. Is this a function that's supposed to be called on block connection?

Its called by ManyChannelMonitor::block_connected, which is a https://docs.rs/lightning/0.0.10/lightning/chain/chaininterface/trait.ChainListener.html#tymethod.block_connected . Ideally ChannelMonitor would implement ChainListener too, but we're a ways away from that - #474 implements one step, though it still returns two other values (unlike ChainListener). I'll add a commit that comments this state.

ariard

Hmmm I dunno about adding yet-another field for data we already track elsewhere. On the other side it makes testing really easy because if we reuse existent fields we may have ambiguity on which outputs have been effectively confirmed onchain (like it can be either prev local or last local but not both).

Okay for now, but we'll have to refactor and make sense of all this fields at some point..

ariard · 2020-02-17T21:20:20Z

lightning/src/ln/channelmonitor.rs

+	///
+	/// Further, the implementer must also ensure that each output returned in
+	/// monitor.get_outputs_to_watch() is registered to ensure that the provided monitor learns about
+	/// any spends of any of the outputs.


"This set of outputs correspond to in-channel outputs, i.e any output for which LN-specific state is needed to solve them and faithfully execute the protocol. Any miss in registering them may entrain a fund loss"

I'm not sure it matters what the outputs are...maybe in the future we'll register revokeable outputs (which we don't currently do and just send a SpendableOutput event to the user) so that we can identify if we went backwards. I added a note that you may lose funds if you fail just emphasize that it matters, though.

ariard · 2020-02-17T21:22:03Z

lightning/src/ln/channelmonitor.rs

@@ -259,6 +263,11 @@ impl<Key : Send + cmp::Eq + hash::Hash + 'static, ChanSigner: ChannelKeys> Simpl
 				self.chain_monitor.watch_all_txn();
 			}
 		}
+		for (txid, outputs) in monitor.get_outputs_to_watch().iter() {
+			for (idx, script) in outputs.iter().enumerate() {


Note: enumerate work right now to get outpoint index because we clone the whole output of tx at parsing but if we start to be more picky (like not-watching output in final state like to_remote) it may break

Right, it would need to change the same as the walk of the return value of block_connected would.

ariard · 2020-02-17T21:30:28Z

lightning/src/ln/channelmonitor.rs

+	// interface knows about the TXOs that we want to be notified of spends of. We could probably
+	// be smart and derive them from the above storage fields, but its much simpler and more
+	// Obviously Correct (tm) if we just keep track of them explicitly.
+	outputs_to_watch: HashMap<Sha256dHash, Vec<Script>>,


Have you try to implement this with just returning back Script from remote_commitment_txn_on_chain+prev_local_commitment_tx+current_local_signed_commitment_tx? I think it covers all scripts we care about (well there is also outputs on revoked HTLC-txb but that's a bug we don't track them yet)

I have not. I was aiming for "quick and easy" given we have a ton of ongoing larger patches and refactors in that area coming up, so landing something complicated would be a waste of time, and a bunch of time. Up to you if you want me to try another way, though.

ariard · 2020-02-17T21:40:59Z

lightning/src/ln/channelmonitor.rs

@@ -2394,6 +2394,10 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 		}
 	}

+	/// Called by ChannelMonitor::block_connected, which implements ChainListener::block_connected.


"Should be called by ManyChannelMonitor implementation"

Sadly, it can't be - its not pub (I have a patch for this, but too many in-flight PRs right now). Its only called by SimpleManyChannelMonitor (which is a typo here, now fixed).

ariard

ACK af4c5a2

jkczyz

Deferring to @ariard on the approach given he's refactoring the code. Left some minor comments.

lightning/src/ln/channelmonitor.rs

lightning/src/ln/functional_test_utils.rs

Upon deserialization/reload we need to be able to register each outpoint which spends the commitment txo which a channelmonitor believes to be on chain. While our other internal tracking is likely sufficient to regenerate these, its much easier to simply track all outpouts we've ever generated, so we do that here.

This tests, after each functional test, that if we serialize and reload all of our ChannelMonitors we end up tracking the same set of outputs as before.

TheBlueMatt · 2020-02-18T23:23:50Z

Address Jeff's comments. Will merge after Travis passes.

TheBlueMatt force-pushed the 2020-01-monitor-reload-watch branch from 955bed5 to 3125760 Compare January 21, 2020 04:25

TheBlueMatt requested a review from jkczyz February 5, 2020 20:04

TheBlueMatt force-pushed the 2020-01-monitor-reload-watch branch from 3125760 to 5e11836 Compare February 5, 2020 20:07

arik-so reviewed Feb 10, 2020

View reviewed changes

TheBlueMatt force-pushed the 2020-01-monitor-reload-watch branch from 5e11836 to 3c10c69 Compare February 11, 2020 00:12

TheBlueMatt mentioned this pull request Feb 17, 2020

Fix Two Bugs around ChannelManager serialization round-trip #503

Merged

ariard reviewed Feb 17, 2020

View reviewed changes

TheBlueMatt force-pushed the 2020-01-monitor-reload-watch branch from 3c10c69 to af4c5a2 Compare February 17, 2020 22:41

TheBlueMatt added this to the 0.0.10 milestone Feb 17, 2020

ariard reviewed Feb 18, 2020

View reviewed changes

jkczyz approved these changes Feb 18, 2020

View reviewed changes

lightning/src/ln/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/ln/functional_test_utils.rs Outdated Show resolved Hide resolved

lightning/src/ln/functional_test_utils.rs Outdated Show resolved Hide resolved

TheBlueMatt added 3 commits February 18, 2020 18:20

Test that we don't forget to track any outputs at monitor-load

662f13a

This tests, after each functional test, that if we serialize and reload all of our ChannelMonitors we end up tracking the same set of outputs as before.

Add comment describing how ChannelMonitor::block_connected is called

5fceb0f

TheBlueMatt force-pushed the 2020-01-monitor-reload-watch branch from af4c5a2 to 5fceb0f Compare February 18, 2020 23:23

TheBlueMatt merged commit 29d14dd into lightningdevkit:master Feb 19, 2020

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

TheBlueMatt mentioned this pull request Sep 27, 2020

Drop the redundant/broken ChannelMonitor::get_monitored_outpoints #722

Merged

Track the full list of outpoints a chanmon wants monitoring for #455

Track the full list of outpoints a chanmon wants monitoring for #455

Uh oh!

Conversation

TheBlueMatt commented Jan 21, 2020

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!

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!

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!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Feb 18, 2020

Uh oh!

Uh oh!