Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BIP44 Key Generation with HD Mint path #392

Merged
merged 47 commits into from Jun 19, 2019

Conversation

@riordant
Copy link
Contributor

commented May 3, 2019

BIP44 Upgrade

The key generation process within zcoind uses BIP32, ie. it creates keys using a Hierarchical Deterministic tree. This keypath is currently set to the default within Bitcoin Core, m/0'/0'/0'. BIP44 is a standard for specifying how various cryptocurrencies should be positioned within the HD tree, and is the standard by which hardware wallet devices such as Ledger and Trezor create addresses.

This PR updates our key generation process to use a BIP44 path. It uses the coinType parameter for Zcoin as described in SLIP-0044. Our BIP44 path for normal address creation is now:
m/44'/136'/0'/0/<address_index>.
(As no functionality exists within zcoind to select any differing parameter for account or change, these have remained as the constants 0' and 0 respectively. This does not affect compatibility with BIP44 supporting wallets).

This has full compatibility with keys generated along m/0'/0'/0' as only new keys generated will use the path.

HD Mints

Secondly, this PR adds mint seed generation using this path. What differs is the change parameter: For mints this parameter is 2.

The BIP44 path for mint seed creation is:
m/44'/136'/0'/2/<seed_index>.

The mint secret is thus generated as follows:

  • A Keypair is generated at <seed_index> with 256-bit privkey
  • A 512-bit mint seed (mintMaster) is generated via: HMAC-SHA512(SHA256(<seed_index>), privkey)
  • The 256-bit ECDSA key (ecdsaSecKey) for each mint is SHA256(Bottom 256 bits of mintMaster)
  • The 256-bit randomness seed (seedRandomness) for each mint is SHA256(Top 256 bits of mintMaster)

Then:
Public key is generated from ecdsaSecKey in the usual way
The ScalarFromSeed function in secp256k1/src/cpp/Scalar.cpp is used to generate valid randomness.

Other changes:
- Modifications to uint256, arith_uint256 and hash to allow for 512 bit operations
- Movement of CZerocoinEntry(V3) to a new file, primitives/zerocoin.h (compilation issues otherwise)
- Updates to Scalar.h: addition of a function to generate randomness from a seed, and a parameter to the hash function to toggle if a mod_p operation should be performed after the hash (set to true by default)

@jivanyan

This comment has been minimized.

Copy link

commented May 3, 2019

Maybe we can consider using HMAC algorithms instead of simple hashing? I suggest using HMAC-SH512(seedMaster, count) instead of just SHA512(seedMaster | count).

@yura-pakhuchiy

This comment has been minimized.

Copy link

commented May 3, 2019

I suggest to use BIP32/BIP43 if possible. With pseudo-BIP44 path - m/44'/136'/account'/2'/mint' or m/44'/136'/1000+account'/mint'.
https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki
https://github.com/bitcoin/bips/blob/master/bip-0043.mediawiki
https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki

@riordant

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

@jivanyan Ok I agree with this, considering also that the other BIP standards use HMAC. So this is what I have now:

  • seedMaster generated (256 bits, using the random generator from the Key class)
  • Next seed: HMAC-SHA512(seedMaster, count)
  • ECDSA key: SHA256(Bottom 256 bits of seed)
  • randomness seed: SHA256(Top 256 bits of seed)

Let me know what you think

@thebevrishot thebevrishot added the sigma label May 7, 2019

@riordant

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

@yura-pakhuchiy I'm looking at upgrading core to use our BIP44 path (It currently derives keys at m/0'/0'/0'). I would like to keep it consistent with the official specification. In Electrum, however, the default path is m/44'/136'/0'. The BIP44 spec is m / purpose' / coin_type' / account' / change / address_index, ie. the Electrum path does not derive the change path, and starts creating addresses at the account level. How do you think we should proceed? (I'm aware users can change the default path in Electrum, but we should try to keep it consistent)

@yura-pakhuchiy

This comment has been minimized.

Copy link

commented May 9, 2019

@riordant Hm, electrum does use change / address_index part at least for BIP39 seeds, otherwise it would be incompatible with trezor and ledger. You can check with this tool. I have not checked for native electrum seeds, but it will be really strange if it treats them differently.

@yura-pakhuchiy

This comment has been minimized.

Copy link

commented May 9, 2019

BTW, consider also using BIP39 seeds in the Zcoin Core and/or new GUI. It is much easier to backup it, rather than lengthy and not human-friendly xprv.

@riordant

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

@yura-pakhuchiy Well the default path shown when setting up a HW wallet (m/44'/136'/0') leaves out the last two value, change. I assume it just resolves to 0' then.
On BIP39, yes I intend to look into it afterward.

@riordant

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

This is the standard Im thinking now:
BIP44: m / purpose' / coin_type' / account' / change / <address_index>
Mint Path: m / 44' / 136' / <account>' / 2' / <seed_index>'
Keypair generated at <seed_index> with 256-bit privkey
512-bit mint seed (mintMaster) is HMAC-SHA512(SHA256(<seed_index>), privkey)
The 256-bit ECDSA key (ecdsaSecKey) for each mint is SHA256(Bottom 256 bits of mintMaster)
The 256-bit randomness seed (seedRandomness) for each mint is SHA256(Top 256 bits of mintMaster)

@yura-pakhuchiy

This comment has been minimized.

Copy link

commented May 9, 2019

You are probably assuming that account is always 0, but it is not. m/44'/136'/1' corresponds to the second Zcoin account on Trezor/Ledger. Account number in path for mints should correspond to the account number for wallet, so user can restore both of them. I think there 2 options to use:

  1. m/44'/136'/account'/2'/mint_index'
    change can be 0 or 1 according to BIPs, so we can use 2 for mints to avoid clashes. However maybe xpub derived we from m/44'/136'/account' or m/44'/136'/account'/{0,1} will leak information about mints derived from m/44'/136'/account'/2'. And since xpub is semi-public information it is quite dangerous.
  2. m/44'/136'/1000000+account'/mint_index'
    High enough constant (1000000 in this example) is added to account number to derive mint seeds. Nothing is leaked in this case, unless user will manually create account with such high number.

I think 1st is more close to BIP44, but since it may leak information, it is probably better to use 2nd option.

@riordant

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

I'm all for the 1st option. In the second case, I really don't like hard-setting some limit on the number of addresses before we should start to consider mints vs normal keys.
Could you explain in more detail how the first option could lead to leaked information? All that's being used is the private key (and index) from the key generated, I don't see how this could leak any info, assuming the same level of security as normal addresses. And given that, couldn't you make the same argument that the xpub in the 2nd option could leak information?

@yura-pakhuchiy

This comment has been minimized.

Copy link

commented May 9, 2019

If you know m/44'/136'/account' you can generate m/44'/136'/account'/...whatever, eg. m/44'/136'/account'/2'/0'. Of course we do not tell anybody m/44'/136'/account', only xpub generated from it, but even xpub leaks some information about m/44'/136'/account', but I'm not cryptographer to estimate how bad it is. Also note that path components without hash are less secure than with hash. This is probably why Bitcoin Core have not adopted BIP44. Maybe @jivanyan can comment whether it is safe to use m/44'/136'/account'/2'/mint_index'?

@riordant

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Ok let's wait for @jivanyan to weigh in. If we don't feel it's secure I won't proceed with it

@jivanyan

This comment has been minimized.

Copy link

commented May 10, 2019

You are probably assuming that account is always 0, but it is not. m/44'/136'/1' corresponds to the second Zcoin account on Trezor/Ledger. Account number in path for mints should correspond to the account number for wallet, so user can restore both of them. I think there 2 options to use:

  1. m/44'/136'/account'/2'/mint_index'
    change can be 0 or 1 according to BIPs, so we can use 2 for mints to avoid clashes. However maybe xpub derived we from m/44'/136'/account' or m/44'/136'/account'/{0,1} will leak information about mints derived from m/44'/136'/account'/2'. And since xpub is semi-public information it is quite dangerous.
  2. m/44'/136'/1000000+account'/mint_index'
    High enough constant (1000000 in this example) is added to account number to derive mint seeds. Nothing is leaked in this case, unless user will manually create account with such high number.

I think 1st is more close to BIP44, but since it may leak information, it is probably better to use 2nd option.

Having public keys generated from m/44'/136'/account'/{0,1}' tell nothing about private keys generated from the path m/44'/136'/account'/2'' so I think the first option is secure

@riordant

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

Thank you for your input, @jivanyan. Given that I will proceed with the first option.

src/arith_uint256.h Show resolved Hide resolved
src/hdmint/chain.cpp Outdated Show resolved Hide resolved
src/hdmint/chain.h Outdated Show resolved Hide resolved
src/hdmint/hdmint.cpp Outdated Show resolved Hide resolved
src/hdmint/tracker.cpp Show resolved Hide resolved
src/wallet/wallet.cpp Outdated Show resolved Hide resolved
src/wallet/wallet.cpp Outdated Show resolved Hide resolved
src/hdmint/mintpool.cpp Outdated Show resolved Hide resolved
src/hdmint/mintpool.cpp Outdated Show resolved Hide resolved
src/hdmint/mintpool.cpp Outdated Show resolved Hide resolved
src/secp256k1/src/cpp/Scalar.cpp Outdated Show resolved Hide resolved
src/hdmint/chain.h Outdated Show resolved Hide resolved
riordant added 4 commits Jun 5, 2019
@thebevrishot
Copy link
Member

left a comment

QT issue, please check.

src/wallet/wallet.cpp Show resolved Hide resolved
src/qt/walletmodel.cpp Show resolved Hide resolved
src/qt/walletmodel.cpp Show resolved Hide resolved
riordant added 2 commits Jun 10, 2019
@thebevrishot
Copy link
Member

left a comment

Thanks for fix QT issue.

@ultimaweapon

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

@riordant please fix CI failure.

@riordant riordant dismissed stale reviews from thebevrishot and levonpetrosyan93 via 69eb9f6 Jun 15, 2019

@thebevrishot thebevrishot merged commit 2d5a06d into sigma Jun 19, 2019

3 checks passed

LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@thebevrishot thebevrishot deleted the hdmint branch Jun 19, 2019

@riordant riordant referenced this pull request Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.