Spurious Forwarding Failures in Async Monitor Update Clients

[TheBlueMatt]

We use Channel::is_live() for a few things that imply "should we consider this channel available for forwarding HTLCs and sending payments", which is great, except it implies races for clients which use async monitor updates. Such clients will always return a TemporaryFailure on monitor updates, leaving the channel in ChannelState::MonitorUpdateFailed until the monitor updates completes. This implies !is_live() which means such clients will refuse to send or forward HTLCs during monitor updates, which they likely should not. The likely fix would be to only !is_live() a channel if the monitor updating has been running for some time without completion, but this probably has implications for the channel state machine around placing such HTLCs in the holding cell in a new case.

[valentinewallace]

Taking a look at #756 and wondering about

but this probably has implications for the channel state machine around placing such HTLCs in the holding cell in a new case.

Could you clarify what implications? (and is "a new case" = the case where we now only return !is_live() if updating has run some time without completion?)

[TheBlueMatt]

Could you clarify what implications? (and is "a new case" = the case where we now only return !is_live() if updating has run some time without completion?)

Basically, any time we place something in the holding cell, we have to make sure we call free_holding_cell when the state transitions away from whatever the requirement was that resulted in it going into the holding cell. You can see this in #756 where, once we stop dropping the HTLCs going into the holding cell we now have to make sure we free_holding_cell when we get a channel_reestablish, as otherwise we have stuff sitting in the holding cell forever. It may be that after 756 all the relevant cases are handled, but I haven't done a careful scan.