Remove Channel's ChannelMonitor copy

[valentinewallace]

In service to the larger refactor of removing the Channel's reference
to its ChannelMonitor.

This also allows us to remove the Channel's channel_monitor() function. Next we can remove all of the Channel's logic for keeping its channel monitor up to date, as well as the update_monitor_ooo logic in ChannelMonitor.

Partially resolvesCloses#657

[codecov]

Codecov Report

Merging #667 into master will increase coverage by 0.10%.
The diff coverage is 92.47%.

@@ Coverage Diff @@## master #667 +/- ##
==========================================
+ Coverage 91.36% 91.46% +0.10% 
==========================================
Files 35 35 Lines 21703 21888 +185 ==========================================
+ Hits 19828 20019 +191 + Misses 1875 1869 -6 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	87.17% <ø> (-0.04%)	⬇️
lightning/src/ln/reorg_tests.rs	98.94% <ø> (ø)
lightning/src/ln/functional_tests.rs	97.15% <88.23%> (+0.14%)	⬆️
lightning/src/ln/channelmonitor.rs	95.56% <90.24%> (-0.15%)	⬇️
lightning/src/ln/channelmanager.rs	85.26% <96.96%> (+<0.01%)	⬆️
lightning/src/util/test_utils.rs	86.13% <100.00%> (ø)
lightning/src/ln/onchaintx.rs	93.75% <0.00%> (-0.20%)	⬇️
lightning/src/routing/router.rs	97.39% <0.00%> (+0.80%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3b4f43...28d9036. Read the comment docs.

[valentinewallace]

Oops, broke build -- will fix in a bit

[valentinewallace]

I looked into extracting some common HTLC selection functionality between monitor_would_broadcast and build_commitment_transaction but they're pretty different, they want different HTLCs, and monitor_would_broadcast needs local and remote HTLCs, and idk. Let me know if I'm missing something and it's definitely a good idea.

[valentinewallace]

Hmm, fuzzing seems slow still. Looking into it.

[arik-so]

not so much related to this diff in particular, but is the string literal the most elegant way to get the state name?

[valentinewallace]

Open to other options 😄 (for a future PR)

[ariard]

I'm all in to remove Channel's ChannelMonitor copy but I think there is a easier way than dc9ef2c. If I understand new would_broadcast_at_height_functionality well, its purpose is only to verify that ChannelMonitor accomplish its job well and broadcast channel update accordingly.

I'm not sure if it's a great approach, in the future if we add new conditions in ChannelMonitor to force-close channel we would need to access them too. Like in case of mempools-congestion, preemptively close channel at risk.

Another way to solve this would be a) pass a reference to ChainWatchInterface to ChannelManager b) at funding_tx detection, return its output to watch c) monitor commitment transaction broadcast. Parsing positively commitment transaction is easy to do due to transaction pattern (one-input), LN commitment number watermark and overall funding outpoint spending. It's more robust also as we shouldn't assume that our ChannelManager and ChannelMonitor are running on the same block provider and thus may have block view latency.

I know that's already the current model to query ChannelMonitor to decide if we should shutdown and broadcast channel update, but I think it's fine to wait a bit for channel update broadcast. Shutdown/broadcast will happen either sooner after any attempt to unsuccessfully update monitors. Or latter after block processing.

If you still want to be quick in term of updating global network view of our channel maybe we can ChannelMonitor signal broadcast with a boolean and us periodically querying it like we're doing for get_and_clear_pending_htlcs_updated ?

Overall, I may miss some onchain/offchain coupling assumptions but it feels like we can short-cut some complexity ?

[TheBlueMatt]

Ooooo, right, so I think that's true - would_broadcast_at_height is only used to close a channel and we could happily have the ChannelMonitor handle that (though currently Channel[Manager] handles that fully). My knee-jerk reaction was that there would be a race condition where we'd continue updating a channel state after its been closed (and the latest local commitment broadcast), but I think we could not even bother notifying the ChannelManager that we're closing the channel and wait for provide_latest_local_commitment_tx_info to return an Err because local_tx_signed is set to close the channel, so there's no race.

[TheBlueMatt]

I had to admit I'm somewhat surprised that's the only test changes you need, but looks good.

[TheBlueMatt]

Aside from the one clarifying change I suggested and the two comment nits, looks good!

[arik-so]

Out of curiosity, how do we decide which events are monitor events? For example, why is HTLCEvent for claiming one, but forwarding offered HTLCs isn't?

[TheBlueMatt]

In this context, its basically "a thing which we detected on-chain which has an impact on our understanding of a channel's state" (ie, basically, "channel has been closed on chain" or "transaction on-chain resolved an HTLC which was outstanding when we went on chain").

[arik-so]

One day we'll have versioning for reads lol :)

[valentinewallace]

Indeed lol

[ariard]

@valentinewallace

I think this is breaking one of the property of our distributed watchtower infrastructure as documented here : https://github.com/rust-bitcoin/rust-lightning/blob/3defcc896266f3d67848ce28981a756e971a3f0c/lightning/src/ln/channelmonitor.rs#L1174

AFAICT, by adding logs flag is true beyond updating local view with latest local state.

I'll fix it and add coverage for this.

[valentinewallace]

Logs flag? Ok I think I see what you mean though, thanks for catching this!

[TheBlueMatt]

We're discussing on IRC still, but I don't think this change is correct - we should refuse to accept updates after we've signed a local tx irrespective of why we signed it.

[ariard]

Sorry, after discussion on IRC and testing I realized that the alleged property was only supported in my mind. In fact #667 makes it easy to actually implement it, so did so in #679 with test coverage.

[valentinewallace]

In service to the larger refactor of removing the Channel's reference
to its ChannelMonitor.

This also allows us to remove the Channel's channel_monitor() function. Next we can remove all of the Channel's logic for keeping its channel monitor up to date, as well as the update_monitor_ooo logic in ChannelMonitor.

Partially resolvesCloses#657

[codecov]

Codecov Report

Merging #667 into master will increase coverage by 0.10%.
The diff coverage is 92.47%.

@@ Coverage Diff @@## master #667 +/- ##
==========================================
+ Coverage 91.36% 91.46% +0.10% 
==========================================
Files 35 35 Lines 21703 21888 +185 ==========================================
+ Hits 19828 20019 +191 + Misses 1875 1869 -6 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	87.17% <ø> (-0.04%)	⬇️
lightning/src/ln/reorg_tests.rs	98.94% <ø> (ø)
lightning/src/ln/functional_tests.rs	97.15% <88.23%> (+0.14%)	⬆️
lightning/src/ln/channelmonitor.rs	95.56% <90.24%> (-0.15%)	⬇️
lightning/src/ln/channelmanager.rs	85.26% <96.96%> (+<0.01%)	⬆️
lightning/src/util/test_utils.rs	86.13% <100.00%> (ø)
lightning/src/ln/onchaintx.rs	93.75% <0.00%> (-0.20%)	⬇️
lightning/src/routing/router.rs	97.39% <0.00%> (+0.80%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3b4f43...28d9036. Read the comment docs.

[valentinewallace]

Oops, broke build -- will fix in a bit

[valentinewallace]

I looked into extracting some common HTLC selection functionality between monitor_would_broadcast and build_commitment_transaction but they're pretty different, they want different HTLCs, and monitor_would_broadcast needs local and remote HTLCs, and idk. Let me know if I'm missing something and it's definitely a good idea.

[valentinewallace]

Hmm, fuzzing seems slow still. Looking into it.

[arik-so]

not so much related to this diff in particular, but is the string literal the most elegant way to get the state name?

[valentinewallace]

Open to other options 😄 (for a future PR)

[ariard]

I'm all in to remove Channel's ChannelMonitor copy but I think there is a easier way than dc9ef2c. If I understand new would_broadcast_at_height_functionality well, its purpose is only to verify that ChannelMonitor accomplish its job well and broadcast channel update accordingly.

I'm not sure if it's a great approach, in the future if we add new conditions in ChannelMonitor to force-close channel we would need to access them too. Like in case of mempools-congestion, preemptively close channel at risk.

Another way to solve this would be a) pass a reference to ChainWatchInterface to ChannelManager b) at funding_tx detection, return its output to watch c) monitor commitment transaction broadcast. Parsing positively commitment transaction is easy to do due to transaction pattern (one-input), LN commitment number watermark and overall funding outpoint spending. It's more robust also as we shouldn't assume that our ChannelManager and ChannelMonitor are running on the same block provider and thus may have block view latency.

I know that's already the current model to query ChannelMonitor to decide if we should shutdown and broadcast channel update, but I think it's fine to wait a bit for channel update broadcast. Shutdown/broadcast will happen either sooner after any attempt to unsuccessfully update monitors. Or latter after block processing.

If you still want to be quick in term of updating global network view of our channel maybe we can ChannelMonitor signal broadcast with a boolean and us periodically querying it like we're doing for get_and_clear_pending_htlcs_updated ?

Overall, I may miss some onchain/offchain coupling assumptions but it feels like we can short-cut some complexity ?

[TheBlueMatt]

Ooooo, right, so I think that's true - would_broadcast_at_height is only used to close a channel and we could happily have the ChannelMonitor handle that (though currently Channel[Manager] handles that fully). My knee-jerk reaction was that there would be a race condition where we'd continue updating a channel state after its been closed (and the latest local commitment broadcast), but I think we could not even bother notifying the ChannelManager that we're closing the channel and wait for provide_latest_local_commitment_tx_info to return an Err because local_tx_signed is set to close the channel, so there's no race.

[TheBlueMatt]

I had to admit I'm somewhat surprised that's the only test changes you need, but looks good.

[TheBlueMatt]

Aside from the one clarifying change I suggested and the two comment nits, looks good!

[arik-so]

Out of curiosity, how do we decide which events are monitor events? For example, why is HTLCEvent for claiming one, but forwarding offered HTLCs isn't?

[TheBlueMatt]

In this context, its basically "a thing which we detected on-chain which has an impact on our understanding of a channel's state" (ie, basically, "channel has been closed on chain" or "transaction on-chain resolved an HTLC which was outstanding when we went on chain").

[arik-so]

One day we'll have versioning for reads lol :)

[valentinewallace]

Indeed lol

[ariard]

@valentinewallace

I think this is breaking one of the property of our distributed watchtower infrastructure as documented here : https://github.com/rust-bitcoin/rust-lightning/blob/3defcc896266f3d67848ce28981a756e971a3f0c/lightning/src/ln/channelmonitor.rs#L1174

AFAICT, by adding logs flag is true beyond updating local view with latest local state.

I'll fix it and add coverage for this.

[valentinewallace]

Logs flag? Ok I think I see what you mean though, thanks for catching this!

[TheBlueMatt]

We're discussing on IRC still, but I don't think this change is correct - we should refuse to accept updates after we've signed a local tx irrespective of why we signed it.

[ariard]

Sorry, after discussion on IRC and testing I realized that the alleged property was only supported in my mind. In fact #667 makes it easy to actually implement it, so did so in #679 with test coverage.

[valentinewallace]

In service to the larger refactor of removing the Channel's reference
to its ChannelMonitor.

This also allows us to remove the Channel's channel_monitor() function. Next we can remove all of the Channel's logic for keeping its channel monitor up to date, as well as the update_monitor_ooo logic in ChannelMonitor.

Partially resolvesCloses#657

[codecov]

Codecov Report

Merging #667 into master will increase coverage by 0.10%.
The diff coverage is 92.47%.

@@ Coverage Diff @@## master #667 +/- ##
==========================================
+ Coverage 91.36% 91.46% +0.10% 
==========================================
Files 35 35 Lines 21703 21888 +185 ==========================================
+ Hits 19828 20019 +191 + Misses 1875 1869 -6 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	87.17% <ø> (-0.04%)	⬇️
lightning/src/ln/reorg_tests.rs	98.94% <ø> (ø)
lightning/src/ln/functional_tests.rs	97.15% <88.23%> (+0.14%)	⬆️
lightning/src/ln/channelmonitor.rs	95.56% <90.24%> (-0.15%)	⬇️
lightning/src/ln/channelmanager.rs	85.26% <96.96%> (+<0.01%)	⬆️
lightning/src/util/test_utils.rs	86.13% <100.00%> (ø)
lightning/src/ln/onchaintx.rs	93.75% <0.00%> (-0.20%)	⬇️
lightning/src/routing/router.rs	97.39% <0.00%> (+0.80%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3b4f43...28d9036. Read the comment docs.

[valentinewallace]

Oops, broke build -- will fix in a bit

[valentinewallace]

I looked into extracting some common HTLC selection functionality between monitor_would_broadcast and build_commitment_transaction but they're pretty different, they want different HTLCs, and monitor_would_broadcast needs local and remote HTLCs, and idk. Let me know if I'm missing something and it's definitely a good idea.

[valentinewallace]

Hmm, fuzzing seems slow still. Looking into it.

[arik-so]

not so much related to this diff in particular, but is the string literal the most elegant way to get the state name?

[valentinewallace]

Open to other options 😄 (for a future PR)

[ariard]

I'm all in to remove Channel's ChannelMonitor copy but I think there is a easier way than dc9ef2c. If I understand new would_broadcast_at_height_functionality well, its purpose is only to verify that ChannelMonitor accomplish its job well and broadcast channel update accordingly.

I'm not sure if it's a great approach, in the future if we add new conditions in ChannelMonitor to force-close channel we would need to access them too. Like in case of mempools-congestion, preemptively close channel at risk.

Another way to solve this would be a) pass a reference to ChainWatchInterface to ChannelManager b) at funding_tx detection, return its output to watch c) monitor commitment transaction broadcast. Parsing positively commitment transaction is easy to do due to transaction pattern (one-input), LN commitment number watermark and overall funding outpoint spending. It's more robust also as we shouldn't assume that our ChannelManager and ChannelMonitor are running on the same block provider and thus may have block view latency.

I know that's already the current model to query ChannelMonitor to decide if we should shutdown and broadcast channel update, but I think it's fine to wait a bit for channel update broadcast. Shutdown/broadcast will happen either sooner after any attempt to unsuccessfully update monitors. Or latter after block processing.

If you still want to be quick in term of updating global network view of our channel maybe we can ChannelMonitor signal broadcast with a boolean and us periodically querying it like we're doing for get_and_clear_pending_htlcs_updated ?

Overall, I may miss some onchain/offchain coupling assumptions but it feels like we can short-cut some complexity ?

[TheBlueMatt]

Ooooo, right, so I think that's true - would_broadcast_at_height is only used to close a channel and we could happily have the ChannelMonitor handle that (though currently Channel[Manager] handles that fully). My knee-jerk reaction was that there would be a race condition where we'd continue updating a channel state after its been closed (and the latest local commitment broadcast), but I think we could not even bother notifying the ChannelManager that we're closing the channel and wait for provide_latest_local_commitment_tx_info to return an Err because local_tx_signed is set to close the channel, so there's no race.

[TheBlueMatt]

I had to admit I'm somewhat surprised that's the only test changes you need, but looks good.

[TheBlueMatt]

Aside from the one clarifying change I suggested and the two comment nits, looks good!

[arik-so]

Out of curiosity, how do we decide which events are monitor events? For example, why is HTLCEvent for claiming one, but forwarding offered HTLCs isn't?

[TheBlueMatt]

In this context, its basically "a thing which we detected on-chain which has an impact on our understanding of a channel's state" (ie, basically, "channel has been closed on chain" or "transaction on-chain resolved an HTLC which was outstanding when we went on chain").

[arik-so]

One day we'll have versioning for reads lol :)

[valentinewallace]

Indeed lol

[ariard]

@valentinewallace

I think this is breaking one of the property of our distributed watchtower infrastructure as documented here : https://github.com/rust-bitcoin/rust-lightning/blob/3defcc896266f3d67848ce28981a756e971a3f0c/lightning/src/ln/channelmonitor.rs#L1174

AFAICT, by adding logs flag is true beyond updating local view with latest local state.

I'll fix it and add coverage for this.

[valentinewallace]

Logs flag? Ok I think I see what you mean though, thanks for catching this!

[TheBlueMatt]

We're discussing on IRC still, but I don't think this change is correct - we should refuse to accept updates after we've signed a local tx irrespective of why we signed it.

[ariard]

Sorry, after discussion on IRC and testing I realized that the alleged property was only supported in my mind. In fact #667 makes it easy to actually implement it, so did so in #679 with test coverage.

[valentinewallace]

In service to the larger refactor of removing the Channel's reference
to its ChannelMonitor.

This also allows us to remove the Channel's channel_monitor() function. Next we can remove all of the Channel's logic for keeping its channel monitor up to date, as well as the update_monitor_ooo logic in ChannelMonitor.

Partially resolvesCloses#657

[codecov]

Codecov Report

Merging #667 into master will increase coverage by 0.10%.
The diff coverage is 92.47%.

@@ Coverage Diff @@## master #667 +/- ##
==========================================
+ Coverage 91.36% 91.46% +0.10% 
==========================================
Files 35 35 Lines 21703 21888 +185 ==========================================
+ Hits 19828 20019 +191 + Misses 1875 1869 -6 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	87.17% <ø> (-0.04%)	⬇️
lightning/src/ln/reorg_tests.rs	98.94% <ø> (ø)
lightning/src/ln/functional_tests.rs	97.15% <88.23%> (+0.14%)	⬆️
lightning/src/ln/channelmonitor.rs	95.56% <90.24%> (-0.15%)	⬇️
lightning/src/ln/channelmanager.rs	85.26% <96.96%> (+<0.01%)	⬆️
lightning/src/util/test_utils.rs	86.13% <100.00%> (ø)
lightning/src/ln/onchaintx.rs	93.75% <0.00%> (-0.20%)	⬇️
lightning/src/routing/router.rs	97.39% <0.00%> (+0.80%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3b4f43...28d9036. Read the comment docs.

[valentinewallace]

Oops, broke build -- will fix in a bit

[valentinewallace]

I looked into extracting some common HTLC selection functionality between monitor_would_broadcast and build_commitment_transaction but they're pretty different, they want different HTLCs, and monitor_would_broadcast needs local and remote HTLCs, and idk. Let me know if I'm missing something and it's definitely a good idea.

[valentinewallace]

Hmm, fuzzing seems slow still. Looking into it.

[arik-so]

not so much related to this diff in particular, but is the string literal the most elegant way to get the state name?

[valentinewallace]

Open to other options 😄 (for a future PR)

[ariard]

I'm all in to remove Channel's ChannelMonitor copy but I think there is a easier way than dc9ef2c. If I understand new would_broadcast_at_height_functionality well, its purpose is only to verify that ChannelMonitor accomplish its job well and broadcast channel update accordingly.

I'm not sure if it's a great approach, in the future if we add new conditions in ChannelMonitor to force-close channel we would need to access them too. Like in case of mempools-congestion, preemptively close channel at risk.

Another way to solve this would be a) pass a reference to ChainWatchInterface to ChannelManager b) at funding_tx detection, return its output to watch c) monitor commitment transaction broadcast. Parsing positively commitment transaction is easy to do due to transaction pattern (one-input), LN commitment number watermark and overall funding outpoint spending. It's more robust also as we shouldn't assume that our ChannelManager and ChannelMonitor are running on the same block provider and thus may have block view latency.

I know that's already the current model to query ChannelMonitor to decide if we should shutdown and broadcast channel update, but I think it's fine to wait a bit for channel update broadcast. Shutdown/broadcast will happen either sooner after any attempt to unsuccessfully update monitors. Or latter after block processing.

If you still want to be quick in term of updating global network view of our channel maybe we can ChannelMonitor signal broadcast with a boolean and us periodically querying it like we're doing for get_and_clear_pending_htlcs_updated ?

Overall, I may miss some onchain/offchain coupling assumptions but it feels like we can short-cut some complexity ?

[TheBlueMatt]

Ooooo, right, so I think that's true - would_broadcast_at_height is only used to close a channel and we could happily have the ChannelMonitor handle that (though currently Channel[Manager] handles that fully). My knee-jerk reaction was that there would be a race condition where we'd continue updating a channel state after its been closed (and the latest local commitment broadcast), but I think we could not even bother notifying the ChannelManager that we're closing the channel and wait for provide_latest_local_commitment_tx_info to return an Err because local_tx_signed is set to close the channel, so there's no race.

[TheBlueMatt]

I had to admit I'm somewhat surprised that's the only test changes you need, but looks good.

[TheBlueMatt]

Aside from the one clarifying change I suggested and the two comment nits, looks good!

[arik-so]

Out of curiosity, how do we decide which events are monitor events? For example, why is HTLCEvent for claiming one, but forwarding offered HTLCs isn't?

[TheBlueMatt]

In this context, its basically "a thing which we detected on-chain which has an impact on our understanding of a channel's state" (ie, basically, "channel has been closed on chain" or "transaction on-chain resolved an HTLC which was outstanding when we went on chain").

[arik-so]

One day we'll have versioning for reads lol :)

[valentinewallace]

Indeed lol

[ariard]

@valentinewallace

I think this is breaking one of the property of our distributed watchtower infrastructure as documented here : https://github.com/rust-bitcoin/rust-lightning/blob/3defcc896266f3d67848ce28981a756e971a3f0c/lightning/src/ln/channelmonitor.rs#L1174

AFAICT, by adding logs flag is true beyond updating local view with latest local state.

I'll fix it and add coverage for this.

[valentinewallace]

Logs flag? Ok I think I see what you mean though, thanks for catching this!

[TheBlueMatt]

We're discussing on IRC still, but I don't think this change is correct - we should refuse to accept updates after we've signed a local tx irrespective of why we signed it.

[ariard]

Sorry, after discussion on IRC and testing I realized that the alleged property was only supported in my mind. In fact #667 makes it easy to actually implement it, so did so in #679 with test coverage.

[valentinewallace]

In service to the larger refactor of removing the Channel's reference
to its ChannelMonitor.

This also allows us to remove the Channel's channel_monitor() function. Next we can remove all of the Channel's logic for keeping its channel monitor up to date, as well as the update_monitor_ooo logic in ChannelMonitor.

Partially resolvesCloses#657

[codecov]

Codecov Report

Merging #667 into master will increase coverage by 0.10%.
The diff coverage is 92.47%.

@@ Coverage Diff @@## master #667 +/- ##
==========================================
+ Coverage 91.36% 91.46% +0.10% 
==========================================
Files 35 35 Lines 21703 21888 +185 ==========================================
+ Hits 19828 20019 +191 + Misses 1875 1869 -6 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	87.17% <ø> (-0.04%)	⬇️
lightning/src/ln/reorg_tests.rs	98.94% <ø> (ø)
lightning/src/ln/functional_tests.rs	97.15% <88.23%> (+0.14%)	⬆️
lightning/src/ln/channelmonitor.rs	95.56% <90.24%> (-0.15%)	⬇️
lightning/src/ln/channelmanager.rs	85.26% <96.96%> (+<0.01%)	⬆️
lightning/src/util/test_utils.rs	86.13% <100.00%> (ø)
lightning/src/ln/onchaintx.rs	93.75% <0.00%> (-0.20%)	⬇️
lightning/src/routing/router.rs	97.39% <0.00%> (+0.80%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3b4f43...28d9036. Read the comment docs.

[valentinewallace]

Oops, broke build -- will fix in a bit

[valentinewallace]

I looked into extracting some common HTLC selection functionality between monitor_would_broadcast and build_commitment_transaction but they're pretty different, they want different HTLCs, and monitor_would_broadcast needs local and remote HTLCs, and idk. Let me know if I'm missing something and it's definitely a good idea.

[valentinewallace]

Hmm, fuzzing seems slow still. Looking into it.

[arik-so]

not so much related to this diff in particular, but is the string literal the most elegant way to get the state name?

[valentinewallace]

Open to other options 😄 (for a future PR)

[ariard]

I'm all in to remove Channel's ChannelMonitor copy but I think there is a easier way than dc9ef2c. If I understand new would_broadcast_at_height_functionality well, its purpose is only to verify that ChannelMonitor accomplish its job well and broadcast channel update accordingly.

I'm not sure if it's a great approach, in the future if we add new conditions in ChannelMonitor to force-close channel we would need to access them too. Like in case of mempools-congestion, preemptively close channel at risk.

Another way to solve this would be a) pass a reference to ChainWatchInterface to ChannelManager b) at funding_tx detection, return its output to watch c) monitor commitment transaction broadcast. Parsing positively commitment transaction is easy to do due to transaction pattern (one-input), LN commitment number watermark and overall funding outpoint spending. It's more robust also as we shouldn't assume that our ChannelManager and ChannelMonitor are running on the same block provider and thus may have block view latency.

I know that's already the current model to query ChannelMonitor to decide if we should shutdown and broadcast channel update, but I think it's fine to wait a bit for channel update broadcast. Shutdown/broadcast will happen either sooner after any attempt to unsuccessfully update monitors. Or latter after block processing.

If you still want to be quick in term of updating global network view of our channel maybe we can ChannelMonitor signal broadcast with a boolean and us periodically querying it like we're doing for get_and_clear_pending_htlcs_updated ?

Overall, I may miss some onchain/offchain coupling assumptions but it feels like we can short-cut some complexity ?

[TheBlueMatt]

Ooooo, right, so I think that's true - would_broadcast_at_height is only used to close a channel and we could happily have the ChannelMonitor handle that (though currently Channel[Manager] handles that fully). My knee-jerk reaction was that there would be a race condition where we'd continue updating a channel state after its been closed (and the latest local commitment broadcast), but I think we could not even bother notifying the ChannelManager that we're closing the channel and wait for provide_latest_local_commitment_tx_info to return an Err because local_tx_signed is set to close the channel, so there's no race.

[TheBlueMatt]

I had to admit I'm somewhat surprised that's the only test changes you need, but looks good.

[TheBlueMatt]

Aside from the one clarifying change I suggested and the two comment nits, looks good!

[arik-so]

Out of curiosity, how do we decide which events are monitor events? For example, why is HTLCEvent for claiming one, but forwarding offered HTLCs isn't?

[TheBlueMatt]

In this context, its basically "a thing which we detected on-chain which has an impact on our understanding of a channel's state" (ie, basically, "channel has been closed on chain" or "transaction on-chain resolved an HTLC which was outstanding when we went on chain").

[arik-so]

One day we'll have versioning for reads lol :)

[valentinewallace]

Indeed lol

[ariard]

@valentinewallace

I think this is breaking one of the property of our distributed watchtower infrastructure as documented here : https://github.com/rust-bitcoin/rust-lightning/blob/3defcc896266f3d67848ce28981a756e971a3f0c/lightning/src/ln/channelmonitor.rs#L1174

AFAICT, by adding logs flag is true beyond updating local view with latest local state.

I'll fix it and add coverage for this.

[valentinewallace]

Logs flag? Ok I think I see what you mean though, thanks for catching this!

[TheBlueMatt]

We're discussing on IRC still, but I don't think this change is correct - we should refuse to accept updates after we've signed a local tx irrespective of why we signed it.

[ariard]

Sorry, after discussion on IRC and testing I realized that the alleged property was only supported in my mind. In fact #667 makes it easy to actually implement it, so did so in #679 with test coverage.

[valentinewallace]

In service to the larger refactor of removing the Channel's reference
to its ChannelMonitor.

This also allows us to remove the Channel's channel_monitor() function. Next we can remove all of the Channel's logic for keeping its channel monitor up to date, as well as the update_monitor_ooo logic in ChannelMonitor.

Partially resolvesCloses#657

[codecov]

Codecov Report

Merging #667 into master will increase coverage by 0.10%.
The diff coverage is 92.47%.

@@ Coverage Diff @@## master #667 +/- ##
==========================================
+ Coverage 91.36% 91.46% +0.10% 
==========================================
Files 35 35 Lines 21703 21888 +185 ==========================================
+ Hits 19828 20019 +191 + Misses 1875 1869 -6 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	87.17% <ø> (-0.04%)	⬇️
lightning/src/ln/reorg_tests.rs	98.94% <ø> (ø)
lightning/src/ln/functional_tests.rs	97.15% <88.23%> (+0.14%)	⬆️
lightning/src/ln/channelmonitor.rs	95.56% <90.24%> (-0.15%)	⬇️
lightning/src/ln/channelmanager.rs	85.26% <96.96%> (+<0.01%)	⬆️
lightning/src/util/test_utils.rs	86.13% <100.00%> (ø)
lightning/src/ln/onchaintx.rs	93.75% <0.00%> (-0.20%)	⬇️
lightning/src/routing/router.rs	97.39% <0.00%> (+0.80%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3b4f43...28d9036. Read the comment docs.

[valentinewallace]

Oops, broke build -- will fix in a bit

[valentinewallace]

I looked into extracting some common HTLC selection functionality between monitor_would_broadcast and build_commitment_transaction but they're pretty different, they want different HTLCs, and monitor_would_broadcast needs local and remote HTLCs, and idk. Let me know if I'm missing something and it's definitely a good idea.

[valentinewallace]

Hmm, fuzzing seems slow still. Looking into it.

[arik-so]

not so much related to this diff in particular, but is the string literal the most elegant way to get the state name?

[valentinewallace]

Open to other options 😄 (for a future PR)

[ariard]

I'm all in to remove Channel's ChannelMonitor copy but I think there is a easier way than dc9ef2c. If I understand new would_broadcast_at_height_functionality well, its purpose is only to verify that ChannelMonitor accomplish its job well and broadcast channel update accordingly.

I'm not sure if it's a great approach, in the future if we add new conditions in ChannelMonitor to force-close channel we would need to access them too. Like in case of mempools-congestion, preemptively close channel at risk.

Another way to solve this would be a) pass a reference to ChainWatchInterface to ChannelManager b) at funding_tx detection, return its output to watch c) monitor commitment transaction broadcast. Parsing positively commitment transaction is easy to do due to transaction pattern (one-input), LN commitment number watermark and overall funding outpoint spending. It's more robust also as we shouldn't assume that our ChannelManager and ChannelMonitor are running on the same block provider and thus may have block view latency.

I know that's already the current model to query ChannelMonitor to decide if we should shutdown and broadcast channel update, but I think it's fine to wait a bit for channel update broadcast. Shutdown/broadcast will happen either sooner after any attempt to unsuccessfully update monitors. Or latter after block processing.

If you still want to be quick in term of updating global network view of our channel maybe we can ChannelMonitor signal broadcast with a boolean and us periodically querying it like we're doing for get_and_clear_pending_htlcs_updated ?

Overall, I may miss some onchain/offchain coupling assumptions but it feels like we can short-cut some complexity ?

[TheBlueMatt]

Ooooo, right, so I think that's true - would_broadcast_at_height is only used to close a channel and we could happily have the ChannelMonitor handle that (though currently Channel[Manager] handles that fully). My knee-jerk reaction was that there would be a race condition where we'd continue updating a channel state after its been closed (and the latest local commitment broadcast), but I think we could not even bother notifying the ChannelManager that we're closing the channel and wait for provide_latest_local_commitment_tx_info to return an Err because local_tx_signed is set to close the channel, so there's no race.

[TheBlueMatt]

I had to admit I'm somewhat surprised that's the only test changes you need, but looks good.

[TheBlueMatt]

Aside from the one clarifying change I suggested and the two comment nits, looks good!

[arik-so]

Out of curiosity, how do we decide which events are monitor events? For example, why is HTLCEvent for claiming one, but forwarding offered HTLCs isn't?

[TheBlueMatt]

In this context, its basically "a thing which we detected on-chain which has an impact on our understanding of a channel's state" (ie, basically, "channel has been closed on chain" or "transaction on-chain resolved an HTLC which was outstanding when we went on chain").

[arik-so]

One day we'll have versioning for reads lol :)

[valentinewallace]

Indeed lol

[ariard]

@valentinewallace

I think this is breaking one of the property of our distributed watchtower infrastructure as documented here : https://github.com/rust-bitcoin/rust-lightning/blob/3defcc896266f3d67848ce28981a756e971a3f0c/lightning/src/ln/channelmonitor.rs#L1174

AFAICT, by adding logs flag is true beyond updating local view with latest local state.

I'll fix it and add coverage for this.

[valentinewallace]

Logs flag? Ok I think I see what you mean though, thanks for catching this!

[TheBlueMatt]

We're discussing on IRC still, but I don't think this change is correct - we should refuse to accept updates after we've signed a local tx irrespective of why we signed it.

[ariard]

Sorry, after discussion on IRC and testing I realized that the alleged property was only supported in my mind. In fact #667 makes it easy to actually implement it, so did so in #679 with test coverage.

[valentinewallace]

In service to the larger refactor of removing the Channel's reference
to its ChannelMonitor.

This also allows us to remove the Channel's channel_monitor() function. Next we can remove all of the Channel's logic for keeping its channel monitor up to date, as well as the update_monitor_ooo logic in ChannelMonitor.

Partially resolvesCloses#657

[codecov]

Codecov Report

Merging #667 into master will increase coverage by 0.10%.
The diff coverage is 92.47%.

@@ Coverage Diff @@## master #667 +/- ##
==========================================
+ Coverage 91.36% 91.46% +0.10% 
==========================================
Files 35 35 Lines 21703 21888 +185 ==========================================
+ Hits 19828 20019 +191 + Misses 1875 1869 -6 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	87.17% <ø> (-0.04%)	⬇️
lightning/src/ln/reorg_tests.rs	98.94% <ø> (ø)
lightning/src/ln/functional_tests.rs	97.15% <88.23%> (+0.14%)	⬆️
lightning/src/ln/channelmonitor.rs	95.56% <90.24%> (-0.15%)	⬇️
lightning/src/ln/channelmanager.rs	85.26% <96.96%> (+<0.01%)	⬆️
lightning/src/util/test_utils.rs	86.13% <100.00%> (ø)
lightning/src/ln/onchaintx.rs	93.75% <0.00%> (-0.20%)	⬇️
lightning/src/routing/router.rs	97.39% <0.00%> (+0.80%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3b4f43...28d9036. Read the comment docs.

[valentinewallace]

Oops, broke build -- will fix in a bit

[valentinewallace]

I looked into extracting some common HTLC selection functionality between monitor_would_broadcast and build_commitment_transaction but they're pretty different, they want different HTLCs, and monitor_would_broadcast needs local and remote HTLCs, and idk. Let me know if I'm missing something and it's definitely a good idea.

[valentinewallace]

Hmm, fuzzing seems slow still. Looking into it.

[arik-so]

not so much related to this diff in particular, but is the string literal the most elegant way to get the state name?

[valentinewallace]

Open to other options 😄 (for a future PR)

[ariard]

I'm all in to remove Channel's ChannelMonitor copy but I think there is a easier way than dc9ef2c. If I understand new would_broadcast_at_height_functionality well, its purpose is only to verify that ChannelMonitor accomplish its job well and broadcast channel update accordingly.

I'm not sure if it's a great approach, in the future if we add new conditions in ChannelMonitor to force-close channel we would need to access them too. Like in case of mempools-congestion, preemptively close channel at risk.

Another way to solve this would be a) pass a reference to ChainWatchInterface to ChannelManager b) at funding_tx detection, return its output to watch c) monitor commitment transaction broadcast. Parsing positively commitment transaction is easy to do due to transaction pattern (one-input), LN commitment number watermark and overall funding outpoint spending. It's more robust also as we shouldn't assume that our ChannelManager and ChannelMonitor are running on the same block provider and thus may have block view latency.

I know that's already the current model to query ChannelMonitor to decide if we should shutdown and broadcast channel update, but I think it's fine to wait a bit for channel update broadcast. Shutdown/broadcast will happen either sooner after any attempt to unsuccessfully update monitors. Or latter after block processing.

If you still want to be quick in term of updating global network view of our channel maybe we can ChannelMonitor signal broadcast with a boolean and us periodically querying it like we're doing for get_and_clear_pending_htlcs_updated ?

Overall, I may miss some onchain/offchain coupling assumptions but it feels like we can short-cut some complexity ?

[TheBlueMatt]

Ooooo, right, so I think that's true - would_broadcast_at_height is only used to close a channel and we could happily have the ChannelMonitor handle that (though currently Channel[Manager] handles that fully). My knee-jerk reaction was that there would be a race condition where we'd continue updating a channel state after its been closed (and the latest local commitment broadcast), but I think we could not even bother notifying the ChannelManager that we're closing the channel and wait for provide_latest_local_commitment_tx_info to return an Err because local_tx_signed is set to close the channel, so there's no race.

[TheBlueMatt]

I had to admit I'm somewhat surprised that's the only test changes you need, but looks good.

[TheBlueMatt]

Aside from the one clarifying change I suggested and the two comment nits, looks good!

[arik-so]

Out of curiosity, how do we decide which events are monitor events? For example, why is HTLCEvent for claiming one, but forwarding offered HTLCs isn't?

[TheBlueMatt]

In this context, its basically "a thing which we detected on-chain which has an impact on our understanding of a channel's state" (ie, basically, "channel has been closed on chain" or "transaction on-chain resolved an HTLC which was outstanding when we went on chain").

[arik-so]

One day we'll have versioning for reads lol :)

[valentinewallace]

Indeed lol

[ariard]

@valentinewallace

I think this is breaking one of the property of our distributed watchtower infrastructure as documented here : https://github.com/rust-bitcoin/rust-lightning/blob/3defcc896266f3d67848ce28981a756e971a3f0c/lightning/src/ln/channelmonitor.rs#L1174

AFAICT, by adding logs flag is true beyond updating local view with latest local state.

I'll fix it and add coverage for this.

[valentinewallace]

Logs flag? Ok I think I see what you mean though, thanks for catching this!

[TheBlueMatt]

We're discussing on IRC still, but I don't think this change is correct - we should refuse to accept updates after we've signed a local tx irrespective of why we signed it.

[ariard]

Sorry, after discussion on IRC and testing I realized that the alleged property was only supported in my mind. In fact #667 makes it easy to actually implement it, so did so in #679 with test coverage.

[valentinewallace]

In service to the larger refactor of removing the Channel's reference
to its ChannelMonitor.

This also allows us to remove the Channel's channel_monitor() function. Next we can remove all of the Channel's logic for keeping its channel monitor up to date, as well as the update_monitor_ooo logic in ChannelMonitor.

Partially resolvesCloses#657

[codecov]

Codecov Report

Merging #667 into master will increase coverage by 0.10%.
The diff coverage is 92.47%.

@@ Coverage Diff @@## master #667 +/- ##
==========================================
+ Coverage 91.36% 91.46% +0.10% 
==========================================
Files 35 35 Lines 21703 21888 +185 ==========================================
+ Hits 19828 20019 +191 + Misses 1875 1869 -6 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	87.17% <ø> (-0.04%)	⬇️
lightning/src/ln/reorg_tests.rs	98.94% <ø> (ø)
lightning/src/ln/functional_tests.rs	97.15% <88.23%> (+0.14%)	⬆️
lightning/src/ln/channelmonitor.rs	95.56% <90.24%> (-0.15%)	⬇️
lightning/src/ln/channelmanager.rs	85.26% <96.96%> (+<0.01%)	⬆️
lightning/src/util/test_utils.rs	86.13% <100.00%> (ø)
lightning/src/ln/onchaintx.rs	93.75% <0.00%> (-0.20%)	⬇️
lightning/src/routing/router.rs	97.39% <0.00%> (+0.80%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3b4f43...28d9036. Read the comment docs.

[valentinewallace]

Oops, broke build -- will fix in a bit

[valentinewallace]

I looked into extracting some common HTLC selection functionality between monitor_would_broadcast and build_commitment_transaction but they're pretty different, they want different HTLCs, and monitor_would_broadcast needs local and remote HTLCs, and idk. Let me know if I'm missing something and it's definitely a good idea.

[valentinewallace]

Hmm, fuzzing seems slow still. Looking into it.

[arik-so]

not so much related to this diff in particular, but is the string literal the most elegant way to get the state name?

[valentinewallace]

Open to other options 😄 (for a future PR)

[ariard]

I'm all in to remove Channel's ChannelMonitor copy but I think there is a easier way than dc9ef2c. If I understand new would_broadcast_at_height_functionality well, its purpose is only to verify that ChannelMonitor accomplish its job well and broadcast channel update accordingly.

I'm not sure if it's a great approach, in the future if we add new conditions in ChannelMonitor to force-close channel we would need to access them too. Like in case of mempools-congestion, preemptively close channel at risk.

Another way to solve this would be a) pass a reference to ChainWatchInterface to ChannelManager b) at funding_tx detection, return its output to watch c) monitor commitment transaction broadcast. Parsing positively commitment transaction is easy to do due to transaction pattern (one-input), LN commitment number watermark and overall funding outpoint spending. It's more robust also as we shouldn't assume that our ChannelManager and ChannelMonitor are running on the same block provider and thus may have block view latency.

I know that's already the current model to query ChannelMonitor to decide if we should shutdown and broadcast channel update, but I think it's fine to wait a bit for channel update broadcast. Shutdown/broadcast will happen either sooner after any attempt to unsuccessfully update monitors. Or latter after block processing.

If you still want to be quick in term of updating global network view of our channel maybe we can ChannelMonitor signal broadcast with a boolean and us periodically querying it like we're doing for get_and_clear_pending_htlcs_updated ?

Overall, I may miss some onchain/offchain coupling assumptions but it feels like we can short-cut some complexity ?

[TheBlueMatt]

Ooooo, right, so I think that's true - would_broadcast_at_height is only used to close a channel and we could happily have the ChannelMonitor handle that (though currently Channel[Manager] handles that fully). My knee-jerk reaction was that there would be a race condition where we'd continue updating a channel state after its been closed (and the latest local commitment broadcast), but I think we could not even bother notifying the ChannelManager that we're closing the channel and wait for provide_latest_local_commitment_tx_info to return an Err because local_tx_signed is set to close the channel, so there's no race.

[TheBlueMatt]

I had to admit I'm somewhat surprised that's the only test changes you need, but looks good.

[TheBlueMatt]

Aside from the one clarifying change I suggested and the two comment nits, looks good!

[arik-so]

Out of curiosity, how do we decide which events are monitor events? For example, why is HTLCEvent for claiming one, but forwarding offered HTLCs isn't?

[TheBlueMatt]

In this context, its basically "a thing which we detected on-chain which has an impact on our understanding of a channel's state" (ie, basically, "channel has been closed on chain" or "transaction on-chain resolved an HTLC which was outstanding when we went on chain").

[arik-so]

One day we'll have versioning for reads lol :)

[valentinewallace]

Indeed lol

[ariard]

@valentinewallace

I think this is breaking one of the property of our distributed watchtower infrastructure as documented here : https://github.com/rust-bitcoin/rust-lightning/blob/3defcc896266f3d67848ce28981a756e971a3f0c/lightning/src/ln/channelmonitor.rs#L1174

AFAICT, by adding logs flag is true beyond updating local view with latest local state.

I'll fix it and add coverage for this.

[valentinewallace]

Logs flag? Ok I think I see what you mean though, thanks for catching this!

[TheBlueMatt]

We're discussing on IRC still, but I don't think this change is correct - we should refuse to accept updates after we've signed a local tx irrespective of why we signed it.

[ariard]

Sorry, after discussion on IRC and testing I realized that the alleged property was only supported in my mind. In fact #667 makes it easy to actually implement it, so did so in #679 with test coverage.