🤔 vasild reviewed a pull request: "net: Replace libnatpmp with built-in PCP implementation"
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2061123015)
Concept ACK

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1603565742)
The following patch uses a netlink socket to get the information from the kernel, that is supported on (at least) Linux and FreeBSD>=13.2:

<details>
<summary>[patch] get default gateway using a netlink socket</summary>

```diff
--- a/src/test/netbase_tests.cpp
+++ b/src/test/netbase_tests.cpp
@@ -1,25 +1,34 @@
// Copyright (c) 2012-2022 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licen
...

💬 hebasto commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#issuecomment-2115606470)
Concept ACK.

💬 ajtowns commented on pull request "policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only)":
(https://github.com/bitcoin/bitcoin/pull/29843#issuecomment-2115614319)
Bumped past #29086

💬 ryanofsky commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2115622164)
> Could this be a case for a clang-tidy plugin?

I think the ideal thing to do for types that are potentially expensive to copy is to disable implicit copies, but provide explicit `T Copy() const` and `void CopyFrom(const T&)` methods so copies can be made where needed. (There was a `DISABLE_IMPLICIT_COPIES` macro proposed in #24641 that tried to do this, but it didn't seem possible to generalize the implementation so we never added it.) It think it would be probably be reasonable to allow uni
...

💬 instagibbs commented on pull request "policy: restrict all TRUC (v3) transactions to 25KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2115624871)
From a more general wallet perspective if we're assuming ~110vbytes of overhead per additional transaction required to match an input/output set, picking 10kvB as the new limit vs 100kvB results in 10*110=1,100vb, or ~1% additional overhead vs 100kvB. At 5kvB you'd be receiving ~2% penalty, and so on.

FWIW 10kvB also seems like plenty of headroom for practical LN channels with >60-HTLCs-per-direction, but below today's spec limit.

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