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. |
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit fc3eb7b. Configure here.
|
@codex review |
The review of #236 found nine plausible mutants of the RPC rendering and startup code that every test passed. Each now fails a test. net_pq_tests calls the RPCs (getpqtransportinfo with the fixture's own connection manager): - pq_rpc_status: status names v2 first, so v2 off reads disabled_v2transport whatever the hybrid switch says; enabled follows. - pq_rpc_load_shedding: with a threshold of 1, the second offer in one second is shed: load_shedding is {active: true, threshold_per_s: 1, since: now}, and inbound shed counts it. - pq_rpc_endpoints: IPv4, IPv4-mapped IPv6, IPv6, ::1, onion, I2P and name-proxy endpoints, each with its kind, its own network, its canonical address and its port, in the outbound ring and the failure streaks. - pq_rpc_windows: one endpoint's windows escalate, 3600, 14400, then 86400, in window_seconds (equal to expires - entered) and next_window_seconds, and an expired window is listed nowhere. - pq_rpc_listed_peers: a confirmed hybrid connection the peer manager doesn't know is in getnetworkinfo's connections but not in connections_pq or getpeerinfo; once known, both show it. rpc_pqtransport.py: - a default node runs native ML-KEM-1024 code where the build and CPU have it. The test config gains ENABLE_MLKEM_NATIVE, and the test reads the CPU's flags on Linux x86_64 (the features cpu_features::HasMlkemX86Native() requires), assumes native code on macOS arm64, and expects portable code without native support; it doesn't check where it can't tell; - getpqtransportinfo with -v2transport=0 alone: disabled_v2transport; - a name-proxy destination through a SOCKS5 proxy that resolves it to a failing node, end to end: the ring entry and the streak name {name_proxy, name_proxy, pqtest.example, 8333} for "PQTest.Example.:8333"; - with local IPv6, an IPv6 connection: {address, ipv6, ::1, port} on the outbound side, and ipv6 ::1 on the inbound side. The whitespace case (-v2pqtransport= 1 doesn't warn) is in the previous commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
Make the hybrid post-quantum v2 transport configurable, off by default
until the default-on change in part 9, through one configuration that
the startup warnings, the startup line, every new connection and RPC
share.
- PQTransportConfig {v2_enabled, pq_requested, fail_first_packet} is
computed once in AppInitParameterInteraction() from the effective
-v2transport, -v2pqtransport and -test settings. NODE_P2P_V2 now
comes from the same value. CConnman::Options::m_pq carries it, and
CConnman::Init() derives m_pq_mode from it: NEGOTIATE only with both
switches on. This is the only way the mode is set; the unit-test
hook ConnmanTestMsg::SetPQMode is removed and net_pq_tests passes a
configuration instead.
- -v2pqtransport (DEFAULT_V2_PQ_TRANSPORT = false) has no effect with
-v2transport=0. An explicit -v2pqtransport=1 then warns:
"-v2pqtransport=1 has no effect because -v2transport=0. Set
-v2transport=1 to use hybrid post-quantum v2 transport." An explicit
0 or the default never warns.
- Booleans are read as integers, so -v2pqtransport=true is 0. A value
that doesn't parse completely as an int, under InterpretBool's
whitespace and sign rules, warns and names the value it is read as:
true, 1abc and an overflowing number. An empty value and a negation
are valid. InterpretBool itself is unchanged.
- -test=pq_fail_first_packet (regtest) sets corrupt_shared_secret on
inbound and outbound transports, so every hybrid handshake fails its
key confirmation. -test=mlkem_portable (regtest) and the hidden
-mlkemportable (debug category, all chains) force portable ML-KEM
code through mlkem::InitializeRuntime(), called before the
connection manager exists.
- The startup line is always logged and names the active backends:
"v2 pq: enabled arith=<a> keccak=<k>", "v2 pq: disabled
(-v2pqtransport=0) ..." or "v2 pq: disabled (-v2transport=0) ..."
when v2 is off, whatever the PQ switch says.
- The local-fault warning names -mlkemportable as the remedy again
while native code runs: " Restart with -mlkemportable to use the
portable implementation."
- MakeTransport() takes the CNodeOptions.
rpc_pqtransport.py checks the switch from the command line, qbit.conf,
-nov2pqtransport and across restarts on a live two-node connection
("v2 pq: switched"), the exact warnings and their absence, -help and
-help-debug, all three startup lines, portable code from both options,
the test options' regtest-only check, and that the test options print
nothing to stderr. net_pq_tests' pq_config checks the mode of inbound
and outbound connections for each configuration and the effect of
fail_first_packet; the local-fault tests check the remedy appears only
while native code runs.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 19268db)
Give operators and monitoring scripts everything they need to see the hybrid post-quantum v2 transport, using the RPC contract in #184. - getpqtransportinfo renders the configuration and part 5's PQTransportStats field for field: enabled; status (enabled, disabled_v2pqtransport or disabled_v2transport); instance_id (64 lowercase hex); the active arith and keccak backends; since; per direction handshake counters, inbound shed and outbound closed_after_switch and fallback included; load_shedding (since is 0 when inactive); fallback_set with window_seconds; failure_streaks with next_window_seconds; and both failure rings, oldest first. Every field is always present and empty collections are []. An endpoint is {kind, network, address, port}: an address's own network (127.0.0.1 is ipv4), or name_proxy and the normalized hostname. The help text lists the fixed reason vocabulary and the counter semantics. - getpeerinfo gains transport_pq and transport_pq_status (pending, hybrid, legacy_peer, fallback, off or v1), carried by CNodeStats from Transport::Info. The help documents part 4's rule: there is no failed status, so a connection closing on any hybrid failure reports pending until it is removed. transport_protocol_type keeps its values, and session_id is the hybrid session id once transport_pq is true. - getnetworkinfo gains connections_pq. getpeerinfo's visibility filter (a peer without node state is skipped) moves into GetListedPeers(), and connections_pq counts transport_pq over exactly those peers, before cs_main is taken. - getpqtransportinfo joins the rpc fuzz target's safe list, and doc/rpc/annotations/v30.2-delta.yml describes the three methods. - qbit-cli -netinfo shows 2p in the v column for a hybrid peer, and its legend explains it. A server without transport_pq, such as v1.0.0, counts as not hybrid. - The GUI's peer details read "v2 (hybrid post-quantum)" for a hybrid peer, and the transport type tooltip explains it. rpc_pqtransport.py gains the RPC cases: every getpqtransportinfo field with the switch off, with v2 off, on, and with an endpoint in the fallback set, and a new instance_id per restart; each transport_pq_status on both sides of a connection; exact per-direction counts for hybrid, legacy and failing connections, with -test=pq_fail_first_packet failing handshakes in both directions; an abandoned offer after eof and after a handshake timeout, with no switch; 300 inbound failures leaving 256 entries with contiguous sequences, 44 dropped, and the outbound ring's entry kept; and connections_pq equal to getpeerinfo's transport_pq count, 0 with the switch off. rpc_net.py's exact getpeerinfo dict gains both fields, interface_bitcoin_cli.py checks 2p and 2 with one hybrid and one legacy peer and the legend, and p2p_v2_previous_release.py runs this release's qbit-cli -netinfo against v1.0.0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit fc3eb7b)
ArgsManager's boolean parsing stays as it is (it matches Bitcoin Core), but -v2pqtransport and -mlkemportable now warn whenever the text given doesn't say what the setting is read as, naming what it was read as and the effect. - A non-numeric value is read by InterpretBool's integer rules: true, yes and on as 0, 1abc or an overflowing number as 1. It warns with the value given: "-v2pqtransport=true is not a number and was read as -v2pqtransport=0: hybrid transport is OFF. Use -v2pqtransport=1 or -v2pqtransport=0." Surrounding whitespace and a sign are allowed, as before. - A double negative, -nov2pqtransport given 0 or any non-numeric value (on the command line or in qbit.conf), switches the option on. ArgsManager keeps only the result of a negation, a bool, so the text is gone; the warning says what it can: "-nov2pqtransport was given 0 or a non-numeric value, so it was read as -v2pqtransport=1: hybrid transport is ON. Use -v2pqtransport=0 to switch it off." -nov2pqtransport=1abc is read as off, like -nov2pqtransport, and can't be told apart, so it doesn't warn. - The effect is the real one: hybrid transport is ON only with -v2transport on too, and the contradiction warning still follows. - -mlkemportable gets the same two warnings, so -mlkemportable=true, which keeps native code, no longer passes silently. Its effect names the code that runs, -test=mlkem_portable included. The setting is now read once, in AppInitParameterInteraction(), and AppInitMain() passes the stored result to mlkem::InitializeRuntime(). - The -v2pqtransport and -mlkemportable help, and getpqtransportinfo's help, say that such values warn at startup. rpc_pqtransport.py checks the exact warnings and startup states for -v2pqtransport=true, yes, on, 1abc and an overflow, none for " 1", +1 and -1, both warnings with -v2transport=0; -nov2pqtransport=0, true and on (and nov2pqtransport=true in qbit.conf) warning and enabled; no warning for -nov2pqtransport, =1 and =1abc; and, for -mlkemportable, =true warning while the default code stays, =1abc, -nomlkemportable=0 and =true warning with portable code, no warning for -nomlkemportable, =1 and -mlkemportable=0, and the test option's effect. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 8ae2a47)
The review of #236 found nine plausible mutants of the RPC rendering and startup code that every test passed. Each now fails a test. net_pq_tests calls the RPCs (getpqtransportinfo with the fixture's own connection manager): - pq_rpc_status: status names v2 first, so v2 off reads disabled_v2transport whatever the hybrid switch says; enabled follows. - pq_rpc_load_shedding: with a threshold of 1, the second offer in one second is shed: load_shedding is {active: true, threshold_per_s: 1, since: now}, and inbound shed counts it. - pq_rpc_endpoints: IPv4, IPv4-mapped IPv6, IPv6, ::1, onion, I2P and name-proxy endpoints, each with its kind, its own network, its canonical address and its port, in the outbound ring and the failure streaks. - pq_rpc_windows: one endpoint's windows escalate, 3600, 14400, then 86400, in window_seconds (equal to expires - entered) and next_window_seconds, and an expired window is listed nowhere. - pq_rpc_listed_peers: a confirmed hybrid connection the peer manager doesn't know is in getnetworkinfo's connections but not in connections_pq or getpeerinfo; once known, both show it. rpc_pqtransport.py: - a default node runs native ML-KEM-1024 code where the build and CPU have it. The test config gains ENABLE_MLKEM_NATIVE, and the test reads the CPU's flags on Linux x86_64 (the features cpu_features::HasMlkemX86Native() requires), assumes native code on macOS arm64, and expects portable code without native support; it doesn't check where it can't tell; - getpqtransportinfo with -v2transport=0 alone: disabled_v2transport; - a name-proxy destination through a SOCKS5 proxy that resolves it to a failing node, end to end: the ring entry and the streak name {name_proxy, name_proxy, pqtest.example, 8333} for "PQTest.Example.:8333"; - with local IPv6, an IPv6 connection: {address, ipv6, ::1, port} on the outbound side, and ipv6 ::1 on the inbound side. The whitespace case (-v2pqtransport= 1 doesn't warn) is in the previous commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 7e0f826)
test/CMakeLists.txt now passes ENABLE_MLKEM_NATIVE to the functional test config, so the contract test that every file mentioning ML-KEM is classified as PQ-relevant failed. Classify it with the other build wiring. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 9e04cfb)
070323f to
f267c54
Compare
Summary
Part 6 of #184. Closes #209. Stacked on #235.
Operator controls and visibility for the hybrid transport. It stays off by default:
-v2pqtransportdefaults to off until part 9 (#222).-v2pqtransport. One immutablePQTransportConfig, computed during parameter interaction, feeds bothNODE_P2P_V2and the connection manager's mode. Part 5's test-onlySetPQModeis removed.-mlkemportable, the field escape hatch that forces portable ML-KEM C, and two regtest-only test options.-mlkemportablewhen native code is active.getpqtransportinfo, following p2p: add hybrid post-quantum key exchange (ML-KEM-1024) to BIP324 v2 transport #184's schema: handshake counters per direction, load shedding,fallback_set, failure streaks, recent-failure rings,instance_id.getpeerinfogainstransport_pqandtransport_pq_status:pending,hybrid,legacy_peer,fallback,offorv1. A connection that failed PQ negotiation reportspendinguntil it is removed, and the help says so.getnetworkinfogainsconnections_pq.2pin-netinfo, the GUI peer string, the RPC fuzz safe list and the RPC docs annotations.Testing
qbitd,qbit-cli,test_qbitand the fuzz binary, every commit on its own. Qt is not built here, so the GUI string is CI-only.rpc_pqtransport.pyandp2p_v2_previous_release.pyagainst real v1.0.0;rpcfuzz target;-Wthread-safety.Target Branch
feature/pq-transport(Hybrid post-quantum v2 transport (#184) — integration #233), stacked on net: hybrid handshake outcomes, 3-strike fallback and load shedding (#184 part 5) #235.Risk / Review Notes
Notes: this adds a public RPC and config surface. No consensus change. With the option off (the default), behavior is unchanged.
Closes #209 · Part of #184
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.