Update the code to edition 2018, and update dependencies #331

elichai · 2021-09-15T08:41:54Z

As proposed in rust-bitcoin/rust-bitcoin#510 (comment) 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

secp256k1-sys/src/macros.rs

thomaseizinger

Thank you for doing this, looking forward to dropping old rand dependencies in our project :)

thomaseizinger · 2021-09-16T02:53:07Z

src/ecdh.rs

+    use crate::Secp256k1;
+
+    use super::SharedSecret;


I'd recommend we move to starting every test module with use super::*. In most cases, that will bring in all the types required for the tests.

examples/generate_keys.rs

src/lib.rs

secp256k1-sys/src/lib.rs

.github/workflows/rust.yml

thomaseizinger

utACK e6dc0b2

Kixunil · 2021-09-17T11:00:44Z

Didn't review but I randomly noticed that we could change CONTEXT from Option to MaybeUninit (probably as a separate PR).

apoelstra · 2021-09-24T22:17:04Z

Cool! concept ACK. Will review in detail later.

I think there is nothing stopping us merging this now? For rust-bitcoin I want a taproot release before the 2018 release, but I believe we don't need any further supporting changes from rust-secp for Taproot?

Kixunil · 2021-09-27T14:34:53Z

src/key.rs

-        match data.len() {
-            constants::SECRET_KEY_SIZE => {
-                let mut ret = [0; constants::SECRET_KEY_SIZE];
+        match <[u8; constants::SECRET_KEY_SIZE]>::try_from(data) {


In general TryFrom should be used to implement fallible conversion, try_into() to call fallible conversion. (because TryFrom implies TryInto) I think it's OK when the types are not generic but maybe keeping the good habit is worth changing?

IMHO try_into hides information into which specific type the conversion happens and makes code less readable / verifiable by human

@Kixunil I don't think that is generally true. AFAIK it is idiomatic to prefer TryInto over TryFrom but if you are not doing trait-bounds at all, TryFrom might be better because it doesn't need type annotations.

This is possible:

let x: Type = y.try_into()?;

But I think TryFrom is OK in this case.

tcharding

The commit message of the final commit (bde5f3a Use new trait TryInto and do small refactoring is now out of date (should be 'TryFrom'). Also there is one lint warning still on this PR

warning: unused import: `fmt`
--> secp256k1-sys/src/types.rs:2:12
|
2 | use core::{fmt, mem};
|            ^^^
|
= note: `#[warn(unused_imports)]` on by default

Apart from that, LGTM :)

tcharding · 2021-10-26T22:41:57Z

.github/workflows/rust.yml

@@ -60,7 +60,7 @@ jobs:
    strategy:
      matrix:
        rust:
-          - 1.29.0
+          - 1.36.0


Does it make any difference if this is 1.36 as opposed to 1.36.0? Said another way, will anything be added to a later patch version of 1.36 that we should be enforcing the usage of? I don't know the answer I'm just wondering.

One thing I noticed in the past, running cargo +1.36 build didn't work while cargo +1.36.0 did, so maybe this has the same interface.

Do you mean didn't work as in the build failed? If so then that would validate my question and we should decide whether we are enforcing 1.36.0 or latest 1.36.*, right?

FTR I have been testing using cargo +1.36 check.

It said unknown toolchain or something like that. But TBH it was a long ago, maybe it changed to support it.

apoelstra · 2022-01-02T23:32:30Z

We do actually need to do a rust-secp release for Taproot, so delaying this until the next major rev.

Also, needs rebase.

tcharding · 2022-01-14T02:44:44Z

In case you want to save yourself some work @elichai I was playing around with this branch and it took me quite a while to rebase it. Feel free to grab what I have done to save yourself having to do it. Same branch name on my tree (github.com/tcharding/rust-secp256k1).

tcharding · 2022-01-15T00:05:24Z

Have I had a git fail @elichai and pushed to your branch? Super sorry if I did, I did not realize pushRemote = [email protected]:elichai/rust-secp256k1.git was set in my config. FTR I checked out your branch using gh pr (github's command line tool). I assumed the branch would be configured to push to origin, I was wrong. I feel pretty rude having done that, please accept my apologies. Although I'm pretty confused right now looking at the times of the commits above, you might have pulled the changes from my tree.

tcharding · 2022-01-15T00:14:23Z

Anyways, there is a branch on my tree now that has been updated to do two things

Use Rust 1.41.1
Try to make the diff as small and easy to review as possible (mainly this means not moving the import statements around anymore than is necessary).

elichai · 2022-01-15T14:06:08Z

No you didn't push here, Thanks for your rebase it helped to make sure I'm rebasing correctly (still rebased myself as to know what actually changed)

* Use Rust 1.41.1

Does that change anything here?

tcharding · 2022-01-15T22:26:50Z

Does that change anything here?

Does not change the patch set AFAICT except for the actual version number itself and the commit messages.

tcharding · 2022-04-25T04:45:09Z

Hey @elichai, no clue what order the MSRV patches are going to merge across the rust-bitcoin stack but we are in the vicinity of ready to merge this one, have you time to update the PR?

tcharding · 2022-04-25T06:35:13Z

I was messing around with rust-bitcoin/rust-bitcoin#967 and chased build errors into secp, then into rand and ended up rebasing my local copy of this PR to see if it was a rand 6 problem. Anyways, in case its of any use I pushed it up to my tree on a branch of the same name again. FTR I'm hitting a build error still when enabling use-serde (on rust-bitcoin), but anyways there it is :)

elichai · 2022-04-26T08:35:02Z

Hey @elichai, no clue what order the MSRV patches are going to merge across the rust-bitcoin stack but we are in the vicinity of ready to merge this one, have you time to update the PR?

I'll rebase today-tomorrow

elichai · 2022-05-06T09:28:18Z

First push is a rebase, and the second one is fixing the nits

apoelstra · 2022-05-06T14:23:02Z

src/schnorr.rs

@@ -139,8 +135,7 @@ impl<C: Signing> Secp256k1<C> {
    #[cfg(any(test, feature = "rand-std"))]
    #[cfg_attr(docsrs, doc(cfg(feature = "rand-std")))]
    pub fn sign_schnorr(&self, msg: &Message, keypair: &KeyPair) -> Signature {
-        let mut rng = thread_rng();
-        self.sign_schnorr_with_rng(msg, keypair, &mut rng)
+        self.schnorrsig_sign_with_rng(msg, keypair, &mut rand::thread_rng())


In f792674:

schnorrsig_sign_with_rng is deprecated; we should use the other method

IMO this is blocking merge.

@elichai I am happy to merge this once you address this

Sorry, missed the notification on this

apoelstra · 2022-05-06T14:26:20Z

src/key.rs

@@ -162,9 +163,8 @@ impl SecretKey {
    /// ```
    #[inline]
    pub fn from_slice(data: &[u8])-> Result<SecretKey, Error> {
-        match data.len() {


In ac8e966:

Maybe worth thinking about turning these from_slice methods into TryFrom impls entirely ... doesn't need to be in this PR.

FWIW I rekon we should keep this PR as lean as possible to help expedite review/merge.

apoelstra

ACK 2f18866 aside from nits

tcharding

ACK 2f18866

tcharding · 2022-05-11T22:06:10Z

Oh, @elichai, I just noticed the PR description mentions 1.36 still.

tcharding · 2022-05-19T06:22:25Z

This is the last on to go and we have done it, friendly bump please reviewers :)

koushiro · 2022-05-30T08:55:12Z

Hi, can I ask when this PR is planned to be merged and a new version containing the PR will be released?

Kixunil · 2022-05-30T12:43:25Z

@koushiro we hope for it to be faster than usual but it'll probably still take some time.

tcharding · 2022-05-30T23:24:58Z

As far as I understand it, soon as this PR merges we can start releasing each of the crates in the rust-bitcoin stack starting at the bottom. rust-bech32 has been done already so next its rust-bitcoinconsensus, then bitcoin_hashes then this one. Hope that helps explain the process and potential timing.

tcharding · 2022-06-06T01:34:54Z

Friendly bump, can we get an ack on this from another maintainer please. (I don't actually know who are the maintainers of this crate? @real-or-random, @jonasnick). Is that what you are waiting for to merge this @apoelstra?

apoelstra · 2022-06-06T14:15:00Z

I am waiting for the use of self.schnorrsig_sign_with_rng to be removed.

elichai · 2022-06-07T21:00:55Z

Fixed, sorry that it took me so long

tcharding

ACK 5d2f1ce

apoelstra

ACK 5d2f1ce

elichai mentioned this pull request Sep 15, 2021

Update rand dependency #328

Closed

RCasatta reviewed Sep 15, 2021

View reviewed changes

secp256k1-sys/src/macros.rs Outdated Show resolved Hide resolved

thomaseizinger reviewed Sep 16, 2021

View reviewed changes

elichai force-pushed the edition-2018 branch 2 times, most recently from 0a096c7 to e6dc0b2 Compare September 16, 2021 14:00

thomaseizinger reviewed Sep 16, 2021

View reviewed changes

This was referenced Sep 20, 2021

Bump rand from 0.6.5 to 0.8.4 get10101/itchysats#35

Closed

Bump rand_chacha from 0.1.1 to 0.3.1 get10101/itchysats#36

Closed

elichai force-pushed the edition-2018 branch from e6dc0b2 to bde5f3a Compare September 26, 2021 08:47

Kixunil reviewed Sep 27, 2021

View reviewed changes

tcharding reviewed Oct 26, 2021

View reviewed changes

tcharding mentioned this pull request Oct 27, 2021

Enable edition 2018 rust-bitcoin/bitcoin_hashes#136

Merged

elichai mentioned this pull request Dec 5, 2021

Add a static immutable zero aligned type #345

Merged

tmpfs mentioned this pull request Dec 7, 2021

Support for wasm32-unknown-unknown target? ZenGo-X/multi-party-ecdsa#138

Closed

elichai force-pushed the edition-2018 branch 2 times, most recently from 6eb22e5 to a2d43df Compare January 14, 2022 14:06

tcharding mentioned this pull request Jan 15, 2022

Rust 2018 & MSRV bump 1.41.1 tracking issue rust-bitcoin/rust-bitcoin#510

Closed

20 tasks

tcharding mentioned this pull request Apr 25, 2022

Upgrade dev dependencies rust-bitcoin/rust-bitcoin#967

Closed

Update MSRV in CI and Readme from 1.29 to 1.41

67c0922

elichai force-pushed the edition-2018 branch from c7103ab to 2f18866 Compare May 6, 2022 09:20

apoelstra reviewed May 6, 2022

View reviewed changes

apoelstra previously approved these changes May 6, 2022

View reviewed changes

tcharding previously approved these changes May 8, 2022

View reviewed changes

elichai added 7 commits June 7, 2022 23:59

Update secp256k1 to edition 2018 and fix imports

626835f

Update rand to 0.8 and replace CounterRng with mock::StepRng

ebe46a4

Update secp256k1-sys to edition 2018 and fix imports

26a52bc

Replace c_void with core::ffi::c_void

bc8c713

Move more things from the std feature to the alloc feature

7d3a149

Use new trait TryFrom and do small refactoring

39aaac6

Fix WASM build

5d2f1ce

elichai dismissed stale reviews from tcharding and apoelstra via 5d2f1ce June 7, 2022 20:59

elichai force-pushed the edition-2018 branch from 2f18866 to 5d2f1ce Compare June 7, 2022 20:59

tcharding approved these changes Jun 7, 2022

View reviewed changes

apoelstra approved these changes Jun 8, 2022

View reviewed changes

apoelstra merged commit 4f7f138 into rust-bitcoin:master Jun 8, 2022

elichai deleted the edition-2018 branch June 8, 2022 21:45

apoelstra mentioned this pull request Jun 21, 2022

Bump crate version to 0.23.0 #444

Merged

Update the code to edition 2018, and update dependencies #331

Update the code to edition 2018, and update dependencies #331

Conversation

elichai commented Sep 15, 2021 • edited Loading

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a comment • edited Loading

Choose a reason for hiding this comment

Kixunil commented Sep 17, 2021 • edited Loading

apoelstra commented Sep 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Jan 2, 2022

tcharding commented Jan 14, 2022

tcharding commented Jan 15, 2022 • edited Loading

tcharding commented Jan 15, 2022

elichai commented Jan 15, 2022 • edited Loading

tcharding commented Jan 15, 2022

tcharding commented Apr 25, 2022

tcharding commented Apr 25, 2022

elichai commented Apr 26, 2022

elichai commented May 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented May 11, 2022

tcharding commented May 19, 2022

koushiro commented May 30, 2022

Kixunil commented May 30, 2022 • edited Loading

tcharding commented May 30, 2022

tcharding commented Jun 6, 2022

apoelstra commented Jun 6, 2022

elichai commented Jun 7, 2022

tcharding left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

elichai commented Sep 15, 2021 •

edited

Loading

thomaseizinger left a comment •

edited

Loading

Kixunil commented Sep 17, 2021 •

edited

Loading

thomaseizinger Sep 27, 2021 •

edited

Loading

tcharding commented Jan 15, 2022 •

edited

Loading

elichai commented Jan 15, 2022 •

edited

Loading

Kixunil commented May 30, 2022 •

edited

Loading