💬 rkrux commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#issuecomment-2117693875)
@kevkevinpal Did you use a tool to generate the verification script?

💬 sipa commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2117698802)
Do we have any information about real-world deployment of PCP vs NAT-PMP? I see PCP dates from 2013, but do we know if it was adopted immediately (and/or, how many older routing devices are in common use)?

RFC 6887 Appendix A explains how one can be compatible with both, though I don't know how much work it would be to implement.

💬 edilmedeiros commented on issue "dumpprivkey error":
(https://github.com/bitcoin/bitcoin/issues/30124#issuecomment-2117722655)
Duplicate of #30129 which has a working solution.

💬 sdaftuar commented on pull request "policy: restrict all TRUC (v3) transactions to 10KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2117722792)
Concept ACK making it 10kvb. Assuming we stick with that, then the commit message in the second commit should be updated (https://github.com/bitcoin/bitcoin/commit/1d95eee9ebfa17dee3c9cf60fea1c28a2cf5d8b5).

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2117723501)
i have no idea, also no idea how to get that information; to be honest if we can remotely avoid it, i'd prefer not to implement unnecessary compatibility stuff, this is already a lot to ask people to review as-is.

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