4a5b50eb - Run the RPC suite on develop/main PRs and detect upcoming upstream consensus deadlines - #10
Draft
davidleomay wants to merge 7 commits into
Draft
4a5b50eb - Run the RPC suite on develop/main PRs and detect upcoming upstream consensus deadlines#10davidleomay wants to merge 7 commits into
davidleomay wants to merge 7 commits into
Conversation
Two gaps in how this fork guards consensus-relevant changes. The extended RPC suite only ran for pull requests targeting master. Upstream syncs land on develop, so the change most in need of the consensus harness was merged without it. The pull_request trigger now also covers develop and main. The push trigger is unchanged: the gate belongs on the pull request, not after the merge. To keep that affordable, guix-build now runs only for master. It produces cross-compiled release artifacts, contributes nothing to the consensus check, and dominates the runtime; without this the develop lane would grow from minutes to hours. Second, nothing compared this fork against upstream releases. A release that requires an upgrade before a given block height could pass unnoticed until the deadline had already taken effect. The new upstream-release-check workflow runs weekly and stays silent unless it has something to say. It scans recent upstream releases for one that carries a deadline height or hard-fork wording, skipping drafts and prereleases, and takes the newest such release. Scanning rather than reading only the latest release matters: a routine release published after a deadline release would otherwise hide it. It then checks whether both develop and main contain that release. main is checked because it is the branch the release image is built from, so a lagging main means the published image is behind even when develop is current. If either branch is behind, the workflow opens or updates a single tracking issue naming the branches and their lag. Once both branches contain the release it comments and closes that issue, so the check needs no manual cleanup. AGENTS.md and CLAUDE.md both described the CI trigger and are brought in line. Both also claimed CI ran on pushes to all branches, which was already wrong before this change; they now state the actual trigger.
Three findings from review, all about an alarm staying useful. Widen the release scan from 20 to 100. Upstream ships roughly seven releases a year, so twenty covered about three years - unlikely to be exceeded, but the failure mode is the alarm going silent, which is worth being generous about. Widen the markers. The hard-fork pattern missed the hyphenated spelling, which appears in this repository's own release notes. The deadline pattern only matched "before/prior to block N" and missed "prior to height N", "before block height N" and "before block #N". Stop asserting an interpretation the release notes may contradict. The check only proves the words appear somewhere in the body, so a release saying it is not a hard fork, or mentioning one in a historical note, would have produced a false claim. The issue now records what was observed rather than what it means.
The scan took the first release whose notes carried a marker and stopped there. That relied on the API listing releases in an order that matches commit ancestry, which it does not - it orders by creation time. A maintenance release cut later from an older commit would have been found first, seen as already contained, and ended the run, leaving a newer and still missing deadline release unexamined. The alarm would have gone quiet on exactly the case it exists for. The scan now collects every marker-bearing release in the window, resolves and compares only those, and reports the newest one that is not yet contained in both branches. No ordering assumption is left. Also adds a concurrency group, following the pattern already used by auto-release-pr. Without it two near-simultaneous runs could each open a tracking issue, and the catch-up path closed only the first, leaving one open that claimed a lag which no longer existed. The catch-up path now closes every matching open issue.
grep matches line by line, and both marker patterns rely on whitespace between their words. A release body wrapping as "upgrade before\nblock height 1,371,000" matched neither pattern, so the release was skipped as unmarked and the run could end quietly. No current upstream release wraps that way, so this was latent rather than broken, but the failure mode is silence and the fix is one step: the body's whitespace is now collapsed before matching. The patterns themselves are unchanged, and the classification of every current upstream release is identical. The catch-up path tolerated a failed issue close so that an issue a human had already closed would not fail the job. That also swallowed a transient API failure: the comment was posted, the close failed, the run stayed green, and the issue stayed open claiming a lag that no longer existed until the next weekly run. The close is now read back. An already-closed issue still passes; an issue that is still open after the attempt fails the job loudly. CLAUDE.md described the ignored paths as README.md where the workflow ignores **/README.md, so nested READMEs are ignored too. That line was touched by this branch and kept the imprecision. Deliberately not changed: the deadline pattern still matches without requiring a nearby context word. Requiring one would trade a cheap failure, an alert somebody reads and dismisses, for the expensive one, silence while a deadline passes.
The height extraction ended in `head -n1`, which closes the pipe as soon as it has its line. Under `set -euo pipefail` a producer still writing at that moment gets SIGPIPE, the pipeline reports 141, and the run aborts - the check would die partway through instead of reporting. Measured rather than assumed: with two matches the construct returns normally, but with twenty thousand it exits 141 and produces nothing. A release body would need hundreds of deadline phrases to reach that, so this was never going to fire in practice. It is removed anyway, because the failure mode is the alarm going quiet and the fix is one line. The result is now captured in full and the first line taken with parameter expansion, so no stage can be killed by a closed reader. Wrapping the line in set +e was avoided deliberately: that would also have hidden a genuine grep or tr failure, which is the opposite of how the rest of this workflow behaves. The same stress that produced exit 141 now returns the correct value, and the classification of every current upstream release is unchanged.
The tracking issue was matched by title prefix alone. This repository is public, so anyone can open an issue titled "Upstream consensus deadline: ..." and the workflow would have adopted it: overwriting its title and body while a lag existed, or commenting on it and closing it once the fork caught up. Both lookups now also require the issue to have been opened by github-actions[bot]. Both lookups also read only the first page of results, and that endpoint returns pull requests alongside issues. With more than a hundred open items a genuine tracking issue could sit unseen behind them, so the update path would open a duplicate and the catch-up path would never close the original. Both now paginate and merge the pages before filtering. Verified against a constructed case: given a stranger's issue with the exact title on page one, a pull request with the same title, an unrelated bot issue, and the real tracking issue on page two, the filter returns only the last of these.
The report was built from a single release, chosen as the most recently published of those the fork does not yet contain. But several deadline releases can be outstanding at once - being behind two mandatory releases simultaneously is what happened here - and an older release usually carries an earlier deadline height than a newer one. Sorting by publication date therefore reported the least urgent unmet deadline and said nothing about the earlier one, which may already have passed. An operator would have read a height still in the future. The issue body now lists every release the fork is missing, ordered by deadline height with the earliest first and releases without a height last. It remains a single issue; upgrading to the newest resolves the older ones, so the title still names one. A release can carry both a deadline height and hard-fork wording - v0.14.18.0 does - so those two lines are independent again rather than alternatives, and such a release keeps its severity signal. Both issue lookups also guard the page merge: with no pages at all, jq's add yields null and the following filter aborts. The real API returns the safe shape and the abort would be loud rather than silent, but the guard costs nothing.
Member
Author
|
EN: DE: DetailsWhat failed
Two cases of the same file fail: It is not caused by this pull requestThe diff here consists solely of workflow YAML and two markdown files — not a line of C++, CMake or build configuration. On the same code:
So the failure is build-configuration-dependent and intermittent: RelWithDebInfo only, and not on every run there. It was invisible until now because Why it matters
Whether this is test nondeterminism, an optimisation-dependent defect, or a real consensus problem cannot be settled from the logs alone. What this needsEnabling the suite as a gate makes develop pull requests red while
None of these has been chosen here. The pull request stays a draft rather than being forced through a gate it cannot pass. Failing run: https://github.com/DFXswiss/firo/actions/runs/34225275183 — job |
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
2 participants
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.
EN:
Runs the extended RPC suite on pull requests targeting
developandmain, not onlymaster, so that upstream syncs get the consensus harness before they are merged. Keeps that affordable by restricting the cross-compiledguix-buildjob tomaster. Adds a weekly check that stays silent unless an upstream release carries a consensus deadline this fork does not yet contain, on eitherdevelopormain. Brings the CI description inAGENTS.mdandCLAUDE.mdin line with the actual triggers.DE:
Führt die erweiterte RPC-Suite auch für Pull Requests gegen
developundmainaus, nicht nur gegenmaster, damit Upstream-Syncs das Konsens-Harness vor dem Merge durchlaufen. Der cross-kompilierteguix-build-Job bleibt dafür aufmasterbeschränkt. Ergänzt einen wöchentlichen Check, der nur dann meldet, wenn ein Upstream-Release eine Konsens-Deadline trägt, die dieser Fork aufdevelopodermainnoch nicht enthält. Zieht die CI-Beschreibung inAGENTS.mdundCLAUDE.mdauf die tatsächlichen Trigger nach.Details
1. Consensus tests on the branches we merge into
ci-master.ymlonly triggered for pull requests targetingmaster. PRs targetingdevelop— where upstream syncs land — randfx-develop-ci.yml, which builds the Docker image and runs CTest, notqa/pull-tester/rpc-tests.py. The change most in need of the consensus harness was therefore merged without it.The
pull_requesttrigger now coversmaster,developandmain. Thepushtrigger is deliberately unchanged and still onlymaster: the gate belongs on the pull request, not after the merge.Why reuse
ci-master.ymlinstead of copying the RPC step intodfx-develop-ci.yml. Mirroring it would duplicate the linux build (toolchain, depends, cmake flags) into a second file, and that copy would drift from upstream on every sync. Reusing upstream's own definition keeps the test path single-sourced. The cost is that this file is inherited from upstream, so the diff is kept deliberately minimal — two branch entries, one comment, oneif:.Why
guix-buildis restricted tomaster. It is a five-host cross-compilation matrix producing release artifacts. It contributes nothing to the consensus question and dominates the runtime; recent full runs of this workflow take roughly two hours. Restricting it keeps the develop lane proportionate. It still runs unchanged forpushevents and for PRs targetingmaster.2. Upstream consensus-deadline check
Nothing in this repository compared the fork against upstream releases.
auto-release-pr.yamlonly comparesdevelopagainstmainwithin the fork, so an upstream gap produces no commit and therefore no signal — it is blind to this case rather than merely quiet.The new workflow runs weekly, plus on dispatch and on pushes to its own file. It is designed to stay silent:
before block N/prior to block N) or hard-fork wording.developandmaincontain it.Why every marker release rather than just the newest. Being behind two mandatory releases at once is not hypothetical — it is what happened. An older release usually carries an earlier deadline than a newer one, so reporting only the most recently published one would name the least urgent deadline and stay silent about the one that may already have passed.
Why scan rather than read
releases/latest. A routine release published after a deadline release would otherwise mask it. That is not hypothetical — upstream's own history hasv0.14.17.2(no marker) published afterv0.14.16.1(deadlineprior to block 1,329,000). Reading only the latest release would have said nothing while a deadline was still unmet.Why the marker is the deadline height rather than the words "hard fork". Across the last seven upstream releases,
hard forkappears in one; a deadline height appears in three, including the two that usemandatory releasewording without calling themselves a fork. Keying on the height catches the class of event; keying on the phrase would not.Why
mainis checked too.firod-prd.yamlbuilds the release image onpushtomain. A release synced intodevelopbut not yet released tomainleaves the published image behind, which is the same failure one stage later.3. Documentation
AGENTS.mdandCLAUDE.mdboth describe the CI trigger and would have gone stale. Both also claimed CI runs "on pushes to all branches", which was already wrong before this change — the file haspush: branches: [master]. Both lines now state the actual trigger, including theguix-buildrestriction.Known limits, deliberately accepted
The matcher is kept broad on purpose. Every proposal to tighten it was declined, because the trade runs the wrong way: a false alarm costs somebody reading an issue and closing it, while a missed marker costs a node sitting on a dead chain.
before/prior tofollowed byblockand/orheightand then digits, optionally prefixed with#. Punctuation between the keyword and the number, or a markdown link around it, would not match. Across the last year every deadline-bearing upstream release used the plain form, so this is unevidenced rather than theoretical-and-likely; widening it further would add branches that cannot be tested against data that does not exist.schedule:triggers in a repository with no activity for 60 days, and that disablement is itself silent. This cannot be addressed from inside the workflow. Note that scheduled runs demonstrably do fire in this repository today — the existing artifact-reclaim workflow has run daily onscheduleand is active.A
grep -qin the hard-fork check was reported as vulnerable to aSIGPIPE/pipefailabort of the kind fixed in the height extraction. Measured at 100 kB, 2 MB and 20 MB of input, the pipeline returns 0 every time, while the construct that was fixed returns 141 under the same conditions. The difference is that the remaining producer is bash'sprintfbuiltin rather than an external command. Not changed.Verification
The workflows parse as YAML; the embedded shell passes
bash -nandshellcheck -S warningcleanly. The API shapes the new workflow relies on were checked against the live upstream and this fork:releases,commits/<tag>andcompare/<branch>...<sha>withahead_by == 0as the "branch contains this release" signal, confirmed both for a synced branch and against an artificially lagging one.The check produces no alert at present, because both branches already contain the newest deadline-bearing release — that is the expected quiet state, not an untested path.