lightningdevkit / rust-lightning

A highly modular Bitcoin Lightning library written in Rust. It's rust-lightning, not Rusty's Lightning!
Other
1.16k stars 366 forks source link

Break `ClosureReason::CommitmentTxConfirmed` Up Somewhat #1488

Closed TheBlueMatt closed 3 months ago

TheBlueMatt commented 2 years ago

If an HTLC times out and we close a channel from the ChannelMonitor, we currently mark the ClosureReason as CommitmentTxConfirmed, even though the commitment tx has not yet confirmed, not to mention it was us who closed the channel! From a cursory glance, the fix is trivial - just interpret (and maybe rename) MonitorEvent::CommitmentTxConfirmed to mean "we force-closed" and then we can think about adding more detail, but I think its literally always due to HTLC timeouts.

colinh80 commented 2 years ago

Hi, is the idea to decide on a more fitting name and replace all uses of CommitmentTxConfirmed with the new name? As well as reinterpret the note on CommitmentTxConfirmed in the Monitor Event enum?

I'm also curious about MonitorEvent::FundingTimedOut. I don't see it used anywhere but it seems more appropriate for HTLC time outs

TheBlueMatt commented 2 years ago

Hi, is the idea to decide on a more fitting name and replace all uses of CommitmentTxConfirmed with the new name?

No, the existing uses of it are fine, its only the MonitorEvent::CommitmentTxConfirmed case that needs changing.

I'm also curious about MonitorEvent::FundingTimedOut. I don't see it used anywhere but it seems more appropriate for HTLC time outs

No, this is about the funding not confirming in enough time, not us force-closing. Currently ProcessingError is what we use for "we force-closed".

Mirebella commented 3 months ago

Is this still open? Looks like https://github.com/lightningdevkit/rust-lightning/commit/a96e2fe144383ea6fd670153fb895ee07a3245ef commit and https://github.com/lightningdevkit/rust-lightning/pull/2887 in "assesment against linked issues" section might have already addressed it.

TheBlueMatt commented 3 months ago

Yep, I think this is fixed, thanks!