test: Fix intermittent failure in feature_dbcrash

[maflcko]

Example backtrace https://cirrus-ci.com/task/6005716207009792?command=functional_test#L817

[maflcko]

This was presumably introduced in aab8172

[jnewbery]

utACK fa2ca0c

Perhaps a longer-term solution would be to change the generate() method in TestNode:

• set an hd seed on the node instead of a private key in import_deterministic_coinbase_privkeys()
• use generatetodescriptor to generate to a key in the hd tree
• keep a static variable in generate() to ensure a new keypath is used for each call.

Could be a good first issue?

[maflcko]

keep a static variable in generate() to ensure a new keypath is used for each call.

You mean a member variable? (Different nodes shouldn't influence each other)

[jnewbery]

You mean a member variable? (Different nodes shouldn't influence each other)

You're right. Should be a member variable.

[glowang]

utACK fa2ca0c

Perhaps a longer-term solution would be to change the generate() method in TestNode:

• set an hd seed on the node instead of a private key in import_deterministic_coinbase_privkeys()
• use generatetodescriptor to generate to a key in the hd tree
• keep a static variable in generate() to ensure a new keypath is used for each call.

Could be a good first issue?

I can work on this!

[maflcko]

Nice! I see that this method is lacking a comment to explain its motivation. The background the tests need deterministic keys is:

• When the wallet is not compiled into Bitcoin Core, the tests should still be able to generate blocks. Blocks need to be generated to an address (or descriptor), so we need some information to derive those
• When the wallet is compiled into Bitcoin Core (and mock-time is used), we want blocks to be generated deterministically in some tests. See for example test/functional/data/rpc_getblockstats.json
• Finally, a minor side-effect, is that we don't need to cache the wallets directory in the functional test cache.

Just make sure to preserve those use-cases.

Let me know if you need any other pointers.

[glowang]

Thank you so much @MarcoFalke and @jnewbery !

1.You mentioned that Blocks need to be generated to an address (or descriptor), so we need some information to derive those. I understand that you want to generate blocks to an address/descriptor, but what woud happen if the keys are not deterministic anyways? What benefits do we get from having information to derive them? Sorry if this is super obvious.

2.I did a little research, but I'm not sure if I'm getting what you meant by Do not cache the wallets directory in functional test cache. Looks like this behavior has been enforced in test_framework.py, os.rmdir(cache_path('wallets')). Is this what you mean? Is this the exact behavior we want to maintain?

3.Additionally, I would like to make sure I understand the context & goal of this task:

Background: feature_dbcrash test experienced intermittent failures due to the fact that generate() uses a deterministic address. Deterministic addresses don't work when a block is invalidated uniformly randomly (but why does invalid block not work well with deterministic addresses? Why does using a new address solve this problem?). This has been worked around by calling the getnewaddress() function.

Goal: we want to rewrite the generate() function to this logic:

• If the wallet is compiled && mocktime is used:
  • Blocks should be generated deterministically, like rpc_getblockstats.json
• If the wallet is not compiled:
  • Blocks are generated to an address or descriptor:
    - Set an hd seed on the node instead of using get_deterministic_priv_key().address
    - Use generatetodesriptor to generate keys from the seed
    - Use a ember variable in generate() to ensure a new keypath is used

[maflcko]

Why does using a new address solve this problem?

If you call

• generate
• invalidateblock(best_block)
• generate

And all of this happens in the same second, then the two generated blocks will be completely identical in all fields of their struct. Thus, the second call to generate will fail because the generated block is already marked invalid.

[maflcko]

Looks like this behavior has been enforced in test_framework.py, os.rmdir(cache_path('wallets')). Is this what you mean? Is this the exact behavior we want to maintain?

Yes, I'd say so. It allows to simply cache the blockchain once instead of MAX_NODES datadirs (including the wallet dirs)

And about your other questions: In general, the more deterministic a test is, the easier it is to reproduce failures.

[glowang]

thank you for your answers!! I have some follow up questions 😂

In your answer, there seems to be an emphasis of using mock time when the wallet is enabled, but not so important if we are only generating to address...is there a reason for that? It seems that we set mocktime when we expect there is some time consuming processes (such as tx rebroadcast) & that we use mocktime to time travel.

The feature_crash.py test is using generatetoaddress for mining after this pr, so right now it is independent of wallet. Does this mean that this test scenario does not require a wallet to be enabled? Is there any harm if I pass in the -disablewallet param?

In the case that wallet is enabled & that we want to generate blocks deterministically, is it okay if we reuse the block data in rpc_getblockstats.json?

In the case that wallet is not enable, do you have any preference as to whether we should use generatetodescriptor or generatetoaddress ? @jnewbery asked specifically to use generatetodescriptor in his previous comment, but @MarcoFalke seemed to suggest that either is okay?

Thank you again 🙏

[maflcko]

Don't worry too much about mocktime. This is just another dimension to achieve more determinism. It should be orthogonal to your changes. rpc_getblockstats.json was just an example. It shouldn't affect your changes.

Sometimes mocktime is used to "speed up" tests by telling them how much time has passed. In other cases, it is used to deterministically generate blocks. (The time ends up in the block header, see https://btcinformation.org/en/developer-reference#block-headers)

Is there any harm if I pass in the -disablewallet param?

Currently the test does require a wallet. However, it might be possible to remove that requirement.

bitcoin/test/functional/feature_dbcrash.py

Line 73 in fa2ca0c

self.skip_if_no_wallet()

[glowang]

I feel like maybe I am spinning my wheel for a bit...

If you got sometime to take a look and let me know if I am heading in the right direction, I would really appreciate it! #19297