Rebroadcast announcement_sigs at peer connection

[ariard]

BOLT 7 : A node upon reconnection if it has NOT received an
announcement_sigs message, SHOULD retransmit the announcement_sigs
message

The requirement "MUST respond to the first announcement_sigs with
its own announcement_sigs message" is covered by this change too.
Instead to wait for other party behaving we send anyway a
announcement_sigs message at connection as there is no order
requirement on announcement_sigs exchange.

Fix tests broken by change.

About SendAnnouncementSignatures in test_restore_channel_monitor, not sure if necessary (I mean don't see requirement in spec)
And isn't a duplicata sending in both funding_locked/block_connected ? (Just go ahead and keep the one in funding_locked without waiting for party first-move)

[TheBlueMatt]

Right, feels a bit awkward to always send it, though I guess by a close reading of the spec its...not exactly in violation. Still, may be nice to do something so that we dont send an announcement_signatures when we reconnect two months later.........is there any checks we can add to do this without addition additional state tracking (and does this result in us sending duplicative channel_announcements to our peers eventually?).

[ariard]

IMO would be better to let freshness/accuracy filtering of channel_update, channel_announcement, announcement_sigs... done by the router (or PeerHandler if it's connection-related) rather than to track connection state in ChannelManager.

[TheBlueMatt]

Sure, but we also want to be robust against a user-provided Router. If we've gotten through an iteration of channel updates, are we guaranteed to have exchanged announcement_sigs?

[ariard]

(and does this result in us sending duplicative channel_announcements to our peers eventually?).

Rn we send duplicate when at block connection of channel outpoint, when receiving a funding_locked, when restoring channel monitor and with this change at reconnection.

Will add a commit to drop the one channel_monitor restoration and funding_locked (as if not both rust-lightning will wait for first move)

[ariard]

If we've gotten through an iteration of channel updates, are we guaranteed to have exchanged announcement_sigs?

Case where other peer wouldn't get the announcement_sigs would be them being disconnected so with this change I assume that's covered?

Sure, but we also want to be robust against a user-provided Router.

Can't it be a requirement and documented somewhere? announcement_sigs is laying at the boundaries having the channel key to assert ownership, verifying state onchain and using it as foundation for gossip. The freshness is more a Router-job, if not we have to add a per-channel timer to bound announcement, it feels a bit gross..

[ariard]

Nah in fact broadcast are needed because our requirements are stronger than BOLT 7, "channel must be funded", (but we may send announcement twice in case we have seen block before we received their funding_locked)

[TheBlueMatt]

Rn we send duplicate when at block connection of channel outpoint, when receiving a funding_locked, when restoring channel monitor

Are you sure about this? The only places I see us calling get_announcement_sigs right now are when we either send or receive a funding_locked. This will only result in us sending it once normally since Channel::get_channel_announcement will only return something if both funding_locked as been sent and received. And currently one extra time if we receive a funding_locked after reconnect.

Are we sure this isnt actually currently sufficient?

[ariard]

I think it's fine as you described it's either we have received funding_locked and not sent our so no announcement_sigs or we have seen block and sent our funding_locked, we received funding_locked and so yes announcement_sigs.

I was confused at some point by BOLT 7 "if it has not sent funding_locked MAY defer handling the announcement_sigs after it has sent funding_locked", interpreted negatively it allow for the receiver to send announcement_sigs without seeing funds being locked on its side..

[TheBlueMatt]

Hmm, so can we just drop thee TODO and add more testing? :)

[ariard]

I think change is tested by updating the tests to make them work, the case of invalid announcement_sig is already covered by test_invalid_channel_announcement.

Or do you have in mind the duplicata/rate-limiting cases ?

[ariard]

See #397