👍 maflcko approved a pull request: "net: make the list of known message types a compile time constant"
(https://github.com/bitcoin/bitcoin/pull/29421#pullrequestreview-2063640367)
Sorry for missing the ping. Now it looks good, because the only change is vector->array and some style fixups.

utACK b3efb486732f3caf8b8a8e9d744e6d20ae4255ef 🎊

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdY
...

💬 maflcko commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#discussion_r1605141730)
why is the `random` needed, when using a counter?

💬 maflcko commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#discussion_r1605146110)
is it required to remove this remove_all call? I read the commit message:

> This approach will provide more accurate benchmark results,
regardless of any delays the OS-dependent directory removal
call could have.

However, there is also a call to `RemoveWallet`, and `UnloadWallet` in this `WalletCreate` benchmark. The additional overhead of the `remove_all` should be fine? The benefit would be that the number of folders is kept constant and does not pile up as the bench runs, which could
...

🤔 mzumsande reviewed a pull request: "kernel: De-globalize fReindex"
(https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2061798427)
Code Review ACK b47bd959207e82555f07e028cc2246943d32d4c3

💬 mzumsande commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1603985526)
Unrelated, but this comment and the `needs_init` variable are confusing. Originally (https://github.com/bitcoin/bitcoin/commit/eda888e57352037ab2e60f6ef90098b3ce23a157) it may have made sense, but now there is just a log message left which doesn't even seem to belong here (no db initialization is happening here). So I think this could be cleaned up, but no need to do it here.

💬 maflcko commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#discussion_r1605154146)
while touching: Instead of c_str, could use `PathFromString`, because the `c_str` interface requires ascii-encoding, and ascii-encoded bytes are unchanged in unicode (via `PathFromString`).

🤔 furszy reviewed a pull request: "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs"
(https://github.com/bitcoin/bitcoin/pull/28979#pullrequestreview-2063691370)
utACK 71aae72e1f

💬 maflcko commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605161357)
Any reason to have a duplicate `None` check for `peer` at this point? Shouldn't it be enough to have the assert in the second-to-previous line?

💬 laanwj commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2117828303)
> I did test an earlier draft of this pull request and the only thing I noticed was that on a fresh desktop install of a Linux distro, sometimes a package would be missing, and the GUI would fail to start.

Do you have specific output or details? That isn't expected behavior. This PR doesn't change the libraries required to be installed on the user's system, all the dependencies that this removes were already built only to link against. The only difference is that they're loaded at runtime usi
...

💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#issuecomment-2117828829)
I've updated the PR to have different child class implementations for `EncryptedP2PState` based on the disconnection scenario we're testing - `EarlyKeyResponseState`, `ExcessGarbageState` etc...

Also introduced 2 more commits for cleaner code:
- c642b08c - logging when the garbage is sent instead of when garbage is generated (suggestion from https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466826427 which is useful now that we have multiple child classes for `EncryptedP2PState`)
-
...

💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1605174508)
done.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1605180790)
done in #29431 actually

💬 maflcko commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2117837985)
> Do you have specific output or details?

The DrahtBot builds (as well as the log outputs) are deleted by now, as they are short-lived test-only assets. I'll create fresh DrahtBot builds and try again with the current state of this pull request.

💬 maflcko commented on issue "Testsuite for Bitcoin Core 27.0.0 - FAIL: qt/test/test_bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30020#issuecomment-2117847337)
> I've overridden some default behaviour of the configure script like: --prefix=$HOME --with-incompatible-bdb --with-gui=no
>
> "Next, run the configure script to automatically discover all the necessary libraries and create a customized build script for your system:"
>
> $ ./configure

test_bitcoin-qt is a GUI executable. Are you sure you passed `--with-gui=no`?

Please share the configure summary after you disable the gui:

```

./configure --prefix=$HOME --with-incompatible-bdb
...

💬 maflcko commented on issue "Testsuite for Bitcoin Core 27.0.0 - FAIL: qt/test/test_bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30020#issuecomment-2117848777)
It should say something like:

```
Options used to compile and link:
external signer = yes
multiprocess = no
with wallet = yes
with sqlite = yes
with bdb = no
with gui / qt = no
...

💬 maflcko commented on issue "Testsuite for Bitcoin Core 27.0.0 - FAIL: qt/test/test_bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30020#issuecomment-2117851678)
Also, is there a need for you to be using BDB? It is deprecated and going forward it will be easier for you if you used sqlite from the start, if you do not need backward compatibility.

💬 maflcko commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1605200936)
nit: For new code it would be good to use `LogInfo`, not the deprecated and confusing alias `LogPrintf`.

💬 tdb3 commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#discussion_r1605207235)
In general, for tests I think the inclination should be to get data from direct sources (e.g. RPC) rather than from the debug log, but this instance seems like an edge case where searching the debug log might make sense (e.g. could match what's in the log with what the RPC call response contains).

I briefly looked for this capability, but didn't see it (I was surprised, so maybe I'm overlooking something simple). If others would like this capability, I'm thinking it's probably something to h
...

💬 furszy commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#discussion_r1605211026)
> However, there is also a call to RemoveWallet, and UnloadWallet in this WalletCreate benchmark. The additional overhead of the remove_all should be fine? The benefit would be that the number of folders is kept constant and does not pile up as the bench runs, which could (in theory?) lead to issues when running for a long time?

hmm, I don't think they are comparable. The overhead of `remove_all` depend on external factors such us the OS and the hard drive. But agree that running this for a l
...

💬 maflcko commented on pull request "build: Enable `thread_local` for MinGW-w64 builds":
(https://github.com/bitcoin/bitcoin/pull/30099#issuecomment-2117871458)
utACK 5822a82a88e46ef08e5c18c41489ffd3e9d899c6