Update `minisketch` subtree and switch to its build script

hebasto commented at 10:17 am on July 2, 2025: member

This PR:

Updates the minisketch subtree to latest upstream, which includes:
- https://github.com/bitcoin-core/minisketch/pull/75
- https://github.com/bitcoin-core/minisketch/pull/98
Switches to minisketch upstream build system.

hebasto added the label Build system on Jul 2, 2025

DrahtBot commented at 10:17 am on July 2, 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/32856.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

Communications efficient -> Communication-efficient [adjective compound modifying “set reconciliation” needs a hyphenated singular form]
filed_sizes -> field_sizes [variable name misspelling in message assignment leads to wrong identifier used when printing “field sizes”]

drahtbot_id_5_m

hebasto commented at 10:18 am on July 2, 2025: member

This PR concludes the joint efforts of @sipa, @purpleKarrot, @theuni, and me.

BrandonOdiwuor commented at 7:07 am on August 28, 2025: contributor

ACK a6b48f009f7bf88bc1f15ed9490276baa898884e

Verified migration to minisketch upstream CMake build script. Successfully built minisketch and Bitcoin Core on macOS 15.6 (Clang 17, CMake 4.0.2) and Ubuntu 24.04 (GCC 13, CMake 3.28.3)

macOS

0$ cmake --build build -j9

Ubuntu

0$ cmake --build build -j9

maflcko added the label DrahtBot Guix build requested on Aug 28, 2025

in cmake/minisketch.cmake:9 in a6b48f009f outdated

83-      ${PROJECT_SOURCE_DIR}/src/minisketch/src/fields/clmul_8bytes.cpp
84+function(add_minisketch subdir)
85+  message("")
86+  message("Configuring minisketch subtree...")
87+  set(BUILD_SHARED_LIBS OFF)
88+  set(CMAKE_EXPORT_COMPILE_COMMANDS OFF)

purpleKarrot commented at 7:32 am on August 28, 2025:

Why?

We should not interfere with CMAKE_EXPORT_COMPILE_COMMANDS. If developers set this option globally in their dotfiles because it will give them code completion in their IDE, setting it to OFF here will break their setup.

hebasto commented at 11:15 am on October 13, 2025:

Why?

Because this PR is not supposed to change behaviour.

We should not interfere with CMAKE_EXPORT_COMPILE_COMMANDS. If developers set this option globally in their dotfiles because it will give them code completion in their IDE, setting it to OFF here will break their setup.

When the decision was made to disable the EXPORT_COMPILE_COMMANDS property for subtree targets during the migration from Autotools to CMake, this use case was not considered, but only the “tidy” CI job, which would otherwise require additional manual filtering of compile_commands.json.

A better approach might be discussed elsewhere.

in cmake/minisketch.cmake:11 in a6b48f009f outdated

85+  message("")
86+  message("Configuring minisketch subtree...")
87+  set(BUILD_SHARED_LIBS OFF)
88+  set(CMAKE_EXPORT_COMPILE_COMMANDS OFF)
89+  # Hard-code subtree options; they won’t appear in the CMake cache.
90+  set(MINISKETCH_INSTALL OFF)

purpleKarrot commented at 7:41 am on August 28, 2025:

This option is useless and can be removed from minisketch because passing EXCLUDE_FROM_ALL to add_subdirectory has the effect that

Any install rules defined in the subdirectory or below will be ignored when installing the parent directory.

hebasto commented at 11:22 am on October 13, 2025:

As stated in the comment above, this is done intentionally to avoid cluttering the CMake cache with options originating from a subtree, which should never be modified by the user.

in src/CMakeLists.txt:21 in a6b48f009f outdated

17@@ -18,6 +18,7 @@ include_directories(${CMAKE_CURRENT_BINARY_DIR} ${CMAKE_CURRENT_SOURCE_DIR})
18 include(../cmake/crc32c.cmake)
19 include(../cmake/leveldb.cmake)
20 include(../cmake/minisketch.cmake)
21+add_minisketch(minisketch)

purpleKarrot commented at 7:55 am on August 28, 2025:

I find it a bit awkward to define this as a function in a module, when it is not really providing reusable functionality. The function may only called in a single place and there is only one valid argument.

A better approach would be to get rid of the ../cmake/minisketch.cmake module and define the function inline without any arguments. Then, add a comment that makes clear that this is a workaround and also notes when it will be replaced:

0// TODO: use `block()` once the minimum cmake is >=2.25
1function(minisketch_block)
2  ...
3endfunction()
4minisketch_block()

hebasto commented at 11:29 am on October 13, 2025:

I agree that using block() would be more appropriate. For now, this code follows the approach used here: https://github.com/bitcoin/bitcoin/blob/563747971be492a8da772fb2f3e45dd5ee05da86/src/CMakeLists.txt#L23-L24 for consistency.

purpleKarrot commented at 1:41 pm on October 14, 2025:

I am not suggesting to use block() here. What I am suggesting is to use function() in a way that can be replaced with block() as soon as the minimum required version can be increased. Since we already know the direction of development, why add more legacy code just for the sake of consistency?

hebasto commented at 4:51 pm on October 16, 2025:

Since we already know the direction of development, why add more legacy code just for the sake of consistency?

It makes the current code more readable (at least for me).

DrahtBot commented at 10:54 pm on August 29, 2025: contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit 7cc9a087069bfcdb79a08ce77eb3a60adf9483af(master)	commit 53fb0bed8c6b095db8a14dbbacaf10576e7de272(pull/32856/merge)
*-aarch64-linux-gnu-debug.tar.gz	`d4d79e719716b3ec...`	`c1906f0adee8ffbb...`
*-aarch64-linux-gnu.tar.gz	`a2d7668998cf5581...`	`f33c164b138e4bb6...`
*-arm-linux-gnueabihf-debug.tar.gz	`7837b2ae895960f0...`	`85253090381c1ca9...`
*-arm-linux-gnueabihf.tar.gz	`d5d7a18af35959f1...`	`7c6ca7737ca58f3e...`
*-arm64-apple-darwin-codesigning.tar.gz	`ff2bc3fc9409522c...`	`d4033b7073389025...`
*-arm64-apple-darwin-unsigned.tar.gz	`3b10528036208e16...`	`a1474c40ebf2a77c...`
*-arm64-apple-darwin-unsigned.zip	`3b1d6add242ef0d7...`	`c0c02a701066e196...`
*-powerpc64-linux-gnu-debug.tar.gz	`4c2b7de0dabcdd4a...`	`000895cac16e7357...`
*-powerpc64-linux-gnu.tar.gz	`f3f972bc01c3ba2a...`	`42a094f6df1a044f...`
*-riscv64-linux-gnu-debug.tar.gz	`89970d2c2338ca29...`	`626110369eda673b...`
*-riscv64-linux-gnu.tar.gz	`0e9938e42a3374e8...`	`2179c86597a2c34d...`
*-x86_64-apple-darwin-codesigning.tar.gz	`8efa84ef1754aeb0...`	`5929ac11470c034e...`
*-x86_64-apple-darwin-unsigned.tar.gz	`cbe1699bdef1590b...`	`88d18471fd19c63f...`
*-x86_64-apple-darwin-unsigned.zip	`279e63b23ff0ce08...`	`22ace0f53beb8b35...`
*-x86_64-linux-gnu-debug.tar.gz	`512b5f4b222c4a46...`	`892b17e56dd5917a...`
*-x86_64-linux-gnu.tar.gz	`56b4aff66d9c4c2d...`	`ee2e5b93fa93f407...`
*.tar.gz	`194017e760c3dc48...`	`f0a9a46c976957a1...`
SHA256SUMS.part	`d6231a052dc6c762...`	`553753a14991a50c...`
guix_build.log	`682a144f4b40ec20...`	`27f2dd82df7e90e4...`
guix_build.log.diff		`6d8a7f57eaa9c089...`

DrahtBot removed the label DrahtBot Guix build requested on Aug 29, 2025

Squashed 'src/minisketch/' changes from ea8f66b1ea..d1bd01e189

d1bd01e189 Merge bitcoin-core/minisketch#98: misc: drop trailing whitespace
fe2c88a4fd misc: drop trailing whitespace
f74b7e2bc2 Merge sipa/minisketch#75: build: Introduce CMake-based build system
850cc868d5 ci: Add GHA workflow with native Windows job
40d56708c8 doc: Add "Building with CMake" section to `README.md`
a0c86c79a7 build: Add CMake-based build system

git-subtree-dir: src/minisketch
git-subtree-split: d1bd01e189e745cd1828707e0a7004b6a6909650

4543a3bde2

Update minisketch subtree to latest upstream c235aa468b

cmake: Switch to `minisketch` upstream build system 82debc3696

hebasto force-pushed on Oct 13, 2025

hebasto commented at 11:37 am on October 13, 2025: member

Refreshed and rebased. The PR description has been updated.

Update minisketch subtree and switch to its build script #32856

Code Coverage & Benchmarks

Reviews

LLM Linter (✨ experimental)

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

Update `minisketch` subtree and switch to its build script #32856