💬 maflcko commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1716448092)
Leaving as-is for now to not invalidate review. I'll follow-up with iwyu on all files, I guess.

💬 maflcko commented on pull request "doc: Change `nproc` in docs to `getconf _NPROCESSORS_ONLN`":
(https://github.com/bitcoin/bitcoin/pull/30619#issuecomment-2288076458)
> `nproc` is linux only

I don't think this is true. I don't have a mac, but I fail to see why installing it won't work. Also, a bash alias similar to `alias nproc="sysctl -n hw.logicalcpu"` or `alias nproc="getconf _NPROCESSORS_ONLN"` should work as well.

No objection to changing this, but I think the claim that `nproc` is linux-only may not be true.

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

💬 maflcko commented on issue "contrib: Automation for Bitcoin Full Node Deployment":
(https://github.com/bitcoin/bitcoin/issues/30638#issuecomment-2288148372)
> Regarding Dockerfile, I believe it can be considered part of Bitcoin Core Software, as a Bitcoin Core user I need it and I can see through the discussions that many need it, could someone enlighten me of what is needed to start accepting Dockerfile as part of Bitcoin Core Software? What are your requirements?

No one is holding anyone back from opening a pull request to add a container engine imagefile. Not sure how much actual interest is there, given that no one has done so far, or the pre
...

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