add bip32 pub key serialization #6215

pull jonasschnelli wants to merge 1 commits into bitcoin:master from jonasschnelli:2015/06/extkey_ser changing 6 files +71 −12

jonasschnelli commented at 2:36 PM on June 1, 2015: contributor

CExtPubKey should be serializable like CPubKey.

EDIT: This would allow storing extended private and public key to support BIP32/HD wallets. Related to #6265
luke-jr commented at 4:11 AM on June 2, 2015: member

Why?
jonasschnelli commented at 7:00 AM on June 2, 2015: contributor

This is a valid question. I'm working on a new wallet and i'm trying to split of and create PRs for everything which could be used independent and might reduce the diff-size when merging a bigger wallet change later. [1] https://github.com/jonasschnelli/bitcoin/tree/2015/05/corewallet

Because there is already unused bip32 stuff in bitcoin core, why not complete it with things which would be necessary in case of a full bip32 support.
luke-jr commented at 7:27 AM on June 2, 2015: member

Oh, I guess this might be useful for storing watch-only HD seeds in the wallet file?
jonasschnelli commented at 7:36 AM on June 2, 2015: contributor

Exactly!.
laanwj added the label Wallet on Jun 3, 2015
jgarzik commented at 2:37 AM on June 7, 2015: contributor

Too many naked constants (74, 75) without a named constant, IMO
jonasschnelli commented at 7:31 AM on June 7, 2015: contributor

Agreed. Factored out the ext keysize of 74 bytes to a constant.
jonasschnelli force-pushed on Jun 7, 2015
jgarzik commented at 7:24 PM on July 23, 2015: contributor
ACK, with comment
- A better test is probably deserialize then serialize, check for byte match.
jonasschnelli force-pushed on Jul 24, 2015
jonasschnelli commented at 8:56 AM on July 24, 2015: contributor

Force push fixed: replace magic number 74 bytes into BIP32_EXTKEY_SIZE in base58.h
jonasschnelli force-pushed on Jul 28, 2015
jonasschnelli commented at 7:15 AM on July 28, 2015: contributor

Added serializing/deserializing of a bip32 extended private key. Followed @jgarzik advice of checking the serialized output by deserialize again and compare the keys (bytes).
jgarzik commented at 6:28 PM on September 15, 2015: contributor

Ping, reviewers
dcousens commented at 8:24 AM on September 16, 2015: contributor

utACK
laanwj commented at 2:53 PM on October 1, 2015: member

~~utACK~~ (retracted for now, see below)
in src/pubkey.h:None in 153305a211 outdated
```
 224 | +    template <typename Stream>
 225 | +    void Unserialize(Stream& s, int nType, int nVersion)
 226 | +    {
 227 | +        unsigned int len = ::ReadCompactSize(s);
 228 | +        unsigned char code[BIP32_EXTKEY_SIZE];
 229 | +        s.read((char *)&code[0], len);
```
laanwj commented at 3:01 PM on October 1, 2015:

This looks like a potential buffer overflow: you read len bytes into a fixed buffer. Please check that len == BIP32_EXTKEY_SIZE

jonasschnelli commented at 6:51 PM on October 1, 2015:

Agreed. Added a len check assertion.
sipa commented at 10:56 PM on November 28, 2015: member

@laanwj All good now?
in src/pubkey.h:None in 497d3dbb51 outdated
```
 224 | +    template <typename Stream>
 225 | +    void Unserialize(Stream& s, int nType, int nVersion)
 226 | +    {
 227 | +        unsigned int len = ::ReadCompactSize(s);
 228 | +        unsigned char code[BIP32_EXTKEY_SIZE];
 229 | +        assert(len == BIP32_EXTKEY_SIZE);
```
laanwj commented at 4:01 PM on April 14, 2016:

IMO it'd be better to throw an exception (serialization framework seems to prefer std::ios_base::failure, see here and here ) which can at least be caught instead of killing the program immediately.

Of course we don't expect to deserialize these from the network, otherwise someone may crash the application, but it may be better to be consistent.
jonasschnelli force-pushed on Apr 14, 2016

add bip32 pubkey serialization

CExtPubKey should be serializable like CPubKey

90604f16af

jonasschnelli force-pushed on Apr 14, 2016
jonasschnelli commented at 7:10 PM on April 14, 2016: contributor

Fixed @laanwj nit. Rebased & squashed.
laanwj merged this on Apr 15, 2016
laanwj closed this on Apr 15, 2016
laanwj referenced this in commit 48c5adfbce on Apr 15, 2016
zkbot referenced this in commit 3b68ab255f on Apr 17, 2018
zkbot referenced this in commit d408e23ab7 on Apr 18, 2018
zkbot referenced this in commit 0753a0e8a9 on Apr 19, 2018
furszy referenced this in commit 8cd9c592a9 on May 29, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#6265 Add HD/Bip32 support