Fetch shutdown script based on commit_upfront_shutdown_pubkey

[jkczyz]

Rather than fetching a channel's shutdown script from KeysInterface at creation time, do so based on ChannelConfig::commit_upfront_shutdown_pubkey (i.e., either creation or shutdown time).

Additionally, support any acceptable shutdown script pubkey as defined by BOLT 2 while maintaining backwards compatibility with the legacy KeysInterface::get_shutdown_pubkey, which is replaced by KeysInterface::get_shutdown_scriptpubkey.

This change requires storing an optional script as a TLV field for both Channel and ChannelMonitor. Additionally, since the field is now optional, a new ChannelMonitorUpdateStep is needed for when the script is generated at shutdown time.

Fixes#994.

[codecov]

Codecov Report

Merging #1019 (b24b4e9) into main (69ee486) will increase coverage by 0.12%.
The diff coverage is 92.15%.

❗ Current head b24b4e9 differs from pull request most recent head 1d3861e. Consider uploading reports for the commit 1d3861e to get more accurate results

@@ Coverage Diff @@## main #1019 +/- ##
==========================================
+ Coverage 90.79% 90.92% +0.12% 
==========================================
Files 61 62 +1 Lines 31578 32253 +675 ==========================================
+ Hits 28672 29326 +654 - Misses 2906 2927 +21 

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	90.00% <ø> (ø)
lightning/src/util/errors.rs	67.30% <0.00%> (-4.13%)	⬇️
lightning/src/util/test_utils.rs	82.29% <76.00%> (-0.26%)	⬇️
lightning/src/chain/channelmonitor.rs	90.43% <83.33%> (-0.15%)	⬇️
lightning/src/ln/channelmanager.rs	86.82% <85.45%> (+1.12%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.11% <91.66%> (-0.07%)	⬇️
lightning/src/ln/script.rs	93.75% <93.75%> (ø)
lightning/src/ln/functional_tests.rs	97.23% <93.87%> (-0.05%)	⬇️
lightning/src/ln/channel.rs	89.34% <95.20%> (+0.11%)	⬆️
lightning-background-processor/src/lib.rs	95.48% <100.00%> (-0.10%)	⬇️
... and 7 more

---

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 69ee486...1d3861e. Read the comment docs.

[TheBlueMatt]

Oops, this needs a similar test fix in the background-processor as well - https://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=commit;h=9218054becc3dcf690bdd8fcf819952ee783365a

[TheBlueMatt]

I think we should return an Result here - version needs to be <= 16 so users can totally make it fail, we shouldn't panic in that case, no? Also, why do the conversion to u5 and then call new_witness_program? That seems like a very roundabout way to get there, just push the version followed by the program as done at https://docs.rs/bitcoin/0.27.0/src/bitcoin/blockdata/script.rs.html#280-290 (but do the push using push_int which does the right thing here, don't have to resort to manual opcode calculation https://docs.rs/bitcoin/0.27.0/src/bitcoin/blockdata/script.rs.html#706-720 )

[TheBlueMatt]

Also this will change soon upstream, not sure how that would impact this. rust-bitcoin/rust-bitcoin#617

[jkczyz]

Yeah, must cleaner like that. Will need to swap NonZeroU8 for WitnessVersion when it is available upstream.

[TheBlueMatt]

Should be handled like any other error - call handle_monitor_err to convert the error into a MsgHandleErrInternal, then store it in a variable outside the lock (like failed_htlcs and chan_option) and handle_error the error outside of the lock.

[jkczyz]

Hmmm... in the case of an error should we still send the shutdown message above? Similarly, should an error short-circuit any remaining code (e.g., failing back HTLCs, broadcasting channel update)?

[TheBlueMatt]

in the case of an error should we still send the shutdown message above?

There are two errors - permanent ones and temporary ones. In the case of permenent, yes, we should force-close instead and send an error message, for temporary, we should still send the shutdown.

failing back HTLCs,

We must never, ever forget to fail back htlcs we were told to fail back.

broadcasting channel update

This is only if we force-closed the channel due to an error, I think, so we shouldn't get to this point today if there's a monitor update, but a monitor update could cause us to get there now (if its a permanent one).

[jkczyz]

Ok, this turned out to be a bit more tricky than I imagined. Confirming my understanding below.

Change is in 49614d8. Let me know if that is what you had in mind and if there's a simpler way of going about this. Need to do the same for internal_shutdown.

There are two errors - permanent ones and temporary ones. In the case of permenent, yes, we should force-close instead and send an error message,

These are handled by handle_monitor_err and handle_error, respectively, IIUC.

for temporary, we should still send the shutdown.

What I don't quite understand is why we would send shutdown if we aren't certain that our ChannelMonitor has been updated with the shutdown script yet. Is it because if TemporaryError ever becomes a PermanentError, we wouldn't care about the shutdown script since we are force closing in that case?

[jkczyz]

Rebased and added for internal_shutdown, too: a024930

[TheBlueMatt]

Change is in 49614d8. Let me know if that is what you had in mind and if there's a simpler way of going about this. Need to do the same for internal_shutdown.

Nope, that's about right. These things are tricky to handle. I left a few specific comments on that commit.

These are handled by handle_monitor_err and handle_error, respectively, IIUC.

That's the MsgHandleErrInternal vs monitor-error distinction, not the permanent-vs-temporary distinction, both of which are handled by handle_monitor_err (and friends)

What I don't quite understand is why we would send shutdown if we aren't certain that our ChannelMonitor has been updated with the shutdown script yet. Is it because if TemporaryError ever becomes a PermanentError, we wouldn't care about the shutdown script since we are force closing in that case?

The Temporary->Permanent thing isn't a big deal, as you note. The real question is what happens if someone gives a Temporary update, and then takes longer to update their ChannelMonitor than we do doing the full closing_signed negotiation (because only afterclosing_signed negotiation could a transaction ever appear on chain with the script contained in the monitor update). Unlike most other monitor updates (at least RevokeAndACK), sending a Shutdown message isn't an immediate lock-in to a new states and the monitor doesn't need to know about it immediately, only before the closing transaction appears on chain.

If we wait for the update to complete here, then we have to add Shutdown resending, which I really don't want to do. An alternative, which is much easier, is to just wait for the monitor update to complete before ever sending a closing_signed, something I can shove in #985

[TheBlueMatt]

The ChannelMonitorUpdate changes look good to me, will re-review the rest a bit later but the rest was already basically good IMO.

[TheBlueMatt]

Basically LGTM

[valentinewallace]

Some of the new logic in ChannelManager is still sinking in for me, but overall this is looking good! Finished a first complete pass

[jkczyz]

Thanks for all the good feedback! Will see what I can do about the missing test coverage. It's a matter how easy it is to induce some error conditions (e.g., monitor update failing, keys provider giving an incompatible script).

[valentinewallace]

Is it the intended behavior to only broadcast a channel update if the channel was < ChannelState::FundingSent? Because I think this line will only return true if that's the situation 🤔 .

I think coverage is missing here too. I get that we're in "crunch time" though, so I'm pretty fine on punting tests if it comes to it

[jkczyz]

I believe it may also occur if we already handled a shutdown from the counterparty but didn't yet send our own shutdown.

[jkczyz]

Ah, actually now that I think of it, you're right. Since we lock channel_state, the situation that I mentioned can't occur.

[valentinewallace]

Yeah, I guess I just wanna get clear that this code block (and similar) are necessary, and what situation they would run in? Double checked and couldn't find any test coverage by adding prints

[TheBlueMatt]

I believe this code block is actually now unreachable. We return an Err in case we're ready to shut down here instead of getting into this block.

[jkczyz]

Removed code block from internal_shutdown rather than here as discussed on Slack.

[valentinewallace]

I can't really find anything to comment on, this is shaping up from my PoV!

[valentinewallace]

Prob not worth checking to see if they set a script here (which would make them a buggy peer), right?

[jkczyz]

They are allowed to support the feature but not necessarily set the shutdown script upfront, if I understand BOLT 2 correctly.

[valentinewallace]

They are allowed to support the feature but not necessarily set the shutdown script upfront,

IIUC, this is allowed unless both peers advertise support for the feature, in which case setting a script is required: from BOLT 2

I was concerned about the case where a peer didn't advertise the feature but still set a shutdown script up front, but checked the spec and that seems to be allowed 🤷

[TheBlueMatt]

It looks like this was rebased without any conflict, was there a reason for the rebase I missed?

[jkczyz]

Added the requested tests. For testing IncompatibleShutdownScript errors, I needed to add InitFeatures to NodeCfg and pass it when connecting peers (see separate commit 2e03385) since ChannelManager::close_channel pulls the features from per_peer_state. We could probably use this in some of the test utilities instead of passing &InitFeatures::known() everywhere, but that's a more involved follow-up.

It looks like this was rebased without any conflict, was there a reason for the rebase I missed?

I was probably proactively rebasing just in case. 🙂

[jkczyz]

They are allowed to support the feature but not necessarily set the shutdown script upfront, if I understand BOLT 2 correctly.

[TheBlueMatt]

I was probably proactively rebasing just in case. slightly_smiling_face

Oops, in general is it possible to avoid this? It makes it hard to diff-tree between revisions to see what changed.

[valentinewallace]

ACK mod one last clarification. Thanks for the additional test coverage added!

[valentinewallace]

They are allowed to support the feature but not necessarily set the shutdown script upfront,

IIUC, this is allowed unless both peers advertise support for the feature, in which case setting a script is required: from BOLT 2

I was concerned about the case where a peer didn't advertise the feature but still set a shutdown script up front, but checked the spec and that seems to be allowed 🤷

[valentinewallace]

Yeah, I guess I just wanna get clear that this code block (and similar) are necessary, and what situation they would run in? Double checked and couldn't find any test coverage by adding prints

[TheBlueMatt]

ACK post-squash.

[jkczyz]

Rather than fetching a channel's shutdown script from KeysInterface at creation time, do so based on ChannelConfig::commit_upfront_shutdown_pubkey (i.e., either creation or shutdown time).

Additionally, support any acceptable shutdown script pubkey as defined by BOLT 2 while maintaining backwards compatibility with the legacy KeysInterface::get_shutdown_pubkey, which is replaced by KeysInterface::get_shutdown_scriptpubkey.

This change requires storing an optional script as a TLV field for both Channel and ChannelMonitor. Additionally, since the field is now optional, a new ChannelMonitorUpdateStep is needed for when the script is generated at shutdown time.

Fixes#994.

[codecov]

Codecov Report

Merging #1019 (b24b4e9) into main (69ee486) will increase coverage by 0.12%.
The diff coverage is 92.15%.

❗ Current head b24b4e9 differs from pull request most recent head 1d3861e. Consider uploading reports for the commit 1d3861e to get more accurate results

@@ Coverage Diff @@## main #1019 +/- ##
==========================================
+ Coverage 90.79% 90.92% +0.12% 
==========================================
Files 61 62 +1 Lines 31578 32253 +675 ==========================================
+ Hits 28672 29326 +654 - Misses 2906 2927 +21 

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	90.00% <ø> (ø)
lightning/src/util/errors.rs	67.30% <0.00%> (-4.13%)	⬇️
lightning/src/util/test_utils.rs	82.29% <76.00%> (-0.26%)	⬇️
lightning/src/chain/channelmonitor.rs	90.43% <83.33%> (-0.15%)	⬇️
lightning/src/ln/channelmanager.rs	86.82% <85.45%> (+1.12%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.11% <91.66%> (-0.07%)	⬇️
lightning/src/ln/script.rs	93.75% <93.75%> (ø)
lightning/src/ln/functional_tests.rs	97.23% <93.87%> (-0.05%)	⬇️
lightning/src/ln/channel.rs	89.34% <95.20%> (+0.11%)	⬆️
lightning-background-processor/src/lib.rs	95.48% <100.00%> (-0.10%)	⬇️
... and 7 more

---

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 69ee486...1d3861e. Read the comment docs.

[TheBlueMatt]

Oops, this needs a similar test fix in the background-processor as well - https://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=commit;h=9218054becc3dcf690bdd8fcf819952ee783365a

[TheBlueMatt]

I think we should return an Result here - version needs to be <= 16 so users can totally make it fail, we shouldn't panic in that case, no? Also, why do the conversion to u5 and then call new_witness_program? That seems like a very roundabout way to get there, just push the version followed by the program as done at https://docs.rs/bitcoin/0.27.0/src/bitcoin/blockdata/script.rs.html#280-290 (but do the push using push_int which does the right thing here, don't have to resort to manual opcode calculation https://docs.rs/bitcoin/0.27.0/src/bitcoin/blockdata/script.rs.html#706-720 )

[TheBlueMatt]

Also this will change soon upstream, not sure how that would impact this. rust-bitcoin/rust-bitcoin#617

[jkczyz]

Yeah, must cleaner like that. Will need to swap NonZeroU8 for WitnessVersion when it is available upstream.

[TheBlueMatt]

Should be handled like any other error - call handle_monitor_err to convert the error into a MsgHandleErrInternal, then store it in a variable outside the lock (like failed_htlcs and chan_option) and handle_error the error outside of the lock.

[jkczyz]

Hmmm... in the case of an error should we still send the shutdown message above? Similarly, should an error short-circuit any remaining code (e.g., failing back HTLCs, broadcasting channel update)?

[TheBlueMatt]

in the case of an error should we still send the shutdown message above?

There are two errors - permanent ones and temporary ones. In the case of permenent, yes, we should force-close instead and send an error message, for temporary, we should still send the shutdown.

failing back HTLCs,

We must never, ever forget to fail back htlcs we were told to fail back.

broadcasting channel update

This is only if we force-closed the channel due to an error, I think, so we shouldn't get to this point today if there's a monitor update, but a monitor update could cause us to get there now (if its a permanent one).

[jkczyz]

Ok, this turned out to be a bit more tricky than I imagined. Confirming my understanding below.

Change is in 49614d8. Let me know if that is what you had in mind and if there's a simpler way of going about this. Need to do the same for internal_shutdown.

There are two errors - permanent ones and temporary ones. In the case of permenent, yes, we should force-close instead and send an error message,

These are handled by handle_monitor_err and handle_error, respectively, IIUC.

for temporary, we should still send the shutdown.

What I don't quite understand is why we would send shutdown if we aren't certain that our ChannelMonitor has been updated with the shutdown script yet. Is it because if TemporaryError ever becomes a PermanentError, we wouldn't care about the shutdown script since we are force closing in that case?

[jkczyz]

Rebased and added for internal_shutdown, too: a024930

[TheBlueMatt]

Change is in 49614d8. Let me know if that is what you had in mind and if there's a simpler way of going about this. Need to do the same for internal_shutdown.

Nope, that's about right. These things are tricky to handle. I left a few specific comments on that commit.

These are handled by handle_monitor_err and handle_error, respectively, IIUC.

That's the MsgHandleErrInternal vs monitor-error distinction, not the permanent-vs-temporary distinction, both of which are handled by handle_monitor_err (and friends)

What I don't quite understand is why we would send shutdown if we aren't certain that our ChannelMonitor has been updated with the shutdown script yet. Is it because if TemporaryError ever becomes a PermanentError, we wouldn't care about the shutdown script since we are force closing in that case?

The Temporary->Permanent thing isn't a big deal, as you note. The real question is what happens if someone gives a Temporary update, and then takes longer to update their ChannelMonitor than we do doing the full closing_signed negotiation (because only afterclosing_signed negotiation could a transaction ever appear on chain with the script contained in the monitor update). Unlike most other monitor updates (at least RevokeAndACK), sending a Shutdown message isn't an immediate lock-in to a new states and the monitor doesn't need to know about it immediately, only before the closing transaction appears on chain.

If we wait for the update to complete here, then we have to add Shutdown resending, which I really don't want to do. An alternative, which is much easier, is to just wait for the monitor update to complete before ever sending a closing_signed, something I can shove in #985

[TheBlueMatt]

The ChannelMonitorUpdate changes look good to me, will re-review the rest a bit later but the rest was already basically good IMO.

[TheBlueMatt]

Basically LGTM

[valentinewallace]

Some of the new logic in ChannelManager is still sinking in for me, but overall this is looking good! Finished a first complete pass

[jkczyz]

Thanks for all the good feedback! Will see what I can do about the missing test coverage. It's a matter how easy it is to induce some error conditions (e.g., monitor update failing, keys provider giving an incompatible script).

[valentinewallace]

Is it the intended behavior to only broadcast a channel update if the channel was < ChannelState::FundingSent? Because I think this line will only return true if that's the situation 🤔 .

I think coverage is missing here too. I get that we're in "crunch time" though, so I'm pretty fine on punting tests if it comes to it

[jkczyz]

I believe it may also occur if we already handled a shutdown from the counterparty but didn't yet send our own shutdown.

[jkczyz]

Ah, actually now that I think of it, you're right. Since we lock channel_state, the situation that I mentioned can't occur.

[valentinewallace]

Yeah, I guess I just wanna get clear that this code block (and similar) are necessary, and what situation they would run in? Double checked and couldn't find any test coverage by adding prints

[TheBlueMatt]

I believe this code block is actually now unreachable. We return an Err in case we're ready to shut down here instead of getting into this block.

[jkczyz]

Removed code block from internal_shutdown rather than here as discussed on Slack.

[valentinewallace]

I can't really find anything to comment on, this is shaping up from my PoV!

[valentinewallace]

Prob not worth checking to see if they set a script here (which would make them a buggy peer), right?

[jkczyz]

They are allowed to support the feature but not necessarily set the shutdown script upfront, if I understand BOLT 2 correctly.

[valentinewallace]

They are allowed to support the feature but not necessarily set the shutdown script upfront,

IIUC, this is allowed unless both peers advertise support for the feature, in which case setting a script is required: from BOLT 2

I was concerned about the case where a peer didn't advertise the feature but still set a shutdown script up front, but checked the spec and that seems to be allowed 🤷

[TheBlueMatt]

It looks like this was rebased without any conflict, was there a reason for the rebase I missed?

[jkczyz]

Added the requested tests. For testing IncompatibleShutdownScript errors, I needed to add InitFeatures to NodeCfg and pass it when connecting peers (see separate commit 2e03385) since ChannelManager::close_channel pulls the features from per_peer_state. We could probably use this in some of the test utilities instead of passing &InitFeatures::known() everywhere, but that's a more involved follow-up.

It looks like this was rebased without any conflict, was there a reason for the rebase I missed?

I was probably proactively rebasing just in case. 🙂

[jkczyz]

They are allowed to support the feature but not necessarily set the shutdown script upfront, if I understand BOLT 2 correctly.

[TheBlueMatt]

I was probably proactively rebasing just in case. slightly_smiling_face

Oops, in general is it possible to avoid this? It makes it hard to diff-tree between revisions to see what changed.

[valentinewallace]

ACK mod one last clarification. Thanks for the additional test coverage added!

[valentinewallace]

They are allowed to support the feature but not necessarily set the shutdown script upfront,

IIUC, this is allowed unless both peers advertise support for the feature, in which case setting a script is required: from BOLT 2

I was concerned about the case where a peer didn't advertise the feature but still set a shutdown script up front, but checked the spec and that seems to be allowed 🤷

[valentinewallace]

Yeah, I guess I just wanna get clear that this code block (and similar) are necessary, and what situation they would run in? Double checked and couldn't find any test coverage by adding prints

[TheBlueMatt]

ACK post-squash.

[jkczyz]

Rather than fetching a channel's shutdown script from KeysInterface at creation time, do so based on ChannelConfig::commit_upfront_shutdown_pubkey (i.e., either creation or shutdown time).

Additionally, support any acceptable shutdown script pubkey as defined by BOLT 2 while maintaining backwards compatibility with the legacy KeysInterface::get_shutdown_pubkey, which is replaced by KeysInterface::get_shutdown_scriptpubkey.

This change requires storing an optional script as a TLV field for both Channel and ChannelMonitor. Additionally, since the field is now optional, a new ChannelMonitorUpdateStep is needed for when the script is generated at shutdown time.

Fixes#994.

[codecov]

Codecov Report

Merging #1019 (b24b4e9) into main (69ee486) will increase coverage by 0.12%.
The diff coverage is 92.15%.

❗ Current head b24b4e9 differs from pull request most recent head 1d3861e. Consider uploading reports for the commit 1d3861e to get more accurate results

@@ Coverage Diff @@## main #1019 +/- ##
==========================================
+ Coverage 90.79% 90.92% +0.12% 
==========================================
Files 61 62 +1 Lines 31578 32253 +675 ==========================================
+ Hits 28672 29326 +654 - Misses 2906 2927 +21 

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	90.00% <ø> (ø)
lightning/src/util/errors.rs	67.30% <0.00%> (-4.13%)	⬇️
lightning/src/util/test_utils.rs	82.29% <76.00%> (-0.26%)	⬇️
lightning/src/chain/channelmonitor.rs	90.43% <83.33%> (-0.15%)	⬇️
lightning/src/ln/channelmanager.rs	86.82% <85.45%> (+1.12%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.11% <91.66%> (-0.07%)	⬇️
lightning/src/ln/script.rs	93.75% <93.75%> (ø)
lightning/src/ln/functional_tests.rs	97.23% <93.87%> (-0.05%)	⬇️
lightning/src/ln/channel.rs	89.34% <95.20%> (+0.11%)	⬆️
lightning-background-processor/src/lib.rs	95.48% <100.00%> (-0.10%)	⬇️
... and 7 more

---

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 69ee486...1d3861e. Read the comment docs.

[TheBlueMatt]

Oops, this needs a similar test fix in the background-processor as well - https://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=commit;h=9218054becc3dcf690bdd8fcf819952ee783365a

[TheBlueMatt]

I think we should return an Result here - version needs to be <= 16 so users can totally make it fail, we shouldn't panic in that case, no? Also, why do the conversion to u5 and then call new_witness_program? That seems like a very roundabout way to get there, just push the version followed by the program as done at https://docs.rs/bitcoin/0.27.0/src/bitcoin/blockdata/script.rs.html#280-290 (but do the push using push_int which does the right thing here, don't have to resort to manual opcode calculation https://docs.rs/bitcoin/0.27.0/src/bitcoin/blockdata/script.rs.html#706-720 )

[TheBlueMatt]

Also this will change soon upstream, not sure how that would impact this. rust-bitcoin/rust-bitcoin#617

[jkczyz]

Yeah, must cleaner like that. Will need to swap NonZeroU8 for WitnessVersion when it is available upstream.

[TheBlueMatt]

Should be handled like any other error - call handle_monitor_err to convert the error into a MsgHandleErrInternal, then store it in a variable outside the lock (like failed_htlcs and chan_option) and handle_error the error outside of the lock.

[jkczyz]

Hmmm... in the case of an error should we still send the shutdown message above? Similarly, should an error short-circuit any remaining code (e.g., failing back HTLCs, broadcasting channel update)?

[TheBlueMatt]

in the case of an error should we still send the shutdown message above?

There are two errors - permanent ones and temporary ones. In the case of permenent, yes, we should force-close instead and send an error message, for temporary, we should still send the shutdown.

failing back HTLCs,

We must never, ever forget to fail back htlcs we were told to fail back.

broadcasting channel update

This is only if we force-closed the channel due to an error, I think, so we shouldn't get to this point today if there's a monitor update, but a monitor update could cause us to get there now (if its a permanent one).

[jkczyz]

Ok, this turned out to be a bit more tricky than I imagined. Confirming my understanding below.

Change is in 49614d8. Let me know if that is what you had in mind and if there's a simpler way of going about this. Need to do the same for internal_shutdown.

There are two errors - permanent ones and temporary ones. In the case of permenent, yes, we should force-close instead and send an error message,

These are handled by handle_monitor_err and handle_error, respectively, IIUC.

for temporary, we should still send the shutdown.

What I don't quite understand is why we would send shutdown if we aren't certain that our ChannelMonitor has been updated with the shutdown script yet. Is it because if TemporaryError ever becomes a PermanentError, we wouldn't care about the shutdown script since we are force closing in that case?

[jkczyz]

Rebased and added for internal_shutdown, too: a024930

[TheBlueMatt]

Change is in 49614d8. Let me know if that is what you had in mind and if there's a simpler way of going about this. Need to do the same for internal_shutdown.

Nope, that's about right. These things are tricky to handle. I left a few specific comments on that commit.

These are handled by handle_monitor_err and handle_error, respectively, IIUC.

That's the MsgHandleErrInternal vs monitor-error distinction, not the permanent-vs-temporary distinction, both of which are handled by handle_monitor_err (and friends)

What I don't quite understand is why we would send shutdown if we aren't certain that our ChannelMonitor has been updated with the shutdown script yet. Is it because if TemporaryError ever becomes a PermanentError, we wouldn't care about the shutdown script since we are force closing in that case?

The Temporary->Permanent thing isn't a big deal, as you note. The real question is what happens if someone gives a Temporary update, and then takes longer to update their ChannelMonitor than we do doing the full closing_signed negotiation (because only afterclosing_signed negotiation could a transaction ever appear on chain with the script contained in the monitor update). Unlike most other monitor updates (at least RevokeAndACK), sending a Shutdown message isn't an immediate lock-in to a new states and the monitor doesn't need to know about it immediately, only before the closing transaction appears on chain.

If we wait for the update to complete here, then we have to add Shutdown resending, which I really don't want to do. An alternative, which is much easier, is to just wait for the monitor update to complete before ever sending a closing_signed, something I can shove in #985

[TheBlueMatt]

The ChannelMonitorUpdate changes look good to me, will re-review the rest a bit later but the rest was already basically good IMO.

[TheBlueMatt]

Basically LGTM

[valentinewallace]

Some of the new logic in ChannelManager is still sinking in for me, but overall this is looking good! Finished a first complete pass

[jkczyz]

Thanks for all the good feedback! Will see what I can do about the missing test coverage. It's a matter how easy it is to induce some error conditions (e.g., monitor update failing, keys provider giving an incompatible script).

[valentinewallace]

Is it the intended behavior to only broadcast a channel update if the channel was < ChannelState::FundingSent? Because I think this line will only return true if that's the situation 🤔 .

I think coverage is missing here too. I get that we're in "crunch time" though, so I'm pretty fine on punting tests if it comes to it

[jkczyz]

I believe it may also occur if we already handled a shutdown from the counterparty but didn't yet send our own shutdown.

[jkczyz]

Ah, actually now that I think of it, you're right. Since we lock channel_state, the situation that I mentioned can't occur.

[valentinewallace]

Yeah, I guess I just wanna get clear that this code block (and similar) are necessary, and what situation they would run in? Double checked and couldn't find any test coverage by adding prints

[TheBlueMatt]

I believe this code block is actually now unreachable. We return an Err in case we're ready to shut down here instead of getting into this block.

[jkczyz]

Removed code block from internal_shutdown rather than here as discussed on Slack.

[valentinewallace]

I can't really find anything to comment on, this is shaping up from my PoV!

[valentinewallace]

Prob not worth checking to see if they set a script here (which would make them a buggy peer), right?

[jkczyz]

They are allowed to support the feature but not necessarily set the shutdown script upfront, if I understand BOLT 2 correctly.

[valentinewallace]

They are allowed to support the feature but not necessarily set the shutdown script upfront,

IIUC, this is allowed unless both peers advertise support for the feature, in which case setting a script is required: from BOLT 2

I was concerned about the case where a peer didn't advertise the feature but still set a shutdown script up front, but checked the spec and that seems to be allowed 🤷

[TheBlueMatt]

It looks like this was rebased without any conflict, was there a reason for the rebase I missed?

[jkczyz]

Added the requested tests. For testing IncompatibleShutdownScript errors, I needed to add InitFeatures to NodeCfg and pass it when connecting peers (see separate commit 2e03385) since ChannelManager::close_channel pulls the features from per_peer_state. We could probably use this in some of the test utilities instead of passing &InitFeatures::known() everywhere, but that's a more involved follow-up.

It looks like this was rebased without any conflict, was there a reason for the rebase I missed?

I was probably proactively rebasing just in case. 🙂

[jkczyz]

They are allowed to support the feature but not necessarily set the shutdown script upfront, if I understand BOLT 2 correctly.

[TheBlueMatt]

I was probably proactively rebasing just in case. slightly_smiling_face

Oops, in general is it possible to avoid this? It makes it hard to diff-tree between revisions to see what changed.

[valentinewallace]

ACK mod one last clarification. Thanks for the additional test coverage added!

[valentinewallace]

They are allowed to support the feature but not necessarily set the shutdown script upfront,

IIUC, this is allowed unless both peers advertise support for the feature, in which case setting a script is required: from BOLT 2

I was concerned about the case where a peer didn't advertise the feature but still set a shutdown script up front, but checked the spec and that seems to be allowed 🤷

[valentinewallace]

Yeah, I guess I just wanna get clear that this code block (and similar) are necessary, and what situation they would run in? Double checked and couldn't find any test coverage by adding prints

[TheBlueMatt]

ACK post-squash.

[jkczyz]

Rather than fetching a channel's shutdown script from KeysInterface at creation time, do so based on ChannelConfig::commit_upfront_shutdown_pubkey (i.e., either creation or shutdown time).

Additionally, support any acceptable shutdown script pubkey as defined by BOLT 2 while maintaining backwards compatibility with the legacy KeysInterface::get_shutdown_pubkey, which is replaced by KeysInterface::get_shutdown_scriptpubkey.

This change requires storing an optional script as a TLV field for both Channel and ChannelMonitor. Additionally, since the field is now optional, a new ChannelMonitorUpdateStep is needed for when the script is generated at shutdown time.

Fixes#994.

[codecov]

Codecov Report

Merging #1019 (b24b4e9) into main (69ee486) will increase coverage by 0.12%.
The diff coverage is 92.15%.

❗ Current head b24b4e9 differs from pull request most recent head 1d3861e. Consider uploading reports for the commit 1d3861e to get more accurate results

@@ Coverage Diff @@## main #1019 +/- ##
==========================================
+ Coverage 90.79% 90.92% +0.12% 
==========================================
Files 61 62 +1 Lines 31578 32253 +675 ==========================================
+ Hits 28672 29326 +654 - Misses 2906 2927 +21 

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	90.00% <ø> (ø)
lightning/src/util/errors.rs	67.30% <0.00%> (-4.13%)	⬇️
lightning/src/util/test_utils.rs	82.29% <76.00%> (-0.26%)	⬇️
lightning/src/chain/channelmonitor.rs	90.43% <83.33%> (-0.15%)	⬇️
lightning/src/ln/channelmanager.rs	86.82% <85.45%> (+1.12%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.11% <91.66%> (-0.07%)	⬇️
lightning/src/ln/script.rs	93.75% <93.75%> (ø)
lightning/src/ln/functional_tests.rs	97.23% <93.87%> (-0.05%)	⬇️
lightning/src/ln/channel.rs	89.34% <95.20%> (+0.11%)	⬆️
lightning-background-processor/src/lib.rs	95.48% <100.00%> (-0.10%)	⬇️
... and 7 more

---

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 69ee486...1d3861e. Read the comment docs.

[TheBlueMatt]

Oops, this needs a similar test fix in the background-processor as well - https://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=commit;h=9218054becc3dcf690bdd8fcf819952ee783365a

[TheBlueMatt]

I think we should return an Result here - version needs to be <= 16 so users can totally make it fail, we shouldn't panic in that case, no? Also, why do the conversion to u5 and then call new_witness_program? That seems like a very roundabout way to get there, just push the version followed by the program as done at https://docs.rs/bitcoin/0.27.0/src/bitcoin/blockdata/script.rs.html#280-290 (but do the push using push_int which does the right thing here, don't have to resort to manual opcode calculation https://docs.rs/bitcoin/0.27.0/src/bitcoin/blockdata/script.rs.html#706-720 )

[TheBlueMatt]

Also this will change soon upstream, not sure how that would impact this. rust-bitcoin/rust-bitcoin#617

[jkczyz]

Yeah, must cleaner like that. Will need to swap NonZeroU8 for WitnessVersion when it is available upstream.

[TheBlueMatt]

Should be handled like any other error - call handle_monitor_err to convert the error into a MsgHandleErrInternal, then store it in a variable outside the lock (like failed_htlcs and chan_option) and handle_error the error outside of the lock.

[jkczyz]

Hmmm... in the case of an error should we still send the shutdown message above? Similarly, should an error short-circuit any remaining code (e.g., failing back HTLCs, broadcasting channel update)?

[TheBlueMatt]

in the case of an error should we still send the shutdown message above?

There are two errors - permanent ones and temporary ones. In the case of permenent, yes, we should force-close instead and send an error message, for temporary, we should still send the shutdown.

failing back HTLCs,

We must never, ever forget to fail back htlcs we were told to fail back.

broadcasting channel update

This is only if we force-closed the channel due to an error, I think, so we shouldn't get to this point today if there's a monitor update, but a monitor update could cause us to get there now (if its a permanent one).

[jkczyz]

Ok, this turned out to be a bit more tricky than I imagined. Confirming my understanding below.

Change is in 49614d8. Let me know if that is what you had in mind and if there's a simpler way of going about this. Need to do the same for internal_shutdown.

There are two errors - permanent ones and temporary ones. In the case of permenent, yes, we should force-close instead and send an error message,

These are handled by handle_monitor_err and handle_error, respectively, IIUC.

for temporary, we should still send the shutdown.

What I don't quite understand is why we would send shutdown if we aren't certain that our ChannelMonitor has been updated with the shutdown script yet. Is it because if TemporaryError ever becomes a PermanentError, we wouldn't care about the shutdown script since we are force closing in that case?

[jkczyz]

Rebased and added for internal_shutdown, too: a024930

[TheBlueMatt]

Change is in 49614d8. Let me know if that is what you had in mind and if there's a simpler way of going about this. Need to do the same for internal_shutdown.

Nope, that's about right. These things are tricky to handle. I left a few specific comments on that commit.

These are handled by handle_monitor_err and handle_error, respectively, IIUC.

That's the MsgHandleErrInternal vs monitor-error distinction, not the permanent-vs-temporary distinction, both of which are handled by handle_monitor_err (and friends)

What I don't quite understand is why we would send shutdown if we aren't certain that our ChannelMonitor has been updated with the shutdown script yet. Is it because if TemporaryError ever becomes a PermanentError, we wouldn't care about the shutdown script since we are force closing in that case?

The Temporary->Permanent thing isn't a big deal, as you note. The real question is what happens if someone gives a Temporary update, and then takes longer to update their ChannelMonitor than we do doing the full closing_signed negotiation (because only afterclosing_signed negotiation could a transaction ever appear on chain with the script contained in the monitor update). Unlike most other monitor updates (at least RevokeAndACK), sending a Shutdown message isn't an immediate lock-in to a new states and the monitor doesn't need to know about it immediately, only before the closing transaction appears on chain.

If we wait for the update to complete here, then we have to add Shutdown resending, which I really don't want to do. An alternative, which is much easier, is to just wait for the monitor update to complete before ever sending a closing_signed, something I can shove in #985

[TheBlueMatt]

The ChannelMonitorUpdate changes look good to me, will re-review the rest a bit later but the rest was already basically good IMO.

[TheBlueMatt]

Basically LGTM

[valentinewallace]

Some of the new logic in ChannelManager is still sinking in for me, but overall this is looking good! Finished a first complete pass

[jkczyz]

Thanks for all the good feedback! Will see what I can do about the missing test coverage. It's a matter how easy it is to induce some error conditions (e.g., monitor update failing, keys provider giving an incompatible script).

[valentinewallace]

Is it the intended behavior to only broadcast a channel update if the channel was < ChannelState::FundingSent? Because I think this line will only return true if that's the situation 🤔 .

I think coverage is missing here too. I get that we're in "crunch time" though, so I'm pretty fine on punting tests if it comes to it

[jkczyz]

I believe it may also occur if we already handled a shutdown from the counterparty but didn't yet send our own shutdown.

[jkczyz]

Ah, actually now that I think of it, you're right. Since we lock channel_state, the situation that I mentioned can't occur.

[valentinewallace]

Yeah, I guess I just wanna get clear that this code block (and similar) are necessary, and what situation they would run in? Double checked and couldn't find any test coverage by adding prints

[TheBlueMatt]

I believe this code block is actually now unreachable. We return an Err in case we're ready to shut down here instead of getting into this block.

[jkczyz]

Removed code block from internal_shutdown rather than here as discussed on Slack.

[valentinewallace]

I can't really find anything to comment on, this is shaping up from my PoV!

[valentinewallace]

Prob not worth checking to see if they set a script here (which would make them a buggy peer), right?

[jkczyz]

They are allowed to support the feature but not necessarily set the shutdown script upfront, if I understand BOLT 2 correctly.

[valentinewallace]

They are allowed to support the feature but not necessarily set the shutdown script upfront,

IIUC, this is allowed unless both peers advertise support for the feature, in which case setting a script is required: from BOLT 2

I was concerned about the case where a peer didn't advertise the feature but still set a shutdown script up front, but checked the spec and that seems to be allowed 🤷

[TheBlueMatt]

It looks like this was rebased without any conflict, was there a reason for the rebase I missed?

[jkczyz]

Added the requested tests. For testing IncompatibleShutdownScript errors, I needed to add InitFeatures to NodeCfg and pass it when connecting peers (see separate commit 2e03385) since ChannelManager::close_channel pulls the features from per_peer_state. We could probably use this in some of the test utilities instead of passing &InitFeatures::known() everywhere, but that's a more involved follow-up.

It looks like this was rebased without any conflict, was there a reason for the rebase I missed?

I was probably proactively rebasing just in case. 🙂

[jkczyz]

They are allowed to support the feature but not necessarily set the shutdown script upfront, if I understand BOLT 2 correctly.

[TheBlueMatt]

I was probably proactively rebasing just in case. slightly_smiling_face

Oops, in general is it possible to avoid this? It makes it hard to diff-tree between revisions to see what changed.

[valentinewallace]

ACK mod one last clarification. Thanks for the additional test coverage added!

[valentinewallace]

They are allowed to support the feature but not necessarily set the shutdown script upfront,

IIUC, this is allowed unless both peers advertise support for the feature, in which case setting a script is required: from BOLT 2

I was concerned about the case where a peer didn't advertise the feature but still set a shutdown script up front, but checked the spec and that seems to be allowed 🤷

[valentinewallace]

Yeah, I guess I just wanna get clear that this code block (and similar) are necessary, and what situation they would run in? Double checked and couldn't find any test coverage by adding prints

[TheBlueMatt]

ACK post-squash.

[jkczyz]

Rather than fetching a channel's shutdown script from KeysInterface at creation time, do so based on ChannelConfig::commit_upfront_shutdown_pubkey (i.e., either creation or shutdown time).

Additionally, support any acceptable shutdown script pubkey as defined by BOLT 2 while maintaining backwards compatibility with the legacy KeysInterface::get_shutdown_pubkey, which is replaced by KeysInterface::get_shutdown_scriptpubkey.

This change requires storing an optional script as a TLV field for both Channel and ChannelMonitor. Additionally, since the field is now optional, a new ChannelMonitorUpdateStep is needed for when the script is generated at shutdown time.

Fixes#994.

[codecov]

Codecov Report

Merging #1019 (b24b4e9) into main (69ee486) will increase coverage by 0.12%.
The diff coverage is 92.15%.

❗ Current head b24b4e9 differs from pull request most recent head 1d3861e. Consider uploading reports for the commit 1d3861e to get more accurate results

@@ Coverage Diff @@## main #1019 +/- ##
==========================================
+ Coverage 90.79% 90.92% +0.12% 
==========================================
Files 61 62 +1 Lines 31578 32253 +675 ==========================================
+ Hits 28672 29326 +654 - Misses 2906 2927 +21 

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	90.00% <ø> (ø)
lightning/src/util/errors.rs	67.30% <0.00%> (-4.13%)	⬇️
lightning/src/util/test_utils.rs	82.29% <76.00%> (-0.26%)	⬇️
lightning/src/chain/channelmonitor.rs	90.43% <83.33%> (-0.15%)	⬇️
lightning/src/ln/channelmanager.rs	86.82% <85.45%> (+1.12%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.11% <91.66%> (-0.07%)	⬇️
lightning/src/ln/script.rs	93.75% <93.75%> (ø)
lightning/src/ln/functional_tests.rs	97.23% <93.87%> (-0.05%)	⬇️
lightning/src/ln/channel.rs	89.34% <95.20%> (+0.11%)	⬆️
lightning-background-processor/src/lib.rs	95.48% <100.00%> (-0.10%)	⬇️
... and 7 more

---

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 69ee486...1d3861e. Read the comment docs.

[TheBlueMatt]

Oops, this needs a similar test fix in the background-processor as well - https://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=commit;h=9218054becc3dcf690bdd8fcf819952ee783365a

[TheBlueMatt]

I think we should return an Result here - version needs to be <= 16 so users can totally make it fail, we shouldn't panic in that case, no? Also, why do the conversion to u5 and then call new_witness_program? That seems like a very roundabout way to get there, just push the version followed by the program as done at https://docs.rs/bitcoin/0.27.0/src/bitcoin/blockdata/script.rs.html#280-290 (but do the push using push_int which does the right thing here, don't have to resort to manual opcode calculation https://docs.rs/bitcoin/0.27.0/src/bitcoin/blockdata/script.rs.html#706-720 )

[TheBlueMatt]

Also this will change soon upstream, not sure how that would impact this. rust-bitcoin/rust-bitcoin#617

[jkczyz]

Yeah, must cleaner like that. Will need to swap NonZeroU8 for WitnessVersion when it is available upstream.

[TheBlueMatt]

Should be handled like any other error - call handle_monitor_err to convert the error into a MsgHandleErrInternal, then store it in a variable outside the lock (like failed_htlcs and chan_option) and handle_error the error outside of the lock.

[jkczyz]

Hmmm... in the case of an error should we still send the shutdown message above? Similarly, should an error short-circuit any remaining code (e.g., failing back HTLCs, broadcasting channel update)?

[TheBlueMatt]

in the case of an error should we still send the shutdown message above?

There are two errors - permanent ones and temporary ones. In the case of permenent, yes, we should force-close instead and send an error message, for temporary, we should still send the shutdown.

failing back HTLCs,

We must never, ever forget to fail back htlcs we were told to fail back.

broadcasting channel update

This is only if we force-closed the channel due to an error, I think, so we shouldn't get to this point today if there's a monitor update, but a monitor update could cause us to get there now (if its a permanent one).

[jkczyz]

Ok, this turned out to be a bit more tricky than I imagined. Confirming my understanding below.

Change is in 49614d8. Let me know if that is what you had in mind and if there's a simpler way of going about this. Need to do the same for internal_shutdown.

There are two errors - permanent ones and temporary ones. In the case of permenent, yes, we should force-close instead and send an error message,

These are handled by handle_monitor_err and handle_error, respectively, IIUC.

for temporary, we should still send the shutdown.

What I don't quite understand is why we would send shutdown if we aren't certain that our ChannelMonitor has been updated with the shutdown script yet. Is it because if TemporaryError ever becomes a PermanentError, we wouldn't care about the shutdown script since we are force closing in that case?

[jkczyz]

Rebased and added for internal_shutdown, too: a024930

[TheBlueMatt]

Change is in 49614d8. Let me know if that is what you had in mind and if there's a simpler way of going about this. Need to do the same for internal_shutdown.

Nope, that's about right. These things are tricky to handle. I left a few specific comments on that commit.

These are handled by handle_monitor_err and handle_error, respectively, IIUC.

That's the MsgHandleErrInternal vs monitor-error distinction, not the permanent-vs-temporary distinction, both of which are handled by handle_monitor_err (and friends)

What I don't quite understand is why we would send shutdown if we aren't certain that our ChannelMonitor has been updated with the shutdown script yet. Is it because if TemporaryError ever becomes a PermanentError, we wouldn't care about the shutdown script since we are force closing in that case?

The Temporary->Permanent thing isn't a big deal, as you note. The real question is what happens if someone gives a Temporary update, and then takes longer to update their ChannelMonitor than we do doing the full closing_signed negotiation (because only afterclosing_signed negotiation could a transaction ever appear on chain with the script contained in the monitor update). Unlike most other monitor updates (at least RevokeAndACK), sending a Shutdown message isn't an immediate lock-in to a new states and the monitor doesn't need to know about it immediately, only before the closing transaction appears on chain.

If we wait for the update to complete here, then we have to add Shutdown resending, which I really don't want to do. An alternative, which is much easier, is to just wait for the monitor update to complete before ever sending a closing_signed, something I can shove in #985

[TheBlueMatt]

The ChannelMonitorUpdate changes look good to me, will re-review the rest a bit later but the rest was already basically good IMO.

[TheBlueMatt]

Basically LGTM

[valentinewallace]

Some of the new logic in ChannelManager is still sinking in for me, but overall this is looking good! Finished a first complete pass

[jkczyz]

Thanks for all the good feedback! Will see what I can do about the missing test coverage. It's a matter how easy it is to induce some error conditions (e.g., monitor update failing, keys provider giving an incompatible script).

[valentinewallace]

Is it the intended behavior to only broadcast a channel update if the channel was < ChannelState::FundingSent? Because I think this line will only return true if that's the situation 🤔 .

I think coverage is missing here too. I get that we're in "crunch time" though, so I'm pretty fine on punting tests if it comes to it

[jkczyz]

I believe it may also occur if we already handled a shutdown from the counterparty but didn't yet send our own shutdown.

[jkczyz]

Ah, actually now that I think of it, you're right. Since we lock channel_state, the situation that I mentioned can't occur.

[valentinewallace]

Yeah, I guess I just wanna get clear that this code block (and similar) are necessary, and what situation they would run in? Double checked and couldn't find any test coverage by adding prints

[TheBlueMatt]

I believe this code block is actually now unreachable. We return an Err in case we're ready to shut down here instead of getting into this block.

[jkczyz]

Removed code block from internal_shutdown rather than here as discussed on Slack.

[valentinewallace]

I can't really find anything to comment on, this is shaping up from my PoV!

[valentinewallace]

Prob not worth checking to see if they set a script here (which would make them a buggy peer), right?

[jkczyz]

They are allowed to support the feature but not necessarily set the shutdown script upfront, if I understand BOLT 2 correctly.

[valentinewallace]

They are allowed to support the feature but not necessarily set the shutdown script upfront,

IIUC, this is allowed unless both peers advertise support for the feature, in which case setting a script is required: from BOLT 2

I was concerned about the case where a peer didn't advertise the feature but still set a shutdown script up front, but checked the spec and that seems to be allowed 🤷

[TheBlueMatt]

It looks like this was rebased without any conflict, was there a reason for the rebase I missed?

[jkczyz]

Added the requested tests. For testing IncompatibleShutdownScript errors, I needed to add InitFeatures to NodeCfg and pass it when connecting peers (see separate commit 2e03385) since ChannelManager::close_channel pulls the features from per_peer_state. We could probably use this in some of the test utilities instead of passing &InitFeatures::known() everywhere, but that's a more involved follow-up.

It looks like this was rebased without any conflict, was there a reason for the rebase I missed?

I was probably proactively rebasing just in case. 🙂

[jkczyz]

They are allowed to support the feature but not necessarily set the shutdown script upfront, if I understand BOLT 2 correctly.

[TheBlueMatt]

I was probably proactively rebasing just in case. slightly_smiling_face

Oops, in general is it possible to avoid this? It makes it hard to diff-tree between revisions to see what changed.

[valentinewallace]

ACK mod one last clarification. Thanks for the additional test coverage added!

[valentinewallace]

They are allowed to support the feature but not necessarily set the shutdown script upfront,

IIUC, this is allowed unless both peers advertise support for the feature, in which case setting a script is required: from BOLT 2

I was concerned about the case where a peer didn't advertise the feature but still set a shutdown script up front, but checked the spec and that seems to be allowed 🤷

[valentinewallace]

Yeah, I guess I just wanna get clear that this code block (and similar) are necessary, and what situation they would run in? Double checked and couldn't find any test coverage by adding prints

[TheBlueMatt]

ACK post-squash.

[jkczyz]

Rather than fetching a channel's shutdown script from KeysInterface at creation time, do so based on ChannelConfig::commit_upfront_shutdown_pubkey (i.e., either creation or shutdown time).

Additionally, support any acceptable shutdown script pubkey as defined by BOLT 2 while maintaining backwards compatibility with the legacy KeysInterface::get_shutdown_pubkey, which is replaced by KeysInterface::get_shutdown_scriptpubkey.

This change requires storing an optional script as a TLV field for both Channel and ChannelMonitor. Additionally, since the field is now optional, a new ChannelMonitorUpdateStep is needed for when the script is generated at shutdown time.

Fixes#994.

[codecov]

Codecov Report

Merging #1019 (b24b4e9) into main (69ee486) will increase coverage by 0.12%.
The diff coverage is 92.15%.

❗ Current head b24b4e9 differs from pull request most recent head 1d3861e. Consider uploading reports for the commit 1d3861e to get more accurate results

@@ Coverage Diff @@## main #1019 +/- ##
==========================================
+ Coverage 90.79% 90.92% +0.12% 
==========================================
Files 61 62 +1 Lines 31578 32253 +675 ==========================================
+ Hits 28672 29326 +654 - Misses 2906 2927 +21 

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	90.00% <ø> (ø)
lightning/src/util/errors.rs	67.30% <0.00%> (-4.13%)	⬇️
lightning/src/util/test_utils.rs	82.29% <76.00%> (-0.26%)	⬇️
lightning/src/chain/channelmonitor.rs	90.43% <83.33%> (-0.15%)	⬇️
lightning/src/ln/channelmanager.rs	86.82% <85.45%> (+1.12%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.11% <91.66%> (-0.07%)	⬇️
lightning/src/ln/script.rs	93.75% <93.75%> (ø)
lightning/src/ln/functional_tests.rs	97.23% <93.87%> (-0.05%)	⬇️
lightning/src/ln/channel.rs	89.34% <95.20%> (+0.11%)	⬆️
lightning-background-processor/src/lib.rs	95.48% <100.00%> (-0.10%)	⬇️
... and 7 more

---

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 69ee486...1d3861e. Read the comment docs.

[TheBlueMatt]

Oops, this needs a similar test fix in the background-processor as well - https://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=commit;h=9218054becc3dcf690bdd8fcf819952ee783365a

[TheBlueMatt]

I think we should return an Result here - version needs to be <= 16 so users can totally make it fail, we shouldn't panic in that case, no? Also, why do the conversion to u5 and then call new_witness_program? That seems like a very roundabout way to get there, just push the version followed by the program as done at https://docs.rs/bitcoin/0.27.0/src/bitcoin/blockdata/script.rs.html#280-290 (but do the push using push_int which does the right thing here, don't have to resort to manual opcode calculation https://docs.rs/bitcoin/0.27.0/src/bitcoin/blockdata/script.rs.html#706-720 )

[TheBlueMatt]

Also this will change soon upstream, not sure how that would impact this. rust-bitcoin/rust-bitcoin#617

[jkczyz]

Yeah, must cleaner like that. Will need to swap NonZeroU8 for WitnessVersion when it is available upstream.

[TheBlueMatt]

Should be handled like any other error - call handle_monitor_err to convert the error into a MsgHandleErrInternal, then store it in a variable outside the lock (like failed_htlcs and chan_option) and handle_error the error outside of the lock.

[jkczyz]

Hmmm... in the case of an error should we still send the shutdown message above? Similarly, should an error short-circuit any remaining code (e.g., failing back HTLCs, broadcasting channel update)?

[TheBlueMatt]

in the case of an error should we still send the shutdown message above?

There are two errors - permanent ones and temporary ones. In the case of permenent, yes, we should force-close instead and send an error message, for temporary, we should still send the shutdown.

failing back HTLCs,

We must never, ever forget to fail back htlcs we were told to fail back.

broadcasting channel update

This is only if we force-closed the channel due to an error, I think, so we shouldn't get to this point today if there's a monitor update, but a monitor update could cause us to get there now (if its a permanent one).

[jkczyz]

Ok, this turned out to be a bit more tricky than I imagined. Confirming my understanding below.

Change is in 49614d8. Let me know if that is what you had in mind and if there's a simpler way of going about this. Need to do the same for internal_shutdown.

There are two errors - permanent ones and temporary ones. In the case of permenent, yes, we should force-close instead and send an error message,

These are handled by handle_monitor_err and handle_error, respectively, IIUC.

for temporary, we should still send the shutdown.

What I don't quite understand is why we would send shutdown if we aren't certain that our ChannelMonitor has been updated with the shutdown script yet. Is it because if TemporaryError ever becomes a PermanentError, we wouldn't care about the shutdown script since we are force closing in that case?

[jkczyz]

Rebased and added for internal_shutdown, too: a024930

[TheBlueMatt]

Change is in 49614d8. Let me know if that is what you had in mind and if there's a simpler way of going about this. Need to do the same for internal_shutdown.

Nope, that's about right. These things are tricky to handle. I left a few specific comments on that commit.

These are handled by handle_monitor_err and handle_error, respectively, IIUC.

That's the MsgHandleErrInternal vs monitor-error distinction, not the permanent-vs-temporary distinction, both of which are handled by handle_monitor_err (and friends)

What I don't quite understand is why we would send shutdown if we aren't certain that our ChannelMonitor has been updated with the shutdown script yet. Is it because if TemporaryError ever becomes a PermanentError, we wouldn't care about the shutdown script since we are force closing in that case?

The Temporary->Permanent thing isn't a big deal, as you note. The real question is what happens if someone gives a Temporary update, and then takes longer to update their ChannelMonitor than we do doing the full closing_signed negotiation (because only afterclosing_signed negotiation could a transaction ever appear on chain with the script contained in the monitor update). Unlike most other monitor updates (at least RevokeAndACK), sending a Shutdown message isn't an immediate lock-in to a new states and the monitor doesn't need to know about it immediately, only before the closing transaction appears on chain.

If we wait for the update to complete here, then we have to add Shutdown resending, which I really don't want to do. An alternative, which is much easier, is to just wait for the monitor update to complete before ever sending a closing_signed, something I can shove in #985

[TheBlueMatt]

The ChannelMonitorUpdate changes look good to me, will re-review the rest a bit later but the rest was already basically good IMO.

[TheBlueMatt]

Basically LGTM

[valentinewallace]

Some of the new logic in ChannelManager is still sinking in for me, but overall this is looking good! Finished a first complete pass

[jkczyz]

Thanks for all the good feedback! Will see what I can do about the missing test coverage. It's a matter how easy it is to induce some error conditions (e.g., monitor update failing, keys provider giving an incompatible script).

[valentinewallace]

Is it the intended behavior to only broadcast a channel update if the channel was < ChannelState::FundingSent? Because I think this line will only return true if that's the situation 🤔 .

I think coverage is missing here too. I get that we're in "crunch time" though, so I'm pretty fine on punting tests if it comes to it

[jkczyz]

I believe it may also occur if we already handled a shutdown from the counterparty but didn't yet send our own shutdown.

[jkczyz]

Ah, actually now that I think of it, you're right. Since we lock channel_state, the situation that I mentioned can't occur.

[valentinewallace]

Yeah, I guess I just wanna get clear that this code block (and similar) are necessary, and what situation they would run in? Double checked and couldn't find any test coverage by adding prints

[TheBlueMatt]

I believe this code block is actually now unreachable. We return an Err in case we're ready to shut down here instead of getting into this block.

[jkczyz]

Removed code block from internal_shutdown rather than here as discussed on Slack.

[valentinewallace]

I can't really find anything to comment on, this is shaping up from my PoV!

[valentinewallace]

Prob not worth checking to see if they set a script here (which would make them a buggy peer), right?

[jkczyz]

They are allowed to support the feature but not necessarily set the shutdown script upfront, if I understand BOLT 2 correctly.

[valentinewallace]

They are allowed to support the feature but not necessarily set the shutdown script upfront,

IIUC, this is allowed unless both peers advertise support for the feature, in which case setting a script is required: from BOLT 2

I was concerned about the case where a peer didn't advertise the feature but still set a shutdown script up front, but checked the spec and that seems to be allowed 🤷

[TheBlueMatt]

It looks like this was rebased without any conflict, was there a reason for the rebase I missed?

[jkczyz]

Added the requested tests. For testing IncompatibleShutdownScript errors, I needed to add InitFeatures to NodeCfg and pass it when connecting peers (see separate commit 2e03385) since ChannelManager::close_channel pulls the features from per_peer_state. We could probably use this in some of the test utilities instead of passing &InitFeatures::known() everywhere, but that's a more involved follow-up.

It looks like this was rebased without any conflict, was there a reason for the rebase I missed?

I was probably proactively rebasing just in case. 🙂

[jkczyz]

They are allowed to support the feature but not necessarily set the shutdown script upfront, if I understand BOLT 2 correctly.

[TheBlueMatt]

I was probably proactively rebasing just in case. slightly_smiling_face

Oops, in general is it possible to avoid this? It makes it hard to diff-tree between revisions to see what changed.

[valentinewallace]

ACK mod one last clarification. Thanks for the additional test coverage added!

[valentinewallace]

They are allowed to support the feature but not necessarily set the shutdown script upfront,

IIUC, this is allowed unless both peers advertise support for the feature, in which case setting a script is required: from BOLT 2

I was concerned about the case where a peer didn't advertise the feature but still set a shutdown script up front, but checked the spec and that seems to be allowed 🤷

[valentinewallace]

Yeah, I guess I just wanna get clear that this code block (and similar) are necessary, and what situation they would run in? Double checked and couldn't find any test coverage by adding prints

[TheBlueMatt]

ACK post-squash.

[jkczyz]

Rather than fetching a channel's shutdown script from KeysInterface at creation time, do so based on ChannelConfig::commit_upfront_shutdown_pubkey (i.e., either creation or shutdown time).

Additionally, support any acceptable shutdown script pubkey as defined by BOLT 2 while maintaining backwards compatibility with the legacy KeysInterface::get_shutdown_pubkey, which is replaced by KeysInterface::get_shutdown_scriptpubkey.

This change requires storing an optional script as a TLV field for both Channel and ChannelMonitor. Additionally, since the field is now optional, a new ChannelMonitorUpdateStep is needed for when the script is generated at shutdown time.

Fixes#994.

[codecov]

Codecov Report

Merging #1019 (b24b4e9) into main (69ee486) will increase coverage by 0.12%.
The diff coverage is 92.15%.

❗ Current head b24b4e9 differs from pull request most recent head 1d3861e. Consider uploading reports for the commit 1d3861e to get more accurate results

@@ Coverage Diff @@## main #1019 +/- ##
==========================================
+ Coverage 90.79% 90.92% +0.12% 
==========================================
Files 61 62 +1 Lines 31578 32253 +675 ==========================================
+ Hits 28672 29326 +654 - Misses 2906 2927 +21 

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	90.00% <ø> (ø)
lightning/src/util/errors.rs	67.30% <0.00%> (-4.13%)	⬇️
lightning/src/util/test_utils.rs	82.29% <76.00%> (-0.26%)	⬇️
lightning/src/chain/channelmonitor.rs	90.43% <83.33%> (-0.15%)	⬇️
lightning/src/ln/channelmanager.rs	86.82% <85.45%> (+1.12%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.11% <91.66%> (-0.07%)	⬇️
lightning/src/ln/script.rs	93.75% <93.75%> (ø)
lightning/src/ln/functional_tests.rs	97.23% <93.87%> (-0.05%)	⬇️
lightning/src/ln/channel.rs	89.34% <95.20%> (+0.11%)	⬆️
lightning-background-processor/src/lib.rs	95.48% <100.00%> (-0.10%)	⬇️
... and 7 more

---

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 69ee486...1d3861e. Read the comment docs.

[TheBlueMatt]

Oops, this needs a similar test fix in the background-processor as well - https://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=commit;h=9218054becc3dcf690bdd8fcf819952ee783365a

[TheBlueMatt]

I think we should return an Result here - version needs to be <= 16 so users can totally make it fail, we shouldn't panic in that case, no? Also, why do the conversion to u5 and then call new_witness_program? That seems like a very roundabout way to get there, just push the version followed by the program as done at https://docs.rs/bitcoin/0.27.0/src/bitcoin/blockdata/script.rs.html#280-290 (but do the push using push_int which does the right thing here, don't have to resort to manual opcode calculation https://docs.rs/bitcoin/0.27.0/src/bitcoin/blockdata/script.rs.html#706-720 )

[TheBlueMatt]

Also this will change soon upstream, not sure how that would impact this. rust-bitcoin/rust-bitcoin#617

[jkczyz]

Yeah, must cleaner like that. Will need to swap NonZeroU8 for WitnessVersion when it is available upstream.

[TheBlueMatt]

Should be handled like any other error - call handle_monitor_err to convert the error into a MsgHandleErrInternal, then store it in a variable outside the lock (like failed_htlcs and chan_option) and handle_error the error outside of the lock.

[jkczyz]

Hmmm... in the case of an error should we still send the shutdown message above? Similarly, should an error short-circuit any remaining code (e.g., failing back HTLCs, broadcasting channel update)?

[TheBlueMatt]

in the case of an error should we still send the shutdown message above?

There are two errors - permanent ones and temporary ones. In the case of permenent, yes, we should force-close instead and send an error message, for temporary, we should still send the shutdown.

failing back HTLCs,

We must never, ever forget to fail back htlcs we were told to fail back.

broadcasting channel update

This is only if we force-closed the channel due to an error, I think, so we shouldn't get to this point today if there's a monitor update, but a monitor update could cause us to get there now (if its a permanent one).

[jkczyz]

Ok, this turned out to be a bit more tricky than I imagined. Confirming my understanding below.

Change is in 49614d8. Let me know if that is what you had in mind and if there's a simpler way of going about this. Need to do the same for internal_shutdown.

There are two errors - permanent ones and temporary ones. In the case of permenent, yes, we should force-close instead and send an error message,

These are handled by handle_monitor_err and handle_error, respectively, IIUC.

for temporary, we should still send the shutdown.

What I don't quite understand is why we would send shutdown if we aren't certain that our ChannelMonitor has been updated with the shutdown script yet. Is it because if TemporaryError ever becomes a PermanentError, we wouldn't care about the shutdown script since we are force closing in that case?

[jkczyz]

Rebased and added for internal_shutdown, too: a024930

[TheBlueMatt]

Change is in 49614d8. Let me know if that is what you had in mind and if there's a simpler way of going about this. Need to do the same for internal_shutdown.

Nope, that's about right. These things are tricky to handle. I left a few specific comments on that commit.

These are handled by handle_monitor_err and handle_error, respectively, IIUC.

That's the MsgHandleErrInternal vs monitor-error distinction, not the permanent-vs-temporary distinction, both of which are handled by handle_monitor_err (and friends)

What I don't quite understand is why we would send shutdown if we aren't certain that our ChannelMonitor has been updated with the shutdown script yet. Is it because if TemporaryError ever becomes a PermanentError, we wouldn't care about the shutdown script since we are force closing in that case?

The Temporary->Permanent thing isn't a big deal, as you note. The real question is what happens if someone gives a Temporary update, and then takes longer to update their ChannelMonitor than we do doing the full closing_signed negotiation (because only afterclosing_signed negotiation could a transaction ever appear on chain with the script contained in the monitor update). Unlike most other monitor updates (at least RevokeAndACK), sending a Shutdown message isn't an immediate lock-in to a new states and the monitor doesn't need to know about it immediately, only before the closing transaction appears on chain.

If we wait for the update to complete here, then we have to add Shutdown resending, which I really don't want to do. An alternative, which is much easier, is to just wait for the monitor update to complete before ever sending a closing_signed, something I can shove in #985

[TheBlueMatt]

The ChannelMonitorUpdate changes look good to me, will re-review the rest a bit later but the rest was already basically good IMO.

[TheBlueMatt]

Basically LGTM

[valentinewallace]

Some of the new logic in ChannelManager is still sinking in for me, but overall this is looking good! Finished a first complete pass

[jkczyz]

Thanks for all the good feedback! Will see what I can do about the missing test coverage. It's a matter how easy it is to induce some error conditions (e.g., monitor update failing, keys provider giving an incompatible script).

[valentinewallace]

Is it the intended behavior to only broadcast a channel update if the channel was < ChannelState::FundingSent? Because I think this line will only return true if that's the situation 🤔 .

I think coverage is missing here too. I get that we're in "crunch time" though, so I'm pretty fine on punting tests if it comes to it

[jkczyz]

I believe it may also occur if we already handled a shutdown from the counterparty but didn't yet send our own shutdown.

[jkczyz]

Ah, actually now that I think of it, you're right. Since we lock channel_state, the situation that I mentioned can't occur.

[valentinewallace]

Yeah, I guess I just wanna get clear that this code block (and similar) are necessary, and what situation they would run in? Double checked and couldn't find any test coverage by adding prints

[TheBlueMatt]

I believe this code block is actually now unreachable. We return an Err in case we're ready to shut down here instead of getting into this block.

[jkczyz]

Removed code block from internal_shutdown rather than here as discussed on Slack.

[valentinewallace]

I can't really find anything to comment on, this is shaping up from my PoV!

[valentinewallace]

Prob not worth checking to see if they set a script here (which would make them a buggy peer), right?

[jkczyz]

They are allowed to support the feature but not necessarily set the shutdown script upfront, if I understand BOLT 2 correctly.

[valentinewallace]

They are allowed to support the feature but not necessarily set the shutdown script upfront,

IIUC, this is allowed unless both peers advertise support for the feature, in which case setting a script is required: from BOLT 2

I was concerned about the case where a peer didn't advertise the feature but still set a shutdown script up front, but checked the spec and that seems to be allowed 🤷

[TheBlueMatt]

It looks like this was rebased without any conflict, was there a reason for the rebase I missed?

[jkczyz]

Added the requested tests. For testing IncompatibleShutdownScript errors, I needed to add InitFeatures to NodeCfg and pass it when connecting peers (see separate commit 2e03385) since ChannelManager::close_channel pulls the features from per_peer_state. We could probably use this in some of the test utilities instead of passing &InitFeatures::known() everywhere, but that's a more involved follow-up.

It looks like this was rebased without any conflict, was there a reason for the rebase I missed?

I was probably proactively rebasing just in case. 🙂

[jkczyz]

They are allowed to support the feature but not necessarily set the shutdown script upfront, if I understand BOLT 2 correctly.

[TheBlueMatt]

I was probably proactively rebasing just in case. slightly_smiling_face

Oops, in general is it possible to avoid this? It makes it hard to diff-tree between revisions to see what changed.

[valentinewallace]

ACK mod one last clarification. Thanks for the additional test coverage added!

[valentinewallace]

They are allowed to support the feature but not necessarily set the shutdown script upfront,

IIUC, this is allowed unless both peers advertise support for the feature, in which case setting a script is required: from BOLT 2

I was concerned about the case where a peer didn't advertise the feature but still set a shutdown script up front, but checked the spec and that seems to be allowed 🤷

[valentinewallace]

Yeah, I guess I just wanna get clear that this code block (and similar) are necessary, and what situation they would run in? Double checked and couldn't find any test coverage by adding prints

[TheBlueMatt]

ACK post-squash.

[jkczyz]

Rather than fetching a channel's shutdown script from KeysInterface at creation time, do so based on ChannelConfig::commit_upfront_shutdown_pubkey (i.e., either creation or shutdown time).

Additionally, support any acceptable shutdown script pubkey as defined by BOLT 2 while maintaining backwards compatibility with the legacy KeysInterface::get_shutdown_pubkey, which is replaced by KeysInterface::get_shutdown_scriptpubkey.

This change requires storing an optional script as a TLV field for both Channel and ChannelMonitor. Additionally, since the field is now optional, a new ChannelMonitorUpdateStep is needed for when the script is generated at shutdown time.

Fixes#994.

[codecov]

Codecov Report

Merging #1019 (b24b4e9) into main (69ee486) will increase coverage by 0.12%.
The diff coverage is 92.15%.

❗ Current head b24b4e9 differs from pull request most recent head 1d3861e. Consider uploading reports for the commit 1d3861e to get more accurate results

@@ Coverage Diff @@## main #1019 +/- ##
==========================================
+ Coverage 90.79% 90.92% +0.12% 
==========================================
Files 61 62 +1 Lines 31578 32253 +675 ==========================================
+ Hits 28672 29326 +654 - Misses 2906 2927 +21 

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	90.00% <ø> (ø)
lightning/src/util/errors.rs	67.30% <0.00%> (-4.13%)	⬇️
lightning/src/util/test_utils.rs	82.29% <76.00%> (-0.26%)	⬇️
lightning/src/chain/channelmonitor.rs	90.43% <83.33%> (-0.15%)	⬇️
lightning/src/ln/channelmanager.rs	86.82% <85.45%> (+1.12%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.11% <91.66%> (-0.07%)	⬇️
lightning/src/ln/script.rs	93.75% <93.75%> (ø)
lightning/src/ln/functional_tests.rs	97.23% <93.87%> (-0.05%)	⬇️
lightning/src/ln/channel.rs	89.34% <95.20%> (+0.11%)	⬆️
lightning-background-processor/src/lib.rs	95.48% <100.00%> (-0.10%)	⬇️
... and 7 more

---

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 69ee486...1d3861e. Read the comment docs.

[TheBlueMatt]

Oops, this needs a similar test fix in the background-processor as well - https://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=commit;h=9218054becc3dcf690bdd8fcf819952ee783365a

[TheBlueMatt]

I think we should return an Result here - version needs to be <= 16 so users can totally make it fail, we shouldn't panic in that case, no? Also, why do the conversion to u5 and then call new_witness_program? That seems like a very roundabout way to get there, just push the version followed by the program as done at https://docs.rs/bitcoin/0.27.0/src/bitcoin/blockdata/script.rs.html#280-290 (but do the push using push_int which does the right thing here, don't have to resort to manual opcode calculation https://docs.rs/bitcoin/0.27.0/src/bitcoin/blockdata/script.rs.html#706-720 )

[TheBlueMatt]

Also this will change soon upstream, not sure how that would impact this. rust-bitcoin/rust-bitcoin#617

[jkczyz]

Yeah, must cleaner like that. Will need to swap NonZeroU8 for WitnessVersion when it is available upstream.

[TheBlueMatt]

Should be handled like any other error - call handle_monitor_err to convert the error into a MsgHandleErrInternal, then store it in a variable outside the lock (like failed_htlcs and chan_option) and handle_error the error outside of the lock.

[jkczyz]

Hmmm... in the case of an error should we still send the shutdown message above? Similarly, should an error short-circuit any remaining code (e.g., failing back HTLCs, broadcasting channel update)?

[TheBlueMatt]

in the case of an error should we still send the shutdown message above?

There are two errors - permanent ones and temporary ones. In the case of permenent, yes, we should force-close instead and send an error message, for temporary, we should still send the shutdown.

failing back HTLCs,

We must never, ever forget to fail back htlcs we were told to fail back.

broadcasting channel update

This is only if we force-closed the channel due to an error, I think, so we shouldn't get to this point today if there's a monitor update, but a monitor update could cause us to get there now (if its a permanent one).

[jkczyz]

Ok, this turned out to be a bit more tricky than I imagined. Confirming my understanding below.

Change is in 49614d8. Let me know if that is what you had in mind and if there's a simpler way of going about this. Need to do the same for internal_shutdown.

There are two errors - permanent ones and temporary ones. In the case of permenent, yes, we should force-close instead and send an error message,

These are handled by handle_monitor_err and handle_error, respectively, IIUC.

for temporary, we should still send the shutdown.

What I don't quite understand is why we would send shutdown if we aren't certain that our ChannelMonitor has been updated with the shutdown script yet. Is it because if TemporaryError ever becomes a PermanentError, we wouldn't care about the shutdown script since we are force closing in that case?

[jkczyz]

Rebased and added for internal_shutdown, too: a024930

[TheBlueMatt]

Change is in 49614d8. Let me know if that is what you had in mind and if there's a simpler way of going about this. Need to do the same for internal_shutdown.

Nope, that's about right. These things are tricky to handle. I left a few specific comments on that commit.

These are handled by handle_monitor_err and handle_error, respectively, IIUC.

That's the MsgHandleErrInternal vs monitor-error distinction, not the permanent-vs-temporary distinction, both of which are handled by handle_monitor_err (and friends)

What I don't quite understand is why we would send shutdown if we aren't certain that our ChannelMonitor has been updated with the shutdown script yet. Is it because if TemporaryError ever becomes a PermanentError, we wouldn't care about the shutdown script since we are force closing in that case?

The Temporary->Permanent thing isn't a big deal, as you note. The real question is what happens if someone gives a Temporary update, and then takes longer to update their ChannelMonitor than we do doing the full closing_signed negotiation (because only afterclosing_signed negotiation could a transaction ever appear on chain with the script contained in the monitor update). Unlike most other monitor updates (at least RevokeAndACK), sending a Shutdown message isn't an immediate lock-in to a new states and the monitor doesn't need to know about it immediately, only before the closing transaction appears on chain.

If we wait for the update to complete here, then we have to add Shutdown resending, which I really don't want to do. An alternative, which is much easier, is to just wait for the monitor update to complete before ever sending a closing_signed, something I can shove in #985

[TheBlueMatt]

The ChannelMonitorUpdate changes look good to me, will re-review the rest a bit later but the rest was already basically good IMO.

[TheBlueMatt]

Basically LGTM

[valentinewallace]

Some of the new logic in ChannelManager is still sinking in for me, but overall this is looking good! Finished a first complete pass

[jkczyz]

Thanks for all the good feedback! Will see what I can do about the missing test coverage. It's a matter how easy it is to induce some error conditions (e.g., monitor update failing, keys provider giving an incompatible script).

[valentinewallace]

Is it the intended behavior to only broadcast a channel update if the channel was < ChannelState::FundingSent? Because I think this line will only return true if that's the situation 🤔 .

I think coverage is missing here too. I get that we're in "crunch time" though, so I'm pretty fine on punting tests if it comes to it

[jkczyz]

I believe it may also occur if we already handled a shutdown from the counterparty but didn't yet send our own shutdown.

[jkczyz]

Ah, actually now that I think of it, you're right. Since we lock channel_state, the situation that I mentioned can't occur.

[valentinewallace]

Yeah, I guess I just wanna get clear that this code block (and similar) are necessary, and what situation they would run in? Double checked and couldn't find any test coverage by adding prints

[TheBlueMatt]

I believe this code block is actually now unreachable. We return an Err in case we're ready to shut down here instead of getting into this block.

[jkczyz]

Removed code block from internal_shutdown rather than here as discussed on Slack.

[valentinewallace]

I can't really find anything to comment on, this is shaping up from my PoV!

[valentinewallace]

Prob not worth checking to see if they set a script here (which would make them a buggy peer), right?

[jkczyz]

They are allowed to support the feature but not necessarily set the shutdown script upfront, if I understand BOLT 2 correctly.

[valentinewallace]

They are allowed to support the feature but not necessarily set the shutdown script upfront,

IIUC, this is allowed unless both peers advertise support for the feature, in which case setting a script is required: from BOLT 2

I was concerned about the case where a peer didn't advertise the feature but still set a shutdown script up front, but checked the spec and that seems to be allowed 🤷

[TheBlueMatt]

It looks like this was rebased without any conflict, was there a reason for the rebase I missed?

[jkczyz]

Added the requested tests. For testing IncompatibleShutdownScript errors, I needed to add InitFeatures to NodeCfg and pass it when connecting peers (see separate commit 2e03385) since ChannelManager::close_channel pulls the features from per_peer_state. We could probably use this in some of the test utilities instead of passing &InitFeatures::known() everywhere, but that's a more involved follow-up.

It looks like this was rebased without any conflict, was there a reason for the rebase I missed?

I was probably proactively rebasing just in case. 🙂

[jkczyz]

They are allowed to support the feature but not necessarily set the shutdown script upfront, if I understand BOLT 2 correctly.

[TheBlueMatt]

I was probably proactively rebasing just in case. slightly_smiling_face

Oops, in general is it possible to avoid this? It makes it hard to diff-tree between revisions to see what changed.

[valentinewallace]

ACK mod one last clarification. Thanks for the additional test coverage added!

[valentinewallace]

They are allowed to support the feature but not necessarily set the shutdown script upfront,

IIUC, this is allowed unless both peers advertise support for the feature, in which case setting a script is required: from BOLT 2

I was concerned about the case where a peer didn't advertise the feature but still set a shutdown script up front, but checked the spec and that seems to be allowed 🤷

[valentinewallace]

Yeah, I guess I just wanna get clear that this code block (and similar) are necessary, and what situation they would run in? Double checked and couldn't find any test coverage by adding prints

[TheBlueMatt]

ACK post-squash.