💬 instagibbs commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#issuecomment-2117642767)
thanks for splitting this up, next on my review docket

👍 rkrux approved a pull request: "test: add conflicting topology test case"
(https://github.com/bitcoin/bitcoin/pull/30066#pullrequestreview-2063486974)
reACK [9365baa](https://github.com/bitcoin/bitcoin/pull/30066/commits/9365baa489e123d9bcaf986e4311d3fa3f1e3f88)

💬 maflcko commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2117664043)
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. If this is expected, I guess the docs should be adjusted to clarify that users would have to install the packages themselves?

💬 furszy commented on pull request "refactor: interfaces, make 'createTransaction' less error-prone ":
(https://github.com/bitcoin-core/gui/pull/807#issuecomment-2117677284)
CI failure is unrelated.

💬 rkrux commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r1605053958)
Extra spaces after 0?

🤔 rkrux reviewed a pull request: "test: create assert_not_equal util"
(https://github.com/bitcoin/bitcoin/pull/29500#pullrequestreview-2063512473)
crACK [f9ba6fd](https://github.com/bitcoin/bitcoin/pull/29500/commits/f9ba6fd46d454d0a56662db751675a6520e1c9f6)

I'm unable to run make successfully because of the following error on my system. I've noticed this in another PR as well and it is due to lack of a commit that was merged in later.
Asked couple questions.

```
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/time.h:127:12: note: 'gmtime_r' declared here
struct tm *gmtime_r(const time_t * __restrict, struct tm *
...

💬 rkrux commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r1605063804)
`"node 0 chain did not reorganize"`
This seems to be the 3rd argument to the `assert_not_equal()` call, i.e. `*args`. Will this piece be kicked in then? `any(thing1 == arg for arg in args)`

I don't get why would we need to search for `main_block_hash` in the above error string.

💬 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