Track the full list of outpoints a chanmon wants monitoring for by TheBlueMatt · Pull Request #455 · lightningdevkit/rust-lightning

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

+	// 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

 }
 }

+ (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

 }
 }
 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

+	///
+	/// 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

 }
 }
+ 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

+	// 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

 }
 	}

+	/// 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.

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 · 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

+	// 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

 }
 }

+ (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

 }
 }
 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

+	///
+	/// 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

 }
 }
+ 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

+	// 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

 }
 	}

+	/// 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.

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 · 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

+	// 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

 }
 }

+ (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

 }
 }
 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

+	///
+	/// 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

 }
 }
+ 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

+	// 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

 }
 	}

+	/// 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.

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 · 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

+	// 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

 }
 }

+ (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

 }
 }
 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

+	///
+	/// 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

 }
 }
+ 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

+	// 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

 }
 	}

+	/// 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.

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 · 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

+	// 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

 }
 }

+ (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

 }
 }
 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

+	///
+	/// 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

 }
 }
+ 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

+	// 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

 }
 	}

+	/// 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.

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 · 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

+	// 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

 }
 }

+ (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

 }
 }
 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

+	///
+	/// 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

 }
 }
+ 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

+	// 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

 }
 	}

+	/// 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.

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 · 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

+	// 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

 }
 }

+ (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

 }
 }
 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

+	///
+	/// 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

 }
 }
+ 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

+	// 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

 }
 	}

+	/// 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.

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 · 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

+	// 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

 }
 }

+ (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

 }
 }
 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

+	///
+	/// 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

 }
 }
+ 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

+	// 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

 }
 	}

+	/// 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.

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 3125760Compare 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 5e11836Compare 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 3c10c69Compare 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 af4c5a2Compare 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

Comment threadlightning/src/ln/channelmonitor.rs Outdated
Comment threadlightning/src/ln/functional_test_utils.rs Outdated
Comment threadlightning/src/ln/functional_test_utils.rs Outdated

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 5fceb0fCompare February 18, 2020 23:23

TheBlueMatt merged commit 29d14dd into lightningdevkit:masterFeb 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