fix CBitcoinExtKeyBase template #6468

pull jonasschnelli wants to merge 3 commits into bitcoin:master from jonasschnelli:2015/07/base58c_extkey_fixes changing 2 files +19 −1

jonasschnelli commented at 2:31 PM on July 23, 2015: contributor
- fix Decode call (req. only one param)
- add constructor for base58c->CExtKey
The current Decode() call with two parameters won't compile (not detected because the c++ temple never was evaluated).

Also adds test coverage.
jonasschnelli force-pushed on Jul 23, 2015

fix and extend CBitcoinExtKeyBase template

- fix Decode call (req. only one param)
- add constructor for base58c->CExtKey

7cb1f9f7eb

extend bip32 tests to cover Base58c/CExtKey decode 8d2af54ecc
jonasschnelli force-pushed on Jul 23, 2015
jgarzik commented at 7:01 PM on July 23, 2015: contributor

ACK
jonasschnelli commented at 7:10 PM on July 23, 2015: contributor

Added a tiny comment on top to prevent a crash when one tries to get a key from a CBitcoinExtKeyBase instance where no base58 decoded data is available.
in src/base58.h:None in 93efc57c60 outdated
```
 145 | @@ -146,7 +146,8 @@ template<typename K, int Size, CChainParams::Base58Type Type> class CBitcoinExtK
 146 |  
 147 |      K GetKey() {
 148 |          K ret;
 149 | -        ret.Decode(&vchData[0], &vchData[Size]);
 150 | +		if (vchData.size() == 74) //if base58 encouded data not holds a ext key, return a !IsValid() key
```
Diapolo commented at 6:01 AM on July 24, 2015:

Nit: Indentation and space after begin of comment.

laanwj commented at 8:43 AM on July 24, 2015:

I'm not sure that a fixed size check is a robust solution here (also, hardwiring 74 without any kind of constant) - doesn't the underlying decode function return an error if it couldn't decode?

laanwj commented at 8:47 AM on July 24, 2015:

OK I understand now why. ret.Decode takes a fixed-size object of 74 bytes. In that case, please define a constant.

jonasschnelli commented at 8:52 AM on July 24, 2015:

I didn't add a constant because fixing this is already covered in: #6215. The Decode really expects 74 bytes (=hardcode check okay IMO)
laanwj added the label Bug on Jul 24, 2015
jonasschnelli force-pushed on Jul 24, 2015
jonasschnelli commented at 8:54 AM on July 24, 2015: contributor

Force push fixed @Diapolo nit.
jonasschnelli force-pushed on Jul 24, 2015

in src/base58.h:None in 5c6a28cb0b outdated

 145 | @@ -146,7 +146,8 @@ template<typename K, int Size, CChainParams::Base58Type Type> class CBitcoinExtK
 146 |  
 147 |      K GetKey() {
 148 |          K ret;
 149 | -        ret.Decode(&vchData[0], &vchData[Size]);
 150 | +        if (vchData.size() == Size) //if base58 encouded data not holds a ext key, return a !IsValid() key

jonasschnelli commented at 11:27 AM on July 24, 2015:

replaced the magic 74 with the template var Size

sipa commented at 5:57 PM on July 24, 2015: member

ACK, apart from style nit.
Diapolo commented at 7:01 PM on July 24, 2015: none

@sipa Let me ask, what is happening with our clang-format cleanup ;)?
don't try to decode invalid encoded ext keys 6f8b6d339b
jonasschnelli force-pushed on Jul 25, 2015
jonasschnelli commented at 7:52 AM on July 25, 2015: contributor

Fixed @sipa 's nit.
laanwj merged this on Jul 27, 2015
laanwj closed this on Jul 27, 2015
laanwj referenced this in commit d43297c5ba on Jul 27, 2015
luke-jr referenced this in commit 2eebadd481 on Jan 9, 2016
luke-jr referenced this in commit 18f320e280 on Jan 9, 2016
luke-jr referenced this in commit f38aea67f9 on Jan 9, 2016
luke-jr referenced this in commit 7ea78632c4 on Jan 9, 2016
luke-jr referenced this in commit 59d8614054 on Jan 10, 2016
luke-jr referenced this in commit 1157680fea on Jan 10, 2016
luke-jr referenced this in commit 04962bb6ec on Jan 10, 2016
luke-jr referenced this in commit 95bd773a64 on Jan 10, 2016
zkbot referenced this in commit df07f9ad23 on Feb 15, 2017
zkbot referenced this in commit ec069ce96c on Mar 3, 2017
zkbot referenced this in commit 702eefc71a on Mar 3, 2017
zkbot referenced this in commit 99c4c6de0c on Mar 3, 2017
random-zebra referenced this in commit 961e2d2a70 on Jun 27, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels