💬 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-
```

💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1994854097)
The invalidation behaviour for Refs seems unclear to me? I think the idea is that if you hold onto a `Ref` you can continue to uniquely refer to a removed transaction, up until the Ref is destructed, at which point the TxGraph is compacted and memory might be reused at that point. This delay then also allows for reordering to occur which can result in optimisations.

I think the intended behaviour could be made a bit clearer in the header file though? It's not immediately obvious to me under w
...

💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1996715147)
Might be worth pointing out that the api is designed towards allowing for an implementation that stores the transitive closure of dependencies -- when querying, if B spends C, then you can't differentiate between A spends B and A spends B and C.