Rollback for dumptxoutset without invalidating blocks #33477

fjahr · 2025-09-24T21:15:11Z

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 · 2025-09-24T21:15:20Z

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  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 · 2025-09-24T21:22:50Z

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

luke-jr · 2025-09-25T10:40:34Z

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

kevkevinpal

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

kevkevinpal · 2025-09-27T12:54:25Z

src/rpc/blockchain.cpp

-        "Unless the \"latest\" type is requested, the node will roll back to the requested height and network activity will be suspended during this process. "
-        "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"
-        "This call may take several minutes. Make sure to use no RPC timeout (bitcoin-cli -rpcclienttimeout=0)",
+        "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"


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

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.

src/rpc/blockchain.cpp

kevkevinpal · 2025-09-27T13:46:58Z

src/rpc/blockchain.cpp

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


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

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)

src/rpc/blockchain.cpp

kevkevinpal · 2025-09-27T14:01:30Z

src/rpc/blockchain.cpp

+    }
+    ~TemporaryUTXODatabase() {
+        try {
+            fs::remove_all(m_path);


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

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.

ajtowns · 2025-09-29T03:27:34Z

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 · 2025-10-09T16:57:40Z

Concept ACK

enirox001 · 2025-10-27T18:38:05Z

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

mzumsande

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

mzumsande · 2025-10-27T20:14:44Z

src/rpc/blockchain.cpp

+        WITH_LOCK(::cs_main, cursor = chainstate.CoinsDB().Cursor());
+
+        size_t coins_count = 0;
+        while (cursor->Valid()) {


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

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.

src/rpc/blockchain.cpp

fjahr · 2025-11-26T21:47:11Z

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.

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

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.

DrahtBot · 2025-11-26T21:52:30Z

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

Hints

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.

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

fjahr · 2025-11-27T00:21:26Z

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.

fjahr mentioned this pull request Sep 24, 2025

rpc: Fix dumptxoutset rollback with competing forks #33444

Open

fjahr force-pushed the 202509-better-rollback branch from 791471a to d51c7d7 Compare September 24, 2025 21:22

fjahr force-pushed the 202509-better-rollback branch 2 times, most recently from 716e1db to 6d409d5 Compare September 24, 2025 22:33

DrahtBot mentioned this pull request Sep 25, 2025

rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script #31560

Open

kevkevinpal reviewed Sep 27, 2025

View reviewed changes

DrahtBot requested review from kevkevinpal, luke-jr and theStack October 27, 2025 18:38

mzumsande reviewed Oct 27, 2025

View reviewed changes

fjahr force-pushed the 202509-better-rollback branch from 6d409d5 to b71ee30 Compare November 26, 2025 21:45

fjahr force-pushed the 202509-better-rollback branch from b71ee30 to 53865e7 Compare November 26, 2025 21:51

DrahtBot added the CI failed label Nov 26, 2025

fjahr added 2 commits November 26, 2025 23:05

rpc: Don't invalidate blocks in dumptxoutset

7d67a66

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

test: Add dumptxoutset fork test

e0162ab

fjahr force-pushed the 202509-better-rollback branch from 53865e7 to e0162ab Compare November 26, 2025 22:05

DrahtBot removed the CI failed label Nov 26, 2025

Rollback for dumptxoutset without invalidating blocks #33477

Are you sure you want to change the base?

Rollback for dumptxoutset without invalidating blocks #33477

Conversation

fjahr commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

fjahr commented Sep 24, 2025

Uh oh!

luke-jr commented Sep 25, 2025

Uh oh!

kevkevinpal left a comment

Choose a reason for hiding this comment

Uh oh!

kevkevinpal Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

fjahr Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevkevinpal Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

fjahr Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevkevinpal Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

fjahr Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

ajtowns commented Sep 29, 2025

Uh oh!

theStack commented Oct 9, 2025

Uh oh!

enirox001 commented Oct 27, 2025

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjahr Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fjahr commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 26, 2025

Uh oh!

fjahr commented Nov 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

fjahr commented Sep 24, 2025 •

edited

Loading

DrahtBot commented Sep 24, 2025 •

edited

Loading

mzumsande Oct 27, 2025 •

edited

Loading

fjahr commented Nov 26, 2025 •

edited

Loading