Repository navigation
Conversation
|
@codex review |
|
bugbot run |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c0f4a4f20
ℹ️ 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".
| // account the hybrid negotiation's close outcome and wipe its pending secrets | ||
| // before the disconnected pool keeps the node | ||
| ObservePQ(*pnode, /*finalizing=*/true); |
There was a problem hiding this comment.
Finalize PQ state in StopNodes before deletion
This finalization only runs through DisconnectNodes(). During shutdown, StopNodes() instead swaps active nodes out of m_nodes and calls CloseSocketDisconnect() followed immediately by DeleteNode() (src/net.cpp:4301-4307), so an outstanding inbound offer is never recorded as abandoned and m_pq_finalized is never set; its pending decapsulation/ECDH secrets also remain available through NetEventsInterface::FinalizeNode() and are wiped only by transport destruction. Apply the same PQ finalization in the direct shutdown path before deleting each active node (explicitly suppressing telemetry first if shutdown outcomes should not be recorded).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 7c0f4a4. Configure here.
| if (m_pq_history.size() >= PQ_MAX_ENDPOINT_HISTORY) { | ||
| m_pq_history.erase(std::ranges::min_element(m_pq_history, {}, [](const auto& entry) { return entry.second.insertion_order; })); | ||
| } | ||
| it = m_pq_history.emplace(endpoint, PQEndpointHistory{.insertion_order = m_pq_history_insertions++}).first; |
There was a problem hiding this comment.
Fallback history eviction drops active windows
Medium Severity
When m_pq_history is full, the oldest insertion is evicted even if that endpoint is still inside an active fallback window. Expired streak-0 entries stay in the map to remember escalation, so a later one-strike failure can drop an in-window fallback. IsPQFallback then returns false and the next outbound uses hybrid again, resetting both the window and completed_windows.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7c0f4a4. Configure here.
Three guards had no test, and each survived a mutation in the deep review of PR #235: - pq_shed_wipes_secrets: a shed responder wipes the retained ECDH secret and transcript at once (ClearHybridSecrets() in the shed branch), not only at finalization, since it runs plain v2 for the whole connection. - pq_inbound_success_does_not_protect: only outbound successes enter the success set, which is never evicted. An inbound peer's key is its address and source port, so an inbound success must neither protect an endpoint nor silence the local-fault warning. - pq_outbound_never_abandoned: only responders offer, so an outbound connection closed before the peer's version, by either side, counts no abandoned and adds no outbound ring entry. Follow-up to work items F2-F4 of #206 (#184 part 5). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
The distinct failing endpoints that the local-fault warning counts are never evicted, so the cap at PQ_LOCAL_FAULT_THRESHOLD is their only bound. No test saw it: the warning fires once either way, and an uncapped set survived mutation in the deep review of PR #235 (finding 5). ConnmanTestMsg::PQLocalFaultEndpoints() reads the set's size, test only. pq_local_fault_warning checks that the set holds exactly the threshold once the warning fires, and still does after 12 distinct endpoints have failed. Follow-up to work item F3 of #206 (#184 part 5). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add NodeCloseCause and CNode::RequestDisconnect(cause). It records the cause with a compare-exchange from NONE before setting fDisconnect, so the first cause wins and later or concurrent calls, including cleanup, cannot relabel it. CloseSocketDisconnect(cause) goes through it. The socket sites record PEER_EOF, PEER_RESET and SEND_ERROR, InactivityCheck() and the ping timeout record TIMEOUT, and every other writer in net.cpp and net_processing.cpp records LOCAL by calling RequestDisconnect() instead of setting fDisconnect directly. A node that reaches DisconnectNodes() with no cause is finalized there as LOCAL. fDisconnect stays for its readers, and no protocol behavior changes. The close_cause_first_wins test drives each site through CConnman and PeerManager: EOF, reset, send error, inactivity, receive-parse failure, operator disconnect, network off, eviction, shutdown and the ping timeout. It also checks that repeated and racing calls keep the first cause and that no node is deleted without one. Work item F1 of #206 (#184 part 5). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Count every hybrid negotiation outcome exactly once, per direction, and keep the recent non-success outcomes as evidence. This is the data source for the fallback (F3), load shedding (F4) and the RPC (F6). CConnman::ObservePQ() snapshots the transport under its own lock, releases it, and only then takes m_pq_mutex; nothing calls into the transport, socket, peer manager or node enumeration while holding it. It runs after ReceiveMsgBytes() and once more, finalizing, in DisconnectNodes() before the disconnected pool keeps the node. Each CNode records the events already accounted, so an event seen after receive, at disconnect and in cleanup counts once. Finalization derives closed_after_switch (outbound, switched but unconfirmed, closed by EOF, reset or timeout) and abandoned (an offer queued, no authenticated version) from the F1 close cause, marks the node final and wipes the transport's pending secrets. With the network off it wipes but records nothing, since every close is then our own. - Transport gains GetPQSnapshot() and ClearPQSecrets(), with no-op defaults for V1Transport. The snapshot gains the legacy reason (no_features or parse_error) and the transport's own reason for an abandoned offer (garbage_terminator, version_length, version_tag), set at V2Transport's existing failure sites. - CNode keeps a typed PQEndpointKey captured at creation through CNodeOptions::pq_endpoint: the resolved CService with its port, the normalized hostname and port of a name-proxy destination (never the proxy's address), or an inbound peer's remote address and source port. CNodeOptions::pq carries the negotiation mode to the transport; CConnman's mode stays off until part 6 adds the option. - Counters count events: switched at key installation, even if the confirmation fails; each failure once; abandoned, closed_after_switch and the shed placeholder per direction. - Two rings of at most 256 entries, inbound and outbound (manual included), each with its own sequence and dropped count, so inbound noise can't evict outbound evidence. Reasons come from the fixed vocabulary only. - instance_id and since identify the process's counting period, and GetPQTransportStats() returns an owning copy. - One "v2 pq:" line per outcome, with fixed fields. Outbound failures log at info level, each from its own call site; inbound outcomes, switched, legacy_peer and abandoned are net debug, from separate call sites so inbound noise can't rate-limit outbound lines. net_pq_tests covers once-only observation (including racing observers), endpoint identity, the ring bounds, switched-then-failed, and every abandoned and closed_after_switch cause; a confirmed handshake that the peer later closes, or that times out, counts nothing. It checks the exact text and level of each outcome's log line in both directions (info for outbound failures, net debug otherwise), and the status each failure reports (off for a local fault before commit, pending while a failed connection closes). pq_cipher_state_closes checks that an inconsistent cipher state closes in both roles, the initiator before it commits included, and counts once as internal_error/cipher_state. pq_finalization_wipes checks, for every close cause (EOF, reset, timeout, local, send error, network off, transport failure) and both roles, that the pending secrets survive the socket close and are gone once the connection is finalized, before the node is deleted. pq_name_proxy_endpoint drives ConnectNode() through a name proxy (a mocked SOCKS5 server) and checks that the endpoint key is the normalized destination and port, never the proxy. ProxySettingsRestorerForTesting, a test-only netbase guard, saves and restores the proxy settings so that no later test sees them. Production construction stays PQ-off. Work item F2 of #206 (#184 part 5). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keep upgraded nodes connected when the hybrid negotiation with one endpoint keeps failing, without letting an outsider downgrade connections cheaply: a single failure is telemetry only. - Counted failures are outbound only, per endpoint key: malformed_record, first_packet_failed, and closed_after_switch (eof, reset or timeout). Our own closes, send errors, legacy outcomes and inbound failures are neutral: they neither advance nor reset a streak. - The third consecutive counted failure puts the endpoint in the fallback set for 1 h, then 4 h, then 24 h for each later entry (PQ_FALLBACK_WINDOWS). At expiry the streak restarts at 0 and the escalation stays. A failure from a connection already running during a window neither extends it nor enters again. - A confirmed hybrid handshake moves the endpoint into a separate success set, never evicted until restart, and drops its history. The check and the insert share m_pq_mutex with the counters and rings, so a late failure can't reinsert a protected endpoint. - The history holds at most 1,000 endpoints, evicting the oldest insertion first; insertion orders are unique, so equal times can't tie. It is memory only and uses the mockable NodeClock. - ConnectNode() asks IsPQFallback(endpoint, now): a fallback connection runs PQMode::FALLBACK, plain v2 that neither parses nor validates the offer. No retry loop is added: the next attempt the connection logic makes asks again, existing connections never renegotiate, and inbound connections never fall back. - Entering counts the outbound fallback counter once, adds a ring entry and logs one warning, "v2 pq: fallback cause=<trigger> until=<UTC time>" with the usual fields. - When 8 distinct endpoints have counted failures and none has succeeded since startup, it warns once that this node's own ML-KEM code may be at fault, naming the active backends. It names no remedy: this tree has no option that forces portable code yet. - GetPQTransportStats() gains the fallback set and the failure streaks for part 6. net_pq_tests covers the threshold for each counted variant with neutral closes in between, the escalating windows under mock time (including new connections getting FALLBACK and existing ones not), success protection against a late failure and 1,001 endpoints of churn, oldest-insertion eviction with tied times and insertion order opposite to key order, the local-fault warning (once, naming the backends, native or portable; never after a success), and a fallback connection completing as plain v2 against a malformed offer. pq_closed_after_switch now uses an endpoint per connection, so that its three counted failures don't enter fallback, and pq_cipher_state_closes checks that a local fault never counts toward the fallback. Work item F3 of #206 (#184 part 5). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bound the extra CPU an inbound connection flood can cost. Each offer
costs the responder one ML-KEM-1024 key generation; above a rate
threshold, responders stop offering and cost exactly what plain v2
costs today.
- V2PQOptions gains an offer gate (a function pointer and context, like
PQRandomSource). A negotiating responder calls it at its key
generation point, once the initiator's 64-byte key has arrived; when
it declines, the responder wipes the retained secret and sends empty
version contents, and its status is off.
- CConnman's gate counts the attempted offers per second. More than
threshold_per_s in the current second starts shedding; ten
consecutive complete seconds with fewer than half the threshold's
attempts stop it. The quiet seconds before a flood don't count
toward its end, so shedding can't flap within a second. The seconds
come from the steady clock (MockableSteadyClock), so a wall clock
step can't merge seconds or stretch a period; only the reported start
time is wall-clock time. The socket handler also checks the clock, so
shedding stops on time when the flood stops entirely.
- Each shed offer increments the inbound shed counter and adds no ring
entry, so a flood can't wash out the evidence. Starting and stopping
each log one info line: "v2 pq: load_shedding started
threshold_per_s=<n>" and "v2 pq: load_shedding stopped".
- The threshold defaults to DEFAULT_PQ_SHED_THRESHOLD_PER_S (1,000)
and CConnman::Options::pq_shed_threshold_per_s overrides it for unit
tests and the flood lab; there is no new user-facing option.
- GetPQTransportStats() gains load_shedding {active, threshold_per_s,
since} for part 6.
- Outbound connections are never affected. The gate's mutex is taken
under the transport's locks, and nothing holding it calls into a
transport or takes m_pq_mutex.
net_pq_tests drives inbound connections through the accept path under
mock time: offers up to the threshold, the first one above it shed
(empty contents, so the initiator sees a legacy responder; status off;
shed +1), outbound connections still hybrid, a second at half the
threshold restarting the quiet run, offers resuming only after ten
quiet seconds, and each transition logged once at info level. A wall
clock step back by an hour under a trickle of offers sheds nothing,
and a step forward doesn't end a flood early. The real socket handler
thread, run as Start() runs it, ends a flood that stopped. With the
switch off the gate is never asked.
Work item F4 of #206 (#184 part 5).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Three guards had no test, and each survived a mutation in the deep review of PR #235: - pq_shed_wipes_secrets: a shed responder wipes the retained ECDH secret and transcript at once (ClearHybridSecrets() in the shed branch), not only at finalization, since it runs plain v2 for the whole connection. - pq_inbound_success_does_not_protect: only outbound successes enter the success set, which is never evicted. An inbound peer's key is its address and source port, so an inbound success must neither protect an endpoint nor silence the local-fault warning. - pq_outbound_never_abandoned: only responders offer, so an outbound connection closed before the peer's version, by either side, counts no abandoned and adds no outbound ring entry. Follow-up to work items F2-F4 of #206 (#184 part 5). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Three tests changed process-wide state and restored it only at their end: pq_endpoint_identity, pq_name_proxy_endpoint and pq_fallback_windows replaced CreateSock, pq_name_proxy_endpoint also cleared the SOCKS5 interrupt, and pq_log_lines set fLogIPs. A failed BOOST_REQUIRE in between throws past the restore. Every later suite in the same test_qbit process would then connect through a mock socket or log differently, and fail far from the cause. A GlobalOverride guard, a MockCreateSock() helper built on it, and a Socks5InterruptCleared guard now restore them on every path, as ProxySettingsRestorerForTesting already does for the proxy settings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A responder that sheds its offer under an inbound flood runs plain v2 and reports off (#206 F4), but the status list in doc/design/pq-transport.md named only the switch and local faults. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Part 4 now wipes the hybrid secrets whenever the peer's version is processed (#230, L5). pq_shed_wipes_secrets already checks right after the initiator's key, before any version, so a shed branch that doesn't wipe is still caught. With the merged part 4, a variant of the test that checks after the version lets that mutant survive. Make the precondition explicit, so the check can't drift past the version. Follow-up to work item F4 of #206 (#184 part 5). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The distinct failing endpoints that the local-fault warning counts are never evicted, so the cap at PQ_LOCAL_FAULT_THRESHOLD is their only bound. No test saw it: the warning fires once either way, and an uncapped set survived mutation in the deep review of PR #235 (finding 5). ConnmanTestMsg::PQLocalFaultEndpoints() reads the set's size, test only. pq_local_fault_warning checks that the set holds exactly the threshold once the warning fires, and still does after 12 distinct endpoints have failed. Follow-up to work item F3 of #206 (#184 part 5). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MSVC requires an accessible copy constructor for `cond ? a : b` with prvalue operands, and V2Transport has none, so test_bitcoin failed to build on the Windows CI job (C2280). Return the transport from a lambda instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Finalization ran only in DisconnectNodes(). StopNodes() swaps the nodes out of m_nodes and deletes them itself, so a node still held at shutdown was never marked finalized, and its pending decapsulation key and retained ECDH secret stayed until the transport's destructor (PR #235 review). StopNodes() now runs the same finalization, ObservePQ(finalizing), before deleting each node: it marks the node final and wipes the pending secrets. A shutdown close is our own decision, so like the network-off path it records nothing: no abandoned offer, ring entry or log line. ObservePQ() gains a record_outcomes flag for that; the nodes in the disconnected pool were already finalized by DisconnectNodes(). pq_shutdown_finalizes stops a connection manager that holds an inbound connection with its offer pending, an outbound one awaiting the version, and a hybrid one. At deletion no node is unfinalized or holds a hybrid secret, and no counter or ring changes. Follow-up to work item F2 of #206 (#184 part 5). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
61a4f5a to
9d59cfb
Compare
net_pq_tests failed on the Windows CI job: no handshake completed, offers were abandoned with reason reset instead of eof, and the rings stayed empty. The log shows the cause: "socket recv error, disconnecting peer=0: The operation completed successfully. (0)". DynSock::Pipe::GetBytes() signals an empty pipe like a non-blocking recv(2) with no data: it returns -1 and sets errno to EAGAIN. The socket handler reads the error with WSAGetLastError(). That is errno on POSIX, but on Windows it is Winsock's own last error, which the mock never set. So Windows saw error 0, treated every idle read as a permanent receive error, and closed the connection as PEER_RESET before the peer's next bytes arrived. Production is right: real sockets set the Winsock error, an orderly close still reads 0 (EOF), and a peer that closes with unread data resets the connection on Windows and Linux alike. GetBytes() now also calls WSASetLastError(WSAEWOULDBLOCK) on Windows. mock_socket_would_block pins it: an empty pipe reads -1 with the would-block error from WSAGetLastError(), EOF reads 0, and a socket handler pass over a connection with nothing to read leaves it open. Making the mock report error 0 on Linux reproduces the Windows failures exactly, and the new test catches it. Four checks dereferenced Streak() or Fallback() without requiring a value; on Windows one read an indeterminate streak. They now BOOST_REQUIRE it first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |


Summary
Part 5 of #184. Closes #206. Stacked on #230, the hybrid handshake.
This tracks how hybrid handshakes end, falls back to plain v2 for an endpoint that keeps failing, and stops offering ML-KEM to inbound peers during a handshake flood. Production construction stays PQ-off: part 6 adds the option, and part 9 turns it on.
instance_idandsince, so a monitor can tell when it missed entries.malformed_record,first_packet_failedandclosed_after_switch(EOF, reset or timeout) count. On the third, the endpoint falls back to plain v2 for 1 h, then 4 h, then 24 h.threshold_per_sinbound offers in the last second (1,000 by default), responders stop offering ML-KEM. They resume after 10 s below half the threshold. Windows are measured on a steady clock, so a wall-clock jump can't trigger or stretch shedding. Outbound connections are unaffected.Testing
test_qbit,qbitdand the fuzz binary, every commit on its own, with no warnings.net_pq_testscases, including the real socket thread running load shedding, and the real name-proxyConnectNodepath through a mocked SOCKS5 server;test_qbitbinary in one process;Assume.-Wthread-safety.An independent adversarial review accepted this with fixes, all folded in here. It ran:
Target Branch
feature/pq-transport, the p2p: add hybrid post-quantum key exchange (ML-KEM-1024) to BIP324 v2 transport #184 integration branch (Hybrid post-quantum v2 transport (#184) — integration #233), stacked on net: hybrid post-quantum v2 handshake with key confirmation (#184 part 4) #230.Risk / Review Notes
Notes:
src/netbase.{h,cpp}: aProxySettingsRestorerForTestingguard, so a unit test can set a name proxy without leaking it into later suites.Docs / Process Impact
Closes #206 · Part of #184
🤖 Generated with Claude Code