Skip to content

Conversation

jonasschnelli
Copy link
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
Copy link
Member

luke-jr commented Jun 2, 2015

Why?

@jonasschnelli
Copy link
Contributor Author

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
Copy link
Member

luke-jr commented Jun 2, 2015

Oh, I guess this might be useful for storing watch-only HD seeds in the wallet file?

@jonasschnelli
Copy link
Contributor Author

Exactly!.

@laanwj laanwj added the Wallet label Jun 3, 2015
@jgarzik
Copy link
Contributor

jgarzik commented Jun 7, 2015

Too many naked constants (74, 75) without a named constant, IMO

@jonasschnelli
Copy link
Contributor Author

Agreed.
Factored out the ext keysize of 74 bytes to a constant.

@jgarzik
Copy link
Contributor

jgarzik commented Jul 23, 2015

ACK, with comment

  • A better test is probably deserialize then serialize, check for byte match.

@jonasschnelli
Copy link
Contributor Author

Force push fixed: replace magic number 74 bytes into BIP32_EXTKEY_SIZE in base58.h

@jonasschnelli
Copy link
Contributor Author

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
Copy link
Contributor

jgarzik commented Sep 15, 2015

Ping, reviewers

@dcousens
Copy link
Contributor

utACK

@laanwj
Copy link
Member

laanwj commented Oct 1, 2015

utACK (retracted for now, see below)

{
unsigned int len = ::ReadCompactSize(s);
unsigned char code[BIP32_EXTKEY_SIZE];
s.read((char *)&code[0], len);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.
Added a len check assertion.

@sipa
Copy link
Member

sipa commented Nov 28, 2015

@laanwj All good now?

{
unsigned int len = ::ReadCompactSize(s);
unsigned char code[BIP32_EXTKEY_SIZE];
assert(len == BIP32_EXTKEY_SIZE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

CExtPubKey should be serializable like CPubKey
@jonasschnelli
Copy link
Contributor Author

Fixed @laanwj nit.
Rebased & squashed.

@laanwj laanwj merged commit 90604f1 into bitcoin:master Apr 15, 2016
laanwj added a commit that referenced this pull request Apr 15, 2016
90604f1 add bip32 pubkey serialization (Jonas Schnelli)
zkbot added a commit to zcash/zcash that referenced this pull request Apr 18, 2018
Upstream serialization improvements

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5264
- bitcoin/bitcoin#6914
- bitcoin/bitcoin#6215
- bitcoin/bitcoin#8068
  - Only the `COMPACTSIZE` wrapper commit
- bitcoin/bitcoin#8658
- bitcoin/bitcoin#8708
  - Only the serializer variadics commit
- bitcoin/bitcoin#9039
- bitcoin/bitcoin#9125
  - Only the first two commits (the last two block on other upstream PRs)

Part of #2074.
zkbot added a commit to zcash/zcash that referenced this pull request Apr 19, 2018
Upstream serialization improvements

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5264
- bitcoin/bitcoin#6914
- bitcoin/bitcoin#6215
- bitcoin/bitcoin#8068
  - Only the `COMPACTSIZE` wrapper commit
- bitcoin/bitcoin#8658
- bitcoin/bitcoin#8708
  - Only the serializer variadics commit
- bitcoin/bitcoin#9039
- bitcoin/bitcoin#9125
  - Only the first two commits (the last two block on other upstream PRs)

Part of #2074.
furszy added a commit to PIVX-Project/PIVX that referenced this pull request May 29, 2020
249cc9d Avoid -Wshadow errors (random-zebra)
8e1ec9e Use fixed preallocation instead of costly GetSerializeSize (random-zebra)
9b801d0 Add optimized CSizeComputer serializers (random-zebra)
0035a54 Make CSerAction's ForRead() constexpr (random-zebra)
9730a3f Get rid of nType and nVersion (random-zebra)
25ce2bb Make GetSerializeSize a wrapper on top of CSizeComputer (random-zebra)
1b479db Make nType and nVersion private and sometimes const (random-zebra)
35f1755 Make streams' read and write return void (random-zebra)
a395914 Remove unused ReadVersion and WriteVersion (random-zebra)
52e614c [WIP] Remove unused statement in serialization (random-zebra)
82a2021 Add COMPACTSIZE wrapper similar to VARINT for serialization (random-zebra)
13ad779 add bip32 pubkey serialization (random-zebra)
9e9b7b5 [QA] Update json files with sig hash type in ASM for bitcoin-util-test (random-zebra)
3383983 Resolve issue bitcoin#3166 (random-zebra)

Pull request description:

  -Based on top of
  - [x] #1629

  Backports the following serialization improvements from upstream and adds the required changes for the 2nd layer network and the legacy zerocoin code.

  - bitcoin#5264
    > show scriptSig signature hash types. fixes bitcoin#3166
    >
    > The fix basically appends the scriptSig signature hash types, within parentheses, onto the end of the signature(s) in the various "asm" json outputs. That's just the first formatting idea that came to my mind.
    >
    > Added some tests for this too.

  - bitcoin#6215
    > CExtPubKey should be serializable like CPubKey.
    > This would allow storing extended private and public key to support BIP32/HD wallets.

  - bitcoin#8068 (only commit 5249dac)
     This adds COMPACTSIZE wrapper similar to VARINT for serialization

  - bitcoin#8658
    > As the line
    > ```
    > nVersion = this->nVersion;
    > ```
    > seems to have no meaning in READ and also in WRITE serialization op, let's remove it and see what our tests/travis will tell us. See bitcoin#8468 for previous discussion.

  - bitcoin#9039
    > The commits in this pull request implement a sequence of changes:
    >
    > - Simplifications:
    >   - **Remove unused ReadVersion and WriteVersion** CDataStream and CAutoFile had a ReadVersion and WriteVersion method that was never used. Remove them.
    >   - **Make nType and nVersion private and sometimes const** Make the various stream implementations' nType and nVersion private and const (except in CDataStream where we really need a setter).
    >   - **Make streams' read and write return void** The stream implementations have two layers (the upper one with operator<< and operator>>, and a lower one with read and write). The lower layer's return values are never used (nor should they, as they should only be used from the higher layer), so make them void.
    >   - **Make GetSerializeSize a wrapper on top of CSizeComputer** Given that in default GetSerializeSize implementations we're already using CSizeComputer(), get rid of the specialized GetSerializeSize methods everywhere, and just use CSizeComputer. This removes a lot of code which isn't actually used anywhere. In a few places, this removes an actually more efficient size computing algorithm, which we'll bring back in the "Add optimized CSizeComputer serializers" commit later.
    >   - **Get rid of nType and nVersion** The big change: remove the nType and nVersion as parameters to all serialization methods and functions. There is only one place where it's read and has an impact (in CAddress), and even there it does not impact any of the member objects' serializations. Instead, the few places that need nType or nVersion read it directly from the stream, through GetType and GetVersion calls which are added to all streams.
    >   - **Avoid -Wshadow errors** As suggested by @paveljanik, remove the few remaining cases of variable shadowing in the serialization code.
    > - Optimizations:
    >   - **Make CSerAction's ForRead() constexpr** The CSerAction's ForRead() method does not depend on any runtime data, so guarantee that requests to it can be optimized out by making it constexpr (suggested by @theuni in bitcoin#8580).
    >   - **Add optimized CSizeComputer serializers** To get the advantages of faster GetSerializeSize implementations back, reintroduce them in the few places where they actually make a difference, in the form of a specialized Serialize implementation. This actually gets us in a better state than before, as these even get used when they're nested inside the serialization of another object.
    >   - **Use fixed preallocation instead of costly GetSerializeSize** dbwrapper uses GetSerializeSize to compute the size of the buffer to preallocate. For some cases (specifically: CCoins) this requires a costly compression call. Avoid this by just using fixed size preallocations instead.
    >
    > This will make it easier to address @TheBlueMatt's comments in bitcoin#8580, resulting is a simpler and more efficient way to simultaneously deserialize+construct objects with const members from streams.

ACKs for top commit:
  furszy:
    Long and nice PR 👌 , code review ACK 249cc9d .
  Fuzzbawls:
    ACK 249cc9d
  furszy:
    tested ACK 249cc9d and merging.

Tree-SHA512: 56b07634b1e18871e7c9a99d412282c83b85f77f1672ec56330a1131fc7c234cd1ba3a053bdd210cc29f1e636ee374477ff614fa9a930329a7f8f912c5006232
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants