Skip to content

update rust-secp to 0.27.0#1714

Merged
apoelstra merged 5 commits intomasterfrom
2023-03--update-secp
Mar 20, 2023
Merged

update rust-secp to 0.27.0#1714
apoelstra merged 5 commits intomasterfrom
2023-03--update-secp

Conversation

@apoelstra
Copy link
Copy Markdown
Member

Also remove the spurious dev-dependency copy of rust-secp, which should've been updated to remove the "recovery" feature in https://www.github.com/rust-bitcoin/rust-bitcoin/pull/545 and then been removed entirely in https://www.github.com/rust-bitcoin/rust-bitcoin/pull/1387

Also remove the spurious dev-dependency copy of rust-secp, which
should've been updated to remove the "recovery" feature in
https://www.github.com/rust-bitcoin/rust-bitcoin/pull/545 and
then been removed entirely in https://www.github.com/rust-bitcoin/rust-bitcoin/pull/1387
@apoelstra apoelstra mentioned this pull request Mar 18, 2023
8 tasks
@apoelstra
Copy link
Copy Markdown
Member Author

CI is breaking because serde recently updated to use syn 2 while mutagen did not. This breaks:

  • compilation on 1.41, since syn 2 uses edition 2021 (sigh)
  • our "duplicate dependency" check

I will add a commit to this PR which whitelists the dupe check, and updates the README to instruct users how to downgrade serde.

tcharding
tcharding previously approved these changes Mar 19, 2023
Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f5f4a33

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Mar 19, 2023

Looks like we need to pin serde/syn for Rust 1.47 as well in bitcoin/contrib/tesh.sh

@tcharding
Copy link
Copy Markdown
Member

I pushed a commit to your branch @apoelstra to pin for 1.47 (I mimicked the current one pinning 1.41)

@tcharding
Copy link
Copy Markdown
Member

And one to update the readme.

We now require pinning for `serde` and `syn` if building with toolchain
1.47. Document this in the README.
@tcharding tcharding force-pushed the 2023-03--update-secp branch from 4424686 to bef7992 Compare March 19, 2023 01:20
@tcharding
Copy link
Copy Markdown
Member

Force push to write the commit log without typos.

@apoelstra
Copy link
Copy Markdown
Member Author

bef7992 looks good to me.

@tcharding you need to ack even though they're your commits -- I cannot ACK my own PR :).

Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK bef7992

Deferring the decision about all tests passing for MSRV to @apoelstra
README can be fixed up later.

# whitelist the 'syn' crate which is duplicated but it's not our fault.
cargo tree --target=all --all-features --duplicates \
| grep '^[0-9A-Za-z]' \
| grep -v 'syn' \
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be something like '[^a-z_-]syn[^a-z_-]' to not filter-out things like foo-syntax. It's fine for now.

But still I think we should pin the crate instead.

# Pin dependencies as required if we are using MSRV toolchain.
if cargo --version | grep "1\.41"; then
# 1.0.157 uses syn 2.0 which requires edition 2018
cargo update -p serde --precise 1.0.156
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be the first commit or we don't care about tests passing MSRV for the purpose of bisecting?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we don't care, on multiple levels :)

  • We don't care about MSRV for non-tip commits
  • We don't care about contrib/test.sh passing for any commits, since this file is really for CI and is a PITA to run locally
  • We can't care about pins working in old commits. Literally every commit in the repo, up to this one, now fail with 1.41, because the dependency changed from under us. So moving it back one or two commits wouldn't change that situation much.

if cargo --version | grep "1\.47"; then
cargo update -p serde --precise 1.0.156
cargo update -p syn --precise 1.0.107
fi
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squash with the previous change into first commit?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave it for you to squash if wanted @apoelstra since it's your branch - I only pushed to it yesterday because I thought it was a trifling matter and I could save you some time, seems not :(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have two ACKs I won't bother squashing.

`no-std`) on **Rust 1.41.1** or **Rust 1.47** with `no-std`.

To build with the MSRV you will need to pin some dependencies:
To build with the MSRV you will need to pin some dependencies (also for `no-std`):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like we don't support no_std with those higher versions. Should say "also for 1.47"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this sentence is awkwardly worded. We can fix it later. But we do say in the above line that the MSRV for no_std is 1.47, so I think this does say the correct thing.

@Kixunil Kixunil added this to the 0.30.0 milestone Mar 19, 2023
Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK bef7992

@apoelstra apoelstra merged commit 2f404f9 into master Mar 20, 2023
@apoelstra apoelstra deleted the 2023-03--update-secp branch March 20, 2023 15:12
apoelstra added a commit that referenced this pull request Mar 22, 2023
ffee8ad Bump version to v0.30.0 (Tobin C. Harding)

Pull request description:

  Add changelog notes and bump the version number to v0.30.0.

  ## TODO - pre-merge

  - [x] Release `bitcoin_hashes` 0.12: #1694
  - [x] Release secp 0.27: rust-bitcoin/rust-secp256k1#588
    - rust-bitcoin/rust-secp256k1#590
  - [x] Update `secp256k1` dependency to use newly released v0.27: #1714
  - [x] Merge
    - ~#1696
    - #1695
    -  #1111
  - [x] If time permits merge these:
    - #1710
    - #1705
    - #1713
  - [x] Set the release date in changelog header
  - [x] And merge these:
    - #1721
    - #1720
    - #1719
    - #1717

  ## TODO  - post release
  - [ ] Release the blogpost: rust-bitcoin/www.rust-bitcoin.org#2
     - ~Set the date in the blog post to match the date 0.30 is released~

ACKs for top commit:
  sanket1729:
    reACK ffee8ad
  Kixunil:
    ACK ffee8ad
  apoelstra:
    ACK ffee8ad

Tree-SHA512: b0ea113ee1726fd9b263d0e01fe14bd544c007c05a9ac43b6c2d4edbeef3bb3ad456b061ef086626e1e1b27a0cda49cb6bc28aac3ad1691d72ffe00400ed5b45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants