💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2730066751)
`ebc7ba0216...e150e3064b`: rebase due to conflicts

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1999116127)
> to me it feels more important to have non-global logging objects first.

Agreed. Updating the docs to reflect this might be good though, e.g.:

> * Not thread-safe. Logging is global. Multiple calls are allowed but
> * must be synchronized and will override previous settings for all
> * existing `kernel_LoggingConnection` instances.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1999120268)
Right, it is because of this:

https://github.com/bitcoin/bitcoin/blob/e150e3064b4c8165a0429a536d5235e8475e0045/src/net.cpp#L2663-L2676

what happens is that there are no free slots, so the grant is not acquired immediately at line 2663. No private broadcast is needed, so it goes to line 2673 to wait for the grant, "indefinitely", until some peer is disconnected. Then a transaction is submitted and a private broadcast is needed, but the code is waiting here on line 2673. Hmm...

🤔 jonatack reviewed a pull request: "doc: Update documentation to include Clang/llvm based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2691136083)
ACK 3c69b071ac4964870abb347f9d9c15df2e60aa62 modulo feedback below

💬 jonatack commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1999092770)
```suggestion
# MacOS may require `-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++"` instead of `-DCMAKE_C_COMPILER="clang" -DCMAKE_CXX_COMPILER="clang++"`
```

Maybe also place this comment after the command, instead of before it.

💬 jonatack commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1999093752)
Directory name: would it make sense to replace the `build` directory in the instructions with, say, `build_cov` instead?

I think we're already using directory naming like `build_fuzz` (in `doc/fuzzing.md`) and `build_dev_mode` (in `CMakePresets.json`).

💬 jonatack commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1999121595)
Should we mention clearing out with `rm -rf build` to begin with a clean slate?

💬 jonatack commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1999113012)
```suggestion
The generated coverage report is located at `build/coverage_report/index.html`. Running `open build/coverage_report/index.html` will open it in your browser.
```

💬 janb84 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1999122264)
> `-DCMAKE_BUILD_TYPE=Debug` adds `-g` already.

Why is there a difference in the reports when manually adding the -g ?

💬 Sjors commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2730102780)
re-ACK 8ef8f799b04eaea9091ed339dfe2cdb3a31ec245

Thanks for adding:

```
libmultiprocess ..................... ON (external)
```

I see c27d3a710b844e845075dea7e51f8f368ebf409f -> 0efd6dd71da70dba4035b498236ebedfcb385bcf now introduces `cmake/libmultiprocess.cmake` and puts `add_libmultiprocess` there instead of instead of `src/ipc/CMakeList.txt`.

I see mpgen now ends up in `build/src/ipc/libmultiprocess/mpgen`. Perhaps after #31375 it can live in `build/libexec`?

💬 hodlinator commented on pull request "net: Block v2->v1 transport downgrade if !fNetworkActive":
(https://github.com/bitcoin/bitcoin/pull/32073#issuecomment-2730139693)
> Could you update the online/offline terminology in the PR?

Fixed to use `fNetworkActive` instead, agree it was a bit of a stretch.

> If we're attempting a v2 connection during the time period when the `fNetworkActive` option is toggled, we would have the 1 extra connection attempt but less probability of this happening/`OpenNetworkConnection` [immediately returns](https://github.com/bitcoin/bitcoin/blob/a799415d84d392c0f877d3619583b9a16f940c54/src/net.cpp#L2996) anyways.

Yes, seems li
...

💬 hodlinator commented on pull request "net: Block v2->v1 transport downgrade if !fNetworkActive":
(https://github.com/bitcoin/bitcoin/pull/32073#discussion_r1999169528)
Also felt it was a bit brittle, fixed in latest push, with added note in commit message.

💬 VolodymyrBg commented on pull request "test: Add support for mainnet addresses in address_to_scriptpubkey":
(https://github.com/bitcoin/bitcoin/pull/32060#issuecomment-2730157607)
@maflcko @davidgumberg is something wrong?

💬 theuni commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2730158691)
> I see mpgen now ends up in `build/src/ipc/libmultiprocess/mpgen`. Perhaps after #31375 it can live in `build/libexec`?

mpgen shouldn't be installed (in makefile-speak it's an intermediate binary, which doesn't seem to exist for CMake), so afaik the libexec question is moot.

💬 Chand-ra commented on pull request "rpc: add cli examples, update docs":
(https://github.com/bitcoin/bitcoin/pull/31958#issuecomment-2730194601)
Concept ACK [8134a6](https://github.com/bitcoin/bitcoin/pull/31958/commits/8134a6b5d40568dcf32fdb21163cb1792efddc27)

The proposed changes _do_ improve the readability of the documentation.

💬 hodlinator commented on pull request "net: Block v2->v1 transport downgrade if !fNetworkActive":
(https://github.com/bitcoin/bitcoin/pull/32073#issuecomment-2730200956)
[Windows CI failure](https://github.com/bitcoin/bitcoin/actions/runs/13904008032/job/38902487830?pr=32073) is unrelated:
```
Downloading https://gitlab.freedesktop.org//freetype/freetype/-/archive/VER-2-13-3/freetype-VER-2-13-3.tar.gz -> freetype-freetype-VER-2-13-3.tar.gz
error: https://gitlab.freedesktop.org//freetype/freetype/-/archive/VER-2-13-3/freetype-VER-2-13-3.tar.gz: failed: status code 503
```
Currently tracked as #32085.

💬 willcl-ark commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r1999216364)
Right, I meant those two only and would agree with adding them to the incantation here, if you agreed.

👍 hodlinator approved a pull request: "test: Update coverage.cpp to drop linux restriction"
(https://github.com/bitcoin/bitcoin/pull/32059#pullrequestreview-2691382483)
re-ACK 54e6eacc1fccd602897d9e3025c62f83194ffd5b

Changes since [first ACK](https://github.com/bitcoin/bitcoin/pull/32059#pullrequestreview-2686973172):
- Moved `weak` attribute on the original declarations so they line up with the fallback definitions and mirror [Clang docs](https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1997328341). Thanks!
- Improved commit message.

Retested using Rust utility on coverage-enabled version of the code with successful determinism result.

💬 hodlinator commented on pull request "test: Update coverage.cpp to drop linux restriction":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1999246344)
Thanks for lining up the attributes!

💬 Sjors commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r1999256826)
Added