💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2301692222)
What's the advantage of duplicating the exact error here, wouldn't this suffice?
```suggestion
"Error: Duplicate binding configuration",
match=ErrorMatch.PARTIAL_REGEX)
```

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2301697493)
The doc at the top is slightly off now:
```
"""
Test starting bitcoind with -bind and/or -bind=...=onion, confirm that
it binds to the expected ports, and verify that duplicate or conflicting
-bind/-whitebind configurations are rejected with a descriptive error.
"""
```

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2301722439)
we could make this manual iteration more pythony with something like:
```python
from itertools import combinations_with_replacement
addr = f"127.0.0.1:11012"
for opt1, opt2 in combinations_with_replacement([f"-bind={addr}", f"-bind={addr}=onion", f"-whitebind=noban@{addr}"], 2):
print(f"opt1 = {opt1}, opt2 = {opt2}") # replace with assert, I just used this to validate if the iteration is the same
```

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2301731934)
I personally consider these comments noisy - all of this is already evident from the code. If it's not, I'd rather change the code. Seeing the unupdated comment below, we shouldn't just add extra work for our future selves.

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2301789293)
Is it expected that I'm seeing this twice?
```
2025-08-26T18:23:45Z [error] Duplicate binding configuration for address 127.0.0.1:11012. Please check your -bind, -whitebind, and onion binding settings.
2025-08-26T18:23:45Z tor: Thread interrupt
2025-08-26T18:23:45Z torcontrol thread exit
2025-08-26T18:23:45Z Shutdown in progress...
Error: Duplicate binding configuration for address 127.0.0.1:11012. Please check your -bind, -whitebind, and onion binding settings.
```

And if we're not ac
...

🤔 janb84 reviewed a pull request: "ci: use LLVM 21"
(https://github.com/bitcoin/bitcoin/pull/33258#pullrequestreview-3156831746)
ACK 4cf0ae474ba03830c86653f1abae4ab4d38c94e4

LLVM 21 has been released (on 26 August 2025 ~14:19 UTC) ✅
Changed CI's are running fine, they can download and run the new LLVM binaries. ✅

💬 polespinasa commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3225288108)
lgtm code review and tested ACK b7b249d3adfbd3c7b0c4d0499a86300f57982972
nothing to add that has not already been said in other comments

💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2301826701)
What do you mean by "for consistency"? It should work fine without specifying the other parameters. Requiring that all string parameters need to be added "for consistency" just makes it more annoying to add new RPCs and parameters.

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2301845877)
Okay, so to verify my claim, please remove these params from the table and recompile the code, and then run the functional tests with CLI. You will see some errors because of inconsistent behaviour of the class. Meaning the code needs to know about all the string parameters for the given method for correctness. And that's only needed for RPCs which might contain an `=` character; the rest can be part of the JSON table without any further rules.

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2301848604)
> Since these arguments are there, but not all RPCs that have these arguments, is why I ask about the criteria. Either we are including all string parameters because they may contain an =, or we are only including parameters that might reasonably contain a =

Any individual string that is allowed to contain an '=' should be to be listed in the string table so the new code in this PR can function and detect that these string arguments should be passed as positional parameters, not named paramet
...

💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2301863072)
> You will see some errors

Then that is not just "for consistency" and that shouldn't be listed as the reason for including the other parameters.

> Either all of a method's string parameters need to be listed in the table or none of them need to be listed for things to work properly.

I see. The comment does not explain this clearly.

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2301870735)
> I see. The comment does not explain this clearly.

If it helps, as mentioned earlier I wrote a comment which does try to explain this https://github.com/bitcoin/bitcoin/blob/b998cc52d51b48db9271fdba0bd69e9aaccb7999/src/rpc/client.cpp#L36-L42

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3225419035)
Code review ACK 6de6c78c676abd1edb96597bfe8b40bd5fbe7122. Just style and naming changes since last review.

Just to restate my opinion of this PR from [earlier](https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3039323143), I think it is not good to have separate tables for string and json parameters. I think it will make this code harder to maintain and it would be better in long run to have a single table with clearly spelled out rules for what parameters need to be included in
...

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2301950715)
> It seems like this might also make it more difficult to use `bitcoin-cli` across versions. If `bitcoind` is a newer version with some RPC that has a new string parameter, it may not be possible to pass that new argument by name with a `bitcoin-cli` from an older version if the RPC has an entry in this table.

This is true, and we know a similar issue already happens if a new JSON parameter is added: old clients will incorrectly pass that parameter as a string instead of a JSON value. This PR
...

💬 kevkevinpal commented on pull request "doc: unify `datacarriersize` warning with release notes":
(https://github.com/bitcoin/bitcoin/pull/33224#issuecomment-3225484378)
ACK [2885bd0](https://github.com/bitcoin/bitcoin/pull/33224/commits/2885bd0e1c4fc863a7f28ff0fd353f5cffb03442)

Makes sense to have consistent working with the release note

💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3225542636)
> I think it is not good to have separate tables for string and json parameters.

I would also prefer that.

💬 naiyoma commented on pull request "rpc: print P2WSH and P2SH redem Script in getrawtransaction":
(https://github.com/bitcoin/bitcoin/pull/31252#discussion_r2302086009)
For a non-P2SH transaction, returning` -1` will result in a `redeemScript` field being added.
I tested this using a P2PKH transaction, and this was the output: `"redeemScript": { "asm": "304402...7981", "type": "nonstandard" }"`

💬 achow101 commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3225666624)
ACK dd7e696762b4863a5f06d150c4fe3fb882ac69dc

🤔 mzumsande reviewed a pull request: "test: Fix CLI_MAX_ARG_SIZE issues"
(https://github.com/bitcoin/bitcoin/pull/33243#pullrequestreview-3157283355)
> * It claims to be a "limit per arg", but it is a "The maximum length of the arguments": See https://www.man7.org/linux/man-pages/man3/sysconf.3.html, section `ARG_MAX - _SC_ARG_MAX`.

the relevant bottleneck for me on Ubuntu was not `ARG_MAX`, but `MAX_ARG_STRLEN`, which accounts for the maximum, not the sum.
e.g. the folllowing passes
```
arg1=$(printf "%0*d" $((131072 - 1)) 0)
arg2=$(printf "%0*d" $((131072 - 1)) 0)
$(which printf) "%s %s" "$arg1" "$arg2" >/dev/null
```
whereas

...

🤔 mzumsande reviewed a pull request: "[29.x] backport #33212"
(https://github.com/bitcoin/bitcoin/pull/33251#pullrequestreview-3157306080)
Code Review ACK fcac8022d839572f5d8781096eec14ca7ea2e0dd