test: Support decoding segwit address in address_to_scriptpubkey() #27269

per the functional test README:

- If more than one name from a module is needed, use lexicographically sorted multi-line imports
  in order to reduce the possibility of potential merge conflicts.

so this should be

from test_framework.segwit_addr import (
    bech32_decode,
    convertbits,
    encode_segwit_address,
)

also, if you end up taking my suggestion to use decode_segwit_address, you can remove the bech32_decode, convertbits imports

Yes, I agree with that, also I will use decode_segwit_address. removing the imports.

since you are already using the encode_segwit_address function, it might be better to also use the decode_segwit_address function here instead of duplicating the logic.

something like:

    hrp = address[:2]
    version, payload = decode_segwit_address(hrp, address)
    return version, bytearray(payload)

Thanks for pointing that out :100:

styling nit: remove the extra line at the end of the file

styling nit: two blank lines between functions, per PEP8 guidlines

functions in the codebase have one blank line in between.

This is because a lot of code was written before we introduced a style guide, or because it was missed during review. If something is not covered by the style guide, then we should try to be consistent with the surrounding code. If something is covered by the style guide, we should be consistent with the style guide, even if that is not consistent with the surrounding code. This helps nudge the codebase towards the style guide over time.

Thanks

I may be mistaken but I believe that actually this should have 1 line gap, as it's a member function? But its OK, I wouldn't change and invalidate the ACKS you've got already! (I configured my editor to show PEP8 diagnostics so I can view them directly as I'm editing).

styling nit: two blank lines between functions, per PEP8 guidelines

I think for consistency in the code base it's okay. There is only one blank line between functions in address.py

The goal is to have the code base consistent with our style guide, not consistent with itself. This means any new code should adhere to the style guide and, if possible, old code should be formatted consistent with the style guide if it's already being touched as part of a PR.

I understand.

styling nit: should be lexicographically sorted and add a comma at the end of each line (even if it is the last one)

Thanks. :+1:

style nit: these should be lexicographically sorted

Okay :+1:

thinking about this a bit more, I think we should have a more robust way of checking this. iirc, taproot would be tb1p on testnet, so this would not work for segwit v1 addresses.

Maybe instead you could do something like version, payload = bech32_to_bytes and then check the version. If the version is None, then move on to payload, version = base58_to_byte etc.

Thanks for the thorough review Josi. I will fix that. This means I have to check in bech32_to_bytes version is None or not so as not to call bytearray(payload) with a None value.

there should be a space between function arguments and I'd return a tuple here to stay consistent with the other functions in the test_framework/segwit_addr.py file:

    version, payload = decode_segwit_address(hrp, address)
    if version is None:
        return (None, None)

Thanks :+1:

This unfortunately doesn't lead to the correct result for regtest segwit addresses, as those have the four-character HRP 'bcrt'. To make it work for addresses on all networks, I'd suggest to:

1. finding the HRP by extracting the string before the bech32 separator character '1' (either the find() or .split() method on strings should be handy for this)
2. assert that the extracted HRP matches our supported segwit addresses (i.e. one of bc, tb, bcrt).

Thank you for your valuable feedback.

• I will use .split('1') on the address to extract the hrp, and also assert to verify if the hrp is valid as you noted.
• I will have to modify address_to_scriptpubkey to check for base58 address first before calling bech32_to_bytes or it will assert false.

I hope this addresses the issue

I'm not sure I understand your second point, @ismaelsadeeq . The order shouldn't matter in address_to_scriptpubkey. It should be sufficient to properly extract the HRP and check that it is a bech32.

bech32_to_bytes will be updated to handle all segwit addresses and check if hrp is valid.

def bech32_to_bytes(address):
    hrp = address.split('1')[0]
    assert hrp in ['bc', 'tb', 'bcrt'], f"hrp should be 'bc', 'tb', or 'bcrt', not '{hrp}'"
    version, payload = decode_segwit_address(hrp, address)
    if version is None:
        return (None, None)
    return version, bytearray(payload)

So calling address_to_scriptpubkey with a base58 address will assert to false, because the hrp is invalid.

def address_to_scriptpubkey(address):
    """Converts a given address to the corresponding output script (scriptPubKey)."""
    version, payload = bech32_to_bytes(address)
    if version is not None:
        return program_to_witness_script(version, payload) # testnet segwit scriptpubkey
    payload, version = base58_to_byte(address)
    if version == 111:  # testnet pubkey hash
        return keyhash_to_p2pkh_script(payload)
    elif version == 196:  # testnet script hash
        return scripthash_to_p2sh_script(payload)
    # TODO: also support other address formats
    else:
        assert False

address_to_scriptpubkey will be updated to check whether an address is in base58 before calling bech32_to_bytes

def address_to_scriptpubkey(address):
    """Converts a given address to the corresponding output script (scriptPubKey)."""
    prefix = address[0]
    if prefix in ['m','n','2']:
        payload, version = base58_to_byte(address)
        if version == 111:  # testnet pubkey hash
            return keyhash_to_p2pkh_script(payload)
        elif version == 196:  # testnet script hash
            return scripthash_to_p2sh_script(payload)

    version, payload = bech32_to_bytes(address)
    return program_to_witness_script(version, payload) #segwit scriptpubkey

ah, i see. I think id approach this in a different way: in bech32_to_bytes, if the HRP doesn't match a bech32 HRP, return (None, None).

that way, address_to_scriptpubkey can continue and try base58 next. if the address is indeed malformed (meaning not bech32 or base58), eventually address_to_scriptpubkey will return false. @theStack I think this is more in line with what you meant by assert?

That will work also, thanks @josibake

@josibake: yes, agree that considering on what we need in address_to_scriptpubkey returning (None, None) is better than asserting. LGTM!

I think this is totally fine as is here for now, but in the future we may want to put these all in a list/enum

Instead of hardcoding the hrp here, why not have check_bech32_decode() take hrp as a parameter, then we can add bc1 test vectors instead of being limited to tb1?

edit: I don't consider this a blocker or needing reponse before merging, but will be nice to consider in the future.