💬 hebasto commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2697383952)
Closing and marking "Up for grabs".

✅ hebasto closed a pull request: "build: Enhance Ccache performance across worktrees and build trees"
(https://github.com/bitcoin/bitcoin/pull/30861)

💬 laanwj commented on pull request "torcontrol: Limit reconnect timeout to max seconds and log delay in whole seconds":
(https://github.com/bitcoin/bitcoin/pull/31979#discussion_r1979326582)
`timeout` should be a float, this a comparison on two float values returning a float.

💬 laanwj commented on pull request "torcontrol: Limit reconnect timeout to max seconds and log delay in whole seconds":
(https://github.com/bitcoin/bitcoin/pull/31979#discussion_r1979328349)
imo it would be better to do the capping here:
```c++
reconnect_timeout = std::min(reconnect_timeout * RECONNECT_TIMEOUT_EXP, RECONNECT_TIMEOUT_MAX);
```

🤔 naiyoma reviewed a pull request: "wallet: fix crash on double block disconnection"
(https://github.com/bitcoin/bitcoin/pull/31757#pullrequestreview-2657287497)
TestedACK [https://github.com/bitcoin/bitcoin/pull/31757/commits/c80b105f77d8ee397c7dc07b013c37b7df63abd5](https://github.com/bitcoin/bitcoin/pull/31757/commits/c80b105f77d8ee397c7dc07b013c37b7df63abd5)

cherry-picked the test commit on master branch and verified that the test case fails on the second invalidation:

```
2025-03-04T12:18:39.077000Z TestFramework (ERROR): Unexpected exception caught during testing
raise RemoteDisconnected("Remote end closed connection without"
http.client.
...

⚠️ arejula27 opened an issue: "Request for Wiki Edit Permissions – Testing Guide: Bitcoin Core 29.0 RC"
(https://github.com/bitcoin/bitcoin/issues/31984)
Hello, maintainers,
We are currently working on the Testing Guide: Bitcoin Core 29.0 Release Candidate as part of the [BOSS program](https://learning.chaincode.com/#seminars). We are:
@musaHaruna
@arejula27
@janb84
@Prabhat1308
We would like to request permission to edit the Wiki so we can contribute and publish our guide.

Looking forward to your response.

Thank you!

💬 laanwj commented on pull request "torcontrol: Limit reconnect timeout to max seconds and log delay in whole seconds":
(https://github.com/bitcoin/bitcoin/pull/31979#issuecomment-2697457225)
Concept ACK.

It's debatable whether we need an exponential backoff here in the first place. After all Tor is generally a local service, there is no risk of creating a DoS. i added it back then, to reduce the amount of log spam in case Tor isn't running. However we have a better logging system now that takes both category and level into account, so that motivation is no longer relevant.

💬 l0rinc commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1979362441)
Typo, maybe something like:
```suggestion
Using `lld` is required due to issues with Apple's `ld` and `LLVM`.
```

💬 l0rinc commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1979383156)
Nice, finally we can get rid of that (tested, seems to be working without the warning, good find)!
Should we anything to https://github.com/llvm/llvm-project/issues/102630?

💬 m3dwards commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1979412430)
gah, taken!

Thanks for the spot.

💬 hebasto commented on pull request "scripted-diff: rename libmultiprocess repository":
(https://github.com/bitcoin/bitcoin/pull/31982#issuecomment-2697553252)
Concept ACK.

💬 l0rinc commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#issuecomment-2697572586)
ACK 75486c8ed87a480b9f0c4dc7a10f3cd4eee87b12

Compared to [previous review](https://github.com/bitcoin/bitcoin/compare/725c0fd95b15157bd0ec141cc24d27818f536656..75486c8ed87a480b9f0c4dc7a10f3cd4eee87b12#diff-7ff2e1d1ed59fa96766a663236d71bfdd00b4af3321fde39f7dbf022bd968a0a), `APPEND_LDFLAGS` was removed and the description simplified to avoid repetition.

💬 laanwj commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14/C++17)":
(https://github.com/bitcoin/bitcoin/pull/31904#issuecomment-2697629617)
Concept and code review ACK 4cd95a2921805f447a8bcecc6b448a365171eb93

Seems low-risk. As this is compile-time logic, i don't think this should actually affect the compiled code? (haven't verified, though)

🤔 vasild reviewed a pull request: "doc: Update documentation to include Clang/llvm based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2656773286)
Approach ACK 1245e05325b41101eddc76ba9214d910489e35b6

In the light of https://github.com/bitcoin/bitcoin/issues/31047 and https://github.com/bitcoin/bitcoin/pull/31394 I have been thinking that is would be useful to at least document the steps needed to gather coverage with clang. Thanks for the initiative!

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979133216)
This works, but maybe it is better to use the `APPEND_*` alternatives?

https://github.com/bitcoin/bitcoin/blob/15717f0ef3960969ee550a4a41741987b86684dc/CMakeLists.txt#L33-L37

i.e.

```suggestion
-DAPPEND_CFLAGS="-fprofile-instr-generate -fcoverage-mapping" \
-DAPPEND_CXXFLAGS="-fprofile-instr-generate -fcoverage-mapping"
```

If you decide to do that, then you will also need `-DAPPEND_LDFLAGS="-fprofile-instr-generate -fcoverage-mapping"` because `CMAKE_CXX_FLAGS` causes the f
...

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979323892)
Why `-sparse`?
```
--sparse[=true|false]
Do not emit function records with 0 execution count. Can only be
used in conjunction with -instr. Defaults to false, since it can
inhibit compiler optimization during PGO.
```

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979077210)
This defaults to `CMAKE_BUILD_TYPE=RelWithDebInfo` which uses `-O2`. Using a debugger on an optimized binary usually results in bogus line numbers due to optimizations that can't be mapped back to source code lines. I do not know if it is the same with coverage.

To be on the safe side I have been using `-DCMAKE_BUILD_TYPE=Debug` (which uses `-O0`). Also, I don't see a reason to use `-O2`, other than to have the tests finish faster.

The example in https://clang.llvm.org/docs/UsersManual.htm
...

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979318687)
Using `%m` will help reduce the chance of `.profraw` files being overwritten in case the same PID is reused by the OS:

```suggestion
LLVM_PROFILE_FILE="default_%m_%p.profraw" ctest --test-dir build # Use "-j N" here for N parallel jobs.
```

https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#running-the-instrumented-program mentions:

> For a program such as the [Lit](https://llvm.org/docs/CommandGuide/lit.html) testing tool which invokes other programs, it may be necessary to set
...

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979457702)
If there are profile files from more than one executable, like from `test_bitcoin` and from `bitcoind` then the syntax of this changes a little bit: drop `build/src/test/test_bitcoin` and add `-object=build/src/test/test_bitcoin -object=build/src/bitcoind`:

```
llvm-cov show
--object=build/src/test/test_bitcoin \
--object=build/src/bitcoind \
--instr-profile=build/coverage.profdata \
--format=html \
--show-instantiation-summary \
--show-line-counts-or-regions \
...

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979389115)
I would suggest collecting coverage from functional tests as well because that would change a bit the arguments of `llvm-cov` (see later) and because the path to the profile file has to be specified as an absolute path which is not obvious:

```md
Generating the raw profile data based on `ctest` execution:

```shell
LLVM_PROFILE_FILE=$(pwd)/build/raw_profile_data/%m_%p.profraw build/test/functional/test_runner.py
`` `
```
it has to be a full path because otherwise the `.profraw` file wi
...

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979463614)
Locally, I have been using this as well:

```
-ignore-filename-regex="src/crc32c/|src/leveldb/|src/minisketch/|src/secp256k1/|src/test/"
```