← Watch feed
lightningd: require local_alias in new_channel().
Public commit record
68/100 · Adequate
What the developer wrote
Authored by Rusty Russell
lightningd: require local_alias in new_channel().
We allowed NULL for stub channels, but just don't put the stub scid
into the hash tables. This cleans up all the callers to make it
clear this is a non-optional parameter.
We opencode channel_set_random_local_alias, since there's only one caller now.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
✓ Descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context
What changed, and why it matters
This commit is a code-cleanup change in Core Lightning that makes the local channel alias a required value when creating a channel record, instead of allowing it to be optional/NULL. It removes a fallback that generated a random alias if one was missing. The change is defensive: it ensures every channel has a known local alias and avoids subtle bugs where a missing alias could lead to inconsistent channel lookup tables. There is no direct evidence in the commit or supplied references that this fixes an active security vulnerability.
Recommended action
Treat as a hardening/cleanup commit. Review that all callers of new_channel() and new_unsaved_channel() in downstream branches provide a valid local alias, and verify the stub SCID (1x1x1) is never inserted into the chanmap. No urgent security action is indicated by the supplied materials.
Security signals we found
01
Defensive invariant enforcement: local alias becomes mandatory rather than optional
02
Removal of NULL fallback path that silently generated random aliases
03
Hash table (chanmap) insertion logic now conditional on non-stub alias
04
Memory ownership semantics clarified (TAKES vs pass-by-value) reducing use-after-free/double-free risk
05
No explicit security bug, CVE, or vulnerability described in commit message
Evidence from the diff
The patch refactors channel creation so new_channel() requires a non-NULL local alias (struct short_channel_id alias_local passed by value, not pointer). It removes channel_set_random_local_alias() and inlines the random alias generation into new_unsaved_channel(). Callers (wallet_commit_channel, stub_chan, wallet_stmt2channel, tests) are updated to always provide a local alias. The alias is now added to the channel hash table unless it is the reserved stub SCID (1x1x1). Memory ownership annotations change from STEALS/TAKES to TAKES for scid and pass-by-value for alias_local. The change prevents channels from being created without a local alias, which could previously happen via the NULL fallback path.
Changed components
lightningd/channel.clightningd/channel.hlightningd/opening_control.cwallet/wallet.clightningd/test/run-invoice-select-inchan.cwallet/test/run-db.cwallet/test/run-wallet.cInspect captured patch +35 / −40
diff --git a/lightningd/channel.c b/lightningd/channel.c
index 589860e1..b6e9593e 100644
--- a/lightningd/channel.c
+++ b/lightningd/channel.c
@@ -280,16 +280,6 @@ static void chanmap_add(struct lightningd *ld,
tal_add_destructor2(scc, destroy_scid_to_channel, ld);
}
-static void channel_set_random_local_alias(struct channel *channel)
-{
- assert(channel->alias[LOCAL] == NULL);
- channel->alias[LOCAL] = tal(channel, struct short_channel_id);
- *channel->alias[LOCAL] = random_scid();
- /* We don't check for uniqueness. We would crash on a clash, but your machine is
- * probably broken beyond repair if it gets two equal 64 bit numbers */
- chanmap_add(channel->peer->ld, channel, *channel->alias[LOCAL]);
-}
-
void channel_set_scid(struct channel *channel, const struct short_channel_id *new_scid)
{
struct lightningd *ld = channel->peer->ld;
@@ -367,8 +357,12 @@ struct channel *new_unsaved_channel(struct peer *peer,
= CLOSING_FEE_NEGOTIATION_STEP_UNIT_PERCENTAGE;
channel->shutdown_wrong_funding = NULL;
channel->closing_feerate_range = NULL;
- channel->alias[REMOTE] = channel->alias[LOCAL] = NULL;
- channel_set_random_local_alias(channel);
+ channel->alias[REMOTE] = NULL;
+ channel->alias[LOCAL] = tal(channel, struct short_channel_id);
+ *channel->alias[LOCAL] = random_scid();
+ /* We don't check for uniqueness. We would crash on a clash, but your machine is
+ * probably broken beyond repair if it gets two equal 64 bit numbers */
+ chanmap_add(channel->peer->ld, channel, *channel->alias[LOCAL]);
channel->shutdown_scriptpubkey[REMOTE] = NULL;
channel->last_was_revoke = false;
@@ -489,9 +483,9 @@ struct channel *new_channel(struct peer *peer, u64 dbid,
struct amount_sat our_funds,
bool remote_channel_ready,
/* NULL or stolen */
- struct short_channel_id *scid,
+ struct short_channel_id *scid TAKES,
struct short_channel_id *old_scids TAKES,
- struct short_channel_id *alias_local TAKES,
+ struct short_channel_id alias_local,
struct short_channel_id *alias_remote STEALS,
struct channel_id *cid,
struct amount_msat our_msat,
@@ -617,22 +611,18 @@ struct channel *new_channel(struct peer *peer, u64 dbid,
channel->push = push;
channel->our_funds = our_funds;
channel->remote_channel_ready = remote_channel_ready;
- channel->scid = tal_steal(channel, scid);
+ channel->scid = tal_dup_or_null(channel, struct short_channel_id, scid);
channel->old_scids = tal_dup_talarr(channel, struct short_channel_id, old_scids);
- channel->alias[LOCAL] = tal_dup_or_null(channel, struct short_channel_id, alias_local);
+ channel->alias[LOCAL] = tal_dup(channel, struct short_channel_id, &alias_local);
/* All these possible short_channel_id variants go in the lookup table! */
/* Stub channels all have the same scid though, *and* get loaded from db! */
if (channel->scid && !is_stub_scid(*channel->scid))
chanmap_add(peer->ld, channel, *channel->scid);
- if (channel->alias[LOCAL])
+ if (!is_stub_scid(*channel->alias[LOCAL]))
chanmap_add(peer->ld, channel, *channel->alias[LOCAL]);
for (size_t i = 0; i < tal_count(channel->old_scids); i++)
chanmap_add(peer->ld, channel, channel->old_scids[i]);
- /* We always make sure this is set (historical channels from db might not) */
- if (!channel->alias[LOCAL])
- channel_set_random_local_alias(channel);
-
channel->alias[REMOTE] = tal_steal(channel, alias_remote); /* Haven't gotten one yet. */
channel->cid = *cid;
channel->our_msat = our_msat;
@@ -739,7 +729,7 @@ struct channel *new_channel(struct peer *peer, u64 dbid,
}
/* scid is NULL when opening a new channel so we don't
* need to set error in that case as well */
- if (scid && is_stub_scid(*scid))
+ if (channel->scid && is_stub_scid(*channel->scid))
channel->error = towire_errorfmt(peer->ld,
&channel->cid,
"We can't be together anymore.");
diff --git a/lightningd/channel.h b/lightningd/channel.h
index 82889071..0e4959e8 100644
--- a/lightningd/channel.h
+++ b/lightningd/channel.h
@@ -393,9 +393,9 @@ struct channel *new_channel(struct peer *peer, u64 dbid,
struct amount_sat our_funds,
bool remote_channel_ready,
/* NULL or stolen */
- struct short_channel_id *scid STEALS,
+ struct short_channel_id *scid TAKES,
struct short_channel_id *old_scids TAKES,
- struct short_channel_id *alias_local STEALS,
+ struct short_channel_id alias_local,
struct short_channel_id *alias_remote STEALS,
struct channel_id *cid,
struct amount_msat our_msatoshi,
diff --git a/lightningd/opening_control.c b/lightningd/opening_control.c
index 521fd833..4924923a 100644
--- a/lightningd/opening_control.c
+++ b/lightningd/opening_control.c
@@ -109,6 +109,7 @@ wallet_commit_channel(struct lightningd *ld,
struct timeabs timestamp;
struct channel_stats zero_channel_stats;
enum addrtype addrtype;
+ struct short_channel_id local_alias;
/* We can't have any payments yet */
memset(&zero_channel_stats, 0, sizeof(zero_channel_stats));
@@ -171,6 +172,8 @@ wallet_commit_channel(struct lightningd *ld,
else
static_remotekey_start = 0x7FFFFFFFFFFFFFFF;
+ local_alias = random_scid();
+
channel = new_channel(uc->peer, uc->dbid,
NULL, /* No shachain yet */
CHANNELD_AWAITING_LOCKIN,
@@ -189,7 +192,7 @@ wallet_commit_channel(struct lightningd *ld,
false, /* !remote_channel_ready */
NULL, /* no scid yet */
NULL, /* no old scids */
- NULL, /* assign random local alias */
+ local_alias, /* random local alias */
NULL, /* They haven't told us an alias yet */
cid,
/* The three arguments below are msatoshi_to_us,
@@ -1485,7 +1488,7 @@ static struct channel *stub_chan(struct command *cmd,
struct peer *peer;
struct pubkey localFundingPubkey;
struct pubkey pk;
- struct short_channel_id *scid;
+ struct short_channel_id scid;
u32 blockht;
u32 feerate;
struct channel_stats zero_channel_stats;
@@ -1563,10 +1566,9 @@ static struct channel *stub_chan(struct command *cmd,
channel_info->old_remote_per_commit = pk;
blockht = 100;
- scid = tal(cmd, struct short_channel_id);
/*To indicate this is an stub channel we keep it's scid to 1x1x1.*/
- if (!mk_short_channel_id(scid, 1, 1, 1))
+ if (!mk_short_channel_id(&scid, 1, 1, 1))
fatal("Failed to make short channel 1x1x1!");
memset(&zero_channel_stats, 0, sizeof(zero_channel_stats));
@@ -1587,9 +1589,9 @@ static struct channel *stub_chan(struct command *cmd,
AMOUNT_MSAT(0),
AMOUNT_SAT(0),
true, /* remote_channel_ready */
- scid,
- NULL,
+ &scid,
NULL,
+ scid,
NULL,
&cid,
/* The three arguments below are msatoshi_to_us,
diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c
index 5dfcd4e2..fc7c6424 100644
--- a/lightningd/test/run-invoice-select-inchan.c
+++ b/lightningd/test/run-invoice-select-inchan.c
@@ -135,6 +135,9 @@ void channel_set_last_tx(struct channel *channel UNNEEDED,
struct bitcoin_tx *tx UNNEEDED,
const struct bitcoin_signature *sig UNNEEDED)
{ fprintf(stderr, "channel_set_last_tx called!\n"); abort(); }
+/* Generated stub for channel_set_scid */
+void channel_set_scid(struct channel *channel UNNEEDED, const struct short_channel_id *new_scid UNNEEDED)
+{ fprintf(stderr, "channel_set_scid called!\n"); abort(); }
/* Generated stub for channel_state_name */
const char *channel_state_name(const struct channel *channel UNNEEDED)
{ fprintf(stderr, "channel_state_name called!\n"); abort(); }
diff --git a/wallet/test/run-db.c b/wallet/test/run-db.c
index 09ecb5b3..369c15a4 100644
--- a/wallet/test/run-db.c
+++ b/wallet/test/run-db.c
@@ -189,7 +189,7 @@ struct channel *new_channel(struct peer *peer UNNEEDED, u64 dbid UNNEEDED,
/* NULL or stolen */
struct short_channel_id *scid STEALS UNNEEDED,
struct short_channel_id *old_scids TAKES UNNEEDED,
- struct short_channel_id *alias_local STEALS UNNEEDED,
+ struct short_channel_id alias_local UNNEEDED,
struct short_channel_id *alias_remote STEALS UNNEEDED,
struct channel_id *cid UNNEEDED,
struct amount_msat our_msatoshi UNNEEDED,
diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c
index f8384643..b3279625 100644
--- a/wallet/test/run-wallet.c
+++ b/wallet/test/run-wallet.c
@@ -1983,6 +1983,7 @@ static bool test_channel_inflight_crud(struct lightningd *ld, const tal_t *ctx)
struct wally_psbt *funding_psbt;
struct channel_info *channel_info = tal(w, struct channel_info);
struct basepoints basepoints;
+ struct short_channel_id alias_local = random_scid();
secp256k1_ecdsa_signature *lease_commit_sig;
u32 feerate, lease_blockheight_start;
u64 dbid;
@@ -2034,7 +2035,7 @@ static bool test_channel_inflight_crud(struct lightningd *ld, const tal_t *ctx)
our_sats,
0, NULL,
NULL, /* old scids */
- NULL, /* alias[LOCAL] */
+ alias_local,
NULL, /* alias[REMOTE] */
&cid,
AMOUNT_MSAT(3333333000),
diff --git a/wallet/wallet.c b/wallet/wallet.c
index ce7c5248..1b2bcaa7 100644
--- a/wallet/wallet.c
+++ b/wallet/wallet.c
@@ -1779,7 +1779,7 @@ static struct channel *wallet_stmt2channel(struct wallet *w, struct db_stmt *stm
struct channel_info channel_info;
struct fee_states *fee_states;
struct height_states *height_states;
- struct short_channel_id *scid, *alias[NUM_SIDES], *old_scids;
+ struct short_channel_id *scid, alias_local, *alias_remote, *old_scids;
struct channel_id cid;
struct channel *chan;
u64 peer_dbid;
@@ -1819,9 +1819,8 @@ static struct channel *wallet_stmt2channel(struct wallet *w, struct db_stmt *stm
scid = db_col_optional_scid(tmpctx, stmt, "scid");
old_scids = db_col_short_channel_id_arr(tmpctx, stmt, "old_scids");
- alias[LOCAL] = tal(tmpctx, struct short_channel_id);
- *alias[LOCAL] = db_col_short_channel_id(stmt, "alias_local");
- alias[REMOTE] = db_col_optional_scid(tmpctx, stmt, "alias_remote");
+ alias_local = db_col_short_channel_id(stmt, "alias_local");
+ alias_remote = db_col_optional_scid(tmpctx, stmt, "alias_remote");
ok &= wallet_shachain_load(w, db_col_u64(stmt, "shachain_remote_id"),
&wshachain);
@@ -1976,7 +1975,7 @@ static struct channel *wallet_stmt2channel(struct wallet *w, struct db_stmt *stm
if (scid)
remote_update->scid = *scid;
else
- remote_update->scid = *alias[LOCAL];
+ remote_update->scid = alias_local;
remote_update->fee_base = db_col_int(stmt, "remote_feerate_base");
remote_update->fee_ppm = db_col_int(stmt, "remote_feerate_ppm");
remote_update->cltv_delta = db_col_int(stmt, "remote_cltv_expiry_delta");
@@ -2033,10 +2032,10 @@ static struct channel *wallet_stmt2channel(struct wallet *w, struct db_stmt *stm
push_msat,
our_funding_sat,
db_col_int(stmt, "funding_locked_remote") != 0,
- scid,
+ take(scid),
old_scids,
- alias[LOCAL],
- alias[REMOTE],
+ alias_local,
+ alias_remote,
&cid,
our_msat,
msat_to_us_min, /* msatoshi_to_us_min */
Risk score
Our methodology →Why this scored 25/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.