Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

maintainence(lib/runtime/wasmer): add support for polkadot runtime v0.9.10 #1818

Merged
merged 9 commits into from
Oct 5, 2021

Conversation

noot
Copy link
Contributor

@noot noot commented Sep 30, 2021

Changes

Tests

go test ./lib/crypto/secp256k1
go test ./lib/runtime/wasmer/ -run TestInstance_Version_PolkadotRuntime_v0910

Issues

Primary Reviewer

@codecov
Copy link

codecov bot commented Sep 30, 2021

Codecov Report

Merging #1818 (cb6e5c6) into development (0c86c24) will decrease coverage by 0.21%.
The diff coverage is 12.50%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1818      +/-   ##
===============================================
- Coverage        59.76%   59.55%   -0.22%     
===============================================
  Files              188      188              
  Lines            19678    19686       +8     
===============================================
- Hits             11761    11724      -37     
- Misses            5936     5982      +46     
+ Partials          1981     1980       -1     
Flag Coverage Δ
unit-tests 59.55% <12.50%> (-0.22%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/crypto/secp256k1/secp256k1.go 40.78% <0.00%> (-2.27%) ⬇️
lib/runtime/constants.go 0.00% <ø> (ø)
lib/runtime/test_helpers.go 17.10% <0.00%> (-0.47%) ⬇️
lib/runtime/wasmer/imports.go 42.81% <20.00%> (+0.09%) ⬆️
dot/network/sync_benchmark.go 14.28% <0.00%> (-85.72%) ⬇️
dot/network/sync.go 61.85% <0.00%> (-5.14%) ⬇️
lib/babe/epoch.go 64.51% <0.00%> (-2.16%) ⬇️
dot/network/service.go 70.00% <0.00%> (-1.15%) ⬇️
dot/core/service.go 52.04% <0.00%> (-0.75%) ⬇️
dot/digest/digest.go 67.16% <0.00%> (+0.99%) ⬆️
... and 2 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 0c86c24...cb6e5c6. Read the comment docs.

Copy link
Member

@edwardmack edwardmack left a comment

Choose a reason for hiding this comment

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

This lgtm, however I'm concerned with the version_2 just calling version_1. I don't see these version_2s defined in the spec, but looking at the substrate code there is a slight difference:
https://github.com/paritytech/substrate/blob/2ab5cd2880b1ec798a7c2a72c29f995f54b45f13/primitives/io/src/lib.rs#L764 is different than: https://github.com/paritytech/substrate/blob/2ab5cd2880b1ec798a7c2a72c29f995f54b45f13/primitives/io/src/lib.rs#L789 for secp256k1_ecdsa_recover.

And for secp256k1_ecdsa_recover_compressed: https://github.com/paritytech/substrate/blob/2ab5cd2880b1ec798a7c2a72c29f995f54b45f13/primitives/io/src/lib.rs#L812 is different than: https://github.com/paritytech/substrate/blob/2ab5cd2880b1ec798a7c2a72c29f995f54b45f13/primitives/io/src/lib.rs#L834

I'm not sure what the difference is between parse_overflowing_slice and parse_standard_slice is, we should determine how this difference affects behavior.

@noot
Copy link
Contributor Author

noot commented Oct 1, 2021

@edwardmack ok not sure how I missed that😬 I'll look into the difference between the two today and let you know when it's updated

@noot noot marked this pull request as draft October 1, 2021 07:54
@noot
Copy link
Contributor Author

noot commented Oct 1, 2021

@edwardmack looked into the difference between parse_overflowing and parse_standard, it seems that parse_overflowing is a non-standard way of parsing a signature that is potentially larger than the secp256k1 curve order: https://github.com/paritytech/libsecp256k1/blob/b30fdab3d323c5418264f961dbef6608bb04f15f/src/lib.rs#L498 I believe that the Go library (which uses the C Bitcoin secp256k1 implementation under the hood) is likely already using "standard" parsing, so this should be ok to do.

@noot noot marked this pull request as ready for review October 1, 2021 13:04
@@ -48,11 +51,27 @@ type PublicKey struct {

// RecoverPublicKey returns the 64-byte uncompressed public key that created the given signature.
func RecoverPublicKey(msg, sig []byte) ([]byte, error) {
if sig[64] == 27 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add code comments to explain what this is doing?

lib/crypto/secp256k1/secp256k1.go Outdated Show resolved Hide resolved
@noot noot changed the title maintainence(lib/runtime.wasmer): add support for polkadot runtime v0.9.10 maintainence(lib/runtime/wasmer): add support for polkadot runtime v0.9.10 Oct 5, 2021
@noot noot merged commit 5c11fb2 into development Oct 5, 2021
@noot noot deleted the noot/runtime-v0.9.10 branch October 5, 2021 11:25
@github-actions
Copy link

github-actions bot commented Dec 3, 2021

🎉 This PR is included in version 0.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

timwu20 pushed a commit to timwu20/gossamer that referenced this pull request Dec 6, 2021
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