assumeutxo: Add dumptxoutset height param, remove shell scripts

[fjahr]

This adds a height parameter to the dumptxoutset RPC. This internalizes the workflow that was previously done by scripts: roll back the chain to the height we actually want the snapshot from, create the snapshot, roll forward to the real tip again.

The nice thing about internalizing this functionality is that we can write tests for the code and it gives us more options to make the functionality robust. The shell scripts we have so far will be more cumbersome to maintain in the long run, especially since we will only notice later when we have broken them. I think it's safe to remove these test_utxo_snapshots.sh as well when we have this option in dumptxoutset because we have also added some good additional functional test coverage for this functionality.

[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
ACK	alfonsoromanz
Concept ACK	theStack, Sjors, TheCharlatan, BrandonOdiwuor

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:

• #29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
• #29681 (test: loading assumeutxo snapshot start states by BrandonOdiwuor)

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.

[theStack]

Concept ACK

Nice idea. I think it makes sense to do the rollback (and roll forward after dump creation) directly in the RPC call, rather than relying on potentially fragile and poorly maintained shell scripts. One suggestion: on pruned nodes, the RPC should fail early if the block of the snapshot height is below the pruned block height, similar like what the script does:

bitcoin/contrib/devtools/utxo_snapshot.sh

Lines 34 to 37 in 98005b6

	if (( GENERATE_AT_HEIGHT < PRUNED )); then
	echo "Error: The requested snapshot height (${GENERATE_AT_HEIGHT}) should be greater than the pruned block height (${PRUNED})."
	exit 1
	fi

I'm neutral about removing test_utxo_snapshots.sh. This seems to be more meant like an interactive demo to get familiar with AssumeUTXO and as such could still make sense for some?

[luke-jr]

Seems like this should freeze indexes, wallets, etc and hold a lock so nothing else happens during the process...?

[Sjors]

Concept ACK. But rather than relying on block invalidation, I would prefer to have a clean rollback. That would be useful as it's own RPC too: turn off network, call rollback N/HASH, turn on network and it will sync normally. See #29565.

[TheCharlatan]

Concept ACK

[BrandonOdiwuor]

Concept ACK

great idea moving InvalidateBlock and ReconsiderBlock to the dumputxoset RPC instead of relying on shell scripts

[fjahr]

Thanks for all the comments! Rebased and addressed feedback.

One suggestion: on pruned nodes, the RPC should fail early if the block of the snapshot height is below the pruned block height

Added, good idea!

I'm neutral about removing test_utxo_snapshots.sh. This seems to be more meant like an interactive demo to get familiar with AssumeUTXO and as such could still make sense for some?

Yeah, I was kind of aiming at that when I wrote "TBD if we need additional documentation.". Since your comment I have been thinking about it a bit more but because that script requires the person that runs it to change the code and recompile bitcoin core I think its audience was always just a tiny group, particularly I think it was useful for reviewers of the concept and code in the early days. Going forward it should be more valuable if we instead document the code properly to make it easy to grasp for contributors (the same goes for the functional tests where we essentially go through similar steps as the script) and for users we should provide easy documentation that explains the usage of the feature properly but doesn't require changing the code.

I have added another commit where I am splitting off a part of the previous design doc into a separate, usage-focused doc. Let me know what you think and if you have ideas for making the usage doc more complete.

Seems like this should freeze indexes, wallets, etc, and hold a lock so nothing else happens during the process...?

But rather than relying on block invalidation, I would prefer to have a clean rollback.

I think these requests are essentially the same, or at least the ideal solution to both of these will look similar.

For now, I have only added that I am setting the network to inactive and I will now work on a draft for the clean rollback. But I am still pushing this here now because I think this is already an improvement as-is and the clean rollback may be a bigger change that could also be done as a follow-up, so I don't think review needs to be blocked here. I will report back on the rollback progress here soon.

[fjahr]

This actually isn't a fully sufficient check because we could be missing the undo data. I am addressing this here: #29668

[Sjors]

Seems like this should freeze indexes, wallets, etc, and hold a lock so nothing else happens during the process...?

But rather than relying on block invalidation, I would prefer to have a clean rollback.

I think these requests are essentially the same, or at least the ideal solution to both of these will look similar.

Another approach, suggested by @maflcko in #29565 is to rewind the UTXO set inside of a cache. If that cache is kept isolated from the rest of the node, then indexes and wallets are not affected. It's also safe to abort the process, worst case just leaving a large temp file around.

[fjahr]

Another approach, suggested by @maflcko in #29565 is to rewind the UTXO set inside of a cache. If that cache is kept isolated from the rest of the node, then indexes and wallets are not affected. It's also safe to abort the process, worst case just leaving a large temp file around.

Yes, I have a similar approach in mind. I think that is the cleanest solution if it's feasible in terms of complexity.

[alfonsoromanz]

Code Review and Tested ACK c83d7a5. My impression is that this PR is good enough as is, unless there is a specific issue with invalidate/reconsider block that I am not aware of.

This will not only prevent errors (by internalizing and testing code previously ran in scripts) but will also offer an easy way for users to create a snapshot from a specific height via RPC. If the rollback functionality gets implemented after this PR is merged, it seems straightforward to change this code to: (1) use the rollback instead of the invalidateBlock, and (2) remove the ReconsiderBlock call as the node will sync to the tip again. So I don't think the rollback feature should be a blocker for this PR.

Note: for testing I ran test/functional/feature_assumeutxo.py to make sure the test passes, and also modified line 391 to load the snapshot created by this new test in node2, to make sure the snapshot is loaded successfully too. i.e (line 391)
loaded = n2.loadtxoutset(dump_output2['path'])