💬 maflcko commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-2117730077)
> My fuzzer is crashing when I run this. I'm going to look more into it tomorrow.

Please indicate the traceback, and the sanitizers you used, also the fuzz input file.

💬 sipa commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2117736423)
@laanwj Yeah, fair enough. Absent any reports of "doesn't work on my router while nat-pmp worked" (which we're quite unlikely to get) there is really no way to assess that.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2117749832)
> Yeah, fair enough. Absent any reports of "doesn't work on my router while nat-pmp worked" (which we're quite unlikely to get) there is really no way to assess that.

For what it's worth, initial support for PCP was added in miniupnpd in [2013](https://github.com/miniupnp/miniupnp/commit/9e1ffd5cd9836053cfd83d95014c729ad0e36872). So the implementation wasn't lagging much behind the standard. Of course, not all routers use miniupnpd but it's extremely common.

💬 maflcko commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1605125827)
unrelated change, but looks fine

👍 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.