💬 maflcko commented on pull request "rest: rename `strURIPart` to `uri_part`":
(https://github.com/bitcoin/bitcoin/pull/32825#issuecomment-3018731998)
lgtm ACK 856f4235b1ae56540e1d2279c27405d44a5c7b34

👍 l0rinc approved a pull request: "p2p: add more bad ports"
(https://github.com/bitcoin/bitcoin/pull/32826#pullrequestreview-2970827023)
ACK 6967e8e8abbc35ac98e8e3745a8bbed56e77526f

Changes since my last review are comment updates and namespace inlines.

My only remaining concern is the unnecessary heavy memory usage of the test - but it's not a blocker.

🤔 vasild reviewed a pull request: "net: Fix Discover() not running when using -bind=0.0.0.0:port"
(https://github.com/bitcoin/bitcoin/pull/32757#pullrequestreview-2970826081)
Approach ACK 776b7ee585

💬 vasild commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#discussion_r2174818751)
Would be nice to keep the [length of lines in commit messages shorter](https://dev.to/noelworden/improving-your-commit-message-with-the-50-72-rule-3g79).

💬 vasild commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#discussion_r2174824246)
The testing framework will add `-bind=0.0.0.0` we want to test this. But we also want to test if no `-bind` is given at all. Here is a patch to do that and also to test `-whitebind=0.0.0.0`:

```diff
--- i/test/functional/feature_bind_port_discover.py
+++ w/test/functional/feature_bind_port_discover.py
@@ -26,23 +26,47 @@ from test_framework.util import (
ADDR1 = '1.1.1.1'
ADDR2 = '2.2.2.2'

class BindPortDiscoverTest(BitcoinTestFramework):
def set_test_params(self):

...

👍 theStack approved a pull request: "test: Add `msgtype` to `msg_generic` slots"
(https://github.com/bitcoin/bitcoin/pull/32833#pullrequestreview-2970865694)
ACK 7dc43ea503a2c145ffd4fe14b794300bfc2bcdee

👍 hebasto approved a pull request: "test: Turn util/test_runner into functional test"
(https://github.com/bitcoin/bitcoin/pull/32697#pullrequestreview-2970870539)
re-ACK fa2163159511a099ecffde2bfddf9cfe33eb9c76, additional feedback has been addressed since my previous [review](https://github.com/bitcoin/bitcoin/pull/32697#pullrequestreview-2940350432).

💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174849258)
> non-standardness of blocks

i.e. mined blocks that contain non-standard transactions - what's the accepted term for that?

> I don't get what you are asking here

Do we have data on how many main-chain transactions historically exceed the new threshold of 2500 legacy sigops?
I’d like to confirm that the new policy would block no real-world traffic and is purely a preventive DoS measure ahead of any future BIP-54 soft-fork activation.

💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174859753)
> We would not mine blocks with such transactions anymore, this is the whole point of this PR.

I don't see how this PR is doing that currently.
I understand that the point is to starve the miners of those pathological transactions before a future soft-fork makes the limit consensus-critical - but for now we have to make absolutely sure we don't accidentally do that and that it can still be mined, right? My question was if we can add that to the end of the test (which we can remove after we a
...

💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174862347)
> your code suggestion is strictly functionally equivalent

Yes, that's why I have suggested it:
> I understand that this corresponds to an empty vector, but maybe we can simplify it in the test

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2174867410)
Can you please tell me about the automated check you are referring to? I saw a test in `test/functional/rpc_help.py` which does similar things.

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174901563)
> The current implementation is ~66% slower

This is suggesting this PR makes transaction processing slower. It does not. The cost of adding an additional iteration through inputs is trivial compared to the total amount of work we do when processing a transaction. This seems silly to even state it. I don't know why you would fixate on this. Please let's spend our time on more important issues than removing the number of `for` loops used by the program.

> This repeats work currently

I'm a
...

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174904635)
I'm not planning on addressing this, as i think it would unnecessarily duplicate the check for no clear benefit.

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174917931)
Exactly. I think it's good to keep the benchmark but its results should not be over interpreted. It is focused on a tiny portion of the total work performed when processing a transaction. I locally have benchmarks of `ProcessTransaction` which i may share in the future. To give you an idea, they suggest (as can be expected from just reading the code) that total work in processing a transaction is *several* orders of magnitude higher than the work done in `AreInputsStandard`. And that's not even
...

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174918740)
Will address if i need to retouch.

💬 dergoegge commented on pull request "test: Add `msgtype` to `msg_generic` slots":
(https://github.com/bitcoin/bitcoin/pull/32833#issuecomment-3018918291)
> Could add a small smoke test, maybe in ./test/functional/p2p_ping.py, but seems fine either way

Not gonna do it here, but if someone wants to do it in a follow up, I can review

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174930831)
> I don't see how this PR is doing that currently.

We only create block templates from the mempool. This PR prevents these transactions from being included in our mempool. As a result, we would never mine a block which include such transactions.

> but for now we have to make absolutely sure we don't accidentally do that and that it can still be mined, right?

I think what you are trying to say here is "that we would still accept a block containing them". That is, that we haven't accident
...

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2174931525)
I won't be making this change.

🚀 fanquake merged a pull request: "rest: rename `strURIPart` to `uri_part`"
(https://github.com/bitcoin/bitcoin/pull/32825)

🚀 fanquake merged a pull request: "test: Add `msgtype` to `msg_generic` slots"
(https://github.com/bitcoin/bitcoin/pull/32833)