💬 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

💬 kristapsk commented on issue "RPC wont bind without an IP address on a non-localhost interface":
(https://github.com/bitcoin/bitcoin/issues/13155#issuecomment-3225810421)
Noticed this when there is only IPv6 address assigned to eth0 interface (lo has both 127.0.0.1 and ::1). When IPv4 address is also added to eth0, problem disappears.

```
Bitcoin Core version v29.0.0 (release build)
Using the 'arm_shani(1way,2way)' SHA256 implementation
Startup time: 2025-08-26T21:30:01Z
Default data directory /.bitcoin
Using data directory /ssd/bitcoind/mainnet
Config file: /home/bitcoind/mainnet.conf
Config file arg: chain="main"
Config file arg: datadir="/ssd/bitcoind/mainnet
...

💬 achow101 commented on issue "integer sanitizer warning, when running with -natpmp=1 enabled":
(https://github.com/bitcoin/bitcoin/issues/33245#issuecomment-3225829239)
It looks like this is being triggered by a `NLMSG_DONE` message and is something that we could avoid by checking `hdr->nlmsg_type` before doing `NLMSG_DATA` and `RTM_PAYLOAD`. This could be included in #32159 since that's already touching this code.

💬 achow101 commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2302222710)
These could be moved after `if (hdr->nlmsg_type == NLMSG_DONE) {` to avoid the integer sanitizer warning in #33245

📝 polespinasa opened a pull request: "rpc, logging: add backgroundvalidation to getblockchaininfo"
(https://github.com/bitcoin/bitcoin/pull/33259)
`getblockchaininfo` returns `verificationprogress=1` and `initialblockdownload=false` even if there's background validation.
This PR adds information about background validation to rpc `getblockchaininfo` in a similar way to `validationprogress` does.

If assume utxo was used the output of a "sync" node performing background validation:
```
$ ./build/bin/bitcoin-cli getblockchaininfo
...
"mediantime": 1756245019,
"verificationprogress": 1,
"initialblockdownload": false,
"backgr
...

💬 achow101 commented on issue "tracing: test `interface_usdt_net.py` fails due to garbage message type in `net:outbound_message` tracepoint":
(https://github.com/bitcoin/bitcoin/issues/33246#issuecomment-3226064329)
I'm able to reproduce this with a release build, but not a debug build.

💬 l0rinc commented on pull request "bench: Add more realistic Coin Selection Bench":
(https://github.com/bitcoin/bitcoin/pull/33160#discussion_r2302109933)
we don't actually need the wallet here, right?

💬 l0rinc commented on pull request "bench: Add more realistic Coin Selection Bench":
(https://github.com/bitcoin/bitcoin/pull/33160#discussion_r2302150941)
we want deterministic randomness for benchmarks, otherwise it's hard to know what we're actually measuring:
```suggestion
FastRandomContext rand{/*fDeterministic=*/true};
```