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

 246 | @@ -247,6 +247,14 @@ def setup_nodes(self):
 247 |          self.add_nodes(self.num_nodes, extra_args)
 248 |          self.start_nodes()
 249 |  
 250 | +    def import_deterministic_coinbase_privkeys(self):
 251 | +        for n in self.nodes:
 252 | +            wallet_enabled = 'Wallet' in [line[3:-3] for line in n.help().splitlines() if line.startswith('==')]

jnewbery commented at 3:46 PM on September 10, 2018:

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.

 246 | @@ -247,6 +247,14 @@ def setup_nodes(self):
 247 |          self.add_nodes(self.num_nodes, extra_args)
 248 |          self.start_nodes()
 249 |  
 250 | +    def import_deterministic_coinbase_privkeys(self):

jnewbery commented at 5:41 PM on September 10, 2018:

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?

MarcoFalke commented at 8:54 PM on September 10, 2018:

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

  96 | @@ -97,6 +97,22 @@ def __init__(self, i, datadir, *, rpchost, timewait, bitcoind, bitcoin_cli, mock
  97 |  
  98 |          self.p2ps = []
  99 |  
 100 | +    def get_deterministic_priv_key(self):
 101 | +        """Return a deterministic priv key in base58, that only depends on the node's index"""
 102 | +        PRIV_KEYS = [
 103 | +            # adress , privkey

practicalswift commented at 7:37 AM on September 11, 2018:

codespell:

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

MarcoFalke commented at 1:23 PM on September 13, 2018:

Fixed typo

  17 | @@ -18,6 +18,11 @@ class ReceivedByTest(BitcoinTestFramework):
  18 |      def set_test_params(self):
  19 |          self.num_nodes = 2
  20 |  
  21 | +    def import_deterministic_coinbase_privkeys(self):
  22 | +        assert_equal(0, len(self.nodes[1].listreceivedbyaddress(minconf=0, include_empty=True, include_watchonly=True)))
  23 | +        super().import_deterministic_coinbase_privkeys()
  24 | +        self.num_cb_reward_addresses = len(self.nodes[1].listreceivedbyaddress(minconf=0, include_empty=True, include_watchonly=True))

ryanofsky commented at 8:11 PM on September 12, 2018:

Maybe substitute cb_reward with just coinbase to be more consistent.

  33 | -                # key = key_label.split(" ")[0]
  34 | -                keytype = key_label.split(" ")[2]
  35 | -                if len(comment) > 1:
  36 | -                    addr_keypath = comment.split(" addr=")[1]
  37 | -                    addr = addr_keypath.split(" ")[0]
  38 | +                key_date_label, comment = line.split("#")

ryanofsky commented at 8:22 PM on September 12, 2018:

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?

MarcoFalke commented at 1:24 PM on September 13, 2018:

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

 167 | @@ -168,6 +168,11 @@ def transact_and_mine(self, numblocks, mining_node):
 168 |                      newmem.append(utx)
 169 |              self.memutxo = newmem
 170 |  
 171 | +    def import_deterministic_coinbase_privkeys(self):
 172 | +        self.start_nodes()

ryanofsky commented at 8:27 PM on September 12, 2018:

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.

  39 | @@ -40,6 +40,13 @@ def set_test_params(self):
  40 |      def setup_nodes(self):
  41 |          skip_if_no_py3_zmq()
  42 |          skip_if_no_bitcoind_zmq(self)
  43 | +
  44 | +        # Import keys

ryanofsky commented at 8:30 PM on September 12, 2018:

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.

MarcoFalke commented at 1:24 PM on September 13, 2018:

Removed the unneeded import_deterministic_coinbase_privkeys

 115 | @@ -116,10 +116,19 @@ def setup_network(self):
 116 |                  extra_args[i] += ["-prune=1"]
 117 |  
 118 |          self.add_nodes(self.num_nodes, extra_args=extra_args)
 119 | +
 120 | +        # Import keys

ryanofsky commented at 8:35 PM on September 12, 2018:

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?

MarcoFalke commented at 1:23 PM on September 13, 2018:

Added a comment