Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Remove k256 crate from frame-support dependencies #14452

Merged
merged 4 commits into from
Jun 25, 2023

Conversation

conr2d
Copy link
Contributor

@conr2d conr2d commented Jun 23, 2023

Description

This PR removes k256 crate from frame-support dependencies.

k256 is used only for converting a compressed secp256k1 public key of Substrate to an uncompressed one to derive Ethereum address. This also can be done secp256k1 crate that is already used by sp-core. This PR removes redundant dependency.

Checklist

  • My PR includes a detailed description as outlined in the "Description" section above
  • My PR follows the labeling requirements of this project (at minimum one label for each A, B, C and D required)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • If this PR alters any external APIs or interfaces used by Polkadot, the corresponding Polkadot PR is ready as well as the corresponding Cumulus PR (optional)

@conr2d conr2d requested review from a team June 23, 2023 18:49
@paritytech-ci paritytech-ci requested a review from a team June 23, 2023 20:11
@ggwpez ggwpez added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jun 24, 2023
@ggwpez
Copy link
Member

ggwpez commented Jun 24, 2023

Please merge in master and format the code with nightly to get the CI green.

@bkchr bkchr merged commit 75be6e2 into paritytech:master Jun 25, 2023
@conr2d conr2d deleted the remove-k256-dep branch June 26, 2023 00:20
@gilescope
Copy link
Contributor

gilescope commented Jun 26, 2023

Interestingly this annoys my machine - seems to not find the right includes when building secp256k1-sys v0.6.1. I thought I would mention it in case it's not just my machine. (if anyone has a nix-shell or flake that builds this do please throw me a link)

@conr2d
Copy link
Contributor Author

conr2d commented Jun 26, 2023

Interestingly this annoys my machine - seems to not find the right includes when building secp256k1-sys v0.6.1. I thought I would mention it in case it's not just my machine. (if anyone has a nix-shell or flake that builds this do please throw me a link)

I reproduced this issue with AppleClang shipped with Xcode. AppleClang doesn't include wasm32 target, so build will fail with the error No available targets are compatible with triple "wasm32-unknown-unknown".

In Intel Mac, the clang command is usually replaced by the homebrew version of LLVM including wasm32 target, so this issue doesn't occur. However, in Apple Silicon (M1), the clang command still points out AppleClang.

This can be solved by setting environment variables like:

export CC=/opt/homebrew/opt/llvm/bin/clang
export CXX=/opt/homebrew/opt/llvm/bin/clang++
export AR=/opt/homebrew/opt/llvm/bin/llvm-ar

If using Intel Mac, but the error is section too large, try setting:

export AR=/usr/local/opt/llvm/bin/llvm-ar

@liamaharon
Copy link
Contributor

This also broke substrate builds on my machine, setting CC, CXX and AR vars didn't work for me.

❰liamaharon❙~/grimoire/substrate(gitmaster)❱✔≻ echo $CC
/opt/homebrew/opt/llvm/bin/clang
❰liamaharon❙~/grimoire/substrate(gitmaster)❱✔≻ echo $CXX
/opt/homebrew/opt/llvm/bin/clang++
❰liamaharon❙~/grimoire/substrate(gitmaster)❱✔≻ echo $AR
/opt/homebrew/opt/llvm/bin/llvm-ar

I get a bunch of warnings like this before it eventually errors out
Screenshot 2023-06-26 at 23 38 59

Screenshot 2023-06-26 at 23 40 22

@conr2d
Copy link
Contributor Author

conr2d commented Jun 27, 2023

@liamaharon Can you share the result of brew info llvm? I could build substrate with the env variables on m1 Mac Studio.

I thought that sp-core already uses secp256k1, so there should not be an error, but it seems that secp256k1 is only used on the native side (not wasm). k256 is a pure rust implementation of secp256k1, so its build does not depend on external toolchain like AppleClang, but secp256k1 is a wrapper of libsecp256k1 written in C, so requires additional build process.

I can look into the issue further, but if it hinders the development, feel free to revert the commit. I am sorry for annoying.

@gilescope
Copy link
Contributor

gilescope commented Jun 27, 2023

Could we switch the native dep with https://github.com/paritytech/libsecp256k1 to get round these problems? (I see this is mentioned here: #13624 )

@liamaharon
Copy link
Contributor

liamaharon commented Jun 27, 2023

@conr2d brew info llvm:

I checked LDFLAGS is also set correctly.

Screenshot 2023-06-27 at 11 29 14

Is anyone else running a Macbook Pro M2 Max able to confirm they're having the same issue as me?

@liamaharon
Copy link
Contributor

I figured it out, it was a local config issue.

I had CPATH=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.3.sdk/usr/include which didn't seem to cause an issue until this commit. I removed it (CPATH is now completely unset) and it compiles fine now.

@jasl jasl mentioned this pull request Jun 27, 2023
2 tasks
bkchr added a commit that referenced this pull request Jul 3, 2023
@bkchr
Copy link
Member

bkchr commented Jul 3, 2023

Yeah let's revert it: #14499

bkchr added a commit that referenced this pull request Jul 3, 2023
* Revert "Remove k256 crate from frame-support dependencies (#14452)"

This reverts commit 75be6e2.

* Keep the test
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* Add test for to_eth_address

* Replace k256 with secp256k1

* Bump Cargo.lock

* Reformat
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* Revert "Remove k256 crate from frame-support dependencies (paritytech#14452)"

This reverts commit 75be6e2.

* Keep the test
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants