test: Add coverage for dumptxoutset failure robustness #30817

1. 
   fjahr commented at 6:15 pm on September 4, 2024: contributor

   This adds a test that checks that network activity is not suspended if dumptxoutset fails in the middle of its process which is implemented with the NetworkDisable RAII class. I would have liked to add coverage for the TemporaryRollback RAII class but that seems a lot more tricky since the failure needs to happen at some point after the rollback and on the scale of our test chain here I couldn’t find a way to do it yet. This was requested by pablomartin4btc here: #30808#pullrequestreview-2280450117. To test the test you can comment out the content of the destructor of NetworkDisable.

   It also addresses the feedback by ryanofsky to use std::optional instead of std::unique_ptr for the management of the RAII object: #30808 (review)

2. 
   DrahtBot commented at 6:15 pm on September 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.

   Type	Reviewers
   ACK	pablomartin4btc, tdb3, BrandonOdiwuor, theStack, achow101

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. DrahtBot added the label Tests on Sep 4, 2024
4. in test/functional/rpc_dumptxoutset.py:67 in 637b43d9ed outdated

   61@@ -60,6 +62,16 @@ def run_test(self):
   62         assert_raises_rpc_error(
   63             -8, 'Invalid snapshot type "bogus" specified. Please specify "rollback" or "latest"', node.dumptxoutset, 'utxos.dat', "bogus")
   64 
   65+        self.log.info(f"Test that dumptxoutset failure does not leave the network activity suspended")
   66+        rev_file = node.chain_path / "blocks" / "rev00000.dat"
   67+        bogus_file = node.chain_path / "blocks" / "bogus.dat"
   

   ---

   ---

   

   tdb3 commented at 1:02 am on September 5, 2024:

   Could use node.blocks_path instead of node.chain_path for these two lines.

   ---

   

   fjahr commented at 8:27 am on September 5, 2024:

   done
5. 
   tdb3 commented at 1:20 am on September 5, 2024: contributor

   Approach ACK

   Nice addition. Definitely don’t want network broken from a failed RPC. Keeping the PR scoped to just NetworkDisable seems good (more concise to reviewers).

6. in test/functional/rpc_dumptxoutset.py:68 in 637b43d9ed outdated

   61@@ -60,6 +62,16 @@ def run_test(self):
   62         assert_raises_rpc_error(
   63             -8, 'Invalid snapshot type "bogus" specified. Please specify "rollback" or "latest"', node.dumptxoutset, 'utxos.dat', "bogus")
   64 
   65+        self.log.info(f"Test that dumptxoutset failure does not leave the network activity suspended")
   66+        rev_file = node.chain_path / "blocks" / "rev00000.dat"
   67+        bogus_file = node.chain_path / "blocks" / "bogus.dat"
   68+        os.rename(rev_file, bogus_file)
   

   ---

   ---

   

   maflcko commented at 4:44 am on September 5, 2024:

   style nit: Pathlib should have the corresponding call as a member method. Using that would avoid the os import.

   ---

   

   fjahr commented at 8:27 am on September 5, 2024:

   done
7. 
   maflcko commented at 7:29 am on September 5, 2024: member

   You can also remove UniValue error, now that it is no longer used:

    0diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
    1index 3ba89d08d9..b88e9103c1 100644
    2--- a/src/rpc/blockchain.cpp
    3+++ b/src/rpc/blockchain.cpp
    4@@ -2822,8 +2822,6 @@ static RPCHelpMan dumptxoutset()
    5     Chainstate* chainstate;
    6     std::unique_ptr<CCoinsViewCursor> cursor;
    7     CCoinsStats stats;
    8-    UniValue result;
    9-    UniValue error;
   10     {
   11         // Lock the chainstate before calling PrepareUtxoSnapshot, to be able
   12         // to get a UTXO database cursor while the chain is pointing at the
   13@@ -2847,6 +2845,7 @@ static RPCHelpMan dumptxoutset()
   14         }
   15     }
   16 
   17+    UniValue
   18     result = WriteUTXOSnapshot(*chainstate, cursor.get(), &stats, tip, afile, path, temppath, node.rpc_interruption_point);
   19     fs::rename(temppath, path);
   20 
   

8. fjahr force-pushed on Sep 5, 2024
9. test: Add coverage for failing dumptxoutset behavior

   In case of a failure to create the dump, the node should not be left in an inconsistent state like deactivated network activity or an invalidated blockchain.

   4b5bf335ad
10. refactor: Manage dumptxoutset RAII classes with std::optional

    Also removes unused UniValue error variable.

    c2b779da4e
11. fjahr force-pushed on Sep 5, 2024
12. 
    fjahr commented at 8:31 am on September 5, 2024: contributor

    You can also remove UniValue error

    Done in the refactoring commit, thanks!

13. DrahtBot added the label CI failed on Sep 5, 2024
14. pablomartin4btc approved
15. 
    pablomartin4btc commented at 4:56 pm on September 5, 2024: member

    cr & tACK c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7
16. DrahtBot requested review from tdb3 on Sep 5, 2024
17. DrahtBot removed the label CI failed on Sep 6, 2024
18. tdb3 approved
19. 
    tdb3 commented at 5:33 pm on September 7, 2024: contributor

    ACK c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7
20. BrandonOdiwuor approved
21. 
    BrandonOdiwuor commented at 2:12 pm on September 8, 2024: contributor

    Code Review ACK c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7
22. theStack approved
23. 
    theStack commented at 10:14 pm on September 8, 2024: contributor

    ACK c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7

    Further potential follow-up material (maybe a “good first issue” candidate): could go one step further and even test that if the network was already disabled (via setnetworkactive False), it won’t be enabled after a dumptxoutset call. Right now the following patch passes all tests:

     0diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
     1index b94b3a64e1..b1267bf808 100644
     2--- a/src/rpc/blockchain.cpp
     3+++ b/src/rpc/blockchain.cpp
     4@@ -2812,9 +2812,7 @@ static RPCHelpMan dumptxoutset()
     5         // Skip if the network is already disabled because this
     6         // automatically re-enables the network activity at the end of the
     7         // process which may not be what the user wants.
     8-        if (connman.GetNetworkActive()) {
     9-            disable_network.emplace(connman);
    10-        }
    11+        disable_network.emplace(connman);
    12 
    13         invalidate_index = WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Next(target_index));
    14         temporary_rollback.emplace(*node.chainman, *invalidate_index);
    

24. DrahtBot added the label CI failed on Sep 9, 2024
25. 
    achow101 commented at 4:48 pm on September 9, 2024: member

    ACK c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7
26. achow101 merged this on Sep 9, 2024
27. achow101 closed this on Sep 9, 2024

    ---

28. 
    fjahr commented at 9:23 pm on September 12, 2024: contributor

    could go one step further and even test that if the network was already disabled (via setnetworkactive False), it won’t be enabled after a dumptxoutset call.

    Added in #30892

Contributors
fjahr DrahtBot tdb3 maflcko pablomartin4btc BrandonOdiwuor theStack achow101

Labels
Tests CI failed