👍 hebasto approved a pull request: "bench: enable wallet creation benchmarks on all platforms"
(https://github.com/bitcoin/bitcoin/pull/30122#pullrequestreview-2061248474)
ACK 7323bf6a2ecc3a2f1e2fdebce53b961767b06e08.

Maybe deduplicate code:
```diff
--- a/src/bench/wallet_create.cpp
+++ b/src/bench/wallet_create.cpp
@@ -35,8 +35,8 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted)
std::vector<bilingual_str> warnings;

int round = 0;
- fs::path wallet_path = test_setup->m_path_root / strprintf("test_wallet%d_%d", round, random.rand32()).c_str();
bench.run([&] {
+ auto wallet_path = test_setup->m_path_root / s
...

💬 vasild commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1603675011)
Added a comment.

💬 vasild commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#issuecomment-2115666825)
`c5f9afd946...d35ba1b3f1`: add a comment as suggested above

🤔 furszy reviewed a pull request: "test: improve robustness of connect_nodes()"
(https://github.com/bitcoin/bitcoin/pull/30118#pullrequestreview-2061247415)
> @furszy Were you able to identify few connections that were dropped in logs of this CI run? https://cirrus-ci.com/task/5805115213348864?logs=ci#L3200

Just one. One of the `P2PInterface` connections I create on the test side gets disconnected after advancing the node time prior to connecting the test nodes again. Need to expand the complete CI logs to find it.
But the fragility is easy to reproduce. Just launch a thread that disconnects a node before calling `connect_nodes()`.

💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1603654247)
> Looking at the sample output of `subversion` - `"subversion": "\/Satoshi:25.1.0\/"`, it doesn't seem unique enough because multiple nodes can be running the same version. Won't this cause issues in `find_conn` later?

The test framework appends the node number to the user agent string. See [test_node](https://github.com/bitcoin/bitcoin/blob/2f53f2273da020d7fabd7c65a1bc7e69a31249b2/test/functional/test_framework/test_node.py#L109).

💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1603657305)
> Might as well make these calls once and store in variable instead of finding 3 times? Unless I'm missing something that requires these calls to be made every time.

The code waits until the data arrives. These requires polling for updates.
We could couple the checks in this way:
```python3
self.wait_until(lambda: (peer := find_conn(from_connection, to_connection_subver, inbound=False)) is not None
and peer['version'] != 0

...

💬 furszy commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#issuecomment-2115677153)
Applied, thanks @hebasto 👍🏼.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1603770501)
Huh interesting. i didn't know netlink worked for multiple operating systems, that's much better.

💬 ryanofsky commented on pull request "JSON-RPC request Content-Type is application/json":
(https://github.com/bitcoin/bitcoin/pull/29946#issuecomment-2115854243)
Code review ACK f90a84d61505443fd3bb83253c091590b3dc7f45, but I think it would be helpful to change description to something like "doc: specify json content type in rpc examples" because the current description doesn't make it obvious that this is a documentation change, not a change in behavior.

PR also needs to be rebased since #27101 was just merged and it conflicts

📝 theStack opened a pull request: "test: improve BDB parser (handle internal/overflow pages, support all page sizes)"
(https://github.com/bitcoin/bitcoin/pull/30125)
This PR adds missing features to our test framework's BDB parser with the goal of hopefully being able to read all legacy wallets that are created with current and past versions of Bitcoin Core. This could be useful both for making review of https://github.com/bitcoin/bitcoin/pull/26606 easier and to also possibly improve our functional tests for the wallet BDB-ro parser by additionally validating it with an alternative implementation. The second commits introduces a test that create a legacy wa
...

💬 laanwj commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2115934921)
Concept ACK.

Agree with @ryanofsky that if copy is something expensive (or generally undesirable) to do, it would make sense to make copy explicit, so that it stands out in code review.

💬 1ma commented on issue "contrib/signet/miner: grind will fail for high difficulty chain":
(https://github.com/bitcoin/bitcoin/issues/30102#issuecomment-2115939051)
Thanks for the pointer. A bit off-topic, but since there are not so many people yet who are familiar with operating a signet let me ask you a follow up question:

I'm don't really understand why the signet miner has so much logic and is so opinionated. My initial idea was to keep the difficulty as low as possible then fire a cronjob every 10 minutes to insta-mine a block at the current timestamp, so the consensus algorithm would be tricked into thinking that is the real difficulty.

I was ne
...

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1603876259)
Is it possible to get the `scope_id` for IPv6 addresses? At least my router gives me an link-scope address.

✅ achow101 closed an issue: "Wrong block mined time in testnet"
(https://github.com/bitcoin/bitcoin/issues/30121)

💬 achow101 commented on issue "Wrong block mined time in testnet":
(https://github.com/bitcoin/bitcoin/issues/30121#issuecomment-2115970264)
This is expected behavior. `UpdateTip` logs the timestamp stored in the block header, which can be in the future and not actually match the time the block was mined. Testnet miners are known to be messing around and doing weird things like this.

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1603902055)
`m_package_feerates` is experiencing mission creep here. Really this is asking "was I called in AcceptSingleTransaction"?

Should we bikeshed this to make it clearer @glozow ?

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1603904807)
Good catch. It looks like BDB doesn't actually set last_pgno to the correct value, so this check is too strict. I've commented it out entirely.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2115990626)
Replaced Linux-specific `QueryDefaultGateway` with @vasild's netlink implementation for Linux and FreeBSD. This may even generalize to more UNIX variants.

Will tackle Windows next.

👍 hebasto approved a pull request: "util: avoid using thread_local variable that has a destructor"
(https://github.com/bitcoin/bitcoin/pull/30095#pullrequestreview-2061691479)
re-ACK d35ba1b3f16071b8fe9b36398ba15352dbf2a54d.

👍 hebasto approved a pull request: "bench: enable wallet creation benchmarks on all platforms"
(https://github.com/bitcoin/bitcoin/pull/30122#pullrequestreview-2061694682)
re-ACK b6b2d822eb06d705e6ec4318211163e5862008a6.