It is not immediately apparent what 'err == 1' means, one must determine
that the FFI function call returns 1 for success. We can help readers of
the code by adding a 'Return' section to the method documentation.
Add trailing full stop to method docs initial line also.
Two functions in the FFI secp code return and accept a parity int.
Currently we are manually converting this to a bool. Doing so forces
readers of the code to think what the bool means even though
understanding this bool is not needed since in is just passed back down
to the FFI code. We can abstract this away by using an opaque type to
hold the original int and not converting it to a boolean value.
Add 'Return' and 'Error' sections to `tweak_add_assign` while fixing the
docs to describe the new opaque parity type.
We have two `tweak_add_assign` methods (one for keypair and one for
x-only pubkey). Both check the return value from a FFI function call.
We can make both sites uniform to _slightly_ reduce cognitive load when
reading the code.
Use C style code to make it obvious to readers that this is basically C
code.
There are currently two unit tests in the `schnorr` module that are
testing keys from the `key` module. This is possible because the tests
are only testing the public interface, none the less they are better
placed in the `key` module.
The import statements can be simplified by using an import
wildcard (`super::*`). While we are at it put them in std, external
crate, this crate order.
21aa914ad2 Change context objects for schnorr sig methods (sanket1729)
Pull request description:
- The current schnorrsig verify methods should operate on verify context
as is done throughout the bitcoin core
- Finally, and importantly the XonlyPublicKey::from_keypair now operates
without any context parameter.
ACKs for top commit:
apoelstra:
ACK 21aa914ad2
Tree-SHA512: 035338f19839805a080eb262ae7b93ab187dabb63086c8b7f6015f3a6006986604dc2c6f329a99a20ddfa78c1ee518f44cd5eee2f73810fbdc83ff8df7d12506
- The current schnorrsig verify methods should operate on verify context
as is done throughout the bitcoin core
- Scondly, and importantly the XonlyPublicKey::from_keypair now operates
without any context objects.
The public key is unrelated to the signature algorithm. It will
be moved out of the module in another commit. For ease of review,
the renamed is kept separate.
With the introduction of Schnorr signatures, exporting a `Signature`
type without any further qualification is ambiguous. To minimize the
ambiguity, the `ecdsa` module is public which should encourage users
to refer to its types as `ecdsa::Signature` and `ecdsa::SerializedSignature`.
To reduce ambiguity in the APIs on `Secp256k1`, we deprecate several
fucntions and introduce new variants that explicitly mention the use of
the ECDSA signature algorithm.
Due to the move of `Signature` and `SerializedSignature` to a new module,
this patch is a breaking change. The impact is minimal though and fixing the
compile errors encourages a qualified naming of the type.
24d6f62603 Use explicit u8 when assigning a byte slice (junderw)
Pull request description:
Is there a way to tell the compiler to not allow `[0; 64]` and require that either the type is explicitly given to the variable, or that each member uses explicit `0u8` notation?
I noticed the usage was a mix of explicit and implicit, so I changed all to explicit.
ACKs for top commit:
apoelstra:
ACK 24d6f62603
Tree-SHA512: f7796dcc3ae240983257bef0f25bd0df741943f75d86e9bca7c45076af179d96ce213bd9c339a01f721f7dc9b96a0a4a56ef2cf44339f4c91d208103b7659d9f
bc42529a16 Rename `secp256k1::bitcoin_hashes` module to `secp256k1::hashes` (Thomas Eizinger)
ae1f8f4609 Bump bitcoin_hashes to version 0.10 (Thomas Eizinger)
Pull request description:
Requires for interoperability of the `ThirtyTwoByteHash` trait with
rust-bitcoin.
ACKs for top commit:
apoelstra:
ACK bc42529a16
Tree-SHA512: 85fcb284ff82b543a0c3ea2b568351b3af938a26ac42c6a975480ae97def84e4f0795105bd4572f930a7bf82654eba416cf0c5e25f62809e2ea331443ffb5807
This reduces the usage of real cryptography in --cfg=fuzzing,
specifically replacing the secret->public key derivation with a
simple copy and ECDH with XOR of the public and private parts
(plus a stream of 1s to make a test pass that expected non-0
output).
It leaves secret tweak addition/multiplication as-is.
It also changes the context creation to over-allocate and store
the context flags at the end of the context buffer, allowing us
to easily test context flags in each function.
While it would be nice to have something fancier (eg XOR-based),
its not immediately obvious how to accomplish this, and better to
fix the issues I have than spend too much time on it.
Fixes#271.
This partially reverts b811ec133a
In the next commit the secret->public key derivation in fuzzing cfg
is changed to be simpler, as well as the validity rules of public
keys relaxed.
This adds a new test to ensure random keys can be added, not just
the hard-coded keys test that exists today.
Panicking from C is not UB in newer rust versions and will reliably
trigger an abort (without unwinding). In older rust versions, it is
technically UB but empirically it seems to "just work" (and what should
it realistically do except crashing, which is what we intent).
Since there's potentially no unwinding, we can't test this behavior
using [should_panic]. This PR will instead check the libtest output
explicitly in our CI tests.
Fixes#228.
There is little reason to pull in the `rand` dep just for the `Rng`
trait for users who want to randomize contexts. We should expose a
randomize function that just takes 32 bytes.
We can now run unit tests with the fuzz feature on, and they'll pass,
which is some assurance that fuzzing with the feature on won't lead to
spurious failures due to the fuzz harness inadequately simulating message
signing.
Currently we are misusing `map` on an iterator to loop `n` times,
additionally the assertion is pointless. Use a for loop and assert
against the length of the set.
Instead of repeating ourselves in defining one big test for the wasm
target, we can override the `test` attribute with the `wasm-bindgen-test`
one and therefore automatically run all (supported) tests in wasm.
Unfortunately, wasm doesn't support catching panics yet which means we
have to disable the `test_panic_raw_ctx` test.
The interfaces for negate should always returns 1 as mentioned secp256k1.h L574, L563.
But in the future it might return 0 if the seckey or pubkey is invalid, but our type system doesn't allow that to ever happen.
libsecp256k1 really only barely uses libc at all, and in practice,
things like memcpy/memcmp get optimized into something other than a
libc call. Thus, if we provide simple stub headers, things seem to
work with wasm-pack just fine.
We would not want to use these functions internally because we rely on
USE_EXTERNAL_DEFAULT_CALLBACKS to provide the callbacks at link time,
see f7a4a7ef57. Moreover, we would not
want to export the functions either.
Tobin Harding
sanket1729
Andrew Poelstra
Dr Maxim Orlovsky
Thomas Eizinger
junderw
Elichai Turkel
Tibo-lg
Matt Corallo
Devrandom
Tim Ruffing
Justin Moon
Sebastian Geisler
Kohei Taniguchi
Kirill Fomichev
Wei Tang
Steven Roose
Tony Jin
Rodolfo Araujo