generatetoaddress extracts parameters using the older request.params[] API, while its sibling generatetodescriptor already uses self.Arg<>() (since c00000df16). This migrates all three parameter extractions in generatetoaddress to the self.Arg<> API for consistency.
As a side-effect, negative maxtries values (e.g. -1) are now correctly rejected with “JSON integer out of range” instead of silently wrapping to UINT64_MAX via implicit conversion.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process. A summary of reviews will appear here.
Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):
assert_raises_rpc_error(-1, "JSON integer out of range", self.generatetoaddress, self.nodes[0], 1, 'mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ', -1) in test/functional/rpc_generate.py2026-03-30 17:14:25
Thanks for taking a look. I’m not sure it was intentional. generatetodescriptor rejects negative values via self.Arg<uint64_t>(), so if the wrapping behavior in generatetoaddress were by design I’d expect the newer sibling RPC to preserve it rather than diverge.
Would you prefer I reframe the PR description as a consistency cleanup rather than a bugfix if that better reflects how the project wants to categorise it?
I’d expect the newer sibling RPC to preserve it rather than diverge.
They diverged since I changed only one of them in c00000df1605788acadceb90c22ae9f00db8a9dc, so I think it makes sense to be consistent. I just didn’t get around to it yet.
Would you prefer I reframe the PR description as a consistency cleanup rather than a bugfix if that better reflects how the project wants to categorise it?
Yeah, I think it makes sense to call this behavior change a cleanup, not a bugfix
🚧 At least one of the CI tasks failed.
Task 32 bit ARM: https://github.com/bitcoin/bitcoin/actions/runs/23739108782/job/69198089153
LLM reason (✨ experimental): CI failed during the link step for bitcoind due to an undefined reference to an RPCHelpMan::ArgValue<std::string> symbol (C++ ABI/library linkage mismatch).
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
generatetoaddress extracts parameters using the older
request.params[] API, while its sibling generatetodescriptor
already uses self.Arg<>() (since c00000df16). Migrate all three
parameter extractions to self.Arg<>, matching generatetodescriptor.
As a side-effect, negative maxtries values (e.g. -1) are now
rejected with "JSON integer out of range" instead of silently
wrapping to UINT64_MAX via implicit conversion.
bitcoind locally, but didn’t have Qt configured so the linker error slipped through. self.Arg<std::string> takes the non triv copyable path and resolves to ArgValue<const std::string&>, which isn’t instantiated in the bitcoin-qt target. I have switched to self.Arg<std::string_view> to match generatetodescriptor. DecodeDestination wants const std::string& so it’s wrapped in std::string{...}.