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

expose ecdsa_sign_prehashed in sp-io #10119

Merged
merged 4 commits into from
Dec 11, 2021
Merged

expose ecdsa_sign_prehashed in sp-io #10119

merged 4 commits into from
Dec 11, 2021

Conversation

dt665m
Copy link
Contributor

@dt665m dt665m commented Oct 29, 2021

Add a new ecdsa_sign_prehashed trait function in sp-io's runtime_interface. The current ecdsa_sign function always hashes with blake2 for ecdsa signatures. Exposing this function opens up new use cases, such as allowing offchain workers to use the keystore to sign ethereum transactions, which uses keccak as the hashing scheme.

Related Issue: #10118

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Oct 29, 2021

User @dt665m, please sign the CLA here.

bkchr
bkchr approved these changes Oct 31, 2021
View reviewed changes
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

As this is used by offchain workers, I would approve this 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. E4-newhostfunctions labels Oct 31, 2021
tomusdrw
tomusdrw reviewed Nov 1, 2021
View reviewed changes
primitives/io/src/lib.rs
/// key type in the keystore.
///
/// Returns the signature.
fn ecdsa_sign_prehashed(
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth adding ecdsa_verify_prehashed for completeness of the API.

AFAICT recover already takes a hash, so that one should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Added in latest commit.

tomusdrw
tomusdrw approved these changes Nov 2, 2021
View reviewed changes
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm! Note this adds new host functions, so it's a breaking change (requires client to be updated first)

@bkchr
Copy link
Member

bkchr commented Nov 2, 2021

lgtm! Note this adds new host functions, so it's a breaking change (requires client to be updated first)

Only for clients that also use this function. Aka, Polkadot/Kusama will not require this.

@pepyakin
Copy link
Contributor

pepyakin commented Nov 2, 2021

Actually, polkadot and kusama does pull those, https://github.com/paritytech/polkadot/blob/4fe4d3f9e6efd6607f570094c6872516f54b9549/node/core/pvf/src/executor_intf.rs#L122 IIUC.

@bkchr
Copy link
Member

bkchr commented Nov 2, 2021

Actually, polkadot and kusama does pull those, https://github.com/paritytech/polkadot/blob/4fe4d3f9e6efd6607f570094c6872516f54b9549/node/core/pvf/src/executor_intf.rs#L122 IIUC.

I mean that this isn't used by the Polkadot/Kusama runtime itself. And yes, for Parachains that want to use this, they will need to wait until all validators have upgraded.

@stale
Copy link

stale bot commented Dec 2, 2021

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Dec 2, 2021
@bkchr
Copy link
Member

bkchr commented Dec 5, 2021

@dt665m sorry that this was overseen. Could you please merge master?

@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Dec 5, 2021
@dt665m
Copy link
Contributor Author

dt665m commented Dec 8, 2021

no worries! Merged, built, and tested locally.

@tomusdrw
Copy link
Contributor

tomusdrw commented Dec 9, 2021

@dt665m could you please reformat the code using cargo +nightly fmt?

@dt665m
Copy link
Contributor Author

dt665m commented Dec 10, 2021

@dt665m could you please reformat the code using cargo +nightly fmt?

done

@bkchr bkchr added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Dec 11, 2021
@bkchr bkchr merged commit c7a3e46 into paritytech:master Dec 11, 2021
seunlanlege pushed a commit to seunlanlege/substrate that referenced this pull request Dec 17, 2021
@dt665m @seunlanlege
expose ecdsa_sign_prehashed in sp-io (paritytech#10119)
77fb37e 
* expose ecdsa_sign_prehashed in sp-io

* add ecdsa_verify_prehashed to host functions for completeness

* cargo fmt
@tomaka tomaka mentioned this pull request Jan 31, 2022
Closed
@librelois librelois mentioned this pull request Feb 7, 2022
Closed
@kamilsa kamilsa mentioned this pull request Feb 7, 2022
Closed
2 tasks
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
@dt665m @grishasobol
expose ecdsa_sign_prehashed in sp-io (paritytech#10119)
af8e60f 
* expose ecdsa_sign_prehashed in sp-io

* add ecdsa_verify_prehashed to host functions for completeness

* cargo fmt
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@dt665m @ark0f
expose ecdsa_sign_prehashed in sp-io (paritytech#10119)
df99348 
* expose ecdsa_sign_prehashed in sp-io

* add ecdsa_verify_prehashed to host functions for completeness

* cargo fmt
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tomusdrw tomusdrw tomusdrw approved these changes

@bkchr bkchr bkchr approved these changes

@pepyakin pepyakin Awaiting requested review from pepyakin

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dt665m @bkchr @pepyakin @tomusdrw