Skip to content
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.

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

Split secp256k1_ec_pubkey_decompress into in-place and copy variants #250

Merged
merged 1 commit into from
Jun 13, 2015

Conversation

apoelstra
Copy link
Contributor

Right now secp256k1_ec_pubkey_decompress takes an in/out pointer to
a public key and replaces the input key with its decompressed variant.
This forces users who store compressed keys in small (<65 byte) fixed
size buffers (for example, the Rust bindings do this) to explicitly
and wastefully copy their key to a larger buffer.

Add a variant secp256k1_ec_pubkey_decompress_copy which takes an
in-pointer and an out-pointer for the public key.

@apoelstra
Copy link
Contributor Author

@gmaxwell suggested I check the rest of the API for cases like this; I did, it is the only one that takes an in/out pointer to a byte buffer.

@dcousens
Copy link
Contributor

ACK

@@ -275,6 +275,24 @@ SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_decompress(
int *pubkeylen
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);

/** Decompress a public key.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just replace the old function.

@gmaxwell
Copy link
Contributor

I'm largely ambivalent but I suggested otherwise because most people will not assume a writable parameter can alias.

@apoelstra
Copy link
Contributor Author

My thoughts were the same as @gmaxwell's. For the sake of a cleaner API I think I lean toward just replacing the function, though.

If @gmaxwell ok's it I'll change the PR to do this.

@gmaxwell
Copy link
Contributor

I certainly don't mind; just wanted sipa to be aware of that argument, if he wasn't in case it changed his thinking.

@ghost
Copy link

ghost commented May 13, 2015

Looks good (Tested ACK), I'll update my JNI PR relative to this once the API is finalized here, too (FWIW I was having trouble the other day with this due to not realizing the buffer had to be large enough to hold the expanded pubkey if the buffer is != 65 bytes going in, this makes the (somewhat obvious) assumption more apparent).

@apoelstra apoelstra force-pushed the decompress-split branch 2 times, most recently from 10105fc to ba46488 Compare May 13, 2015 21:59
@apoelstra
Copy link
Contributor Author

Updated PR to just replace the function.

@gmaxwell
Copy link
Contributor

One might wonder how one can decompress uninitialized memory. :) I think that part of the comment is a bit weird and you can probably drop it; in C I've never worried that my output needed to be initialized.

@apoelstra
Copy link
Contributor Author

Sure, changed :)

Right now `secp256k1_ec_pubkey_decompress` takes an in/out pointer to
a public key and replaces the input key with its decompressed variant.
This forces users who store compressed keys in small (<65 byte) fixed
size buffers (for example, the Rust bindings do this) to explicitly
and wastefully copy their key to a larger buffer.

[API BREAK]
@gmaxwell
Copy link
Contributor

utACK.

@ghost ghost mentioned this pull request May 15, 2015
@sipa sipa merged commit 210ffed into bitcoin-core:master Jun 13, 2015
sipa added a commit that referenced this pull request Jun 13, 2015
210ffed Use separate in and out pointers in `secp256k1_ec_pubkey_decompress` (Andrew Poelstra)
@apoelstra apoelstra deleted the decompress-split branch June 19, 2017 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants