💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2729878994)
Thanks for the reviews!

Rebased e7b6abb4f829abe6818d058fc7865b6a003d8a9e -> d6f218d967111a6525b744a327ed9946498bdb77 ([`pr/subtree.22`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.22) -> [`pr/subtree.23`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.23), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/subtree.22-rebase..pr/subtree.23)) due to conflict with #32070 and implementing some suggestions.
Updated d6f218d967111a6525b744a327ed9946498bdb77 -> 8ef8f799b0
...

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1998871564)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1998673375

Moved this up a level now. Thanks for the suggestion.

👍 vasild approved a pull request: "doc: Update documentation to include Clang/llvm based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2690964671)
ACK 3c69b071ac4964870abb347f9d9c15df2e60aa62

Thank you!

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

💬 saikiran57 commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1999016132)
done

💬 mzumsande commented on pull request "validation: Make ReplayBlocks interruptible":
(https://github.com/bitcoin/bitcoin/pull/30155#issuecomment-2730000225)
> It appears to me that the size of the cache used in ReplayBlocks is never checked if it grows too large. Wouldn't this mean that if the node was sufficiently far behind, the rolling forward would fill up the cache to levels exceeding dbcache? This would result in a cycle of rolling forward and then OOM crashing, never being able to write any chainstate to disk.

Good point - I think that is true, `ReplayBlocks()` uses its own cache, which doesn't seem to be subject to any limits. However, th
...

💬 theuni commented on pull request "build: align debugging flags to `-O0`":
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2730027918)
Fwiw I do think it's interesting to test this _somehow_ now and then to surface bugs like that. I'm just not convinced that `-O0` is necessary for every debug build.

💬 ryanofsky commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2730033762)
> rfm?

Agree this looks ready and I could merge it, but it's assigned to @achow101 and she has the most expertise here and didn't comment on the PR at all yet, so I'd be inclined to wait. But feel free to ping me if anyone thinks this should be merged sooner.

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