💬 edilmedeiros commented on pull request "test: remove unneeded `-maxorphantx=1000` settings":
(https://github.com/bitcoin/bitcoin/pull/30133#discussion_r1605890277)
I don't see this as a problem per se. [test/functional/feature_rbf.py](https://github.com/bitcoin/bitcoin/blob/058af75874ffa2b4064e3d6d30cc50f0ec754ba8/test/functional/feature_rbf.py#L38) already had the same pattern.

💬 edilmedeiros commented on pull request "test: remove unneeded `-maxorphantx=1000` settings":
(https://github.com/bitcoin/bitcoin/pull/30133#issuecomment-2118991469)
Tested ACK 8950053636cb38ed85fe2d58b53e5d0acb35c390

The changes don't break the tests.
I took some time to think if the removed params would be essential, but can't see how.

🤔 AngusP reviewed a pull request: "contrib/signet/miner: increase miner search space"
(https://github.com/bitcoin/bitcoin/pull/30130#pullrequestreview-2064943454)
Approach ACK, one suggestion to explicitly check for the exhaustion case and a couple of nits

💬 AngusP commented on pull request "contrib/signet/miner: increase miner search space":
(https://github.com/bitcoin/bitcoin/pull/30130#discussion_r1605890224)
The `subprocess.CalledProcessError` could contain the stderr&out and the return code -- given there are a couple of other cases where `bitcoin-util grind` can fail, it might be good to check we actually hit an exhaustion rather than the block header decode issue to prevent many retries when something else is wrong (even if in a rare/"should never happen" case)?

```suggestion
if e.returncode == 1 and "Could not satisfy difficulty target" in e.stderr.decode("utf-8"):

...

💬 AngusP commented on pull request "contrib/signet/miner: increase miner search space":
(https://github.com/bitcoin/bitcoin/pull/30130#discussion_r1605888170)
nit: given this is a debug log message and you're "expecting" this exception, I'm not sure it makes sense to include the word `exception`? Implies something went wrong.

```suggestion
logging.debug('grind subprocess: {}'.format(e))
```

nit-ier: could use a shorter `f` string:

```suggestion
logging.debug(f'grind subprocess: {e}')
```

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