From 71c21ef0b4706263723ff751e8e4a0d099cb7510 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 25 Mar 2026 21:44:55 +1030 Subject: [PATCH 1/4] common: add tal_free_if_taken() helper for common case. Uses found and changed by Claude. Signed-off-by: Rusty Russell --- bitcoin/script.c | 3 +-- bitcoin/tx.c | 3 +-- common/cryptomsg.c | 4 ++-- common/features.c | 15 +++++---------- common/json_stream.c | 15 +++++---------- common/read_peer_msg.c | 3 +-- common/sphinx.c | 3 +-- common/utils.c | 3 +-- common/utils.h | 7 +++++++ connectd/multiplex.c | 3 +-- db/utils.c | 6 ++---- devtools/gossmap-compress.c | 3 +-- gossipd/gossipd.c | 3 +-- lightningd/channel.c | 3 +-- lightningd/channel_gossip.c | 6 ++---- lightningd/jsonrpc.c | 3 +-- lightningd/peer_htlcs.c | 6 ++---- lightningd/plugin.c | 6 ++---- lightningd/watch.c | 3 +-- onchaind/onchaind.c | 3 +-- onchaind/test/run-grind_feerate-bug.c | 3 +-- plugins/bkpr/test/run-recorder.c | 6 ++---- plugins/libplugin-pay.c | 3 +-- plugins/libplugin.c | 6 ++---- plugins/renepay/routefail.c | 3 +-- wallet/account_migration.c | 3 +-- wallet/invoices.c | 12 ++++-------- wallet/wallet.c | 6 ++---- wire/wire_sync.c | 4 ++-- 29 files changed, 55 insertions(+), 92 deletions(-) diff --git a/bitcoin/script.c b/bitcoin/script.c index 30cc0897a73c..3b38eae9d426 100644 --- a/bitcoin/script.c +++ b/bitcoin/script.c @@ -239,8 +239,7 @@ u8 *bitcoin_scriptsig_redeem(const tal_t *ctx, script_push_bytes(&script, redeemscript, tal_count(redeemscript)); - if (taken(redeemscript)) - tal_free(redeemscript); + tal_free_if_taken(redeemscript); return script; } diff --git a/bitcoin/tx.c b/bitcoin/tx.c index 0d5d6e560a89..fe25b10e6a0f 100644 --- a/bitcoin/tx.c +++ b/bitcoin/tx.c @@ -359,8 +359,7 @@ void bitcoin_tx_input_set_witness(struct bitcoin_tx *tx, int innum, wally_psbt_input_set_final_witness(&tx->psbt->inputs[innum], stack); tal_wally_end(tx->psbt); - if (taken(witness)) - tal_free(witness); + tal_free_if_taken(witness); } void bitcoin_tx_input_set_script(struct bitcoin_tx *tx, int innum, u8 *script) diff --git a/common/cryptomsg.c b/common/cryptomsg.c index 4225fefa8f0c..8fb8d34cb72a 100644 --- a/common/cryptomsg.c +++ b/common/cryptomsg.c @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -232,7 +233,6 @@ u8 *cryptomsg_encrypt_msg(const tal_t *ctx, maybe_rotate_key(&cs->sn, &cs->sk, &cs->s_ck); - if (taken(msg)) - tal_free(msg); + tal_free_if_taken(msg); return out; } diff --git a/common/features.c b/common/features.c index 7573fea58e0f..94e0167b5362 100644 --- a/common/features.c +++ b/common/features.c @@ -225,8 +225,7 @@ bool feature_set_or(struct feature_set *a, for (size_t j = 0; j < tal_bytelen(b->bits[i])*8; j++) { if (feature_is_set(b->bits[i], j) && feature_offered(a->bits[i], j)) { - if (taken(b)) - tal_free(b); + tal_free_if_taken(b); return false; } } @@ -239,8 +238,7 @@ bool feature_set_or(struct feature_set *a, } } - if (taken(b)) - tal_free(b); + tal_free_if_taken(b); return true; } @@ -252,8 +250,7 @@ bool feature_set_sub(struct feature_set *a, for (size_t j = 0; j < tal_bytelen(b->bits[i])*8; j++) { if (feature_is_set(b->bits[i], j) && !feature_offered(a->bits[i], j)) { - if (taken(b)) - tal_free(b); + tal_free_if_taken(b); return false; } } @@ -268,8 +265,7 @@ bool feature_set_sub(struct feature_set *a, } - if (taken(b)) - tal_free(b); + tal_free_if_taken(b); return true; } @@ -534,8 +530,7 @@ u8 *featurebits_or(const tal_t *ctx, const u8 *f1 TAKES, const u8 *f2 TAKES) result[l1 - l2 + i] |= f2[i]; /* Cleanup the featurebits if we were told to do so. */ - if (taken(f2)) - tal_free(f2); + tal_free_if_taken(f2); return result; } diff --git a/common/json_stream.c b/common/json_stream.c index b272760c2ac0..6a0746074584 100644 --- a/common/json_stream.c +++ b/common/json_stream.c @@ -190,8 +190,7 @@ void json_add_primitive(struct json_stream *js, const char *val TAKES) { json_add_primitive_fmt(js, fieldname, "%s", val); - if (taken(val)) - tal_free(val); + tal_free_if_taken(val); } void json_add_string(struct json_stream *js, @@ -200,8 +199,7 @@ void json_add_string(struct json_stream *js, { if (json_filter_ok(js->filter, fieldname)) json_out_addstr(js->jout, fieldname, str); - if (taken(str)) - tal_free(str); + tal_free_if_taken(str); } static char *json_member_direct(struct json_stream *js, @@ -304,8 +302,7 @@ void json_add_stringn(struct json_stream *result, const char *fieldname, const char *value TAKES, size_t value_len) { json_add_str_fmt(result, fieldname, "%.*s", (int)value_len, value); - if (taken(value)) - tal_free(value); + tal_free_if_taken(value); } void json_add_bool(struct json_stream *result, const char *fieldname, bool value) @@ -345,8 +342,7 @@ void json_add_escaped_string(struct json_stream *result, const char *fieldname, memcpy(dest + 1, esc->s, strlen(esc->s)); dest[1+strlen(esc->s)] = '"'; } - if (taken(esc)) - tal_free(esc); + tal_free_if_taken(esc); } void json_add_timeabs(struct json_stream *result, const char *fieldname, @@ -607,8 +603,7 @@ void json_add_psbt(struct json_stream *stream, const char *psbt_b64; psbt_b64 = fmt_wally_psbt(NULL, psbt); json_add_string(stream, fieldname, take(psbt_b64)); - if (taken(psbt)) - tal_free(psbt); + tal_free_if_taken(psbt); } void json_add_amount_msat(struct json_stream *result, diff --git a/common/read_peer_msg.c b/common/read_peer_msg.c index 292cc6ecc0bc..c83d8f173afe 100644 --- a/common/read_peer_msg.c +++ b/common/read_peer_msg.c @@ -18,8 +18,7 @@ bool handle_peer_error_or_warning(struct per_peer_state *pps, /* Simply log incoming warnings */ err = is_peer_warning(tmpctx, msg); if (err) { - if (taken(msg)) - tal_free(msg); + tal_free_if_taken(msg); status_info("Received %s", err); return true; } diff --git a/common/sphinx.c b/common/sphinx.c index 5cecf68777b5..66074296ffdf 100644 --- a/common/sphinx.c +++ b/common/sphinx.c @@ -165,8 +165,7 @@ void sphinx_add_hop(struct sphinx_path *path, const struct pubkey *pubkey, size_t len = tal_bytelen(payload); towire_bigsize(&with_len, len); towire_u8_array(&with_len, payload, len); - if (taken(payload)) - tal_free(payload); + tal_free_if_taken(payload); if (!sphinx_add_hop_has_length(path, pubkey, take(with_len))) abort(); diff --git a/common/utils.c b/common/utils.c index 83c8b59416a9..102eed7a59b3 100644 --- a/common/utils.c +++ b/common/utils.c @@ -171,8 +171,7 @@ char *utf8_str(const tal_t *ctx, const u8 *buf TAKES, size_t buflen) char *ret; if (!utf8_check(buf, buflen)) { - if (taken(buf)) - tal_free(buf); + tal_free_if_taken(buf); return NULL; } diff --git a/common/utils.h b/common/utils.h index a95da849f7ea..cc378e62ff1d 100644 --- a/common/utils.h +++ b/common/utils.h @@ -97,6 +97,13 @@ void tal_arr_remove_(void *p, size_t elemsize, size_t n); (*(p))[n] = (v); \ } while(0) +/* Helper to free an ptr if it's taken() */ +static inline void tal_free_if_taken(const tal_t *p) +{ + if (taken(p)) + tal_free(p); +} + /* Check for valid UTF-8 */ bool utf8_check(const void *buf, size_t buflen); diff --git a/connectd/multiplex.c b/connectd/multiplex.c index 0c5b0e4f0222..b81c3678913e 100644 --- a/connectd/multiplex.c +++ b/connectd/multiplex.c @@ -428,8 +428,7 @@ static u8 *process_batch_elements(const tal_t *ctx, struct peer *peer, const u8 } while(plen); - if (taken(msg)) - tal_free(msg); + tal_free_if_taken(msg); return ret; } diff --git a/db/utils.c b/db/utils.c index 2091111089ce..ebe7c35118c5 100644 --- a/db/utils.c +++ b/db/utils.c @@ -200,8 +200,7 @@ void db_exec_prepared_v2(struct db_stmt *stmt TAKES) db_fatal(stmt->db, "Error executing statement: %s", stmt->error); } - if (taken(stmt)) - tal_free(stmt); + tal_free_if_taken(stmt); } size_t db_count_changes(struct db_stmt *stmt) @@ -221,8 +220,7 @@ u64 db_last_insert_id_v2(struct db_stmt *stmt TAKES) assert(stmt->executed); id = stmt->db->config->last_insert_id_fn(stmt); - if (taken(stmt)) - tal_free(stmt); + tal_free_if_taken(stmt); return id; } diff --git a/devtools/gossmap-compress.c b/devtools/gossmap-compress.c index 5c0be91b5e39..662b44178398 100644 --- a/devtools/gossmap-compress.c +++ b/devtools/gossmap-compress.c @@ -304,8 +304,7 @@ static void write_msg_to_gstore(int outfd, const u8 *msg TAKES) || !write_all(outfd, msg, tal_bytelen(msg))) { err(1, "Writing gossip_store"); } - if (taken(msg)) - tal_free(msg); + tal_free_if_taken(msg); } /* BOLT #7: diff --git a/gossipd/gossipd.c b/gossipd/gossipd.c index 6c0d07396d51..a2c6fdf26315 100644 --- a/gossipd/gossipd.c +++ b/gossipd/gossipd.c @@ -103,8 +103,7 @@ void queue_peer_msg(struct daemon *daemon, u8 *outermsg = towire_gossipd_send_gossip(NULL, peer, msg); daemon_conn_send(daemon->connectd, take(outermsg)); - if (taken(msg)) - tal_free(msg); + tal_free_if_taken(msg); } /*~Routines to handle gossip messages from peer, forwarded by connectd. diff --git a/lightningd/channel.c b/lightningd/channel.c index b15fdb2c2672..6ca82cb47681 100644 --- a/lightningd/channel.c +++ b/lightningd/channel.c @@ -1249,8 +1249,7 @@ void channel_set_billboard(struct channel *channel, bool perm, const char *str) if (str) { *p = tal_fmt(channel, "%s:%s", channel_state_name(channel), str); - if (taken(str)) - tal_free(str); + tal_free_if_taken(str); } } diff --git a/lightningd/channel_gossip.c b/lightningd/channel_gossip.c index cb5a42d83489..5daa79b68080 100644 --- a/lightningd/channel_gossip.c +++ b/lightningd/channel_gossip.c @@ -378,8 +378,7 @@ static void msg_to_peer(const struct peer *peer, const u8 *msg TAKES) msg))); } - if (taken(msg)) - tal_free(msg); + tal_free_if_taken(msg); } static void addgossip_reply(struct subd *gossipd, @@ -1110,8 +1109,7 @@ void channel_gossip_update_from_gossipd(struct channel *channel, * when we restarted; ignore, as it will catch up soon. */ case CGOSSIP_CHANNEL_ANNOUNCED_DEAD: case CGOSSIP_CHANNEL_ANNOUNCED_DYING: - if (taken(channel_update)) - tal_free(channel_update); + tal_free_if_taken(channel_update); return; /* This happens: we step back a block when restarting. */ diff --git a/lightningd/jsonrpc.c b/lightningd/jsonrpc.c index cb73246e02bc..6d9e83aa605e 100644 --- a/lightningd/jsonrpc.c +++ b/lightningd/jsonrpc.c @@ -1571,8 +1571,7 @@ struct jsonrpc_request *jsonrpc_request_start_( } else { r->id = tal_fmt(r, "\"cln:%s#%"PRIu64"\"", method, next_request_id); } - if (taken(id_prefix)) - tal_free(id_prefix); + tal_free_if_taken(id_prefix); next_request_id++; r->notify_cb = notify_cb; r->response_cb = response_cb; diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index b64337505a73..505803984464 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -277,8 +277,7 @@ void local_fail_in_htlc(struct htlc_in *hin, const u8 *failmsg TAKES) hin->shared_secret, failmsg); - if (taken(failmsg)) - tal_free(failmsg); + tal_free_if_taken(failmsg); fail_in_htlc(hin, take(failonion)); } @@ -994,8 +993,7 @@ static u8 *prepend_length(const tal_t *ctx, const u8 *payload TAKES) ret = tal_arr(ctx, u8, len + tal_bytelen(payload)); memcpy(ret, buf, len); memcpy(ret + len, payload, tal_bytelen(payload)); - if (taken(payload)) - tal_free(payload); + tal_free_if_taken(payload); return ret; } diff --git a/lightningd/plugin.c b/lightningd/plugin.c index 447eb59d7cde..a9098ad0a81c 100644 --- a/lightningd/plugin.c +++ b/lightningd/plugin.c @@ -357,8 +357,7 @@ struct plugin *plugin_register(struct plugins *plugins, const char* path TAKES, "Plugin changed, needs restart."); break; } - if (taken(path)) - tal_free(path); + tal_free_if_taken(path); return NULL; } } @@ -2454,8 +2453,7 @@ bool plugin_single_notify(struct plugin *p, } else interested = false; - if (taken(n)) - tal_free(n); + tal_free_if_taken(n); return interested; } diff --git a/lightningd/watch.c b/lightningd/watch.c index fdc57f07f99d..e18ea8d107fd 100644 --- a/lightningd/watch.c +++ b/lightningd/watch.c @@ -315,8 +315,7 @@ void txwatch_inform(const struct chain_topology *topo, } /* If we don't clone above, handle take() now */ - if (taken(tx)) - tal_free(tx); + tal_free_if_taken(tx); } struct scriptpubkeywatch { diff --git a/onchaind/onchaind.c b/onchaind/onchaind.c index 5f2ad78dfd56..9b9beb26a571 100644 --- a/onchaind/onchaind.c +++ b/onchaind/onchaind.c @@ -215,8 +215,7 @@ static void send_coin_mvt(struct chain_coin_mvt *mvt TAKES) wire_sync_write(REQ_FD, take(towire_onchaind_notify_coin_mvt(NULL, mvt))); - if (taken(mvt)) - tal_free(mvt); + tal_free_if_taken(mvt); } static void record_channel_withdrawal(const struct bitcoin_txid *tx_txid, diff --git a/onchaind/test/run-grind_feerate-bug.c b/onchaind/test/run-grind_feerate-bug.c index a77627bcd093..485490201537 100644 --- a/onchaind/test/run-grind_feerate-bug.c +++ b/onchaind/test/run-grind_feerate-bug.c @@ -229,8 +229,7 @@ u8 *wire_sync_read(const tal_t *ctx, int fd UNNEEDED) bool wire_sync_write(int fd UNNEEDED, const void *msg TAKES) { - if (taken(msg)) - tal_free(msg); + tal_free_if_taken(msg); return true; } diff --git a/plugins/bkpr/test/run-recorder.c b/plugins/bkpr/test/run-recorder.c index db05aae63db7..0f3f6d03140c 100644 --- a/plugins/bkpr/test/run-recorder.c +++ b/plugins/bkpr/test/run-recorder.c @@ -118,8 +118,7 @@ struct json_out *json_out_obj(const tal_t *ctx, json_out_start(jout, NULL, '{'); if (str) json_out_addstr(jout, fieldname, str); - if (taken(str)) - tal_free(str); + tal_free_if_taken(str); json_out_end(jout, '}'); json_out_finished(jout); @@ -540,8 +539,7 @@ const jsmntok_t *jsonrpc_request_sync(const tal_t *ctx, assert(json_parse_input(&parser, &toks, buf, strlen(buf), &complete)); assert(complete); - if (taken(params)) - tal_free(params); + tal_free_if_taken(params); *resp = buf; return toks; } diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index a1895605fd35..9504950a7ec8 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -1547,8 +1547,7 @@ static u8 *patch_channel_update(const tal_t *ctx, u8 *channel_update TAKES) fixed = tal_arr(ctx, u8, 0); towire_u16(&fixed, WIRE_CHANNEL_UPDATE); towire(&fixed, channel_update, tal_bytelen(channel_update)); - if (taken(channel_update)) - tal_free(channel_update); + tal_free_if_taken(channel_update); return fixed; } else { return tal_dup_talarr(ctx, u8, channel_update); diff --git a/plugins/libplugin.c b/plugins/libplugin.c index 86305935eee0..b39058d46559 100644 --- a/plugins/libplugin.c +++ b/plugins/libplugin.c @@ -543,8 +543,7 @@ struct json_out *json_out_obj(const tal_t *ctx, json_out_start(jout, NULL, '{'); if (str) json_out_addstr(jout, fieldname, str); - if (taken(str)) - tal_free(str); + tal_free_if_taken(str); json_out_end(jout, '}'); json_out_finished(jout); @@ -791,8 +790,7 @@ static const jsmntok_t *sync_req(const tal_t *ctx, json_out_start(jout, "params", '{'); json_out_end(jout, '}'); } - if (taken(params)) - tal_free(params); + tal_free_if_taken(params); /* If we're past init, we may need a new fd (the old one * is being used for async comms). */ diff --git a/plugins/renepay/routefail.c b/plugins/renepay/routefail.c index 4f450c6a4ace..a6df2c2bd013 100644 --- a/plugins/renepay/routefail.c +++ b/plugins/renepay/routefail.c @@ -73,8 +73,7 @@ static u8 *patch_channel_update(const tal_t *ctx, u8 *channel_update TAKES) fixed = tal_arr(ctx, u8, 0); towire_u16(&fixed, WIRE_CHANNEL_UPDATE); towire(&fixed, channel_update, tal_bytelen(channel_update)); - if (taken(channel_update)) - tal_free(channel_update); + tal_free_if_taken(channel_update); return fixed; } else { return tal_dup_talarr(ctx, u8, channel_update); diff --git a/wallet/account_migration.c b/wallet/account_migration.c index 812b4d6aefb4..f913c14ab57c 100644 --- a/wallet/account_migration.c +++ b/wallet/account_migration.c @@ -184,8 +184,7 @@ static struct chain_event **find_chain_events(const tal_t *ctx, tal_arr_expand(&results, e); } - if (taken(stmt)) - tal_free(stmt); + tal_free_if_taken(stmt); return results; } diff --git a/wallet/invoices.c b/wallet/invoices.c index 085618c77ecd..3da390451ce3 100644 --- a/wallet/invoices.c +++ b/wallet/invoices.c @@ -278,10 +278,8 @@ bool invoices_create(struct invoices *invoices, u64 now = clock_time().ts.tv_sec; if (invoices_find_by_label(invoices, inv_dbid, label)) { - if (taken(msat)) - tal_free(msat); - if (taken(label)) - tal_free(label); + tal_free_if_taken(msat); + tal_free_if_taken(label); return false; } @@ -335,10 +333,8 @@ bool invoices_create(struct invoices *invoices, install_expiration_timer(invoices); } - if (taken(msat)) - tal_free(msat); - if (taken(label)) - tal_free(label); + tal_free_if_taken(msat); + tal_free_if_taken(label); return true; } diff --git a/wallet/wallet.c b/wallet/wallet.c index d2a3e215b3a5..bfdf31e3d4a6 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -6922,8 +6922,7 @@ static u64 insert_channel_mvt(struct lightningd *ld, db_exec_prepared_v2(take(stmt)); notify_channel_mvt(ld, chan_mvt, id); - if (taken(chan_mvt)) - tal_free(chan_mvt); + tal_free_if_taken(chan_mvt); return id; } @@ -7115,8 +7114,7 @@ void wallet_save_chain_mvt(struct lightningd *ld, id = insert_chain_mvt(ld, ld->wallet->db, chain_mvt); notify_chain_mvt(ld, chain_mvt, id); out: - if (taken(chain_mvt)) - tal_free(chain_mvt); + tal_free_if_taken(chain_mvt); } static void db_cols_account(struct db_stmt *stmt, diff --git a/wire/wire_sync.c b/wire/wire_sync.c index ce2107b93c2b..9545151362c2 100644 --- a/wire/wire_sync.c +++ b/wire/wire_sync.c @@ -1,6 +1,7 @@ #include "config.h" #include #include +#include #include #include #include @@ -14,8 +15,7 @@ bool wire_sync_write(int fd, const void *msg TAKES) ret = write_all(fd, &hdr, sizeof(hdr)) && write_all(fd, msg, tal_count(msg)); - if (taken(msg)) - tal_free(msg); + tal_free_if_taken(msg); return ret; } From 448f71fa93b4eb2b8351c35b7ac1d3ac924ed46e Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 25 Mar 2026 21:45:05 +1030 Subject: [PATCH 2/4] common: add (and use) status_unusual_once and status_broken_once helpers. Clarify the case where we only want to log once. Signed-off-by: Rusty Russell --- common/status.h | 17 +++++++++++++++++ connectd/connectd.c | 8 +++----- connectd/multiplex.c | 10 ++++------ gossipd/gossmap_manage.c | 14 ++++++-------- 4 files changed, 30 insertions(+), 19 deletions(-) diff --git a/common/status.h b/common/status.h index 95d51593b2c3..fb2cad77ae01 100644 --- a/common/status.h +++ b/common/status.h @@ -50,6 +50,23 @@ void status_io(enum log_level iodir, #define status_broken( ...) \ status_fmt(LOG_BROKEN, NULL, __VA_ARGS__) +/* Common case of logging once, based on bool flag */ +#define status_unusual_once(flag, ...) \ + do { \ + if (*(flag) != true) { \ + status_fmt(LOG_UNUSUAL, NULL, __VA_ARGS__); \ + (*flag) = true; \ + } \ + } while(0) + +#define status_broken_once(flag, ...) \ + do { \ + if (*(flag) != true) { \ + status_fmt(LOG_BROKEN, NULL, __VA_ARGS__); \ + (*flag) = true; \ + } \ + } while(0) + /* For daemons which handle multiple peers */ #define status_peer_trace(peer, ...) \ status_fmt(LOG_TRACE, (peer), __VA_ARGS__) diff --git a/connectd/connectd.c b/connectd/connectd.c index 14dbb36bdba6..4689ff74cea1 100644 --- a/connectd/connectd.c +++ b/connectd/connectd.c @@ -646,11 +646,9 @@ static struct io_plan *connection_in(struct io_conn *conn, /* Did we fail to accept? */ if (!conn) { static bool accept_logged = false; - if (!accept_logged) { - status_broken("accepting incoming fd failed: %s", - strerror(errno)); - accept_logged = true; - } + status_broken_once(&accept_logged, + "accepting incoming fd failed: %s", + strerror(errno)); /* Maybe free up some fds by closing something. */ close_random_connection(daemon); return NULL; diff --git a/connectd/multiplex.c b/connectd/multiplex.c index b81c3678913e..394042519c44 100644 --- a/connectd/multiplex.c +++ b/connectd/multiplex.c @@ -1626,12 +1626,10 @@ void peer_connect_subd(struct daemon *daemon, const u8 *msg, int fd) * (subd will see immediate hangup). */ if (fd == -1) { static bool recvfd_logged = false; - if (!recvfd_logged) { - status_broken("receiving lightningd fd failed for %s: %s", - fmt_node_id(tmpctx, &id), - strerror(errno)); - recvfd_logged = true; - } + status_broken_once(&recvfd_logged, + "receiving lightningd fd failed for %s: %s", + fmt_node_id(tmpctx, &id), + strerror(errno)); /* Maybe free up some fds by closing something. */ close_random_connection(daemon); return; diff --git a/gossipd/gossmap_manage.c b/gossipd/gossmap_manage.c index 3210f31fbb75..67089e42d883 100644 --- a/gossipd/gossmap_manage.c +++ b/gossipd/gossmap_manage.c @@ -184,14 +184,12 @@ static bool map_add(struct cannounce_map *map, { /* More than 10000 pending things? Stop! */ if (map->count > 10000) { - if (!map->flood_reported) { - status_unusual("%s being flooded by %s: dropping some", - map->name, - pca->source_peer - ? fmt_node_id(tmpctx, pca->source_peer) - : "unknown"); - map->flood_reported = true; - } + status_unusual_once(&map->flood_reported, + "%s being flooded by %s: dropping some", + map->name, + pca->source_peer + ? fmt_node_id(tmpctx, pca->source_peer) + : "unknown"); return false; } From 41bd01b232532a5809ea17e44783dd73ab27eadc Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 25 Mar 2026 21:45:06 +1030 Subject: [PATCH 3/4] gossipd: put common size restrictions on all maps. We already limited pending_ann_map and early_ann_map to 10,000 entries: do the same for pending_nannounces, pending_cupdates and early_cupdates. Add CI_UNEXPECTED so CI gets upset if this happens unexpectedly. Signed-off-by: Rusty Russell --- gossipd/gossmap_manage.c | 40 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-) diff --git a/gossipd/gossmap_manage.c b/gossipd/gossmap_manage.c index 67089e42d883..4cedf2117f7e 100644 --- a/gossipd/gossmap_manage.c +++ b/gossipd/gossmap_manage.c @@ -29,6 +29,7 @@ #include #include +#define PENDING_LIMIT 10000 #define GOSSIP_STORE_COMPACT_FILENAME "gossip_store.compact" struct pending_cannounce { @@ -133,9 +134,24 @@ static void enqueue_cupdate(struct pending_cupdate ***queue, u32 fee_proportional_millionths, u32 timestamp, const u8 *update TAKES, - const struct node_id *source_peer TAKES) + const struct node_id *source_peer) { - struct pending_cupdate *pcu = tal(*queue, struct pending_cupdate); + struct pending_cupdate *pcu; + + if (tal_count(*queue) > PENDING_LIMIT) { + static bool warned = false; + status_unusual_once(&warned, + CI_UNEXPECTED + "channel_updates being flooded by %s: dropping some", + source_peer + ? fmt_node_id(tmpctx, source_peer) + : "unknown"); + tal_free_if_taken(update); + tal_free_if_taken(source_peer); + return; + } + + pcu = tal(*queue, struct pending_cupdate); pcu->scid = scid; pcu->signature = *signature; @@ -159,7 +175,22 @@ static void enqueue_nannounce(struct pending_nannounce ***queue, const u8 *nannounce TAKES, const struct node_id *source_peer TAKES) { - struct pending_nannounce *pna = tal(*queue, struct pending_nannounce); + struct pending_nannounce *pna; + + if (tal_count(*queue) > PENDING_LIMIT) { + static bool warned = false; + status_unusual_once(&warned, + CI_UNEXPECTED + "node_announcements being flooded by %s: dropping some", + source_peer + ? fmt_node_id(tmpctx, source_peer) + : "unknown"); + tal_free_if_taken(nannounce); + tal_free_if_taken(source_peer); + return; + } + + pna = tal(*queue, struct pending_nannounce); pna->node_id = *node_id; pna->timestamp = timestamp; @@ -183,8 +214,9 @@ static bool map_add(struct cannounce_map *map, struct pending_cannounce *pca) { /* More than 10000 pending things? Stop! */ - if (map->count > 10000) { + if (map->count > PENDING_LIMIT) { status_unusual_once(&map->flood_reported, + CI_UNEXPECTED "%s being flooded by %s: dropping some", map->name, pca->source_peer From 71899b0e8baa0bdec8cc304a55702abd317daa05 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 27 Mar 2026 13:40:00 +1030 Subject: [PATCH 4/4] connectd: limit incoming traffic to 1MB per second. Decryption is pretty efficient, but incoming traffic can bog down connectd, especially on smaller nodes, so simply limit it to 1MB per second. This triggers in various tests, which is good: shows that it's working, and that we continue to (slowly!) process traffic. Changelog-Fixed: connectd: throttle incoming peers to give fairer peer handling under stress. Signed-off-by: Rusty Russell --- connectd/connectd.c | 9 ++++++++- connectd/connectd.h | 14 +++++++++++++- connectd/multiplex.c | 38 ++++++++++++++++++++++++++++++++++++++ tests/test_askrene.py | 3 +++ tests/test_gossip.py | 4 +++- tests/test_plugin.py | 2 +- tests/test_xpay.py | 1 + 7 files changed, 67 insertions(+), 4 deletions(-) diff --git a/connectd/connectd.c b/connectd/connectd.c index 4689ff74cea1..ad3f11eca9cf 100644 --- a/connectd/connectd.c +++ b/connectd/connectd.c @@ -128,6 +128,10 @@ static struct peer *new_peer(struct daemon *daemon, peer->cs = *cs; peer->subds = tal_arr(peer, struct subd *, 0); peer->peer_in = NULL; + peer->bytes_rcvd_this_second = 0; + peer->bytes_rcvd_start_time = time_mono(); + peer->recv_timer = NULL; + peer->throttle_warned = false; membuf_init(&peer->encrypted_peer_out, tal_arr(peer, u8, 0), 0, membuf_tal_resize); @@ -1751,8 +1755,10 @@ static void connect_init(struct daemon *daemon, const u8 *msg) } /* 500 bytes per second, not 1M per second */ - if (dev_throttle_gossip) + if (dev_throttle_gossip) { daemon->gossip_stream_limit = 500; + daemon->incoming_stream_limit = 500; + } if (dev_limit_connections_inflight) daemon->max_connect_in_flight = 1; @@ -2565,6 +2571,7 @@ int main(int argc, char *argv[]) daemon->dev_keep_nagle = false; /* We generally allow 1MB per second per peer, except for dev testing */ daemon->gossip_stream_limit = 1000000; + daemon->incoming_stream_limit = 1000000; daemon->scid_htable = new_htable(daemon, scid_htable); /* stdin == control */ diff --git a/connectd/connectd.h b/connectd/connectd.h index ea012f6a2824..102d70500fb1 100644 --- a/connectd/connectd.h +++ b/connectd/connectd.h @@ -83,6 +83,15 @@ struct peer { /* Input buffer. */ u8 *peer_in; + /* Bytes received in the last second. */ + size_t bytes_rcvd_this_second; + /* When that second starts */ + struct timemono bytes_rcvd_start_time; + /* Timer when we're throttling input */ + struct oneshot *recv_timer; + /* Only send message once if peer gets throttled */ + bool throttle_warned; + /* Output buffer. */ struct msg_queue *peer_outq; @@ -309,9 +318,12 @@ struct daemon { /* Allow localhost to be considered "public", only with --developer */ bool dev_allow_localhost; - /* How much to gossip allow a peer every 60 seconds (bytes) */ + /* How much to gossip allow a peer every second (bytes) */ size_t gossip_stream_limit; + /* How much incomign traffic do we allow per peer every second (bytes) */ + size_t incoming_stream_limit; + /* We support use of a SOCKS5 proxy (e.g. Tor) */ struct addrinfo *proxyaddr; diff --git a/connectd/multiplex.c b/connectd/multiplex.c index 394042519c44..c219ba8d77c2 100644 --- a/connectd/multiplex.c +++ b/connectd/multiplex.c @@ -1395,6 +1395,8 @@ static struct io_plan *read_body_from_peer_done(struct io_conn *peer_conn, tal_bytelen(peer->peer_in)); return io_close(peer_conn); } + + peer->bytes_rcvd_this_second += tal_bytelen(peer->peer_in); tal_free(peer->peer_in); type = fromwire_peektype(decrypted); @@ -1511,16 +1513,52 @@ static struct io_plan *read_body_from_peer(struct io_conn *peer_conn, if (!cryptomsg_decrypt_header(&peer->cs, peer->peer_in, &len)) return io_close(peer_conn); + peer->bytes_rcvd_this_second += tal_bytelen(peer->peer_in); + tal_resize(&peer->peer_in, (u32)len + CRYPTOMSG_BODY_OVERHEAD); return io_read(peer_conn, peer->peer_in, tal_count(peer->peer_in), read_body_from_peer_done, peer); } +static void recv_throttle_timeout(struct peer *peer) +{ + peer->recv_timer = tal_free(peer->recv_timer); + io_wake(&peer->peer_in); +} + static struct io_plan *read_hdr_from_peer(struct io_conn *peer_conn, struct peer *peer) { + struct timemono now = time_mono(); assert(peer->to_peer == peer_conn); + /* If it's been over a second, make a fresh start. */ + if (time_to_sec(timemono_between(now, peer->bytes_rcvd_start_time)) > 0) { + peer->bytes_rcvd_start_time = now; + peer->bytes_rcvd_this_second = 0; + } + + /* You sent too much this second? */ + if (peer->bytes_rcvd_this_second > peer->daemon->incoming_stream_limit) { + status_unusual_once(&peer->throttle_warned, + CI_UNEXPECTED + "Throttling incoming peer %s:" + " too much traffic", + fmt_node_id(tmpctx, &peer->id)); + + /* Set timer for next second (if not already) */ + if (!peer->recv_timer) { + peer->recv_timer = new_abstimer(&peer->daemon->timers, + peer, + timemono_add(peer->bytes_rcvd_start_time, + time_from_sec(1)), + recv_throttle_timeout, + peer); + } + return io_wait(peer_conn, &peer->peer_in, + read_hdr_from_peer, peer); + } + /* BOLT #8: * * ### Receiving and Decrypting Messages diff --git a/tests/test_askrene.py b/tests/test_askrene.py index 513e9ec7197f..43ebf3088a4d 100644 --- a/tests/test_askrene.py +++ b/tests/test_askrene.py @@ -1463,6 +1463,7 @@ def test_real_data(node_factory, bitcoind, executor): opts=[{'gossip_store_file': outfile.name, 'allow_warning': True, 'dev-throttle-gossip': None, + 'broken_log': 'Throttling incoming peer', # This can be slow! 'askrene-timeout': TIMEOUT}, {'allow_warning': True}]) @@ -1573,6 +1574,7 @@ def test_real_biases(node_factory, bitcoind, executor): l1, l2 = node_factory.line_graph(2, fundamount=AMOUNT, opts=[{'gossip_store_file': outfile.name, 'allow_warning': True, + 'broken_log': 'Throttling incoming peer', 'dev-throttle-gossip': None}, {'allow_warning': True}]) @@ -1685,6 +1687,7 @@ def test_askrene_fake_channeld(node_factory, bitcoind): opts=[{'gossip_store_file': outfile.name, 'subdaemon': 'channeld:../tests/plugins/channeld_fakenet', 'allow_warning': True, + 'broken_log': 'Throttling incoming peer', 'dev-throttle-gossip': None}, {'allow_bad_gossip': True}]) diff --git a/tests/test_gossip.py b/tests/test_gossip.py index 30f94174ded9..c8c838e40d81 100644 --- a/tests/test_gossip.py +++ b/tests/test_gossip.py @@ -2164,7 +2164,9 @@ def test_dump_own_gossip(node_factory): def test_gossip_throttle(node_factory, bitcoind, chainparams): """Make some gossip, test it gets throttled""" l1, l2, l3, l4 = node_factory.line_graph(4, wait_for_announce=True, - opts=[{}, {}, {}, {'dev-throttle-gossip': None}]) + opts=[{}, {}, {}, + {'broken_log': 'Throttling incoming peer', + 'dev-throttle-gossip': None}]) # We expect: self-advertizement (3 messages for l1 and l4) plus # 4 node announcements, 3 channel announcements and 6 channel updates. diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 6815fb080801..84b53f726365 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -3160,7 +3160,7 @@ def test_commando(node_factory, executor): @pytest.mark.slow_test def test_commando_stress(node_factory, executor): """Stress test to slam commando with many large queries""" - nodes = node_factory.get_nodes(5) + nodes = node_factory.get_nodes(5, opts=[{'broken_log': 'Throttling incoming peer'}, {}, {}, {}, {}]) rune = nodes[0].rpc.createrune()['rune'] for n in nodes[1:]: diff --git a/tests/test_xpay.py b/tests/test_xpay.py index c5ad9160600b..9dd07c32c041 100644 --- a/tests/test_xpay.py +++ b/tests/test_xpay.py @@ -241,6 +241,7 @@ def test_xpay_fake_channeld(node_factory, bitcoind, chainparams, slow_mode): 'allow_warning': True, 'dev-throttle-gossip': None, 'log-level': 'info', + 'broken_log': 'Throttling incoming peer', # xpay gets upset if it's aging when we remove cln-askrene! 'dev-xpay-no-age': None, },