💬 vasild commented on pull request "build: Drop option to disable hardening.":
(https://github.com/bitcoin/bitcoin/pull/32071#discussion_r1998578969)
`hardening_interface` is not needed anymore? The surrounding code appends stuff directly to `core_interface`. Maybe remove these 2 lines and `s/hardening_interface/core_interface/` all over the place?
(https://github.com/bitcoin/bitcoin/pull/32071#discussion_r1998578969)
`hardening_interface` is not needed anymore? The surrounding code appends stuff directly to `core_interface`. Maybe remove these 2 lines and `s/hardening_interface/core_interface/` all over the place?
💬 hebasto commented on pull request "depends: Avoid using helper variables in toolchain file":
(https://github.com/bitcoin/bitcoin/pull/31360#issuecomment-2729224892)
Rebased.
(https://github.com/bitcoin/bitcoin/pull/31360#issuecomment-2729224892)
Rebased.
💬 l0rinc commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2729225347)
After this change I keep getting `untracked files will be overwritten by rebase` for `contrib/devtools/deterministic-fuzz-coverage/Cargo.lock`: added a follow-up to commit it https://github.com/bitcoin/bitcoin/pull/32082
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2729225347)
After this change I keep getting `untracked files will be overwritten by rebase` for `contrib/devtools/deterministic-fuzz-coverage/Cargo.lock`: added a follow-up to commit it https://github.com/bitcoin/bitcoin/pull/32082
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1998587491)
I meant have `kernel_LogCategory` be a subset of `BCLog::LogFlags`, instead of having to remap them. But I didn't realize that that would either require including logging.h in bitcoinkernel.h (impossible), or manually ensuring the enums are synced (bad).
My main gripe was the string re-conversion, which is now gone - so yes, current approach resolves my concern, thanks!
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1998587491)
I meant have `kernel_LogCategory` be a subset of `BCLog::LogFlags`, instead of having to remap them. But I didn't realize that that would either require including logging.h in bitcoinkernel.h (impossible), or manually ensuring the enums are synced (bad).
My main gripe was the string re-conversion, which is now gone - so yes, current approach resolves my concern, thanks!
📝 l0rinc opened a pull request: "doc: shallow clone qa-assets"
(https://github.com/bitcoin/bitcoin/pull/32083)
While reviewing https://github.com/bitcoin/bitcoin/pull/31457#pullrequestreview-2690077410 I noticed that cloning `qa-assets` takes a lot of time - shallow cloning should suffice here.
I haven't checked the other clones in this file but suggestion are welcome.
(https://github.com/bitcoin/bitcoin/pull/32083)
While reviewing https://github.com/bitcoin/bitcoin/pull/31457#pullrequestreview-2690077410 I noticed that cloning `qa-assets` takes a lot of time - shallow cloning should suffice here.
I haven't checked the other clones in this file but suggestion are welcome.
📝 l0rinc opened a pull request: "doc: use `libfuzzer-nosan` for macOS"
(https://github.com/bitcoin/bitcoin/pull/32084)
After a recent `brew upgrade` (or maybe after the other recent fuzzing changes) I couldn't run the fuzzer anymore, I was getting:
> Linker did not accept requested flags, you are missing required libraries
Changing the preset to avoid the other sanitizers seems to solve the issue.
Since `libfuzzer-nosan` builds to a different folder, I've added the full build steps after configuration.
I've also deleted the `brew install llvm` duplication.
(https://github.com/bitcoin/bitcoin/pull/32084)
After a recent `brew upgrade` (or maybe after the other recent fuzzing changes) I couldn't run the fuzzer anymore, I was getting:
> Linker did not accept requested flags, you are missing required libraries
Changing the preset to avoid the other sanitizers seems to solve the issue.
Since `libfuzzer-nosan` builds to a different folder, I've added the full build steps after configuration.
I've also deleted the `brew install llvm` duplication.
⚠️ l0rinc opened an issue: "build: status code 503 for freetype"
(https://github.com/bitcoin/bitcoin/issues/32085)
A few recent builds have started failing with
> error: https://gitlab.freedesktop.org//freetype/freetype/-/archive/VER-2-13-3/freetype-VER-2-13-3.tar.gz: failed: status code 503
note: If you are using a proxy, please ensure your proxy settings are correct.
* https://github.com/bitcoin/bitcoin/actions/runs/13885898347/job/38850557530?pr=32033
* https://github.com/bitcoin/bitcoin/actions/runs/13894602737/job/38872405141?pr=31766
* https://github.com/bitcoin/bitcoin/pull/32082
(https://github.com/bitcoin/bitcoin/issues/32085)
A few recent builds have started failing with
> error: https://gitlab.freedesktop.org//freetype/freetype/-/archive/VER-2-13-3/freetype-VER-2-13-3.tar.gz: failed: status code 503
note: If you are using a proxy, please ensure your proxy settings are correct.
* https://github.com/bitcoin/bitcoin/actions/runs/13885898347/job/38850557530?pr=32033
* https://github.com/bitcoin/bitcoin/actions/runs/13894602737/job/38872405141?pr=31766
* https://github.com/bitcoin/bitcoin/pull/32082
💬 hebasto commented on pull request "Update `secp256k1` subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/32028#issuecomment-2729300327)
Rebased and updated with the recent changes from the upstream repo.
(https://github.com/bitcoin/bitcoin/pull/32028#issuecomment-2729300327)
Rebased and updated with the recent changes from the upstream repo.
💬 hebasto commented on issue "build: status code 503 for freetype":
(https://github.com/bitcoin/bitcoin/issues/32085#issuecomment-2729316596)
See: https://github.com/Microsoft/vcpkg/issues/44429
(https://github.com/bitcoin/bitcoin/issues/32085#issuecomment-2729316596)
See: https://github.com/Microsoft/vcpkg/issues/44429
💬 l0rinc commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14/C++17)":
(https://github.com/bitcoin/bitcoin/pull/31904#issuecomment-2729322992)
Thank you for the review and checking the binary!
(https://github.com/bitcoin/bitcoin/pull/31904#issuecomment-2729322992)
Thank you for the review and checking the binary!
📝 Sjors opened a pull request: "Shuffle depends instructions and recommend modern make for macOS"
(https://github.com/bitcoin/bitcoin/pull/32086)
macOS ships with GNU Make 3.81 from 2006. This has caused
difficult to debug issues, e.g. #32070 and #30978.
Tell users / developers who use the depends system to install a modern version of `make`.
Although Homebrew allows overriding the system `make`, we instead just instruct users to build with `gmake`. This way there should be no impact on other projects they wish to compile.
To increase the likeliness of anyone actually seeing and following this instruction, the first commit moves
...
(https://github.com/bitcoin/bitcoin/pull/32086)
macOS ships with GNU Make 3.81 from 2006. This has caused
difficult to debug issues, e.g. #32070 and #30978.
Tell users / developers who use the depends system to install a modern version of `make`.
Although Homebrew allows overriding the system `make`, we instead just instruct users to build with `gmake`. This way there should be no impact on other projects they wish to compile.
To increase the likeliness of anyone actually seeing and following this instruction, the first commit moves
...
💬 Sjors commented on pull request "build: use make < 3.82 syntax for define directive":
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2729339929)
Proposed in #32086.
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2729339929)
Proposed in #32086.
💬 Sjors commented on issue "depends: llvm-ranlib (etc): "No such file or directory" on Intel macOS 15.0":
(https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2729341917)
"fixing" this with #32086...
(https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2729341917)
"fixing" this with #32086...
🤔 janb84 reviewed a pull request: "doc: Update documentation to include Clang/llvm based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2690222695)
While re-testing this PR, I found that adding the `-g` flag (debug symbols) impacts code coverage reports. With `-g`, more source lines are mapped, lowering the coverage percentage by revealing untested code that’s hidden without it. I tested this with and without `-DCMAKE_BUILD_TYPE=Debug`, and both setups give a different result.
I agree with the rest of the PR changes. But for the most accurate coverage of the source code as written, I recommend sticking with `Debug` and adding `-g`.
(I
...
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2690222695)
While re-testing this PR, I found that adding the `-g` flag (debug symbols) impacts code coverage reports. With `-g`, more source lines are mapped, lowering the coverage percentage by revealing untested code that’s hidden without it. I tested this with and without `-DCMAKE_BUILD_TYPE=Debug`, and both setups give a different result.
I agree with the rest of the PR changes. But for the most accurate coverage of the source code as written, I recommend sticking with `Debug` and adding `-g`.
(I
...
💬 janb84 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1998592459)
```suggestion
-DAPPEND_CFLAGS="-fprofile-instr-generate -fcoverage-mapping -g" \
-DAPPEND_CXXFLAGS="-fprofile-instr-generate -fcoverage-mapping -g" \
```
NIT: In Debug builds, the compiler usually disables optimizations, but without explicit -g, some optimizations might still occur. Given that the code coverage results are indeed different when running without `-g` and with `-g`, I would say some optimization still occurs interfering with the code coverage results.
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1998592459)
```suggestion
-DAPPEND_CFLAGS="-fprofile-instr-generate -fcoverage-mapping -g" \
-DAPPEND_CXXFLAGS="-fprofile-instr-generate -fcoverage-mapping -g" \
```
NIT: In Debug builds, the compiler usually disables optimizations, but without explicit -g, some optimizations might still occur. Given that the code coverage results are indeed different when running without `-g` and with `-g`, I would say some optimization still occurs interfering with the code coverage results.
💬 brunoerg commented on pull request "doc: use `libfuzzer-nosan` for macOS":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2729352010)
> "Changing the preset to avoid the other sanitizers seems to solve the issue."
It would be good to understand why it doesn't work anymore first.
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2729352010)
> "Changing the preset to avoid the other sanitizers seems to solve the issue."
It would be good to understand why it doesn't work anymore first.
💬 Eunovo commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1998654566)
> For example, if the script is a P2SH output and the provider lacks the redeem script (watch-only scenario), the expected behavior is to return a `addr()`. But with your changes, it would return `nullptr instead.
Are you certain? I checked this and I couldn't verify this claim. If the redeem script is not available in the provider, `provider.GetCScript(scriptid, subscript)` should evaluate to `false` and the expected `addr()` desc will be returned. I tested this by adding the following test
...
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1998654566)
> For example, if the script is a P2SH output and the provider lacks the redeem script (watch-only scenario), the expected behavior is to return a `addr()`. But with your changes, it would return `nullptr instead.
Are you certain? I checked this and I couldn't verify this claim. If the redeem script is not available in the provider, `provider.GetCScript(scriptid, subscript)` should evaluate to `false` and the expected `addr()` desc will be returned. I tested this by adding the following test
...
👍 rkrux approved a pull request: "test: avoid disk space warning for non-regtest"
(https://github.com/bitcoin/bitcoin/pull/32057#pullrequestreview-2690371123)
ACK 20fe41e9e83d510fd467f5a999d55a614b16ef89
The last statement in the PR description seems to be contradicting with the title & the code comment.
> This PR fixes the issue by allowing the space warning on both testnet3 and testnet4.
(https://github.com/bitcoin/bitcoin/pull/32057#pullrequestreview-2690371123)
ACK 20fe41e9e83d510fd467f5a999d55a614b16ef89
The last statement in the PR description seems to be contradicting with the title & the code comment.
> This PR fixes the issue by allowing the space warning on both testnet3 and testnet4.
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#issuecomment-2729392547)
Rebased it against latest [master](https://github.com/bitcoin/bitcoin/commit/db2c57ae9eebdb75c58cd165ac929919969c19a9), ran all tests locally, searched for remaining `\bSpan\b` and compared to previously acked fab51b26d68b37f5914314ad29eeb930538ea7ae the only diff was @theuni's [comment](https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1980199782).
<details>
<summary>Details</summary>
```diff
---
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
--- a/src/test/uti
...
(https://github.com/bitcoin/bitcoin/pull/31519#issuecomment-2729392547)
Rebased it against latest [master](https://github.com/bitcoin/bitcoin/commit/db2c57ae9eebdb75c58cd165ac929919969c19a9), ran all tests locally, searched for remaining `\bSpan\b` and compared to previously acked fab51b26d68b37f5914314ad29eeb930538ea7ae the only diff was @theuni's [comment](https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1980199782).
<details>
<summary>Details</summary>
```diff
---
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
--- a/src/test/uti
...
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1998673375)
> @hebasto should we unset `CMAKE_RUNTIME_OUTPUT_DIRECTORY` here to avoid `mpgen` and `mpgen` landing in the `bin/` directory? Maybe it would be better to move the `libmultiprocess` subtree up one level?
I like the idea of moving the add_subdirectory call up a level so to avoid this problem so will experiment with this.
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1998673375)
> @hebasto should we unset `CMAKE_RUNTIME_OUTPUT_DIRECTORY` here to avoid `mpgen` and `mpgen` landing in the `bin/` directory? Maybe it would be better to move the `libmultiprocess` subtree up one level?
I like the idea of moving the add_subdirectory call up a level so to avoid this problem so will experiment with this.