qa: Fixups to "Run all tests even if wallet is not compiled" #14179

maflcko · 2018-09-09T15:33:25Z

Currently the functional tests require the wallet module to be compiled into the Bitcoin Core executable. For example the premine (or datadir cache) to speed up tests when run in parallel would mine a bunch of blocks and store the private keys to sign the coinbase tx outputs in a wallet. There is no need to have the overhead of the whole wallet module by using keys that are deterministic for all runs.

Note that this change most likely requires the ./test/cache/ to be cleared.

Sjors · 2018-09-10T08:27:22Z

tACK fae497d on macOS 10.13.6

jnewbery

Concept ACK.

A couple of suggested changes inline. There are also a bunch of unrelated style changes in your commit. Can you remove those or split them into their own commit?

jnewbery · 2018-09-10T15:46:46Z

test/functional/test_framework/test_framework.py

@@ -247,6 +247,14 @@ def setup_nodes(self):
        self.add_nodes(self.num_nodes, extra_args)
        self.start_nodes()

+    def import_deterministic_coinbase_privkeys(self):
+        for n in self.nodes:
+            wallet_enabled = 'Wallet' in [line[3:-3] for line in n.help().splitlines() if line.startswith('==')]


This seems a little brittle (based on the exact text output of the help text).

Calling getwalletinfo and catching the returned error is an alternative way to do this.

jnewbery · 2018-09-10T17:41:09Z

test/functional/test_framework/test_framework.py

@@ -247,6 +247,14 @@ def setup_nodes(self):
        self.add_nodes(self.num_nodes, extra_args)
        self.start_nodes()

+    def import_deterministic_coinbase_privkeys(self):


Can you add a call to import_deterministic_coinbase_privkeys() to setup_nodes()? Would that reduce the number of tests that you need to change in this commit?

Good idea to call this by default... This is now always called before run_test, unless self.setup_clean_chain is set.

practicalswift · 2018-09-11T07:37:06Z

test/functional/test_framework/test_node.py

+    def get_deterministic_priv_key(self):
+        """Return a deterministic priv key in base58, that only depends on the node's index"""
+        PRIV_KEYS = [
+            # adress , privkey


codespell:

test/functional/test_framework/test_node.py:103: adress ==> address

ryanofsky · 2018-09-12T20:11:00Z

test/functional/wallet_listreceivedby.py

+    def import_deterministic_coinbase_privkeys(self):
+        assert_equal(0, len(self.nodes[1].listreceivedbyaddress(minconf=0, include_empty=True, include_watchonly=True)))
+        super().import_deterministic_coinbase_privkeys()
+        self.num_cb_reward_addresses = len(self.nodes[1].listreceivedbyaddress(minconf=0, include_empty=True, include_watchonly=True))


Maybe substitute cb_reward with just coinbase to be more consistent.

ryanofsky · 2018-09-12T20:22:41Z

test/functional/wallet_dump.py

-                if len(comment) > 1:
-                    addr_keypath = comment.split(" addr=")[1]
-                    addr = addr_keypath.split(" ")[0]
+                key_date_label, comment = line.split("#")


Previous code and new changes here are pretty inscrutable. Can you explain at a high level what is changing here and why? Can you perhaps add comments to the code to make it clearer, for example to explain why new code skips lines containing 1970?

Added a comment. Note that it is easier to see that nothing changed with --ignore-all-space

ryanofsky · 2018-09-12T20:27:32Z

test/functional/feature_fee_estimation.py

@@ -168,6 +168,11 @@ def transact_and_mine(self, numblocks, mining_node):
                    newmem.append(utx)
            self.memutxo = newmem

+    def import_deterministic_coinbase_privkeys(self):
+        self.start_nodes()


Couldn't the test framework implementation of import_deterministic_coinbase_privkeys be smart enough to know whether nodes are running or not with node.is_node_stopped (or other state)? It seems unfortunate that an individual test would need to override the import_deterministic_coinbase_privkeys method to get such basic functionality.

ryanofsky · 2018-09-12T20:30:27Z

test/functional/interface_zmq.py

@@ -40,6 +40,13 @@ def set_test_params(self):
    def setup_nodes(self):
        skip_if_no_py3_zmq()
        skip_if_no_bitcoind_zmq(self)
+
+        # Import keys


Why doesn't the default framework import_deterministic_coinbase_privkeys call work in this test? There should be a comment here to explain why this override is needed if the default behavior can't be fixed.

Removed the unneeded import_deterministic_coinbase_privkeys

ryanofsky · 2018-09-12T20:35:32Z

test/functional/wallet_import_rescan.py

@@ -116,10 +116,19 @@ def setup_network(self):
                extra_args[i] += ["-prune=1"]

        self.add_nodes(self.num_nodes, extra_args=extra_args)
+
+        # Import keys


Can you add comment explaining need for this override? The stop_nodes immediately followed by start_nodes below seems very odd. And given that this is a wallet test, maybe it would be simpler to just keep the old behavior and just use the wallet instead of generating to hardcoded addresses?

Added a comment

…add comments

ryanofsky

utACK fa8433e, thanks for cleanup!

…iled" fa8433e qa: Remove unneded import_deterministic_coinbase_privkeys overwrite, add comments (MarcoFalke) e413c2d qa: Fix codespell error and have lint-spelling error instead of warn (MarcoFalke) Pull request description: Currently the functional tests require the wallet module to be compiled into the Bitcoin Core executable. For example the premine (or datadir cache) to speed up tests when run in parallel would mine a bunch of blocks and store the private keys to sign the coinbase tx outputs in a wallet. There is no need to have the overhead of the whole wallet module by using keys that are deterministic for all runs. Note that this change most likely requires the `./test/cache/` to be cleared. Tree-SHA512: 9ce26036b0e10f0f888f66a1e50be6a357343f9ffb302ae24a7bb3df2f083a31702ef308b738a03b08a1b623aeddac5d6563dc1b15078c0357b7dafad7808ec3

c32cf6a Add ignored word: mut (practicalswift) 4ae50da Revert "qa: Fix codespell error and have lint-spelling error instead of warn" (practicalswift) Pull request description: Revert `codespell` policy change introduced in #14179. Context: #13954 (comment) Tree-SHA512: 4606b19bb32cdd661f90b3778759818d3493e5ed1a4a2f95982f07eeb6b9c889bc8d53cde31706e0a3b9524c3d3a7378f1b769a60baeb0d00da4c68fd3068114

c32cf6a Add ignored word: mut (practicalswift) 4ae50da Revert "qa: Fix codespell error and have lint-spelling error instead of warn" (practicalswift) Pull request description: Revert `codespell` policy change introduced in bitcoin#14179. Context: bitcoin#13954 (comment) Tree-SHA512: 4606b19bb32cdd661f90b3778759818d3493e5ed1a4a2f95982f07eeb6b9c889bc8d53cde31706e0a3b9524c3d3a7378f1b769a60baeb0d00da4c68fd3068114

…ot compiled" fa8433e qa: Remove unneded import_deterministic_coinbase_privkeys overwrite, add comments (MarcoFalke) e413c2d qa: Fix codespell error and have lint-spelling error instead of warn (MarcoFalke) Pull request description: Currently the functional tests require the wallet module to be compiled into the Bitcoin Core executable. For example the premine (or datadir cache) to speed up tests when run in parallel would mine a bunch of blocks and store the private keys to sign the coinbase tx outputs in a wallet. There is no need to have the overhead of the whole wallet module by using keys that are deterministic for all runs. Note that this change most likely requires the `./test/cache/` to be cleared. Tree-SHA512: 9ce26036b0e10f0f888f66a1e50be6a357343f9ffb302ae24a7bb3df2f083a31702ef308b738a03b08a1b623aeddac5d6563dc1b15078c0357b7dafad7808ec3

maflcko added the Tests label Sep 9, 2018

maflcko force-pushed the Mf1809-qaPremine branch 2 times, most recently from 666627b to fae497d Compare September 9, 2018 16:46

lucash-dev mentioned this pull request Sep 10, 2018

qa: Run all tests even if wallet is not compiled #14180

Merged

jnewbery reviewed Sep 10, 2018

View reviewed changes

maflcko force-pushed the Mf1809-qaPremine branch 2 times, most recently from fa2f2bf to faa669c Compare September 10, 2018 21:51

practicalswift reviewed Sep 11, 2018

View reviewed changes

ryanofsky reviewed Sep 12, 2018

View reviewed changes

qa: Fix codespell error and have lint-spelling error instead of warn

e413c2d

maflcko changed the title ~~qa: Premine to deterministic address with -disablewallet~~ qa: Fixups to "Run all tests even if wallet is not compiled" Sep 13, 2018

qa: Remove unneded import_deterministic_coinbase_privkeys overwrite, …

fa8433e

…add comments

maflcko force-pushed the Mf1809-qaPremine branch from fa513ea to fa8433e Compare September 13, 2018 13:22

ryanofsky reviewed Sep 13, 2018

View reviewed changes

maflcko merged commit fa8433e into bitcoin:master Sep 13, 2018

maflcko deleted the Mf1809-qaPremine branch September 13, 2018 21:49

Bushstar mentioned this pull request Oct 11, 2018

Updates from bitcoin/master FeatherCoin/Feathercoin#366

Merged

laanwj mentioned this pull request Oct 16, 2018

Warn (don't fail!) on spelling errors. Fix typos reported by codespell. #13954

Merged

practicalswift mentioned this pull request Oct 16, 2018

build: Warn (don't fail!) on spelling errors #14495

Merged

jnewbery mentioned this pull request Feb 26, 2019

test: Add .style.yapf #15479

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

qa: Fixups to "Run all tests even if wallet is not compiled" #14179

qa: Fixups to "Run all tests even if wallet is not compiled" #14179

Uh oh!

Conversation

maflcko commented Sep 9, 2018

Uh oh!

Sjors commented Sep 10, 2018

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!