Currently we serialize keys using the `BytesVisitor`, this causes the
serialized data to contain additional metadata encoding the length (an
extra 8 bytes) when serialized with the `bincode` crate. This extra data
is unnecessary since we know in advance the length of these types.
It would be useful for users of the lib to be able to get a fixed width
binary serialization, this can be done but it depends on the crate used
to do the serialization. We elect to optimise for `bincode` and add docs
noting that other binary serialization crates may differ (rustdocs added
in separate patches).
Implement a tuple based visitor that encodes the keys as fixed width
data.
Do fixed width serde implementations for:
- `SecretKey`
- `PublicKey`
- `KeyPair`
- `XOnlyPublicKey`
5d2f1ceb64 Fix WASM build (Elichai Turkel)
39aaac6834 Use new trait TryFrom and do small refactoring (Elichai Turkel)
7d3a149ca5 Move more things from the std feature to the alloc feature (Elichai Turkel)
bc8c713631 Replace c_void with core::ffi::c_void (Elichai Turkel)
26a52bc8c8 Update secp256k1-sys to edition 2018 and fix imports (Elichai Turkel)
ebe46a4d4e Update rand to 0.8 and replace CounterRng with mock::StepRng (Elichai Turkel)
626835f540 Update secp256k1 to edition 2018 and fix imports (Elichai Turkel)
67c0922a46 Update MSRV in CI and Readme from 1.29 to 1.41 (Elichai Turkel)
Pull request description:
As proposed in https://github.com/rust-bitcoin/rust-bitcoin/issues/510#issuecomment-881686342 this PR raises the MSRV to 1.41.1 it also changes the code to be Edition 2018.
The PR contains a few things:
* Moving to edition 2018 and fixing the imports
* Sorting and combining imports to make them more concise
* Replacing our c_void with `core::ffi::c_void`
* Bumping the `rand` version to latest and modifying our `RngCore` implementations accordingly
* Doing some small refactoring and using the new `TryInto` trait where it makes the code nicer
If people prefer I can split this PR into multiple and/or drop some commits
ACKs for top commit:
tcharding:
ACK 5d2f1ceb64
apoelstra:
ACK 5d2f1ceb64
Tree-SHA512: 5bf84e7ebb6286d59f8cada0bb712c46336f0dd6c35b67e6f4ba323b5484ad925b99b73e778ae4608f123938e7ee8705a0aec576cd9c065072c4ecf1248e3470
ef7f1972a7 Derive Hash for Signature (Tobin C. Harding)
Pull request description:
In preparation for deriving `Hash` in miniscript, derive `Hash` on the `ecdsa::Signature`.
ref: https://github.com/rust-bitcoin/rust-miniscript/issues/226
ACKs for top commit:
apoelstra:
ACK ef7f1972a7
elichai:
ACK ef7f1972a7
Tree-SHA512: 7313f59971444ae18611adbafe86a09478eddd7357f2b7f3ad3bb1761609b6358b156975086f6c318eb2777018b7b2f44386321108939acbcf2d0a522e7e208e
f08276adfc Add convenience methods for keys (Tobin Harding)
b4c7fa0d4e Let the compiler work out int size (Tobin Harding)
c612130864 Borrow secret key (Tobin Harding)
Pull request description:
We have a bunch of `from_<key>` methods for converting between key types. To make the API more ergonomic to use we can add methods that do the same but called on a variable e.g., once applied the following are equivalent:
- `let pk = PublicKey::from_keypair(kp)`
- `let pk = kp.public_key()`
Do this for `SecretKey`, `PublicKey`, `KeyPair`, and `XOnlyKeyPair`.
Fixes: #428
### Note to reviewers
- `XOnlyPublicKey` -> `PublicKey` logic is made up by me, I could not work out how to get `libsecp256k1` to do this.
- Please review the tests carefully, they include assumptions based on my current understanding of the cryptography :)
ACKs for top commit:
sanket1729:
ACK f08276adfc. Thanks for going through all the iterations.
apoelstra:
ACK f08276adfc
Tree-SHA512: 1503a6e570a3958110c6f24cd6d075fe5694b3b32b91a7a9d332c63aa0806198ff10bdd95e7f9de0cf73cbf4e3655c6826bd04e5044d1b019f551471b187c8ea
676a9800df Remove unnecessary panic message (sanket1729)
aa50cc6ced Remove Schnorr word from keypairs (sanket1729)
Pull request description:
Keypairs are pair of EC points that don't have anything to do with the
signature algorithm
ACKs for top commit:
apoelstra:
ACK 676a9800df
tcharding:
ACK 676a9800df
Tree-SHA512: ed3e6f5e821d18641234b308b130271dcd2ec0dd6519a0e9d91564ab8e902b82180d7df377f2bcf08cd3ca1df7ce775422e4a3c386637eaff348e58b033de3ea
We have a bunch of `from_<key>` methods for converting between key types.
To improve the API and make it more ergonomic to use we can add methods
that do the same but can be called on the initial key instead of on the
resulting key's type. E.g. once applied the following are equivalent:
- `let pk = PublicKey::from_keypair(kp)`
- `let pk = kp.public_key()`
Do this for `SecretKey`, `PublicKey`, `KeyPair`, and `XOnlyKeyPair`.
We have two places in the code where we pass a mutable parity integer
to ffi code. At one callsite we tell the compiler explicitly what type
it is (`::secp256k1_sys::types::c_int`) and at the other call site we
let the compiler figure out the type.
Is one way better than the other? I don't know. But letting the compiler
figure it out seems to make the code easier to read.
`SecretKey` implements `Copy` and it is fine to take owneship of it; we
have multiple methods called `from_secret_key` and they all borrow the
secret key parameter. Favour consistency over perfection.
Borrow secret key parameter as is done in other `from_secret_key`
methods.
As we do for other keys implement serde de/serialization for the
`SharedSecret`. Includes implementation of `from_slice` method that is
the borrowed version of `from_bytes` as well as a `FromStr`
implementation that parses a hex string.
Recently we moved from using the identifier 'schnorrsig' to 'schnorr',
we omitted to update the tests.
While we are at it use more idiomatic Rust unit test names (i.e., do not
start test name with `test_` because it stutters when the name is read
in output of `cargo test`).
Recently we moved from using the identifier 'schnorrsig' to 'schnorr',
we omitted to update the schnorr signature size constant.
Deprecate `SCHNORRSIG_SIGNATURE_SIZE` and add
`SCHONORR_SIGNATURE_SIZE`.
Recently we moved from using the identifier 'schnorrsig' to 'schnorr',
we omitted to update the schnorr public key size constant.
Deprecate `SCHNORRSIG_PUBLIC_KEY_SIZE` and add
`SCHONORR_PUBLIC_KEY_SIZE`.
5acf6d23d3 `Parity` conversion and error handling cleanup (Martin Habovstiak)
Pull request description:
This removes the deprecated `From` conversion, replaces it with
`TryFrom`, and adds more convenience conversions. A new error type is
created for the invalid parity error with conversion to catch-all
`Error`.
This is intended for an API-breaking version.
ACKs for top commit:
apoelstra:
ACK 5acf6d23d3
Tree-SHA512: 49b73fc90455c172012b46f36eafa7d256b940f4b431b4eedb577ab07d9402eae40af931e00b3c409bbe502dbcac064a742e874a5e8bedd8d0cbe92a468ae4f6
Currently printing the `SharedSecret` using `Display` or `Debug` prints
the real secret, this is sub-optimal. We have a solution for other
secrets in the project where printing is obfuscated and we provide a
`display_secret` method for explicitly printing.
Mirror the logic for other secrets and obfuscate the `SharedSecret` when printing.
Improve rustdocs on `display_secret` by doing:
- Minor improvements to the rustdocs to aid readability in the editor.
- Do not guarantee (`assert_eq!`) debug output
The `serialize_secret` method is a getter method, it does not do any
serialisation. However we use the method on secret keys and key types so
in order for the name to be uniform use the descriptive name
`secret_bytes`.
Rename `serialize_secret` to be `secret_bytes`.
The identifier `i` is predominantly used for indexing an array but we
are using it as a place holder for the iterated value of an array that
is then printed. The identifier `byte` is more descriptive.
Done in preparation for adding similar code to the `ecdh` module.
In array initialisation we use magic number 64, this is the secret bytes
length multiplied by 2.
Please note; we still use the magic number 32, left as such because it
is used in various ways and its not immediately clear that using a
single const would be any more descriptive.
Use `SECRET_KEY_SIZE * 2` instead of magic number 64.
Hashing the debug output for secrets can be done with `bitcoin_hashes`
not just `std`. Mention this in the obfuscated string output when
neither are available.
5603d71ad3 Limit SharedSecret to 32 byte buffer (Tobin Harding)
d5eeb099ad Use more intuitive local var numbering (Tobin Harding)
834f63c26c Separate new_with_hash into public function (Tobin Harding)
Pull request description:
Currently `SharedSecret` provides a way to get a shared secret using SHA256 _as well as_ a way to use a custom hash function to get the shared secret. Internally `SharedSecret` uses a 256 byte buffer, this is a tad wasteful. We would like to keep the current functionality but reduce memory usage.
- Patch 1: Pulls the `new_with_hash` logic out into a standalone public function that just returns the 64 bytes representing the x,y co-ordinates of the computed shared secret point. Callers are then responsible for hashing this point to get the shared secret (idea by @Kixunil, thanks).
- Patch 2: Does trivial refactor
- Patch 3: Uses a 32 byte buffer internally for `SharedSecret`. This is basically a revert of the work @elichai did to add the custom hashing logic. @elichai please holla if you are not happy with me walking all over this code :)
### Note to reviewers
Secret obfuscation is done on top of this in https://github.com/rust-bitcoin/rust-secp256k1/pull/396, they could be reviewed in order if this work is of interest to you.
ACKs for top commit:
apoelstra:
ACK 5603d71ad3
Tree-SHA512: 48982a4a6a700a111e4c1d5d21d62503d34f433d8cb303d11ff018d2f2be2467fa806107018db16b6d0fcc5ff1a0325dd5790c62c47831c7cd2141a1b6f9467d
e6cb588a23 Breaking: changed `Parity` serialization to `u8` (Martin Habovstiak)
Pull request description:
Serializing the value as `u8` is more compact but this is a breaking
change.
`Visitor` was renamed to avoid hungarian notation and maybe allow other
integers in the future.
For next major version, depends on #400
ACKs for top commit:
tcharding:
tACK e6cb588
apoelstra:
ACK e6cb588a23
Tree-SHA512: 1432a2f3c913c3a7eaec5228fd2dd4e8320d828128bec71812cbf56dd8950c969ed22c69867402eb9e820127868d29b291f3374c6e15de0a3ff2341420c4bbab
The `SharedSecret` uses sha256 to hash the secret, this implies the
secret is 32 bytes of data.
Currently we use a buffer of 256 bytes, this is unnecessary.
Change the implementation of `SharedSecret` to use a 32 byte buffer.
In test code we use multiple pub/sec keys. It is more intuitive if the
'secret 1' is generated by the owner of secret key 1.
Refactor only, no logic changes.
In preparation for simplifying the `SharedSecret` internals pull the
`new_with_hash` function logic out into a standalone public function
that provides similar functionality without use of the `SharedSecret`
struct. Function now returns the 64 bytes of data representing a shared
point on the curve, callers are expected to the hash these bytes to get
a shared secret.
We are using `cfg_attr` to instruct the rustdocs build system to
highlight feature requirements for functions, there is no need to
explicitly mention feature requirements in the text.
Recently we fixed a bunch of feature gates to use `rand-std` instead
of `rand` but in doing so did not notice that the same feature gates
were using `alloc` which is meaningless if `std` is enabled.
Feature gate on `std` if we are using `rand-std`.
c73eb2f391 Use 'extra' instead of 'cheap' (Tobin Harding)
c79eb976ca Remove unnecessary explanation (Tobin Harding)
f95e91a6da Use isn't instead of shouldn't (Tobin Harding)
c9e6ca1680 Use rust-bitcoin module doc style (Tobin Harding)
3fa6762437 Add link to referenced commit (Tobin Harding)
f5e68f3ba7 Add ticks around code snippet (Tobin Harding)
d25431c1da Use 3rd person tense for function docs (Tobin Harding)
c3be285c1d Fix size constant docs (Tobin Harding)
5e07e7596b Add period to sentences (Tobin Harding)
269bde042f Remove unnecessary capitalisation (Tobin Harding)
Pull request description:
In a continued effort to find my feet around here, and inspired by issue #128 I've done a codebase wide audit of the docs (primarily just rustdocs but I glanced at `//` docs as well). Each change is in a separate commit so can be removed if resistance is met. (_"resistance is futile"_).
I've based the stylistic decisions on [work done](https://github.com/rust-bitcoin/rust-bitcoin/pull/704) in rust-bitcoin.
I believe the only controversial change is the last (commit: da161c9 Use rust-bitcoin module doc style), please review that one carefully.
ACKs for top commit:
apoelstra:
ACK c73eb2f391
Tree-SHA512: 5ea215de3fd23ca2a4f25d8f8d59a85a299044fe495269c43b621291ea50c58856fa8544e36cc109b7bdb1a7a59bcab8711f30113572ddce4509d3b06ff0d3b6
Serializing the value as `u8` is more compact but this is a breaking
change.
`Visitor` was renamed to avoid hungarian notation and maybe allow other
integers in the future.
705c9cfbc1 Clarified conversions between `Parity` and integers (Martin Habovstiak)
Pull request description:
This was discussed in https://github.com/rust-bitcoin/rust-secp256k1/pull/390#issuecomment-1033018430
ACKs for top commit:
apoelstra:
ACK 705c9cfbc1
Tree-SHA512: 3ba2ec566099c3c6d1c6f830e4959312b818b8766d924e3d995e6b23bd196ab747cc03d46f494ef451569188b0163f53e3236cacd20bfae9118ee76bcdbc9c02
4c43d5e20f Add custom Debug impl for RecoverableSignature (Tobin Harding)
Pull request description:
Currently when debug printing the `RecoverableSignature` we do so byte by byte, this means that the output differs depending on the endianess of the machine. If instead we serialize the signature in compact form then the output is the same irrespective of the endianess.
With this applied the following two commands now pass:
```
cargo test test_debug_output --features=recovery
```
```
cross test --target powerpc-unknown-linux-gnu test_debug_output --features=recovery
```
Fixes: #375
ACKs for top commit:
apoelstra:
ACK 4c43d5e20f
Tree-SHA512: 073c2e0e23ce41a2b35f1b1193b07a755b726bf565d61e6bcb23b6bdaab31ba3591f31aa92230b07f7dfc018de0401eba09a6858dc261e66dacb331355f40d76
Tobin Harding
Andrew Poelstra
Elichai Turkel
Tibo-lg
junderw
sanket1729
Dominik Spicher
Martin Habovstiak