Always re-send ChannelUpdate to private channels

[akumaigorodski]

Current approach seems to be too brittle as ChannelUpdate is only sent once but may not reach a remote peer due to disconnect (this did happen to me).

This is what other implementations (LND at least) do already, I suppose that traffic overhead of this would be minimal. If it is a concern though then perhaps a following smarter strategy could work: send ChannelUpdate to private channel on each reconnect until there was an incoming payment attempt (respected commit number updated in commitments?).

[t-bast]

I agree with the overall goal: it's cheap to send channel_updates for unannounced channels more aggressively (as they won't be forwarded further in the network).

But your current proposal will send it way too often (every time the commitment tx is updated).
I would instead only add a re-broadcast when going OFFLINE -> NORMAL.

[akumaigorodski]

Looks like this was catching a superfluous ChannelUpdate on entering into OFFLINE state, it's not sent now.

[t-bast]

I think that's incorrect. You still always want to wait for d2.buried before sending a channel_update (which isn't the case here if d1.channelUpdate != d2.channelUpdate.

I'm a bit weary of matching now on states + data, there may be a case we're losing compared to the previous code.

Here is my take:

// if channel is private, we send the channel_update directly to remote
// they need it "to learn the other end's forwarding parameters" (BOLT 7)
(state, nextState, stateData, nextStateData) match {
  case (_, _, d1: DATA_NORMAL, d2: DATA_NORMAL) if !d1.commitments.announceChannel && !d1.buried && d2.buried =>
    // for a private channel, when the tx was just buried we need to send the channel_update to our peer (even if it didn't change)
    forwarder ! d2.channelUpdate
  case (OFFLINE, NORMAL, d1: DATA_NORMAL, d2: DATA_NORMAL) if !d1.commitments.announceChannel && d2.buried =>
    // otherwise if we're coming back online, we rebroadcast the latest channel_update
    // this makes sure that if the channel_update was missed, we have a chance to re-send it
    forwarder ! d2.channelUpdate
  case (_, _, d1: DATA_NORMAL, d2: DATA_NORMAL) if !d1.commitments.announceChannel && d1.channelUpdate != d2.channelUpdate && d2.buried =>
    // otherwise, we only send it when it is different, and tx is already buried
    forwarder ! d2.channelUpdate
  case _ => ()
}

I believe it's safer as it keeps the existing behavior and only adds a new case when coming back online. WDYT?

[akumaigorodski]

I think that's incorrect.

It's less code and not sure it's strictly incorrect: NORMAL_DATA is present so at least default 3 confirmations are there, occasional premature update at this point won't add too much risk.

But I'm also totally fine with using your code.

[t-bast]

LGTM, thanks!