Abstract chaincodes into CChainCode #6034

pull jonasschnelli wants to merge 2 commits into bitcoin:master from jonasschnelli:2015/04/bip32_chaincode changing 6 files +28 −28

jonasschnelli commented at 2:31 PM on April 20, 2015: contributor

Rehashed work from sipa. Slightly improves current Bip32 support (CExtKey, CExtPubKey). Code is unused at the moment (only used from bip32_tests.cpp)
in src/key.h:None in 6d762237a5 outdated
```
   5 | @@ -6,6 +6,7 @@
   6 |  #ifndef BITCOIN_KEY_H
   7 |  #define BITCOIN_KEY_H
   8 |  
   9 | +#include "pubkey.h"
```
jonasschnelli commented at 2:31 PM on April 20, 2015:

I hope nobody complains about this new include?

theuni commented at 9:01 PM on April 21, 2015:

From my POV, that's on the list of "safe to include everywhere" since it's used in libbitcoinconsensus.
jonasschnelli force-pushed on Apr 20, 2015
jonasschnelli force-pushed on Apr 20, 2015
sipa commented at 11:13 AM on April 21, 2015: member

utACK
theuni commented at 10:14 PM on April 21, 2015: member

Looks good to me, but if you're going to the trouble of abstracting CChainCode, you may as well treat chaincodes as objects wholesale. See https://github.com/theuni/bitcoin/commit/58fe156ec2672b354650c3e3d89d0724723bbc34 for a quick (untested) example.
jonasschnelli commented at 6:29 AM on April 22, 2015: contributor

Thanks @theuni! Right. This would be the next logical step. Reviewed and added (as it is) your https://github.com/theuni/bitcoin/commit/58fe156ec2672b354650c3e3d89d0724723bbc34 to this PR.
sipa commented at 7:07 AM on April 22, 2015: member

key already implicitly depends on pubkey anyway (through the forward declaration).
sipa commented at 10:46 AM on April 24, 2015: member

utACK both commits
jgarzik commented at 1:05 PM on April 24, 2015: contributor

ACK
laanwj commented at 1:57 PM on May 1, 2015: member

Playing devil's advocate: how is a CChainCode different from a uint256? (the new type without arithmethic operators) It is the same size, has a superset of the methods (like SetNull and such) and serializes the same way.
theuni commented at 2:06 PM on May 1, 2015: member

@laanwj heh, very good point.
jonasschnelli commented at 6:15 PM on May 1, 2015: contributor

Indeed! If there are no concerns i'll change this so it will use uint256 instead of CChainCode (or the current unsigned char chainCode[32])
sipa commented at 6:23 PM on May 1, 2015: member

If you plan to later extend CChainCode with extra functionality, you could at this point just do a

typedef uint256 ChainCode;

(leave the C prefix of for new classes, we don't care)
jonasschnelli force-pushed on May 2, 2015
Abstract chaincodes into CChainCode
# Conflicts: # src/key.cpp # src/key.h
8cf1485f3b
jonasschnelli commented at 9:32 AM on May 2, 2015: contributor

Replaced the CChainCode struct with a uint256 typedef.

in src/hash.cpp:None in e980948b72 outdated

  80 |      num[1] = (nChild >> 16) & 0xFF;
  81 |      num[2] = (nChild >>  8) & 0xFF;
  82 |      num[3] = (nChild >>  0) & 0xFF;
  83 | -    CHMAC_SHA512(chainCode, 32).Write(&header, 1)
  84 | +    CHMAC_SHA512(chainCode.begin(), chainCode.size()).Write(&header, 1)
  85 |                                 .Write(data, 32)

sipa commented at 12:57 PM on May 2, 2015:

Indentation is off now. No problem if you don't want to keep this style, but now it becomes inconsistent.

jonasschnelli commented at 1:01 PM on May 2, 2015:

Agreed. Removed the indentation.

jonasschnelli force-pushed on May 2, 2015
laanwj commented at 3:14 PM on May 6, 2015: member

utACK (after squash)

chaincodes: abstract away more chaincode behavior

[squashme] replace struct CCainCode with a typedef uint256 ChainCode

a574899671

jonasschnelli force-pushed on May 6, 2015
laanwj merged this on May 6, 2015
laanwj closed this on May 6, 2015
laanwj referenced this in commit 6a877e870e on May 6, 2015
MarcoFalke locked this on Sep 8, 2021

Contributors