💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#issuecomment-2674495672)
> re-ACK [61885ad](https://github.com/bitcoin/bitcoin/commit/61885ad9d406e25ec3b6523d01918e886996f1c3)
>
> Dropped the else: [#31622 (comment)](https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1963881147)
@Sjors 61885ad9d406e25ec3b6523d01918e886996f1c3 commit seems to be outdated now.
(https://github.com/bitcoin/bitcoin/pull/31622#issuecomment-2674495672)
> re-ACK [61885ad](https://github.com/bitcoin/bitcoin/commit/61885ad9d406e25ec3b6523d01918e886996f1c3)
>
> Dropped the else: [#31622 (comment)](https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1963881147)
@Sjors 61885ad9d406e25ec3b6523d01918e886996f1c3 commit seems to be outdated now.
💬 maflcko commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2674514881)
> What are you talking about?
The set of binaries that are built can be picked by setting configure options, so the most obvious way to end up with stale binaries is to make a build will all binaries and then another build with a subset of the binaries (intentionally or accidentally) without clearing the build dir. This is one example. Another example would be a stale cmake cache, which influences the build result (intentionally or accidentally). I am sure there are other examples.
All of
...
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2674514881)
> What are you talking about?
The set of binaries that are built can be picked by setting configure options, so the most obvious way to end up with stale binaries is to make a build will all binaries and then another build with a subset of the binaries (intentionally or accidentally) without clearing the build dir. This is one example. Another example would be a stale cmake cache, which influences the build result (intentionally or accidentally). I am sure there are other examples.
All of
...
💬 maflcko commented on issue "Unable to generate coverage report using lcov on MacOs 15.3.1":
(https://github.com/bitcoin/bitcoin/issues/31927#issuecomment-2674522975)
This is a known issue. While I don't have macOS, some reported that it was working with clang/llvm tooling.
You can try `-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'` and `llvm-cov`/`llvm-profdata` and see if that works for you.
(https://github.com/bitcoin/bitcoin/issues/31927#issuecomment-2674522975)
This is a known issue. While I don't have macOS, some reported that it was working with clang/llvm tooling.
You can try `-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'` and `llvm-cov`/`llvm-profdata` and see if that works for you.
💬 vasild commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#issuecomment-2674531786)
Almost ACK dcf9a45aab4c4a7f9f900d5dafcdfe0e9a598a38, modulo squash the two commits into one and rebase.
(https://github.com/bitcoin/bitcoin/pull/29500#issuecomment-2674531786)
Almost ACK dcf9a45aab4c4a7f9f900d5dafcdfe0e9a598a38, modulo squash the two commits into one and rebase.
👍 vasild approved a pull request: "Release `LockData::dd_mutex` before calling `*_detected` functions"
(https://github.com/bitcoin/bitcoin/pull/26781#pullrequestreview-2633025335)
ACK 38f93fe0cb680425f5fec7c794b39c0e1795f9dc
(repeat my earlier comment, DrahtBot was confused)
(https://github.com/bitcoin/bitcoin/pull/26781#pullrequestreview-2633025335)
ACK 38f93fe0cb680425f5fec7c794b39c0e1795f9dc
(repeat my earlier comment, DrahtBot was confused)
✅ Sjors closed an issue: "Rename bitcoin-wallet?"
(https://github.com/bitcoin/bitcoin/issues/31827)
(https://github.com/bitcoin/bitcoin/issues/31827)
💬 Sjors commented on issue "Rename bitcoin-wallet?":
(https://github.com/bitcoin/bitcoin/issues/31827#issuecomment-2674540897)
It sounds like we've now documented that nothing needs to change, so closing this.
(https://github.com/bitcoin/bitcoin/issues/31827#issuecomment-2674540897)
It sounds like we've now documented that nothing needs to change, so closing this.
💬 Sjors commented on issue "rfc: store PSBTs in wallet ":
(https://github.com/bitcoin/bitcoin/issues/17619#issuecomment-2674549337)
@BrandonOdiwuor none. Reviewing pull requests like #27286, #27865 and #21283 may improve the wallet foundations and move this a few inches closer, but there's no open pull request that implements this feature.
(https://github.com/bitcoin/bitcoin/issues/17619#issuecomment-2674549337)
@BrandonOdiwuor none. Reviewing pull requests like #27286, #27865 and #21283 may improve the wallet foundations and move this a few inches closer, but there's no open pull request that implements this feature.
🤔 marcofleon reviewed a pull request: "fuzz: split `coinselection` harness"
(https://github.com/bitcoin/bitcoin/pull/31870#pullrequestreview-2633040561)
Code review ACK 9b293d031c941dc6af0c4274e43388c57c702047
Knapsack is quite a bit slower than the other two, so separating them out is probably a good idea.
(https://github.com/bitcoin/bitcoin/pull/31870#pullrequestreview-2633040561)
Code review ACK 9b293d031c941dc6af0c4274e43388c57c702047
Knapsack is quite a bit slower than the other two, so separating them out is probably a good idea.
📝 hebasto opened a pull request: "ci: Fix filtering out Qt-generated files from `compile_commands.json`"
(https://github.com/bitcoin/bitcoin/pull/31928)
This PR:
1. Adjusts the regex for Qt-generated files to match the CMake build directory structure.
2. Moves the filtering command to run before `clang-tidy`, ensuring that Qt-generated files are not needlessly processed.
Fixes https://github.com/bitcoin/bitcoin/issues/31801.
(https://github.com/bitcoin/bitcoin/pull/31928)
This PR:
1. Adjusts the regex for Qt-generated files to match the CMake build directory structure.
2. Moves the filtering command to run before `clang-tidy`, ensuring that Qt-generated files are not needlessly processed.
Fixes https://github.com/bitcoin/bitcoin/issues/31801.
💬 theStack commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1965485239)
```suggestion
eprintln!("Usage: program ./build_dir <custom test filter>");
```
as there is no default yet and specifying a filter is required by now?
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1965485239)
```suggestion
eprintln!("Usage: program ./build_dir <custom test filter>");
```
as there is no default yet and specifying a filter is required by now?
🤔 theStack reviewed a pull request: "contrib: Add deterministic-unittest-coverage"
(https://github.com/bitcoin/bitcoin/pull/31901#pullrequestreview-2633053901)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31901#pullrequestreview-2633053901)
Concept ACK
💬 theStack commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1965482744)
On the machine where I tried this yesterday quickly (OpenBSD 7.6, with LLVM 16.0.6 included in the base system), neither `llvm-profdata` nor `diff` know a `--version` argument, so the sanity check failed even though the binaries were there. Will try on a Linux machine with Ubuntu in a bit.
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1965482744)
On the machine where I tried this yesterday quickly (OpenBSD 7.6, with LLVM 16.0.6 included in the base system), neither `llvm-profdata` nor `diff` know a `--version` argument, so the sanity check failed even though the binaries were there. Will try on a Linux machine with Ubuntu in a bit.
💬 Sjors commented on issue "Awesome multisig PR labyrinth guide":
(https://github.com/bitcoin/bitcoin/issues/24861#issuecomment-2674581760)
The action moved to https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2459516090
(https://github.com/bitcoin/bitcoin/issues/24861#issuecomment-2674581760)
The action moved to https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2459516090
💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1965508525)
It should be possible to drop `diff` and use `git diff` (or something else). As for the llvm tool, it looks like they added the arg later:
```
# llvm-profdata-19 --version
Debian LLVM version 19.1.4
Optimized build.
# llvm-profdata-16 --version
llvm-profdata-16: Unknown command!
USAGE: llvm-profdata-16 <merge|show|overlap> [args...]
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1965508525)
It should be possible to drop `diff` and use `git diff` (or something else). As for the llvm tool, it looks like they added the arg later:
```
# llvm-profdata-19 --version
Debian LLVM version 19.1.4
Optimized build.
# llvm-profdata-16 --version
llvm-profdata-16: Unknown command!
USAGE: llvm-profdata-16 <merge|show|overlap> [args...]
💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1965512116)
Looks like they added it in 17, so I could require that, or remove the `--version` check.
```
# llvm-profdata-17 --version
llvm-profdata-17
Ubuntu LLVM version 17.0.6
Optimized build.
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1965512116)
Looks like they added it in 17, so I could require that, or remove the `--version` check.
```
# llvm-profdata-17 --version
llvm-profdata-17
Ubuntu LLVM version 17.0.6
Optimized build.
💬 maflcko commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1965528993)
why are you removing this check?
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1965528993)
why are you removing this check?
💬 maflcko commented on pull request "ci: Fix filtering out Qt-generated files from `compile_commands.json`":
(https://github.com/bitcoin/bitcoin/pull/31928#issuecomment-2674686213)
ACK d82dc1041524e8402d201d32c9143233b5ec1baf 🚂
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK d82dc1041524e8402d201d32c9
...
(https://github.com/bitcoin/bitcoin/pull/31928#issuecomment-2674686213)
ACK d82dc1041524e8402d201d32c9143233b5ec1baf 🚂
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK d82dc1041524e8402d201d32c9
...
💬 maflcko commented on issue "Unable to generate coverage report using lcov on MacOs 15.3.1":
(https://github.com/bitcoin/bitcoin/issues/31927#issuecomment-2674699477)
If it works for you, feel free to update the `doc/developer-notes.md` to mention it. Also, I wonder it it could make sense to add a CMakePreset for a clang coverage build based on the flags?
(https://github.com/bitcoin/bitcoin/issues/31927#issuecomment-2674699477)
If it works for you, feel free to update the `doc/developer-notes.md` to mention it. Also, I wonder it it could make sense to add a CMakePreset for a clang coverage build based on the flags?
✅ dergoegge closed an issue: "build: x86 ASan build broken "error: inline assembly requires more registers than available""
(https://github.com/bitcoin/bitcoin/issues/31913)
(https://github.com/bitcoin/bitcoin/issues/31913)