channeld: remove dead HTLCs from htable and free them (eventually)

The channel->htlcs map was exhibiting unbounded growth, as elements were
never removed from it. This was causing lightning_channeld processes to
consume ever-increasing amounts of memory, and iterating over the map
was causing ever-increasing CPU utilization. There were FIXME comments
suggesting that the intention was to remove HTLCs from the map upon
their deaths. This commit implements that intention.

Changelog-Fixed: channeld no longer retains dead HTLCs in memory.

channeld/channeld_htlc.h

@@ -71,7 +71,6 @@ static inline struct htlc *htlc_get(struct htlc_map *htlcs, u64 id, enum side ow
 	return NULL;
 }
 
-/* FIXME: Move these out of the hash! */
 static inline bool htlc_is_dead(const struct htlc *htlc)
 {
 	return htlc->state == RCVD_REMOVE_ACK_REVOCATION

channeld/full_channel.c

@@ -934,10 +934,15 @@ static int change_htlcs(struct channel *channel,
 		for (i = 0; i < n_hstates; i++) {
 			if (h->state == htlc_states[i]) {
 				htlc_incstate(channel, h, sidechanged, owed);
+				if (htlc_is_dead(h)) {
+					htlc_map_delval(channel->htlcs, &it);
+					tal_steal(htlcs ? *htlcs : tmpctx, h);
+				}
 				dump_htlc(h, prefix);
 				htlc_arr_append(htlcs, h);
 				cflags |= (htlc_state_flags(htlc_states[i])
 					   ^ htlc_state_flags(h->state));
+				break;
 			}
 		}
 	}
@@ -1144,20 +1149,24 @@ bool channel_sending_revoke_and_ack(struct channel *channel)
 	return (change & HTLC_REMOTE_F_PENDING);
 }
 
-size_t num_channel_htlcs(const struct channel *channel)
+static inline bool any_htlc_is_dead(const struct channel *channel)
 {
 	struct htlc_map_iter it;
 	const struct htlc *htlc;
-	size_t n = 0;
 
 	for (htlc = htlc_map_first(channel->htlcs, &it);
 	     htlc;
 	     htlc = htlc_map_next(channel->htlcs, &it)) {
-		/* FIXME: Clean these out! */
-		if (!htlc_is_dead(htlc))
-			n++;
+		if (htlc_is_dead(htlc))
+			return true;
 	}
-	return n;
+	return false;
+}
+
+size_t num_channel_htlcs(const struct channel *channel)
+{
+	assert(!any_htlc_is_dead(channel));
+	return htlc_map_count(channel->htlcs);
 }
 
 static bool adjust_balance(struct balance view_owed[NUM_SIDES][NUM_SIDES],

channeld/test/run-full_channel.c

@@ -266,14 +266,16 @@ static void send_and_fulfill_htlc(struct channel *channel,
 	struct sha256 rhash;
 	u8 *dummy_routing = tal_arr(channel, u8, TOTAL_PACKET_SIZE);
 	bool ret;
-	const struct htlc **changed_htlcs;
+	const struct htlc *htlc, **changed_htlcs;
 
 	memset(&r, 0, sizeof(r));
 	sha256(&rhash, &r, sizeof(r));
 
 	assert(channel_add_htlc(channel, sender, 1337, msatoshi, 900, &rhash,
 				dummy_routing, NULL, NULL, NULL)
 	       == CHANNEL_ERR_ADD_OK);
+	htlc = channel_get_htlc(channel, sender, 1337);
+	assert(htlc);
 
 	changed_htlcs = tal_arr(channel, const struct htlc *, 0);
 
@@ -297,8 +299,7 @@ static void send_and_fulfill_htlc(struct channel *channel,
 		assert(ret);
 		ret = channel_rcvd_revoke_and_ack(channel, &changed_htlcs);
 		assert(!ret);
-		assert(channel_get_htlc(channel, sender, 1337)->state
-		       == RCVD_REMOVE_ACK_REVOCATION);
+		assert(htlc->state == RCVD_REMOVE_ACK_REVOCATION);
 	} else {
 		ret = channel_rcvd_commit(channel, &changed_htlcs);
 		assert(ret);
@@ -318,9 +319,9 @@ static void send_and_fulfill_htlc(struct channel *channel,
 		assert(ret);
 		ret = channel_sending_revoke_and_ack(channel);
 		assert(!ret);
-		assert(channel_get_htlc(channel, sender, 1337)->state
-		       == SENT_REMOVE_ACK_REVOCATION);
+		assert(htlc->state == SENT_REMOVE_ACK_REVOCATION);
 	}
+	assert(!channel_get_htlc(channel, sender, 1337));
 }
 
 static void update_feerate(struct channel *channel, u32 feerate)