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

Extend the Math API with EVM precompiles #3954

Closed
wants to merge 96 commits into from
Closed

Extend the Math API with EVM precompiles #3954

wants to merge 96 commits into from

Conversation

artob
Copy link
Contributor

@artob artob commented Feb 12, 2021

@artob artob added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) A-cryptography Area: Cryptography A-EVM Area: Native EVM implementation and support T-public-interfaces Team: issues relevant to the public interfaces team labels Feb 12, 2021
@artob artob self-assigned this Feb 12, 2021
Copy link
Member

@ailisp ailisp left a comment

Choose a reason for hiding this comment

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

Code looks good. This is considered as a protocol version change, so we need to introduce a new protocol feature & protocol version and import it with guard:

// #["protocol_feature_evm", EVM] test_api<[a: u64] -> []>,

@artob artob force-pushed the evm-math-api branch 3 times, most recently from e5afcb9 to f03aca2 Compare February 19, 2021 03:30
@artob artob requested a review from ailisp February 19, 2021 03:31
@artob
Copy link
Contributor Author

artob commented Feb 19, 2021

@ailisp Please re-review, this is substantially closer to graduating from draft status.

The top open question I still have is how, concretely, I should go about measuring and assigning the gas costs for the new functions. (Also, the runtime-params-estimator exits with a failure, so I may still be missing something in that regard.)

Code looks good. This is considered as a protocol version change, so we need to introduce a new protocol feature & protocol version and import it with guard:

Thanks. I've guarded everything new behind the EVM feature for now.

@ilblackdragon
Copy link
Member

ilblackdragon commented Feb 19, 2021

I also think this should be a separate feature from EVM, so that it can be released separately from the full precompiled EVM.

This can be (and probably should be) batched with bn128 host functions.
And as I mentioned a really useful would be to have host function for current storage cost.

@ailisp
Copy link
Member

ailisp commented Feb 19, 2021

Looks like runtime param estimator tests failed because it isn't compiled with protocol-feature-evm enabled, so those new functions isn't exported:

https://buildkite.com/nearprotocol/nearcore/builds/5739#17b95e82-bc41-4b95-b620-44d8c5b42c89

I suggest:

  1. change ci config to make it compile and pass. On CI it is using Time metric, which is fast to run, but isn't the one we use as actual cost.
  2. Actual cost we use "count of CPU instructions on qemu", this takes 6+ hours to run, boot a cloud instance (at least 4 cpu 16g ram) and update runtime-params-estimator/estimate.sh to compile with protocol-feature-evm. Then run estimate.sh. Steps are here:

Use this to build a docker image:
https://github.com/near/nearcore/blob/master/runtime/runtime-params-estimator/emu-cost/README.md#usage

once in docker, disregard above steps, simply run estimate.sh
https://github.com/near/nearcore/blob/master/runtime/runtime-params-estimator/estimate.sh

@abacabadabacaba
Copy link
Collaborator

There is a problem with the BLAKE2b function added here. While this PR adds the whole BLAKE2b function, what EVM should provide to smart contracts is the compression function F (see EIP 152), and you can't use the whole BLAKE2b function to implement just the compression function.

While we can add just the function F as a host function, it would be preferred that the function is usable not just for the EVM but also for other contracts that may want to compute BLAKE2 hashes. So, I think that a better idea is to add a function that evaluates a sequence of calls to the compression function in a way similar to how the BLAKE2 function itself does it. Such function could be used to perform a single compression function call (as needed by the EVM), and it can also compute the BLAKE2 hash itself in a single call.

Another thing that I would have changed is to charge gas proportional to the number of blocks, not bytes.

@artob artob assigned artob and unassigned artob Mar 4, 2021
@artob
Copy link
Contributor Author

artob commented Mar 4, 2021

@joshuajbouw I will not have time to come back to this for a bit, so please take over this task and push it to completion. The subtasks I currently know about include the following:

  • Investigate if/how we can batch this with feat: Add precompiled contracts for alt_bn128 curve [rebased] #3971, already merged (and also a good example of adding new host functions)
  • Add any and all missing feature guards, as per @ilblackdragon's comment
  • Change the feature guards in this PR to be based on a new distinct feature name, as per @ilblackdragon's comment
  • Flesh out the ecrecover_10k() test case for the runtime-params-estimator, as per @ailisp's comment
  • Add support for EIP-152 (the BLAKE2 F compression function) as per @abacabadabacaba's comment; I would retain the already implemented BLAKE2b function, so this would be a new function
  • Measure and assign the gas costs for the new functions using the runtime-params-estimator, as per @ailisp's comment; for any support on that, ping @ailisp
  • Move the PR out of Draft status and re-request reviews once ready to go

@artob artob assigned artob and unassigned artob Mar 5, 2021
@joshuajbouw
Copy link
Member

Ok, yes will go through all that today and this weekend.

@artob artob removed their assignment Mar 5, 2021
runtime/near-vm-logic/src/logic.rs Outdated Show resolved Hide resolved
runtime/near-vm-logic/src/logic.rs Outdated Show resolved Hide resolved
runtime/near-vm-logic/src/logic.rs Outdated Show resolved Hide resolved
runtime/near-vm-logic/src/logic.rs Outdated Show resolved Hide resolved
runtime/near-vm-logic/src/logic.rs Outdated Show resolved Hide resolved
runtime/near-vm-logic/src/logic.rs Outdated Show resolved Hide resolved
@joshuajbouw joshuajbouw requested a review from matklad June 19, 2021 05:55
@joshuajbouw
Copy link
Member

Had to import two additional crates num-integer and byte-slice-cast, they are listed in our Cargo.lock as our dependencies do use it as well. Would be great if we had our own library or something similar to add in this missing functionality / unstable API from std.

@joshuajbouw
Copy link
Member

Closing in favour of a new PR with blake2 descoped as we have to ensure that it is completely correct. Ecrecover is the most important method that we need in the PR for Aurora.

@joshuajbouw joshuajbouw deleted the evm-math-api branch June 30, 2021 13:16
@joshuajbouw joshuajbouw restored the evm-math-api branch June 30, 2021 13:16
@Ekleog-NEAR Ekleog-NEAR deleted the evm-math-api branch March 29, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cryptography Area: Cryptography A-EVM Area: Native EVM implementation and support A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) C-enhancement Category: An issue proposing an enhancement or a PR with one. T-public-interfaces Team: issues relevant to the public interfaces team
Projects
None yet
Development

Successfully merging this pull request may close these issues.