Conversation
The IBD perf workflow declared REPLAY_TIMEOUT, NETWORK_HEADERS_TIMEOUT, NETWORK_TIP_TIMEOUT and NETWORK_IBD_EXIT_TIMEOUT as reserved inputs but never passed them to the harness, so a requested timeout was recorded in host.env while every lane ran with the harness defaults. Move the lane command builders into a sourced helper, ci/ibd-perf-lanes.sh, and append each timeout as one --flag=value token whenever its input is nonempty. No numeric rule lives in the shell: a blank input omits the flag so the harness default applies, and any other value reaches the harness verbatim, where argparse rejects malformed values and validate_options rejects values below one. Before any lane runs, preflight_ibd_timeouts validates every nonempty request through the same builder with the framework's --test_methods validate_options switch, so a typo for a disabled lane fails the step instead of being ignored. host.env keeps the four requested values and gains <key>_forwarded and <key>_forwarded_reason markers written by the same helper; effective values come only from the harness reports. ci/checks/test_ibd_timeout_wiring.py executes the helper with bash, captures each argv through a controlled python3 shim, feeds it to the real parser and validate_options, and, when QBIT_IBD_PERF_CONFIGFILE points at a build, runs tiny real lanes and the real preflight against the report fields. It runs build-less in core-checks and with the build in the IBD perf workflow. Closes #158. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The workflow-wiring test only forbade the four exact requested-key echo lines, so a workflow that echoed an extra timeout-named key, or that called write_ibd_timeout_evidence outside the command group redirected into host.env, still passed. Assert instead that the helper is sourced and the evidence writer is called exactly once inside that group, and that no echo line in the host-metadata step carries a timeout-named key. Both mutations now fail the test.
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: 17a054e148
ℹ️ 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".
Check out support scripts at github.workflow_sha in a separate directory. Run those helpers against the selected benchmark sources, record both revisions, and cover a benchmark checkout that lacks the helper scripts.
Pick up the Qt wallet shutdown test rework so CI no longer evaluates the removed wall-clock bound.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Forward the IBD perf workflow's four reserved timeout inputs (
REPLAY_TIMEOUT,NETWORK_HEADERS_TIMEOUT,NETWORK_TIP_TIMEOUT,NETWORK_IBD_EXIT_TIMEOUT) to the harness flags they were reserved for, and make the wiring testable. Backing issue: #158.ci/ibd-perf-lanes.sh, which builds every replay and network command through one function per lane and appends each nonempty timeout as a single--flag=valuetoken. A blank input omits the flag so the harness default (3600/600/1800/600 s) applies. No numeric rule lives in the shell: malformed or empty values are rejected by the harness parser (exit 2) and values below one by itsvalidate_options(exit 1), exactly as when the harness is run by hand.--test_methods validate_optionsswitch, so a typo for a lane that is disabled by the profile is reported instead of silently ignored. Any nonzero exit aborts the lanes step underset -euo pipefailbefore a lane runs.summary/host.envkeeps the four requested strings unchanged and adds<key>_forwarded=true|falseand<key>_forwarded_reason=forwarded|blank|lane-disabled, written by the same helper that builds the commands. Effective values are only ever written by the harness reports (replay_timeout,network_*_timeout); a lane that did not run produces no report and therefore no effective claim.0reaches the harness and is rejected there).ci/checks/test_ibd_timeout_wiring.pyexecutes the real helper with bash under a closed environment, captures each harness argv through a controlled interpreter shim, feeds it to the real harness parser and validation, checks exit-status propagation, and, whenQBIT_IBD_PERF_CONFIGFILEpoints at a built tree, runs tiny real lanes and the real preflight and checks the harness-written report fields. Without a build those cases report as skipped, never as passed. The test runs build-less in core checks and with the gate in the IBD perf workflow next to the existing harness validation step.Testing
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Debug -DBUILD_GUI=OFF -DBUILD_TESTS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=OFF -DENABLE_IPC=OFF -DWITH_ZMQ=OFF -DWITH_USDT=OFF, targetsqbitd qbit-cli).python3 ci/checks/test_ibd_timeout_wiring.py -vbuild-less (6 tests, 2 skipped as designed) and withQBIT_IBD_PERF_CONFIGFILE=build/test/config.ini(6 tests, none skipped; real replay and network lanes report 1234/77/88/99 when requested and 3600/600/1800/600 when omitted);python3 test/functional/feature_ibd_perf_validation.py --configfile=build/test/config.ini; both under Python 3.10.ci/lint_imagefile) on a clean clone of the final commit, exit 0; shellcheck covers the new helper.workflow_dispatchon GitHub was not executed from this branch; the hosted gated step will run on the first campaign.Target Branch
mainor a maintainer-requested release branch such as0.1.x. (Targets1.x.xas requested by the maintainer.)Risk / Review Notes
Notes: CI-only. The lane loop, witness-tail computation and report paths are moved verbatim into the helper; the 720-minute job cap and the harness defaults are unchanged. The helper exports
LC_ALL=C(shell lint requirement) into the lanes and metadata steps. The gated wiring step adds up to four tiny lanes and, when timeouts are requested, up to two validate-only node startups per campaign. Overlaps with the scheduled-validation work on the same workflow only in disjoint blocks; a trial merge is clean.Docs / Process Impact
Choose exactly one:
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.