Peer storage sent too early

[thomash-acinq]

Issue and Steps to Reproduce

When opening a new channel, CLN immediately sends a peer_storage before the channel is fully open. Eclair will ignore such peer_storage as we only accept storage from peers that have an active channel with us.
When reconnecting with a peer, CLN sends a peer_storage immediately, even before sending peer_storage_retrieval. This will override any previous storage backup and cancel any benefit from the feature (or in Eclair's case it will simply be ignored because there is no active channel yet).

getinfo output

"version": "v24.08.2"

[adi2011]

Looking into this...

[madelinevibes]

hi @adi2011 ! are you still looking into this? I'm going to assign a bounty to help incentivize if you've got something in progress so far. https://discordapp.com/channels/899980449231814676/1485847967104762077

[vincenzopalazzo]

I took a deep look at this and I think there are two distinct issues happening inside plugins/chanbackup.c.

1. peer_storage sent before channel is active

The peer_connected hook (line 717) immediately sends peer_storage via sendcustommsg before any channel reaches CHANNELD_NORMAL. The comment at line 715 says "we want to send data to peer before any reconnect messages (which might make it hang up!)" — but per BOLT #1, peer_storage is connection-scoped ("MAY send whenever necessary"), and there's no reason it needs to race ahead of channel_reestablish.

The spec says the receiver "if it has an open channel with the sender: MUST store the message" — Eclair interprets "open channel" as active/established, which is reasonable. CLN is not violating the spec, but the early send breaks interop.

2. peer_storage_retrieval sent before receiving updated peer_storage

On reconnect the flow is:

peer_connected() → sends our peer_storage (line 728)
  → peer_after_send_scb() callback (line 482)
    → send_peers_scb() → sends peer_storage_retrieval (line 465)

So we send back their old backup before they have a chance to send us an updated peer_storage. If their state changed while disconnected, we're pushing stale data back to them. The spec says "MUST send peer_storage_retrieval again after reconnection, after exchanging init messages" — but it doesn't say we should do it before they can update their storage.

Also, I noticed that when receiving peer_storage_retrieval from multiple peers (we send to NUM_BACKUP_PEERS=2), the handler at line 893 does create-or-replace on chanbackup/latestscb without comparing the timestamp field that is already inside the encrypted payload. So last-writer-wins regardless of which backup is newer. This is not great for a fresh node recovery scenario.

Fix approach:

• Move the peer_storage send from peer_connected to after channel_state_changed reports CHANNELD_NORMAL (the infrastructure at line 692 already detects this transition)
• Delay peer_storage_retrieval until after the peer has a chance to send us their updated peer_storage, or at least until channels are active
• Compare timestamps when receiving peer_storage_retrieval from multiple peers to keep the newest one

I can work on a PR for this if nobody else is already on it.

[madelinevibes]

sounds great, @vincenzopalazzo , it's all yours!
For your info, our next release is 26.06 with a code freeze on 30 April so if by chance it's ready by then, we can include in the next release, no worries if not! 😁

[adi2011]

@vincenzopalazzo Thanks for digging into it!
#8422 already implements timestamp comparison and is waiting for approval from @rustyrussell .