💬 AngusP commented on pull request "test: remove unneeded `-maxorphantx=1000` settings":
(https://github.com/bitcoin/bitcoin/pull/30133#discussion_r1605890828)
Fair, it's a stylistic nit so just an opinion, can be disregarded

💬 AngusP commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605892537)
Worth adding a note to the comment?

```python
# Use subversion as peer id -- test nodes have their number appended to the UA
```

🤔 AngusP reviewed a pull request: "test: improve robustness of connect_nodes()"
(https://github.com/bitcoin/bitcoin/pull/30118#pullrequestreview-2064946909)
Approach ACK, some nits/questions

💬 AngusP commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605892564)
Worth adding a note to the comment?

```python
# Use subversion as peer id. Test nodes have their node number appended to the UA
```

💬 AngusP commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605893483)
This comment now doesn't really make sense to me?

Maybe
```suggestion
# wait until peers have completed a version handshake and so match the
# expected connection subversion
```

💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605903225)
> This comment now doesn't really make sense to me?

Why? the procedure behavior is the same.

💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605903301)
> Worth adding a note to the comment?
>
> ```python
> # Use subversion as peer id. Test nodes have their node number appended to the UA
> ```

sure. Pushed.

💬 rustyrussell commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1605921836)
Sure, wasn't sure this was the place, but is there somewhere better? Seems like all the regulars "know" this, so putting it where a beginner might look is helpful.

Was thinking the version info is useful for backporting, but that's a limited window and is already in the commit message if people want.

💬 edilmedeiros commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1605924276)
I think adding this info here as you propose with the wording suggested by @laanwj would have some chance to be merged (I would ack it). Just don't expect it to attract much attention of reviewers since lacking this info doesn't seem to create much problem.

About backporting, even small nits of documentation are somewhat complex and is reserved to be done by a few maintainers from time to time. I don't believe it will happen with this since this is not critical and doesn't apply to earlier vers
...

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2119145090)
Squashed all fixme's into their logical commit. This should be in a good state for review now. The only TODO that is left to do is the Qt settings migration, i could use some help with that because i don't really know the new mechanism.

💬 GregTonoski commented on pull request "Policy: Enforce witness script size limit for tapscript":
(https://github.com/bitcoin/bitcoin/pull/29769#issuecomment-2119182536)
Could @glozow summarize cons, please? I couldn't find any in spite of huge amount of text in the comments. The PR does not introduce a "breaking change" contrary to what is claimed by opponents.

👍 AngusP approved a pull request: "test: improve robustness of connect_nodes()"
(https://github.com/bitcoin/bitcoin/pull/30118#pullrequestreview-2065044318)
tACK f4c588c98f163e9fa9083821ba36e1053f8c1496

💬 AngusP commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1606000057)
I think just the way it read with the old impl vs the new, "waiting until we find a peer connection matching some predicate" rather than "wait until the peer has a non-zero version" . But on second thoughts you're right it's not functionally different so disregard my suggestion

💬 marcofleon commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-2119185471)
Adjusted some things on my machine and used `-merge=1` and it successfully merges after several attempts. Thank you for the help @maflcko

💬 Psifour commented on pull request "Policy: Enforce witness script size limit for tapscript":
(https://github.com/bitcoin/bitcoin/pull/29769#issuecomment-2119197600)
You are asking the wrong question, instead it should be framed as "what technical threat is posed and is said threat sufficient to warrant the creation of a new standardness rule?" The answers to those questions are "none" and subsequently "no".

Applying rules meant to solve a specific problem to systems that can't/don't exhibit the same vulnerabilities would be wildly inappropriate. It would be similar to applying the bare multi-sig limit (3) to ALL multisigs.

TL;DR: The PR attempts to so
...

💬 GregTonoski commented on pull request "Policy: Enforce witness script size limit for tapscript":
(https://github.com/bitcoin/bitcoin/pull/29769#issuecomment-2119239759)
> ... policy rule in earlier Script version environments is to discourage constructs that suffer from DoS concerns in the interpreter (known and unknown) (...) It is of course possible there are oversights...

Can I ask to point to test results of the change introduced in TapRoot (relaxation of the 3600 bytes script size standard limit/mempool policy rule), please?

🤔 stratospher reviewed a pull request: "test: improve robustness of connect_nodes()"
(https://github.com/bitcoin/bitcoin/pull/30118#pullrequestreview-2065078997)
tested ACK f4c588c9.

very cool! i like how the logic depends only on the node we're connecting to.
reproduced the intermittent failure using this patch and verified that the new logic fixes it!

self.nodes[0].add_p2p_connection(P2PInterface(), send_version=False, wait_for_verack=False)
self.connect_nodes(0, 2)

also didn't observe that much of a time increase (only around 70 s) when running all the functional tests in my local config (without wallets).

💬 stratospher commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1606032360)
wondering if we need the inbound check because an inbound and an outbound connection to the same `TestNode` isn't done. or did you keep it for assurance about the connection direction?

💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1606039710)
Ok. The "version handshake" wording refers to the complete initial negotiation round.
(1) outbound send version, (2) inbound send version, (3) inbound send verack, (4) outbound send verack.

💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1606038839)
> wondering if we need the inbound check because an inbound and an outbound connection to the same `TestNode` isn't done. or did you keep it for assurance about the connection direction?

Bidirectional connections are allowed and quite common. E.g. in [p2p_disconnect_ban.py](https://github.com/bitcoin/bitcoin/blob/058af75874ffa2b4064e3d6d30cc50f0ec754ba8/test/functional/p2p_disconnect_ban.py#L117) or in [rpc_net.py](https://github.com/bitcoin/bitcoin/blob/058af75874ffa2b4064e3d6d30cc50f0ec754b
...