Repository navigation
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dd57fcf73
ℹ️ 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".
| # main's maintained-branch caller runs this copy on its schedule; the | ||
| # inputs mirror workflow_dispatch. | ||
| workflow_call: |
There was a problem hiding this comment.
When the new reusable workflow is invoked by the scheduled caller on main, github.event_name is workflow_call, while the github context remains associated with the caller (GitHub documentation). Consequently, the existing resolver selects manual mode and checks out the caller's github.sha from main, rather than resolving 1.x.x, so the advertised maintained-branch nightly jobs validate the wrong source. Pass an explicit source ref/mode from the caller or treat this workflow-call path as maintained-branch validation.
Useful? React with 👍 / 👎.
| if baseline_ratio is None: | ||
| if baseline[1] == 0: | ||
| return Figure(name, UNKNOWN, 0, None, "no baseline: no valid paired samples before the canary start") | ||
| baseline_ratio = baseline[0] / baseline[1] |
There was a problem hiding this comment.
When valid pre-canary samples have connections_in == 0 for the archive but a positive control count, this computes a baseline ratio of zero; any sufficiently covered canary day then reaches the relative-ratio calculation and raises ZeroDivisionError, aborting the release-gate evaluator instead of producing an unknown result. Treat a zero archive baseline as unusable evidence (or explicitly define zero-baseline comparison semantics) before storing the ratio.
Useful? React with 👍 / 👎.
|
@codex review |
3dd57fc to
5690a4f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5690a4fd7a
ℹ️ 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".
| found_triage.append(describe(host, entry, label)) | ||
| if not window.contains(record.get("time", 0)): | ||
| continue | ||
| for item in record.get("fallback_set") or []: |
There was a problem hiding this comment.
When getpqtransportinfo omits fallback_set, the sampler records it as null, but this expression converts that missing evidence to an empty list. If the rings and sample coverage are otherwise valid, Figure 2 therefore passes even though fallback evidence was unavailable, contradicting the evaluator's fail-closed rule that missing evidence must be unknown. Validate that every record contains a list-valued fallback_set and mark the affected interval unknown when it does not.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e9eb66eb2
ℹ️ 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".
| instance = record.get("instance_id") | ||
| since = record.get("since") if isinstance(record.get("since"), int) else time |
There was a problem hiding this comment.
When getpqtransportinfo omits or malforms instance_id, the sampler persists it as null, but this code treats that value as a stable process identity. If the node restarts between samples and its new ring reaches sequence values that do not go backwards, failures lost with the previous process are not detected, allowing Figure 2 to report pass despite incomplete evidence. Mark the affected interval unknown unless every record has a valid instance identity.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22fbdc7020
ℹ️ 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".
| if label is None: | ||
| continue |
There was a problem hiding this comment.
When a ring entry has a missing or malformed endpoint, KnownGood.label() returns None and this branch treats it exactly like a valid unrelated peer. Consequently, otherwise complete evidence containing a fallback, malformed_record, or first_packet_failed entry with incomplete endpoint telemetry can still produce a passing release-gate figure. Distinguish structurally invalid endpoints from valid non-known-good endpoints and mark the affected interval unknown.
Useful? React with 👍 / 👎.
| def by_slot(rows: list[dict[str, str | None]]) -> dict[int, dict[str, str | None]]: | ||
| return {int(int(row["time"]) / interval + 0.5): row for row in rows if row["time"] is not None} | ||
|
|
||
| archive_slots, control_slots = by_slot(archive), by_slot(control) |
There was a problem hiding this comment.
If the operator accidentally supplies the same CSV for --archive and --control, or both files carry the same host label, these maps accept both streams and Figure 4 reports a perfect ratio and can pass at full coverage. That compares a node with itself rather than proving the required independent control baseline; validate that each role has one stable host identity and that archive and control are distinct before evaluating connections.
Useful? React with 👍 / 👎.
| all_tags = sorted([*set([v['tag'] for v in SHA256_SUMS.values()]), *QBIT_RELEASES]) | ||
| parser.add_argument('tags', nargs='*', default=all_tags, |
There was a problem hiding this comment.
On a powerpc64le host, which set_host() explicitly recognizes, running this script without tags now always includes v1.0.0; however, the qbit release table has no powerpc64le archive, so resolve_download() returns None and the default previous-release download aborts. Filter unavailable qbit releases from the implicit default list for the detected host, while retaining an error when an unavailable tag is requested explicitly.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a423bbff2a
ℹ️ 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".
| def skip_test_if_missing_module(self): | ||
| self.skip_if_no_previous_releases() |
There was a problem hiding this comment.
When the previous-releases directory contains any release but not qbit v1.0.0—for example, an existing cache containing only Bitcoin Core releases—skip_if_no_previous_releases() does not skip, because --previous-releases defaults true for any nonempty directory. setup_nodes() then unconditionally requests version 10000 and aborts on the missing qbitd/qbit-cli, breaking ordinary functional-suite runs. Check for the required v1.0.0 binaries here before enabling the test, as tool_pq_canary.py already attempts to do.
Useful? React with 👍 / 👎.
| rows: list[tuple[int, Row | None]] = [(time, row) for time, row in sample_rows(pool) if window.contains(time)] | ||
| previous = window.start - interval | ||
| for time, row in rows + [(window.end + interval, None)]: |
There was a problem hiding this comment.
If samples.csv contains duplicate rows for a timestamp—such as from multiple sampler timers or concatenated copies—every duplicate is counted as another usable sample, while missing slots are counted only once. This can inflate coverage above --min-coverage, turn insufficient evidence into a fail, and with enough duplicated clean rows even satisfy the 99% pass threshold, contrary to the documented definition based on expected five-minute samples. Reject duplicate slots or collapse each slot to one sample before calculating good, bad, and coverage.
Useful? React with 👍 / 👎.
Add test_framework/crypto/mlkem.py, a pure-Python FIPS 203 ML-KEM-1024 written from the standard and independent of the C library: KeyGen from a 64-byte d || z seed, Encaps from a 32-byte m, Decaps with implicit rejection, and the section 7.2 (length and modulus) and 7.3 (length and hash) input checks. Hashing uses hashlib's SHA3 and SHAKE only. TestMLKEM1024, registered in feature_framework_unit_tests.py, checks: - NIST ACVP-Server ML-KEM-1024 keyGen and encapsulation vectors (exact ek, dk, ct and shared secret); - ACVP decapsulation vectors, including modified ciphertexts whose expected secret is the implicit-rejection value J(z || ct); - ACVP encapsulationKeyCheck vectors: valid keys pass the section 7.2 check and encapsulate, the others are rejected; - C2SP CCTV modulus vectors (q and 4095 at the first and last coefficient of every polynomial, plus a sample of the full run), each rejected, and accepted once the bad coefficient is set to 3328; - round trips, implicit rejection on corrupted ciphertexts, and input size and hash checks. mlkem1024_test_vectors.txt holds the subset with its sources and pinned commits in a header comment. The new tests take about 0.07 s; the whole feature_framework_unit_tests.py run about 4 s. Part of #184 (part 2, #186, work item B2). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Let functional tests download and run qbit release binaries the way they run Bitcoin Core releases, starting with v1.0.0. get_previous_releases.py resolves each tag through resolve_download(). qbit tags come from explicit descriptors: the GitHub release URL, the archive per host (the "-unsigned" tarballs on macOS, whose -unsigned.zip holds only the GUI app, and the "-unsigned" zip on win64), the archive root and each archive's SHA256 copied from the release's SHA256SUMS. Unsigned arm64 macOS binaries are self-signed as old Core ones are. A host without a qbit archive fails with a message instead of a guessed download. Bitcoin Core URLs, archives, hashes and messages are unchanged, and qbit tags join the default download list, so the previous-releases and Windows jobs fetch v1.0.0. The test framework maps client versions through explicit ReleaseInfo metadata (test_node.py): 10000 is qbit v1.0.0, based on Core v30.2 (core_compat_version 300200), with qbitd/qbit-cli binary names. A version from 140000 (Core v0.14) up is the Core release it names, as before; any lower version not in QBIT_RELEASES is an unknown qbit release and raises, instead of being read as a Core version. version_is_at_least() compares core_compat_version, so v1.0.0 nodes get -v2transport and the logging options, and qbit_version_is_at_least() gates qbit-only options: -p2mronly=0 now reaches qbit releases that have it, and supports_v2_pq marks which nodes may get -v2pqtransport. tool_qbit_release_resolution.py (qbit_release_resolution) checks v1.0.0 on every published host triple including win64 against a verbatim copy of SHA256SUMS, the Core mappings, the rejection of unknown qbit versions, and the options a v1.0.0, Core and current node get. It needs no downloads, so it runs in every functional job, Linux and Windows. Part of #184 (part 2, #186, work item B1). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add p2p_v2_previous_release.py, which pins how the qbit v1.0.0 release binary treats non-empty BIP324 version-packet contents, where the hybrid post-quantum handshake (#184) will carry its ML-KEM-1024 key and ciphertext. It drives a BIP324 peer on a plain socket, so it controls every byte and pause, against v1.0.0 in both roles: - test_version_contents: with 0, 1,572 and 65,000 bytes of contents, and with record-shaped contents (CompactSize(len) || header || payload): one 0xF0 record exactly as the hybrid offer and accept carry it (fd 21 06 f0 || 1,568 bytes), a 0xF0 record followed by an unknown 0xF1 record, and a single 65,000-byte record, each sent after three decoy packets, both immediately and after a 10 s pause under the default 60 s -peertimeout, v1.0.0 completes the handshake, exchanges version/verack and ping/pong, sends empty contents itself, and reports the plain BIP324 session id. As initiator it sends its version packet and VERSION message before receiving ours. - test_limits: v1.0.0 keeps a connection whose packet length is exactly MAX_CONTENTS_LEN (4,000,013), drops one at 4,000,014 bytes at once ("packet too large"), and drops a handshake stalled after the keys at -peertimeout ("V2 handshake timeout"), within a bounded time. The test skips without previous releases and runs in the previous-releases job, which downloads v1.0.0 since the previous commit. It takes about 24 s. Part of #184 (part 2, #186, work item B3). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add .github/workflows/ci-pq.yml, one reusable workflow for the post-quantum transport jobs, and make ci-nightly-heavy.yml callable and job-selectable, so main's callers (C4), on a schedule and on demand, can run either against a recorded commit. GitHub dispatches only workflows whose file is on the default branch, so on-demand PQ runs go through C4's caller on main (workflow_dispatch with source_ref and jobs, calling ci-pq.yml@1.x.x), available after this part merges; ci-pq.yml keeps its own workflow_dispatch trigger for when it is on main. ci-nightly-heavy.yml is already on main, so it can also be dispatched on a branch directly. ci-pq.yml runs on workflow_call and workflow_dispatch with two inputs: - source_ref: resolved once by the resolve-source job to a full commit (empty: the triggering commit; a branch or tag, looked up on the remote with annotated tags peeled; or a full hash). A missing, ambiguous or malformed ref fails the run. Every job checks out that commit, verifies HEAD against it, and the summaries record it. - jobs: a comma-separated allowlist; empty runs all jobs, and an unknown, repeated or empty name fails the run. The resolve-source job emits the matrix, since a job-level if cannot see matrix values. Its first job, aarch64-unit, runs on GitHub's ubuntu-24.04-arm runner with the public-runner environment (docker.io/library images, CI_ENFORCE_INTERNAL_REGISTRY=0, public DNS, no tailnet search domains), builds only test_bitcoin and runs bip324_tests and net_tests (ci/test/00_setup_env_native_aarch64_pq.sh). Its summary shows the source commit, labels this the pre-library phase, and lists both suites with their results. ci/test/03_test_script.sh: - CI_BUILD_TARGET (default all) selects the target of the build and of its verbose retry; GOAL's install/deploy targets apply to full builds; - CTEST_REGEX selects tests by name and is rejected together with CTEST_INCLUDE_RANGE; - ctest runs with --no-tests=error, lists the selected tests first and writes JUnit results; when CTEST_EXPECTED_SUITES is set, ci/checks/ctest_evidence.py fails the job unless every expected suite was selected, executed and passed (ctest itself passes a skipped suite, for example one compiled out). ci-nightly-heavy.yml gains workflow_call inputs mirroring workflow_dispatch (choices become strings, which run-scanners.py validates), an optional LIBBITCOINPQC_READ_TOKEN secret, and a jobs input over arm32-unit-1, arm32-unit-2, previous-releases, fuzz and scanner. The matrix entries are unchanged and now come from resolve-source. A called run gets its caller's github context, so github.event_name, github.ref and github.sha are main's and cannot choose what a called run validates. Only workflow_call takes a source_ref input (default 1.x.x, must not be empty), and a non-empty source_ref marks a run as called: - a called run resolves source_ref once, with the same function as ci-pq.yml (which the contract test keeps identical), never reads github.ref or github.sha, and records source_mode maintained when source_ref names 1.x.x and requested otherwise; it honors run_test_matrix, run_scanners, the scanner settings and jobs whatever its caller's event; - a direct scheduled run still resolves 1.x.x and follows the CI_NIGHTLY_HEAVY_RUN_* variables and scheduled defaults, and a direct dispatch keeps its selected ref and event commit; - a first step fails the run when source_ref disagrees with GitHub's job.workflow_ref, for example a caller that passes an empty source_ref, and skips that check, saying so in its summary, where job.workflow_ref is unavailable; - a called run's concurrency group names its caller's event and source_ref, so callers for different sources, and an on-demand call beside the scheduled one, never cancel each other; - the summaries record the source_ref input and source_mode. ci/checks/test_ci_pq_contract.py executes the workflows' inline steps and the 03_test_script.sh blocks against git fixtures and stub cmake/ctest: job selection, rejection of unknown jobs, an immutable source commit, suites that really ran, and called and direct nightly runs with their concurrency groups. The shared expression evaluator follows GitHub's loose equality and supports format() and toJSON(). test_scheduled_validation_contract.py reads the nightly matrix from its new catalog. Both now run in required CI: the Required Merge Gate's classify-changes job (.github/workflows/required-merge-gate.yml) gains one step that runs them, beside the classifier contract. Part of #184 (part 2, #186, work item C1). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Scripted evidence for the mainnet canary of the hybrid post-quantum v2 transport (#184, R5), ready before the 14-day R0 baseline starts on v1.0.0 nodes. Everything lives in contrib/pq-canary/. pq-canary-sample.sh runs every 5 minutes from cron or a systemd timer on each canary node and the control node. Through qbit-cli it appends one row to samples.csv: time, host label, uptime, version; enabled, instance_id, backends, since, every per-direction handshake counter, load_shedding.active and each ring's last_sequence and dropped from getpqtransportinfo; connections_in and connections_pq; and, on the pool node (--pinned), the pinned archive node's getpeerinfo connection_type, transport_pq and transport_pq_status. sample_ok is 1 only when uptime, getnetworkinfo and, on the pool node, getpeerinfo answered. A failed call or a missing field is NA, never 0, and a real 0 stays 0, so a v1.0.0 node writes NA for the new fields with sample_ok=1. The raw recent_failures and fallback_set go to failures.jsonl, which keeps its addresses between the team's canary nodes. --pinned and --host are validated. pq_canary.py holds the format shared with the evaluator. pq-canary-eval.py prints each figure with its sample count, coverage and pass, fail or unknown: the pinned link manual and hybrid in at least 99% of samples excluding restarts (with the disconnectnode remedy when an automatic connection holds the address); no malformed_record, first_packet_failed or fallback entry between known-good endpoints, on every monitored canary node (one node's failures file cannot pass it); closed_after_switch entries between them, listed for triage; and the archive node's connections_in within +-20% of the control's, relative to their baseline ratio, per UTC day, excluding 48 hours after a restart. A jump in boot time is a restart gap. A ring that wrapped stays judged while the entries read continue from the last sequence processed; only a wrap that evicted unread entries, a changed instance_id or an inconsistent ring makes that ring's interval unknown. A figure, node or day below --min-coverage (default 90% of the expected 5-minute samples) is unknown, not judged, and every figure reports its coverage. Missing evidence is unknown, never clean. The report names known-good peers by label and never prints a peer address, so it can be posted. Unknown, malformed or missing arguments are rejected with a message and exit status 2. Tests: contrib/pq-canary/test/test_pq_canary.py (stdlib only, fixtures in the #184 getpqtransportinfo schema) covers, among others, a monitored outbound failure surviving 300 unrelated inbound failures, unrelated noise, a restart gap, an RPC outage, a wrapped ring with and without lost entries, per-node evidence, coverage thresholds, address redaction and argument validation. The Required Merge Gate's PQ contract step (.github/workflows/required-merge-gate.yml) now runs it as well, and test/functional/tool_pq_canary.py runs the real sampler against the node under test and, when previous releases are present, a v1.0.0 node. The README covers install, the cron lines and how to read the output. Part of #184 (part 2, #186, work item C5). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The evaluator read a missing or malformed fallback_set as an empty list, so a sample without fallback evidence could pass Figure 2. Record it as an unknown interval for that host instead, as for malformed rings, and iterate fallback_set only when it is a list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A missing or malformed instance_id was compared like a real process identity, so a restart between samples whose new ring did not go backwards went unnoticed and lost failures could pass Figure 2. Mark both rings unknown from the previous sample through the next record with a valid 64-hex-digit instance_id. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A ring entry whose endpoint was missing or malformed got no known-good label and was skipped like a valid unrelated peer, so a fallback, malformed_record or first_packet_failed entry without endpoint telemetry could still let the failure figure pass. Check that an endpoint has the getpqtransportinfo Endpoint shape: a kind and network that agree, an address of that network without a port, and a port from 0 to 65535. An entry that counts towards figure 2 or 3 and has neither a known-good nor a well-formed endpoint now makes that figure unknown. A malformed endpoint that still names a known-good peer remains that peer's failure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Giving the same samples.csv for --archive and --control, or two files with the same host label, made figure 4 compare a node with itself: a perfect ratio that could pass at full coverage. The same applied to --pool and --archive, which also set the nodes monitored for figure 2. Require --pool, --archive and --control to be three different nodes. A file given for two roles, two roles sharing a host label or an instance_id, and a file whose rows carry no host label or more than one are bad input: the evaluator exits with status 2 and names the roles. The node test gave one node's samples as both pool and archive; give it the pool samples alone and check the rejection instead. The unit tests' simulated nodes now get distinct instance_ids, as real nodes do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On powerpc64le, a host set_host() recognizes, running get_previous_releases.py without tags now always included v1.0.0, which has no powerpc64le archive, so the default download stopped with "No qbit v1.0.0 archive". Without tags, download every Core tag as before but only the qbit releases that have an archive for the host, and say which were skipped. A tag given explicitly still fails when it has no archive for the host. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Audit every failures.jsonl field the evaluator reads, so that a missing, malformed or contradictory value makes its stretch unknown rather than clean, and never crashes the evaluator into a "fail" exit status: - recent_failures that is not an object, and ring counters or sequences that are booleans, negative or zero, are a malformed ring (a non-object recent_failures raised AttributeError); - an entry outcome outside the getpqtransportinfo vocabulary may be a monitored or a triage one, so both figures are unknown (it was skipped as clean; an object outcome raised TypeError); - an entry time that is missing, malformed, before its process started or more than CLOCK_SLACK after the sample that read it is unknown (it was read as 1970 and skipped, or raised TypeError); - a fallback-set entry that is not an object or lacks a well-formed endpoint is unknown, once per distinct entry (it was skipped); - since that is missing, malformed, after the sample or changed within one instance_id makes the record's process identity unusable, like a missing instance_id (it fell back to the sample time); - a failures.jsonl line needs a time in Unix seconds up to year 9999 and a host label, else it is bad input (a boolean time was accepted, and a huge one crashed the report); - a known-good failure whose reason, cause, connection type, peer id, streak or times are malformed is still a failure, and those fields print as "malformed": a malformed field could put an address in the report, and malformed times or unhashable values raised. Labels are now matched in full, so a host label with a trailing newline is rejected. The unit tests' simulated nodes now report only failures recorded by the time of each sample, and boot before their first sample. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Audit every samples.csv field the evaluator reads, so that a missing, malformed or contradictory value is unknown rather than a pass, a miss or a crash: - time, uptime and connections_in must be plain non-negative decimal integers. int() also took "-30", " 30", "3_0" and non-ASCII digits, so a negative uptime looked like a fresh restart and excluded samples from figures 1 and 4; a time such as "soon" raised ValueError. A row without a usable time is now a missing sample. --canary-start and --end must lie between 1970 and 9999, and so must a sample time, which the report prints. - A row with more or fewer fields than the header, such as a line cut short by a crash and continued by the next sample, has values in the wrong columns, so it is not a sample. - A connected pinned peer needs a connection type, transport_pq 0 or 1 and a transport_pq_status from the getpeerinfo vocabulary, with transport_pq 1 exactly when the status is hybrid. A malformed or contradictory value is unknown: it counted as a miss, and transport_pq=1 with status "pending" counted as hybrid. - Figure 4 pairs only samples with sample_ok=1. - An uptime whose boot time contradicts the one already seen for the same instance_id is unknown: it was read as a restart and excluded samples. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The sampler wrote any integer, string or boolean it found: true became a connections_in or counter of 1, a negative uptime or since was kept, and an instance_id of any shape was recorded. Write each column only when the RPC value has its contract type (counts are non-negative integers, flags booleans, backends and statuses strings, instance_id 64 lowercase hex digits); anything else is NA, which the evaluator reads as unknown. Reject --host NA, which would read back as a missing host label. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nknown Two of the new unknown rules reached further than the evidence: - An uptime was checked against the first boot time seen for its instance_id. A restart between the sampler's uptime and getpqtransportinfo calls pairs the old uptime with the new instance_id once, which made every later uptime of that process unknown. Check each uptime against the median boot time of its instance_id instead, so one racy or malformed value is the only unknown. - An entry with an implausible time made figure 2 or 3 unknown even when the sample that first read it was taken long before the window. Such an entry was recorded before that sample, so it cannot be in the window. Also rename the samples() helper to sample_rows(), which evaluate_failures shadowed with its sample count. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tool_qbit_release_resolution's default-tags case set HOST to make get_previous_releases.py act as a powerpc64le host. set_host() ignores HOST on Windows and answers win64, so on the Windows CI job the case downloaded the v1.0.0 win64 archive and failed. Stand in for set_host() so the case checks main()'s use of the host on any platform. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6317035 to
46cad5d
Compare
p2p_v2_previous_release.py only checked that previous releases were enabled. A releases directory that holds other releases, such as Bitcoin Core's, but not qbit v1.0.0 enables them, so the test went on and setup_nodes() aborted on the missing qbitd and qbit-cli. Skip unless both v1.0.0 binaries are present, as tool_pq_canary.py leaves out its v1.0.0 sample. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Figure 1 counted every samples.csv row in the window, so rows repeated for one sampling slot (overlapping files, a manual run beside cron) added to its total: 95 of 100 expected samples is unknown, but the same rows recorded six times read as 99.1% hybrid and passed. Expected samples are now --interval apart from --canary-start, a sample stands for the nearest one, and every figure counts an expected sample once. Several samples for one count as the least favorable of them, so an extra sample can never improve the evidence: for the pinned link a miss, then unknown, then excluded, then hybrid; for inbound connections unknown, then excluded, then a measurement, where measurements that disagree are unknown (figure 4 kept whichever row came last). Figure 2 counted a record nearest to an expected sample just outside the window as covering one inside it; it now uses the same expected samples. A row repeated exactly is one sample, so repeated rows cannot outvote the median boot time that decides whether an uptime is a restart: one racy uptime copied a hundred times made two misses look like a restart and excluded them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8aea727cf
ℹ️ 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".
| "arm-linux-gnueabihf": ("qbit-1.0.0-arm-linux-gnueabihf.tar.gz", "5607d03b68d804934ae5918f7b8bdf4c6e0da3507e809bc05b97146de13156d7"), | ||
| "arm64-apple-darwin": ("qbit-1.0.0-arm64-apple-darwin-unsigned.tar.gz", "a98310136b57f49471bb19b7bbba1ccf3801da9a1d64202cf7dcf6756db0b3a1"), | ||
| "powerpc64-linux-gnu": ("qbit-1.0.0-powerpc64-linux-gnu.tar.gz", "acfc5098de60029e359fffda6438f29822e12a0ed5504b1a5940409c627f0384"), |
There was a problem hiding this comment.
On 32-bit ARM and big-endian powerpc64 hosts, these newly registered archives cannot be downloaded: set_host() has mappings for AArch64 and powerpc64le, but none for an ARM GNU/Linux triple or powerpc64-*-linux-*, so the command exits with “Not sure which binary to download” before resolve_download() sees either entry. Add host-detection mappings for both published targets and exercise them through set_host() rather than only calling resolve_download() directly.
Useful? React with 👍 / 👎.
| v1_0_0_bin = Path(self.options.previous_releases_path) / release_info(V1_0_0).tag / "bin" | ||
| self.with_v1_0_0 = self.options.prev_releases and v1_0_0_bin.is_dir() |
There was a problem hiding this comment.
When a stale or partially extracted cache contains v1.0.0/bin/ but is missing qbitd or qbit-cli, this directory-only check enables the second node and add_nodes() aborts on the missing executable instead of running the canary test against the current node and skipping the optional v1.0.0 sample. This is possible because the downloader creates the release directory before extraction and can leave it behind on failure; check both resolved binary paths, as the interoperability test does.
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Summary
Part 2 of #184 (#186): the foundations later parts build on. There is no change to node behavior. This is the bottom of the #184 PR stack on
1.x.x. C4, a small scheduler PR onmain, follows after this merges, and completes #186.B2. Independent Python ML-KEM-1024.
test_framework/crypto/mlkem.pyis a pure-Python FIPS 203 implementation with self-tests, written independently of the C library. Its public API is four functions (mlkem1024_keygen,mlkem1024_encaps,mlkem1024_decaps,mlkem1024_check_ek), which later parts' vector generator and Python hybrid peer use. The self-tests cover pinned NIST ACVP keyGen, encapDecap and key-check vectors, C2SP CCTV modulus vectors and implicit rejection.B1. qbit releases in the previous-release tooling. It uses explicit release metadata (
ReleaseInfo). It resolves qbit's GitHub release assets, including the-unsignedmacOS and win64 names, and takes hashes from the release'sSHA256SUMS.version_is_at_leastcomparescore_compat_version(qbit 1.0.x → 300200), and a newqbit_version_is_at_leasthandles qbit-only options. Unknown or too-low qbit versions are rejected. Every Bitcoin Core mapping still resolves as before.B3. v1.0.0 interoperability baseline.
p2p_v2_previous_release.pychecks v1.0.0 in both roles with these version-packet contents:0xF0record;This pins today's behavior before any node change.
C1. Reusable PQ CI workflow.
.github/workflows/ci-pq.yml: asource_refinput resolved once to a SHA, ajobsallowlist and matrix, andaarch64-uniton GitHub's arm64 runner with a public-runner environment.03_test_script.sh:CI_BUILD_TARGETandCTEST_REGEX, with--no-tests=errorand JUnit evidence that every expected suite ran and passed.ci-nightly-heavy.yml:workflow_callwith asource_refinput (default1.x.x) and ajobsinput. A called run validatessource_ref, resolved once, never the caller's ref or commit; it honors the caller's inputs, and fails closed ifsource_refis empty (cross-checked withjob.workflow_ref). Direct scheduled and dispatched runs behave as before. Concurrency groups include the source, so callers for different sources don't cancel each other.ci/checks/, run in the Required Merge Gate's classify step.GitHub only dispatches workflows that exist on the default branch, so on-demand PQ runs will go through C4's caller on
main.C5. Canary tools.
contrib/pq-canary/adds a 5-minute sampler and an evaluator for the mainnet canary in the release gate.NA, never 0, and missing evidence isunknown, never clean.unknown, never divided by.tool_pq_canary.pyruns the real sampler against the node under test, and against v1.0.0 when previous releases are present.Testing
feature_framework_unit_tests.py(ML-KEM self-tests),tool_qbit_release_resolution.py,p2p_v2_previous_release.pyagainst the real v1.0.0 binaries,tool_pq_canary.pyand the canary suite (34 tests);wallet_listtransactions.pythat passed 3 of 3 times on rerun.ci/checkscontract tests, of which 14 of 14 mutations were caught.ci-pq.ymlruns follow C4.Two independent adversarial reviews found no correctness defect in B1–B3; their fixes are folded in.
SHA256SUMS.Target Branch
1.x.x, where the p2p: add hybrid post-quantum key exchange (ML-KEM-1024) to BIP324 v2 transport #184 work lands.Risk / Review Notes
Notes: these are CI and test-framework changes only.
required-merge-gate.ymlgains one fast, stdlib-only step that runs the new contract tests and the canary evaluator tests. Nosrc/file changes.Docs / Process Impact
contrib/pq-canary/README.mdand the CI contract notes.Refs #186 · Part of #184
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.