[test] WIP: rewrite generate() in test_node to gain determinism in test data #19297

glowang commented at 3:36 PM on June 16, 2020: contributor

This improvement was proposed by jnewbery in #19022

We want to rewrite generate() function by using an hd seed to create addresses. This allows the test to have unique but deterministic addresses to which blocks can be generated.

TODO: Investigate why certain created keypaths do not increase incrementally/consistently:

currently, I am using a counter keypathnonce to assert every key path generated from my HD seed is unique and increment by one (maybe this was the wrong assumption to start with?). Interestingly, when I ran tests, it seems that one particular key path,m/0'/0'/105 and m/0'/0'/106 , are always skipped. Whereas the rest of my hdkeypath increase consistently by one:

"m/0'/0'/0'
...
"m/0'/0'/101'
"m/0'/0'/102'
"m/0'/0'/103'
"m/0'/0'/104'
(skipping 105 and 106)
"m/0'/0'/107'

My assertion error

2020-06-16T15:29:57.583000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/Users/gwang/Documents/bitcoin/test/functional/test_framework/test_framework.py", line 116, in main
    self.run_test()
  File "test/functional/feature_dbcrash.py", line 223, in run_test
    utxo_list = create_confirmed_utxos(self.nodes[3].getnetworkinfo()['relayfee'], self.nodes[3], 5000)
  File "/Users/gwang/Documents/bitcoin/test/functional/test_framework/util.py", line 547, in create_confirmed_utxos
    node.generate(1)
  File "/Users/gwang/Documents/bitcoin/test/functional/test_framework/test_node.py", line 307, in generate
    assert_equal(hd_info["hdkeypath"], "m/0'/0'/"+str(self.hdkeypathnonce)+"'")
  File "/Users/gwang/Documents/bitcoin/test/functional/test_framework/util.py", line 46, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(m/0'/0'/107' == m/0'/0'/105')

fanquake added the label Tests on Jun 16, 2020

fanquake requested review from jnewbery on Jun 16, 2020

MarcoFalke commented at 4:00 PM on June 16, 2020: member

You may find the issue if you inspect the log generated by the --tracerpc -l DEBUG options

in test/functional/test_framework/test_node.py:299 in 5914d8cdb6 outdated

 294 | @@ -293,8 +295,18 @@ def wait_for_cookie_credentials(self):
 295 |          self._raise_assertion_error("Unable to retrieve cookie credentials after {}s".format(self.rpc_timeout))
 296 |  
 297 |      def generate(self, nblocks, maxtries=1000000):
 298 | -        self.log.debug("TestNode.generate() dispatches `generate` call to `generatetoaddress`")
 299 | -        return self.generatetoaddress(nblocks=nblocks, address=self.get_deterministic_priv_key().address, maxtries=maxtries)
 300 | +        # if wallet is not compiled, blocks are generated to an address or descriptor using an HD seed
 301 | +        hd_address = self.getnewaddress()

jnewbery commented at 5:36 PM on June 16, 2020:

getnewaddress is a wallet RPC. If you want this to work when the wallet isn't compiled, you'll need to generate the HD keys in the python test framework.

glowang commented at 3:31 PM on June 17, 2020:

Thanks John! Would you mind giving an example of where this is done currently? I looked around in the test_framework and just the functional test dir in general, but I couldn't find any example of generating hd keys without wallet. Or maybe this is a function that doesn't exist in the functional test dir yet and we can write it?

MarcoFalke commented at 4:43 PM on June 17, 2020:

The RPC has non-wallet descriptor RPCs: https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md#support-for-output-descriptors-in-bitcoin-core

If I had to guess, you are probably looking for deriveaddresses

jnewbery commented at 6:37 PM on June 18, 2020:

I was thinking that the keys would be generated in the python test framework. I just assumed that we'd have some HD key generation code to test BIP32, but it appears I was wrong.

Marco's suggestion to use the deriveaddress seems ok, but I don't think you even need to do that. I think you can just pass the descriptor with keypath in to generatetodescriptor

in test/functional/test_framework/test_node.py:304 in 5914d8cdb6 outdated

 301 | +        hd_address = self.getnewaddress()
 302 | +        hd_info = self.getaddressinfo(hd_address)
 303 | +        if self.getwalletinfo()["descriptors"]:
 304 | +            #TODO: figure out how to make sure the keypath does increase & is unique
 305 | +            assert_equal(hd_info["hdkeypath"], "m/84'/1'/0'/0/" + str(self.hdkeypathnonce))
 306 | +            self.hdkeypathnonce += 1

glowang commented at 3:18 PM on June 17, 2020:

I added hdkeypathnonce here to make sure a new key path is used. But I realized that incrementing the counter variable like this won't work, because many test files would call getnewaddress() outside of generate(). These new keys generated out of band also belong to the family of our HD tree, but our counter variable will not be aware of this. (like feature_segwit.py).

Hence, although we want to make sure each time a new key path is used, it doesn't make sense to use member variable like this. Maybe the best way to check is to compare the new keypath with the existing keypool? Or do you have other suggestions? @jnewbery

glowang commented at 6:07 PM on June 17, 2020: contributor

You may find the issue if you inspect the log generated by the --tracerpc -l DEBUG options

Thanks Marco, really helpful!

glowang force-pushed on Jun 28, 2020

in test/functional/test_framework/test_node.py:308 in 799ac270ca outdated

 306 | +            keypath = "/44'/0'/0'/0/" + str(self.hdkeypathnonce)
 307 | +            desc = "pkh([" + finger_print + keypath + "]" + xpub+ "/1)" 
 308 | +            self.hdkeypathnonce += 1
 309 | +            return self.generatetodescriptor(nblocks, desc, maxtries)
 310 | +        else:
 311 | +            hd_address = self.getnewaddress()

glowang commented at 7:42 PM on July 4, 2020:

Once I generate to a new address, many tests that check whether the wallet addr number matches up start failing. I think this is because the getnewaddress is messing up the address count for some tests. But at the same time, we also don't want to generate to deterministic addresses either... I'm not sure what direction I should take here...

WIP rough outline for rewriting the generate function.

TODO: investigate why created keypaths do not increase incrementally/consistently

d33934120e

WIP 41ce9805eb

glowang force-pushed on Aug 17, 2020

Preset HD seed for testnode if wallet is compiled 671ae41c43

glowang force-pushed on Aug 17, 2020

WIP de1d6c83cf

DrahtBot commented at 10:39 PM on August 20, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20034 (test: Get rid of default wallet hacks by ryanofsky)
#19168 (Refactor: Improve setup_clean_chain semantics by fjahr)

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.

glowang closed this on Dec 17, 2020

DrahtBot locked this on Feb 15, 2022