Bitcoin Core Github
42 subscribers
126K links
Download Telegram
👍 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)
Sjors closed an issue: "Rename bitcoin-wallet?"
(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.
💬 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.
🤔 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.
📝 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.
💬 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?
🤔 theStack reviewed a pull request: "contrib: Add deterministic-unittest-coverage"
(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.
💬 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...]
💬 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.
💬 maflcko commented on pull request "fuzz: split `coinselection` harness":
(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
...
💬 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?
dergoegge closed an issue: "build: x86 ASan build broken "error: inline assembly requires more registers than available""
(https://github.com/bitcoin/bitcoin/issues/31913)
💬 dergoegge commented on issue "build: x86 ASan build broken "error: inline assembly requires more registers than available"":
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2674701021)
So turns out newer versions of afl++ (i.e. newer than the one oss-fuzz is using) will insert `-flto` when `-fcf-protection` is detected. That results in `-flto -fcf-protection -fsanitize=address` and for some reason that combination doesn't work, not sure if it's just not supported or there's a bug but for us a simple workaround is to simply set `-DENABLE_HARDENING=OFF` when fuzzing with afl++ and ASan on x86. The PR that changes this on the afl++ side is: https://github.com/AFLplusplus/AFLplusp
...
💬 Sjors commented on pull request "validation: log which peer sent us a header":
(https://github.com/bitcoin/bitcoin/pull/27826#issuecomment-2674735658)
Rebased and split into three commits for hopefully easier review.
💬 maflcko commented on issue "build: broken CMake *flags output":
(https://github.com/bitcoin/bitcoin/issues/31482#issuecomment-2674744613)
I guess this can be removed from the milestone? Surely, it is ugly, but harmless and the fix seems to be non-trivial and shouldn't be a blocker?
👍 dergoegge approved a pull request: "fuzz: add targets for PCP and NAT-PMP port mapping requests"
(https://github.com/bitcoin/bitcoin/pull/31676#pullrequestreview-2633277793)
utACK c73b59d47f1ec6fff1ad9155181c2285a5ef5cf4