test: added test coverage to loadtxoutset could not open file #30053

kevkevinpal commented at 9:32 PM on May 6, 2024: contributor

The functional test coverage did not cover the rpc error of "Couldn't open file..." for loadtxoutset and this test adds coverage for it

This adds coverage to this line https://github.com/bitcoin/bitcoin/blob/master/src/rpc/blockchain.cpp#L2777

DrahtBot commented at 9:32 PM on May 6, 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	maflcko, rkrux, alfonsoromanz, tdb3, davidgumberg

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:

#29996 (test: Assumeutxo: import snapshot in a node with a divergent chain by alfonsoromanz)
#29428 (test: Assumeutxo: snapshots with less work should not be loaded by hernanmarino)

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.

DrahtBot added the label Tests on May 6, 2024

test: added test coverage to loadtxoutset

The functional test coverage did not cover the rpc error of Couldn't
open file for loadtxoutset and this test adds coverage for it

ee67bba76c

kevkevinpal force-pushed on May 6, 2024

maflcko commented at 5:18 AM on May 7, 2024: member

ACK ee67bba76cca2355541f99bb731f58479981b29e

in test/functional/feature_assumeutxo.py:158 in ee67bba76c

 151 | @@ -152,6 +152,12 @@ def test_invalid_mempool_state(self, dump_output_path):
 152 |  
 153 |          self.restart_node(2, extra_args=self.extra_args[2])
 154 |  
 155 | +    def test_invalid_file_path(self):
 156 | +        self.log.info("Test bitcoind should fail when file path is invalid.")
 157 | +        node = self.nodes[0]
 158 | +        path = node.datadir_path / node.chain / "invalid" / "path"

rkrux commented at 10:34 AM on May 7, 2024:

Got to learn about the functionality of the slash operator in case of Pathlib.Path! https://docs.python.org/3/library/pathlib.html#operators

rkrux approved

rkrux commented at 10:41 AM on May 7, 2024: contributor

ACK ee67bba

Make successful, unit and functional tests successful. Short and sweet!

alfonsoromanz approved

alfonsoromanz commented at 3:13 PM on May 7, 2024: contributor

ACK ee67bba76cca2355541f99bb731f58479981b29e. Code looks good to me. I also ran test/functional/feature_assumeutxo.py to make sure all tests passes, including this one.

in test/functional/feature_assumeutxo.py:159 in ee67bba76c

 151 | @@ -152,6 +152,12 @@ def test_invalid_mempool_state(self, dump_output_path):
 152 |  
 153 |          self.restart_node(2, extra_args=self.extra_args[2])
 154 |  
 155 | +    def test_invalid_file_path(self):
 156 | +        self.log.info("Test bitcoind should fail when file path is invalid.")
 157 | +        node = self.nodes[0]
 158 | +        path = node.datadir_path / node.chain / "invalid" / "path"
 159 | +        assert_raises_rpc_error(-8, "Couldn't open file {} for reading.".format(path), node.loadtxoutset, path)

tdb3 commented at 2:26 AM on May 8, 2024:

Minor nit: Style guidelines prefer f'{}'. (https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines)

e.g.

diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
index a65a11a9b5..47f87297f3 100755
--- a/test/functional/feature_assumeutxo.py
+++ b/test/functional/feature_assumeutxo.py
@@ -156,7 +156,7 @@ class AssumeutxoTest(BitcoinTestFramework):
         self.log.info("Test bitcoind should fail when file path is invalid.")
         node = self.nodes[0]
         path = node.datadir_path / node.chain / "invalid" / "path"
-        assert_raises_rpc_error(-8, "Couldn't open file {} for reading.".format(path), node.loadtxoutset, path)
+        assert_raises_rpc_error(-8, f"Couldn't open file {path} for reading.", node.loadtxoutset, path)
 
     def run_test(self):
         """

maflcko commented at 7:27 AM on May 8, 2024:

With 5 ACKs this nit does not seem worth it to address at this point. Can do in one of the other pulls that touch this file, in a follow-up?

tdb3 commented at 2:31 AM on May 8, 2024: contributor

ACK for ee67bba76cca2355541f99bb731f58479981b29e

Thank you for increasing test coverage. Ran a sanity check that created the .../invalid/path file from dumptxoutset and the test failed (as expected). Left a minor nit.

davidgumberg commented at 3:03 AM on May 8, 2024: contributor

LGTM ACK https://github.com/bitcoin/bitcoin/pull/30053/commits/ee67bba76cca2355541f99bb731f58479981b29e

This PR improves coverage of the loadtxoutset rpc, I tested by removing the error throw from blockchain.cpp and verifying that the test complains:

 FILE* file{fsbridge::fopen(path, "rb")};
 AutoFile afile{file};
 if (afile.IsNull()) {
-    throw JSONRPCError(
-        RPC_INVALID_PARAMETER,
-        "Couldn't open file " + path.utf8string() + " for reading.");
 }

+1 to @tdb3 style nit above

I thought for a bit about whether or not we should also check files with invalid path names or bad permissions, but I think this check sufficiently demonstrates that if a file passed to loadtxoutset cannot be opened for any reason at all, an error will be thrown.

fanquake merged this on May 8, 2024

fanquake closed this on May 8, 2024

Fabcien referenced this in commit dc73f741fa on Apr 17, 2025

bitcoin locked this on May 8, 2025