👍 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";`.

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716680866)
style nit in 6a256b96af8c94804538eb9e78964557c032b74f: I think in tests it is more important that they are short, easy to read, and write. Compile time enforcements are neat at best, but shouldn't be a goal. Generally, the unit tests are single threaded and fully deterministic, so if the test ran once (and passed), it should always pass.

So my preference would be to just call `data = *Assert(TryParseHex(str))` inside `ScriptFromHex`.

This makes the diff smaller and also ensures that the sa
...

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716703571)
in 29090eca423327353b474615d02ed7c3190e4a50: When touching all of those lines, I wonder if it makes sense to switch them to `std::byte`, but that will probably be more involved.

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716693711)
ToByteVector seems to be called in a few places, though mostly in tests.

I wonder if it makes sense to just go with https://github.com/hodlinator/bitcoin/blob/2024-07/uint256S_consteval_VecFromHex/src/util/strencodings.h#L95-L110

💬 stickies-v commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1716712062)
That makes sense, but I don't quite see the case for changing `GetNetworkForMagic` to accept a span in the future, so it feels a bit like premature optimization to me, but I may be missing context.

It's a nit, and the commit is orthogonal to the PR anyway, so this is the last I'll comment on it - up to you what you prefer, but I feel like this would probably be the most elegant implementation:

```cpp
std::optional<ChainType> GetNetworkForMagic(const MessageStartChars& message)
{
sta
...