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/32103.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

It looks like you are trying to change the task to test changes merged into master to avoid running into issues fixed previously.

Correct. Whilst we can try to rely on developers keeping up-to-date with rebases on master it seems less error-prone to do it as part of the task.

However, you check out the previous commit, so overall this change is a no-op , unless I am missing something?

Maybe you wanted to instead rebase the changes onto master? However, this is not possible in general, as some commits are not possible to rebase. For example, a subtree merge commit is not possible to rebase.

Yes you're right, I think the correct checkout command would in fact be git checkout HEAD^2 as HEAD~~ does indeed skip back to the previous merge commit, which is not what was intended here. AFAIK a merge would work with a subtree merge commit?

Also, the goal of this check is to find bisect errors early, so the whole point is to run on the original changes, not merged into master.

Thinking about alternative fixes, obviously the best fix would be to not have any intermittent issues. Absent that, it would be good for devs to rebase on latest master if they want to ensure the CI gets all the latest fixes. Alternatively, failures in this task could be ignored or the task could be re-run.

Fair enough. Ignoring failures in this task seems a little sub-optimal to me though.

Another alternative could be to auto-prompt a rebase on failure, something like:

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 4f0ed47b951..9e75bf0a258 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -69,11 +69,18 @@ jobs:
           sudo apt-get update
           sudo apt-get install clang ccache build-essential cmake pkgconf python3-zmq libevent-dev libboost-dev libsqlite3-dev libdb++-dev systemtap-sdt-dev libzmq3-dev qtbase5-dev qttools5-dev qttools5-dev-tools qtwayland5 libqrencode-dev -y
       - name: Compile and run tests
+        id: run_tests
         run: |
           # Run tests on commits after the last merge commit and before the PR head commit
           # Use clang++, because it is a bit faster and uses less memory than g++
           git rebase --exec "echo Running test-one-commit on \$( git log -1 ) && CC=clang CXX=clang++ cmake -B build -DWERROR=ON -DWITH_ZMQ=ON -DBUILD_GUI=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DWITH_BDB=ON -DWITH_USDT=ON -DCMAKE_CXX_FLAGS='-Wno-error=unused-member-function' && cmake --build build -j $(nproc) && ctest --output-on-failure --stop-on-failure --test-dir build -j $(nproc) && ./build/test/functional/test_runner.py -j $(( $(nproc) * 2 )) --combinedlogslen=99999999" ${{ env.TEST_BASE }}
 
+      - name: Suggest rebase
+        if: failure() && steps.run_tests.outcome == 'failure'
+        run: |
+          echo "If you haven't already, please try rebasing your branch on the latest master"
+          exit 1
+
   macos-native-arm64:
     name: ${{ matrix.job-name }}
     # Use any image to support the xcode-select below, but hardcode version to avoid silent upgrades (and breaks).

But it sounds like it would be best to close this (and #31946 ? ) ?

It looks like you are trying to change the task to test changes merged into master to avoid running into issues fixed previously.

Correct. Whilst we can try to rely on developers keeping up-to-date with rebases on master it seems less error-prone to do it as part of the task.

It is possible to write code in a commit that compiles fine on master, but not when compiling the commit itself. (The reverse of a silent merge conflict) Though, there only was one instance of this. More likely would be that devs are wholesale not compiling/testing intermediate commits, which would also be covered if the changes are "rebased".

Yes you're right, I think the correct checkout command would in fact be git checkout HEAD^2 as HEAD~~ does indeed skip back to the previous merge commit, which is not what was intended here. AFAIK a merge would work with a subtree merge commit?

I think what you want is to checkout each old commit and then merge it (inside the CI task) with master. Not sure how to achieve it otherwise. Maybe you can share your steps to reproduce with git?

Fair enough. Ignoring failures in this task seems a little sub-optimal to me though.

Another alternative could be to auto-prompt a rebase on failure, something like:
Details
But it sounds like it would be best to close this (and #31946 ? ) ?

Yeah, I meant "manually ignoring", not silently ignoring the return code. 😅

Other than that, no strong opinion. I think anything is fine.

I think what you want is to checkout each old commit and then merge it (inside the CI task) with master. Not sure how to achieve it otherwise. Maybe you can share your steps to reproduce with git?

To clarify, my suggestion would be to modify the git rebase --exec 'run_test' base into git rebase --exec 'git log -1 && git merge --no-commit master && echo run_test && git merge --abort' base. However, I haven't tested this.

I think what you want is to checkout each old commit and then merge it (inside the CI task) with master. Not sure how to achieve it otherwise. Maybe you can share your steps to reproduce with git?

To clarify, my suggestion would be to modify the git rebase --exec 'run_test' base into git rebase --exec 'git log -1 && git merge --no-commit master && echo run_test && git merge --abort' base. However, I haven't tested this.

Would it not in that case be simpler to checkout the PR HEAD commit, fetch and rebase onto master, before doing a more "normal" rebase from the MERGE_BASE? Something like:

#!/usr/bin/env bash
set -x

git switch master

# Create a dummy branch with a few empty commits
git switch -c dummy-branch
for i in {1..5}; do
    git commit --allow-empty -m "empty commit $i"
done

# This state we are in here currently is the same as the PR HEAD commit we currently check out

# Rebase these commits onto latest master (origin in the CI, may be upstream locally)
git pull --rebase origin master
git log --oneline -n7

# This checks out "empty commit 4"
git checkout HEAD~
MERGE_BASE=$(git rev-list -n1 --merges HEAD)
EXCLUDE_MERGE_BASE_ANCESTORS=

# MERGE_BASE can be empty due to limited fetch-depth
if test -n "$MERGE_BASE"; then
    EXCLUDE_MERGE_BASE_ANCESTORS=^${MERGE_BASE}^@
fi
TEST_BASE=$(git rev-list -n7 --reverse HEAD "$EXCLUDE_MERGE_BASE_ANCESTORS" | head -1)

echo would run: git rebase --exec "echo Running test-one-commit on \$( git log -1 ) && CC=clang CXX=clang++ cmake -B build -DWERROR=ON -DWITH_ZMQ=ON -DBUILD_GUI=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DWITH_BDB=ON -DWITH_USDT=ON -DCMAKE_CXX_FLAGS='-Wno-error=unused-member-function' && cmake --build build -j $(nproc) && ctest --output-on-failure --stop-on-failure --test-dir build -j $(nproc) && ./build/test/functional/test_runner.py -j $(( $(nproc) * 2 )) --combinedlogslen=99999999" "$TEST_BASE"

git branch -D dummy-branch

This gets the following log output for me, which appears to be at least what I would expect this CI job to be doing:

$ ./checkout.sh
+ git switch master
Already on 'master'
Your branch is up to date with 'origin/master'.
+ git switch -c dummy-branch
Switched to a new branch 'dummy-branch'
+ for i in {1..5}
+ git commit --allow-empty -m 'empty commit 1'
[dummy-branch 58ecc5950e8] empty commit 1

<snip>

+ git pull --rebase origin master
From https://github.com/willcl-ark/bitcoin
 * branch                    master     -> FETCH_HEAD
Current branch dummy-branch is up to date.
+ git log --oneline -n7
af33d1bef61 (HEAD -> dummy-branch) empty commit 5
82be04b802c empty commit 4
7f0435dc544 empty commit 3
d82729928f9 empty commit 2
58ecc5950e8 empty commit 1
998386d4462 (upstream/master, upstream/HEAD, origin/master, master) Merge bitcoin/bitcoin#31866: test, refactor: Add TestNode.binaries to hold binary paths
aa87e0b4460 Merge bitcoin/bitcoin#31519: refactor: Use std::span over Span
+ git checkout HEAD~
HEAD is now at 82be04b802c empty commit 4
++ git rev-list -n1 --merges HEAD
+ MERGE_BASE=998386d4462f5e06412303ba559791da83b913fb
+ EXCLUDE_MERGE_BASE_ANCESTORS=
+ test -n 998386d4462f5e06412303ba559791da83b913fb
+ EXCLUDE_MERGE_BASE_ANCESTORS='^998386d4462f5e06412303ba559791da83b913fb^@'
++ git rev-list -n7 --reverse HEAD '^998386d4462f5e06412303ba559791da83b913fb^@'
++ head -1
+ TEST_BASE=998386d4462f5e06412303ba559791da83b913fb
++ nproc
++ nproc
++ nproc
+ echo would run: git rebase --exec 'echo Running test-one-commit on $( git log -1 ) && CC=clang CXX=clang++ cmake -B build -DWERROR=ON -DWITH_ZMQ=ON -DBUILD_GUI=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DWITH_BDB=ON -DWITH_USDT=ON -DCMAKE_CXX_FLAGS='\''-Wno-error=unused-member-function'\'' && cmake --build build -j 16 && ctest --output-on-failure --stop-on-failure --test-dir build -j 16 && ./build/test/functional/test_runner.py -j 32 --combinedlogslen=99999999' 998386d4462f5e06412303ba559791da83b913fb
would run: git rebase --exec echo Running test-one-commit on $( git log -1 ) && CC=clang CXX=clang++ cmake -B build -DWERROR=ON -DWITH_ZMQ=ON -DBUILD_GUI=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DWITH_BDB=ON -DWITH_USDT=ON -DCMAKE_CXX_FLAGS='-Wno-error=unused-member-function' && cmake --build build -j 16 && ctest --output-on-failure --stop-on-failure --test-dir build -j 16 && ./build/test/functional/test_runner.py -j 32 --combinedlogslen=99999999 998386d4462f5e06412303ba559791da83b913fb
+ git branch -D dummy-branch
Deleted branch dummy-branch (was af33d1bef61).

Would it not in that case be simpler to checkout the PR HEAD commit, fetch and rebase onto master

Again, I don't think this is possible.

In your example the rebase succeeds, because it is not needed. See:

git pull --rebase origin master
From https://github.com/willcl-ark/bitcoin
 * branch                    master     -> FETCH_HEAD
Current branch dummy-branch is up to date.

However, in the general case, it is not possible to rebase all commits of all pull requests on top of current master, even if the pull request merges fine into current master. For example:

• One commit in the pull reqeust could be changing a file that was deleted/modified in master and the next commit fully or partially reverts those changes (for whatever reason). A merge with master may succeed, but a rebase may fail.
• Another example would be a subtree merge (or any unclean merge commit) as part of the pull request.

With exact steps to reproduce:

sh-5.2$ git fetch origin 0e8254fef9f0d37f88d1f077f0165e7febd3bfa0  # subtree merge
...
 * branch                  0e8254fef9f0d37f88d1f077f0165e7febd3bfa0 -> FETCH_HEAD
sh-5.2$ git checkout 0e8254fef9f0d37f88d1f077f0165e7febd3bfa0
...
HEAD is now at 0e8254fef9 Update secp256k1 subtree to latest master
sh-5.2$ git rebase 998386d4462  # master
...
Could not apply dd59896431... Squashed 'src/secp256k1/' changes from 0cdc758a56..70f149b9a1

Due to lack of activity here, I've implemented a replacement in #32203

Due to lack of activity here, I've implemented a replacement in #32203

Great, thanks @maflcko .

I’ll review that one.