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

Add precompiles for pallet_evm operators: blake2f, modexp, bn128, ed25519verify#7016

Closed
drewstone wants to merge 5 commits intoparitytech:masterfrom
drewstone:drew-precompiles
Closed

Add precompiles for pallet_evm operators: blake2f, modexp, bn128, ed25519verify#7016
drewstone wants to merge 5 commits intoparitytech:masterfrom
drewstone:drew-precompiles

Conversation

@drewstone
Copy link
Contributor

Adds some precompiles to the repo for functionality that may or may not exist on other blockchains but are useful nonetheless. These are not tested as I'm unsure how to test them yet but most of it has been migrated from openethereum and existing EIPs.

The modexp one in particular is unsure to be correct as IO between the rust openethereum client and the substrate implementation are a bit different. I'm unsure how to replicate certain IO operations so I opted to manually implement the input parsing.

@drewstone
Copy link
Contributor Author

Currently BLS precompile cannot be added until I find a no_std compatible repo for doing elilptic curve operations on the BLS381 curve. The repo here mentions adding this functionality but has not indicated it is completed: https://github.com/matter-labs/eip1962

@drewstone drewstone changed the title Add precompiles: blake2f, modexp, bn128, ed25519verify Add precompiles for pallet_evm operators: blake2f, modexp, bn128, ed25519verify Sep 3, 2020
Copy link
Contributor

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. The testing error seems to be something unrelated (but we still need to figure out and fix it).

Only grumble is if we can add feature gates for the additional dependencies. Then we enable those feature gates by default for ease of testing.

@bkchr
Copy link
Member

bkchr commented Sep 8, 2020

@drewstone any updates?

@drewstone
Copy link
Contributor Author

Welp, now I'm getting std errors trying to work around this...

@drewstone
Copy link
Contributor Author

Alright all updated without changing wabt

@drewstone
Copy link
Contributor Author

Just updated again, thanks @NikVolf for the suggestion!

@gnunicorn
Copy link
Contributor

gnunicorn commented Oct 1, 2020

As this touches evm, @sorpaas should sign off at least.

@gnunicorn gnunicorn added A3-needsresolving A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. labels Oct 1, 2020
@sorpaas
Copy link
Contributor

sorpaas commented Oct 1, 2020

Still hoping @drewstone can add the feature gates as requested in the last review! It's otherwise LGTM.

@drewstone
Copy link
Contributor Author

Will get to this this week! Sorry for the delay @sorpaas

@drewstone
Copy link
Contributor Author

drewstone commented Oct 15, 2020

@sorpaas I added the feature flags. Lmk what you think!

Copy link
Contributor

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me, but needs to:

  1. Check in the updated lock file.
  2. Fix line width CI.
  3. Fix the styling nits.

drewstone and others added 2 commits October 16, 2020 20:26
Co-authored-by: Wei Tang <accounts@that.world>
@drewstone drewstone requested a review from sorpaas October 27, 2020 20:04
Comment on lines +290 to +301
impl Precompile for Blake2F {
fn execute(
input: &[u8],
target_gas: Option<usize>,
) -> core::result::Result<(ExitSucceed, Vec<u8>, usize), ExitError> {
let cost = ensure_linear_cost(target_gas, input.len(), 15, 3)?;
let mut state = Blake2b::new(64);
state.update(&input);
let ret = state.finalize().as_bytes().to_vec();
Ok((ExitSucceed::Returned, ret, cost))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to be the EIP-152 precompile, but rather the full Blake2 hash function.

@bkchr
Copy link
Member

bkchr commented Nov 24, 2020

Pallet EVM was removed, so this pr is not required in Substrate anymore.

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. A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants