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

contracts: add sr25519_verify #13724

Merged
merged 45 commits into from
Apr 12, 2023

Conversation

pgherveou
Copy link
Contributor

@pgherveou pgherveou commented Mar 27, 2023

fix #13703

@pgherveou pgherveou closed this Mar 29, 2023
@pgherveou pgherveou force-pushed the pg/contracts-add-sr25519_recover branch from 485f0aa to 83c8198 Compare March 29, 2023 19:46
@pgherveou pgherveou reopened this Mar 29, 2023
@pgherveou pgherveou marked this pull request as ready for review March 29, 2023 23:03
@pgherveou pgherveou requested a review from athei as a code owner March 29, 2023 23:03
@pgherveou pgherveou changed the title contracts: add sr25519_recover contracts: add sr25519_verify Mar 29, 2023
@pgherveou
Copy link
Contributor Author

bot bench $ pallet dev pallet_contracts

@command-bot
Copy link

command-bot bot commented Mar 29, 2023

@pgherveou https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2614559 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_contracts. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 17-f5846515-5654-48b7-8ccb-dba7ae90bd09 to cancel this command or bot cancel to cancel all commands in this pull request.

@pgherveou pgherveou added A0-please_review Pull request needs code review. 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 B1-note_worthy Changes should be noted in the release notes T4-smart_contracts This PR/Issue is related to smart contracts. T1-runtime This PR/Issue is related to the topic “runtime”. and removed T4-smart_contracts This PR/Issue is related to smart contracts. labels Mar 29, 2023
@pgherveou
Copy link
Contributor Author

pgherveou commented Mar 30, 2023

@athei quick questions, before I take a second pass at this:

  • should we implement a generic verify function as demonstrated here
/// Ext defines a generic 
fn sig_verify(&self, payload: SigVerifierPayload) -> bool;

// each variant define the message, signature, and public key with the appropriate types
pub enum SigVerifierPayload {
    Sr25519 { ...  },
    Ed25519 { ... },
    Ecdsa { ... },
}

vs

what I started the other day

fn sr25519_verify(&self, signature: &[u8; 64], message: &[u8], pub_key: &[u8; 32]) -> bool;

and in this option here I am not sure if we want the message to be a fixed sized hash or just plain &[u8] as defined here

@command-bot
Copy link

command-bot bot commented Mar 30, 2023

@pgherveou Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet_contracts has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2614559 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2614559/artifacts/download.

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

should we implement a generic verify function as demonstrated here
https://github.com/AstarNetwork/astar-frame/pull/141/files#diff-16c73d79bacd21e8243b65478b218afa791cb812008c4f8055a549d03cd4862a

I would say we opt for individual host functions. We don't have precedence for multiplexed functions. Reason is that it makes benchmarking harder when we multiplex: The weight will depend on the argument which decides which algorithm to depley. Hence this PR should a function for sr25519 and one for ed25519.

Not too sure what are the exact use cases here, but do we want the message payload to be hashed so it's constant size or
should it just be an arbitrary payload

The message will be an arbitrary payload as demonstrated in this PR. Reason is that this is what the functions in sp_io::crypto expect. Weight needs to be linear over its length.

frame/contracts/src/exec.rs Outdated Show resolved Hide resolved
frame/contracts/src/wasm/runtime.rs Outdated Show resolved Hide resolved
frame/contracts/src/wasm/mod.rs Show resolved Hide resolved
frame/contracts/src/wasm/runtime.rs Outdated Show resolved Hide resolved
frame/contracts/src/tests.rs Outdated Show resolved Hide resolved
frame/contracts/fixtures/sr25519_verify.wat Outdated Show resolved Hide resolved
@pgherveou pgherveou requested a review from athei April 6, 2023 13:20
frame/contracts/src/tests.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@agryaznov agryaznov left a comment

Choose a reason for hiding this comment

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

only nitpicks

frame/contracts/src/wasm/runtime.rs Outdated Show resolved Hide resolved
frame/contracts/src/wasm/runtime.rs Outdated Show resolved Hide resolved
frame/contracts/src/schedule.rs Outdated Show resolved Hide resolved
frame/contracts/src/schedule.rs Outdated Show resolved Hide resolved
@pgherveou
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit b4f857e into master Apr 12, 2023
@paritytech-processbot paritytech-processbot bot deleted the pg/contracts-add-sr25519_recover branch April 12, 2023 14:49
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/april-updates-for-substrate-and-polkadot-devs/2764/1

nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* wip

* fix

* wip

* fix lint

* rm fixture fix

* missing comment

* fix lint

* add comment to the wsm file

* fix comment

* Apply suggestions from code review

Co-authored-by: Sasha Gryaznov <[email protected]>

* wip

* wip weights

* wip weights

* PR comment: test with return code

* wip

* PR review add mock test

* remove

* lint

* Update frame/contracts/fixtures/sr25519_verify.wat

* fix comments

* Update frame/contracts/src/benchmarking/mod.rs

* Update frame/contracts/src/wasm/runtime.rs

* Update frame/contracts/fixtures/sr25519_verify.wat

* Update frame/contracts/src/benchmarking/mod.rs

* fix lint

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* Update frame/contracts/src/wasm/runtime.rs

Co-authored-by: Alexander Theißen <[email protected]>

* PR: review use unstable + remove arbitrary index 4

* Add benchmark for calculating overhead of calling sr25519_verify

* fix message length encoding

* fix weights

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_contracts

* Apply suggestions from code review

* Update frame/contracts/src/wasm/runtime.rs

* Update frame/contracts/src/wasm/runtime.rs

* Update frame/contracts/src/benchmarking/mod.rs

* Update frame/contracts/src/benchmarking/mod.rs

* Update frame/contracts/src/schedule.rs

Co-authored-by: Sasha Gryaznov <[email protected]>

* Update frame/contracts/src/schedule.rs

Co-authored-by: Sasha Gryaznov <[email protected]>

* Update frame/contracts/src/wasm/runtime.rs

* Update frame/contracts/src/wasm/runtime.rs

Co-authored-by: Sasha Gryaznov <[email protected]>

* PR review

---------

Co-authored-by: Sasha Gryaznov <[email protected]>
Co-authored-by: command-bot <>
Co-authored-by: Alexander Theißen <[email protected]>
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. B1-note_worthy Changes should be noted in the 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 T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

contracts: Sr25519 (Schnorrkel) signature verification
5 participants