RPC: Reject RPC requests with same named parameter specified multiple times

[ryanofsky]

Make the JSON-RPC server reject requests with the same named parameter specified multiple times, instead of silently overwriting earlier parameter values with later ones.

Generally JSON keys are supposed to unique, and their order isn't supposed to be significant, so having the server silently discard duplicate keys is error-prone. Most likely if an RPC client is sending a request with duplicate keys it means something is wrong with the request and there should be an error.

After this change, named parameters are still allowed to specified multiple times on the bitcoin-cli command line, since bitcoin-cli automatically replaces earlier values with later values before sending the JSON-RPC request. This makes sense, since it's not unusual for the order of command line options to be significant or for later command line options to override earlier ones.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	MarcoFalke, kristapsk, stickies-v
Concept ACK	luke-jr
Stale ACK	promag

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #26612 ([WIP] refactor: RPC: pass named argument as string_view by stickies-v)
• #26506 (refactor: rpc: use convenience fn to auto parse non-string parameters by stickies-v)
• #26485 (RPC: Add support for name-only parameters by ryanofsky)
• #19949 (cli: Parse and allow hash value by fjahr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[maflcko]

lgtm, left two nits

[stickies-v]

Concept ACK

[stickies-v]

Approach ACK 763d394

[luke-jr]

Multiple keys with the same name is IIRC a supported feature of JSON, but concept ACK on rejecting them rather than ignoring all but one.

I think bitcoin-cli should also reject duplicate parameters, though. It's not obvious to a user that later ones will override earlier ones - they might expect to be able to specify some parameters twice and get the behaviour of both somehow. For example, getblock blockhash=... blockhash=... would logically make more sense as a batch call (though I don't think it's practical to support that, so an error is best IMO).

[promag]

Code review ACK 763d394.

[promag]

@luke-jr suggestion to fail on duplicate named parameters also makes sense to me, either implemented here or in a different branch.

[ryanofsky]

Thanks for the reviews!

Updated 763d394 -> 8c3ff7d (pr/nmult.1 -> pr/nmult.2, compare) with suggestions.

re: #26628 (comment)

I think bitcoin-cli should also reject duplicate parameters, though. It's not obvious to a user that later ones will override earlier ones - they might expect to be able to specify some parameters twice and get the behaviour of both somehow. For example, getblock blockhash=... blockhash=... would logically make more sense as a batch call (though I don't think it's practical to support that, so an error is best IMO).

I think it's pretty normal for command line tools to let later option values override earlier ones. Our binaries bitcoind, bitcoin-qt, bitcoin-wallet and bitcoin-cli all support this for options which don't accept multiple values. In general I think it's good for command line tools to support shortcuts that make scripting easier and remove the need for unnecessary typing and editing, but I can understand the opposite point of view. In any case, i think it would be better to follow up on this in another PR. This PR is trying to avoid the need for a decision by keeping the current behavior unchanged.

[maflcko]

review ACK 8c3ff7d 🗂

Show signature

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK 8c3ff7d52ae3314959e1e66da8718a3f0d30abaa 🗂
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjCxwwAvbXzoDqw5hQHSmtV1qviWaF9zdsfXYeGfXy4TlIyT5jk/pSAmpMj6vqc
gry65aiuJiZ1mP3ywqMXw7+Fs6m/8wJCXl1cBHt43HDLyD1GDMJusVD3A5FsrtPv
1zfZ4zt24cuDnEjImYdf+aWmszO1/PWeXs9n7WJ9AxhV7lE/740lHgjDj8or/yLz
KwuHwmGE1Z1pwV8t9aM0VLgPB6VWcAfXREoxeRbftF84UZ2/tTwER3YNw9Oh2Mbs
hOh1dCDUyzizziKSWlpzXUjPBLeGoe70W0W2CP9iukl/sRpLgEcDooPwqI9DQ1wv
htJqNJrXN/SDtnSulGSNC57500g00J/Jy/Xp+flridDtdZpqQB9LPx0KE+U+0oYz
vRaF9DsrOWtR6QSLqheFwjusosOKm4g7OV8DRrCIdbZVwiwZDeNFW/bfEBrTRvvk
T+efue7d+zc8Gq/oWQ+hRMUfj0GkESfWDlUZok/SqWvKWQnA82bvMi/qmb7BhPB9
swtWkjI9
=ECGn
-----END PGP SIGNATURE-----

[kristapsk]

ACK 8c3ff7d

[stickies-v]

ACK 8c3ff7d

No strong view on the bitcoin-cli behaviour. Intuitively I'd have implemented it to fail on duplicated names, but I think both approaches have merit. Happy to start with the current behaviour and potentially change in the future.

[LarryRuane]

I think it's pretty normal for command line tools to let later option values override earlier ones. Our binaries bitcoind, bitcoin-qt, bitcoin-wallet and bitcoin-cli all support this for options which don't accept multiple values. In general I think it's good for command line tools to support shortcuts that make scripting easier and remove the need for unnecessary typing and editing, but I can understand the opposite point of view.

FWIW, I also favor the behavior of later arguments overriding earlier ones (when multiple don't make sense). I'm not sure if the following is what you're referring to, but I think it's helpful to be able to change a default in a shell script or an alias, but also allow the user to override the new default, something like:

alias printblock='bitcoin-cli -named getblock verbose=true'

Then you can do: printblock -blockhash=00...9 to get verbose output, or printblock -verbose=false -blockhash=00...9 to get a non-verbose result.