Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: Add CMake-based build system (5 of N) #13

Merged
merged 8 commits into from
Apr 19, 2023
Merged

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Apr 5, 2023

It was suggested:

It might be helpful to get some more of the depends interactions hooked up as a next step

Done in this PR.


The parent PR: bitcoin#25797.
The previous PRs in the staging branch: #5, #6, #7, #10.


EXAMPLES:

  1. Cross-compiling for Windows:
make -C depends HOST=x86_64-w64-mingw32 NO_QT=1 NO_WALLET=1
cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE=depends/x86_64-w64-mingw32/share/toolchain.cmake
cmake --build build
  1. Cross-compiling for macOS (arm64, x86_64):
make -C depends HOST=arm64-apple-darwin NO_QT=1 NO_WALLET=1
cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE=depends/arm64-apple-darwin/share/toolchain.cmake
cmake --build build
  1. Building on macOS, arm64:
  • essential build tools:
brew install cmake pkg-config
  • optional build tools:
brew install ccache
  • essential dependencies:
brew install boost libevent
  • optional dependencies:
brew install libnatpmp miniupnpc zeromq
  • configure and build:
cmake -S . -B build
cmake --build build
  1. Building on Windows:
choco install ccache --version=4.7.4
  • the vcpkg package manager is used to provide other dependencies:
vcpkg --triplet=x64-windows-static install boost-multi-index boost-signals2 libevent miniupnpc zeromq
  • configure and build:
cmake -S . -B build -DCMAKE_TOOLCHAIN_FILE=C:/vcpkg/scripts/buildsystems/vcpkg.cmake -DVCPKG_TARGET_TRIPLET=x64-windows-static
cmake --build build -- -property:UseMultiToolTask=true -maxCpuCount

NOTE

As always, make sure your build tree is clean :)

@hebasto
Copy link
Owner Author

hebasto commented Apr 5, 2023

Moved from #12 after rebasing from the master branch to new cmake-staging one.

Comments still need to be addressed:

@hebasto
Copy link
Owner Author

hebasto commented Apr 6, 2023

Reworked after testing on macOS.

PR description has been updated with an example for macOS.

A few notes for reviewers:

  1. Homebrew provides only recent CMake version, which is 3.26.3 currently.
  2. The recent CMake handles arm64-arch specific Homebrew's installation paths perfectly. No need to provide additional hints.

(I can't pinpoint the specific version of CMake that learnt this nice behavior first).

@hebasto
Copy link
Owner Author

hebasto commented Apr 6, 2023

@Sjors @jarolrod @stickies-v

Mind joining to testing this PR on macOS?

Copy link

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work fine on macOS arm64, following just the instructions from the PR description.

cmake -S . -B build
cmake --build build

resulted in

...
[100%] Building CXX object src/CMakeFiles/bitcoind.dir/bitcoind.cpp.o
[100%] Building CXX object src/CMakeFiles/bitcoind.dir/init/bitcoind.cpp.o
[100%] Linking CXX executable bitcoind
[100%] Built target bitcoind

Briefly ran ./build/src/bitcoind -signet and can't see any issues in the logs.

@Sjors
Copy link

Sjors commented Apr 6, 2023

From the title "5 of N" I'm not really sure what you're looking to test here.

Tried on Intel based macOS 13.3.

Very briefly checked that the resulting bitcoind runs.

Configure summary
=================
Executables:
  bitcoind ............................ ON
Optional packages:
  NAT-PMP ............................. ON
  UPnP ................................ ON
  ZeroMQ .............................. ON
  USDT tracing ........................ OFF

Cross compiling ....................... FALSE
Preprocessor defined macros ........... MAC_OSX
C compiler ............................ /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc
CFLAGS ................................ 
C++ compiler .......................... /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++
CXXFLAGS .............................. 
Common compile options ................ 
Common link options ................... 
Build type:
 - CMAKE_BUILD_TYPE ................... 
 - CFLAGS ............................. 
 - CXXFLAGS ........................... 
 - LDFLAGS for executables ............ 
 - LDFLAGS for shared libraries ....... 
Use assembly routines ................. ON
Use ccache for compiling .............. ON

This isn't showing options the wallet and GUI in the options - and it's also not building those. Is that expected?

How do I build in parallel? cmake --build build -j17 is not making my fans blow as much as I'm used to :-) (maybe because the wallet and GUI are not compiled)

@willcl-ark
Copy link

Also building successfully via ccache on an Intel i7 running on Ventura 13.1:


$ command -v clang
/usr/local/opt/ccache/libexec/clang

...

Configure summary
=================
Executables:
  bitcoind ............................ ON
Optional packages:
  NAT-PMP ............................. ON
  UPnP ................................ ON
  ZeroMQ .............................. ON
  USDT tracing ........................ OFF

...

[ 99%] Built target bitcoin_node
[100%] Building CXX object src/CMakeFiles/bitcoind.dir/init/bitcoind.cpp.o
[100%] Building CXX object src/CMakeFiles/bitcoind.dir/bitcoind.cpp.o
[100%] Linking CXX executable bitcoind
[100%] Built target bitcoind

Will take a closer review of the code shortly.

@Sjors yes this is just targetting bitcoind, now with UPnP, ZMQ and USDT (and ccache).

@hebasto
Copy link
Owner Author

hebasto commented Apr 6, 2023

How do I build in parallel? cmake --build build -j17 is not making my fans blow as much as I'm used to :-) (maybe because the wallet and GUI are not compiled)

Exactly like that. At least on Linux and macOS :)

@hebasto
Copy link
Owner Author

hebasto commented Apr 6, 2023

Updated 2fdfb82 -> 0b5ccaa (cmake13.02 -> cmake13.03, diff):

  • fixed check_evhttp_connection_get_peer macro
  • removed dependency on pkgconf package when building with MSVC

Comment on lines +11 to +22
if(MSVC)
# See https://github.com/ccache/ccache/wiki/MS-Visual-Studio
set(MSVC_CCACHE_WRAPPER_CONTENT "\"${CCACHE_EXECUTABLE}\" \"${CMAKE_CXX_COMPILER}\"")
set(MSVC_CCACHE_WRAPPER_FILENAME wrapped-cl.bat)
file(WRITE ${CMAKE_BINARY_DIR}/${MSVC_CCACHE_WRAPPER_FILENAME} "${MSVC_CCACHE_WRAPPER_CONTENT} %*")
set(CMAKE_VS_GLOBALS
"CLToolExe=${MSVC_CCACHE_WRAPPER_FILENAME}"
"CLToolPath=${CMAKE_BINARY_DIR}"
"TrackFileAccess=false"
"UseMultiToolTask=true"
"DebugInformationFormat=OldStyle"
)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently, this code does not work with cache 4.8 (

Going to investigate the issue, but I'm really hoping this won't be a blocker for this PR.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ccache 4.8 is broken, ccache 4.7.4, 4.7.5 work fine.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What/how is it broken in 4.8?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following https://github.com/ccache/ccache/wiki/MS-Visual-Studio literally for the simplest source code like "int main { return 0; }" works for 4.7, but 4.8 reports zero of "cacheable calls".

For example, https://cirrus-ci.com/task/6670364843966464.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this been reported upstream?

Copy link
Owner Author

@hebasto hebasto Apr 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this been reported upstream?

ccache/ccache#1262 is somewhat related.

@hebasto
Copy link
Owner Author

hebasto commented Apr 6, 2023

An example of building on Windows with MSVC has been added to the PR description.

@sipsorcery Want to look into this stuff?

@sipsorcery
Copy link

bitcoin_node is failing to build for me. Doesn't look like a cmake issue but I haven't seen that error on the main repo.

C:\dev\github\hebasto-bitcoin\src\httpserver.cpp(637,9): error C2664: 'void evhttp_connection_get_peer(evhttp_con
nection *,const char **,uint16_t *)': cannot convert argument 2 from 'char **' to 'const char **' [C:\dev\github\
hebasto-bitcoin\build\src\bitcoin_node.vcxproj]
C:\dev\github\hebasto-bitcoin\src\httpserver.cpp(637,49): message : Conversion loses qualifiers [C:\dev\github\he
basto-bitcoin\build\src\bitcoin_node.vcxproj]
C:\dev\github\vcpkg\installed\x64-windows-static\include\event2/http.h(1001,6): message : see declaration of 'evh
ttp_connection_get_peer' [C:\dev\github\hebasto-bitcoin\build\src\bitcoin_node.vcxproj]
C:\dev\github\hebasto-bitcoin\src\httpserver.cpp(637,9): message : while trying to match the argument list '(evht
tp_connection *, char **, uint16_t *)' [C:\dev\github\hebasto-bitcoin\build\src\bitcoin_node.vcxproj]

Win 11
msbuild --version
MSBuild version 17.4.1+9a89d02ff for .NET Framework
17.4.1.60106

@hebasto
Copy link
Owner Author

hebasto commented Apr 6, 2023

bitcoin_node is failing to build for me. Doesn't look like a cmake issue but I haven't seen that error on the main repo.


C:\dev\github\hebasto-bitcoin\src\httpserver.cpp(637,9): error C2664: 'void evhttp_connection_get_peer(evhttp_con

nection *,const char **,uint16_t *)': cannot convert argument 2 from 'char **' to 'const char **' [C:\dev\github\

hebasto-bitcoin\build\src\bitcoin_node.vcxproj]

C:\dev\github\hebasto-bitcoin\src\httpserver.cpp(637,49): message : Conversion loses qualifiers [C:\dev\github\he

basto-bitcoin\build\src\bitcoin_node.vcxproj]

C:\dev\github\vcpkg\installed\x64-windows-static\include\event2/http.h(1001,6): message : see declaration of 'evh

ttp_connection_get_peer' [C:\dev\github\hebasto-bitcoin\build\src\bitcoin_node.vcxproj]

C:\dev\github\hebasto-bitcoin\src\httpserver.cpp(637,9): message : while trying to match the argument list '(evht

tp_connection *, char **, uint16_t *)' [C:\dev\github\hebasto-bitcoin\build\src\bitcoin_node.vcxproj]

Win 11

msbuild --version

MSBuild version 17.4.1+9a89d02ff for .NET Framework

17.4.1.60106

Thanks for testing!

You didn't mention the branch you build, but I believe it is fixed in #13 (comment).

In the main repo see bitcoin#27335

@sipsorcery
Copy link

You didn't mention the branch you build,

This branch #13 commit ID 0b5ccaa.

I left out the use of ccache. In theory all it should do is make the build faster. We had a few issues with it on the msvc build around 2019/20 (could have been later) and eventually removed it. Or am I missing something? Does it provide different functionality for cmake?

@hebasto
Copy link
Owner Author

hebasto commented Apr 6, 2023

I left out the use of ccache. In theory all it should do is make the build faster. We had a few issues with it on the msvc build around 2019/20 (could have been later) and eventually removed it. Or am I missing something? Does it provide different functionality for cmake?

We use ccache in "Win64 native [vs2022]" CI task, and it is quite effective :)

@hebasto
Copy link
Owner Author

hebasto commented Apr 6, 2023

You didn't mention the branch you build,

This branch #13 commit ID 0b5ccaa.

Thank you! Will work on it.

@theuni
Copy link

theuni commented Apr 7, 2023

For those on platforms where CMake isn't readily available (basically everywhere but Linux), I've hooked up a native cmake build to depends: theuni@cb2a9d6

Unfortunately I had to patch CMake in order to be able to build with CC/CXX vars which contain spaces (simplest example: gcc -m32, but our osx cmdlines are more complicated). I'll work on upstreaming those patches next week.

@hebasto
Copy link
Owner Author

hebasto commented Apr 9, 2023

For those on platforms where CMake isn't readily available (basically everywhere but Linux)...

On macOS, CMake is available via Homebrew. That is exactly the same way how we install automake and libtool now.

On Windows, CMake is available via MSVC Build Tools installation.

@hebasto
Copy link
Owner Author

hebasto commented Apr 9, 2023

@sipsorcery

bitcoin_node is failing to build for me.

Fixed in the recent push.

@hebasto
Copy link
Owner Author

hebasto commented Apr 10, 2023

Moved from #12 after rebasing from the master branch to new cmake-staging one.

Comments still need to be addressed:

@theuni

Why not do a compile test like configure.ac? I think that would be more straightforward?

FWIW, the same approach is used in CMake's builtin modules. For example, in FindPNG.cmake:

      file(STRINGS "${PNG_PNG_INCLUDE_DIR}/png.h" png_version_str REGEX "^#define[ \t]+PNG_LIBPNG_VER_STRING[ \t]+\".+\"")

      string(REGEX REPLACE "^#define[ \t]+PNG_LIBPNG_VER_STRING[ \t]+\"([^\"]+)\".*" "\\1" PNG_VERSION_STRING "${png_version_str}")
      unset(png_version_str)

Tbh, I'm not sure of the rationale for choosing between the two mentioned approaches.

UPD. A Find Module is expected to set ${Package}_VERSION* variables. But a compile test is able just to check the version.

Copy link

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 127a132

I reviewed the code and checked that NAT-PMP, UPnP and ZeroMQ are properly detected and linked against (on FreeBSD). Tracepoints (USDT) are not detected even though sys/sdt.h is available in /usr/include, this is the same as with autotools (unrelated to the cmake effort).

@vasild
Copy link

vasild commented Apr 12, 2023

I think that following 18fb363 and 00e9b97 in master, this is needed:

diff --git i/src/util/CMakeLists.txt w/src/util/CMakeLists.txt
index f11795c4d7..40f1059892 100644
--- i/src/util/CMakeLists.txt
+++ w/src/util/CMakeLists.txt
@@ -7,12 +7,14 @@ add_library(bitcoin_util STATIC EXCLUDE_FROM_ALL
   bip32.cpp
   bytevectorhash.cpp
   check.cpp
   error.cpp
   exception.cpp
   fees.cpp
+  fs.cpp
+  fs_helpers.cpp
   getuniquepath.cpp
   hasher.cpp
   message.cpp
   moneystr.cpp
   rbf.cpp
   readwritefile.cpp
@@ -29,13 +31,12 @@ add_library(bitcoin_util STATIC EXCLUDE_FROM_ALL
   threadinterrupt.cpp
   threadnames.cpp
   time.cpp
   tokenpipe.cpp
   ../chainparamsbase.cpp
   ../clientversion.cpp
-  ../fs.cpp
   ../logging.cpp
   ../random.cpp
   ../randomenv.cpp
   ../support/cleanse.cpp
   ../support/lockedpool.cpp
   ../sync.cpp

(I merged this PR@127a1329f0da038d98aaea74bafef6d8b679fc80 into master@cae0608ad4b195652e0cfcc905e19b50197d43ab and tried to compile)

@hebasto
Copy link
Owner Author

hebasto commented Apr 12, 2023

@vasild

I think that following 18fb363 and 00e9b97 in master, this is needed

This step will be done separately. See previous ones: #8, #11.

@theuni
Copy link

theuni commented Apr 12, 2023

Homebrew provides only recent CMake version, which is 3.26.3 currently.

So is the plan to have different CMake version requirements for different platforms? I'm not exactly opposed, but if we're going to do that I think the rationale and dependencies should be well-documented.

I can't say I love the idea though, as it introduces the notion of one platform's builds being potentially "superior" to another.

@@ -42,6 +42,8 @@ option(BUILD_DAEMON "Build bitcoind executable." ON)
option(ASM "Use assembly routines." ON)
cmake_dependent_option(CXX20 "Enable compilation in C++20 mode." OFF "NOT MSVC" ON)
option(THREADLOCAL "Enable features that depend on the C++ thread_local keyword (currently just thread names in debug logs)." ON)
include(TristateOption)
tristate_option(CCACHE "Use ccache for compiling." "if ccache is found." AUTO)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From @fanquake on #12:

Yea. We certainly wont be adopting this tri-state logic/machinery into our CMake build system long-term/at-merge-time. I'd consider it a regression to reimplement this Autotools behaviour. As for disabling certain "features" by default, yes, I think that is what we are going to do. It continues to make less & less sense to compile in stuff like this by default, let alone auto-opt-in builders to it, based on what happens to be installed on their systems at the time of compilation.

At this point I agree with that and I'd prefer to just never start with this tri-state stuff as it's quite not-cmake-like. But I realize I originally asked for 1:1 feature parity with autotools for the sake of review, so it's not really fair to ask for that change now.

How about a comment above with a loud warning that says "These tri-state options will be removed and most features will become opt-in by default before merging into master" ?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +11 to +22
if(MSVC)
# See https://github.com/ccache/ccache/wiki/MS-Visual-Studio
set(MSVC_CCACHE_WRAPPER_CONTENT "\"${CCACHE_EXECUTABLE}\" \"${CMAKE_CXX_COMPILER}\"")
set(MSVC_CCACHE_WRAPPER_FILENAME wrapped-cl.bat)
file(WRITE ${CMAKE_BINARY_DIR}/${MSVC_CCACHE_WRAPPER_FILENAME} "${MSVC_CCACHE_WRAPPER_CONTENT} %*")
set(CMAKE_VS_GLOBALS
"CLToolExe=${MSVC_CCACHE_WRAPPER_FILENAME}"
"CLToolPath=${CMAKE_BINARY_DIR}"
"TrackFileAccess=false"
"UseMultiToolTask=true"
"DebugInformationFormat=OldStyle"
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this been reported upstream?

@@ -57,3 +57,29 @@ if(WITH_MINIUPNPC)
message(FATAL_ERROR "libminiupnpc requested, but not found.")
endif()
endif()

if(WITH_ZMQ)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zmq started shipping a cmake config file starting with v4.2.2.

Once we bump our zmq requirement in the future, we'll be able to use find_package(ZeroMQ). Mind adding a note?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zmq started shipping a cmake config file starting with v4.2.2.

But not Ubuntu's libzmq3-dev packages, unfortunately.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, mind adding a note that it's supported upstream and we'll be able to switch at some point in the future?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, mind adding a note that it's supported upstream and we'll be able to switch at some point in the future?

Sure. I'll add it.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 8ee7537

Comment on lines +46 to +47
# TODO: These tri-state options will be removed and most features
# will become opt-in by default before merging into master.
Copy link

@vasild vasild Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "opt-in by default" mean? That there will be just two options to choose from (not 3): ON and OFF, that it will be ON by default and (it follows that) if the corresponding library is not found, then it will result in an error?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "opt-in by default" mean? .... that it will be ON by default

No. It means that basically all features/options will be OFF by default, and builders will have to actively opt-in to what they want. If they opt-in to something, and do not have the required libs/prerequisites, the build will error.

@hebasto
Copy link
Owner Author

hebasto commented Apr 17, 2023

Friendly ping @TheCharlatan @theuni :)

Copy link

@theuni theuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 8ee7537

Mind responding to my question above though? And maybe adding a comment or two:

Homebrew provides only recent CMake version, which is 3.26.3 currently.

So is the plan to have different CMake version requirements for different platforms? I'm not exactly opposed, but if we're going to do that I think the rationale and dependencies should be well-documented.

I can't say I love the idea though, as it introduces the notion of one platform's builds being potentially "superior" to another.

@hebasto
Copy link
Owner Author

hebasto commented Apr 18, 2023

@theuni

Homebrew provides only recent CMake version, which is 3.26.3 currently.

So is the plan to have different CMake version requirements for different platforms? I'm not exactly opposed, but if we're going to do that I think the rationale and dependencies should be well-documented.

I can't say I love the idea though, as it introduces the notion of one platform's builds being potentially "superior" to another.

We always do not stipulate any particular version of any build tool, except for release binaries in the Guix build environment. Only minimum required versions are specified, including Autoconf and Automake. I see no reasons why CMake-based build system should restrict the user's choice. Anyway the latest CMake version is available for any supported build platform.

Besides, CMake has policies, which in turn:

are used to preserve backward compatible behavior across multiple releases.

Our usage of policies is self-documented in the cmake_minimum_required() command:

cmake_minimum_required(VERSION 3.13...3.24)

And maybe adding a comment or two

I'll be happy to add comments to the code. Mind suggesting a good wording?

@theuni
Copy link

theuni commented Apr 18, 2023

@hebasto I think it'd be helpful to use code to document: if possible, conditionally set cmake_minimum_required per-platform. So, specifically, if the builder is arm64 && macos, set the minimum version as required for that platform.

If it's not possible to set conditionally, I think that would be the place to add the comment.

@hebasto
Copy link
Owner Author

hebasto commented Apr 18, 2023

I think it'd be helpful to use code to document: if possible, conditionally set cmake_minimum_required per-platform. So, specifically, if the builder is arm64 && macos, set the minimum version as required for that platform.

If it's not possible to set conditionally, I think that would be the place to add the comment.

Sorry, but I'm confused. At this stage, how an arm64 && macos builder platform differs from others?

conditionally set cmake_minimum_required per-platform

What platforms need such conditions? I'll try to implement them.

@theuni
Copy link

theuni commented Apr 19, 2023

I was going by your comment above:

A few notes for reviewers:

1. Homebrew [provides](https://formulae.brew.sh/formula/cmake) only recent CMake version, which is 3.26.3 currently.

2. The recent CMake handles `arm64`-arch specific Homebrew's installation paths perfectly. No need to provide additional hints.

(I can't pinpoint the specific version of CMake that learnt this nice behavior first).

To me that implied that CMake 3.13 isn't smart enough for M1's, so a more recent CMake is required in order to successfully build.

But now, re-reading, maybe your point was that CMake works fine for m1's except for homebrew paths, but that's not a problem because there's no available homebrew CMake version which doesn't handle those paths correctly. Is that right?

So to clarify my specific question: Should an m1 builder using CMake 3.13 be able to build successfully using depends?

If that's the case, I've misunderstood and will shut up :)

@hebasto
Copy link
Owner Author

hebasto commented Apr 19, 2023

@theuni

Thanks for clarification!

But now, re-reading, maybe your point was that CMake works fine for m1's except for homebrew paths, but that's not a problem because there's no available homebrew CMake version which doesn't handle those paths correctly. Is that right?

Right. But I did not check older CMake versions on macOS M1.

Should an m1 builder using CMake 3.13 be able to build successfully using depends?

I have to investigate this case.

Can we proceed with this PR for now? Going to get the staging branch rebased again :)

@theuni
Copy link

theuni commented Apr 19, 2023

ACK, let's keep things moving :)

@theuni
Copy link

theuni commented Apr 19, 2023

Note that if we do find that newer cmake is required, this is what I had in mind for documenting it via code:

if(CMAKE_HOST_APPLE AND CMAKE_HOST_SYSTEM_PROCESSOR STREQUAL "arm64")
  cmake_minimum_required(VERSION 3.xy)
else()
  cmake_minimum_required(VERSION 3.13)
endif()

But cmake_minimum_required is weird and special, so I'm not sure if that's allowed.

Edit: I suppose we could do:

cmake_minimum_required(VERSION 3.13)
if(CMAKE_HOST_APPLE AND CMAKE_HOST_SYSTEM_PROCESSOR STREQUAL "arm64" AND CMAKE_VERSION VERSION_LESS 3.xy)
  message(FATAL_ERROR "Newer CMake Required")
endif()

Also... considering that CMAKE_APPLE_SILICON_PROCESSOR was added in 3.19.2, I wouldn't be surprised if CMake gained some machinery that helps with arm builds around that time.

@theuni
Copy link

theuni commented Apr 19, 2023

ACK, let's keep things moving :)

Once merged and rebased, I'll adapt my cmake-depends branch to build 3.13 instead (that'll be more useful anyway), and see if it works on my m1.

@hebasto hebasto merged commit 2428f6c into cmake-staging Apr 19, 2023
@hebasto
Copy link
Owner Author

hebasto commented Apr 19, 2023

Also... considering that CMAKE_APPLE_SILICON_PROCESSOR was added in 3.19.2, I wouldn't be surprised if CMake gained some machinery that helps with arm builds around that time.

Right. CMake 3.19.2 is also the first version which Apple Silicon CMake binaries are available for.

@theuni
Copy link

theuni commented Apr 19, 2023

Good news: a self-built CMake 3.13 from depends seems to have no trouble building cmake-staging.

Also something fun I discovered along the way from zmq:

if(${CMAKE_SYSTEM_NAME} STREQUAL Darwin)
  cmake_minimum_required(VERSION 3.0.2)
else()
  cmake_minimum_required(VERSION 2.8.12)
endif()

So if we ever do end up in a case where we have different version requirements, we can enforce that programmatically. Neat :)

@TheCharlatan
Copy link

Post-merge ACK 8ee7537

hebasto pushed a commit that referenced this pull request May 2, 2023
f952e67 ci: remove usage of untrusted bpfcc-tools (fanquake)
1232c2f ci: use LLVM/clang-16 in native_asan job (fanquake)

Pull request description:

  Similar to bitcoin#27298. Working for me on `x86_64` and solves the issue I currently see with TSAN on `aarch64` with master (6882828):
  ```bash
  crc32c/src/crc32c_arm64.cc:101:26: runtime error: load of misaligned address 0xffff84400406 for type 'uint64_t' (aka 'unsigned long'), which requires 8 byte alignment
  0xffff84400406: note: pointer points here
   b9 c5 22 00 01 01  1a 6c 65 76 65 6c 64 62  2e 42 79 74 65 77 69 73  65 43 6f 6d 70 61 72 61  74 6f
               ^
      #0 0xaaaaaddaf0b4 in crc32c::ExtendArm64(unsigned int, unsigned char const*, unsigned long) src/./src/crc32c/src/crc32c_arm64.cc:101:26
      #1 0xaaaaadd2c838 in leveldb::crc32c::Value(char const*, unsigned long) src/./leveldb/util/crc32c.h:20:60
      #2 0xaaaaadd2c838 in leveldb::log::Reader::ReadPhysicalRecord(leveldb::Slice*) src/./src/leveldb/db/log_reader.cc:246:29
      #3 0xaaaaadd2ba9c in leveldb::log::Reader::ReadRecord(leveldb::Slice*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*) src/./src/leveldb/db/log_reader.cc:72:38
      #4 0xaaaaadd41710 in leveldb::VersionSet::Recover(bool*) src/./src/leveldb/db/version_set.cc:910:19
      #5 0xaaaaadcf9fec in leveldb::DBImpl::Recover(leveldb::VersionEdit*, bool*) src/./src/leveldb/db/db_impl.cc:320:18
      #6 0xaaaaadd12068 in leveldb::DB::Open(leveldb::Options const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, leveldb::DB**) src/./src/leveldb/db/db_impl.cc:1487:20
      #7 0xaaaaad314e80 in CDBWrapper::CDBWrapper(DBParams const&) src/./src/dbwrapper.cpp:156:30
      #8 0xaaaaace94880 in CBlockTreeDB::CBlockTreeDB(DBParams const&) src/./txdb.h:89:23
      #9 0xaaaaace94880 in std::_MakeUniq<CBlockTreeDB>::__single_object std::make_unique<CBlockTreeDB, DBParams>(DBParams&&) /usr/bin/../lib/gcc/aarch64-linux-gnu/11/../../../../include/c++/11/bits/unique_ptr.h:962:34
      #10 0xaaaaace94880 in ChainTestingSetup::ChainTestingSetup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<char const*, std::allocator<char const*> > const&) src/./src/test/util/setup_common.cpp:188:51
      #11 0xaaaaace95da0 in TestingSetup::TestingSetup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<char const*, std::allocator<char const*> > const&, bool, bool) src/./src/test/util/setup_common.cpp:243:7
      #12 0xaaaaace96730 in TestChain100Setup::TestChain100Setup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<char const*, std::allocator<char const*> > const&, bool, bool) src/./src/test/util/setup_common.cpp:274:7
      #13 0xaaaaac1ddbc8 in blockfilter_index_tests::BuildChainTestingSetup::BuildChainTestingSetup() src/./src/test/blockfilter_index_tests.cpp:26:8
      #14 0xaaaaac1ddbc8 in blockfilter_index_tests::blockfilter_index_initial_sync::blockfilter_index_initial_sync() src/./src/test/blockfilter_index_tests.cpp:112:1
      #15 0xaaaaac1ddbc8 in blockfilter_index_tests::blockfilter_index_initial_sync_invoker() src/./src/test/blockfilter_index_tests.cpp:112:1
      #16 0xaaaaabf08f7c in boost::function0<void>::operator()() const /usr/include/boost/function/function_template.hpp:763:14
      #17 0xaaaaabf95468 in boost::detail::forward::operator()() /usr/include/boost/test/impl/execution_monitor.ipp:1388:32
      #18 0xaaaaabf95468 in boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) /usr/include/boost/function/function_template.hpp:137:18
      #19 0xaaaaabf8e12c in boost::function0<int>::operator()() const /usr/include/boost/function/function_template.hpp:763:14
      #20 0xaaaaabe7be14 in boost::execution_monitor::catch_signals(boost::function<int ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:903:16
      #21 0xaaaaabe7c1c0 in boost::execution_monitor::execute(boost::function<int ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:1301:16
      #22 0xaaaaabe6f47c in boost::execution_monitor::vexecute(boost::function<void ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:1397:5
      #23 0xaaaaabe75124 in boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned long) /usr/include/boost/test/impl/unit_test_monitor.ipp:49:9
      #24 0xaaaaabed19fc in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /usr/include/boost/test/impl/framework.ipp:815:44
      #25 0xaaaaabed0f6c in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /usr/include/boost/test/impl/framework.ipp:784:58
      #26 0xaaaaabed0f6c in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /usr/include/boost/test/impl/framework.ipp:784:58
      #27 0xaaaaabe73878 in boost::unit_test::framework::run(unsigned long, bool) /usr/include/boost/test/impl/framework.ipp:1721:29
      #28 0xaaaaabe9d244 in boost::unit_test::unit_test_main(boost::unit_test::test_suite* (*)(int, char**), int, char**) /usr/include/boost/test/impl/unit_test_main.ipp:250:9
      #29 0xffff8f0773f8  (/lib/aarch64-linux-gnu/libc.so.6+0x273f8) (BuildId: f37f3aa07c797e333fd106472898d361f71798f5)
      #30 0xffff8f0774c8 in __libc_start_main (/lib/aarch64-linux-gnu/libc.so.6+0x274c8) (BuildId: f37f3aa07c797e333fd106472898d361f71798f5)
      #31 0xaaaaabda55ac in _start (/home/fedora/ci_scratch/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/test_bitcoin+0x10e55ac) (BuildId: b7909adaefd9db6cd6a7c4d3d40207cf6bdaf4b3)

  SUMMARY: UndefinedBehaviorSanitizer: misaligned-pointer-use crc32c/src/crc32c_arm64.cc:101:26 in
  ```

ACKs for top commit:
  dergoegge:
    utACK f952e67
  MarcoFalke:
    lgtm ACK f952e67

Tree-SHA512: 9dee2abf73d3f23bb9979bfb453b48e39f0b7a5f58d43824ecf053a53e9800ed413b915382b274d1a84baf2999683e3b485463e377e0455b3f0ead65ed1d1916
hebasto pushed a commit that referenced this pull request Jun 23, 2023
682274a ci: install llvm-symbolizer in MSAN jobs (fanquake)
96527cd ci: use LLVM 16.0.6 in MSAN jobs (fanquake)

Pull request description:

  Fixes: bitcoin#27737 (comment).

  Tested (locally) with bitcoin#27495 that it produces a symbolized backtrace:
  ```bash
  2023-06-20T17:5Uninitialized bytes in __interceptor_strlen at offset 113 inside [0x719000006908, 114)
  ==35429==WARNING: MemorySanitizer: use-of-uninitialized-value
      #0 0x56060fae8c4b in sqlite3Strlen30 /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:32670:28
      #1 0x56060fb0fcf4 in sqlite3PagerOpen /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:57953:17
      #2 0x56060fb0f48b in sqlite3BtreeOpen /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:68679:10
      #3 0x56060fb01384 in openDatabase /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:171911:8
      #4 0x56060fb016ca in sqlite3_open_v2 /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:172034:10
      #5 0x56060e8a94db in wallet::SQLiteDatabase::Open() src/wallet/sqlite.cpp:250:19
      #6 0x56060e8a30fd in wallet::SQLiteDatabase::SQLiteDatabase(fs::path const&, fs::path const&, wallet::DatabaseOptions const&, bool) src/wallet/sqlite.cpp:133:9
      #7 0x56060e8b78f5 in std::__1::__unique_if<wallet::SQLiteDatabase>::__unique_single std::__1::make_unique[abi:v160006]<wallet::SQLiteDatabase, std::__1::__fs::filesystem::path, fs::path&, wallet::DatabaseOptions const&>(std::__1::__fs::filesystem::path&&, fs::path&, wallet::DatabaseOptions const&) /home/ubuntu/ci_scratch/ci/scratch/msan/cxx_build/include/c++/v1/__memory/unique_ptr.h:686:30
      #8 0x56060e8b5240 in wallet::MakeSQLiteDatabase(fs::path const&, wallet::DatabaseOptions const&, wallet::DatabaseStatus&, bilingual_str&) src/wallet/sqlite.cpp:641:19
      #9 0x56060e83560b in wallet::MakeDatabase(fs::path const&, wallet::DatabaseOptions const&, wallet::DatabaseStatus&, bilingual_str&) src/wallet/walletdb.cpp:1261:16
      #10 0x56060e7546e9 in wallet::MakeWalletDatabase(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, wallet::DatabaseOptions const&, wallet::DatabaseStatus&, bilingual_str&) src/wallet/wallet.cpp:2905:12
      #11 0x56060e4bc03f in wallet::TestLoadWallet(wallet::WalletContext&) src/wallet/test/util.cpp:68:21
      #12 0x56060e349ad4 in wallet::wallet_tests::ZapSelectTx::test_method() src/wallet/test/wallet_tests.cpp:897:19
      #13 0x56060e348598 in wallet::wallet_tests::ZapSelectTx_invoker() src/wallet/test/wallet_tests.cpp:891:1
      #14 0x56060cfec325 in boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:117:11
      #15 0x56060ced3a7e in boost::function0<void>::operator()() const /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:763:14
      #16 0x56060ced3a7e in boost::detail::forward::operator()() /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:1388:32
      #17 0x56060ced3a7e in boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:137:18
      #18 0x56060cda71c2 in boost::function0<int>::operator()() const /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:763:14
      #19 0x56060cda71c2 in int boost::detail::do_invoke<boost::shared_ptr<boost::detail::translator_holder_base>, boost::function<int ()>>(boost::shared_ptr<boost::detail::translator_holder_base> const&, boost::function<int ()> const&) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:301:30
      #20 0x56060cda71c2 in boost::execution_monitor::catch_signals(boost::function<int ()> const&) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:903:16
      #21 0x56060cda784a in boost::execution_monitor::execute(boost::function<int ()> const&) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:1301:16
      #22 0x56060cd9ec3a in boost::execution_monitor::vexecute(boost::function<void ()> const&) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:1397:5
      #23 0x56060cd9ec3a in boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned long) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/unit_test_monitor.ipp:49:9
      #24 0x56060ce1a07b in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/framework.ipp:815:44
      #25 0x56060ce1ad8b in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/framework.ipp:784:58
      #26 0x56060ce1ad8b in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/framework.ipp:784:58
      #27 0x56060cd9b8de in boost::unit_test::framework::run(unsigned long, bool) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/framework.ipp:1722:29
      #28 0x56060cdd4fac in boost::unit_test::unit_test_main(boost::unit_test::test_suite* (*)(int, char**), int, char**) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/unit_test_main.ipp:250:9
      #29 0x56060cdd6094 in main /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/unit_test_main.ipp:306:12
      #30 0x7f7379691d8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
      #31 0x7f7379691e3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
      #32 0x56060cce2e24 in _start (/home/ubuntu/ci_scratch/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/test_bitcoin+0x188e24)

    Uninitialized value was created by a heap allocation
      #0 0x56060cd163f2 in malloc /ci_base_install/ci/scratch/msan/llvm-project/compiler-rt/lib/msan/msan_interceptors.cpp:934:3
      #1 0x56060fc10069 in sqlite3MemMalloc /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:25163:7
      #2 0x56060fb063bc in mallocWithAlarm /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:28846:7
      #3 0x56060fae4eb9 in sqlite3Malloc /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:28876:5
      #4 0x56060faf9e19 in sqlite3DbMallocRaw /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:29176:7
      #5 0x56060fb0fc67 in sqlite3PagerOpen /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:57938:17
      #6 0x56060fb0f48b in sqlite3BtreeOpen /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:68679:10
      #7 0x56060fb01384 in openDatabase /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:171911:8
      #8 0x56060fb016ca in sqlite3_open_v2 /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:172034:10
      #9 0x56060e8a94db in wallet::SQLiteDatabase::Open() src/wallet/sqlite.cpp:250:19
      #10 0x56060e8a30fd in wallet::SQLiteDatabase::SQLiteDatabase(fs::path const&, fs::path const&, wallet::DatabaseOptions const&, bool) src/wallet/sqlite.cpp:133:9
      #11 0x56060e8b78f5 in std::__1::__unique_if<wallet::SQLiteDatabase>::__unique_single std::__1::make_unique[abi:v160006]<wallet::SQLiteDatabase, std::__1::__fs::filesystem::path, fs::path&, wallet::DatabaseOptions const&>(std::__1::__fs::filesystem::path&&, fs::path&, wallet::DatabaseOptions const&) /home/ubuntu/ci_scratch/ci/scratch/msan/cxx_build/include/c++/v1/__memory/unique_ptr.h:686:30
      #12 0x56060e8b5240 in wallet::MakeSQLiteDatabase(fs::path const&, wallet::DatabaseOptions const&, wallet::DatabaseStatus&, bilingual_str&) src/wallet/sqlite.cpp:641:19
      #13 0x56060e83560b in wallet::MakeDatabase(fs::path const&, wallet::DatabaseOptions const&, wallet::DatabaseStatus&, bilingual_str&) src/wallet/walletdb.cpp:1261:16
      #14 0x56060e7546e9 in wallet::MakeWalletDatabase(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, wallet::DatabaseOptions const&, wallet::DatabaseStatus&, bilingual_str&) src/wallet/wallet.cpp:2905:12
      #15 0x56060e4bc03f in wallet::TestLoadWallet(wallet::WalletContext&) src/wallet/test/util.cpp:68:21
      #16 0x56060e349ad4 in wallet::wallet_tests::ZapSelectTx::test_method() src/wallet/test/wallet_tests.cpp:897:19
      #17 0x56060e348598 in wallet::wallet_tests::ZapSelectTx_invoker() src/wallet/test/wallet_tests.cpp:891:1
      #18 0x56060cfec325 in boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:117:11
      #19 0x56060ced3a7e in boost::function0<void>::operator()() const /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:763:14
      #20 0x56060ced3a7e in boost::detail::forward::operator()() /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:1388:32
      #21 0x56060ced3a7e in boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:137:18

  SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:32670:28 in sqlite3Strlen30
  ```

  as opposed to unsymbolized: https://cirrus-ci.com/task/6005512018329600?logs=ci#L3245.

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 682274a

Tree-SHA512: 8f3e7636761c956537a472989bf07529f5afbd988c5e7e1f07ece8b2599608fa4fe9e1efdc6e302cf0f7f44dec3cf9a3c1e68b758af81a8a8b476a43d3220807
hebasto added a commit that referenced this pull request Jul 7, 2023
43123cf FIXUP: Same as PR27458 (Hennadii Stepanov)
2d8930e FIXUP: Same as PR27656 (Hennadii Stepanov)
a112470 cmake: Include CTest (Hennadii Stepanov)
cb19814 cmake: Build `test_bitcoin` executable (Hennadii Stepanov)
751453f cmake: Build `bench_bitcoin` executable (Hennadii Stepanov)
a2c3493 cmake: Add test config and runners (Hennadii Stepanov)
cb7dc94 test: Explicitly specify directory where to search tests for (Hennadii Stepanov)
2fd303f test: Make `util/test_runner.py` honor `BITCOINUTIL` and `BITCOINTX` (Hennadii Stepanov)
2e3721e cmake: Add wallet functionality (Hennadii Stepanov)
f944ccd cmake: Build `bitcoin-util` executable (Hennadii Stepanov)
d1c319d cmake: Build `bitcoin-tx` executable (Hennadii Stepanov)
1934755 cmake: Build `bitcoin-cli` executable (Hennadii Stepanov)
5fc2cee [FIXUP] cmake: Add workaround for https://gitlab.kitware.com/cmake/cmake/-/issues/20652 (Hennadii Stepanov)
b2bea9f [FIXUP] cmake: Consider `ASM` option when checking for `HAVE_64BIT_ASM` (Hennadii Stepanov)

Pull request description:

  The parent PR: bitcoin#25797.
  The previous PRs in the staging branch: #5, #6, #7, #10, #13.

  ---

  What is NEW:
  - `bitcoin-cli`
  - `bitcoin-tx`
  - `bitcoin-util`
  - wallet functionality and `bitcoin-wallet`
  - `bench_bitcoin`
  - `test_bitcoin`

  EXAMPLES:
  ```
  cmake -S . -B build
  cd build
  cmake --build . -j $(nproc)
  ctest -j $(nproc)
  ./test/functional/test_runner.py -j $(nproc)
  ```

  Using a multi-configuration generator (CMake 3.17+ is required):
  ```
  cmake -S . -B build -G "Ninja Multi-Config"
  cd build
  cmake --build . -j $(nproc)
  ctest -j $(nproc) -C Debug
  ```
  ---

  What to test:
  - old CMake versions, for example v3.13
  - multi-config generators, for example `-G "Ninja Multi-Config"`

  ---

  What to consider additionally:
  - is it worth to pull the "[test: Make util/test_runner.py honor BITCOINUTIL and BITCOINTX](268aca3)" commit to the main repo now?

  FWIW, we use the same approach for functional tests providing the `BITCOIND` environment variable when needed.

ACKs for top commit:
  TheCharlatan:
    ACK 43123cf

Tree-SHA512: a04cfca266bcde780528c915cb01f69189f36a0e1beb521fe75e4816ca4f9ab9440646529bbf2cbdfe76275debc5107ee2433e1027405094d3e8a74ec0d1d077
hebasto added a commit that referenced this pull request Jul 20, 2023
a933ddf [FIXUP] Better document a workaround (Hennadii Stepanov)
769633e [FIXUP] for "cmake: Add wallet functionality" (Hennadii Stepanov)
31e4e62 [FIXUP] for "cmake: Build `test_bitcoin` executable" (Hennadii Stepanov)
f2dbb17 [FIXUP] for "cmake: Build `test_bitcoin` executable" (Hennadii Stepanov)
91d7327 [FIXUP] Boost (Hennadii Stepanov)

Pull request description:

  The parent PR: bitcoin#25797.
  The previous PRs in the staging branch: #5, #6, #7, #10, #13, #15.

  This PR consists of fixups only to make reviewing easier :)

ACKs for top commit:
  theuni:
    ACK a933ddf

Tree-SHA512: 270869a3bf9b9f2d56b75d169f25032385fa2d1297951a23c0d1900d9668a40b153c7a5d9b51fa87596eec681107c40da8e55f405d28a7ecd2ec0f72f3550bbf
hebasto added a commit that referenced this pull request Jul 20, 2023
ac7bc5b [FIXUP] Rename CCACHE_EXECUTABLE --> CCACHE_COMMAND for consistency (Hennadii Stepanov)
0476509 [FIXUP] Learn to work with recent ccache in MSVC builds (Hennadii Stepanov)
2fd67c7 [FIXUP] Do not disable `TrackFileAccess` in MSVC builds (Hennadii Stepanov)
a53ae12 [FIXUP] Use Multi-ToolTask in MSVC builds by default (Hennadii Stepanov)

Pull request description:

  The parent PR: bitcoin#25797.
  The previous PRs in the staging branch: #5, #6, #7, #10, #13, #15, #17, #18.

  This PR consists of fixups related to using [Ccache](https://ccache.dev/) in MSVC builds.

Top commit has no ACKs.

Tree-SHA512: dc01202b054aaeb5b46607bc93126bee0df523df3b4f2df54b566b2e2d6306d784a723fd4a999d0d84fdf31e926a72304790f94b9d57034db0c112ee9f52070e
hebasto added a commit that referenced this pull request Sep 13, 2023
a65da0d cmake: Redefine configuration flags (Hennadii Stepanov)
1a1dda5 [FIXUP] Evaluate flags set in depends _after_ config-specific flags (Hennadii Stepanov)
482e844 cmake: Warn about not encapsulated build properties (Hennadii Stepanov)
3736470 cmake: Add platform-specific flags (Hennadii Stepanov)
e0621e9 cmake: Add `TryAppendLinkerFlag` module (Hennadii Stepanov)
ae430cf cmake: Add `TryAppendCXXFlags` module (Hennadii Stepanov)
7903bd5 [FIXUP] Encapsulate common build flags into `core` interface library (Hennadii Stepanov)

Pull request description:

  The parent PR: bitcoin#25797.
  The previous PRs in the staging branch: #5, #6, #7, #10, #13, #15, #17, #19.

  ---

  What is NEW:
  - functions for checking compiler and linker flags
  - managing flags for different build types (configurations)

  EXAMPLES of configuration output on Ubuntu 22.04:

  -  for a single-config generator:
  ```
  $ cmake .. -G "Unix Makefiles"
  ...
  Cross compiling ....................... FALSE
  Preprocessor defined macros ...........
  C compiler ............................ /usr/bin/cc
  CFLAGS ................................
  C++ compiler .......................... /usr/bin/c++
  CXXFLAGS ..............................
  Common compile options ................
  Common link options ...................
  Linker flags for executables ..........
  Linker flags for shared libraries .....
  Build type (configuration):
   - CMAKE_BUILD_TYPE ................... RelWithDebInfo
   - Preprocessor defined macros ........
   - CFLAGS ............................. -O2 -g
   - CXXFLAGS ........................... -O2 -g
   - LDFLAGS for executables ............
   - LDFLAGS for shared libraries .......
  Use assembly routines ................. ON
  Use ccache for compiling .............. ON
  ...
  ```

  - for a multi-config generator:
  ```
  $ cmake .. -G "Ninja Multi-Config"
  ...
  Cross compiling ....................... FALSE
  Preprocessor defined macros ...........
  C compiler ............................ /usr/bin/cc
  CFLAGS ................................
  C++ compiler .......................... /usr/bin/c++
  CXXFLAGS ..............................
  Common compile options ................
  Common link options ...................
  Linker flags for executables ..........
  Linker flags for shared libraries .....
  Available build types (configurations)  RelWithDebInfo Debug Release
  'RelWithDebInfo' build type (configuration):
   - Preprocessor defined macros ........
   - CFLAGS ............................. -O2 -g
   - CXXFLAGS ........................... -O2 -g
   - LDFLAGS for executables ............
   - LDFLAGS for shared libraries .......
  'Debug' build type (configuration):
   - Preprocessor defined macros ........ DEBUG DEBUG_LOCKORDER DEBUG_LOCKCONTENTION RPC_DOC_CHECK ABORT_ON_FAILED_ASSUME
   - CFLAGS ............................. -O0 -g3
   - CXXFLAGS ........................... -O0 -g3 -ftrapv
   - LDFLAGS for executables ............
   - LDFLAGS for shared libraries .......
  'Release' build type (configuration):
   - Preprocessor defined macros ........
   - CFLAGS ............................. -O2
   - CXXFLAGS ........................... -O2
   - LDFLAGS for executables ............
   - LDFLAGS for shared libraries .......
  Use assembly routines ................. ON
  Use ccache for compiling .............. ON
  ...
  ```

  - cross-compiling for Windows:
  ```
  $ make -C depends HOST=x86_64-w64-mingw32 DEBUG=1 NO_QT=1
  $ cmake -B build --toolchain depends/x86_64-w64-mingw32/share/toolchain.cmake -DCMAKE_BUILD_TYPE=Debug
  ...
  Cross compiling ....................... TRUE, for Windows, x86_64
  Preprocessor defined macros ........... _WIN32_WINNT=0x0601 _WIN32_IE=0x0501 WIN32_LEAN_AND_MEAN NOMINMAX WIN32 _WINDOWS _MT _GLIBCXX_DEBUG _GLIBCXX_DEBUG_PEDANTIC
  C compiler ............................ /usr/bin/x86_64-w64-mingw32-gcc
  CFLAGS ................................ -pipe -std=c11 -O1
  C++ compiler .......................... /usr/bin/x86_64-w64-mingw32-g++-posix
  CXXFLAGS .............................. -pipe -std=c++17 -O1
  Common compile options ................
  Common link options ................... -Wl,--major-subsystem-version,6 -Wl,--minor-subsystem-version,1
  Linker flags for executables .......... -static
  Linker flags for shared libraries .....
  Build type (configuration):
   - CMAKE_BUILD_TYPE ................... Debug
   - Preprocessor defined macros ........ DEBUG DEBUG_LOCKORDER DEBUG_LOCKCONTENTION RPC_DOC_CHECK ABORT_ON_FAILED_ASSUME
   - CFLAGS ............................. -O0 -g3
   - CXXFLAGS ........................... -O0 -g3 -ftrapv
   - LDFLAGS for executables ............
   - LDFLAGS for shared libraries .......
  Use assembly routines ................. ON
  Use ccache for compiling .............. ON
  ...
  ```

  **A cross-project note.** The `ProcessConfigurations.cmake` is based on the same module that was suggested in bitcoin-core/secp256k1#1291. So, cross-reviewing is welcome :)

ACKs for top commit:
  theuni:
    ACK a65da0d to keep this moving. This has been sitting for too long :(

Tree-SHA512: 57c5e91ddf9675c6a2b56c0cb70fd3f045af8076bee74c49390de38b8d514e130d2086fde6d83d2d1278b437d0a10cc721f0aa44934698110aeadb3a1aef9e64
hebasto pushed a commit that referenced this pull request Nov 29, 2023
…BlockTx suppression

fa9dc92 test: Add missing CBlockPolicyEstimator::processBlockTx suppression (MarcoFalke)

Pull request description:

  Fixes bitcoin#28865 (comment)

  ```
  # FUZZ=policy_estimator UBSAN_OPTIONS="suppressions=/root/fuzz_dir/scratch/fuzz_gen/code/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./src/test/fuzz/fuzz /tmp/crash-154b42214e70781a9c1ad72d3f2693913dcf8c06

  ...

  policy/fees.cpp:632:27: runtime error: implicit conversion from type 'unsigned int' of value 4294574080 (32-bit, unsigned) to type 'int' changed the value to -393216 (32-bit, signed)
      #0 0x55cbbe10daee in CBlockPolicyEstimator::processBlockTx(unsigned int, CTxMemPoolEntry const*) src/policy/fees.cpp:632:27
      #1 0x55cbbe10e361 in CBlockPolicyEstimator::processBlock(unsigned int, std::vector<CTxMemPoolEntry const*, std::allocator<CTxMemPoolEntry const*>>&) src/policy/fees.cpp:680:13
      #2 0x55cbbd84af48 in policy_estimator_fuzz_target(Span<unsigned char const>)::$_1::operator()() const src/test/fuzz/policy_estimator.cpp:69:40
      #3 0x55cbbd84af48 in unsigned long CallOneOf<policy_estimator_fuzz_target(Span<unsigned char const>)::$_0, policy_estimator_fuzz_target(Span<unsigned char const>)::$_1, policy_estimator_fuzz_target(Span<unsigned char const>)::$_2, policy_estimator_fuzz_target(Span<unsigned char const>)::$_3>(FuzzedDataProvider&, policy_estimator_fuzz_target(Span<unsigned char const>)::$_0, policy_estimator_fuzz_target(Span<unsigned char const>)::$_1, policy_estimator_fuzz_target(Span<unsigned char const>)::$_2, policy_estimator_fuzz_target(Span<unsigned char const>)::$_3) src/./test/fuzz/util.h:43:27
      #4 0x55cbbd84af48 in policy_estimator_fuzz_target(Span<unsigned char const>) src/test/fuzz/policy_estimator.cpp:38:9
      #5 0x55cbbda1cc18 in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
      #6 0x55cbbda1cc18 in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:178:5
      #7 0x55cbbd26a944 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x190e944) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d)
      #8 0x55cbbd253916 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x18f7916) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d)
      #9 0x55cbbd25945a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x18fd45a) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d)
      #10 0x55cbbd284026 in main (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x1928026) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d)
      #11 0x7fe4aa8280cf  (/lib/x86_64-linux-gnu/libc.so.6+0x280cf) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89)
      #12 0x7fe4aa828188 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28188) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89)
      #13 0x55cbbd24e494 in _start (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x18f2494) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d)

  SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change policy/fees.cpp:632:27 in
  ```

  ```
  # base64 /tmp/crash-154b42214e70781a9c1ad72d3f2693913dcf8c06
  AQEAAAAAADkFlVwAAQEAAAAAADkFlZVcACTDSSsP3746IAZrH48khwMAAQEB/QEALQAACwAAAAAA
  FgAAAAAAAQAABgAAAAAAAAAAAAAAAAAAACcQAAAAAAAAAAAAAAAAAAAAAAD6AAAAOQWVXAABAQAA
  AAAAOQWVlVwAIMNJKw/fvjogBmsfjySHAwABAQH9AQAtAAALAAAAAAAAAAABAAAGAAAAAAAAAAAA
  AAAAAAAAJxAAAAAAAAAAAAAAAAAAAAAAAPr/AAAAAAAAAAAAAAQAAAAA/wAAAAAAAAAAAAAEAAAA
  AAEBAeAIAVwBXAAA/jbSBvwBKABSKBwBYgEB2wAEkvXInHYAAAAAAAAAvgAAAAAA/9//6v8e/xIk
  MgAlAiUAOw==

ACKs for top commit:
  fanquake:
    ACK fa9dc92
  dergoegge:
    utACK fa9dc92

Tree-SHA512: 3898c17c928ecc2bcc8c7086359e9ae00da2197b4d8e10c7bf6d12415326c9bca3ef6e1d8d3b83172ccfa604ce7e7371415262ba705225f9ea4da8b1a7eb0306
hebasto pushed a commit that referenced this pull request Nov 30, 2023
…tifications fuzz target

fab164f fuzz: Avoid signed-integer-overflow in wallet_notifications fuzz target (MarcoFalke)

Pull request description:

  Should avoid

  ```
  policy/feerate.cpp:29:63: runtime error: signed integer overflow: 77600710321911316 * 149 cannot be represented in type 'int64_t' (aka 'long')
      #0 0x563a1775ed66 in CFeeRate::GetFee(unsigned int) const src/policy/feerate.cpp:29:63
      #1 0x563a15913a69 in wallet::COutput::COutput(COutPoint const&, CTxOut const&, int, int, bool, bool, bool, long, bool, std::optional<CFeeRate>) src/./wallet/coinselection.h:91:57
      #2 0x563a16fa6a6d in wallet::FetchSelectedInputs(wallet::CWallet const&, wallet::CCoinControl const&, wallet::CoinSelectionParams const&) src/wallet/spend.cpp:297:17
      #3 0x563a16fc4512 in wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, int, wallet::CCoinControl const&, bool) src/wallet/spend.cpp:1105:33
      #4 0x563a16fbec74 in wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, int, wallet::CCoinControl const&, bool) src/wallet/spend.cpp:1291:16
      #5 0x563a16fcf6df in wallet::FundTransaction(wallet::CWallet&, CMutableTransaction&, long&, int&, bilingual_str&, bool, std::set<int, std::less<int>, std::allocator<int>> const&, wallet::CCoinControl) src/wallet/spend.cpp:1361:16
      #6 0x563a1597b7b9 in wallet::(anonymous namespace)::FuzzedWallet::FundTx(FuzzedDataProvider&, CMutableTransaction) src/wallet/test/fuzz/notifications.cpp:162:15
      #7 0x563a15958240 in wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_0::operator()() const src/wallet/test/fuzz/notifications.cpp:228:23
      #8 0x563a15958240 in unsigned long CallOneOf<wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_0, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_1>(FuzzedDataProvider&, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_0, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_1) src/./test/fuzz/util.h:43:27
      #9 0x563a15958240 in wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>) src/wallet/test/fuzz/notifications.cpp:196:9
      #10 0x563a15fdef0c in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
      #11 0x563a15fdef0c in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:178:5
      #12 0x563a158032a4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19822a4) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06)
      #13 0x563a15802999 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1981999) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06)
      #14 0x563a15804586 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1983586) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06)
      #15 0x563a15804aa7 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1983aa7) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06)
      #16 0x563a157f21fb in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19711fb) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06)
      #17 0x563a1581c766 in main (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x199b766) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06)
      #18 0x7f499e17b0cf  (/lib/x86_64-linux-gnu/libc.so.6+0x280cf) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89)
      #19 0x7f499e17b188 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28188) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89)
      #20 0x563a157e70c4 in _start (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19660c4) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06)

  SUMMARY: UndefinedBehaviorSanitizer: signed-integer-overflow policy/feerate.cpp:29:63 in
  MS: 0 ; base unit: 0000000000000000000000000000000000000000
  0x3f,0x0,0x2f,0x5f,0x5f,0x5f,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0xff,0xff,0xff,0xff,0xff,0x53,0xff,0xff,0xff,0xff,0xff,0x0,0x0,0x0,0x0,0x0,0x0,0x13,0x5e,0x5f,0x5f,0x8,0x25,0x0,0x5f,0x5f,0x5f,0x5f,0x5f,0x5f,0x8,0x25,0xca,0x7f,0x5f,0x5f,0x5f,0x13,0x13,0x5f,0x5f,0x5f,0x2,0xdb,0xca,0x0,0x0,0xe7,0xe6,0x66,0x65,0x0,0x0,0x0,0x0,0x44,0x3f,0xa,0xa,0xff,0xff,0xff,0xff,0xff,0x61,0x76,0x6f,0x69,0x0,0xb5,0x15,
  ?\000/___}}}}}}}}}}}}}}}}}}}}\377\377\377\377\377S\377\377\377\377\377\000\000\000\000\000\000\023^__\010%\000______\010%\312\177___\023\023___\002\333\312\000\000\347\346fe\000\000\000\000D?\012\012\377\377\377\377\377avoi\000\265\025
  artifact_prefix='./'; Test unit written to ./crash-4d3bac8a64d4e58b2f0943e6d28e6e1f16328d7d
  Base64: PwAvX19ffX19fX19fX19fX19fX19fX19fX3//////1P//////wAAAAAAABNeX18IJQBfX19fX18IJcp/X19fExNfX18C28oAAOfmZmUAAAAARD8KCv//////YXZvaQC1FQ==

ACKs for top commit:
  dergoegge:
    ACK fab164f
  brunoerg:
    ACK fab164f

Tree-SHA512: f416828f4394aa7303ee437f141e9bbd23c0e0f1b830e4ef3932338858249ba68a811b9837c5b7ad8c6ab871b6354996434183597c1a910a8d8e8d829693e4b2
hebasto pushed a commit that referenced this pull request Nov 2, 2024
…et_create_transaction

5a26cf7 fuzz: fix `implicit-integer-sign-change` in wallet_create_transaction (brunoerg)

Pull request description:

  This PR limites the value of `m_confirm_target` to avoid `implicit-integer-sign-change`:
  ```
  /ci_container_base/src/wallet/fees.cpp:58:58: runtime error: implicit conversion from type 'unsigned int' of value 4294967292 (32-bit, unsigned) to type 'int' changed the value to -4 (32-bit, signed)
      #0 0x55b6fd26c021 in wallet::GetMinimumFeeRate(wallet::CWallet const&, wallet::CCoinControl const&, FeeCalculation*) ci/scratch/build-x86_64-pc-linux-gnu/src/wallet/./src/wallet/fees.cpp:58:58
      #1 0x55b6fd3ef5ca in wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) ci/scratch/build-x86_64-pc-linux-gnu/src/wallet/./src/wallet/spend.cpp:1101:49
      #2 0x55b6fd3ebea5 in wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) ci/scratch/build-x86_64-pc-linux-gnu/src/wallet/./src/wallet/spend.cpp:1382:16
      #3 0x55b6fccbc154 in wallet::(anonymous namespace)::wallet_create_transaction_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>) ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/./src/wallet/test/fuzz/spend.cpp:99:11
      #4 0x55b6fccda45d in std::function<void (std::span<unsigned char const, 18446744073709551615ul>)>::operator()(std::span<unsigned char const, 18446744073709551615ul>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
      #5 0x55b6fccda45d in LLVMFuzzerTestOneInput ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/util/./src/test/fuzz/fuzz.cpp:211:5
      #6 0x55b6fc368484 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c8a484) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5)
      #7 0x55b6fc367b79 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c89b79) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5)
      #8 0x55b6fc369796 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c8b796) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5)
      #9 0x55b6fc369ca7 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c8bca7) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5)
      #10 0x55b6fc35719f in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c7919f) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5)
      #11 0x55b6fc381826 in main (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1ca3826) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5)
      #12 0x7f934c6661c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)
      #13 0x7f934c66628a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)
      #14 0x55b6fc34c184 in _start (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c6e184) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5)

  SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change /ci_container_base/src/wallet/fees.cpp:58:58
  MS: 0 ; base unit: 0000000000000000000000000000000000000000
  0x2e,0x1,0xb0,0xb8,0x0,0xff,0xff,0xff,0xff,0x60,0x14,0x22,0xff,0xff,0xff,0xff,0xff,0xfd,0xff,0xff,0xff,0xff,0xff,0x7e,0xf9,0x41,0x8,0x2b,0x17,0x58,0xb,0x17,0xfc,0xff,0xff,0xff,0xff,0xff,0xff,0x7e,0xf9,0x41,0x8,0x2b,0x17,0x58,0xb,
  .\001\260\270\000\377\377\377\377`\024\"\377\377\377\377\377\375\377\377\377\377\377~\371A\010+\027X\013\027\374\377\377\377\377\377\377~\371A\010+\027X\013
  artifact_prefix='./'; Test unit written to ./crash-5627f57ffba7568a500f8379f62c3338978b43f2
  Base64: LgGwuAD/////YBQi///////9//////9++UEIKxdYCxf8////////fvlBCCsXWAs=
  ```

ACKs for top commit:
  maflcko:
    lgtm ACK 5a26cf7
  dergoegge:
    utACK 5a26cf7

Tree-SHA512: a1b129d81d42350cf85ff6fb95cd6982b6aac88467a526ee4b3c9b3313af2f7952c5dfa9886f455756faba399d8356b6c318d7ab2d6318e08fea838bee90b2fe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants