Removing Bitcoin core text where unnecessary

Ataraxia009 commented at 11:37 am on August 2, 2025: none

We do not need Bitcoin Core specific text on these guix-build flows. This makes forking the open source project easier.

These make the scripts more reusable.

DrahtBot commented at 11:37 am on August 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/33126.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	dergoegge, willcl-ark
Concept ACK	luke-jr, BitcoinMechanic, bigshiny90, jonatack, l0rinc
User requested bot ignore	janb84

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33158 (macdeploy: avoid use of Bitcoin Core in Linux cross build by fanquake)
#33155 (contrib: drop bitcoin-util exception from FORTIFY check by fanquake)

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.

Sammie05 commented at 2:56 pm on August 2, 2025: none

Tested locally. Changes are minimal and improve flexibility for forks. Looks good.

in contrib/guix/libexec/build.sh:9 in 85cfa530e5 outdated

5@@ -6,6 +6,8 @@ export LC_ALL=C
6 set -e -o pipefail
7 export TZ=UTC
8 
9+CLIENT_NAME="Bitcoin-Core"

luke-jr commented at 9:44 pm on August 2, 2025:

This should come (ultimately) from /CMakeLists.txt, not have to be re-specified here

Ataraxia009 commented at 5:50 am on August 3, 2025:

Yeah thats better, is there a way to pipe it now? @luke-jr

hodlinator commented at 1:16 pm on August 4, 2025:

Might be able to parse CLIENT_NAME from build/src/bitcoin-build-config.h and replace with -?

Ataraxia009 commented at 11:11 am on August 5, 2025:

used an * instead as per @luke-jr ’s suggestion

in contrib/guix/security-check.py:19 in 85cfa530e5 outdated

15@@ -16,6 +16,8 @@
16 
17 import lief
18 
19+CLIENT_NAME = 'Bitcoin Core'

luke-jr commented at 9:44 pm on August 2, 2025:

same

Ataraxia009 commented at 11:11 am on August 5, 2025:

Dont need to worry about this since we are just removing the text

luke-jr changes_requested

in contrib/guix/security-check.py:129 in 85cfa530e5 outdated

125@@ -124,7 +126,7 @@ def check_ELF_CONTROL_FLOW(binary) -> bool:
126 def check_ELF_FORTIFY(binary) -> bool:
127 
128     # bitcoin-util does not currently contain any fortified functions
129-    if 'Bitcoin Core bitcoin-util utility version ' in binary.strings:
130+    if f'{CLIENT_NAME} bitcoin-util utility version ' in binary.strings:

maflcko commented at 8:32 am on August 4, 2025:

could just remove it?

luke-jr commented at 9:23 pm on August 4, 2025:

0    if any(' bitcoin-util utility version ' in s for s in binary.strings):

Ataraxia009 commented at 11:11 am on August 5, 2025:

I did this slighly differently, please check it out

Ataraxia009 commented at 11:12 am on August 5, 2025:

The new way feels cleaner than the recommended change

Ataraxia009 commented at 10:28 am on August 7, 2025:

Decided @luke-jr ’s solution is best here, so adopted the changes suggested here ^

dergoegge commented at 8:54 am on August 4, 2025: member

to better support forked clients

Concept NACK

This is not something Bitcoin Core needs to support.

janb84 commented at 11:56 am on August 4, 2025: contributor

~~Concept NACK~~

What is the usecase for this ? forks would also fork the GUIX build script and if this is intended as multi client build script than move it to an independent repo.

EDIT No longer NACK-ing this PR: see #33126 (comment)

willcl-ark commented at 1:48 pm on August 4, 2025: member

Concept NACK

Unless this value was picked up from elsewhere, I don’t see how this makes it easier to support “multi clients”"? A two-line downstream patch… remains a two-line downstream patch. Only the lines being patched have changed?

in contrib/guix/libexec/build.sh:374 in 85cfa530e5 outdated

370@@ -369,7 +371,7 @@ mkdir -p "$DISTSRC"
371             ;;
372         *darwin*)
373             cmake --build build --target deploy ${V:+--verbose}
374-            mv build/dist/Bitcoin-Core.zip "${OUTDIR}/${DISTNAME}-${HOST}-unsigned.zip"
375+            mv "build/dist/${CLIENT_NAME}.zip" "${OUTDIR}/${DISTNAME}-${HOST}-unsigned.zip"

luke-jr commented at 9:26 pm on August 4, 2025:

0            mv build/dist/*.zip "${OUTDIR}/${DISTNAME}-${HOST}-unsigned.zip"

luke-jr commented at 9:32 pm on August 4, 2025: member

and ftr, Concept ACK. Core has always had a policy of using CLIENT_NAME correctly and not breaking needlessly when it’s changed. PR title could be updated to appease naysayers.

BitcoinMechanic commented at 10:30 pm on August 4, 2025: none

to better support forked clients

Concept N-A-C-K

This is not something Bitcoin Core needs to support.

No I guess it isn’t, but hopefully that means the spirit of FOSS that is invoked as an excuse when Bitcoin Core does something a large % of its users dislike is something we can stop pretending applies here.

Making life harder for no good reason for people who fork this repo is lamentable behaviour.

Otherwise, ACK.

Ataraxia009 force-pushed on Aug 5, 2025

Ataraxia009 renamed this:
~~Allowing multi client support in guix-build~~
Removing Bitcoin core text where unnecessary
on Aug 5, 2025

Ataraxia009 commented at 11:14 am on August 5, 2025: none

to better support forked clients

Concept NACK

This is not something Bitcoin Core needs to support.

This objectively makes Core easier to fork and to maintain the fork.

It doesn’t need to support it, but it does make it a more accessible open source project.

And its just cleaner.

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

bitcoin deleted a comment on Aug 5, 2025

in contrib/guix/libexec/build.sh:372 in 2c9aa1afa8 outdated

368@@ -369,7 +369,7 @@ mkdir -p "$DISTSRC"
369             ;;
370         *darwin*)
371             cmake --build build --target deploy ${V:+--verbose}
372-            mv build/dist/Bitcoin-Core.zip "${OUTDIR}/${DISTNAME}-${HOST}-unsigned.zip"
373+            mv build/dist/*.zip "${OUTDIR}/${DISTNAME}-${HOST}-unsigned.zip"

achow101 commented at 5:47 pm on August 5, 2025:

Potentially moving multiple files to a single file is not a good idea. Using a wildcard here makes this much more fragile and prone to error.

luke-jr commented at 8:35 pm on August 5, 2025:

Moving multiple files to a single file is an error, so it should cause the guix build to abort?

Ataraxia009 commented at 1:31 am on August 6, 2025:

I think this is fine here since it is in the deploy output folder. It should really never be giving out more than a single zip. It’s safe to assume here that the deploy folder will just have the one

hodlinator commented at 10:12 pm on August 6, 2025:

Haven’t explored this in depth, but my guess is that the zip filename comes from this line in add_macos_deploy_target: https://github.com/bitcoin/bitcoin/blob/2c9aa1afa8dab8cf2c86b10010f1d3b342d93425/cmake/module/Maintenance.cmake#L87 An alternative approach might be to change that to something generic like CFBundleName = \"BundleName\", or make it have something closer to the final name in the first place.

Ataraxia009 commented at 10:27 am on August 7, 2025:

Changing it to CFBundleName = \"BundleName\" would be confusing or weird for people just running the raw deploy build, no?

hodlinator commented at 1:00 pm on August 7, 2025:

True, maybe one could make it possible for build.sh to override the bundle name when doing the deploy:

0            BUNDLE_NAME="${DISTNAME}-${HOST}-unsigned"
1            cmake --build build --target deploy ${V:+--verbose} "-DBUNDLE_NAME=${BUNDLE_NAME}"
2            mv "build/dist/${BUNDLE_NAME}.zip" "${OUTDIR}/"

Then have Maintenance.cmake pick that up if set, otherwise fall back to CLIENT_NAME.

fanquake commented at 4:26 pm on August 8, 2025:

Opened an alternative in #33158.

DrahtBot commented at 9:14 pm on August 5, 2025: contributor

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

File	commit fd813bf863b1ffa91429de6342285b35bab2bfa4(master)	commit 9a5e29ab0b33831a011564e0a53e34f72425e9bb(pull/33126/merge)
*-aarch64-linux-gnu-debug.tar.gz	`0fca353c35d56894...`
*-aarch64-linux-gnu.tar.gz	`68c4fb0e5bb8723f...`
*-arm-linux-gnueabihf-debug.tar.gz	`e585818d2a7111a6...`
*-arm-linux-gnueabihf.tar.gz	`e5f64c657be612bd...`
*-arm64-apple-darwin-codesigning.tar.gz	`af8a0f2e9b51d375...`
*-arm64-apple-darwin-unsigned.tar.gz	`1c02e8f7e9bd7ace...`
*-arm64-apple-darwin-unsigned.zip	`8ee2034bc764ada1...`
*-powerpc64-linux-gnu-debug.tar.gz	`6cdd0c745c461a47...`
*-powerpc64-linux-gnu.tar.gz	`3439c91e85664d03...`
*-riscv64-linux-gnu-debug.tar.gz	`c94059448029af2e...`
*-riscv64-linux-gnu.tar.gz	`b250a8f24f86e8ae...`
*-x86_64-apple-darwin-codesigning.tar.gz	`595024244445bc56...`
*-x86_64-apple-darwin-unsigned.tar.gz	`933354116e065ae7...`
*-x86_64-apple-darwin-unsigned.zip	`720d862e21bb32d7...`
*-x86_64-linux-gnu-debug.tar.gz	`c9a74d9865916ede...`
*-x86_64-linux-gnu.tar.gz	`78f28bc55cc42f2a...`
*.tar.gz	`c2dfee232da81a31...`	`34f75b4280262f3c...`
SHA256SUMS.part	`c01f8a26017bb2c2...`
guix_build.log	`a557e059591acc3e...`	`3349999b04f4dcaf...`
guix_build.log.diff		`a6070322972484f4...`

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

Ataraxia009 commented at 4:13 pm on August 6, 2025: none

Can we merge this, the builds seem fine

dergoegge commented at 4:17 pm on August 6, 2025: member

This has 3 nacks with no reasonable rebuttals, ready for close?

Ataraxia009 commented at 4:38 pm on August 6, 2025: none

Concept NACK

What is the usecase for this ? forks would also fork the GUIX build script and if this is intended as multi client build script than move it to an independent repo.

This makes conducting a fork easier, and rebasing the fork/absorbing cores changes if required in a fork easier for a maintainer.

The code is also a lot cleaner, given we check for bitcoin-utils in a better way,

and don’t have an unnecessary Bitcoin-Core based text in the build.sh

Ataraxia009 commented at 4:39 pm on August 6, 2025: none

Concept NACK

Unless this value was picked up from elsewhere, I don’t see how this makes it easier to support “multi clients”"? A two-line downstream patch… remains a two-line downstream patch. Only the lines being patched have changed?

This is addressed, and a fair callout.

Now the downstream patch for a forked client is not a two line downstream patch

Ataraxia009 commented at 4:40 pm on August 6, 2025: none

This has 3 nacks with no reasonable rebuttals, ready for close?

Just addressed them.

Feel like this is objectively more reusable code

KurtisStirling commented at 4:46 pm on August 6, 2025: none

This has 3 nacks with no reasonable rebuttals, ready for close?

Rebuttal: claiming to be open source, but not acting in the spirit of open source, is bad for Core’s already tarnished reputation.

Sure, no one has to do anything, but ngaf about people outside of your immediate circle is no way to win kudos.

bigshiny90 commented at 4:50 pm on August 6, 2025: none

Concept ACK - obviously.

Simple change that simplifies forking. Core may not NEED to do this, but why would Core NOT do this?

janb84 commented at 5:06 pm on August 6, 2025: contributor

Can we merge this, the builds seem fine

Isn’t the missing links to the guix build outcomes not an indication that the build is indeed not fine ? would expect a result more like #32865 (comment)

dergoegge commented at 5:06 pm on August 6, 2025: member

You are free to fork the code and change whatever you like, but the burden of maintaining your fork falls on you not the contributors of Bitcoin Core.

achow101 commented at 5:09 pm on August 6, 2025: member

Can we merge this, the builds seem fine

No, there are no ACKs from regular contributors. No PR is merged just because it builds fine.

janb84 commented at 5:21 pm on August 6, 2025: contributor

Concept NACK What is the usecase for this ? forks would also fork the GUIX build script and if this is intended as multi client build script than move it to an independent repo.

This makes conducting a fork easier, and rebasing the fork/absorbing cores changes if required in a fork easier for a maintainer.

The code is also a lot cleaner, given we check for bitcoin-utils in a better way,

and don’t have an unnecessary Bitcoin-Core based text in the build.sh

Thanks for addressing my question. With a more descriptive PR title your intentions with the code changes (PR) are better understood. Although I’m not agreeing that the current state of the code in the PR is “a lot cleaner” I’m also not going to block the PR any longer. Have removed my NACK.

jonatack commented at 9:47 pm on August 6, 2025: member

Concept ACK, provided this small patch is robust. It’s a minor change to be sure. Nevertheless, it seems cleaner not to hardcode it and would convey a magnanimous FOSS spirit that I encourage.

l0rinc commented at 0:51 am on August 7, 2025: contributor

I’m not sure why it would be our job to make the code more forkable - but we do need to make it maintainable, so concept ACK for less hard-coding - no opinion on the implementation details, will let others review the specifics.

Removing 'Bitcoin Core' text wherever unnecessary 0560c98ca1

Ataraxia009 force-pushed on Aug 7, 2025

Ataraxia009 commented at 4:20 am on August 7, 2025: none

Isn’t the missing links to the guix build outcomes not an indication that the build is indeed not fine ? would expect a result more like #32865 (comment)

good call, there was a problem with using binary.name in the new python lief version. We should be fine now, just pushed a fix.

However i cant run the drahbot build, if somebody could add the label for building, would be helpful

Ataraxia009 commented at 4:25 am on August 7, 2025: none

You are free to fork the code and change whatever you like, but the burden of maintaining your fork falls on you not the contributors of Bitcoin Core.

The contributors and builders of the bitcoin network do have a responsibility towards helping decentralise the network, which leads to overall network health.

In my humble opinion, helping support more diversity in client choice for node runners should be, if not already is, a core principle of the network.

Also this leads to less hardcoding and code cleanliness, as agreed upon by other reviewers.

maflcko commented at 6:16 am on August 7, 2025: member

Can we merge this, the builds seem fine

No, it didn’t. The build failed and you haven’t tested this yourself at all?

However i cant run the drahbot build, if somebody could add the label for building, would be helpful

A passing CI (or build) is not sufficient for a merge. Especially here, because a full guix build (with signing) isn’t run by any CI. The burden to explain the changes (and why they are safe and correct, and how to test them) is on the pull request author. Instead of spamming the thread with comments about rushing a merge here without proper testing and review, it would be better to make testing and review easier.

Ataraxia009 commented at 10:16 am on August 7, 2025: none

Can we merge this, the builds seem fine

No, it didn’t. The build failed and you haven’t tested this yourself at all?

However i cant run the drahbot build, if somebody could add the label for building, would be helpful

A passing CI (or build) is not sufficient for a merge. Especially here, because a full guix build (with signing) isn’t run by any CI. The burden to explain the changes (and why they are safe and correct, and how to test them) is on the pull request author. Instead of spamming the thread with comments about rushing a merge here without proper testing and review, it would be better to make testing and review easier.

Hey apologies, my intent was not to rush, was just trying to address everyones concerns

Did run tests, but had a bad local environment, unfortunately

Anyways, everything should be fine now, should attach a drahbot guix build request to be sure

TheCharlatan commented at 10:27 am on August 7, 2025: contributor

Anyways, everything should be fine now, should attach a drahbot guix build request to be sure

Ideally you would do the full guix build yourself and post the build artifact hashes. Your dev environment should have no influence on that. Then reviewers can compare their results to yours.

Ataraxia009 commented at 2:18 pm on August 7, 2025: none

guix build for verification

 0daa954407e65e2f3baba9ed349d46f321786db7822db988f522577a1eb7647c1  guix-build-0560c98ca110/output/dist-archive/bitcoin-0560c98ca110.tar.gz
 1f7bcb25a8f555b22b07c713d13823f3e07cfb9f4e2e1da1ac197d019fb89da60  guix-build-0560c98ca110/output/riscv64-linux-gnu/SHA256SUMS.part
 23275ce7634c4b93b5733985aa6b761ed2b87d7984e44bb79f375a3e25dbd6511  guix-build-0560c98ca110/output/riscv64-linux-gnu/bitcoin-0560c98ca110-riscv64-linux-gnu.tar.gz
 37b5988013467c397fd2f4c23105fc905a190033728c7c8ce9eca6540e3015187  guix-build-0560c98ca110/output/riscv64-linux-gnu/bitcoin-0560c98ca110-riscv64-linux-gnu-debug.tar.gz
 479ca6d9b03a850ed6cd4d0f9dc72ca180ee23862e739efdad3e37b5ee1a54e9f  guix-build-0560c98ca110/output/powerpc64-linux-gnu/bitcoin-0560c98ca110-powerpc64-linux-gnu.tar.gz
 5a04588aba9080007be2dba0fa9ae63d503e3018e680a0f135f30b644d8bb03b6  guix-build-0560c98ca110/output/powerpc64-linux-gnu/SHA256SUMS.part
 60a4de5e49c9ce698a1b3b737db9604230676cea16cf4afda0d0b30c539e1527d  guix-build-0560c98ca110/output/powerpc64-linux-gnu/bitcoin-0560c98ca110-powerpc64-linux-gnu-debug.tar.gz
 7749e9d66a8869e935d8b469e0af080b9be50f21e3cce82c3ac5c55c0c289a270  guix-build-0560c98ca110/output/arm64-apple-darwin/bitcoin-0560c98ca110-arm64-apple-darwin-unsigned.zip
 86417faef9652a76824177c91b05caacaaa5755f1b1af206a59891a81fc2aff0b  guix-build-0560c98ca110/output/arm64-apple-darwin/SHA256SUMS.part
 967cac3f596f8d7f3623660b0c440de2506444f473891613d9fe30c47f6ed9bba  guix-build-0560c98ca110/output/arm64-apple-darwin/bitcoin-0560c98ca110-arm64-apple-darwin-codesigning.tar.gz
10abb16a9182273e2fd80a6197e90c060d3d971d6386dbe401d17efa686328a965  guix-build-0560c98ca110/output/arm64-apple-darwin/bitcoin-0560c98ca110-arm64-apple-darwin-unsigned.tar.gz
111a96eacf57752c4588a4ac7ac8c0cfa950c3d28ca9860c948eb1bf9d586f23ce  guix-build-0560c98ca110/output/arm-linux-gnueabihf/bitcoin-0560c98ca110-arm-linux-gnueabihf.tar.gz
12ae460ee8a39778f3cc0167e14a951e4ff3d52815a080941a1fdcea8ce2464805  guix-build-0560c98ca110/output/arm-linux-gnueabihf/SHA256SUMS.part
13ad3e50cf615e770e7aa696118497eb4df29d1950fdc07c4b715ea1765922b084  guix-build-0560c98ca110/output/arm-linux-gnueabihf/bitcoin-0560c98ca110-arm-linux-gnueabihf-debug.tar.gz
142f63caa57dc7806f62dbc379c0fa3a7d27e4c4f5967d33b231ab332af062116f  guix-build-0560c98ca110/output/x86_64-linux-gnu/SHA256SUMS.part
15bafe716285828988ceccf06facf8dd93f44189a61aad3d307078f16ea4a524f5  guix-build-0560c98ca110/output/x86_64-linux-gnu/bitcoin-0560c98ca110-x86_64-linux-gnu-debug.tar.gz
1641a9f679a06da5859f0c9df0fd5065da74c81e209856d77acc28076b891100ad  guix-build-0560c98ca110/output/x86_64-linux-gnu/bitcoin-0560c98ca110-x86_64-linux-gnu.tar.gz
177b530cb932bf0133e5a4ec829ecefa5ffb8219f4cce3c8098b2d02d56e88461a  guix-build-0560c98ca110/output/x86_64-w64-mingw32/SHA256SUMS.part
18265c38e8a115e568f60cd347532219ab825ba274dd3099ab3b6ac0ff095da506  guix-build-0560c98ca110/output/x86_64-w64-mingw32/bitcoin-0560c98ca110-win64-unsigned.zip
19adbe1db99fbdd861dec43c85fa68838af5d202fa6dd0897b55a68885698b5bb9  guix-build-0560c98ca110/output/x86_64-w64-mingw32/bitcoin-0560c98ca110-win64-codesigning.tar.gz
20a1bff07d2cf16013cce8c8281ee59c52be6c82ceeea72cb00c93b77d847be57b  guix-build-0560c98ca110/output/x86_64-w64-mingw32/bitcoin-0560c98ca110-win64-setup-unsigned.exe
21e111af7a06809ad48b8ad1edf63d31d9dce657021166c13afe7a53b84dff564f  guix-build-0560c98ca110/output/x86_64-w64-mingw32/bitcoin-0560c98ca110-win64-debug.zip
223a3ee54a1ebb55b59f77d03f61d8be5b2e0d65920fa18c90b7e19964af067498  guix-build-0560c98ca110/output/aarch64-linux-gnu/bitcoin-0560c98ca110-aarch64-linux-gnu-debug.tar.gz
238fec1930c52d8abdd7bbc4a77e0d1b01766e9192873ab9f28bd60ed039fea86a  guix-build-0560c98ca110/output/aarch64-linux-gnu/SHA256SUMS.part
2436fef4ab9f8ec8975a826045dfc3df9e5f0470da32cc38216275deea86f63718  guix-build-0560c98ca110/output/aarch64-linux-gnu/bitcoin-0560c98ca110-aarch64-linux-gnu.tar.gz
25ecb8e7646231d7f6d2bc091eded179b5d9bf0888dc30014440c74c4c1682c8f0  guix-build-0560c98ca110/output/x86_64-apple-darwin/bitcoin-0560c98ca110-x86_64-apple-darwin-unsigned.zip
26aac8c9f4e9b720fd36d015358477c954ac3f3f64e0ba734576183a3fb8cf9b8e  guix-build-0560c98ca110/output/x86_64-apple-darwin/bitcoin-0560c98ca110-x86_64-apple-darwin-unsigned.tar.gz
277743006a89b46e6af88fcc2b85b6da1689f92be8c5441e766e7b8537a8673189  guix-build-0560c98ca110/output/x86_64-apple-darwin/bitcoin-0560c98ca110-x86_64-apple-darwin-codesigning.tar.gz
28fd20d767413b012411e8977834d2ee123a265b8b44c72a21d6886274ea030869  guix-build-0560c98ca110/output/x86_64-apple-darwin/SHA256SUMS.part

in contrib/guix/security-check.py:127 in 0560c98ca1

123@@ -124,7 +124,7 @@ def check_ELF_CONTROL_FLOW(binary) -> bool:
124 def check_ELF_FORTIFY(binary) -> bool:
125 
126     # bitcoin-util does not currently contain any fortified functions
127-    if 'Bitcoin Core bitcoin-util utility version ' in binary.strings:
128+    if any(' bitcoin-util utility version ' in s for s in binary.strings):

fanquake commented at 9:29 am on August 8, 2025:

This can just be removed: #33155.

DrahtBot added the label Needs rebase on Aug 11, 2025

DrahtBot commented at 11:18 am on August 11, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Ataraxia009 commented at 4:29 am on August 12, 2025: none

Seems like the above 2 PRs will cover this just fine:

#33155 #33158

No need to merge this then, closing

Ataraxia009 closed this on Aug 12, 2025

Removing Bitcoin core text where unnecessary #33126

Code Coverage & Benchmarks

Reviews

Conflicts

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