Skip to content

Use CPrivKey typedef for keydata in CKey#11482

Closed
AmirAbrams wants to merge 1 commit intobitcoin:masterfrom
AmirAbrams:patch-3
Closed

Use CPrivKey typedef for keydata in CKey#11482
AmirAbrams wants to merge 1 commit intobitcoin:masterfrom
AmirAbrams:patch-3

Conversation

@AmirAbrams
Copy link
Copy Markdown
Contributor

@AmirAbrams AmirAbrams commented Oct 11, 2017

Seems like the keydata member variable in CKey should be a CPrivKey type. Is there a reason it was re-declared as a std::vector<unsigned char, secure_allocator<unsigned char> > and the CPrivKey typedef isn't used?

@promag
Copy link
Copy Markdown
Contributor

promag commented Oct 11, 2017

When keydata was added the typedef was already there, see f4d1fc2#diff-dd01571c9de4333a6af0b54c71ae2ad8R32.

ACK 21f69f6.

@sipa
Copy link
Copy Markdown
Member

sipa commented Oct 11, 2017

CPrivKey was intended to be a type for DER-encoded private keys, which the contents of CKey is not.

If you're changing CPrivKey to be "generic secure storage of bytes", please update the comment above CPrivKey as well.

- keydata uses a secure generic storage of bytes via secure_allocator
@AmirAbrams
Copy link
Copy Markdown
Contributor Author

AmirAbrams commented Oct 12, 2017

@sipa, I updated the comment to //! A generic storage of bytes using secure allocators
Is that what you had in mind? Thanks

On a related subject, in some cases, it might be better to use an alias instead of a typedef because they can be used with templates: Stack Overflow typedef and using in C++11
typedef std::vector<unsigned char, secure_allocator<unsigned char> > CPrivKey;

to

using CPrivKey = std::vector<unsigned char, secure_allocator<unsigned char> >;

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Nov 9, 2017

Is that what you had in mind? Thanks

He means the comment above CPrivKey here: https://github.com/bitcoin/bitcoin/blob/master/src/key.h#L30
This specifically mentions a size and encoding, which is no longer true if you use it as a generic typedef.

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Dec 20, 2017

Closing this for now, feel free to ping me if you want to pick this up again.

@laanwj laanwj closed this Dec 20, 2017
@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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants