Page MenuHomePhabricator

Add a --descriptors option to various tests
ClosedPublic

Authored by PiRK on Sep 29 2021, 13:57.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCe854278f6db0: Add a --descriptors option to various tests
Summary

Adds a --descriptors option globally to the test framework. This will
make the test create and use descriptor wallets. However some tests may
not work with this.

Some tests are modified to work with --descriptors and run with that
option in test_runer:

  • wallet_encryption.py
  • wallet_keypool.py
  • wallet_keypool_topup.py
  • wallet_labels.py
  • wallet_avoidreuse.py

This is a backport of core#16528 [43a/43]
https://github.com/bitcoin/bitcoin/pull/16528/commits/223588b1bbc63dc57098bbd0baa48635e0cc0b82

I'm splitting this commit into multiple commits to make review easier.
This first part contains everything that works more or less out of the
box and does not require significant changes to work with our codebase.

All the deviations from the original PR in this revision are explained by out of order
backports and lack of segwit and bech32. For instance, changes to key.py
were already included in D8475 & D9934. Deviations in RPCOverloadWrapper.createwallet
are explained by D10185 and D9101.

I excluded specifically 3 tests that require changes to transaction
amounts and fees when descriptors are used. With our codebase,
descriptor wallets generate different fees, because in some situations
the legacy wallet generates legacy p2pkh outputs and descriptor wallets generate
scriptPubKey / p2sh outputs. I will address these in one or two
separate diffs.

These excluded tests are:

  • rpc_psbt.py
  • wallet_basic.py
  • wallet_keypool.py
Test Plan

ninja check-functional

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Sep 29 2021, 13:57
Fabien requested changes to this revision.Oct 4 2021, 08:00
Fabien added a subscriber: Fabien.
Fabien added inline comments.
test/functional/rpc_createmultisig.py
79 ↗(On Diff #30203)

The PR uses node0. Note that this should not impact the behavior because the test is skipped if there is no wallet, but better stick to the original material

84 ↗(On Diff #30203)

Not from this diff, but the comment is wrong

87 ↗(On Diff #30203)

And the test does nothing

test/functional/wallet_hd.py
100 ↗(On Diff #30203)

Please move the os.path.join and comment change above to their own diff (the comment is from PR16681, I did not search for the os.path.join)

This revision now requires changes to proceed.Oct 4 2021, 08:01

address review:

  • use node0 when creating multisig address in rpc_createmultisig (like in the original PR)
  • remove test related to segwit, that in our codebase just duplicates the previous line of test (needlesly added in D6552)
  • undo a couple of changes that don't belong in this backport ("regtest" -> "chain" in a comment and style change)
This revision is now accepted and ready to land.Oct 5 2021, 08:17