💬 optout21 commented on pull request "mempool: Avoid needless vtx iteration in `removeForBlock` during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2174806312)
I think it's a different question: does it need to be called ...

1. when the mempool is empty, or
2. when the list of transactions to be removed is empty.

💬 maflcko commented on pull request "mempool: Avoid needless vtx iteration in `removeForBlock` during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2174809229)
Probably only if `firstRecordedHeight` is zero, which would need to be checked inside.

💬 maflcko commented on pull request "mempool: Avoid needless vtx iteration in `removeForBlock` during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#issuecomment-3018723953)
> Similarly to [#32730 (comment)](https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2140691354),

Could make sense to include that change here as well?

💬 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_r2174811056)
> its already checked with `bind_on_any` bool.

I do not think so - the `bind_on_any` bool will be true if `-whitebind` and `-bind` are not given. The current patch will not run `Discover()` if `-whitebind=0.0.0.0` is used but it should. Here is an example patch to fix that:

```diff
--- i/src/init.cpp
+++ w/src/init.cpp
@@ -2030,12 +2030,20 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
if (bind_addr.IsBindAny()) {
shou
...

💬 hebasto commented on pull request "build, docs: Fix Boost-related issues on NetBSD":
(https://github.com/bitcoin/bitcoin/pull/32828#issuecomment-3018727545)
> > The boost-headers package does not provide CMake configuration files
>
> Is there an upstream issue for this?

https://github.com/NetBSD/pkgsrc/issues/168.

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