test: implement 'bech32m' mode for `getnewdestination()` helper #25289

theStack commented at 12:13 AM on June 7, 2022: member

This PR adds the missing 'bech32m' mode for the getnewdestination() helper and sets it as default, i.e. the function returns a tuple (output x-only-pubkey, scriptPubKey, taproot address) now if not specified otherwise. In a preparation commit, the helpers output_key_to_p2tr{_script} are introduced. Note that in contrast to all other common script output types, there are usually two keys involved in creating a taproot output (internal key and output key), hence the prefix output_ is used to clarify that the output key is expected and the helpers don't do any key tweaking.

Thanks to michaelfolkson (for pointing out this TODO that I forgot about) and sipa (for patiently explaining basic things about BIP341).

DrahtBot added the label Tests on Jun 7, 2022

DrahtBot commented at 8:38 AM on June 7, 2022: 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:

#23127 (tests: Use test framework utils where possible by vincenzopalazzo)

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.

in test/functional/test_framework/wallet.py:316 in 51b5dafdb4 outdated

 311 | -    else:
 312 | -        assert False
 313 | +    elif address_type == 'bech32m':
 314 | +        pubkey = taproot_construct(compute_xonly_pubkey(key.get_bytes())[0]).output_pubkey
 315 | +        scriptpubkey = output_key_to_p2tr_script(pubkey)
 316 | +        address = output_key_to_p2tr(pubkey)

laanwj commented at 10:51 AM on June 7, 2022:

you might want to keep the else: assert False in case anyone passes an unknown string in (e.g. typo)

theStack commented at 11:13 AM on June 7, 2022:

Good idea, thanks. Done.

theStack force-pushed on Jun 7, 2022

in test/functional/wallet_taproot.py:184 in 39918b8caf outdated

 181 | @@ -183,10 +182,7 @@ def multi_a(k, hex_keys, sort=False):
 182 |  
 183 |  def compute_taproot_address(pubkey, scripts):
 184 |      """Compute the address for a taproot output with given inner key and scripts."""

michaelfolkson commented at 12:27 PM on June 7, 2022:

Could be done in a follow up PR (happy to do it) but the terminology we are using is "internal key" rather than "inner key". (There is also a inner_keys variable name to be replaced in feature_taproot.py.)

michaelfolkson commented at 12:29 PM on June 7, 2022: contributor

Concept ACK, Approach ACK

The following from the PR author on IRC may be useful for reviewers:

i think we need to 1) create a private key via generate_privkey(), 2) pass that to compute_xonly_pubkey(...), 3) pass the result to taproot_construct(...), 4) use the result's .scriptPubKey to create the address via encode_segwit_addr(...)

in test/functional/test_framework/wallet.py:312 in 39918b8caf outdated

 306 | @@ -301,7 +307,10 @@ def getnewdestination(address_type='bech32'):
 307 |      elif address_type == 'bech32':
 308 |          scriptpubkey = key_to_p2wpkh_script(pubkey)
 309 |          address = key_to_p2wpkh(pubkey)
 310 | -    # TODO: also support bech32m (need to generate x-only-pubkey)
 311 | +    elif address_type == 'bech32m':
 312 | +        pubkey = taproot_construct(compute_xonly_pubkey(key.get_bytes())[0]).output_pubkey
 313 | +        scriptpubkey = output_key_to_p2tr_script(pubkey)

w0xlt commented at 1:56 PM on June 7, 2022:

Does taproot_construct() already return the scriptPubKey in TaprootInfo?

michaelfolkson commented at 11:05 AM on June 14, 2022:

@w0xlt: Yeah it appears to me it does too. Would the output_key_to_p2tr_script helper be used elsewhere in future (when taproot_construct() isn't used) or should it be ditched?

theStack commented at 11:45 AM on June 14, 2022:

Good point, changed in the latest force-push to use scriptPubKey from the returned TaprootInfo object. I think it still makes sense to keep the output_key_to_p2tr_script helper, first for consistency reasons (we usually have both the x_to_P2yz and x_to_P2yz_script helpers available) and on the other hand for cases where we might don't use taproot_construct() in the future (for bare outputs where we don't know use the script tree or internal key, see e.g. #23480).

w0xlt approved

w0xlt commented at 1:56 PM on June 7, 2022: contributor

tACK https://github.com/bitcoin/bitcoin/pull/25289/commits/39918b8cafe0034e45d695d2c675a4d89a2056f7

test: add helpers for creating P2TR scripts/addresses from output key 1999dcfa40

test: implement 'bech32m' mode for `getnewdestination()` helper dcf36fe8e3

theStack force-pushed on Jun 14, 2022

michaelfolkson commented at 1:42 PM on June 14, 2022: contributor

ACK dcf36fe8e3e1fc1e865072232281b72889586e40

Code review, ran functional test suite on MacOS. Adds output_key_to_p2tr_script helper but this PR doesn't use it.

Re-ACK @w0xlt?

w0xlt approved

w0xlt commented at 8:40 PM on June 17, 2022: contributor

reACK https://github.com/bitcoin/bitcoin/pull/25289/commits/dcf36fe8e3e1fc1e865072232281b72889586e40

laanwj merged this on Jun 17, 2022

laanwj closed this on Jun 17, 2022

theStack deleted the branch on Jun 18, 2022

sidhujag referenced this in commit 5808bfd012 on Jun 19, 2022

DrahtBot locked this on Jun 18, 2023