ci: Pin native tests on cross-builds to same commit

maflcko commented at 8:08 am on December 16, 2025: member

After commit 13809b867ad980a12f886316b18bb2d3d2848ac2, the native tests may check out a different commit than the cross-build task that produced the artefacts they run on.

Obviously, this may lead to test failures.

Fix it, by first determining a fixed commit, to be used for both the build and the native tests.

An alternative could be to fully or partially revert 13809b867ad980a12f886316b18bb2d3d2848ac2, but that comes again with the downsides making it harder to detect silent merge conflicts by re-running CI, or clearing unrelated and fixed intermittent test issues by re-running CI. Then, the only alternative would be to close and re-open the pull request.

DrahtBot added the label Tests on Dec 16, 2025

DrahtBot commented at 8:08 am on December 16, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34080.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	janb84, ryanofsky, hodlinator

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33593 (guix: Use UCRT runtime for Windows release binaries by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in .github/workflows/ci.yml:358 in 0819248992 outdated

353+    steps:
354+      - *ANNOTATION_PR_NUMBER
355+      - *CHECKOUT
356+      - name: Record commit
357+        id: record-commit
358+        run: echo "commit=$(git rev-parse HEAD)" >> "$GITHUB_OUTPUT"

hodlinator commented at 8:56 am on December 16, 2025:

Is the commit the PR HEAD, HEAD on master or the merged HEAD?

Might be nice to output the value inside of this step, or is it already visible in the CI web interface? With it, one could compare with the hashes from the Checkout step and figure out which of the 3 it is.

maflcko commented at 9:03 am on December 16, 2025:

The CHECKOUT definition should have docs on what it checks out.

The value is also printed by checkout:

https://github.com/bitcoin/bitcoin/actions/runs/20260880309/job/58172444051?pr=34080#step:3:92

0/usr/bin/git log -1 --format=%H
1cd7a26c63908e2a878d9797675cae9de97c56ab7

hodlinator commented at 9:23 am on December 16, 2025:

Yes, I guess the git context changing between CHECKOUT and the next step would defeat CHECKOUT’s whole purpose. Still I think it would make things more transparent to output it again in this recording/freezing step. But just nit-level.

maflcko commented at 9:40 am on December 16, 2025:

Happy to push any diff or commit, if someone writes one. I am just not sure what to do here myself. Should I add another git log -1 somewhere?

hodlinator commented at 1:23 pm on December 17, 2025:

I’ll punt on devising an example diff for now unless the ACKs get invalidated.

maflcko force-pushed on Dec 16, 2025

DrahtBot added the label CI failed on Dec 16, 2025

DrahtBot removed the label CI failed on Dec 16, 2025

janb84 commented at 12:06 pm on December 16, 2025: contributor

I’m not sure why this only applies for the windows builds. I would say this needs to be resolved for everything not only windows.

As I see it, in https://github.com/bitcoin/bitcoin/commit/13809b867ad980a12f886316b18bb2d3d2848ac2 we circumvented the standard behavior of determine state of repo once to determine every time. This behavior is now excessive, as in the state of repo can change mid run, resulting in the possibility of mid run changes (eg testing and compiling differences).

In my mind the checkout step should be changed to first fix the state of the repo and then use that fixed state during the whole run.

maflcko commented at 12:09 pm on December 16, 2025: member

I’m not sure why this only applies for the windows builds. I would say this needs to be resolved for everything not only windows.

Why? The other tasks do not split the build and tests into two task, each with a different checkout, so how would they run into this class of issue?

janb84 commented at 12:20 pm on December 16, 2025: contributor

I’m not sure why this only applies for the windows builds. I would say this needs to be resolved for everything not only windows.

Why? The other tasks do not split the build and tests into two task, each with a different checkout, so how would they run into this class of issue?

No, you are correct but I think that you have discovered something more broadly applicable.

Correct me I’m wrong, but because we now do not “fixate” the state of the repo on creation, we can run into the following:

The matrix run checksout and merges 000001 ands starts running. In the mean time the master branch changes to 0000002 The windows build start running and checksout and merges 0000002.

Now this does not cause any trouble because the matrix tests what it compiles, but the matrix run differs from the windows because of the mid run pr merge. But It only shows in the windows builds because of the change in compile vs testing state.

maflcko commented at 12:40 pm on December 16, 2025: member

Yeah, I think this is fine for different CI tasks in one check run to run on different commits.

However, it would also be possible to freeze the commit for all CI tasks. This would also mean that on a re-run (for a silent merge conflict check, or to get rid of an intermittent test issue) can only be done for all tasks, or none.

Not sure which one is preferable, but happy to push either approach.

in .github/workflows/ci.yml:346 in faa51d4f1a

342@@ -343,9 +343,23 @@ jobs:
343         run: |
344           py -3 test/fuzz/test_runner.py --par $NUMBER_OF_PROCESSORS --loglevel DEBUG "${RUNNER_TEMP}/qa-assets/fuzz_corpora"
345 
346+  checkout-commit:

ryanofsky commented at 4:41 pm on December 16, 2025:

In commit “ci: Pin native tests on cross-builds to same commit” (faa51d4f1a8ed921809500b77b1b9b1fe0636024)

Maybe call this record-frozen-commit to be consistent with the displayed job name? I already find it difficult to keep track of the relationships between displayed job names and their definitions in configuration files and scripts, and it woudl be nice to be more consistent.

maflcko commented at 5:06 pm on December 16, 2025:

thx, done. Also put [meta] in the two non-meat tasks.

ryanofsky approved

ryanofsky commented at 4:45 pm on December 16, 2025: contributor

Code review ACK faa51d4f1a8ed921809500b77b1b9b1fe0636024

Thanks for implementing this! It looks like this should fix the issue that left me confused the other day #33657 (review).

I guess this fix doesn’t work the way I would have expected it to work, but it seems reasonable. I would have expected the fix to just straightforwardly have the windows-cross job output its commit hash, and have the windows-native-test job use it. But it looks like because both of these jobs are matrix jobs and github doesn’t provide any way in ${{}} expressions to reference an output from a previous job that depends on a current matrix value, this isn’t possible to implement as long as windows-native-test is a matrix job. (An alternate approach that could work would be to have the windows-native-test checkout the windows-cross hash with a checkout command, instead of an action and ${{}}, but this would also be messy.) Overall, I don’t like how adding a checkout-commit job complicates the build graph and doesn’t let the windows-cross job use the latest sources on reruns like other jobs, but this seems hard to avoid if we want to avoid CI errors on windows from mixed commits, and want windows-native-test to be a matrix job.

maflcko commented at 4:58 pm on December 16, 2025: member

Overall, I don’t like how adding a checkout-commit job complicates the build graph and doesn’t let the windows-cross job use the latest sources on reruns like other jobs,

Actually, it makes it easier to re-run with the latest sources: Previously, it is not possible to re-run only one “leg” of the matrix with one click of a button. Also, it is not possible to run the full matrix with one click.

Now, it is possible to run the full matrix with both “legs” in one click, by re-running the new parent job.

ci: Pin native tests on cross-builds to same commit faa8ee62f5

maflcko force-pushed on Dec 16, 2025

janb84 commented at 6:47 pm on December 16, 2025: contributor

ACK faa8ee62f5c1606217fbe9eacdd504ec133920a4

Code review, looks correct; Record-fronzen-commit sets steps.record-commit.outputs.commit which is used as a reference in the checkout step.

This would also mean that on a re-run (for a silent merge conflict check, or to get rid of an intermittent test issue) can only be done for all tasks, or none.

Re running everything does not look like a good tradeoff in this case. Given this, I think this PR is currently the best solution for the problem.

DrahtBot requested review from ryanofsky on Dec 16, 2025

ryanofsky approved

ryanofsky commented at 7:51 pm on December 16, 2025: contributor

Code review ACK faa8ee62f5c1606217fbe9eacdd504ec133920a4. Thanks for the naming & display updates since last review!

re: #34080 (comment)

Overall, I don’t like how adding a checkout-commit job complicates the build graph and doesn’t let the windows-cross job use the latest sources on reruns like other jobs,

Actually, it makes it easier to re-run with the latest sources

My point is that if you restart the windows-cross jobs directly they just repeat the same build, which is inconsistent with the other jobs, and this is unfortunate. This would be straightforward to fix if the windows jobs were not matrix jobs, but since they are, we have to live with the limitations.

Previously, it is not possible to re-run only one “leg” of the matrix with one click of a button.

I’m not sure I understand. Each individual job in the matrix has a restart button. So I can restart the windows-cross msvcrt job without restarting the ucrt job, or vice versa. It is true that both windows-native-test jobs get restarted as well, but that happens with or without this PR. And it shows another disadvantage of using a matrix for windows-native-test jobs: in addition to not being able to reference previous outputs in ${{}} expressions, they are also not able to directly depend on other jobs they rely on.

Also, it is not possible to run the full matrix with one click.

Now, it is possible to run the full matrix with both “legs” in one click, by re-running the new parent job.

I wouldn’t say it’s good to add an unnecessary job just to provide a button that allows restarting some arbitrary collection of other jobs. And I think it’s probably surprising to see restarting the “frozen commit” job only restarts windows jobs. But I do think it’s reasonable to add the “frozen commit” job to work around the other limitations here and make jobs more reliable. So overall the change is very nice, despite the complications.

hodlinator approved

hodlinator commented at 1:32 pm on December 17, 2025: contributor

crACK faa8ee62f5c1606217fbe9eacdd504ec133920a4

I’m surprised the Windows runners are able to find the commit resulting from a merge between PR & master that happened on an Ubuntu runner. But looking at the CI logs that does appear to be the case.

Agree that it would be nice to use the frozen merge commit for all jobs, but if it makes re-running an all or nothing proposition (https://github.com/bitcoin/bitcoin/pull/34080#issuecomment-3660339716), I agree on avoiding it.

Since we didn’t proceed with #33509 there’s no equivalent need for MacOS for now.

maflcko commented at 2:35 pm on December 17, 2025: member

I’m surprised the Windows runners are able to find the commit resulting from a merge between PR & master that happened on an Ubuntu runner. But looking at the CI logs that does appear to be the case.

The merge happens on the github infra, and anyone can fetch any commit. I have a local function to do that:

0$ type gfbb 
1gfbb is a function
2gfbb () 
3{ 
4    git fetch https://github.com/bitcoin/bitcoin "$1"
5}

fanquake merged this on Dec 17, 2025

fanquake closed this on Dec 17, 2025

maflcko deleted the branch on Dec 17, 2025

ci: Pin native tests on cross-builds to same commit #34080

Code Coverage & Benchmarks

Reviews

Conflicts