Fix undeleted private channel update gossip_store entry

[endothermicdev]

trafficstars

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

[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 368dcc3a9a3d5be28227d5f191b6621b1cc30a6e

[cdecker]

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