Fix undeleted private channel update gossip_store entry

[endothermicdev]

gossipd: ensure old private channel updates are properly deleted

When private channel updates exceed the gossip ratelimit, the previous gossip store entry was not deleted even though all private channel updates are stored. This caused gossip store corruption due to duplicate entries for the same channel.
Fixes: #5656

Changelog-Fixed: Fixed gossip_store corruption from duplicate private channel update

[cdecker]

ACK 08f6b1c

[rustyrussell]

This fix seems correct, but I'm reminded of Kernighan and Plauger's aphorism "Don't patch bad code, rewrite it".

Could we see a third commit that cleans all this up? In particular, private updates are never spam. And deletion of old spam records if they exist should be a clearly named function.

Something like:

/* Remove any prior entries */
delete_spam_update(...);
if (!spam)
    delete_update(...);
else
    /* Private channel updates never considered spam */
    assert(chan_is_public);

[endothermicdev]

In particular, private updates are never spam.

It's funny how obvious this is in hindsight. Makes the code much more reasonable. Thanks!

[rustyrussell]

Slightly neater:

diff --git a/gossipd/routing.c b/gossipd/routing.c
index 6cf0d7b69..f85cff821 100644
--- a/gossipd/routing.c
+++ b/gossipd/routing.c
@@ -1409,9 +1409,11 @@ bool routing_add_channel_update(struct routing_state *rstate,
 			return true;
 		}
 
-		/* Make sure it's not spamming us. */
-		if (!ratelimit(rstate,
-			       &hc->tokens, hc->bcast.timestamp, timestamp)) {
+		/* Make sure it's not spamming us (private channel
+		 * updates are never considered spam) */
+		if (is_chan_public(chan)
+		    && !ratelimit(rstate,
+				  &hc->tokens, hc->bcast.timestamp, timestamp)) {
 			status_peer_debug(peer ? &peer->id : NULL,
 					  "Spammy update for %s/%u flagged"
 					  " (last %u, now %u)",
@@ -1420,8 +1422,7 @@ bool routing_add_channel_update(struct routing_state *rstate,
 							 &short_channel_id),
 					  direction,
 					  hc->bcast.timestamp, timestamp);
-			/* Private channel updates are never considered spam */
-			spam = is_chan_public(chan);
+			spam = true;
 		} else {
 			spam = false;
 		}
@@ -1431,21 +1432,18 @@ bool routing_add_channel_update(struct routing_state *rstate,
 	if (force_spam_flag)
 		spam = true;
 
-	/* Delete any prior entries */
+	/* Delete any prior entries (noop if they don't exist) */
 	delete_spam_update(rstate, hc, is_chan_public(chan));
-	if (spam) {
-		assert(is_chan_public(chan));
-	} else {
-		/* Safe to broadcast */
-		hc->bcast.timestamp = timestamp;
-		/* Harmless if it was never added. */
+	if (!spam)
 		gossip_store_delete(rstate->gs, &hc->bcast,
 				    is_chan_public(chan)
 				    ? WIRE_CHANNEL_UPDATE
 				    : WIRE_GOSSIP_STORE_PRIVATE_UPDATE);
-	}
-	/* Routing graph always uses the latest message. */
+
+	/* Update timestamp(s) */
 	hc->rgraph.timestamp = timestamp;
+	if (!spam)
+		hc->bcast.timestamp = timestamp;
 
 	/* BOLT #7:
 	 *   - MUST consider the `timestamp` of the `channel_announcement` to be

[rustyrussell]

Merged in that patch, testing now.

Ack 368dcc3

[vincenzopalazzo]

I found this crash of lnprototest in the CI it is new to me! Looks like that, but not sure if this is related to this change!

ERROR:concurrent.futures:exception calling callback for <Future at 0x7fd189204d90 state=finished raised RpcError>
Traceback (most recent call last):
  File "/usr/lib/python3.10/concurrent/futures/_base.py", line 342, in _invoke_callbacks
    callback(self)
  File "/work/external/lnprototest/lnprototest/clightning/clightning.py", line 304, in _done
    raise exception
  File "/usr/lib/python3.10/concurrent/futures/thread.py", line 58, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/work/external/lnprototest/lnprototest/clightning/clightning.py", line 297, in _fundchannel
    return runner.rpc.fundchannel(
  File "/work/contrib/pyln-client/pyln/client/lightning.py", line 771, in fundchannel
    return self.call("fundchannel", payload)
  File "/work/contrib/pyln-client/pyln/client/lightning.py", line 408, in call
    raise RpcError(method, payload, resp['error'])
pyln.client.lightning.RpcError: RPC call failed: method: fundchannel, payload: {'id': '02c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5', 'amount': 999877, 'feerate': '253perkw', 'announce': True}, error: {'code': 400, 'message': 'Unable to connect, no address known for peer', 'data': {'id': '02c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5', 'method': 'connect'}}

[cdecker]

Rebased on top of master, @bitcoin-bot seems to have auto-ACKed it (finally!)