💬 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

🤔 ajtowns reviewed a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2684192006)
This seems fairly difficult to review -- there's a lot of mostly implicit background in what TxGraph does and why it does it that way, as evidenced by both the description in this PR and in the [review club](https://bitcoincore.reviews/31363).

When I started typing this comment, I was wondering if maybe it would make sense to make the commits here into more of a gentle introduction to TxGraph, rather than one commit that does everything, then optimisations. But I don't really think that appro
...

💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1994918766)
`std::array<std::vector<std::unique_ptr<Cluster>>,int{QualityLevel::NONE}> m_clusters;` might read clearer?

💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1994939901)
"A class of objects" doesn't seem like a very useful description? "Internal information about each transaction in a TxGraphImpl" ?

💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1994757992)
This commit could be a scripted diff.

```
-BEGIN VERIFY SCRIPT-
sed -i 's/Data type to represent transaction indices in clusters./Data type to represent transaction indices in DepGraphs and the clusters they represent./' $(git grep -l 'using ClusterIndex')
sed -i 's|\<ClusterIndex\>|DepGraphIndex|g' $(git grep -l 'ClusterIndex')
-END VERIFY SCRIPT-
```