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

Revert k256 removal #14499

Merged
merged 2 commits into from
Jul 3, 2023
Merged

Revert k256 removal #14499

merged 2 commits into from
Jul 3, 2023

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Jul 3, 2023

Let's keep using k256 in wasm to not fuck up people compiling their stuff and apparently it also started to include __stack_chk_fail on my machine as "host function".

@bkchr bkchr 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 Jul 3, 2023
@bkchr bkchr requested review from a team July 3, 2023 13:06
Copy link
Contributor

@gilescope gilescope left a comment

Choose a reason for hiding this comment

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

Good call.

@bkchr bkchr merged commit dfd8286 into master Jul 3, 2023
@bkchr bkchr deleted the bkchr-revert-k256-removal branch July 3, 2023 21:38
@pgherveou
Copy link
Contributor

just fyi, this seems to have quite bad perf implication, this is a result of benchmarking ran today on pallet-contracts
Screenshot 2023-07-06 at 22 13 21

@gilescope
Copy link
Contributor

It's not a performance regression if it was never released. This lib also likely has less unsafety. Personally I think we should put in the effort to improve the performance of the pure rust libs.

@bkchr
Copy link
Member Author

bkchr commented Jul 7, 2023

just fyi, this seems to have quite bad perf implication, this is a result of benchmarking ran today on pallet-contracts Screenshot 2023-07-06 at 22 13 21

When was the last time this benchmark was run?

@pgherveou
Copy link
Contributor

pgherveou commented Jul 7, 2023

It's not a performance regression if it was never released.

Yes indeed, just sharing some data from the last benchmark, @bkchr the last one was from this commit
b55dbcb 4 days ago

@bkchr
Copy link
Member Author

bkchr commented Jul 7, 2023

This was also run on the same machine? I didn't followed, but I knew that we switched or wanted to switch now to the benchmarks on VM.

@pgherveou
Copy link
Contributor

not too sure they were both ran with the b*t bench $ pallet dev pallet_contracts command

@bkchr
Copy link
Member Author

bkchr commented Jul 7, 2023

The weight files contains the configuration of the machine. You can compare this.

@pgherveou
Copy link
Contributor

pgherveou commented Jul 7, 2023

Screenshot 2023-07-07 at 11 10 18
Yes same machine

@bkchr
Copy link
Member Author

bkchr commented Jul 7, 2023

Okay, yeah then the pure rust implementation is really that bad. 🙈

But yeah, we know about them as otherwise we would use k256 everywhere: #8089 (comment)

@ggwpez
Copy link
Member

ggwpez commented Jul 7, 2023

I compared it again on the same machine and it used just 35% of the time before this MR was merged. But okay, if it causes build error then we cannot use it now.

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.

7 participants