Skip to content

fix CBitcoinExtKeyBase template #6468

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 27, 2015
Merged

fix CBitcoinExtKeyBase template #6468

merged 3 commits into from
Jul 27, 2015

Conversation

jonasschnelli
Copy link
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 jonasschnelli force-pushed the 2015/07/base58c_extkey_fixes branch from 0739a38 to c6ec07f Compare July 23, 2015 14:31
@jonasschnelli
fix and extend CBitcoinExtKeyBase template
7cb1f9f 
- fix Decode call (req. only one param)
- add constructor for base58c->CExtKey
@jonasschnelli
extend bip32 tests to cover Base58c/CExtKey decode
8d2af54
@jonasschnelli jonasschnelli force-pushed the 2015/07/base58c_extkey_fixes branch from c6ec07f to 8d2af54 Compare July 23, 2015 14:32
@jgarzik
Copy link
Contributor

jgarzik commented Jul 23, 2015

ACK

@jonasschnelli
Copy link
Contributor Author

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.

Diapolo
Diapolo reviewed Jul 24, 2015
View reviewed changes
src/base58.h
@@ -146,7 +146,8 @@ template<typename K, int Size, CChainParams::Base58Type Type> class CBitcoinExtK

K GetKey() {
K ret;
ret.Decode(&vchData[0], &vchData[Size]);
if (vchData.size() == 74) //if base58 encouded data not holds a ext key, return a !IsValid() key
Copy link

Choose a reason for hiding this comment

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

Nit: Indentation and space after begin of comment.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 laanwj added the Bug label Jul 24, 2015
@jonasschnelli jonasschnelli force-pushed the 2015/07/base58c_extkey_fixes branch from 93efc57 to 5c46b2b Compare July 24, 2015 08:54
@jonasschnelli
Copy link
Contributor Author

Force push fixed @Diapolo nit.

@jonasschnelli jonasschnelli force-pushed the 2015/07/base58c_extkey_fixes branch from 5c46b2b to 5c6a28c Compare July 24, 2015 11:21
jonasschnelli
jonasschnelli reviewed Jul 24, 2015
View reviewed changes
src/base58.h
@@ -146,7 +146,8 @@ template<typename K, int Size, CChainParams::Base58Type Type> class CBitcoinExtK

K GetKey() {
K ret;
ret.Decode(&vchData[0], &vchData[Size]);
if (vchData.size() == Size) //if base58 encouded data not holds a ext key, return a !IsValid() key
Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced the magic 74 with the template var Size

@sipa
Copy link
Member

sipa commented Jul 24, 2015

ACK, apart from style nit.

@Diapolo
Copy link

Diapolo commented Jul 24, 2015

@sipa Let me ask, what is happening with our clang-format cleanup ;)?

@jonasschnelli jonasschnelli force-pushed the 2015/07/base58c_extkey_fixes branch from 5c6a28c to 6f8b6d3 Compare July 25, 2015 07:52
@jonasschnelli
Copy link
Contributor Author

Fixed @sipa 's nit.

@laanwj laanwj merged commit 6f8b6d3 into bitcoin:master Jul 27, 2015
laanwj added a commit that referenced this pull request Jul 27, 2015
@laanwj
Merge pull request #6468
d43297c 
6f8b6d3 don't try to decode invalid encoded ext keys (Jonas Schnelli)
8d2af54 extend bip32 tests to cover Base58c/CExtKey decode (Jonas Schnelli)
7cb1f9f fix and extend CBitcoinExtKeyBase template (Jonas Schnelli)
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jan 9, 2016
@jonasschnelli @luke-jr
fix CBitcoinExtKeyBase template
2eebadd 
- fix Decode call (req. only one param)

Github-Pull: bitcoin#6468
Partial-Rebased-From: 7cb1f9f
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jan 9, 2016
@jonasschnelli @luke-jr
fix and extend CBitcoinExtKeyBase template
18f320e 
- fix Decode call (req. only one param)
- add constructor for base58c->CExtKey

Github-Pull: bitcoin#6468
Rebased-From: 7cb1f9f
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jan 9, 2016
@jonasschnelli @luke-jr
extend bip32 tests to cover Base58c/CExtKey decode
f38aea6 
Github-Pull: bitcoin#6468
Rebased-From: 8d2af54
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jan 9, 2016
@jonasschnelli @luke-jr
don't try to decode invalid encoded ext keys
7ea7863 
Github-Pull: bitcoin#6468
Rebased-From: 6f8b6d3
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jan 10, 2016
@jonasschnelli @luke-jr
fix CBitcoinExtKeyBase template
59d8614 
- fix Decode call (req. only one param)

Github-Pull: bitcoin#6468
Partial-Rebased-From: 7cb1f9f
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jan 10, 2016
@jonasschnelli @luke-jr
fix and extend CBitcoinExtKeyBase template
1157680 
- fix Decode call (req. only one param)
- add constructor for base58c->CExtKey

Github-Pull: bitcoin#6468
Rebased-From: 7cb1f9f
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jan 10, 2016
@jonasschnelli @luke-jr
extend bip32 tests to cover Base58c/CExtKey decode
04962bb 
Github-Pull: bitcoin#6468
Rebased-From: 8d2af54
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jan 10, 2016
@jonasschnelli @luke-jr
don't try to decode invalid encoded ext keys
95bd773 
Github-Pull: bitcoin#6468
Rebased-From: 6f8b6d3
@str4d str4d mentioned this pull request Feb 15, 2017
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Mar 3, 2017
@zkbot
Auto merge of #2099 - str4d:2074-misc, r=arcalinea
99c4c6d 
Bitcoin 0.12 misc PRs 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6198
- bitcoin/bitcoin#6206
- bitcoin/bitcoin#5927
- bitcoin/bitcoin#6213
- bitcoin/bitcoin#6061
- bitcoin/bitcoin#6283 (partial, remainder was pulled in #929)
- bitcoin/bitcoin#6272
- bitcoin/bitcoin#6316
- bitcoin/bitcoin#6133
- bitcoin/bitcoin#6387
- bitcoin/bitcoin#6401
- bitcoin/bitcoin#6434
- bitcoin/bitcoin#6372
- bitcoin/bitcoin#6447
- bitcoin/bitcoin#6149
- bitcoin/bitcoin#6468

Part of #2074.
@dagurval dagurval mentioned this pull request Dec 28, 2017
Merged
@furszy furszy mentioned this pull request Jun 11, 2020
Merged
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Jun 27, 2020
@random-zebra
Merge #1676: [Backport] Base58 performance improvements
961e2d2 
aa633c8 CBase58Data::SetString: cleanse the full vector (Kaz Wesley)
0b77f8a Improve readability of DecodeBase58Check(...) (practicalswift)
dadadee Use prefix operator in for loop of DecodeBase58. (Jiaxing Wang)
305c382 base58: Improve DecodeBase58 performance. (Jiaxing Wang)
3cb418b Improve EncodeBase58 performance (João Barbosa)
4d17f71 don't try to decode invalid encoded ext keys (Jonas Schnelli)
eef4ec6 extend bip32 tests to cover Base58c/CExtKey decode (furszy)
7239aaf fix and extend CBitcoinExtKeyBase template - fix Decode call (req. only one param) - add constructor for base58c->CExtKey (furszy)
13f09c3 remove unused inv from ConnectTip() (furszy)

Pull request description:

  Coming from:

  * bitcoin#6468 .
  * bitcoin#7656 .
  * bitcoin#7922
  * bitcoin#8736 .
  * bitcoin#10961 .

ACKs for top commit:
  random-zebra:
    ACK aa633c8
  Fuzzbawls:
    ACK aa633c8

Tree-SHA512: 3add3106a847b0b3d768df2c4ab5eae7d009e3820998fb5a4cd274169c64622e83ecd14dca51c31f3f6053199834129a1c6920b7ef1193632339297a041173d6
@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.
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jonasschnelli @jgarzik @sipa @Diapolo @laanwj