Rollback for dumptxoutset without invalidating blocks

fjahr commented at 9:15 pm on September 24, 2025: contributor

This is an alternative approach to implement dumptxoutset with rollback that was discussed a few times. It does not rely on invalidateblock and reconsiderblock and instead creates a temporary copy of the coins DB, modifies this copy by rolling back as many blocks as necessary and then creating the dump from this temp copy DB. See also #29553 (comment), #32817 (comment) and #29565 discussions.

The nice side-effects of this are that forks can not interfere with the rollback and network activity does not have to be suspended. But there are also some downsides when comparing to the current approach: this does require some additional disk space for the copied coins DB and performance is slower (master took 3m 17s vs 9m 16s in my last test with the code here, rolling back ~1500 blocks). However, there is also not much code being added here, network can stay active throughout and performance would stay constant with this approach while it would impact master if there were forks that needed to be invalidated as well (see #33444 for the alternative approach), so this could still be considered a good trade-off.

DrahtBot commented at 9:15 pm on September 24, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33477.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	luke-jr, kevkevinpal, theStack, mzumsande
Stale ACK	enirox001

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33444 (rpc: Fix dumptxoutset rollback with competing forks by enirox001)
#31560 (rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script by theStack)

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.

LLM Linter (✨ experimental)

Possible places where named args may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

temp_cache.AddCoin(key, std::move(coin), false) in src/rpc/blockchain.cpp

2025-11-26

fjahr force-pushed on Sep 24, 2025

fjahr commented at 9:22 pm on September 24, 2025: contributor

cc @Sjors since you were asking for this approach a few times :)

fjahr force-pushed on Sep 24, 2025

luke-jr commented at 10:40 am on September 25, 2025: member

Concept ACK, this seems cleaner.

master took 3m 17s vs 9m 16s in my last test with the code here

I suspect if you go back further, this approach will end up performing better because we no longer need to roll back forward at the end

in src/rpc/blockchain.cpp:2995 in f046286c0c outdated

2991@@ -3020,9 +2992,8 @@ static RPCHelpMan dumptxoutset()
2992     return RPCHelpMan{
2993         "dumptxoutset",
2994         "Write the serialized UTXO set to a file. This can be used in loadtxoutset afterwards if this snapshot height is supported in the chainparams as well.\n\n"
2995-        "Unless the \"latest\" type is requested, the node will roll back to the requested height and network activity will be suspended during this process. "
2996-        "Because of this it is discouraged to interact with the node in any other way during the execution of this call to avoid inconsistent results and race conditions, particularly RPCs that interact with blockstorage.\n\n"
2997-        "This call may take several minutes. Make sure to use no RPC timeout (bitcoin-cli -rpcclienttimeout=0)",
2998+        "This creates a temporary UTXO database when rolling back, keeping the main chain intact. Should the node experience an unclean shutdown the temporary database may need to be removed from the datadir manually.\n\n"

kevkevinpal commented at 12:54 pm on September 27, 2025:

It may be worth noting that “network activity will not be suspended during this process.”

fjahr commented at 9:48 pm on November 26, 2025:

I don’t think this is necessary, I don’t think a user would naturally assume that there is a reason to suspend network activity for this. Previously we had to do this as basically a hack. When we don’t do this anymore it would seem odd to me to mention it.

in src/rpc/blockchain.cpp:2996 in f046286c0c outdated

2991@@ -3020,9 +2992,8 @@ static RPCHelpMan dumptxoutset()
2992     return RPCHelpMan{
2993         "dumptxoutset",
2994         "Write the serialized UTXO set to a file. This can be used in loadtxoutset afterwards if this snapshot height is supported in the chainparams as well.\n\n"
2995-        "Unless the \"latest\" type is requested, the node will roll back to the requested height and network activity will be suspended during this process. "
2996-        "Because of this it is discouraged to interact with the node in any other way during the execution of this call to avoid inconsistent results and race conditions, particularly RPCs that interact with blockstorage.\n\n"
2997-        "This call may take several minutes. Make sure to use no RPC timeout (bitcoin-cli -rpcclienttimeout=0)",
2998+        "This creates a temporary UTXO database when rolling back, keeping the main chain intact. Should the node experience an unclean shutdown the temporary database may need to be removed from the datadir manually.\n\n"
2999+        "This call may take several minutes for deep rollbacks. Make sure to use no RPC timeout (bitcoin-cli -rpcclienttimeout=0)",

kevkevinpal commented at 12:56 pm on September 27, 2025:

Maybe this text would make more sense

“For deep rollbacks, make sure to use no RPC timeout (bitcoin-cli -rpcclienttimeout=0) as it may take several minutes.”

fjahr commented at 9:48 pm on November 26, 2025:

Taken

in src/rpc/blockchain.cpp:3208 in 6d409d5970 outdated

3194+        node.rpc_interruption_point();
3195+
3196+        CBlock block;
3197+        if (!node.chainman->m_blockman.ReadBlock(block, *block_index)) {
3198+            throw JSONRPCError(RPC_INTERNAL_ERROR,
3199+                strprintf("Failed to read block at height %d", block_index->nHeight));

kevkevinpal commented at 1:46 pm on September 27, 2025:

Might be able to add a functional test for this rpc error

fjahr commented at 9:48 pm on November 26, 2025:

Hm, this one and the other similar comments about test coverage are all cases that are pretty hard to hit because they should only be possible in a case of db corruption or a similarly unlikely event. Not saying that this wouldn’t be valuable coverage, but afaik we hardly ever go through the hassle to cover such cases and this RPC is far from being a critical path in the comparison to the rest of the code base. So, unless you have a specific suggestion for how the can be hit in practical way I would suggest we keep these for a follow-up. The current changes should be a robustness improvement by themselves.

(Marking the other comments as resolved for now but feel free to correct me if you think different about one of them specifically)

in src/rpc/blockchain.cpp:3214 in 6d409d5970 outdated

3200+        }
3201+
3202+        WITH_LOCK(::cs_main, res = chainstate.DisconnectBlock(block, block_index, rollback_cache));
3203+        if (res == DISCONNECT_FAILED) {
3204+            throw JSONRPCError(RPC_INTERNAL_ERROR,
3205+                strprintf("Failed to roll back block at height %d", block_index->nHeight));

kevkevinpal commented at 1:47 pm on September 27, 2025:

Might be able to add a functional test for this rpc error

in src/rpc/blockchain.cpp:3236 in 6d409d5970 outdated

3222+                                                          chainstate.m_blockman,
3223+                                                          CoinStatsHashType::HASH_SERIALIZED,
3224+                                                          node.rpc_interruption_point);
3225+
3226+    if (!maybe_stats) {
3227+        throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to compute UTXO statistics");

kevkevinpal commented at 1:47 pm on September 27, 2025:

Might be able to add a functional test for this rpc error

in src/rpc/blockchain.cpp:3241 in 6d409d5970 outdated

3227+        throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to compute UTXO statistics");
3228+    }
3229+
3230+    std::unique_ptr<CCoinsViewCursor> pcursor{temp_db->Cursor()};
3231+    if (!pcursor) {
3232+        throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to create UTXO cursor");

kevkevinpal commented at 1:47 pm on September 27, 2025:

Might be able to add a functional test for this rpc error

in src/rpc/blockchain.cpp:3122 in 6d409d5970 outdated

3108+    TemporaryUTXODatabase(const fs::path& path) : m_path(path) {
3109+        fs::create_directories(m_path);
3110+    }
3111+    ~TemporaryUTXODatabase() {
3112+        try {
3113+            fs::remove_all(m_path);

kevkevinpal commented at 2:01 pm on September 27, 2025:

It might be useful to add a log here to inform the user the temp UTXO db was cleaned up since we mentioned in logs that we are creating a temp UTXO db

fjahr commented at 9:47 pm on November 26, 2025:

Hm, I don’t think we log anything on the creation of the DB so I think I would keep it the same on the destruction. It should only be a debug level log if we would add anything like that since it seems a bit low level for the general user.

kevkevinpal commented at 2:04 pm on September 27, 2025: contributor

Concept ACK 6d409d5

This approach makes more sense. I reviewed the code a bit and made some comments, but nothing blocking

I also added comments on possible functional tests for the new JSONRPCError but these can be done in a followup

ajtowns commented at 3:27 am on September 29, 2025: contributor

Nice!

performance is slower (master took 3m 17s vs 9m 16s in my last test with the code here,

That probably makes sense. It might be possible to do it faster and with less disk usage for relatively short rollbacks via a two step process:

create a read-only snapshot of the db
create an empty “coins-delta” db
iterate through the rev data to rollback, update the coins-delta db:
- when you rollback past a coin’s creation:
  - if the coin was in the snapshot db, add “[coin] deleted”
  - otherwise, if it was in the coins-delta db, remove “[coin]”
  - (otherwise, there’s a bug)
- when you rollback past a coin’s spend, add “[coin]”
when you’ve finished the rollback,
- iterate through the snapshot coins, skipping any where there is a “[coin] deleted” entry, reporting them
- iterate through the non-deleted coins-delta coins, reporting them
delete the coins-delta db, delete the snapshot

Rather than being direct RPC functionality, maybe it would be better to have an RPC function to export a copy of the utxo set at the current height, and have a separate bitcoin-kernel binary that performs the rollback and utxoset stats calculation itself?

theStack commented at 4:57 pm on October 9, 2025: contributor

Concept ACK

enirox001 commented at 6:38 pm on October 27, 2025: contributor

ACK 6d409d5

This is a good change. Using a temporary coins DB for the rollback is a much cleaner and safer solution than the invalidateblock method. It correctly solves fork-related bugs by isolating the process and avoids the need for network suspension, making it a superior approach to what I proposed in #33444.

The code is well-contained, and the new TemporaryUTXODatabase class handles the DB lifecycle cleanly.

I’ve pulled the branch, compiled, and the full functional test suite passes, including the rpc_dumptxoutset test

DrahtBot requested review from kevkevinpal on Oct 27, 2025

DrahtBot requested review from theStack on Oct 27, 2025

DrahtBot requested review from luke-jr on Oct 27, 2025

in src/rpc/blockchain.cpp:3168 in f046286c0c outdated

3154+
3155+        std::unique_ptr<CCoinsViewCursor> cursor;
3156+        WITH_LOCK(::cs_main, cursor = chainstate.CoinsDB().Cursor());
3157+
3158+        size_t coins_count = 0;
3159+        while (cursor->Valid()) {

mzumsande commented at 8:14 pm on October 27, 2025:

Don’t we need to hold cs_main throughout the copying phase? What if the utxo set changes while we are copying coins?

fjahr commented at 9:47 pm on November 26, 2025:

I don’t think so, my understanding is that the cursor/LevelDB iterator works on a snapshot of the DB itself which doesn’t get mutated. You can see the same pattern in WriteUTXOSnapshot where we are working with a cursor without holding cs_main as well.

in src/rpc/blockchain.cpp:3217 in f046286c0c outdated

3212+        }
3213+
3214+        block_index = block_index->pprev;
3215+    }
3216+
3217+    rollback_cache.SetBestBlock(target->GetBlockHash());

mzumsande commented at 8:31 pm on October 27, 2025:

This seems unnecessary. As far as I can see the final DisconnectBlock call should have set the best block already, so that could we instead Assume here that rollback_cache.GetBestBlock() == target->GetBlockHash().

fjahr commented at 9:47 pm on November 26, 2025:

Changed to an Assume instead.

mzumsande commented at 8:53 pm on October 27, 2025: contributor

Concept ACK

I suspect if you go back further, this approach will end up performing better because we no longer need to roll back forward at the end

Would be interesting if someone could try out by going back 25k blocks or more, as is usually done for creating snapshots (I can’t right now).

fjahr force-pushed on Nov 26, 2025

fjahr commented at 9:47 pm on November 26, 2025: contributor

Thanks for all the feedback so far and sorry for the slow response!

Rather than being direct RPC functionality, maybe it would be better to have an RPC function to export a copy of the utxo set at the current height, and have a separate bitcoin-kernel binary that performs the rollback and utxoset stats calculation itself?

Hm, feels like a bit overengineered for this functionality, considering the overhead for test coverage and build changes for this. Maybe I am overcomplicating it in my head, I have not done much with kernel yet. But if this is considerably more complex I would rather first go ahead with this and then I would rather keep it for consideration of a future change.

Would be interesting if someone could try out by going back 25k blocks or more, as is usually done for creating snapshots (I can’t right now).

I tried with 880,000, our v29 param, so 45,000 blocks rollback. It took just under 20min in total and it returned the correct hash.

0$ build/bin/bitcoin-cli -rpcclienttimeout=0 -named dumptxoutset ~/Downloads/utxo880k.dat rollback=880000
1{
2  "coins_written": 184821030,
3  "base_hash": "000000000000000000010b17283c3c400507969a9c2afd1dcf2082ec5cca2880",
4  "base_height": 880000,
5  "path": "/Users/FJ/Downloads/utxo880k.dat",
6  "txoutset_hash": "dbd190983eaf433ef7c15f78a278ae42c00ef52e0fd2a54953782175fbadcea9",
7  "nchaintx": 1145604538
8}

It might be possible to do it faster and with less disk usage for relatively short rollbacks via a two step process:

I actually had a similar idea early on but then stayed with the simpler approach. I will try this out with a POC and check the performance impact.

fjahr force-pushed on Nov 26, 2025

DrahtBot added the label CI failed on Nov 26, 2025

DrahtBot commented at 9:52 pm on November 26, 2025: contributor

🚧 At least one of the CI tasks failed. Task Windows-cross to x86_64: https://github.com/bitcoin/bitcoin/actions/runs/19718249847/job/56495362448 LLM reason (✨ experimental): Compile errors in rpc/blockchain.cpp due to RPCHelpMan constructor signature mismatch (brace-initializer/API change).

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.

rpc: Don't invalidate blocks in dumptxoutset

Instead this new approach uses a temporary coins db to roll back the
UTXO set.

7d67a66a9a

test: Add dumptxoutset fork test e0162ab0ab

fjahr force-pushed on Nov 26, 2025

DrahtBot removed the label CI failed on Nov 26, 2025

fjahr commented at 0:21 am on November 27, 2025: contributor

It might be possible to do it faster and with less disk usage for relatively short rollbacks via a two step process:

I actually had a similar idea early on but then stayed with the simpler approach. I will try this out with a POC and check the performance impact.

I tried to a few different takes on the delta-based idea including some vibe coding tests. Here is the latest one, something is definitely still broken there but the dump it generates is correct. All tests I did had in common that the processing time actually took longer than the current approach, almost 28 min with the latest code version. I am now thinking that longer rollbacks may not be quicker, only shorter ones will be because the big copy overhead in the beginning is saved. But then we also don’t have that much to gain. Even if the performance can be improved, now that I have played around with it, it doesn’t seem worth the additional code complexity it introduces. It would need to be significantly faster to justify that and I am currently not seeing that.

Rollback for dumptxoutset without invalidating blocks #33477

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)