Might fix #28620
This PR only modifies the behavior of loadtxoutset to require a mandatory “start” string as a first parameter.
This change will allow to implement “status”, “abort” or similar future parameters in follow-ups, without breaking the current RPC protocol with incompatible changes (if this were to be merged in the current release cycle)
Also, it is implemented in a minimalist way, to make it easy to review.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | pablomartin4btc |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
2733@@ -2732,12 +2734,15 @@ static RPCHelpMan loadtxoutset()
2734 }
2735 },
2736 RPCExamples{
2737- HelpExampleCli("loadtxoutset", "utxo.dat")
2738+ HelpExampleCli("loadtxoutset", "start utxo.dat")
2739 },
2740 [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
2741 {
2742+ if (request.params[0].get_str() != "start") {
self.Arg<std::string>(0) for new code?
Is there another RPC method that supports subcommands (“start” “status” “abort”) like this? It seems our RPC dispatching and documentation framework might have to be changed a lot to support a RPC methods with multiple subcommands if the subcommands don’t all take the same parameters. Also the JSON-RPC protocol doesn’t provide a way to represent subcommands, so it might be awkward for example in python to write the command as a string argument instead of a method name.
Maybe a simpler alternative would be to add loadtxoutsetstatus and loadtxoutsetabort RPC methods.
Is there another RPC method that supports subcommands (“start” “status” “abort”) like this?
Yes, the issue mentions scanblocks which folllows the exact same protocol.
I also found scantxoutset , same protocol.
Thanks for the examples. This confirms my doubts about the approach, since documentation produced by “bitcoin-cli help scanblocks” is pretty messy, and it seems like this approach can only work if none of the other subcommands take any positional RPC arguments, since they would conflict with arguments for the “start” subcommand.
So having separate RPC methods still seems a little cleaner to me, but maybe it’s worth accepting some limitations if it makes the 3 RPCs more consistent with each other. Either way seems reasonable.
Thanks for the examples. This confirms my doubts about the approach, since documentation produced by “bitcoin-cli help scanblocks” is pretty messy, and it seems like this approach can only work if none of the other subcommands take any positional RPC arguments, since they would conflict with arguments for the “start” subcommand.
So having separate RPC methods still seems a little cleaner to me, but maybe it’s worth accepting some limitations if it makes the 3 RPCs more consistent with each other. Either way seems reasonable.
Yes , although I like a little bit more this approach I understand your point of view. I also agree that something can be done in the future to improve this situation. It might be a refactoring of RPCHelpMan logic, or some local handling in each RPC function implementation.
If there is consensus to go ahead with this approach, I volunteer myself to also implement the abort and status subcommands in follow-ups, and perhaps try a solution for what you described as well.
loadtxoutset the same (it implies start), and then have loadtxoutsetstatus and loadtxoutsetabort.
Are you still working on this?
Yes ! I was witing for more reviews, but I’ll go ahead with @Sjors suggestion asap.
This commit modifies the behavior of loadtxoutset to require a
mandatory "start" parameter before specifying the filename.
This change allows to implement "status", "abort" or similar future
parameters in follow-ups, without breaking the current RPC
protocol with incompatible changes.
Ok, maybe turn it into draft, until you are done and the CI is passing?
Actually, I ’d like to ask you and @pablomartin4btc (who also reviewed) a question . I am convinced now to follow @ryanofsky ( and @Sjors ) suggestion of implementing this with 3 commands instead of the current approach of subcommands.
If you agree with this new approach , I think I’ll close this PR and implement the functionality in one (or two) followup PRs implementing the new commands, since there is no point in keeping this PR if loadtxoutset remains unchanged.
loadtxoutset async now (no? :thinking:) so the other 2 new RPC commands (to be created) work/ interact well.
Closing for now. This has been a placeholder for months now, with no code to review or merge, and the CI failing anyway. Feel free to open a new pull request once and if there is something to review. If you have questions, you can leave them in the existing thread: #28620
I agree with closing, consensus was leaning towards to a different approach, in wich I am already working. I ’ll open a new PR when ready, thanks.