💬 Sjors commented on pull request "[29.x] backports and rc2":
(https://github.com/bitcoin/bitcoin/pull/32062#issuecomment-2729135997)
ACK 74df31cb0bdef9cce31ae62ed71a1e386cba0274
Don't forget to add https://github.com/bitcoin-core/gui/pull/858 to the PR description.
(https://github.com/bitcoin/bitcoin/pull/32062#issuecomment-2729135997)
ACK 74df31cb0bdef9cce31ae62ed71a1e386cba0274
Don't forget to add https://github.com/bitcoin-core/gui/pull/858 to the PR description.
👍 hebasto approved a pull request: "[29.x] backports and rc2"
(https://github.com/bitcoin/bitcoin/pull/32062#pullrequestreview-2690117498)
ACK 74df31cb0bdef9cce31ae62ed71a1e386cba0274, I have reviewed the code and it looks OK.
I've backported PRs and generated `contrib/devtools/gen-bitcoin-conf.sh` locally and got zero-diff with this PR branch.
(https://github.com/bitcoin/bitcoin/pull/32062#pullrequestreview-2690117498)
ACK 74df31cb0bdef9cce31ae62ed71a1e386cba0274, I have reviewed the code and it looks OK.
I've backported PRs and generated `contrib/devtools/gen-bitcoin-conf.sh` locally and got zero-diff with this PR branch.
💬 hebasto commented on pull request "build: use make < 3.82 syntax for define directive":
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2729155648)
> But note that I run the stock macOS GNU make all the time, as do many other devs. So new issues are likely to be caught during review, though they'll still waste time debugging.
FWIW, the stock macOS Make has [other issues](https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2582535849).
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2729155648)
> But note that I run the stock macOS GNU make all the time, as do many other devs. So new issues are likely to be caught during review, though they'll still waste time debugging.
FWIW, the stock macOS Make has [other issues](https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2582535849).
💬 Sjors commented on pull request "build: use make < 3.82 syntax for define directive":
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2729165254)
Is there a way we can pick up a modern homebrew version of make without forcing macOS devs and users to change their default make? (and without the use of env variables)
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2729165254)
Is there a way we can pick up a modern homebrew version of make without forcing macOS devs and users to change their default make? (and without the use of env variables)
👍 willcl-ark approved a pull request: "ci: [lint] Use Cirrus dockerfile cache"
(https://github.com/bitcoin/bitcoin/pull/31948#pullrequestreview-2690146465)
ACK fa3b4427158d48f7d899582580f8f6a7b1bc981d
Sensible change to speedup the lint task startup.
The change looks correct, and to me it looks like it also works, judging by the log lines:
```log
11:51:41 PM Successfully pulled image "gcr.io/cirrus-ci-community/bitcoin/bitcoin/ci/lint_imagefile:714fc33ccc0872eef4aa515b2c226494648ec0f52aa6d3afbaf779c6d1a976b3" in 17.583s (18.557s including waiting). Image size: 699506090 bytes.
```
...and from the "dependency run" step, which is buildi
...
(https://github.com/bitcoin/bitcoin/pull/31948#pullrequestreview-2690146465)
ACK fa3b4427158d48f7d899582580f8f6a7b1bc981d
Sensible change to speedup the lint task startup.
The change looks correct, and to me it looks like it also works, judging by the log lines:
```log
11:51:41 PM Successfully pulled image "gcr.io/cirrus-ci-community/bitcoin/bitcoin/ci/lint_imagefile:714fc33ccc0872eef4aa515b2c226494648ec0f52aa6d3afbaf779c6d1a976b3" in 17.583s (18.557s including waiting). Image size: 699506090 bytes.
```
...and from the "dependency run" step, which is buildi
...
💬 hebasto commented on pull request "build: use make < 3.82 syntax for define directive":
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2729173888)
> Is there a way we can pick up a modern homebrew version of make without forcing macOS devs and users to change their default make? (and without the use of env variables)
I imagine something like that:
```
gmake -C depends -j 4
```
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2729173888)
> Is there a way we can pick up a modern homebrew version of make without forcing macOS devs and users to change their default make? (and without the use of env variables)
I imagine something like that:
```
gmake -C depends -j 4
```
🤔 l0rinc reviewed a pull request: "fuzz: Speed up *_package_eval fuzz targets a bit"
(https://github.com/bitcoin/bitcoin/pull/31457#pullrequestreview-2690077410)
I will run the `tx_package_eval` fuzz corpora a bit later, but my previous benchmarks indicated that unordered map/set may not always be faster.
Is this a correct way to measure the speed difference (on a Mac)?
```bash
git clone --depth=1 https://github.com/bitcoin-core/qa-assets
cmake --preset=libfuzzer-nosan \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DCMAKE_EXE_LINKER_FLAGS="-fuse-ld=lld"
cmake --bui
...
(https://github.com/bitcoin/bitcoin/pull/31457#pullrequestreview-2690077410)
I will run the `tx_package_eval` fuzz corpora a bit later, but my previous benchmarks indicated that unordered map/set may not always be faster.
Is this a correct way to measure the speed difference (on a Mac)?
```bash
git clone --depth=1 https://github.com/bitcoin-core/qa-assets
cmake --preset=libfuzzer-nosan \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DCMAKE_EXE_LINKER_FLAGS="-fuse-ld=lld"
cmake --bui
...
💬 l0rinc commented on pull request "fuzz: Speed up *_package_eval fuzz targets a bit":
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1998511564)
While I was investigating https://github.com/bitcoin/bitcoin/pull/31682/files#diff-c613867a2f35f2221c4e37262bee556e5b472aa95cd95e1eb0cbf8fe49ad83ffL41 I have also measured something like this, i.e. `std::set<COutPoint>` vs `std::unordered_set<COutPoint, SaltedOutpointHasher>` but interestingly I found the opposite - maybe it's size dependent and small trees are faster than the `SipHash` calculation:
<details>
<summary>`CheckBlockBench` measurements repeated 3x</summary>
> std::set<COutPo
...
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1998511564)
While I was investigating https://github.com/bitcoin/bitcoin/pull/31682/files#diff-c613867a2f35f2221c4e37262bee556e5b472aa95cd95e1eb0cbf8fe49ad83ffL41 I have also measured something like this, i.e. `std::set<COutPoint>` vs `std::unordered_set<COutPoint, SaltedOutpointHasher>` but interestingly I found the opposite - maybe it's size dependent and small trees are faster than the `SipHash` calculation:
<details>
<summary>`CheckBlockBench` measurements repeated 3x</summary>
> std::set<COutPo
...
💬 l0rinc commented on pull request "fuzz: Speed up *_package_eval fuzz targets a bit":
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1998549014)
If you think `unordered_map` is indeed faster (I'll measure it once my benchmarking servers will be available), we could change the `std::set` instances as well - and given that they're array based, we might as well pre-allocate them.
<details>
<summary>Details</summary>
```patch
diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
--- a/src/test/fuzz/package_eval.cpp (revision 147655098edde6feffbbe2c601d5874a7d3a3c5c)
+++ b/src/test/fuzz/package_eval.cpp (date
...
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1998549014)
If you think `unordered_map` is indeed faster (I'll measure it once my benchmarking servers will be available), we could change the `std::set` instances as well - and given that they're array based, we might as well pre-allocate them.
<details>
<summary>Details</summary>
```patch
diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
--- a/src/test/fuzz/package_eval.cpp (revision 147655098edde6feffbbe2c601d5874a7d3a3c5c)
+++ b/src/test/fuzz/package_eval.cpp (date
...
💬 suiyuan1314 commented on pull request "docs: remove CI passing requirement for PR merging":
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1998570290)
I have changed the commit message, the PR title and PR description, are they okay now?
Thanks for your review ^_^
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1998570290)
I have changed the commit message, the PR title and PR description, are they okay now?
Thanks for your review ^_^
📝 l0rinc opened a pull request: "Add `deterministic-fuzz-coverage/Cargo.lock` file to git"
(https://github.com/bitcoin/bitcoin/pull/32082)
This untracked file generated from https://github.com/bitcoin/bitcoin/pull/31836/files#diff-46025ad2b0c46d10d0fd6585c2985aab8044c203e4758f2b4f052ad173557a15 prevents branch switches.
Wanted to `.gitignore` at first, but found a similar commit in
https://github.com/bitcoin/bitcoin/commit/bbbbdb0cd57d75a06357d2811363d30a498f4499#diff-1ad842b18e768175e065568f0c834b736881839873b8bb8d36c8ebb43abbefa1
(https://github.com/bitcoin/bitcoin/pull/32082)
This untracked file generated from https://github.com/bitcoin/bitcoin/pull/31836/files#diff-46025ad2b0c46d10d0fd6585c2985aab8044c203e4758f2b4f052ad173557a15 prevents branch switches.
Wanted to `.gitignore` at first, but found a similar commit in
https://github.com/bitcoin/bitcoin/commit/bbbbdb0cd57d75a06357d2811363d30a498f4499#diff-1ad842b18e768175e065568f0c834b736881839873b8bb8d36c8ebb43abbefa1
👍 vasild approved a pull request: "build: Drop option to disable hardening."
(https://github.com/bitcoin/bitcoin/pull/32071#pullrequestreview-2690198569)
ACK ecf2046d4b5c43ddf64f62f09cd3ed70dd5caafb
(https://github.com/bitcoin/bitcoin/pull/32071#pullrequestreview-2690198569)
ACK ecf2046d4b5c43ddf64f62f09cd3ed70dd5caafb
💬 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.