Upgrade secp dependency #2098
Upgrade secp dependency #2098
Conversation
In preparation for updating the secp dependency to v0.28.0, which includes a change of `KeyPair` to `Keypair`, change our identifier usage to indicate that "keypair" is a single word. Deprecate the old forms.
Upgrade the `secp256k1` dependency to the newly released `v0.28.0`. FTR this includes two simple changes: - Use `Message::from_digest_slice` instead of `Message::from_slice`. - Use `secp256k1::Keypair` instead of `secp256k1::KeyPair`.
tcharding
force-pushed
the
10-02-upgrade-secp
branch
from
49b0917
to
6f30ac9
Compare
|
Do you happen to be online at the moment @clarkmoody? We are psyched to get an |
| /// Untweaked BIP-340 key pair | ||
| pub type UntweakedKeypair = Keypair; | ||
|
|
||
| /// Tweaked BIP-340 key pair | ||
| #[deprecated(since = "0.31.0", note = "use TweakedKeypair instead")] | ||
| #[allow(deprecated)] | ||
| pub type TweakedKeyPair = TweakedKeypair; |
There was a problem hiding this comment.
My 2sats here from a person that does not like type alias are: what is the reason for re-aliasing this type?
Rust docs do not do a really good job of putting documentation on the re-alias type so due to this is not doing really anything it is possible to re-evaluate the type alias here?
There was a problem hiding this comment.
We have to re-alias it so that we don't break compilation for people using the existing type.
There was a problem hiding this comment.
We have to re-alias it so that we don't break compilation for people using the existing type.
well, we are already breaking people from time to time :) but I see your point
There was a problem hiding this comment.
We are trying to minimize or eliminate this. Sometimes it's not possible. But in the case of pure renames of structs or functions, it definitely is possible :).
There was a problem hiding this comment.
yeah, my point was just that we are deprecating the value and not removing it. So maybe we can redirect the user directly to the secp type without alias if also others fine this realising confusing
There was a problem hiding this comment.
Thanks for the review @vincenzopalazzo, appreciate your input. I had play with this and rendered the docs with various comments on it. The alias does show up with a link to the original type and the deprecation is handled in a clear way also. I think this is ok.
FTR we remove deprecated stuff in the next release or two so this won't be here for long. (I think our policy is deprecate for one release cycle but I may be wrong, it might be two?).
bitcoin/src/psbt/mod.rs
Outdated
| @@ -407,7 +407,8 @@ impl Psbt { | |||
| let sighash = cache.legacy_signature_hash(input_index, spk, hash_ty.to_u32())?; | |||
| // TODO: After upgrade of secp change this to Message::from_digest(sighash.to_byte_array()). | |||
There was a problem hiding this comment.
I do not get the meaning of this TODO, maybe is outdated?
There was a problem hiding this comment.
Can we address it now that we have a rust-secp release?
There was a problem hiding this comment.
Oh nice, crazy that I missed this, it literally shows up in the diff of the upgrade. Added as a new additional patch.
We have a new API function available with recent version of `secp256k1` to create a `Message` directly from a sighash byte array. Use `Message::from_digest(sighash.to_byte_array())` to construct messages ready to sign.
|
Legend, thanks @vincenzopalazzo |
There was a problem hiding this comment.
ACK 678eee8.
At a later point, we should also provide From impls from Various sighash types into Message.
Nice idea, I put it on my simple-afternoon-tasks TODO list. |
Upgrade the
secp256k1dependency to the newly releasedv0.28.0.FTR this includes two simple changes:
Message::from_digest_sliceinstead ofMessage::from_slice.secp256k1::Keypairinstead ofsecp256k1::KeyPair.But to stay in line with the keypair change we deprecate and rename the tweaked alias.