Repository navigation
net: turn hybrid v2 transport on by default - #241
Conversation
|
@codex review |
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 them coming! 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". |
549c1a4 to
e30efc0
Compare
Release N ships hybrid v2 transport on by default (#222, R1). DEFAULT_V2_PQ_TRANSPORT is now true: a node with v2 transport, also on by default, offers and accepts hybrid keys unless -v2pqtransport=0, -nov2pqtransport or v2pqtransport=0 in qbit.conf switches it off. The startup warnings need no change: "has no effect because -v2transport=0" fires only for an ON value given explicitly (1, an empty value, a non-numeric value read as 1, or a double negative), never for the default or an explicit 0. rpc_pqtransport.py now checks each case, including a non-numeric value with -v2transport=0, which is read as 0 and must not report the contradiction, and the same settings in qbit.conf. Tests that assumed the old default: - rpc_pqtransport.py starts from the default, switches it off with -v2pqtransport=0 and in qbit.conf, checks precedence both ways, and checks that getpqtransportinfo, the startup line and transport_pq_status agree for each -v2transport (0, 1) and -v2pqtransport (default, 0, 1). - p2p_v2_previous_release.py: a node with the default and v1.0.0 connect both ways, as legacy_peer over v2 and over v1, and v1.0.0 ignores v2pqtransport in qbit.conf but refuses it on its command line, as the operator guide says. The functional suite's run modes keep their meaning, as #212's G3 set them up for this change: the default mode runs v1 between nodes, and tests that start nodes with v2 now get hybrid; plain --v2transport (the i686 job of ci.yml) passes -v2pqtransport=0, and --v2pqtransport (pq-functional) -v2pqtransport=1, explicitly, so both jobs keep their coverage whatever the default, also if the release-cut decision reverts this commit. ci/checks now checks each mode's node arguments, and the help text and comments say what each mode means. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The operator guide, the release note and the Bitcoin Core differences page say the feature is on by default, and how to switch it off. The quick start starts its nodes without -v2pqtransport, so it shows the default, and Switch It On or Off gives v2pqtransport=0, with v2pqtransport=1 as the explicit form. Upgrading and Rolling Back say that an upgrade switches it on; that an operator who wants it off adds v2pqtransport=0 to qbit.conf before upgrading, since v1.0.0 ignores it there but refuses any form of it on its command line; and that a node that never set the option has nothing to remove before a rollback. feature_pq_quickstart.py checks the default, the guide's new qbit.conf line and the command-line overrides, and that the quick start sets no -v2pqtransport. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ransport With hybrid v2 transport on by default, the only whole-suite v2 run on pull requests, the i686 job, is plain v2: a regression that only the hybrid suite catches would surface nightly, through ci-pq.yml's pq-functional job. The classifier gets a pq_functional_required output: true whenever pq_unit_required is, and for the hybrid transport paths the PQ unit jobs neither build nor run: src/init.cpp (the hybrid setting), src/rpc/net.cpp (the hybrid RPC fields), rpc_net.py (whose hybrid checks run only in the hybrid suite), the p2p_v2_*.py tests, the test framework (the run modes and the Python peers), and the job's own setup. The macOS and AArch64 unit jobs still start only for pq_unit_required. A new pq-functional job of Full Validation calls ci-pq.yml with only pq-functional on the change itself, on its GitHub-hosted ubuntu-24.04 runner, when that output is true and the pull request is not a draft. The Full Validation Gate needs it and treats it as it treats pq-unit, now through one function: it must succeed when required, may be skipped when not, and a missing or other classifier output fails the gate. The Required Merge Gate waits for the Full Validation Gate, so it needs no change. test_pq_merge_gate_contract.py checks both jobs: the call and what ci-pq.yml then runs, each job's condition, the gate's needs, env and outcomes, and changed files traced through classify-changes' outputs to each job's condition and the gate; and that every path the classifier names exists. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e30efc0 to
c3501bf
Compare
|
@codex review |
|
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". |
Part of #222 (R1). Stacked on #240; merges just before the release candidate is cut.
Release N ships hybrid v2 transport on by default. This PR makes that change and nothing else: the load-shedding threshold stays as it is (R3 decides it).
What changes
DEFAULT_V2_PQ_TRANSPORTis nowtrue. A node with v2 transport, also on by default, offers and accepts hybrid keys with every upgraded peer, unless-v2pqtransport=0,-nov2pqtransportorv2pqtransport=0inqbit.confswitches it off. The setting reachesg_pq_transport_configthroughGetSetting, and from there the startup warnings, the startup line, every connection andgetpqtransportinfo; nothing else reads it.-v2pqtransport=truenow switches the feature off against the default, and says so.-v2pqtransport, so it shows the default. Upgrading and Rolling Back say that an upgrade switches it on, and that an operator who wants it off addsv2pqtransport=0toqbit.confbefore upgrading: v1.0.0 ignores the line there, but refuses any form of the option on its command line. The spec and the RPC and option help text don't state the default, so they don't change.Tests
rpc_pqtransport.pystarts from the default, switches the feature off with-v2pqtransport=0and inqbit.conf, and checks precedence both ways. It checks each warning case, warns or doesn't, on the command line and inqbit.conf, and thatgetpqtransportinfo'sstatusandenabled, the startup line andtransport_pq_statuson both sides of a connection agree for each-v2transport(0, 1) and-v2pqtransport(default, 0, 1).p2p_v2_previous_release.py: a node with the default and v1.0.0 connect both ways, aslegacy_peerover v2 with the same plain BIP324 session id on both sides, and over v1. v1.0.0 ignoresv2pqtransport=0or=1inqbit.confand refuses-v2pqtransport=0,-nov2pqtransportand-v2pqtransport=1on its command line, as the guide says.feature_pq_quickstart.pychecks the default, the guide's newqbit.confline and the command-line overrides, and that the quick start sets no-v2pqtransport.rpc_net.py,p2p_v2_misbehaving.pyand thecontrib/pq-canarytools needed no change: they read the run mode or the node's own state, not the default.Functional suite modes and CI
The run modes keep their meaning, as #212's G3 set them up for this change:
-v2transport=0ci.ymljobs--v2transport-v2transport=1 -v2pqtransport=0ci.ymlci-matrix "no IPC, i686, DEBUG"--v2pqtransport-v2transport=1 -v2pqtransport=1ci-pq.ymlpq-functional: nightly, and now on pull requests (below)Both explicit values make each job keep its coverage whatever the default, also if the release-cut decision (R7) reverts this PR.
ci/checks/test_ci_pq_contract.pynow checks the node arguments the framework passes in each mode, and the help text and comments say what each mode means.The hybrid suite on pull requests (
ci: run the hybrid functional suite…)With hybrid on by default, the only whole-suite v2 run on pull requests, the i686 job, is plain v2, so a hybrid-only regression would surface only nightly. Now:
Classifier.
ci/checks/classify_merge_profile.pyreportspq_functional_required. It is true wheneverpq_unit_requiredis, and also for these hybrid paths that the PQ unit jobs don't cover:src/init.cpp, the hybrid setting;src/rpc/net.cpp, the hybrid RPC fields;test/functional/rpc_net.py, whose hybrid checks run only in the hybrid suite;test/functional/p2p_v2_*;test/functional/test_framework/, the run modes and Python peers;ci/test/00_setup_env_native_pq_functional.sh.The macOS and AArch64 unit jobs still start only for
pq_unit_required.Job. A new Full Validation job,
PQ functional tests, callsci-pq.ymlwith onlypq-functional. It runs on the change itself, on GitHub-hostedubuntu-24.04, when that output is true and the PR isn't a draft.Gate. The Full Validation Gate needs the new job and checks it as it checks
pq-unit, now through one function:The Required Merge Gate waits only for the Full Validation Gate, so it needs no change.
Tests.
test_pq_merge_gate_contract.pycovers both jobs:ci-pq.ymlthen runs;needs, env and outcomes;classify-changes' outputs to each job and the gate;This PR touches
src/net.h, so once it is ready for review, its own Full Validation runsPQ functional tests / Functional tests with hybrid transport, and the gate logsPQ functional tests: success.Verification
Run on this change on an earlier #240 head (
814dc84eda). Since then, the parts below gained three test-only fixes: Windows line endings inrpc_pqtransport.py, the test-vector path intest_framework/v2_p2p.py, and the quick start's port substitution. This branch carries them as merged, with one line of this PR'srpc_pqtransport.pychange adjusted to match the first.rpc_pqtransport.py,feature_pq_quickstart.pyand the framework unit tests were rerun on this head.-j 4, default and--v2pqtransport: 297 passed, 20 skipped (environmental), 1 failed in each.wallet_listtransactions.py: itskeypoolrefill(1000)exceeds the 30 s RPC timeout on the loaded test host.--timeout-factor=4.--v2transportand--v1transport, with the same result.p2p_v2_previous_release.pyandtool_pq_canary.pypass in default,--v2transportand--v2pqtransport.net_pq_tests,net_tests,argsman_tests,getarg_tests.ci/checks(157 tests with the CI commit), actionlint 1.7.12,lint-pq-doc-tokens.py,check-doc.py, ruff with the lint runner's rules, codespell.--v2transport's explicit-v2pqtransport=0;-v2transport=0;statusthat ignores-v2transport=0;needs;pq_unit_required;src/init.cppdropped from the classifier;jobsinput, gate check removed, test framework dropped, drafts).PQ functional tests(the hybrid suite), and the previous-releases job.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.