test: Check already deactivated network stays suspended after dumptxoutset #30892

pull fjahr wants to merge 1 commits into bitcoin:master from fjahr:2024-09-network-inactive changing 1 files +18 −9

fjahr commented at 9:22 PM on September 12, 2024: contributor

Follow-up to #30817 which covered the robustness of dumptxoutset: network is deactivated during the run but re-activated even when an issue was encountered. But it did not cover the case if the user had deactivated the network themselves before. In that case the user may want the network to stay off so the network is not reactivated after dumptxoutset finishes. A test for this behavior is added here.
test: Check that network stays suspended after dumptxoutset if it was off before 72c9a1fe94
DrahtBot commented at 9:22 PM on September 12, 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, theStack, tdb3, 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 12, 2024
in test/functional/rpc_dumptxoutset.py:26 in 72c9a1fe94
```
  18 | @@ -19,6 +19,17 @@ def set_test_params(self):
  19 |          self.setup_clean_chain = True
  20 |          self.num_nodes = 1
  21 |  
  22 | +    def check_expected_network(self, node, active):
  23 | +        rev_file = node.blocks_path / "rev00000.dat"
  24 | +        bogus_file = node.blocks_path / "bogus.dat"
  25 | +        rev_file.rename(bogus_file)
  26 | +        assert_raises_rpc_error(
```
pablomartin4btc commented at 10:34 PM on September 12, 2024:

nit:

self.log.info(f"Test that dumptxoutset with rollback type fails given an invalid height") assert_raises_rpc_error(
fjahr commented at 10:47 PM on September 12, 2024:

Hm, I don't think it makes sense to add that. First, the actual error here is not the point of this test. This test is about the network activity and even if it covers something else I would like that to be structured differently then and have the logs be in run_test. Where it is now it would also be printed twice. Second, the content isn't right: The reason this fails is not the rollback height, it's because the rev file can not be read, so the rolling back action failed as in, it could not reach the requested height. But if the re file wasn't moved above there should be no error raised.

pablomartin4btc commented at 11:23 PM on September 12, 2024:

Yes, that's correct. Thanks! I got confused by the error message, block height was actually valid (COINBASE_MATURITY = 100), then we are missing testing the rollback type given an invalid height (?).

fjahr commented at 9:16 AM on September 13, 2024:

Seems like we don't have a test for that explicitly. It's handled by ParseHashOrHeight() which used in other RPCs and is covered in other tests but still would be good to have it for dumptxoutset. Do you want to open a PR for that @pablomartin4btc ? Otherwise, I can add it here.

pablomartin4btc commented at 9:33 AM on September 13, 2024:

I can open a new PR to test the rollback type option with both a valid and invalid heights. I'll do as soon as this one gets merged. Thanks.
pablomartin4btc commented at 10:37 PM on September 12, 2024: member

ACK 72c9a1fe94f927220d3159f516fda684ae9d4caa
theStack approved
theStack commented at 10:17 AM on September 13, 2024: contributor

utACK 72c9a1fe94f927220d3159f516fda684ae9d4caa
tdb3 approved
tdb3 commented at 6:23 PM on September 13, 2024: contributor

tested ACK 72c9a1fe94f927220d3159f516fda684ae9d4caa

Great addition. Added improper node.setnetworkactive() lines above each call to self.check_expected_network() to ensure the checks would fail. They did (no surprise).
achow101 commented at 8:06 PM on September 13, 2024: member

ACK 72c9a1fe94f927220d3159f516fda684ae9d4caa
achow101 merged this on Sep 13, 2024
achow101 closed this on Sep 13, 2024
TheCharlatan referenced this in commit 69282950aa on Sep 16, 2024
TheCharlatan referenced this in commit dfe0cd4ec5 on Sep 16, 2024
bitcoin locked this on Sep 13, 2025