Fix spurious panic on receipt of a block while awaiting funding #1711
Conversation
When we receive a block we always test if we should send our channel_ready via `check_get_channel_ready`. If the channel in question requires confirmations, we quickly return if the funding transaction has not yet confirmed (or even been defined), however for 0conf channels the checks are necessarily more involved. In any case, we wish to panic if the funding transaction has confirmations prior to when it should have been broadcasted. This is useful as it is easy for users to violate our broadcast-time invariants without noticing and the panic gives us an opportunity to catch it. Sadly, in the case of 0conf channels, if we hadn't yet seen the funding transaction at all but receive a block we would hit this sanity check as we don't check whether there are actually funding transaction confirmations prior to panicing.
There was a problem hiding this comment.
Oops 🙈 I'm trying to think of how we could've caught this in review. Just more careful looking over of the code paths and brainstorming tests I suppose. But I do wonder if the ChannelState flags could somehow be made less confusing.
Test fails as expected on main
|
Reviewing. |
| @@ -4780,7 +4783,7 @@ impl<Signer: Sign> Channel<Signer> { | |||
| // We got a reorg but not enough to trigger a force close, just ignore. | |||
| false | |||
| } else { | |||
| if self.channel_state < ChannelState::ChannelFunded as u32 { | |||
| if self.funding_tx_confirmation_height != 0 && self.channel_state < ChannelState::ChannelFunded as u32 { | |||
There was a problem hiding this comment.
Minor: comment could be stronger here it's preventing to hit the panic when we receive a transactions_confirmed() in the 0conf case as raised in pre-review.
There was a problem hiding this comment.
You mean the top comment? I'm less worried about a new panic here, and more worried about someone breaking the logic. Its all a bit hairy, so good to be aware of the cases we have to handle correctly, panic or not.
|
OOC, was this found via fuzzing or? |
Prod, or at least testnet-prod-testing, 😭 . |
|
Gonna go ahead and land, can revisit the comment later if it matters. |
When we receive a block we always test if we should send our channel_ready via
check_get_channel_ready. If the channel in question requires confirmations, we quickly return if the funding transaction has not yet confirmed (or even been defined), however for 0conf channels the checks are necessarily more involved.In any case, we wish to panic if the funding transaction has confirmations prior to when it should have been broadcasted. This is useful as it is easy for users to violate our broadcast-time invariants without noticing and the panic gives us an opportunity to catch it.
Sadly, in the case of 0conf channels, if we hadn't yet seen the funding transaction at all but receive a block we would hit this sanity check as we don't check whether there are actually funding transaction confirmations prior to panicing.