📝 frankomosh converted_to_draft a pull request: "fuzz: add target for `DifferenceFormatter` and invariant check in `net_processing.cpp`"
(https://github.com/bitcoin/bitcoin/pull/33252)
Adds a fuzz test for the [`DifferenceFormatter`](https://github.com/bitcoin/bitcoin/blob/e3f416dbf7633b2fb19c933e5508bd231cc7e9cf/src/blockencodings.h#L22-L42) (used in [`BlockTransactionsRequest`](https://github.com/bitcoin/bitcoin/blob/master/src/blockencodings.h#L44-L54), [BIP 152](https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki)). The DifferenceFormatter class implements differential encoding for compact block transactions (BIP 152). This PR ensures that its strictly-monotonic
...

🤔 l0rinc requested changes to a pull request: "net: Prevent node from binding to the same `CService`"
(https://github.com/bitcoin/bitcoin/pull/33231#pullrequestreview-3149500267)
Concept ACK

I personally would not try to correct the behavior (especially silently), given that it's rare, I would simply give the user a warning or error and let them fix the arguments.

I also think the tests are a bit complicated and more stateful now, I have added a few suggestions, maybe we can simplify based on them.

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2296803517)
output parameters are quite hard to reason about - given that the call-site assigns `connOptions` directly, can we either split this into 2-3 helper methods that return the new values (the vectors are tiny, they're likely as fast as passing it around), or if you insist on this big one, can we pass a single `CConnman::Options& connOptions` instead?
Also, some branches return, others don't erase, others replace one of the inputs - seems like has more than a single, easily defined responsibility (
...

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2296801852)
nit:
```suggestion
if (whitebind_services.contains(bind)) {
```

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2296806927)
nit:
```suggestion
auto it = std::ranges::remove_if(onion_binds,
```

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2296866701)
do we really need to recreate this or can we reuse `seen_whitebinds` or even better, just iterate `vWhiteBinds` values directly?

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2296866208)
as mentioned before, I think we can safely check `vBinds` directly - it's simpelr and likely even faster than this

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2296873010)
nit: in first case `increment` means +1, here it's `+1` - but more generally, the code is already, it's enough if we just document the intent:
```suggestion
port += 2 # avoid conflicts since we've used two ports before
```

----

or even better, why are we even reusing the same variable, why not create a port iterator and instead of the list a lambda accepting two ports something like (untested, since this test is skipped on a Mac):
```python
def run_test(self):
lp = addr_t
...

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2296807196)
Since we don't expect this to happen often, I guess it's enough to stop at the first error instead of listing all 👍

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2297089974)
I find it a bit inconsistent that we're not erring here - we don't know why they provided the same thing twice, seems like a mistake, not sure we should attempt to correct it silently. Would also simplify the code if we unified the way we handle these

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2297095727)
nit: would make sense to invert this if wee keep this format, it makes a bit more sense to use `it` in the condition where we're checking it (would also enable embedding it in the condition).
Not sure this applies if you remove the reundant deduplication code and use iteration instead of sets. I think that would simplify all of this considerably.

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2297097850)
nit: if it's a regex, we need to escape some chars

💬 maflcko commented on pull request "fuzz: add target for `DifferenceFormatter` and invariant check in `net_processing.cpp`":
(https://github.com/bitcoin/bitcoin/pull/33252#issuecomment-3218891081)
@frankomosh Is this LLM generated?

✅ maflcko closed a pull request: "fuzz: add target for `DifferenceFormatter` and invariant check in `net_processing.cpp`"
(https://github.com/bitcoin/bitcoin/pull/33252)

💬 maflcko commented on pull request "fuzz: add target for `DifferenceFormatter` and invariant check in `net_processing.cpp`":
(https://github.com/bitcoin/bitcoin/pull/33252#issuecomment-3219006626)
Thanks, but the value of purely LLM generated "vibe" pull requests is limited, because:

* The "author" does not understand the changes and can not reply to code review feedback.
* There are more than 300 pull requests (most written by real humans) waiting for review.
* Incentive to review purely LLM generated changes is limited, because the feedback likely does not help to train a real human planning to contribute in the future, but is likely only passed on to an LLM api.

Also, specific
...

📝 maflcko reopened a pull request: "fuzz: add target for `DifferenceFormatter` and invariant check in `net_processing.cpp`"
(https://github.com/bitcoin/bitcoin/pull/33252)
Adds a fuzz test for the [`DifferenceFormatter`](https://github.com/bitcoin/bitcoin/blob/e3f416dbf7633b2fb19c933e5508bd231cc7e9cf/src/blockencodings.h#L22-L42) (used in [`BlockTransactionsRequest`](https://github.com/bitcoin/bitcoin/blob/master/src/blockencodings.h#L44-L54), [BIP 152](https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki)). The DifferenceFormatter class implements differential encoding for compact block transactions (BIP 152). This PR ensures that its strictly-monotonic
...

💬 maflcko commented on pull request "fuzz: add target for `DifferenceFormatter` and invariant check in `net_processing.cpp`":
(https://github.com/bitcoin/bitcoin/pull/33252#issuecomment-3219107162)
Re-opening, based on request by @dergoegge

🤔 maflcko reviewed a pull request: "fuzz: add target for `DifferenceFormatter` and invariant check in `net_processing.cpp`"
(https://github.com/bitcoin/bitcoin/pull/33252#pullrequestreview-3150210173)
(left my previous comment as in-line review comments)

💬 maflcko commented on pull request "fuzz: add target for `DifferenceFormatter` and invariant check in `net_processing.cpp`":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2297310457)
in 358ad3f7df44e2715b69e80fc973e4f55517a67c: This is not around "validation", the commit title should start with "p2p" and the pull request title as well. (This code is run in production, not only during fuzz tests)

💬 maflcko commented on pull request "fuzz: add target for `DifferenceFormatter` and invariant check in `net_processing.cpp`":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2297313734)
nit: should have a comment to be based on `BlockTransactionsRequest`?