Upgrade `libsecp256k1` from 0.3.5 ➔ 0.7.0 #967

cmichi · 2021-10-19T10:50:24Z

This is required to fix the failing cargo-contract CI (specifically the Windows run).

We are currently using a way outdated version of libsecp256k1, upgrading it turned out harder than just switching to the other no_std compatible crate.

crates/eth_compatibility/Cargo.toml

codecov-commenter · 2021-10-20T05:49:26Z

Codecov Report

Merging #967 (ffbb1e8) into master (c90da11) will increase coverage by 13.39%.
The diff coverage is 75.00%.

@@             Coverage Diff             @@
##           master     #967       +/-   ##
===========================================
+ Coverage   62.62%   76.01%   +13.39%     
===========================================
  Files         244      244               
  Lines        9147     9149        +2     
===========================================
+ Hits         5728     6955     +1227     
+ Misses       3419     2194     -1225

Impacted Files	Coverage Δ
crates/engine/src/tests.rs	`100.00% <ø> (ø)`
crates/eth_compatibility/src/lib.rs	`74.07% <ø> (+74.07%)`	⬆️
...tes/env/src/engine/experimental_off_chain/impls.rs	`55.23% <50.00%> (+12.38%)`	⬆️
crates/engine/src/ext.rs	`69.07% <100.00%> (+14.47%)`	⬆️
crates/env/src/engine/off_chain/impls.rs	`42.98% <100.00%> (+9.64%)`	⬆️
crates/allocator/src/bump.rs	`62.50% <0.00%> (-4.17%)`	⬇️
crates/lang/ir/src/ir/item_impl/mod.rs	`67.79% <0.00%> (+0.84%)`	⬆️
crates/lang/ir/src/ir/item_impl/impl_item.rs	`42.55% <0.00%> (+2.12%)`	⬆️
crates/lang/ir/src/ir/attrs.rs	`82.13% <0.00%> (+3.13%)`	⬆️
crates/env/src/api.rs	`36.36% <0.00%> (+4.54%)`	⬆️
... and 39 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c90da11...ffbb1e8. Read the comment docs.

…cp256k1-to-0.7.0

Robbepop · 2021-10-21T12:34:11Z

Cargo.toml

@@ -1,4 +1,5 @@
 [workspace]
+resolver = "2"


Why is the new cargo feature resolver required for this change?

Ok, so the issue is that cargo's old resolver does not handle features in dependencies correctly. In our case we have the libsecp256k1 crate as (1) a dependency for non-std and (2) as a dep for the off-chain engines. Both cases require a different feature set, of which some features are not compatible to non-std.

The old feature resolver doesn't handle this properly and merges the features, thus resulting in errors a la error: duplicate lang item in crate std (which secp256k1 depends on): panic_impl.

But since we'll migrate to the 2021 edition soon this will be obsolete anyway (because resolver = "2" is the default there).

Shouldn't this be unnecessary since we are using edition 2021 now?

Shouldn't this be unnecessary since we are using edition 2021 now?

Absolutely my assumption as well, but unfortunately not true: rust-lang/cargo#9956.

If you are using a [virtual workspace], you will still need to explicitly set the [resolver field] in the [workspace] definition if you want to opt-in to the new resolver.

crates/env/Cargo.toml

crates/env/src/engine/off_chain/impls.rs

Robbepop · 2021-10-21T16:19:19Z

What is the reason for using libsecp256k1 crate again instead of secp256k1?

cmichi · 2021-10-21T16:31:52Z

What is the reason for using libsecp256k1 crate again instead of secp256k1?

I've switched back to libsecp256k1 by now, since it is used in Substrate as well. Hence I think it makes most sense to have a consistent crate throughout the stack.Also that crate is maintained by Parity.

…cp256k1-to-0.7.0

Robbepop

LGTM

Please fix CI and we can merge.

xgreenx · 2021-10-21T18:16:04Z

Did you try to build a contract that uses the to_eth_address function? When we tried the latest version of libsecp256k1 the build failed in no_std mode. If not, maybe we need to add a unit test with contract which uses everything from the eth_compatibility crate.

Robbepop · 2021-10-21T18:35:35Z

.rustfmt.toml

-edition = "2018" # changed
+edition = "2021" # changed


good catch!

Robbepop · 2021-10-21T18:35:55Z

Cargo.toml

@@ -1,4 +1,5 @@
 [workspace]
+resolver = "2"


Shouldn't this be unnecessary since we are using edition 2021 now?

cmichi · 2021-10-22T07:26:24Z

Did you try to build a contract that uses the to_eth_address function? When we tried the latest version of libsecp256k1 the build failed in no_std mode. If not, maybe we need to add a unit test with contract which uses everything from the eth_compatibility crate.

@xgreenx Yes, I tested it. Ideally we would also have an example contract that uses these eth compatibility functions, so if you have any idea feel free to say :-).

The issue that you ran into is detected in our CI with this stage:

$ export ALSO_WASM_CRATES="env storage storage/derive allocator prelude primitives lang lang/macro lang/ir"
$ for crate in ${ALSO_WASM_CRATES}; do cargo check --verbose --no-default-features --target wasm32-unknown-unknown --manifest-path crates/${crate}/Cargo.toml; done

I had this to, specifically it was these errors:

error: duplicate lang item in crate `std` (which `secp256k1` depends on): `panic_impl`.
  |                                                                            
  = note: the lang item is first defined in crate `ink_env` (which `ink_eth_compatibility` depends on)
  = note: first definition in `ink_env` loaded from /home/michi/projects/ink/examples/flipper/target/wasm32-unknown-unknown/debug/deps/libink_env-473e311d04c72
977.rmeta                             
  = note: second definition in `std` loaded from /home/michi/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib/libstd-
ecfc68b7dd3a7740.rlib                                                                                                                                                                                                                                                                                                         
error: duplicate lang item in crate `std` (which `secp256k1` depends on): `oom`.                                                                               
  |                                   
  = note: the lang item is first defined in crate `ink_allocator` (which `ink_env` depends on)
  = note: first definition in `ink_allocator` loaded from /home/michi/projects/ink/examples/flipper/target/wasm32-unknown-unknown/debug/deps/libink_allocator-9
5e21e4d041b01e9.rmeta
  = note: second definition in `std` loaded from /home/michi/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib/libstd-
ecfc68b7dd3a7740.rlib

error: could not compile `ink_eth_compatibility` due to 2 previous errors

What I found is that cargo's feature resolver 1 does not handle features in dependencies correctly. In our case we have the libsecp256k1 crate as

a dependency for no-std and
as a dependency for the off-chain testing engines.
Both cases require a different feature set, of which some features are not compatible to non-std. The old feature resolver doesn't handle this properly and merges features. This results in features which should only be enabled for the off-chain engine (so in std) being enabled for wasm32 targets as well.

We've fixed this by migrating to edition 2021, which has resolver = "2" as a default, as well as using conditional dependencies a la [target.'cfg(target_arch = "wasm32")'.dependencies].

Upgrade libsecp256k1 from 0.3.5 ➔ 0.7.0

766d043

cmichi requested a review from ascjones October 19, 2021 10:50

cmichi marked this pull request as draft October 19, 2021 11:12

cmichi removed the request for review from ascjones October 19, 2021 11:25

cmichi added 3 commits October 19, 2021 13:36

Include off-chain dependencies only when necessary

9610d3a

Include off-chain dependency only when necessary

a32ce16

Include off-chain dependency only when necessary

f4d8959

xgreenx reviewed Oct 19, 2021

View reviewed changes

crates/eth_compatibility/Cargo.toml Outdated Show resolved Hide resolved

cmichi added 3 commits October 19, 2021 19:29

Switch to secp256k1 for eth_compatibility crate

f339fdf

Remove conditional dependency

1d4ff6d

Fix serialize

bf40498

cmichi added 6 commits October 21, 2021 12:05

Switch to secp256k1 crate everywhere

d22c6da

Merge remote-tracking branch 'origin/master' into cmichi-update-libse…

599fd21

…cp256k1-to-0.7.0

Switch to secp256k1 crate everywhere

92d0577

Fix compilation for wasm32 target

6f4e16e

Ensure enabled features are not merged for targets

a30a790

Make clippy happy

1736999

cmichi changed the title ~~Upgrade libsecp256k1 from 0.3.5 ➔ 0.7.0~~ Switch from libsecp256k1 to secp256k1 Oct 21, 2021

cmichi marked this pull request as ready for review October 21, 2021 12:11

cmichi requested a review from Robbepop October 21, 2021 12:11

Use resolver 2 in all examples

f12e024

Robbepop reviewed Oct 21, 2021

View reviewed changes

cmichi added 3 commits October 21, 2021 17:51

Switch to libsecp256k1 0.7.0

f3c5161

Use default features

c68fcbe

Bring comment back

b5ee091

cmichi changed the title ~~Switch from libsecp256k1 to secp256k1~~ Upgrade libsecp256k1 from 0.3.5 ➔ 0.7.0 Oct 21, 2021

Reduce diff

2186ceb

cmichi added 2 commits October 21, 2021 19:11

Merge remote-tracking branch 'origin/master' into cmichi-update-libse…

1480c60

…cp256k1-to-0.7.0

Remove unnecessary import

da18644

Robbepop approved these changes Oct 21, 2021

View reviewed changes

cmichi added 2 commits October 21, 2021 20:28

Update rustfmt config to edition 2018

f9f4b78

Use resolver 2 for workspace

ffbb1e8

Robbepop reviewed Oct 21, 2021

View reviewed changes

cmichi merged commit 4534c4a into master Oct 22, 2021

cmichi deleted the cmichi-update-libsecp256k1-to-0.7.0 branch October 22, 2021 07:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade `libsecp256k1` from 0.3.5 ➔ 0.7.0 #967

Upgrade `libsecp256k1` from 0.3.5 ➔ 0.7.0 #967

cmichi commented Oct 19, 2021 •

edited

Loading

codecov-commenter commented Oct 20, 2021 •

edited

Loading

Robbepop Oct 21, 2021

cmichi Oct 21, 2021

Robbepop Oct 21, 2021

cmichi Oct 22, 2021

Robbepop commented Oct 21, 2021

cmichi commented Oct 21, 2021

Robbepop left a comment

xgreenx commented Oct 21, 2021

Robbepop Oct 21, 2021

Robbepop Oct 21, 2021

cmichi commented Oct 22, 2021

Upgrade libsecp256k1 from 0.3.5 ➔ 0.7.0 #967

Upgrade libsecp256k1 from 0.3.5 ➔ 0.7.0 #967

Conversation

cmichi commented Oct 19, 2021 • edited Loading

codecov-commenter commented Oct 20, 2021 • edited Loading

Codecov Report

Robbepop Oct 21, 2021

Choose a reason for hiding this comment

cmichi Oct 21, 2021

Choose a reason for hiding this comment

Robbepop Oct 21, 2021

Choose a reason for hiding this comment

cmichi Oct 22, 2021

Choose a reason for hiding this comment

Robbepop commented Oct 21, 2021

cmichi commented Oct 21, 2021

Robbepop left a comment

Choose a reason for hiding this comment

xgreenx commented Oct 21, 2021

Robbepop Oct 21, 2021

Choose a reason for hiding this comment

Robbepop Oct 21, 2021

Choose a reason for hiding this comment

cmichi commented Oct 22, 2021

Upgrade `libsecp256k1` from 0.3.5 ➔ 0.7.0 #967

Upgrade `libsecp256k1` from 0.3.5 ➔ 0.7.0 #967

cmichi commented Oct 19, 2021 •

edited

Loading

codecov-commenter commented Oct 20, 2021 •

edited

Loading