💬 Sjors commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r1998898227)
The Required Dependencies would defeat the purpose of depends. at least `boost` and `libevent`. But maybe `pkgconf` and `cmake` should be added to the brew install incantation?

💬 polespinasa commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1998903473)
I agree. If it is not done in a separate PR we can simply add it back into the PR where we remove the RPC call. We will have to leave the test to its default form again anyway.

👍 darosior approved a pull request: "refactor: Make node_id a const& in RemoveBlockRequest"
(https://github.com/bitcoin/bitcoin/pull/31282#pullrequestreview-2690856800)
utACK fa21f83d2983d97006ec1e3c47634dc0fe0349dc

💬 laanwj commented on pull request "build: Drop option to disable hardening.":
(https://github.com/bitcoin/bitcoin/pull/32071#issuecomment-2729857146)
> The only case I'm aware of where some hardening flags cause an issue is on NetBSD 10, and this is expected to be fixed in NetBSD 11.

Right. Not sure if it's worth it in this specific case, as it's going to be fixed upstream soon, but imo in general if some flags (hardening or otherwise) are a problem on some platform this should be taken into account by the build system automatically instead of having the user disable hardening entirely, which is a hassle and creates a bigger risk than nece
...

💬 yancyribbens commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2729873842)
I did some testing on this issue and I found that if the [parameter](https://github.com/bitcoin/bitcoin/blob/eb9730ab65887279309af338ad0201924f945ac5/src/wallet/coinselection.cpp#L177) is removed, one of [tests](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L427) finishes in 3 fewer iterations, from 38 iterations to 35. The reason the test is more efficient without the added parameter is that there are two UTXOs with the same effective value of `5000000 s
...

🤔 ismaelsadeeq reviewed a pull request: "[29.x] backports and rc2"
(https://github.com/bitcoin/bitcoin/pull/32062#pullrequestreview-2690931188)
Code review ACK 74df31cb0bdef9cce31ae62ed71a1e386cba0274

I've verified that the patch-ids are identical. Only 4e438d326ea55ac0f98f89e41e69b56354e801e7 has a different patch-id from the original commit, and that's due to line number differences.

I've verified that the backported commits is identical to the PR commits.

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