test: add missing stop_node calls to feature_coinstatsindex and feature_prune #25034

pull mzumsande wants to merge 1 commits into bitcoin:master from mzumsande:20224_coinstatsindex_fix changing 3 files +4 −0

mzumsande commented at 7:48 PM on April 29, 2022: contributor

In #24789, I forgot to stop the node before using assert_start_raises_init_error in feature_coinstatsindex. This resulted in a bitcoind process that is not being terminated after the test finishes. feature_prune has the same problem and also creates a zombie bitcoind process.

Also adds an assert to assert_start_raises_init_error to make sure the node isn't already running to prevent this sort of mistake in the future.
mzumsande cross-referenced this on Apr 29, 2022 from issue [kernel 2a/n] Split hashing/index `GetUTXOStats` codepaths, decouple from `coinstatsindex` by dongcarl
mzumsande force-pushed on Apr 29, 2022
DrahtBot added the label Tests on Apr 29, 2022

test: stop node before calling assert_start_raises_init_error

...in feature_coinstatsindex and feature_pruning.
Also add an assert to assert_start_raises_init_error that the node is
not already running.

a3cd7dbfd8

mzumsande force-pushed on Apr 29, 2022
mzumsande renamed this:
~~test: add missing stop_node call to feature_coinstatsindex~~
test: add missing stop_node calls to feature_coinstatsindex and feature_prune
on Apr 29, 2022
mzumsande commented at 9:06 PM on April 29, 2022: contributor

An alternative fix would be to have assert_start_raises_init_error stop the node before starting it again (instead of asserting).
brunoerg commented at 9:46 PM on April 29, 2022: contributor

I noticed there are other tests which don't stop the node before assert_start_raises_init_error, should we update all of them?
mzumsande commented at 10:17 PM on April 29, 2022: contributor

I noticed there are other tests which don't stop the node before assert_start_raises_init_error, should we update all of them?

Which ones? Since I added the assert to assert_start_raises_init_error, I would expect these tests to assert now. I'm not sure - on the one hand it might be easier to let assert_start_raises_init_error just stop the node, on the other hand I think it would be clearer if it's visible in the actual test when a node is stopped.

Oh, and thanks @willcl-ark I for finding this bug!
brunoerg commented at 10:44 PM on April 29, 2022: contributor

Which ones? Sorry, my bad! With these changes, I think all the tests are right. I thought it was missing intest_invalid_command_line_options (feature_config_args) but in the end of the previous function it stops the node.

in test/functional/feature_coinstatsindex.py:240 in a3cd7dbfd8

 235 |          self.nodes[1].assert_start_raises_init_error(
 236 |              expected_msg='Error: -reindex-chainstate option is not compatible with -coinstatsindex. '
 237 |              'Please temporarily disable coinstatsindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes.',
 238 |              extra_args=['-coinstatsindex', '-reindex-chainstate'],
 239 |          )
 240 | +        self.restart_node(1, extra_args=["-coinstatsindex"])

MarcoFalke commented at 6:35 AM on April 30, 2022:

I think the default extra args are persisted in node, so no need to pass them here again.

MarcoFalke approved
MarcoFalke commented at 7:25 AM on April 30, 2022: member

LGTM
MarcoFalke merged this on Apr 30, 2022
MarcoFalke closed this on Apr 30, 2022
sidhujag referenced this in commit eacf5a4216 on Apr 30, 2022
bitcoin deleted a comment on May 2, 2022
mzumsande deleted the branch on May 19, 2022
bitcoin locked this on May 19, 2023

Contributors

Labels

Linked (view graph)

#24410 [kernel 2a/n] Split hashing/index `GetUTXOStats` codepaths, decouple from `coinstatsindex`#24789 init, index: disallow indexes when running reindex-chainstate