test: ExtendedPrivateKey follow-ups #35619

rkrux commented at 2:35 PM on June 29, 2026: contributor

Looks like a case was missed in PR #35543 that introduced the utility xprv and xpub classes.

Also, unify how to avoid the creation of the default wallet in nodes.

DrahtBot added the label Tests on Jun 29, 2026

DrahtBot commented at 2:35 PM on June 29, 2026: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35619.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	davidgumberg

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35377 (wallet: Allow importing of descriptors without private keys when the wallet has the private keys by achow101)

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.

maflcko commented at 2:51 PM on June 29, 2026: member

I don't understand this pull request. You claim that it is dead code, but don't give a rationale for it.

I read it as an alias for self.wallet_names = []. However, this test doesn't set self.wallet_names, so this code isn't dead?

rkrux commented at 2:55 PM on June 29, 2026: contributor

I read it as an alias for self.wallet_names = []

Why?

rkrux commented at 4:44 PM on June 29, 2026: contributor

so this code isn't dead?

It appears so! This is a method that's overriding the init_wallet method of the test framework to not create the default wallet that can either be named self.wallet_names[node] or self.default_wallet_name. This is the first time I'm seeing this overriden method in the tests as it's used rarely (there's only one other occurrence of this pattern).

Let me see what I can do about this PR now because I got confused here.

rkrux force-pushed on Jun 29, 2026

rkrux renamed this:
~~test: remove dead code from wallet_taproot.py~~
test: add comment in wallet_taproot.py
on Jun 29, 2026

rkrux commented at 5:02 PM on June 29, 2026: contributor

I've updated the PR to add the same comment as in wallet_listdescriptors.py.

But this PR has moved away from its original intention now, a direction that I don't feel strongly about. So, will close this PR if reviewers don't find it worthwhile.

Serafimov2350 commented at 5:16 PM on June 29, 2026: none

borisserafimov6@gmail.com

rkrux renamed this:
~~test: add comment in wallet_taproot.py~~
test: ExtendedPrivateKey follow-ups
on Jun 29, 2026

DrahtBot added the label CI failed on Jun 29, 2026

rkrux commented at 5:42 PM on June 29, 2026: contributor

Let me see what I can do about this PR now because I got confused here.

I took this PR as an opportunity to also cover a use case that was missed in PR #35543, so I believe this PR is good to review now. Sorry for the initial confusion caused.

DrahtBot removed the label CI failed on Jun 29, 2026

test: use ExtendedPrivateKey in wallet_importdescriptors.py

Looks like a case was missed in the previous PR 35543 that introduced
the utility xprv and xpub classes. This patch covers that remaining
case in the test.

4da5619a6a

test: unify how to avoid the creation of the default wallet in nodes

If test doesn't need the default wallet creation, then the more common
way is to set the wallet_names argument instead of the providing a
custom implementation of the init_wallet method that's a no-op, which
can be confusing for the readers. Also there is a tendency for this
pattern to be copied over other tests that refer the existing ones,
so unifying the pattern is in best interest of the readability of
the test framework.

6b0011630f

in test/functional/wallet_taproot.py:67 in c2b9a7cc69

  63 | @@ -64,6 +64,7 @@ def skip_test_if_missing_module(self):
  64 |      def setup_network(self):
  65 |          self.setup_nodes()
  66 |  
  67 | +    # do not create any wallet by default

maflcko commented at 6:09 AM on June 30, 2026:

heh, I think your initial diff was a good step in the right direction. I think the function should still be removed from the two tests.

However, it should set self.wallet_names = ... correctly instead, so that all tests use the same pattern.

Generally, it is better to use one pattern consistently, than trying to re-invent many different patterns to do the same thing and use all of them in the codebase mixed.

Maybe the docstring of init_wallet could be adjusted to say:

"""
Refer to the self.wallet_names docstring on how to use this.
"""

?

rkrux commented at 9:43 AM on June 30, 2026:

Generally, it is better to use one pattern consistently, than trying to re-invent many different patterns to do the same thing and use all of them in the codebase mixed.

Yeah, this is what confused me initially. I have removed the custom implementation of initi_wallet and unified the pattern for not creating the default wallet in nodes.

Maybe the docstring of init_wallet could be adjusted to say:

Done this as well.

rkrux force-pushed on Jun 30, 2026

in test/functional/wallet_importdescriptors.py:499 in 4da5619a6a

 497 | +            assert_equal(shwpkh_addr_info['desc'][:26], f'sh(wpkh([{root_fingerprint}/0h/0h/{i}]')
 498 |  
 499 |              pkh_addr = w1.getnewaddress('', 'legacy')
 500 |              pkh_addr_info = w1.getaddressinfo(pkh_addr)
 501 | -            assert_equal(pkh_addr_info['desc'][:22], 'pkh([12345678/0h/0h/{}]'.format(i))
 502 | +            assert_equal(pkh_addr_info['desc'][:22], f'pkh([{root_fingerprint}/0h/0h/{i}]')

davidgumberg commented at 10:06 PM on July 1, 2026:

nit:

+
+            key_origin = f'[{root_fingerprint}/0h/0h/{i}]'
+
             bech32_addr_info = w1.getaddressinfo(received_addr)
-            assert_equal(bech32_addr_info['desc'][:23], f'wpkh([{root_fingerprint}/0h/0h/{i}]')
+            assert bech32_addr_info['desc'].startswith(f'wpkh({key_origin}')

            key_origin = f'[{root_fingerprint}/0h/0h/{i}]'

            bech32_addr_info = w1.getaddressinfo(received_addr)
            assert bech32_addr_info['desc'].startswith(f'wpkh({key_origin}')

            shwpkh_addr = w1.getnewaddress('', 'p2sh-segwit')
            shwpkh_addr_info = w1.getaddressinfo(shwpkh_addr)
            assert shwpkh_addr_info['desc'].startswith(f'sh(wpkh({key_origin}')

            pkh_addr = w1.getnewaddress('', 'legacy')
            pkh_addr_info = w1.getaddressinfo(pkh_addr)
            assert pkh_addr_info['desc'].startswith(f'pkh({key_origin}')

davidgumberg commented at 10:07 PM on July 1, 2026: contributor

crACK https://github.com/bitcoin/bitcoin/commit/6b0011630fe9a6ed1010fb49993a874c7477d800