assumeutxo: Add dumptxoutset height param, remove shell scripts #29553

pull fjahr wants to merge 8 commits into bitcoin:master from fjahr:2024-03-dumptxoutset-height changing 11 files +349 −412
  1. fjahr commented at 3:58 pm on March 4, 2024: contributor

    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.

  2. DrahtBot commented at 3:58 pm on March 4, 2024: contributor

    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.

    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:

    • #30713 (rpc: add revelant_blocks to scanblocks status by tdb3)
    • #30666 (validation: improve m_best_header tracking by mzumsande)
    • #30635 (rpc: add optional blockhash to waitfornewblock by Sjors)
    • #30409 (Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block by Sjors)

    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.

  3. in src/rpc/blockchain.cpp:2636 in c2f62ebc44 outdated
    2631+        invalidate_index = WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Next(target_index));
    2632+        InvalidateBlock(*node.chainman, invalidate_index->GetBlockHash());
    2633+        const CBlockIndex* new_tip_index{WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Tip())};
    2634+        // In case there is any issue with a block being read from disc due to
    2635+        // pruning for example, we need to stop here, otherwise the dump would
    2636+        // still be greated for the wrong height.
    


    theStack commented at 4:40 pm on March 4, 2024:
    0        // still be created for the wrong height.
    

    fjahr commented at 8:37 pm on March 16, 2024:
    fixed
  4. in test/functional/feature_assumeutxo.py:346 in a480d9748b outdated
    212@@ -210,6 +213,13 @@ def run_test(self):
    213 
    214         assert_equal(n0.getblockchaininfo()["blocks"], FINAL_HEIGHT)
    215 
    216+        self.log.info(f"Check that dumptxoutsetinfo works for past block heights")
    217+        dump_output2 = n0.dumptxoutset('utxos2.dat', SNAPSHOT_BASE_HEIGHT)
    218+        check_dump_output(dump_output2)
    


    theStack commented at 4:49 pm on March 4, 2024:

    nit: could additionally check that the two dump file contents are equal, e.g.

     0diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
     1index 71c3c2c096..a9ba96ae25 100755
     2--- a/test/functional/feature_assumeutxo.py
     3+++ b/test/functional/feature_assumeutxo.py
     4@@ -39,6 +39,7 @@ from test_framework.test_framework import BitcoinTestFramework
     5 from test_framework.util import (
     6     assert_equal,
     7     assert_raises_rpc_error,
     8+    sha256sum_file,
     9 )
    10 from test_framework.wallet import (
    11     getnewdestination,
    12@@ -216,6 +217,7 @@ class AssumeutxoTest(BitcoinTestFramework):
    13         self.log.info(f"Check that dumptxoutsetinfo works for past block heights")
    14         dump_output2 = n0.dumptxoutset('utxos2.dat', SNAPSHOT_BASE_HEIGHT)
    15         check_dump_output(dump_output2)
    16+        assert_equal(sha256sum_file(dump_output['path']), sha256sum_file(dump_output2['path']))
    17 
    18         # Ensure n0 is back at the tip
    19         assert_equal(n0.getblockchaininfo()["blocks"], FINAL_HEIGHT)
    

    fjahr commented at 8:38 pm on March 16, 2024:
    added
  5. theStack commented at 5:25 pm on March 4, 2024: contributor

    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: https://github.com/bitcoin/bitcoin/blob/98005b6a17907c4f7bdcf802ee96c274492f902a/contrib/devtools/utxo_snapshot.sh#L34-L37

    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?

  6. luke-jr commented at 7:14 pm on March 4, 2024: member
    Seems like this should freeze indexes, wallets, etc and hold a lock so nothing else happens during the process…?
  7. Sjors commented at 10:51 am on March 5, 2024: member
    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.
  8. TheCharlatan commented at 12:40 pm on March 6, 2024: contributor
    Concept ACK
  9. BrandonOdiwuor commented at 2:20 pm on March 6, 2024: contributor

    Concept ACK

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

  10. DrahtBot added the label Needs rebase on Mar 13, 2024
  11. fjahr force-pushed on Mar 16, 2024
  12. DrahtBot removed the label Needs rebase on Mar 16, 2024
  13. fjahr commented at 8:57 pm on March 16, 2024: contributor

    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.

  14. in src/rpc/blockchain.cpp:2671 in 5da057c533 outdated
    2658+        // If the node is running in pruned mode we ensure all necessary block
    2659+        // data is available before starting to roll back.
    2660+        if (node.chainman->m_blockman.IsPruneMode()) {
    2661+            LOCK(::cs_main);
    2662+            const CBlockIndex* current_tip{node.chainman->ActiveChain().Tip()};
    2663+            const CBlockIndex* first_block{node.chainman->m_blockman.GetFirstStoredBlock(*current_tip)};
    


    fjahr commented at 4:46 pm on March 17, 2024:
    This actually isn’t a fully sufficient check because we could be missing the undo data. I am addressing this here: #29668

    ryanofsky commented at 1:38 pm on June 21, 2024:

    In commit “RPC: Add height parameter to dumptxoutset” (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)

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

    Can you say what the problem is in a code comment so it is clear what current behavior is? Comment can removed when the problem is fixed later.


    fjahr commented at 4:52 pm on July 11, 2024:
    This is fixed now since #29668 got merged and I am using this here now.
  15. Sjors commented at 12:56 pm on March 18, 2024: member

    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.

  16. fjahr commented at 1:00 pm on March 18, 2024: contributor

    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.

  17. DrahtBot added the label Needs rebase on Mar 20, 2024
  18. fjahr force-pushed on Mar 20, 2024
  19. fjahr force-pushed on Mar 20, 2024
  20. DrahtBot removed the label Needs rebase on Mar 20, 2024
  21. DrahtBot added the label Needs rebase on Mar 25, 2024
  22. fjahr force-pushed on Mar 25, 2024
  23. DrahtBot removed the label Needs rebase on Mar 25, 2024
  24. laanwj added the label RPC/REST/ZMQ on Apr 24, 2024
  25. alfonsoromanz approved
  26. alfonsoromanz commented at 3:25 pm on May 1, 2024: contributor

    Code Review and Tested ACK c83d7a5dec3595abe4cc6c8dcb29a016c6905a98. 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'])

  27. DrahtBot requested review from theStack on May 1, 2024
  28. DrahtBot requested review from BrandonOdiwuor on May 1, 2024
  29. DrahtBot requested review from Sjors on May 1, 2024
  30. DrahtBot requested review from TheCharlatan on May 1, 2024
  31. DrahtBot added the label Needs rebase on Jun 5, 2024
  32. fjahr force-pushed on Jun 6, 2024
  33. fjahr commented at 9:07 am on June 6, 2024: contributor

    Rebased

    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.

    If the rollback functionality gets implemented after this PR is merged, it seems straightforward to change this code

    Yes, either way, this PR should be an improvement. I am still planning to do the rollback in a cleaner way but if this PR is merged before that is ready, there should be no overhead resulting from that. It would become a refactoring follow-up that does not change the RPC interface again.

  34. in src/rpc/blockchain.cpp:2688 in 71b9fa96bf outdated
    2683+
    2684+        invalidate_index = WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Next(target_index));
    2685+        InvalidateBlock(*node.chainman, invalidate_index->GetBlockHash());
    2686+        const CBlockIndex* new_tip_index{WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Tip())};
    2687+
    2688+        // In case there is any issue with a block being read from disc we need
    


    maflcko commented at 9:13 am on June 6, 2024:
    Another issue could be that a sister block of invalidate_index was activated instead, if one exists? If so, could make sense to mention that as well.

    fjahr commented at 3:30 pm on June 6, 2024:
    Yes, interesting edge case! I extended the comment.
  35. in src/rpc/blockchain.cpp:2669 in 71b9fa96bf outdated
    2664+        const CBlockIndex* target_index = ParseHashOrHeight(request.params[1], *node.chainman);
    2665+
    2666+        // If the node is running in pruned mode we ensure all necessary block
    2667+        // data is available before starting to roll back.
    2668+        if (node.chainman->m_blockman.IsPruneMode()) {
    2669+            LOCK(::cs_main);
    


    maflcko commented at 9:15 am on June 6, 2024:
    style nit for new code, here and elsewhere: Could make sense to use the chainman.GetMutex() alias for chainman locking, instead of the global cs_main?

    fjahr commented at 3:30 pm on June 6, 2024:
    Done
  36. DrahtBot removed the label Needs rebase on Jun 6, 2024
  37. fjahr force-pushed on Jun 6, 2024
  38. Sjors commented at 1:53 pm on June 10, 2024: member

    I am still planning to do the rollback in a cleaner way but[…] It would become a refactoring follow-up that does not change the RPC interface again.

    That makes sense.

  39. in src/rpc/blockchain.cpp:2679 in 2cdce8fd9c outdated
    2674+            }
    2675+        }
    2676+
    2677+        // Suspend network activity for the duration of the process when we are
    2678+        // rolling back the chain to get a utxo set from a past height.
    2679+        connman.SetNetworkActive(false);
    


    ryanofsky commented at 1:34 pm on June 21, 2024:

    In commit “RPC: Add height parameter to dumptxoutset” (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)

    I don’t think it’s sufficient to call connman.SetNetworkActive() manually in this function, because the InvalidateBlock and ReconsiderBlock functions called here can both throw exceptions and leave the network disabled after this RPC call if there is an error. Would suggest making a NetworkDisable disable_network{connman}; RAII class that disables the network in its constructor and enables it in its destructor to improve the error handling.

    Also, can there be a comment to say why this is suspending network activity? I’m not actually sure why that is needed, since presumably networking code should not download invalid blocks, so it’s not clear what effect this has.


    fjahr commented at 4:51 pm on July 11, 2024:
    I have implemented the RAII class and expanded the comment. If my understanding is correct I think we would punish a peer as misbehaving if they send us a block that is connecting to an invalid block (https://github.com/bitcoin/bitcoin/blob/master/src/net_processing.cpp#L1927) so we would punish any peer sending us a normal new block. We also wouldn’t accept transactions that have a locktime above the snapshot height though I am not sure if switching off network activity is getting us a better result in that case.
  40. in src/rpc/blockchain.cpp:2688 in 2cdce8fd9c outdated
    2683+
    2684+        invalidate_index = WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Next(target_index));
    2685+        InvalidateBlock(*node.chainman, invalidate_index->GetBlockHash());
    2686+        const CBlockIndex* new_tip_index{WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Tip())};
    2687+
    2688+        // In case there is any issue with a block being read from disc we need
    


    ryanofsky commented at 1:34 pm on June 21, 2024:

    In commit “RPC: Add height parameter to dumptxoutset” (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)

    s/disc/disk/


    fjahr commented at 4:51 pm on July 11, 2024:
    fixed
  41. in doc/assumeutxo.md:12 in 6a5408dd7d outdated
     7+
     8+## Loading a snapshot
     9+
    10+There is currently no canonical source for snapshots, but any downloaded snapshot
    11+will be checked against a hash that's been hardcoded in source code. If there is
    12+no source for the snapshot you need, you can generate it yourself using
    


    ryanofsky commented at 6:47 pm on June 21, 2024:

    In commit “doc: Improve guide for usage of assumeutxo” (6a5408dd7da4c31283cb4f632b3a7aa692a53ca3)

    Maybe say “generate it yourself from another node” to be clear you need a different node to generate it.


    fjahr commented at 4:51 pm on July 11, 2024:
    added and mentioned also that this node needs to be synced already
  42. in doc/assumeutxo.md:45 in 6a5408dd7d outdated
    39+Indexes work but don't take advantage of this feature. They always start building
    40+from the genesis block. Once the background validation reaches the snapshot block,
    41+indexes will continue to build all the way to the tip.
    42+
    43+For indexes that support pruning, note that no pruning will take place between
    44+the snapshot and the tip, until the background sync has completed - after which
    


    ryanofsky commented at 6:56 pm on June 21, 2024:

    In commit “doc: Improve guide for usage of assumeutxo” (6a5408dd7da4c31283cb4f632b3a7aa692a53ca3)

    Is it true that no pruning will take place between the snapshot and the tip? I haven’t experimented but it looks like GetPruneRange will only return a minimum prune height for the snapshot chainstate, not the background chainstate.


    fjahr commented at 4:51 pm on July 11, 2024:
    I need to spend some more time with this and test if this actually changed but this was added in #28618 which goes back to the conversation in the last assumeutxo PR: #27596 (comment). I think nothing has changed in that regard though. I think the prune locks prevent the blocks of the snapshot from being pruned.

    ryanofsky commented at 11:52 am on July 24, 2024:

    re: #29553 (review)

    Oh, I think I just forgot about prune locks added by indexes when I wrote this comment. If indexes are disabled, then the normal GetPruneRange function that controls pruning will allow pruning blocks between the snapshot and the tip.

    But if indexes are enabled, because indexes prevent any blocks that haven’t been indexed yet from being pruned, and because indexes currently just ignore the snapshot and don’t take advantage of it, indexes only allow pruning between the genesis block and the last block indexed, and don’t allow anything after that to be pruned, including blocks between the snapshot and the tip.

    I think this section could probably be made clearer by spelling everything out explicitly:

    • Indexes don’t currently take advantage of the snapshot at all. Right now they can only index blocks in order, starting from genesis.
    • Indexes only allow blocks that were indexed to be pruned, and don’t allow any block that hasn’t been indexed yet to be pruned.
    • If the snapshot is old, then a lot of blocks after the snapshot block will need to be downloaded, and these blocks can’t be pruned until they are indexed, so they could consume a lot of disk space until indexing catches up to the snapshot block.

    fjahr commented at 9:48 pm on August 15, 2024:
    I made some edits here, making sure to clearly spell out what you suggested to include.
  43. in src/rpc/blockchain.cpp:2618 in 2cdce8fd9c outdated
    2614@@ -2615,6 +2615,9 @@ static RPCHelpMan dumptxoutset()
    2615         "Write the serialized UTXO set to a file.",
    2616         {
    2617             {"path", RPCArg::Type::STR, RPCArg::Optional::NO, "Path to the output file. If relative, will be prefixed by datadir."},
    2618+            {"height", RPCArg::Type::NUM, RPCArg::DefaultHint{"the current tip"},
    


    ryanofsky commented at 7:02 pm on June 21, 2024:

    In commit “RPC: Add height parameter to dumptxoutset” (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)

    For a followup, it might be convenient if height value defaulted to the last height value which could actually be loaded as a snapshot. It could also accept a “latest” string value in case the user does want to generate the latest snapshot.


    fjahr commented at 4:50 pm on July 11, 2024:

    Right, I think that makes sense and this will be called mostly with this argument than anything else. That hadn’t occurred to me so far honestly because I mostly wanted this for us to create snapshots that will potentially be added to the code in the future.

    I have changed this to default to the latest valid snapshot height. That makes getting the tip more cumbersome for the users but I think that’s ok since this should not be what most of the want, I think.

  44. in src/rpc/blockchain.cpp:2762 in 2cdce8fd9c outdated
    2681+            throw JSONRPCError(RPC_MISC_ERROR, "Network activity could not be suspended.");
    2682+        }
    2683+
    2684+        invalidate_index = WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Next(target_index));
    2685+        InvalidateBlock(*node.chainman, invalidate_index->GetBlockHash());
    2686+        const CBlockIndex* new_tip_index{WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Tip())};
    


    ryanofsky commented at 7:05 pm on June 21, 2024:

    In commit “RPC: Add height parameter to dumptxoutset” (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)

    It seems racy to unlock cs_main before calling CreateUTXOSnapshot. Not sure if there is a way to implement this in a race-free way without looping though. Could be something to follow up on later.


    fjahr commented at 4:50 pm on July 11, 2024:
    Leaving this for a follow-up for now.

    maflcko commented at 8:07 am on July 12, 2024:
    I guess it would still be good to leave a comment about the race in the source code (or in the docs on how to avoid the race). I guess the user would simply have to avoid calling other *block RPCs for the duration of dumptxoutset? (The p2p is already stopped, so shouldn’t be the cause of a race)

    fjahr commented at 9:20 am on July 12, 2024:
    I added a small comment in the code and also mention it now in the usage guide as well as the rpc help text.

    ryanofsky commented at 1:43 pm on July 24, 2024:

    re: #29553 (review)

    I don’t think this is critical to fix but if you wanted to remove the race condition, the following change should work:

      0--- a/src/rpc/blockchain.cpp
      1+++ b/src/rpc/blockchain.cpp
      2@@ -74,6 +74,22 @@ static GlobalMutex cs_blockchange;
      3 static std::condition_variable cond_blockchange;
      4 static CUpdatedBlock latestblock GUARDED_BY(cs_blockchange);
      5 
      6+std::tuple<std::unique_ptr<CCoinsViewCursor>, CCoinsStats, const CBlockIndex*>
      7+PrepareUTXOSnapshot(
      8+    Chainstate& chainstate,
      9+    const std::function<void()>& interruption_point = {})
     10+    EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
     11+
     12+UniValue WriteUTXOSnapshot(
     13+    Chainstate& chainstate,
     14+    CCoinsViewCursor* pcursor,
     15+    CCoinsStats* maybe_stats,
     16+    const CBlockIndex* tip,
     17+    AutoFile& afile,
     18+    const fs::path& path,
     19+    const fs::path& temppath,
     20+    const std::function<void()>& interruption_point = {});
     21+
     22 /* Calculate the difficulty for a given block index.
     23  */
     24 double GetDifficulty(const CBlockIndex& blockindex)
     25@@ -2730,7 +2746,7 @@ static RPCHelpMan dumptxoutset()
     26         target_index = ParseHashOrHeight(request.params[1], *node.chainman);
     27     }
     28 
     29-    const auto tip{WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Tip())};
     30+    const CBlockIndex* tip{WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Tip())};
     31     const CBlockIndex* invalidate_index{nullptr};
     32     std::unique_ptr<NetworkDisable> disable_network;
     33 
     34@@ -2754,32 +2770,41 @@ static RPCHelpMan dumptxoutset()
     35         // seems wrong in this temporary state. For example a normal new block
     36         // would be classified as a block connecting an invalid block.
     37         disable_network = std::make_unique<NetworkDisable>(connman);
     38-
     39-        // Note: Unlocking cs_main before CreateUTXOSnapshot might be racy
     40-        // if the user interacts with any other *block RPCs.
     41         invalidate_index = WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Next(target_index));
     42         InvalidateBlock(*node.chainman, invalidate_index->GetBlockHash());
     43-        const CBlockIndex* new_tip_index{WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Tip())};
     44+    }
     45 
     46+    Chainstate* chainstate;
     47+    std::unique_ptr<CCoinsViewCursor> cursor;
     48+    CCoinsStats stats;
     49+    UniValue result;
     50+    UniValue error;
     51+    {
     52+        LOCK(node.chainman->GetMutex());
     53+        chainstate = &node.chainman->ActiveChainstate();
     54         // In case there is any issue with a block being read from disk we need
     55         // to stop here, otherwise the dump could still be created for the wrong
     56         // height.
     57         // The new tip could also not be the target block if we have a stale
     58         // sister block of invalidate_index. This block (or a descendant) would
     59         // be activated as the new tip and we would not get to new_tip_index.
     60-        if (new_tip_index != target_index) {
     61-            ReconsiderBlock(*node.chainman, invalidate_index->GetBlockHash());
     62-            throw JSONRPCError(RPC_MISC_ERROR, "Could not roll back to requested height, reverting to tip.");
     63+        if (target_index != chainstate->m_chain.Tip()) {
     64+            error = JSONRPCError(RPC_MISC_ERROR, "Could not roll back to requested height, reverting to tip.");
     65+        } else {
     66+            std::tie(cursor, stats, tip) = PrepareUTXOSnapshot(*chainstate, node.rpc_interruption_point);
     67         }
     68     }
     69 
     70-    UniValue result = CreateUTXOSnapshot(
     71-        node, node.chainman->ActiveChainstate(), afile, path, temppath);
     72-    fs::rename(temppath, path);
     73-
     74+    if (error.isNull()) {
     75+        result = WriteUTXOSnapshot(*chainstate, cursor.get(), &stats, tip, afile, path, temppath, node.rpc_interruption_point);
     76+        fs::rename(temppath, path);
     77+    }
     78     if (invalidate_index) {
     79         ReconsiderBlock(*node.chainman, invalidate_index->GetBlockHash());
     80     }
     81+    if (!error.isNull()) {
     82+        throw error;
     83+    }
     84 
     85     result.pushKV("path", path.utf8string());
     86     return result;
     87@@ -2787,12 +2812,10 @@ static RPCHelpMan dumptxoutset()
     88     };
     89 }
     90 
     91-UniValue CreateUTXOSnapshot(
     92-    NodeContext& node,
     93+std::tuple<std::unique_ptr<CCoinsViewCursor>, CCoinsStats, const CBlockIndex*>
     94+PrepareUTXOSnapshot(
     95     Chainstate& chainstate,
     96-    AutoFile& afile,
     97-    const fs::path& path,
     98-    const fs::path& temppath)
     99+    const std::function<void()>& interruption_point)
    100 {
    101     std::unique_ptr<CCoinsViewCursor> pcursor;
    102     std::optional<CCoinsStats> maybe_stats;
    103@@ -2811,11 +2834,11 @@ UniValue CreateUTXOSnapshot(
    104         // See discussion here:
    105         //   [#15606 (review)](/bitcoin-bitcoin/15606/#discussion_r274479369)
    106         //
    107-        LOCK(::cs_main);
    108+        AssertLockHeld(::cs_main);
    109 
    110         chainstate.ForceFlushStateToDisk();
    111 
    112-        maybe_stats = GetUTXOStats(&chainstate.CoinsDB(), chainstate.m_blockman, CoinStatsHashType::HASH_SERIALIZED, node.rpc_interruption_point);
    113+        maybe_stats = GetUTXOStats(&chainstate.CoinsDB(), chainstate.m_blockman, CoinStatsHashType::HASH_SERIALIZED, interruption_point);
    114         if (!maybe_stats) {
    115             throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to read UTXO set");
    116         }
    117@@ -2823,7 +2846,19 @@ UniValue CreateUTXOSnapshot(
    118         pcursor = chainstate.CoinsDB().Cursor();
    119         tip = CHECK_NONFATAL(chainstate.m_blockman.LookupBlockIndex(maybe_stats->hashBlock));
    120     }
    121+    return {std::move(pcursor), *CHECK_NONFATAL(maybe_stats), tip};
    122+}
    123 
    124+UniValue WriteUTXOSnapshot(
    125+    Chainstate& chainstate,
    126+    CCoinsViewCursor* pcursor,
    127+    CCoinsStats* maybe_stats,
    128+    const CBlockIndex* tip,
    129+    AutoFile& afile,
    130+    const fs::path& path,
    131+    const fs::path& temppath,
    132+    const std::function<void()>& interruption_point)
    133+{
    134     LOG_TIME_SECONDS(strprintf("writing UTXO snapshot at height %s (%s) to file %s (via %s)",
    135         tip->nHeight, tip->GetBlockHash().ToString(),
    136         fs::PathToString(path), fs::PathToString(temppath)));
    137@@ -2859,7 +2894,7 @@ UniValue CreateUTXOSnapshot(
    138     pcursor->GetKey(key);
    139     last_hash = key.hash;
    140     while (pcursor->Valid()) {
    141-        if (iter % 5000 == 0) node.rpc_interruption_point();
    142+        if (iter % 5000 == 0) interruption_point();
    143         ++iter;
    144         if (pcursor->GetKey(key) && pcursor->GetValue(coin)) {
    145             if (key.hash != last_hash) {
    146@@ -2890,6 +2925,17 @@ UniValue CreateUTXOSnapshot(
    147     return result;
    148 }
    149 
    150+UniValue CreateUTXOSnapshot(
    151+    node::NodeContext& node,
    152+    Chainstate& chainstate,
    153+    AutoFile& afile,
    154+    const fs::path& path,
    155+    const fs::path& tmppath)
    156+{
    157+    auto [cursor, stats, tip]{WITH_LOCK(::cs_main, return PrepareUTXOSnapshot(chainstate, node.rpc_interruption_point))};
    158+    return WriteUTXOSnapshot(chainstate, cursor.get(), &stats, tip, afile, path, tmppath, node.rpc_interruption_point);
    159+}
    160+
    161 static RPCHelpMan loadtxoutset()
    162 {
    163     return RPCHelpMan{
    

    fjahr commented at 9:11 pm on August 15, 2024:
    I have introduced your suggested fix as a separate commit because the main commit is already quite big and I think it’s easier to review this way. I added some minor documentation changes since CreateUTXOSnaphot() is now only used in tests.
  45. in src/rpc/blockchain.cpp:2615 in 2cdce8fd9c outdated
    2614@@ -2615,6 +2615,9 @@ static RPCHelpMan dumptxoutset()
    2615         "Write the serialized UTXO set to a file.",
    


    ryanofsky commented at 8:00 pm on June 21, 2024:

    In commit “RPC: Add height parameter to dumptxoutset” (https://github.com/bitcoin/bitcoin/commit/2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)

    Documentation here is really terse. For a folluwup , I think it would be nice to add some of the documentation from the usage guide to the RPC documentation so it is more accessible.


    fjahr commented at 4:50 pm on July 11, 2024:
    Added a bit more detail in the docs improvement commit.
  46. ryanofsky approved
  47. ryanofsky commented at 8:03 pm on June 21, 2024: contributor
    Code review ACK 6a5408dd7da4c31283cb4f632b3a7aa692a53ca3. This is a really nice improvement over existing shell scripts, with a surprisingly simple implementation. I left some suggestions, but none are important and this looks good to merge in its current form.
  48. DrahtBot requested review from alfonsoromanz on Jun 21, 2024
  49. achow101 referenced this in commit f4849f6922 on Jul 10, 2024
  50. fjahr force-pushed on Jul 11, 2024
  51. DrahtBot added the label CI failed on Jul 11, 2024
  52. fjahr force-pushed on Jul 11, 2024
  53. DrahtBot removed the label CI failed on Jul 12, 2024
  54. fjahr force-pushed on Jul 12, 2024
  55. fjahr commented at 10:17 am on July 12, 2024: contributor
    Rebased and addressed feedback from @ryanofsky and @maflcko, thank you! This now includes the check for undo data since this was made possible by merge of #29668.
  56. Asma1665 commented at 7:28 am on July 15, 2024: none
    Revarse my assist
  57. fjahr force-pushed on Jul 15, 2024
  58. fjahr commented at 10:35 am on July 15, 2024: contributor
    Fixed some outdated information in the remainin design doc and added that to the last commit.
  59. in src/rpc/blockchain.cpp:2708 in 5df60fb49c outdated
    2674@@ -2656,6 +2675,9 @@ static RPCHelpMan dumptxoutset()
    2675         "Write the serialized UTXO set to a file.",
    2676         {
    2677             {"path", RPCArg::Type::STR, RPCArg::Optional::NO, "Path to the output file. If relative, will be prefixed by datadir."},
    2678+            {"height", RPCArg::Type::NUM, RPCArg::DefaultHint{"the latest valid snapshot height"},
    2679+                "Height of the UTXO set file. Note: The further this number is from the tip, the longer this process will take. Consider setting a higher -rpcclienttimeout value in this case."
    2680+            },
    


    ryanofsky commented at 12:14 pm on July 24, 2024:

    In commit “RPC: Add height parameter to dumptxoutset” (5df60fb49cfae1e3a56b68e2f43ff0722f39abb7)

    Seems like this accepts a height or a hash, so could mention this accepts block hashes and maybe call this height_or_hash or whatever the convention is


    fjahr commented at 9:06 pm on August 15, 2024:
    Adopted the type approach you suggested so this is resolved as well with that.
  60. in src/rpc/blockchain.cpp:2728 in 27767184aa outdated
    2723+
    2724+    const CBlockIndex* target_index{};
    2725+    if (request.params[1].isNull()) {
    2726+        auto snapshot_heights = node.chainman->GetParams().GetAvailableSnapshotHeights();
    2727+        auto max_height = std::max_element(snapshot_heights.begin(), snapshot_heights.end());
    2728+        target_index = ParseHashOrHeight(*max_height, *node.chainman);
    


    ryanofsky commented at 1:58 pm on July 24, 2024:

    In commit “RPC: Add height parameter to dumptxoutset” (5df60fb49cfae1e3a56b68e2f43ff0722f39abb7)

    It’s not safe to dereference max_height if snapshot_heights is empty. Would be good to either assert snapshot_heights is nonempty, or just leave target_index null in this case and not roll back if it is null.


    fjahr commented at 9:05 pm on August 15, 2024:
    fixed (as part of one of the larger changes)
  61. ryanofsky approved
  62. ryanofsky commented at 2:03 pm on July 24, 2024: contributor

    Code review ACK 27767184aaa7fa94600c70334cc4122e25872ff6.

    Main change since last review is documenting dumptxoutset better and making default behavior be to dump the latest snapshot that can be loaded, instead of the current utxo set. Also improved other documentation and added NetworkDisable class to more reliably restore network state in case of error.

    Left some additional suggestions but they aren’t very important

  63. in src/rpc/blockchain.cpp:2676 in 27767184aa outdated
    2671@@ -2672,7 +2672,9 @@ static RPCHelpMan dumptxoutset()
    2672 {
    2673     return RPCHelpMan{
    2674         "dumptxoutset",
    2675-        "Write the serialized UTXO set to a file.",
    2676+        "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"
    2677+        "Unless the requested height is the current tip, the node will roll back to the requested height and network activity will be suspended during this process."
    


    mzumsande commented at 6:51 pm on July 25, 2024:
    nit: needs a space after “process”.

    fjahr commented at 9:05 pm on August 15, 2024:
    fixed
  64. in src/rpc/blockchain.cpp:2723 in 5df60fb49c outdated
    2716@@ -2695,10 +2717,66 @@ static RPCHelpMan dumptxoutset()
    2717     }
    2718 
    2719     NodeContext& node = EnsureAnyNodeContext(request.context);
    2720+    CConnman& connman = EnsureConnman(node);
    2721+
    2722+    const CBlockIndex* target_index{};
    2723+    if (request.params[1].isNull()) {
    


    mzumsande commented at 7:25 pm on July 25, 2024:

    I don’t like this default. This RPC is already kind of dangerous, it could roll back and forth the chain for hundred thousands of blocks (if executed a few years after it’s released), which could take days, and could lead to inconsistencies that would need manual effort to fix (if the process was interrupted by user, power failure etc.).

    I think that we should make it harder to accidentally misuse it and require users to explicitly state the height/ block beofre attempting huge rollbacks. So I’d prefer to either dump the tip by default, or make the height param mandatory.

    Also, I’m not sure the use case is strong enough to warrant this default. It can be used both to create new snapshots (not registered before) and to “recreate” a snapshot, maybe in order to distribute it to others. I don’t really see why the second use case is so much more important that it should be the default.


    ryanofsky commented at 10:53 pm on July 25, 2024:

    I don’t like this default. This RPC is already kind of dangerous, it could roll back and forth the chain for hundred thousands of blocks (if executed a few years after it’s released), which could take days

    That’s a good point. Given a choice of whether the default behavior might create a useless snapshot that can’t be loaded or take the node offline trying to produce a really old snapshot, that makes me think there should probably not be a default value at all and the user should have to explicitly specify the behavior they want.

    Would suggest adding a type parameter for the user to specify whether they want the latest snapshot or to roll back to a historic snapshot, and a rollback option to optionally specify the height or hash of a historic snapshot. Usage would look like:

    0# create the a snapshot of the latest utxo set
    1bitcoin-cli dumptxoutset utxo.dat latest
    2# roll back and create a snapshot of the latest utxo set that can be loaded
    3bitcoin-cli dumptxoutset utxo.dat rollback
    4# roll back to the specified height and create snapshot of that utxo set
    5bitcoin-cli -named dumptxoutset utxo.dat rollback=853456
    

    And a possible implementation would be:

     0--- a/src/rpc/blockchain.cpp
     1+++ b/src/rpc/blockchain.cpp
     2@@ -2673,12 +2673,17 @@ static RPCHelpMan dumptxoutset()
     3     return RPCHelpMan{
     4         "dumptxoutset",
     5         "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"
     6-        "Unless the requested height is the current tip, the node will roll back to the requested height and network activity will be suspended during this process."
     7+        "Unless the requested height is the current tip, the node will roll back to the requested height and network activity will be suspended during this process. "
     8         "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.",
     9         {
    10             {"path", RPCArg::Type::STR, RPCArg::Optional::NO, "Path to the output file. If relative, will be prefixed by datadir."},
    11-            {"height", RPCArg::Type::NUM, RPCArg::DefaultHint{"the latest valid snapshot height"},
    12-                "Height of the UTXO set file. Note: The further this number is from the tip, the longer this process will take. Consider setting a higher -rpcclienttimeout value in this case."
    13+            {"type", RPCArg::Type::STR, RPCArg::Default(""), "The type of snapshot to create. Can be \"latest\" to create a snapshot of the current UTXO set or \"rollback\" to temporarily roll back the state of the node to a historical block before creating the snapshot of a historical UTXO set. This parameter can be omitted if a separate \"rollback\" named parameter is specified indicating the height or hash of a specific historical block. If \"rollback\" is specified and separate \"rollback\" named parameter is not specified, this will roll back to the latest valid snapshot block that currently be loaded with loadtxoutset."},
    14+            {"options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "",
    15+                {
    16+                    {"rollback", RPCArg::Type::NUM, RPCArg::Optional::OMITTED,
    17+                        "Height or hash of the block to roll back to before creating the snapshot. Note: The further this number is from the tip, the longer this process will take. Consider setting a higher -rpcclienttimeout value in this case.",
    18+                    RPCArgOptions{.skip_type_check = true, .type_str = {"", "string or numeric"}}},
    19+                },
    20             },
    21         },
    22         RPCResult{
    23@@ -2693,10 +2698,33 @@ static RPCHelpMan dumptxoutset()
    24                 }
    25         },
    26         RPCExamples{
    27-            HelpExampleCli("dumptxoutset", "utxo.dat")
    28+            HelpExampleCli("dumptxoutset", "utxo.dat latest") +
    29+            HelpExampleCli("dumptxoutset", "utxo.dat rollback") +
    30+            HelpExampleCli("-named dumptxoutset", R"(utxo.dat rollback=853456)")
    31         },
    32         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
    33 {
    34+    NodeContext& node = EnsureAnyNodeContext(request.context);
    35+    const CBlockIndex* tip{WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Tip())};
    36+    const CBlockIndex* target_index{nullptr};
    37+    const std::string snapshot_type{self.Arg<std::string>("type")};
    38+    const UniValue options{request.params[2].isNull() ? UniValue::VOBJ : request.params[2]};
    39+    if (options.exists("rollback")) {
    40+        if (!snapshot_type.empty() && snapshot_type != "rollback") {
    41+            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid snapshot type \"%s\" specified with rollback option", snapshot_type));
    42+        }
    43+        target_index = ParseHashOrHeight(options["rollback"], *node.chainman);
    44+    } else if (snapshot_type == "rollback") {
    45+        auto snapshot_heights = node.chainman->GetParams().GetAvailableSnapshotHeights();
    46+        CHECK_NONFATAL(snapshot_heights.size() > 0);
    47+        auto max_height = std::max_element(snapshot_heights.begin(), snapshot_heights.end());
    48+        target_index = ParseHashOrHeight(*max_height, *node.chainman);
    49+    } else if (snapshot_type == "latest") {
    50+        target_index = tip;
    51+    } else {
    52+        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid snapshot type \"%s\" specified. Please specify \"rollback\" or \"latest\"", snapshot_type));
    53+    }
    54+  
    55     const ArgsManager& args{EnsureAnyArgsman(request.context)};
    56     const fs::path path = fsbridge::AbsPathJoin(args.GetDataDirNet(), fs::u8path(request.params[0].get_str()));
    57     // Write to a temporary path and then move into `path` on completion
    58@@ -2718,19 +2746,7 @@ static RPCHelpMan dumptxoutset()
    59             "Couldn't open file " + temppath.utf8string() + " for writing.");
    60     }
    61 
    62-    NodeContext& node = EnsureAnyNodeContext(request.context);
    63     CConnman& connman = EnsureConnman(node);
    64-
    65-    const CBlockIndex* target_index{};
    66-    if (request.params[1].isNull()) {
    67-        auto snapshot_heights = node.chainman->GetParams().GetAvailableSnapshotHeights();
    68-        auto max_height = std::max_element(snapshot_heights.begin(), snapshot_heights.end());
    69-        target_index = ParseHashOrHeight(*max_height, *node.chainman);
    70-    } else {
    71-        target_index = ParseHashOrHeight(request.params[1], *node.chainman);
    72-    }
    73-
    74-    const auto tip{WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Tip())};
    75     const CBlockIndex* invalidate_index{nullptr};
    76     std::unique_ptr<NetworkDisable> disable_network;
    77 
    78--- a/src/rpc/client.cpp
    79+++ b/src/rpc/client.cpp
    80@@ -185,7 +185,8 @@ static const CRPCConvertParam vRPCConvertParams[] =
    81     { "gettxoutproof", 0, "txids" },
    82     { "gettxoutsetinfo", 1, "hash_or_height" },
    83     { "gettxoutsetinfo", 2, "use_index"},
    84-    { "dumptxoutset", 1, "height"},
    85+    { "dumptxoutset", 2, "options" },
    86+    { "dumptxoutset", 2, "rollback" },
    87     { "lockunspent", 0, "unlock" },
    88     { "lockunspent", 1, "transactions" },
    89     { "lockunspent", 2, "persistent" },
    

    fjahr commented at 9:09 pm on August 15, 2024:
    Thanks, I agree that the default wasn’t very sensitive when looking at it this way. I have adopted the type approach that @ryanofsky suggested. I think it may be a bit more complicated to understand initially for users but it’s also easier to hand because you don’t have to deal with a hash or height in the most common use cases.
  65. mzumsande commented at 8:02 pm on July 25, 2024: contributor
    Concept ACK - though I don’t like having potentially large rollbacks as default behavior, see below.
  66. DrahtBot added the label Needs rebase on Aug 5, 2024
  67. fjahr force-pushed on Aug 15, 2024
  68. fjahr force-pushed on Aug 15, 2024
  69. fjahr commented at 9:50 pm on August 15, 2024: contributor

    Addressed feedback and rebased.

    There is a bit of a hack in the tests now that is made necessary by an unrelated bug: #26245 I marked this with an TODO accordingly.

  70. in src/rpc/blockchain.cpp:2811 in 0c1238917a outdated
    2801+    std::unique_ptr<CCoinsViewCursor> cursor;
    2802+    CCoinsStats stats;
    2803+    UniValue result;
    2804+    UniValue error;
    2805+    {
    2806+        LOCK(node.chainman->GetMutex());
    


    ryanofsky commented at 9:58 pm on August 15, 2024:

    In commit “rpc, refactor: Prevent potential race conditions in dumptxoutset” (0c1238917a5edf6bdcae15aab5f91b1bdd2540b9)

    Would be nice to add a comment explaining locking here. Maybe “// Lock the chainstate before calling PrepareUtxoSnapshot, to be able to get a UTXO database cursor while the chain is pointing at the target block. After that, release the lock while calling WriteUTXOSnapshot. The cursor will remain valid and be used by WriteUTXOSnapshot to write a consistent snapshot even if the chainstate changes.”


    fjahr commented at 10:22 pm on August 21, 2024:
    Taken, thanks!
  71. in src/rpc/blockchain.cpp:2845 in 0c1238917a outdated
    2843 }
    2844 
    2845-UniValue CreateUTXOSnapshot(
    2846-    NodeContext& node,
    2847+std::tuple<std::unique_ptr<CCoinsViewCursor>, CCoinsStats, const CBlockIndex*>
    2848+PrepareUTXOSnapshot(
    


    ryanofsky commented at 10:01 pm on August 15, 2024:

    In commit “rpc, refactor: Prevent potential race conditions in dumptxoutset” (0c1238917a5edf6bdcae15aab5f91b1bdd2540b9)

    Would be good to update comment on line 2852. Could replace “use below this block” with “use in WriteUTXOSnapshot”


    fjahr commented at 10:22 pm on August 21, 2024:
    Done
  72. in src/rpc/blockchain.cpp:2831 in 0c1238917a outdated
    2827-
    2828+    if (error.isNull()) {
    2829+        result = WriteUTXOSnapshot(*chainstate, cursor.get(), &stats, tip, afile, path, temppath, node.rpc_interruption_point);
    2830+        fs::rename(temppath, path);
    2831+    }
    2832     if (invalidate_index) {
    


    ryanofsky commented at 10:10 pm on August 15, 2024:

    In commit “rpc, refactor: Prevent potential race conditions in dumptxoutset” (0c1238917a5edf6bdcae15aab5f91b1bdd2540b9)

    Would it make any sense to call ReconsiderBlock() and disable_network.reset() before calling WriteUTXOSnapshot instead of after?

    Not sure if that might make the node more usable while the snapshot is being created, or just slow down writing the snapshot and increase resource usage.

    Change wouldn’t be appropriate in this commit anyway, but could be a possible followup or possible future dumptxoutset RPC option if it makes sense.


    fjahr commented at 10:28 pm on August 21, 2024:

    I did some light testing of this and didn’t see a negative performance impact. Actually the three times I ran with this change were slightly faster on average than with the current code here. But there was also quite a bit of volatility so that might have been pure luck. It doesn’t look like it slows things down though, at least not on my machine.

    I have left it out for now but can add it. I am curious what you mean by having it as an RPC option. This seems like something that we should just do if it’s safe and we think it’s beneficial for users and if not we just leave it out. I’m not sure which users would care enough about such a setting. This is also not a RPC that a lot of people will use when they do it’s rather infrequently I assume.


    Sjors commented at 10:25 am on August 22, 2024:

    It seems conceptually simpler to first write the snapshot, reconsider all blocks that were marked invalid and only then restore network activity.

    make the node more usable while the snapshot is being created

    Lower usability is probably a feature. The user should not be doing anything important until the chain is back at the original tip, e.g. loading a wallet could still cause confusion.


    ryanofsky commented at 3:23 pm on August 22, 2024:

    re: #29553 (review)

    Interesting results and I agree it makes sense to leave this alone for now. I thought it adding an option might be reasonable because doing this might increase resource usage even if it is faster. And maybe it is better if node is unusable for longer as Sjors says. I don’t have a clear sense of what is best here.

  73. DrahtBot removed the label Needs rebase on Aug 15, 2024
  74. ryanofsky approved
  75. ryanofsky commented at 10:27 pm on August 15, 2024: contributor

    Code review ACK 0c1238917a5edf6bdcae15aab5f91b1bdd2540b9. Main change since last review is that dumptxoutset will no longer dump a snapshot that can’t be loaded by default or roll back the chain by default. Instead you need to specify “latest” or “rollback” to choose the behavior you want, or specify the height or hash of a specific block.

    It is now possible to specify a snapshot block hash instead of a just height, and there are test and documentation improvements, and a new commit that locks cs_main between rolling back the chainstate and getting the UTXO database cursor to guarantee the right snapshot block is always used.

  76. DrahtBot requested review from mzumsande on Aug 15, 2024
  77. pablomartin4btc commented at 3:22 pm on August 21, 2024: member

    Concept ACK

    I agree with the reasoning behind removing the scripts, specially after founding this bug during testing. I’ll also test this PR soon.

  78. fjahr force-pushed on Aug 21, 2024
  79. fjahr commented at 10:29 pm on August 21, 2024: contributor
    Improved comments based on suggestions by @ryanofsky , thanks a lot!
  80. in doc/assumeutxo.md:76 in bb3981c019 outdated
    71+```
    72+$ bitcoin-cli dumptxoutset /path/to/output rollback
    73+```
    74+
    75+For most of the duration of `dumptxoutset` running the node is in a temporary
    76+state that does not actually reflect reality, i.e. blocks are marked valid
    


    Sjors commented at 9:03 am on August 22, 2024:
    bb3981c01985989b42da0a24c027be757ab54612: -> “are marked invalid”

    fjahr commented at 10:13 pm on August 23, 2024:
    fixed
  81. in doc/assumeutxo.md:79 in bb3981c019 outdated
    74+
    75+For most of the duration of `dumptxoutset` running the node is in a temporary
    76+state that does not actually reflect reality, i.e. blocks are marked valid
    77+although we know they are not invalid. Because of this it is discouraged to
    78+interact with the node in any other way during this time to avoid inconsistent
    79+results and race conditions, particularly RPCs that interact with blockstorage.
    


    Sjors commented at 9:05 am on August 22, 2024:
    bb3981c01985989b42da0a24c027be757ab54612: may be add: “This inconsistent state is also why network activity is temporarily disabled, causing us to disconnect from all peers.”

    fjahr commented at 10:13 pm on August 23, 2024:
    added
  82. in src/rpc/blockchain.cpp:2679 in bb3981c019 outdated
    2674@@ -2675,7 +2675,9 @@ static RPCHelpMan dumptxoutset()
    2675 {
    2676     return RPCHelpMan{
    2677         "dumptxoutset",
    2678-        "Write the serialized UTXO set to a file.",
    2679+        "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"
    2680+        "Unless the the latest type is requested, the node will roll back to the requested height and network activity will be suspended during this process. "
    


    Sjors commented at 9:08 am on August 22, 2024:

    bb3981c01985989b42da0a24c027be757ab54612 \"latest\"?

    The first time I read “latest type” as meaning a new type.


    fjahr commented at 10:13 pm on August 23, 2024:
    done
  83. in src/rpc/blockchain.cpp:2701 in b23abfca68 outdated
    2677@@ -2659,6 +2678,14 @@ static RPCHelpMan dumptxoutset()
    2678         "Write the serialized UTXO set to a file.",
    2679         {
    2680             {"path", RPCArg::Type::STR, RPCArg::Optional::NO, "Path to the output file. If relative, will be prefixed by datadir."},
    2681+            {"type", RPCArg::Type::STR, RPCArg::Default(""), "The type of snapshot to create. Can be \"latest\" to create a snapshot of the current UTXO set or \"rollback\" to temporarily roll back the state of the node to a historical block before creating the snapshot of a historical UTXO set. This parameter can be omitted if a separate \"rollback\" named parameter is specified indicating the height or hash of a specific historical block. If \"rollback\" is specified and separate \"rollback\" named parameter is not specified, this will roll back to the latest valid snapshot block that currently be loaded with loadtxoutset."},
    


    Sjors commented at 9:54 am on August 22, 2024:

    b23abfca68f7b349e6d2f9e04369f0fdc82d2a98: I had to read this description several times to get it, but the examples make it clear.

    Having a third positional optional argument height would have seemed easier to me, but it’s fine like this.


    fjahr commented at 10:13 pm on August 23, 2024:
    I agree that -height=XXXX is better but having -rollback and -height=XXXXX seems worse than rollback and -rollback=XXXXX and of course just -height doesn’t make sense. So leaving this as is for now.
  84. in src/rpc/blockchain.cpp:2817 in b23abfca68 outdated
    2779+
    2780+        // In case there is any issue with a block being read from disk we need
    2781+        // to stop here, otherwise the dump could still be created for the wrong
    2782+        // height.
    2783+        // The new tip could also not be the target block if we have a stale
    2784+        // sister block of invalidate_index. This block (or a descendant) would
    


    Sjors commented at 10:12 am on August 22, 2024:

    b23abfca68f7b349e6d2f9e04369f0fdc82d2a98: a followup could handle multiple blocks at the same height by running a loop that invalidates all (new) tips at the target height that are not the block hash we expected.

    Those could be tracked in invalidate_indexes and be reconsidered in reverse order (and with the network still off) after the snapshot is made.


    fjahr commented at 10:13 pm on August 23, 2024:
    Leaving this for a follow-up.

    Sjors commented at 9:17 am on August 26, 2024:
    It can wait until some bully actually wastes $100K+ to produce such a stale block at an existing assume utxo height :-)
  85. in src/rpc/blockchain.cpp:2786 in b23abfca68 outdated
    2782+        // height.
    2783+        // The new tip could also not be the target block if we have a stale
    2784+        // sister block of invalidate_index. This block (or a descendant) would
    2785+        // be activated as the new tip and we would not get to new_tip_index.
    2786+        if (new_tip_index != target_index) {
    2787+            ReconsiderBlock(*node.chainman, invalidate_index->GetBlockHash());
    


    Sjors commented at 10:18 am on August 22, 2024:
    b23abfca68f7b349e6d2f9e04369f0fdc82d2a98: assuming this is blocking and it could take hours before the throw below, it would be good to emit a log message in the mean time.

    fjahr commented at 10:13 pm on August 23, 2024:
    done
  86. in src/rpc/blockchain.cpp:2792 in b23abfca68 outdated
    2788+            throw JSONRPCError(RPC_MISC_ERROR, "Could not roll back to requested height, reverting to tip.");
    2789+        }
    2790+    }
    2791+
    2792     UniValue result = CreateUTXOSnapshot(
    2793         node, node.chainman->ActiveChainstate(), afile, path, temppath);
    


    Sjors commented at 10:20 am on August 22, 2024:
    b23abfca68f7b349e6d2f9e04369f0fdc82d2a98: for a followup, please check if this path is writable early in the RPC function. I once wasted a few hours because I used ~/someplace which it didn’t understand.

    fjahr commented at 10:13 pm on August 23, 2024:

    Hm, doesn’t this code catch this already?

    0    FILE* file{fsbridge::fopen(temppath, "wb")};
    1    AutoFile afile{file};
    2    if (afile.IsNull()) {
    

    I get this here:

    0$ src/bitcoin-cli dumptxoutset \~/Downloads/utxo.dat latest
    1error code: -8
    2error message:
    3Couldn't open file /Users/XXX/Library/Application Support/Bitcoin/~/Downloads/utxo.dat.incomplete for writing.
    

    Can you give a more detailed example to reproduce this maybe?


    Sjors commented at 9:14 am on August 26, 2024:
    I ran into that issue with the bash script, not this PR. This PR actually fixes the problem. The bash script would first do the rollback and only then call dumptxoutset, which is where the check is. So it’s fine now.
  87. Sjors approved
  88. Sjors commented at 10:56 am on August 22, 2024: member

    tACK b5292334e5e8792644123b627896359eb1da8d25

    I was initially confused because I got this:

    0$ src/bitcoin-cli dumptxoutset $HOME/dev/utxo-snapshots/utxo-840000-pr29553.dat rollback
    1error code: -1
    2error message:
    3Internal bug detected: snapshot_heights.size() > 0
    4rpc/blockchain.cpp:2738 (operator())
    5Bitcoin Core v27.99.0-b5292334e5e8
    6Please report this issue here: https://github.com/bitcoin/bitcoin/issues
    

    But that’s because this PR wasn’t rebased after #28553 got merged.

    After rebasing it myself, I was able to reproduce the snapshot at height 840,000.

    This can wait for a followup, but can you add -rpcclienttimeout=0 to all the examples? And to the help text:

    0"This call may take several minutes. Make sure to use no RPC timeout (bitcoin-cli -rpcclienttimeout=0)",
    

    The one thing I like about the original script, which the RPC can’t give us, is a progress report.

    This is a general problem with long running RPC calls.

    I’m a bit confused about the last commit, especially @ryanofsky’s comment:

    0// The cursor will remain valid and be used by
    1// WriteUTXOSnapshot to write a consistent snapshot even if the
    2// chainstate changes.
    

    Does CCoinsViewCursor (and LevelDB) somehow guarantee that if coins are deleted from the chainstate database, as new blocks are processed, we can still access them? (and that we won’t see newly added coins).

    Or merely that the cursor becomes invalid so we can detect that things changed from under us? If so, where is that handled?

    For the (main) use case of rolling back to a specific height it doesn’t matter, because we first create and save the snapshot and only then call ReconsiderBlock. But for the case of making a snapshot at the tip, it’s not clear to me what would happen.

  89. DrahtBot requested review from pablomartin4btc on Aug 22, 2024
  90. DrahtBot requested review from ryanofsky on Aug 22, 2024
  91. ryanofsky approved
  92. ryanofsky commented at 3:41 pm on August 22, 2024: contributor

    Code review ACK b5292334e5e8792644123b627896359eb1da8d25, just updated code comments since last review.


    re: #29553#pullrequestreview-2253922313

    Does CCoinsViewCursor (and LevelDB) somehow guarantee that if coins are deleted from the chainstate database, as new blocks are processed, we can still access them? (and that we won’t see newly added coins).

    CCoinsViewCursor is a dumb cursor, so leveldb would be doing all the work, but my understanding is leveldb cursors iterate over the database at a snapshot in time and don’t reflect future updates or deletions.

  93. fjahr force-pushed on Aug 23, 2024
  94. fjahr commented at 10:16 pm on August 23, 2024: contributor

    I addressed @Sjors ’s feedback and rebased so the behavior can be tested with the mainnet params.

    This can wait for a followup, but can you add -rpcclienttimeout=0 to all the examples?

    Done

    But that’s because this PR wasn’t rebased after #28553 got merged.

    Rebased for easier testing.

  95. Sjors commented at 9:20 am on August 26, 2024: member
    re-utACK ac9d53d0dee26db58ab125aa369f476c232da660
  96. DrahtBot requested review from ryanofsky on Aug 26, 2024
  97. ryanofsky approved
  98. ryanofsky commented at 2:36 pm on August 26, 2024: contributor
    Code review ACK ac9d53d0dee26db58ab125aa369f476c232da660. Nice documentation improvements since last review, and a new log statement
  99. achow101 referenced this in commit a1f2b5bbb5 on Aug 26, 2024
  100. DrahtBot added the label Needs rebase on Aug 26, 2024
  101. fjahr force-pushed on Aug 26, 2024
  102. fjahr commented at 10:53 pm on August 26, 2024: contributor
    Rebased due to #30690
  103. DrahtBot removed the label Needs rebase on Aug 27, 2024
  104. Sjors commented at 6:33 am on August 27, 2024: member

    re-utACK 46c8d75d24d3355ec468f7f608effe94636e16db

    #30690 just modified the script this PR deletes.

  105. DrahtBot requested review from ryanofsky on Aug 27, 2024
  106. pablomartin4btc approved
  107. pablomartin4btc commented at 10:59 pm on August 30, 2024: member

    tACK 46c8d75d24d3355ec468f7f608effe94636e16db

    I’ve reviewed the code, it looks very neat.

     0/src/bitcoin-cli -datadir=${AU_DATADIR} -rpcclienttimeout=0 dumptxoutset utxo-840000.dat rollback
     1{
     2  "coins_written": 176948713,
     3  "base_hash": "0000000000000000000320283a032748cef8227873ff4872689bf23f1cda83a5",
     4  "base_height": 840000,
     5  "path": "/home/pablo/.test_utxo_840/utxo-840000.dat",
     6  "txoutset_hash": "a2a5521b1b5ab65f67818e5e8eccabb7171a517f9e2382208f77687310768f96",
     7  "nchaintx": 991032194
     8}
     9
    10
    11./src/bitcoin-cli -datadir=${AU_DATADIR} -rpcclienttimeout=0 dumptxoutset utxo-859000.dat latest
    12{
    13  "coins_written": 185421121,
    14  "base_hash": "00000000000000000002806cbe89e9694072f8f2a04cc77b73f69a4c9cdc5ec3",
    15  "base_height": 859144,
    16  "path": "/home/pablo/.test_utxo_840/utxo-859000.dat",
    17  "txoutset_hash": "2c0d181bcbef8364ac4202671960e91484ee3b64d33a72df35f4ffe5064852ae",
    18  "nchaintx": 1070242632
    19}
    20
    21./src/bitcoin-cli -datadir=${AU_DATADIR} -rpcclienttimeout=0 -named dumptxoutset utxo-858000.dat rollback=858000
    22{
    23  "coins_written": 184931392,
    24  "base_hash": "00000000000000000001b44004f45de683f7f5dc792519b47f867baac2d4bf56",
    25  "base_height": 858000,
    26  "path": "/home/pablo/.test_utxo_840/utxo-858000.dat",
    27  "txoutset_hash": "d0b6dd9367746570658e9052ff814b864643d8aeccb2ccc81d438123242db63b",
    28  "nchaintx": 1064936785
    29}
    

    As @Sjors mentioned above please consider adding -rpcclienttimeout=0 to the calls also in the documentation. Furthermore, these example outputs could be useful there too.

  108. fjahr commented at 3:19 pm on September 1, 2024: contributor

    As @Sjors mentioned above please consider adding -rpcclienttimeout=0 to the calls also in the documentation. Furthermore, these example outputs could be useful there too.

    Can you say which examples you mean? I added it to the RPC help examples in my last push but I guess you might be referring to the README examples where I didn’t do it yet. Or is there another place I am still missing?

  109. RPC: Extract InvalidateBlock helper 446ce51c21
  110. RPC: Extract ReconsiderBlock helper fccf4f91d2
  111. RPC: Add type parameter to dumptxoutset 993cafe7e4
  112. test: Test for dumptxoutset at specific height 8426850352
  113. contrib: Remove test_utxo_snapshots.sh 20a1c77aa7
  114. assumeutxo: Remove devtools/utxo_snapshot.sh b29c21fc92
  115. doc: Improve assumeutxo guide and add more docs/comments
    Also fixes some outdated information in the remaining design doc.
    e868a6e070
  116. rpc, refactor: Prevent potential race conditions in dumptxoutset
    Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
    94b0adcc37
  117. fjahr force-pushed on Sep 1, 2024
  118. fjahr commented at 7:10 pm on September 1, 2024: contributor

    As @Sjors mentioned above please consider adding -rpcclienttimeout=0 to the calls also in the documentation. Furthermore, these example outputs could be useful there too.

    Can you say which examples you mean? I added it to the RPC help examples in my last push but I guess you might be referring to the README examples where I didn’t do it yeah. Or is there another place I am still missing?

    I just added the timeout to the README example as well and rebased since that was recommend for all PRs anyway in the light of the CMake introduction.

    Should be trivial to re-ack with

    0$ git range-diff master 46c8d75d24d3355ec468f7f608effe94636e16db 94b0adcc371540732453d70309c4083d4bd9cd6b
    
  119. Sjors commented at 6:49 am on September 2, 2024: member

    re-utACK 94b0adcc371540732453d70309c4083d4bd9cd6b

    Just a rebase and documentation improvement.

  120. in src/rpc/blockchain.cpp:2682 in 993cafe7e4 outdated
    2678@@ -2660,6 +2679,14 @@ static RPCHelpMan dumptxoutset()
    2679         "Write the serialized UTXO set to a file.",
    2680         {
    2681             {"path", RPCArg::Type::STR, RPCArg::Optional::NO, "Path to the output file. If relative, will be prefixed by datadir."},
    2682+            {"type", RPCArg::Type::STR, RPCArg::Default(""), "The type of snapshot to create. Can be \"latest\" to create a snapshot of the current UTXO set or \"rollback\" to temporarily roll back the state of the node to a historical block before creating the snapshot of a historical UTXO set. This parameter can be omitted if a separate \"rollback\" named parameter is specified indicating the height or hash of a specific historical block. If \"rollback\" is specified and separate \"rollback\" named parameter is not specified, this will roll back to the latest valid snapshot block that currently be loaded with loadtxoutset."},
    


    mzumsande commented at 10:30 am on September 2, 2024:
    nit: missing “can” (before or after “currently”)
  121. in src/rpc/blockchain.cpp:2774 in 993cafe7e4 outdated
    2770+        // Suspend network activity for the duration of the process when we are
    2771+        // rolling back the chain to get a utxo set from a past height. We do
    2772+        // this so we don't punish peers that send us that send us data that
    2773+        // seems wrong in this temporary state. For example a normal new block
    2774+        // would be classified as a block connecting an invalid block.
    2775+        disable_network = std::make_unique<NetworkDisable>(connman);
    


    mzumsande commented at 11:33 am on September 2, 2024:
    This should be conditional on networkactive being true - I don’t think this should re-enable network activity if it was previously disabled by a user-specified setnetworkactive false, as it currently would.

    pablomartin4btc commented at 5:43 pm on September 2, 2024:
    I do agree unless there’s another use case that invalidates it.
  122. mzumsande commented at 1:31 pm on September 2, 2024: contributor

    ACK 94b0adcc371540732453d70309c4083d4bd9cd6b

    It would be good to fix the networkactive issue below, here or in a follow-up.

  123. pablomartin4btc commented at 6:00 pm on September 2, 2024: member

    As @Sjors mentioned above please consider adding -rpcclienttimeout=0 to the calls also in the documentation. Furthermore, these example outputs could be useful there too.

    Can you say which examples you mean? I added it to the RPC help examples in my last push but I guess you might be referring to the README examples where I didn’t do it yet. Or is there another place I am still missing?

    Thanks! I’ve checked the places where you added the -rpcclienttimeout=0, also it could be added into the loadtxoutset rpc call in the same file (/doc/assumeutxo.md), as it was suggested by @Sjors in the issue he opened to make the rpc async at some point.

    The other thing I was suggesting was to add the outputs (or a part of it) of the commands I ran in my review, just to the user to see what to expect and compare the hash, etc. But it’s up to you ofc.

  124. pablomartin4btc approved
  125. pablomartin4btc commented at 6:01 pm on September 2, 2024: member

    $ git range-diff master 46c8d75d24d3355ec468f7f608effe94636e16db 94b0adcc371540732453d70309c4083d4bd9cd6b

    re-ACK 94b0adcc371540732453d70309c4083d4bd9cd6b

  126. bitcoin deleted a comment on Sep 2, 2024
  127. fjahr commented at 4:42 pm on September 3, 2024: contributor
    There are now 3 ACKs so unless need to retouch I will make a quick follow-up addressing the open comments from @pablomartin4btc and @mzumsande .
  128. achow101 commented at 7:12 pm on September 3, 2024: member
    ACK 94b0adcc371540732453d70309c4083d4bd9cd6b
  129. DrahtBot requested review from achow101 on Sep 3, 2024
  130. achow101 merged this on Sep 3, 2024
  131. achow101 closed this on Sep 3, 2024

  132. fjahr commented at 2:32 pm on September 4, 2024: contributor

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-28 22:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me