test: Add coverage for dumptxoutset failure robustness

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)

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.

DrahtBot added the label Tests on Sep 4, 2024

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

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

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

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

fjahr force-pushed on Sep 5, 2024

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

refactor: Manage dumptxoutset RAII classes with std::optional

Also removes unused UniValue error variable.

c2b779da4e

fjahr force-pushed on Sep 5, 2024

fjahr commented at 8:31 am on September 5, 2024: contributor

You can also remove UniValue error

Done in the refactoring commit, thanks!

DrahtBot added the label CI failed on Sep 5, 2024

pablomartin4btc approved

pablomartin4btc commented at 4:56 pm on September 5, 2024: member

cr & tACK c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7

DrahtBot requested review from tdb3 on Sep 5, 2024

DrahtBot removed the label CI failed on Sep 6, 2024

tdb3 approved

tdb3 commented at 5:33 pm on September 7, 2024: contributor

ACK c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7

BrandonOdiwuor approved

BrandonOdiwuor commented at 2:12 pm on September 8, 2024: contributor

Code Review ACK c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7

theStack approved

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

DrahtBot added the label CI failed on Sep 9, 2024

achow101 commented at 4:48 pm on September 9, 2024: member

ACK c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7

achow101 merged this on Sep 9, 2024

achow101 closed this on Sep 9, 2024

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

achow101 referenced this in commit 87d54500bf on Sep 13, 2024

TheCharlatan referenced this in commit 69282950aa on Sep 16, 2024

TheCharlatan referenced this in commit dfe0cd4ec5 on Sep 16, 2024

Fabcien referenced this in commit d2078ef2ef on May 5, 2025

Fabcien referenced this in commit 7acba5eca4 on May 5, 2025

Fabcien referenced this in commit 0ed1bd0f37 on May 5, 2025

bitcoin locked this on Sep 12, 2025

test: Add coverage for dumptxoutset failure robustness #30817

Code Coverage

Reviews