assumeutxo, rpc: Add 'start' parameter to loadtxoutset

[hernanmarino]

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.

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #29345 (rpc: Do not wait for headers inside loadtxoutset by maflcko)

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.

[hernanmarino]

Rebased. Also followed @maflcko nit advise.

[pablomartin4btc]

Concept ACK

[ryanofsky]

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.

[hernanmarino]

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.

[ryanofsky]

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.

[hernanmarino]

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.

[Sjors]

I think @ryanofsky's suggestion makes sense. We could even keep loadtxoutset the same (it implies start), and then have loadtxoutsetstatus and loadtxoutsetabort.

[maflcko]

Are you still working on this?

[hernanmarino]

Are you still working on this?

Yes ! I was witing for more reviews, but I'll go ahead with @Sjors suggestion asap.

[maflcko]

Ok, maybe turn it into draft, until you are done and the CI is passing?

[hernanmarino]

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.

[pablomartin4btc]

I think it's up to you really, you could either use this PR, updating title & description (mentioning previous approach) or create a new one referring this one, but you would need to make loadtxoutset async now (no? 🤔) so the other 2 new RPC commands (to be created) work/ interact well.

[maflcko]

I think it is fine to not have an abort command for now, because currently it is also not possible to abort, unless the complete process is shut down. But up to you.

[maflcko]

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

[hernanmarino]

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.