Skip to content

Stabilize test_grpc_wrong_auth, fix askrene node-bias hashing - #9580

Open
cdecker wants to merge 4 commits into
masterfrom
2026m09-ci-fix
Open

cdecker wants to merge 4 commits into
masterfrom
2026m09-ci-fix

Conversation

@cdecker

@cdecker cdecker commented Sep 28, 2026

Copy link
Copy Markdown
Member

A couple of small fixes collected in one place:

  • pytest: make test_grpc_wrong_auth cross the wires on one port, with deadlines. After l1 stopped, the old stub dialled l1's now-free gRPC port while l2 listened elsewhere, so the test never checked a foreign certificate against a live node. Its calls also had no deadline, so a client retrying a rejected handshake could hang the test forever. l2 now starts on l1's port, the client connects to 127.0.0.1 explicitly, and every call has a deadline.
  • askrene: don't type-pun the node id when hashing node biases. hash_nodeid() read the 33-byte node id through a size_t pointer. That's undefined behaviour, and at -O3 the load can be hoisted above the copy in set_node_bias(), so a persistent layer's node biases were duplicated on every restart and could trip the NODUPS assertion when loading the layer. The key is now hashed with siphash, like hash_str().

🤖 Generated with Claude Code

cdecker and others added 2 commits September 28, 2026 13:12
…eadlines

After l1 stopped, the old stub dialled l1's now-free gRPC port while l2
listened on a different one, so the test never checked a foreign
certificate against a live node. Its gRPC calls also had no deadline: when
the client keeps retrying a handshake the server rejects, the call blocked
forever and hung the test.

Start l2 on l1's port, connect to 127.0.0.1 explicitly, and give every call
a deadline. The call with the wrong credentials may now end in
DEADLINE_EXCEEDED as well as UNAVAILABLE; both mean it was refused.

Changelog-None

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
hash_nodeid() read the 33-byte node id through a size_t pointer.  That
is undefined behaviour, and at -O3 the compiler is entitled to hoist
that load above the `bias->node = *node` copy in set_node_bias(), since
a u8 array and a size_t cannot alias.  The entry then went into the
table under the hash of uninitialised memory and later lookups missed
it, so every load of a persistent layer created a second node_biases
entry for the same node.  With load_node_bias() now setting the two
directions in two calls, the second call trips the NODUPS assertion in
node_bias_hash_add() and askrene, an important plugin, takes lightningd
down with it.

Hash the whole key with siphash, as hash_str() below already does.

Changelog-Fixed: askrene: node biases on a persistent layer were duplicated on every restart, and could abort the plugin when loading a layer.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@cdecker
cdecker marked this pull request as ready for review September 28, 2026 13:18
@cdecker
cdecker requested a review from Lagrang3 as a code owner September 28, 2026 13:18
daywalker90
daywalker90 previously approved these changes Sep 28, 2026
The file is no longer used by any pipeline and has gone stale.

Changelog-None

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@cdecker
cdecker enabled auto-merge (rebase) September 28, 2026 16:29

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants