Skip to content

net: exempt manual peers from the hybrid fallback, fix its state - #250

Merged
djh58 merged 1 commit into
feature/pq-transportfrom
djh58/pq233-net-fallback
Oct 23, 2026
Merged

djh58 merged 1 commit into
feature/pq-transportfrom
djh58/pq233-net-fallback

Conversation

@djh58

@djh58 djh58 commented Oct 23, 2026 •

Copy link
Copy Markdown
Member

Fixes from review of #233. The main one is that manual peers never fall back.

  • Manual peers never fall back. An attacker on the network path needs no keys to make an endpoint's hybrid handshakes fail after the switch: they can damage the peer's key confirmation in transit, reset the connection, or drop its traffic. Three such failures put the endpoint in the fallback set, so connections to it ran ECDH-only for 1h, then 4h, then 24h, and this was repeatable.
    • Failures of MANUAL connections (addnode, -connect) no longer count toward a streak.
    • A manual connection always negotiates, even to an endpoint already in the fallback set.
    • Counters, rings, log lines and the local-fault warning still record these failures.
    • Automatic connections keep the fallback.
    • The spec's downgrade note, which said a downgrade needs a real-time ECDH break or a relaying man-in-the-middle, now describes this. The operator guide, the release note and the RPC reference are updated too.
  • The success set is capped at 1,000 endpoints, least recently confirmed evicted first. Feelers to advertised endpoints could grow it without bound. A separate flag records "a handshake succeeded since startup" for the local-fault warning.
  • ObservePQ() no longer re-reads fNetworkActive.
    • A verified confirmation is always accounted.
    • DisconnectNodes() passes its snapshot, so closes caused by switching the network off are never recorded as peer outcomes.
  • Fallback windows also end on the steady clock, so a backward wall-clock step can't stretch one. The RPC keeps wall-clock times.
  • The local-fault warning counts only failures this node's ML-KEM code can cause: ek_modulus, length, tag, and closed_after_switch with eof or reset. It used to count peer-format failures such as ek_length too.
  • getnetworkinfo counts connections_pq through CConnman::GetPQNodeCount(). It no longer copies every peer's stats or takes cs_main per peer, and the result is unchanged.
  • A backward step of the mocked steady clock restarts load shedding's window, so shedding can't latch on (test-only).
  • Test hygiene:
    • an RAII guard for CreateSock in net_pq_tests;
    • the rpc fuzz target now mocks the steady clock, because getpqtransportinfo reads it and the target aborted on real system time.
  • Test changes: the fallback functional tests use automatic connections (addconnection), since manual ones no longer fall back.

Verification:

  • Seven new net_pq_tests cases. Each one fails when its fix is reverted.
  • net_pq_tests, net_tests and bip324_tests pass.
  • The functional tests p2p_v2_pq, p2p_v2_pq_fallback, p2p_v2_pq_misbehaving, rpc_pqtransport, rpc_net, feature_pq_quickstart and tool_pq_canary pass.
  • lint-pq-doc-tokens.py and the vectors check pass.
  • A clang build with -Wthread-safety gives no warnings.
  • Fuzz targets ran 2,000 runs each with no aborts: rpc (with getpqtransportinfo, getnetworkinfo, getpeerinfo, and unlimited), connman, net, process_message(s), p2p_handshake, p2p_headers_presync, p2p_v2_pq_malicious_peer and p2p_transport_serialization.
  • An independent review traced each fix and found the fuzz abort, which is now fixed.

Since #245, pull requests into feature/pq-transport don't run CI. #233's full CI covers this change once it merges.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

Found in review of #233.

- Manual peers never fall back. An attacker on the path can make an
  endpoint's hybrid handshakes fail without any keys (damage the
  peer's confirmation, reset the connection, drop its traffic), and
  three failures put it in the fallback set, repeatably. Failures of
  MANUAL connections (addnode, -connect) no longer count toward a
  streak, and a manual connection always negotiates, even to an
  endpoint in the fallback set. Counters, rings, log lines and the
  local-fault warning still see them. Automatic connections keep the
  fallback.
- The success set is capped at 1,000 endpoints, the least recently
  confirmed evicted first: feelers to advertised endpoints grew it
  without bound. A separate flag keeps "a handshake succeeded since
  startup" for the local-fault warning.
- ObservePQ() no longer re-reads fNetworkActive. Events from the
  peer's bytes, a verified confirmation included, are always
  accounted; DisconnectNodes() passes the snapshot it closed
  connections with, so closes caused by switching the network off
  are never derived as abandoned or closed_after_switch.
- A fallback window also ends once its length has passed on the
  steady clock, so a wall clock step back can't stretch it; the
  stats keep their wall clock times.
- The local-fault warning counts only failures this node's own ML-KEM
  code can cause: ek_modulus, length, tag, and closed_after_switch
  with eof or reset. ek_length, not_decoy and timeout are the peer's
  or the path's.
- getnetworkinfo counts connections_pq from the connection manager's
  node list, without CopyStats() and a cs_main lock per peer. Every
  node in the list is one getpeerinfo lists, so the result is the
  same.
- A backward step of the mocked steady clock restarts load
  shedding's current second and quiet run, so shedding can't latch.
- net_pq_tests restores CreateSock with an RAII guard in pq_config.
- The rpc fuzz target mocks the steady clock too, as
  getpqtransportinfo now reads it.
- The specification's downgrade note, the operator guide (fallback,
  local_fault, abandoned), the RPC delta reference and the release
  note say what the code does. The fallback functional tests use
  automatic connections.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@djh58

djh58 commented Oct 23, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-23T18:22:43.733667Z 713b443 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: 713b4436c5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@djh58
djh58 merged commit f376093 into feature/pq-transport Oct 23, 2026
8 checks passed
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.

1 participant