-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Implemented seal_ecdsa_recovery
function in the contract pallet
#9686
Conversation
Added benchmark and unit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your great work. Looks very good already. Almost all of my comments are rather minor in nature. Please make sure to merge master and run cargo +nightly fmt
in order to pass the CI. The unleash check can be ignored.
# Conflicts: # frame/contracts/src/wasm/runtime.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making good progress.
I set weights based on the result of my local machine, I will update it after CI.
I will run the benchbot on this PR once all comments are addressed and CI is green. No need for you to do anything in this department.
Co-authored-by: Alexander Theißen <alex.theissen@me.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Before merging some stuff needs to be done:
- Improve instruction benchmarks #9712 needs to be merged and master needs to be merged here after that
- I need to run benchbot in this PR
LGTM. Want to hold off with merging a bit, though.
Co-authored-by: Alexander Theißen <alex.theissen@me.com>
/benchmark runtime pallet pallet_contracts |
Benchmark Runtime Pallet for branch "feature/ecdsa_recovery" with command cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_contracts --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/contracts/src/weights.rs --template=./.maintain/frame-weight-template.hbs Results
ERROR: Unable to push ./frame/contracts/src/weights.rs |
@xgreenx Can you please merge master? Also, can you please check the box to allow edits by maintainers. Otherwise the bot can't publish the results. |
I don't see this checkbox=( Seems because I created PR from the organization isaacs/github#1681. |
Maybe I can push weights from the bot manually? Or I can create this PR from my own account(not from the organization). |
It is because the PR was created from a fork (the only way as an external contributor). There is a checkbox to allow edits when creating the PR: https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork Not sure if you can never change that for an existing PR or whether this is because this is an organization account.
I will send you the file. |
@xgreenx This is the result from the benchbot. Kudos to @joao-paulo-parity for recovering this file. Extract the source file and overwrite the existing |
CI is flaky at the moment. The failure is not related to the changes here. |
// hash is placed. Should be decodable as a 32 bytes. Traps otherwise. | ||
// - `output_ptr`: the pointer into the linear memory where the output | ||
// data is placed. The buffer should be 33 bytes. Traps otherwise. | ||
// The function will write the result directly into this buffer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation in the lines above is messed up, probably tabs vs. spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the perils of function like macros. Someone really should convert this to an attribute macro so we can use rustfmt there 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice that you implemented this, @xgreenx !
I just added some nitpicks regarding consistency of the comments, but it looks good to me besides that. Are you planning on doing the ink! side as well?
Yes, the change for ink! is ready, I will create PR after merging of this=) |
Co-authored-by: Michael Müller <mich@elmueller.net>
Awesome! Can you merge master again? Just to make sure CI failing is not caused by this. |
You need to merge master one last time. There was a change that is needed. |
bot merge |
Trying merge. |
…ritytech#9686) * Implemented `seal_ecdsa_recovery` function in the contract pallet. Added benchmark and unit test. * Run `cargo fmt` * Skip fmt for slices * Changes according comments in pull request. * Fix build without `unstable-interface` feature * Applied suggestion from the review * Apply suggestions from code review Co-authored-by: Alexander Theißen <alex.theissen@me.com> * Apply suggestions from code review Co-authored-by: Alexander Theißen <alex.theissen@me.com> * Changed RecoveryFailed to EcdsaRecoverFailed * Manually updated weights.rs * Apply suggestions from code review Co-authored-by: Michael Müller <mich@elmueller.net> Co-authored-by: Alexander Theißen <alex.theissen@me.com> Co-authored-by: Michael Müller <mich@elmueller.net>
This PR fixes #9609.
Based on the signature(65 bytes, where the last byte is recovery id) and the message hash(it is 32 bytes, so the developer must hash the message before using the recovery function),
seal_ecdsa_recovery
returns compressed ECDSA public key(33 bytes).We know the input arguments size and output size, so the function doesn't take the length of buffers.
Benchmark and unit tests are added.
I set weights based on the result of my local machine, I will update it after CI.