f87dc91151 Derive Hash for Scalar (Jiri Jakes)
Pull request description:
Unless there is some safety concern I cannot see, having Scalar implementing Hash would be useful.
ACKs for top commit:
Kixunil:
ACK f87dc91151
apoelstra:
ACK f87dc91151
Tree-SHA512: 276670a3d938fade3828123547b01e38b1a3c56775fb3c545d2c392622de808cf2bf8125925c7896ee27bacd5897d88908384d8c157dc8ec7d605781475964fd
0ed5a55829 release 0.28.1 (Andrew Poelstra)
Pull request description:
We need a new rust-secp release to deal with the new rust-secp-sys release.
ACKs for top commit:
sanket1729:
utACK 0ed5a55829
Kixunil:
ACK 0ed5a55829
Tree-SHA512: 6d54acce52acc03d09272a20242e9a086783e471153d77d10f21bf8924595d43717fd85e8aa4a9b7075c93a04ad88404377e06ca5200b1c9b45c08c01d6f77a8
c1ccda18b6 secp256k1-sys: Bump version to 0.9.1 (Tobin C. Harding)
Pull request description:
In preparation for doing a point release add a changelog entry, bump the version, and update all the `0_9_0` identifiers in the vendored code.
Done so we can release the wasm-build bug fix.
Fix: #667
ACKs for top commit:
apoelstra:
ACK c1ccda18b6
Tree-SHA512: 4fda58bd2d97b73280c00f06109d4ecf913e87ecbd1e31465efe94b96589c819ffda0ae5c622428f0a7f2e09ec45033a0e683dfd954cf06369aac6b9369471c1
In preparation for doing a point release add a changelog entry, bump the
version, and update all the `0_9_0` identifiers in the vendored code.
Done so we can release the wasm-build bug fix.
7a0c60edcd secp256k1-sys: patch out checked_malloc (Andrew Poelstra)
942a0e5e2c build.rs: patch out any calls to `printf` (Andrew Poelstra)
51dab7ac04 vendor-libsecp: remove util.h patch (Andrew Poelstra)
Pull request description:
Rather than using a new patchfile, just `#define` it away. Also includes a commit which removes one of the existing patchfiles, which I discovered was out of date while auditing the others to see if they could be replaced by `#define`s. (No, they cannot.)
Fixes#660
ACKs for top commit:
tcharding:
AFAICT this is right to go, ACK 7a0c60edcd
Kixunil:
ACK 7a0c60edcd
Tree-SHA512: 83ba70b000919fb8a929804c9d5929a9929b80515f0594925d3789ef896889d3c909f9fa920bac45470611607b84f509723544fa442ff1a51eefba0de75bf68f
458712212d Improve code comments (Tobin C. Harding)
Pull request description:
These comments say what the code is doing, they add no value.
ACKs for top commit:
Kixunil:
ACK 458712212d
apoelstra:
ACK 458712212d
Tree-SHA512: 0f31d36d7dfbde5bf32d622c3ba1845d5c6989c3eff0e520b77eeee595453299551172331851176e50aef4f78eacb7033c0ff798077e44e1c63313013b073f0b
dd6bf7c10d Fix unit test import statements (Tobin C. Harding)
Pull request description:
In `lib.rs` unit tests we are getting build warnings because of how we are importing things, just import with `super::*` unconditionally and be done with it.
This patch is the only good one out of #661.
ACKs for top commit:
apoelstra:
ACK dd6bf7c10d
Kixunil:
ACK dd6bf7c10d
Tree-SHA512: 3970f4c1374ec6de4798bfb52b561e9ac4611ec3a3885edc79639566f777e1fbb502cb36fa7abd015f3fd4a9ca4b6a4931b4ecb2e629e967b4e49391db97a97f
This patch adds a declaration of the `ecdsa_parse_compact` function to
util.h. This function isn't called from within libsecp proper; it is
called in lax_der_parse.c (which we patch separately with a declaration)
and in example code (which we don't compile at all).
In `lib.rs` unit tests we are getting build warnings because of how we
are importing things, just import with `super::*` unconditionally and be
done with it.
e9e17a0039 Check for changes to the public API (Tobin C. Harding)
Pull request description:
We would like to get to a stage where we can commit to the public API. To help us achieve this add a script that generates the public API and checks it against three committed files, one for each feature set: no features, alloc, std.
The idea is that with this applied any PR that changes the public API should include a final patch that is just the changes to the api/*.txt files, that way reviewers can discuss the changes without even needing to look at the code, quickly giving concept ACK/NACKs. We also run the script in CI to make sure we have not accidentally changed the public API so that we can be confident that don't break semver during releases. The script can also be used to diff between two release versions to get a complete list of API changes, useful for writing release notes and for users upgrading.
There is a development burden involved if we apply this patch.
ACKs for top commit:
apoelstra:
ACK e9e17a0039
Tree-SHA512: 94a2cedb132db457b67b3c60cde8843d9db1d2bc8dba0530cd5c518ebed955bd66a1649c61e0cb96b6f293ce6b9b0395582877ce9f1de003e0020a66100d172f
We would like to get to a stage where we can commit to the public API.
To help us achieve this add a script that generates the public API and
checks it against three committed files, one for each feature set: no
features, alloc, std.
The idea is that with this applied any PR that changes the public API
should include a final patch that is just the changes to the api/*.txt
files, that way reviewers can discuss the changes without even needing
to look at the code, quickly giving concept ACK/NACKs. We also run the
script in CI to make sure we have not accidentally changed the public
API so that we can be confident that don't break semver during releases.
The script can also be used to diff between two release versions to get
a complete list of API changes, useful for writing release notes and for
users upgrading.
There is a development burden involved if we apply this patch.
3dc5b16540 Bump version to v0.28.0 (Tobin C. Harding)
Pull request description:
Bump the version of `secp256k1` ready for release.
Includes changelog for the already-bumped `secp256k1-sys`, changelog for `secp256k1`, and the version bump for `secp256k1`.
ACKs for top commit:
apoelstra:
ACK 3dc5b16540
Tree-SHA512: 404a5512f7bb245cde12d11eb20f2e7bedfa5a8d4124101e04bf51010301fe7d186c3baedf8744006d1c56c0af1bf214f0226c71a54dc6677384ecc8dcc8fde0
Prepare for release by doing:
- Add changelog entry to `secp256k1-sys` for the recent version bump ready
for release.
- Bump the version of secp256k1 to 0.28.0
- Add changelog entry to `secp256k1` for the imminent release.
39febcb866 Create rust-bidings (Davidson Souza)
Pull request description:
~**Marking as draft as this is an unreleased feature from libsecp**~
From upstream:
This implements encoding of curve points using the ElligatorSwift algorithm, using 4 new API calls:
secp256k1_ellswift_encode, which converts a public key to a 64-byte pseudorandom encoding.
secp256k1_ellswift_decode, the reverse operation to convert back to normal public keys.
secp256k1_ellswift_create, which can be seen as a combination of secp256k1_ec_pubkey_create + secp256k1_ellswift_encode, but is somewhat safer.
secp256k1_ellswift_xdh, which implements x-only Diffie-Hellman directly on top of 64-byte encoded public keys, and more efficiently than decoding + invoking normal ECDH.
This algorithm allows mapping any pair of field elements (u, t) to a (valid) x coordinate in the curve. This allows representing a field element as a 64-bytes bit string that is indistinguishable from random. You can build a pair of (u, t) from any group element as well.
We also have an integrated x-only ECDH that can be used to establish a shared secret between two parties. All algorithms are compatible with BIP324 and are tested against the BIP's test cases.
I have a few questions about the rust side of the implementation:
Should it be always on, or leave it behind a cargo feature? In `libsecp` this module is default on, but you can disable it.
I'm not exposing the low-level functions, instead you can use high-level types to interact with ellswift. Is it reasonable to also expose a safe version of the functions above?
ACKs for top commit:
tcharding:
ACK 39febcb866
apoelstra:
ACK 39febcb866
Tree-SHA512: a3c06304a03af9509ff3ef16fd39ee56ec22fc12d1b36be4c20aaa2ad01e98dd34ea64c66db782d3a2c10c3a7b44c701762d45f8d82f45b62db3379710c89c42
Create bindings for all methods and static types in ellswift.h in
secp256k1-sys and their respective safe-rust types.
All methods are extensively commented and tested using BIP324's
test vectors
80b2a8d4aa Update vendored libsecp to v0.4.0 (Davidson Souza)
d2285c929a ci: Remove MIPS* from CI (Davidson Souza)
0d58f50d52 ci: generalize grp in "illegal callback" test (Andrew Poelstra)
acf9ac13e9 delete `test_manual_create_destroy` test (Andrew Poelstra)
04ce50891b lib: fix bad unit test (Andrew Poelstra)
e4cca901ea gitignore: remove things that shouldn't be there (Andrew Poelstra)
Pull request description:
Replaces #645 and #652. Precedes #627.
I'm basically using #652 but resolving the linking problems,
My local CI is erring on windows cross-test, but I can compile without issue with `cargo build --target x86_64-pc-windows-gnu`. Some MIPS jobs failed before even installing cross, I think those aren't really related to this PR. Any ideas on what can be happening?
ACKs for top commit:
apoelstra:
ACK 80b2a8d4aa
Tree-SHA512: 62c2e04348110e3995111fa666f10dcc403b963770d047361f9209cf45b45db8744a7eb6d9ee3278d18007412dab5131ac3e1dd3e3d704963c6a6f232d57199a
MIPS was recently downgraded to Tier 3, which means it won't be installable by
rustup and may not work as expected. This commit removes all MIPS-related
CI jobs.
This is just a bad test. It constructs a preallocated context object by
starting from a non-preallocated context object, in a way that can't be
done by users (since it directly constructs a `Secp256k1` struct) and a
way that is very difficult to unwind, because you wind up with two
pointers to the same underlying context object, one a "preallocated" one
and one a normal one.
If you then drop the preallocated one, it will call
`secp256k1_context_destroy`, forcing you to manually deallocate the
other one. If you drop the normally-allocated one, you need to
mem::forget the preallocated one to avoid calling
`secp256k1_context_destroy` twice. The whole thing is pretty fragile.
There is another unit test, `test_raw_ctx`, which gets into the same
situation but using the public API, and demonstrates a few ways to get
out of it.
upstream libsecp now has a CMakeLists.txt file. Many years ago we added
some things to .gitignore which appear to be local developers committing
the names of their own stray files, and now this is causing the
revendoring script to lose track of vendored files.
936421476e Use range dependency for hashes v0.12.0 - v0.13.0 (Tobin C. Harding)
Pull request description:
There are zero code changes required to support v.0.13.0 so we elect to use a range dependency to make the upgrade path for downstream users more pleasant.
Upgrade the dependency of `hashes` to be either v0.12.0 or v0.13.0
Use v0.13.0 in the recent/minimal lockfiles.
ACKs for top commit:
apoelstra:
ACK 936421476e
Tree-SHA512: c411acbd6025a751920fe2c432bf8c9ab78e891a3b32170f85873732c595777e075bd8b1277bf7b156e941a3caf532af71af36e99f88aa36dadbd66c61384166
There are zero code changes required to support v.0.13.0 so we elect to
use a range dependency to make the upgrade path for downstream users
more pleasant.
Upgrade the dependency of `hashes` to be either v0.12.0 or v0.13.0
Use v0.13.0 in the recent/minimal lockfiles.
aa4489c71b key: Improve docs (Tobin C. Harding)
Pull request description:
`PublicKey` types are for verifying ECDSA signatures, when these docs where written there were no other types of signatures. With the addition of taproot these docs have become stale.
ACKs for top commit:
apoelstra:
ACK aa4489c71b
Tree-SHA512: bb24d82f2bf316f8907b1bf02132d454d21f0b13d57f06f09f9985bc7fbf7b36e6972a0fdaf3a68967577dbe1995f2a14fd06fddd38eb46718f04bca1c50a445
33747bb16f Rename KeyPair to Keypair (Tobin C. Harding)
Pull request description:
We use "keypair" in identifiers (local vars and function names) but `KeyPair` - one of them is wrong.
Elect to follow upstream and define keypair as a single word i.e., use `Keypair` for type name and `keypair` in identifiers.
This patch can be reproduced mechanically by doing two search-and-replace operations on all files excluding the CHANGELOG
- Replace "KeyPair" with "Keypair"
- Replace "key_pair" with "keypair"
Fix#647
ACKs for top commit:
apoelstra:
ACK 33747bb16f
Tree-SHA512: 2cb67cde3a39c4681e0628b17b12724fe17d00bc71e985b4eb0b3ad58d574f4a372aa782fc9cb7221cd2032d8312c579195ad2230a3eeb4c550865fa36e42c0b
We use "keypair" in identifiers (local vars and function names) but
`KeyPair` - one of them is wrong.
Elect to follow upstream and define keypair as a single word i.e., use
`Keypair` for type name and `keypair` in identifiers.
This patch can be reproduced mechanically by doing two
search-and-replace operations on all files excluding the CHANGELOG
- Replace "KeyPair" with "Keypair"
- Replace "key_pair" with "keypair"
6fdd3b1da5 Clean up hashes import statements (Tobin C. Harding)
6d7c653b64 Use hashes instead of bitcoin_hashes (Tobin C. Harding)
Pull request description:
Use the more terse `hashes` by way of the `package` field in the manifest.
Allows us to remove the ugly feature alias "bitcoin-hashes" -> "bitcoin_hashes" and removes all the bother with the underscore.
ACKs for top commit:
apoelstra:
ACK 6fdd3b1da5
Tree-SHA512: 2c6c81b6ef900bfe930df48ddd32e795997c107a2a94b9e2ea014da0be7aefaa952e1259ddb570b30f5ad7e94c0a9443b691645cb386a8d0349dc7a81f659ed6
Now that we have `hashes` as the crate name of `bitcoin_hashes` we can
slightly clean up the import statements.
This is based on the convention we have to import things directly from
the crate if we depend on it and not from the crate level re-export.
Use the more terse `hashes` by way of the `package` field in the
manifest.
Allows us to remove the ugly feature alias "bitcoin-hashes" ->
"bitcoin_hashes" and removes all the bother with the underscore.
Why did we not think of this 2 years ago?
3da39c6fb6 Run test with recent/minimal lock files (Tobin C. Harding)
4b9168ca25 Run WASM tests from test wrapper script (Tobin C. Harding)
637d08f1fe Add a layer of indirection to the test script (Tobin C. Harding)
d9b70d27b0 Remove trailing whitespace (Tobin C. Harding)
461bae9244 Move recent/minimal lock files (Tobin C. Harding)
Pull request description:
A while back we added two lock files, one for testing with recent dependency versions and one for testing with minimal dependency versions but at the time we never used them in CI.
Move the two current lock files and use them in CI (mirroring what is done in `rust-bitcoin`).
ACKs for top commit:
apoelstra:
ACK 3da39c6fb6
Tree-SHA512: 5d293689e8a67373cbf0d6b04894c38e636bb7da19db62ac2cc1b83f1dc8184e92169a834d9adf4de3c61c34d5f6f443a1be1d0c2503bb03f08fc486d68beb71
The `wasm-pack` command does not honour `cargo` flags passed to it so we
cannot use `--locked` and test against pre-made lock files. Instead just
run the WASM test from the test script wrapper.
We would like to be able to run the test script with different lock
files, in preparation for doing so move the `test.sh` script to
`_test.sh` and add a new `test.sh` that runs `_test.sh`.
Keep the outer script as `test.sh` so that we do not change the workflow
for those running the script including the github actions.
benthecarman
Andrew Poelstra
Jiri Jakes
Tobin C. Harding
Martin Habovstiak
Davidson Souza