💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1716512192)
The return value is used here:

```cpp
// This is done when we get a PONG from the peer. Repeat here too in case we never receive a PONG.
if (node.IsPrivateBroadcastConn() &&
m_tx_for_private_broadcast.FinishBroadcast(nodeid, /*confirmed_by_node=*/false)) {
...
log a message that we never got a PONG response
```

The point is to avoid printing this log message in the happy path where we do get a PONG response.

💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2288154025)
> What is the expected speed up?

Probably depends on the machine, but it should be +30% or so.

💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2288228196)
I see another 5% in the ECC_Start flame on my machine, which can be avoided, so I'll try to rework the second commit.

💬 paplorinc commented on pull request "doc: Change `nproc` in docs to `getconf _NPROCESSORS_ONLN`":
(https://github.com/bitcoin/bitcoin/pull/30619#issuecomment-2288241884)
Thanks @maflcko, I would actually prefer mentioning nproc (+ alternatives) only once in the docs - will be easier once the cmake PR updates are merged.

👍 hebasto approved a pull request: "Migrate legacy wallets that are not loaded"
(https://github.com/bitcoin-core/gui/pull/824#pullrequestreview-2237595730)
ACK 8f2522d242961ceb9e79672aa43e856863a1a6dd.

💬 hebasto commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#issuecomment-2288250542)
cc @furszy

💬 brunoerg commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1716596528)
Oh sorry, missed that. It's fine.

💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2288318410)
Force pushed for :rocket: lazy re-init, which gives:

* 100x iterations/s in the lazy case
* 1.35x iterations/s in the expensive case

🤔 glozow reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2237602476)
Thanks @theStack!

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716582838)
Ah, that looks like a bad rebase - deleted the comment now, thanks

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716644034)
Right, I don't think it's problematic to not log tx invs during IBD since they are ignored anyway.

I've moved the logging change to its own commit, to keep the pure refactoring-ness of this commit :+1:

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716644076)
Ah, good observation. I've now gated the `AlreadyHaveTx` check inside of `AddTxAnnouncement` on `p2p_inv` so this is no longer happening.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716591125)
done :+1:

👍 theStack approved a pull request: "doc: Deduplicate list of possible chain strings in RPC help texts"
(https://github.com/bitcoin/bitcoin/pull/30648#pullrequestreview-2237733485)
ACK 9b297555207b4ea54bc0051f09c7084797aa9def

💬 hebasto commented on pull request "depends: Set `CMAKE_SYSTEM_VERSION` for CMake builds":
(https://github.com/bitcoin/bitcoin/pull/30465#issuecomment-2288387364)
> I just wonder, why the change to `depends/funcs.mk` is not in `hebasto/cmake-staging`, but is in this PR?

All changes to `depends/funcs.mk` from this PR must be ported to `hebasto/cmake-staging`.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2288403119)
Would it help if I put the test/fuzz changes in a separate PR from the refactoring changes? It would make the first PR less exciting to review, but perhaps easier to review?

👍 danielabrozzoni approved a pull request: "doc: Deduplicate list of possible chain strings in RPC help texts"
(https://github.com/bitcoin/bitcoin/pull/30648#pullrequestreview-2237782147)
ACK 9b297555207b4ea54bc0051f09c7084797aa9def

🤔 maflcko reviewed a pull request: "refactor: Replace ParseHex with consteval ArrayFromHex"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2237724166)
Left some style nits / questions, feel free to ignore.

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716660247)
style nit in 29090eca423327353b474615d02ed7c3190e4a50: Can drop the trailing `\`, since this is a normal function and not a macro.

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716665464)
nit in 9dab917088e83f627786ed5caafec859a3481b78: If you change the behavior, it also needs to adjust the error string. Now it should say: `throw "Hex string must only contain lowercase hex chars";`.