💬 fjahr commented on pull request "[29.x] backport #33212":
(https://github.com/bitcoin/bitcoin/pull/33251#issuecomment-3218400051)
> Can you add the GitHub-Pull / Rebased-From metadata to each commit.

done

💬 ariard commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-3218427453)
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Concept ACK.

I'm not going to enter into the polemic about what should be or not the broader consensus process.
During the year of 2022 and 2023, in reaction to the failed CTV activation attempt, I did launch
and maintain for roughly a year the bitcoin contracting WG on IRC open to everyone, as a transparent
forum to discuss consensus changes, in the goal to make real and step-by-step progress among all the
domain experts.

While, I d
...

💬 151henry151 commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3218539977)
My Guix build resulting binary hashes:

`75d4eb10b95a64c595ee1ddf31d3c1e0086958a1ae3d64f9e2bbc4930cd88366 dist-archive/bitcoin-e922e27a2ecd.tar.gz
f7d2d6cd820561229ab5fa476c06484ced81a1fa8b90d2fbbd7a82fa915aa956 x86_64-linux-gnu/bitcoin-e922e27a2ecd-x86_64-linux-gnu-debug.tar.gz
df24c2269cf96dbf011692fbc0b21dab7786abd43dbb82e4351f0117e95169e5 x86_64-linux-gnu/bitcoin-e922e27a2ecd-x86_64-linux-gnu.tar.gz`

📝 frankomosh opened 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
...

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