Enabled key derivation for testnet account extended private keys (tprv) by fixing a bug in the raw_bip32_ckd function.

[nicolas3355]

The code uses the vbytes to determine if the input key is private or public.

However, it's only checking against MAINNET_PRIVATE by default, which can lead to issues if you're using testnet prefixes.

private = (vbytes == prefixes[0])
has been changed to
private = vbytes in PRIVATE

With this change, both mainnet (xprv) and testnet (tprv) private keys will be recognized correctly.

[primal100]

Thanks for your suggestion, @nicolas3355

5 tests inside test_wallet.TestWalletKeystoreAddressIntegrity are failing because of this PR.

    raise Exception("Can't do private derivation on public key!")
Exception: Can't do private derivation on public key!

The reason is that these are segwit tests so the prefixes are different. What you are doing is hard-coding the prefixes whereas the prefixes are sent in as an argument depending on the coin and address type (legacy, segwit, and other address types have different prefixes).

Of course we also support multiple different coins so hard-coding the prefixes will break them also.

Can you show me what exactly you are trying to do?

Most methods in the library are designed to be run from the Coin class which has a testnet option:

from cryptos.coins import Bitcoin
coin = Bitcoin(testnet=True)
wallet = coin.p2wpkh_p2sh_wallet(seed='treat dwarf wealth gasp brass outside high rent blood crowd make initial')

This will make sure the correct prefixes are passed to those functions.

If you want to run those functions directly you can pass in the correct prefixes for the testnet for private and public using the prefixes keyword.

There are some variables (PRIVATE, PUBLIC, DEFAULT) in the module that are there for legacy reasons and are still used as helpful default values.

We cannot hardcode them because we support multiple coins and address types all of which have different prefixes.

[nicolas3355]

My bad i will fix it for all address types and then do another pull request.

"Can you show me what exactly you are trying to do?"
I am trying to run the bip32 key derivation function on the tprv not an xprv (on testnet). The code inside the function will only consider the key passed as a secret key (and not a public key), if the prefixes at index[0] is MAINNET_PRIVATE not TESTNET_PRIVATE (the line that causes this is this: "private = vbytes == prefixes[0]"). However, non of the three possible prefixes have that!

PRIVATE = [MAINNET_PRIVATE, TESTNET_PRIVATE]
PUBLIC = [MAINNET_PUBLIC, TESTNET_PUBLIC]
DEFAULT = (MAINNET_PRIVATE, MAINNET_PUBLIC)

To replicate the bug:

from cryptos.coins import Bitcoin
coin = Bitcoin(testnet=True)
wallet = coin.p2wpkh_p2sh_wallet(seed='treat dwarf wealth gasp brass outside high rent blood crowd make `initial')`
# Try running this functions with all the parameters you have: DEFAULT, PUBLIC, PRIVATE. It will fail for all of them.
child_tprv = bip32_ckd(tprv, "m/0/1", prefixes=DEFAULT, public=False)

[primal100]

The function excepts a prefixes argument. You need to send in the private and public key prefix for the coin and address type. Yes it can be complicated to get the correct prefixes but this is a very low level function.

It's not designed to be used with with other module level variables (PRIVATE, PUBLIC, etc) except DEFAULT. Those module level variables are used for other things.

Here are some examples of it working for bitcoin testnet standard address:

from cryptos.deterministic import raw_bip32_ckd, TESTNET_PRIVATE, TESTNET_PUBLIC
raw_bip32_ckd((b'\x045\x83\x94', 0, b'\x00\x00\x00\x00', 0, b'.\xb6\xb7\x17G\xaa:\tge\x83*\x0e\x98\x1cc\xc2\x1d4B\xcar\x82\xca\xfc\xb1\xfe\xc2n_G\xbe', b"\xd6>ZS\x81\xeeWR\x05S_gZ\xb5\x8a\xcf\t\x8aX\x05'\xf8\x92\xbe-\xc7\xdf\x80\x81<\xd5\x84\x01"), 2147483692, prefixes=(TESTNET_PRIVATE, TESTNET_PUBLIC))

Result

(b'\x045\x83\x94', 1, b'\x95Y\xfb\xd1', 2147483692, b'D\x0b\xe7\xf8\xd3\xae\xa0\x8b-\x1d\x93\x1b\x11\xe0\xc8\x12\xdb%kpiV>jA\xdcu;\x99\xb3m\x08', b'\xf6\xd8\xd2K4g\x83p4x\xfd\ryg\x97\xfe\x11\x97\xa1iH\x04\xd6\xd4\xe80\xc9\xa4\xd7\xa2\xe7h\x01')

Or

from cryptos.coins_async import Bitcoin

priv_prefix = Bitcoin.testnet_overrides['xprv_headers']['p2pkh'].to_bytes(4, byteorder='big')
pub_prefix = Bitcoin.testnet_overrides['xpub_headers']['p2pkh'].to_bytes(4, byteorder='big')

raw_bip32_ckd((b'\x045\x83\x94', 0, b'\x00\x00\x00\x00', 0, b'.\xb6\xb7\x17G\xaa:\tge\x83*\x0e\x98\x1cc\xc2\x1d4B\xcar\x82\xca\xfc\xb1\xfe\xc2n_G\xbe', b"\xd6>ZS\x81\xeeWR\x05S_gZ\xb5\x8a\xcf\t\x8aX\x05'\xf8\x92\xbe-\xc7\xdf\x80\x81<\xd5\x84\x01"), 2147483692, prefixes=(priv_prefix , pub_prefix ))

Result:

(b'\x045\x83\x94', 1, b'\x95Y\xfb\xd1', 2147483692, b'D\x0b\xe7\xf8\xd3\xae\xa0\x8b-\x1d\x93\x1b\x11\xe0\xc8\x12\xdb%kpiV>jA\xdcu;\x99\xb3m\x08', b'\xf6\xd8\xd2K4g\x83p4x\xfd\ryg\x97\xfe\x11\x97\xa1iH\x04\xd6\xd4\xe80\xc9\xa4\xd7\xa2\xe7h\x01')

It already works for all coins simply by supplying the correct prefixes. Generally this function is not intended to be run directly. But it will be called as part of the higher level Keystore and Coin methods. The correct prefixes to use are passed down from the higher level methods. If you can find that the correct prefxies are not being passed down then let me know as that would be considered a bug. But the function itself works as expected.

[nicolas3355]

The child key derivation function doesn't care about the type of address (to some extent). It just appends the version bytes no matter what it is. Passing prefixes is redundant and it can lead to problems if not properly used. All the information you need are in the version bytes anyway. I understand that for backward compatibility you need the prefixes variable to stay, but i don't think the logic of the function should depend on it.

There are people out there (including me) who want the low level functions but not the high level functions. It would be nice to have it polished for them. I just didn't want to implement my own child key derivation function and i wanted to re-use yours and thought i would do a pull request. But now i get that the function would have worked properly, if I passed the right prefixes but i didn't know the format of the prefixes that was expected.

I updated the PRIVATE, PUBLIC lists with the version bytes that I could think of and I made another pull request. I ran the tests using python3 -m unittest discover in the root directory but i have a lot of failures regarding fees but i think the segwit problems should be fixed now. I am assuming the funds have been depleted? Anyway, feel free not to merge but i thought would to the pull request anyway, since i did it on my branch.

Thanks for the discussion! It cleared things up!

[primal100]

Hi yes,

Don't worry about the transaction tests, I need to add coins to testnet to run them (mainly I need to work on mocking transactions requests at some point).

For the purpose of this TR only test_wallet.py is required.

Just to explain about how the library works the prefixes are defined in coin classes like this:

from ..explorers import blockchain
from .base import BaseCoin


class Bitcoin(BaseCoin):
    coin_symbol = "BTC"
    display_name = "Bitcoin"
    segwit_supported = True
    magicbyte = 0
    script_magicbyte = 5
    minimum_fee = 450
    segwit_hrp = "bc"
    wif_prefix: int = 0x80
    client_kwargs = {
        'server_file': 'bitcoin.json',
    }

    testnet_overrides = {
        'display_name': "Bitcoin Testnet",
        'coin_symbol': "BTCTEST",
        'magicbyte': 111,
        'script_magicbyte': 196,
        'segwit_hrp': 'tb',
        'hd_path': 1,
        'wif_prefix': 0xef,
        'minimum_fee': 1000,
        'client_kwargs': {
            'server_file': 'bitcoin_testnet.json',
            'use_ssl': False
        },
        'electrum_pkey_format': 'wif',
        'xprv_headers': {
            'p2pkh': 0x04358394,
            'p2wpkh-p2sh': 0x044a4e28,
            'p2wsh-p2sh': 0x295b005,
            'p2wpkh': 0x04358394,
            'p2wsh': 0x2aa7a99
        },
        'xpub_headers': {
            'p2pkh': 0x043587cf,
            'p2wpkh-p2sh': 0x044a5262,
            'p2wsh-p2sh': 0x295b43f,
            'p2wpkh': 0x043587cf,
            'p2wsh': 0x2aa7ed3
        },
    }

There are mainnet values and testnet overrides

These are passed down to the lower level functions.

I really don't want to start adding these values in lower level modules as well as it will get messy to maintain. It breaks the principle of these coin-specific values existing in the higher level classes and being passed down to the lower level ones through arguments.

The solution for a function to understand coin or mainnet/testnet context is to add a new method to the coin class or the Keystore class (which is passed the correct coin object to use and so knows which prefixes to use).

For example in cryptos/keystore.py there is a method which calls bip32_ckd:

    def add_xprv_from_seed(self, bip32_seed, xtype, derivation, electrum=False):
        self.set_xtype(xtype, electrum=electrum)
        self.root_derivation = derivation
        xprv = bip32_master_key(bip32_seed, self.bip39_prefixes)
        xprv = bip32_ckd(xprv, derivation, self.bip39_prefixes)
        self.add_xprv(xprv)

So by creating a keystore class and callling the method it knows which prefixes to use.

What we could do is add bip32_ckd to the keystore class so that function can be called directly.

You could do this yourself by subclassing the Keystore class and adding this method to it (which then calls the existing bip32_ckd with the correct prefixes).

class CustomKeystore(BIP32_KeyStore):
 def bip32_ckd(xprv, derivation):
       return bip32_ckd(xprv, derivation, self.bip39_prefixes)
       
coin = Bitcoin(testnet=True)
keystore = CustomKeystore(coin)
keystore.bip32_ckd(xprv, derivation)

Or just provide the prefixes youself as explained above. This is why using the lower level functions is hard, there are many different data formats in python and bitcoin that are used all over the place (going back to Vitalik's original version of the library that I forked).

You should probably check if there is a higher level function that already does what you want to do.

I understand that the raw_bip32_ckd itself only uses the prefixes to verify that the key provided is a private key and it doesn't actually do anything. That logic goes back to the original version of the library. Perhaps prefixes could be removed as a requirement but I need to check why that logic is needed. Even so, the correct prefixes still need to be used in bip32_ckd function and passed to bip32_deserialize, so your solution wouldn't help with that as bip32_deserialize needs to know which prefixes to check.

There are lots of places all over the library even in the low level functons where they need to coin-aware and mainnet/testnet-aware, simply providing a list of all possible prefixes isn't enough.