valgrind currently does not work on GCC compiled executables, due to an upstream bug. https://bugs.kde.org/show_bug.cgi?id=472329
So temporarily switch to clang, so that a long term solution can be figured out in the meantime.
Also, expand the first valgrind suppression, so that it works with newer clang versions as well.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | l0rinc |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, this pull request conflicts with the following ones:
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.
16@@ -17,5 +17,6 @@ export FUZZ_TESTS_CONFIG="--valgrind"
17 export GOAL="all"
18 export BITCOIN_CONFIG="\
19 -DBUILD_FOR_FUZZING=ON \
20- -DCMAKE_CXX_FLAGS='-Wno-error=array-bounds' \
21+ -DCMAKE_C_COMPILER=clang \
22+ -DCMAKE_CXX_COMPILER=clang++ \
no-error=array-bounds related to the mentioned bug?
5@@ -6,9 +6,9 @@
6
7 export LC_ALL=C.UTF-8
8
9-export CI_IMAGE_NAME_TAG="mirror.gcr.io/debian:trixie"
10+export CI_IMAGE_NAME_TAG="mirror.gcr.io/ubuntu:26.04"
container image version
it’s not strictly just the version, is this part of a unification attempts?
26.04 version? In other cases we’ve used the24.04 LTS.
https://releases.ubuntu.com/26.04-snapshot1 seems empty.
The build is passing (though I don’t see the valgrind tasks referenced by the current GH Actions matrix anymore), not sure what I’m missing :/
Added to the commit msg:
26.04 tag - is that a custom snapshot that we want to use temporarily? We don’t usually depend on unreleased OS, I’m confused why it’s necessary here.
11@@ -12,13 +12,28 @@
12 # --error-limit=no build/bin/test_bitcoin
13 #
14 # Note that suppressions may depend on OS and/or library versions.
15-# Tested on aarch64 and x86_64 with Ubuntu Noble system libs, using clang-16
16-# and GCC, without gui.
17+# Tested on x86_64 with Ubuntu 26.04 system libs, using clang, without gui.
18+{
Reverting them will make the CI fail.
I think you can also test locally by compiling with clang-21 and then running:
./bld-cmake/test/functional/interface_bitcoin_cli.py --valgrind
I tried this on an RPI5 with Clang version 22.0.0 (aarch64) for fa173ec59a9dcc2e7956366344d68999d1d1249e.
It seems to trigger similar valgrind false positives on aarch64 - the updated suppression doesn’t seem to cover it, see:
02026-02-14T19:42:42.017831Z TestFramework (ERROR): Unexpected exception
1Traceback (most recent call last):
2 File "/mnt/my_storage/bitcoin/test/functional/test_framework/test_framework.py", line 138, in main
3 self.setup()
4 ~~~~~~~~~~^^
5 File "/mnt/my_storage/bitcoin/test/functional/test_framework/test_framework.py", line 272, in setup
6 self.setup_network()
7 ~~~~~~~~~~~~~~~~~~^^
8 File "/mnt/my_storage/bitcoin/test/functional/test_framework/test_framework.py", line 363, in setup_network
9 self.setup_nodes()
10 ~~~~~~~~~~~~~~~~^^
11 File "/mnt/my_storage/bitcoin/test/functional/test_framework/test_framework.py", line 385, in setup_nodes
12 self.start_nodes()
13 ~~~~~~~~~~~~~~~~^^
14 File "/mnt/my_storage/bitcoin/test/functional/test_framework/test_framework.py", line 531, in start_nodes
15 node.wait_for_rpc_connection()
16 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
17 File "/mnt/my_storage/bitcoin/test/functional/test_framework/test_node.py", line 316, in wait_for_rpc_connection
18 raise FailedToStartError(self._node_msg(
19 f'bitcoind exited with status {self.process.returncode} during initialization. {str_error}'))
20test_framework.test_node.FailedToStartError: [node 0] bitcoind exited with status 1 during initialization. ==68399== Use of uninitialised value of size 8
21==68399== at 0x22AD68: _M_is_engaged (optional:563)
22==68399== by 0x22AD68: operator=<node::BlockfileCursor> (optional:1036)
23==68399== by 0x22AD68: node::BlockManager::LoadBlockIndexDB(std::optional<uint256> const&) (???:565)
24==68399== by 0x46C717: ChainstateManager::LoadBlockIndex() (../src/validation.cpp:4939)
25==68399== by 0x241A8F: node::CompleteChainstateInitialization(ChainstateManager&, node::ChainstateLoadOptions const&) (../src/node/chainstate.cpp:43)
26==68399== by 0x24142B: node::LoadChainstate(ChainstateManager&, kernel::CacheSizes const&, node::ChainstateLoadOptions const&) (../src/node/chainstate.cpp:184)
27==68399== by 0x1950C7: operator() (../src/init.cpp:1392)
28==68399== by 0x1950C7: operator()<(lambda at ../src/init.cpp:1392:45)> (???:1386)
29==68399== by 0x1950C7: InitAndLoadChainstate(node::NodeContext&, bool, bool, kernel::CacheSizes const&, ArgsManager const&) (???:1392)
30==68399== by 0x18FE6B: AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) (../src/init.cpp:1809)
31==68399== by 0x17BA97: AppInit (../src/bitcoind.cpp:242)
32==68399== by 0x17BA97: main (???:283)
33==68399==
34{
35 <insert_a_suppression_name_here>
36 Memcheck:Value8
37 fun:_M_is_engaged
38 fun:operator=<node::BlockfileCursor>
39 fun:_ZN4node12BlockManager16LoadBlockIndexDBERKSt8optionalI7uint256E
40 fun:_ZN17ChainstateManager14LoadBlockIndexEv
41 fun:_ZN4nodeL32CompleteChainstateInitializationER17ChainstateManagerRKNS_21ChainstateLoadOptionsE
42 fun:_ZN4node14LoadChainstateER17ChainstateManagerRKN6kernel10CacheSizesERKNS_21ChainstateLoadOptionsE
43 fun:operator()
44 fun:operator()<(lambda at ../src/init.cpp:1392:45)>
45 fun:_ZL21InitAndLoadChainstateRN4node11NodeContextEbbRKN6kernel10CacheSizesERK11ArgsManager
46 fun:_Z11AppInitMainRN4node11NodeContextEPN10interfaces21BlockAndHeaderTipInfoE
47 fun:AppInit
48 fun:main
49}
50==68399==
51==68399== Exit program on first error (--exit-on-first-error=yes)
52************************
concept ACK
Could you please add a link to the bug for more context and split the change to commits that explain all the different changes. And it’s “temporary”, can we add a todo to the code to make it official :)?
And it’s “temporary”, can we add a todo to the code to make it official :)?
It is not clear what to do, so a tracking issue seems more appropriate than a vague todo.
Could you please add a link to the bug for more context and split the change to commits that explain all the different changes.
The upstream bugs are https://bugs.kde.org/show_bug.cgi?id=485276 and https://bugs.kde.org/show_bug.cgi?id=472329
This allows to run the test under valgrind:
./bld-cmake/test/functional/feature_dbcrash.py --timeout-factor=10 --valgrind
valgrind currently does not work on GCC -O2 compiled executables, which
contain std::optional use, due to an upstream bug. See
https://bugs.kde.org/show_bug.cgi?id=472329
One workaround could be to use -O1. However, that seems brittle, as
variantions of the bug were seen with -O1 as well.
So temporarily use clang in the valgrind CI tasks, because this also
allows to drop a false-positive suppression for:
-DCMAKE_CXX_FLAGS='-Wno-error=array-bounds'
Also, bump the container CI_IMAGE_NAME_TAG, while touching the config.
This bumps the valgrind version to be more recent:
https://packages.ubuntu.com/resolute/valgrind vs
https://packages.debian.org/trixie/valgrind
Also, update the comment in contrib/valgrind.supp to remove GCC and
aarch64:
* GCC wasn't tested with the suppressions file, due to the mentioned bug
* Clang-17 (or later) on aarch64 wasn't tested due to bug
https://github.com/bitcoin/bitcoin/issues/29635 and the minimum
supported clang version is clang-17 right now.
This means the only tested config right now is the one mentioned in the
suppression file:
"Tested on x86_64 with Ubuntu 26.04 system libs, using clang, without gui."