rpc: migrate generatetoaddress params to self.Arg for consistency #34950

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.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

LLM Linter (✨ experimental)

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

^{2026-03-30 17:14:25}

Pretty sure this was intentional and is harmless, not a bug. Though, it could make sense to make the change for consistency, and because likely no one is affected by the behavior change.

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

Also, seems simple enough to squash. Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

Is this LLM generated? Did you even try to compile it locally?

<!--85328a0da195eb286784d51f73fa0af9-->

🚧 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).}

<details><summary>Hints</summary>

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.

</details>

I built against 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{...}.

Looks good to me. Aligning with generatetodescriptor is the obvious cleanup and the std::string_view → std::string{...} step for DecodeDestination reads fine after the linker fix discussion above.

One small thing worth flagging: the maxtries=-1 behavior change is user-visible. Before, a negative int would silently wrap to UINT64_MAX through the implicit int → uint64_t conversion, now it gets rejected as out of range. Almost certainly nobody depends on the old wrap (it was clearly accidental) but it might still be worth a release-notes line under RPC so integrators don't get surprised.

Didn't build locally on this one, just read the diff against generatetodescriptor.

The bottleneck in this project has always been review and testing, not writing code. Development here is intentionally conservative and slow, and reviewer attention is the scarcest resource we have. LLMs have made this worse, anyone can now prompt them and post their output as PRs. There is an infinite amount plausible looking "improvements" for LLMs to suggest and work on.

Unless we fully trust LLMs to both write and review code, humans still have to spend time understanding the proposed changes, which incurs a non-zero cost for every opened PR.

I understand that contributing to this project can be intimidating, and using LLMs may seem tempting, but it really creates more issues for this project than it solves. The best way to help this project, is to review and test changes. You can use LLMs for this, but you shouldn't solely rely on them, or just post their output.

I'm not asking you to close this PR. I am asking you to reconsider whether it's something you genuinely think the project should pursue, independent of what your LLM suggested.