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

Aurora Engine Benchmarks #41

Merged
merged 17 commits into from
May 4, 2021
Merged

Conversation

birchmd
Copy link
Member

@birchmd birchmd commented Apr 22, 2021

The Near runtime has the following requirements:

  1. Regardless of what work is done in a transaction (computation, io, etc), Near gas used should always be proportional to the wall clock execution time
  2. This proportionality should be such that 1000 Tgas can be consumed in < 1s (otherwise Near's blocktime can be thrown off)

The purpose of this PR is to create benchmarks involving the EVM wasm contract to test these requirements hold.

Wall-clock time is an inherently volatile metric because many factors contribute to it, so results from this should be taken with a grain of salt; though great deviations from expectations should be investigated more closely.

The EVM contract is executed within the near-vm-runner. This also records the Near gas spent on the computation. The wall clock measurements are done using criterion.

Two work loads are considered:

  • Transferring eth between two accounts
  • Deploying an eth smart contract (of various sizes)

Note: these work loads are done as eth transactions sent to the EVM contract, so both also involve checking the validity of the transaction which includes a Keccak256 hash computation and Secp256k1 signature check. (In fact, this is likely the bulk of the work involved).

Using the timing measurements from criterion and the gas measurements from the near-vm-runner, we can measure the relationship between wall-clock time and gas used.

Known limitation: all state is kept in memory, so IO time may not be accurately accounted for.

Closes #31

@birchmd birchmd requested a review from ilblackdragon April 22, 2021 21:48
@artob artob added the C-enhancement Category: New feature or request label Apr 26, 2021
@birchmd birchmd marked this pull request as ready for review April 28, 2021 14:43
@birchmd birchmd requested a review from artob as a code owner April 28, 2021 14:43
@birchmd birchmd requested review from joshuajbouw and maxzaver April 28, 2021 14:43
@birchmd
Copy link
Member Author

birchmd commented Apr 28, 2021

I think this is in good enough shape to review (and merge if there are no changes requested). There are a couple remaining TODO items that I have captured as issues, but they are not blocking to this PR in my opinion.

This PR is important because it contains some of the only tests we have for some features. Creating the PR even revealed some bugs which could then be fixed (#42 #44). We will focus more on testing in the future as well of course.

The original impetus for this PR was to benchmark the Near runtime using the EVM contract, as well as get a sense for the cost of common EVM operations. the results of this are summarized in the plot below (note: I've called out a few points of interest in orange, but all the points are present from the benchmarks in this PR):

aurora_benchmark

Some takeaways from this:

  • Due to signature checking currently happening in wasm itself (instead of being a host function in the runtime), everything is very expensive in terms of Near gas. This makes getting the new host functions in place very important for usability. Extend the Math API with EVM precompiles near/nearcore#3954
  • The points generally lie on a straight line, which is good for our assumption that time is proportional to gas usage. However, the slope of the line looks too steep to me. It means it would take ~1.3 seconds to consume 1000 Tgas, which I think is too long based on Near's target block time (please correct me if I am wrong cc @bowenwang1996 @ailisp @olonho)
  • There is some deviation from the straight line at the top end gas usage, which I am not sure how to reconcile.

@joshuajbouw
Copy link
Contributor

joshuajbouw commented Apr 28, 2021

Interesting. Shoot. I didn't expect them to be so high, but makes sense given they are backed into the contract. That really adds a lot more importance on near/nearcore#3954.

@artob artob assigned artob and unassigned birchmd Apr 28, 2021
@alexauroradev
Copy link

@birchmd can you please create a plot NEAR Gas vs. Ethereum Gas. This will help us to determine the recalculation formula + estimate what size of a transaction we can accommodate within 300 Tgas.

Copy link
Contributor

@joshuajbouw joshuajbouw left a comment

Choose a reason for hiding this comment

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

Looks really good. Nothing to really note or point out from me. Good work.

benches/eth_deploy_code.rs Show resolved Hide resolved
@birchmd
Copy link
Member Author

birchmd commented Apr 29, 2021

@djsatok The graph you asked for is below:

eth_gas_benchmark

The correlation here is not very strong. The differences seem to be that we charge much more for pre-compiles, and much less for deploying code. Both of these make sense to me. Our "pre-compiles" are actually executing in wasm right now, so the gas cost is far off what it should be; again pointing to the importance of near/nearcore#3954 . And I think Near storage is supposed to be much cheaper in terms of gas than on Eth because of the "storage staking" concept.

The Eth transfer and ERC20 transfer both happen to lie close-ish to the linear regression line, so maybe it could be used to give a rough estimate of the relationship between Near and Eth gas (obviously it totally does not work in the extreme cases of transactions that are all storage or all pre-compiles). Using that measure, you should be able to fit approximately 230,000 Eth gas into 300 Near Tgas.

@artob artob assigned birchmd and unassigned artob Apr 30, 2021
@artob
Copy link
Contributor

artob commented Apr 30, 2021

@birchmd Assigning this back to you to wrap up review discussions. Please assign to me once ready to merge.

@birchmd birchmd assigned artob and unassigned birchmd Apr 30, 2021
@birchmd
Copy link
Member Author

birchmd commented Apr 30, 2021

Back to you @artob

Should be good to merge once CI finishes.

Note: there will need to be a follow-up PR after #48 lands because it changes the output format of the contract, and I rely on the output to learn the address of deployed contracts (e.g. ERC-20). So once that PR is merged we'll need to fix the benchmarks to consume the new output format. I can take care of that though, should be quite small.

Copy link
Contributor

@joshuajbouw joshuajbouw left a comment

Choose a reason for hiding this comment

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

New changes LGTM.

@artob
Copy link
Contributor

artob commented May 3, 2021

Note: there will need to be a follow-up PR after #48 lands because it changes the output format of the contract, and I rely on the output to learn the address of deployed contracts (e.g. ERC-20). So once that PR is merged we'll need to fix the benchmarks to consume the new output format. I can take care of that though, should be quite small.

@birchmd I've merged #48 just now. Please update this as needed and then reassign back to me for final review and merge.

@artob artob assigned birchmd and unassigned artob May 3, 2021
Copy link
Contributor

@joshuajbouw joshuajbouw left a comment

Choose a reason for hiding this comment

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

New changes look fine.

@birchmd
Copy link
Member Author

birchmd commented May 3, 2021

@artob should be good to go now

@birchmd birchmd assigned artob and unassigned birchmd May 3, 2021
Copy link
Contributor

@artob artob left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Really great work, @birchmd!

@artob artob merged commit a3996cb into aurora-is-near:master May 4, 2021
@bowenwang1996
Copy link

@birchmd any idea what the benchmark would be if the precompiles are executed as host functions? I am not sure what host functions are needed, but I believe some are already available on nightly protocol.

@birchmd
Copy link
Member Author

birchmd commented May 4, 2021

@bowenwang1996
@djsatok also asked me to get an estimate with precompiles as host functions. I thought the biggest contribution to current gas costs was probably ecrecover, so I ran a benchmark with that one as a host function using the work from near/nearcore#3954 . With that change, an eth transfer drops to around 1Tgas and and erc20 transfer drops to around 15 Tgas. In short, it was a very big improvement.

I didn't run all the benchmarks or re-generate the Tgas vs time graph, but I can do that if you are interested.

The code with ecrecover integrated as a host function is in a branch on my fork of the repo. https://github.com/birchmd/aurora-engine/tree/evm-benchmark-with-math-api

mrLSD pushed a commit that referenced this pull request May 5, 2021
* Link to docs in the README. (#18)

* Change deprecated `u64::max_value` to `u64::MAX`. (#38)

* Support custom error messages. (#40)

* Implement `begin_chain` for evm-bully. (#30)

* Implement a faucet method. (#39)

* Implement all Istanbul HF precompiles. (#21)

* Check and increment nonces. (#42)

* Fix the RIPEMD160 and ModExp precompiles. (#44)

* Implement a first draft of `COINBASE` and `GASLIMIT`. (#47)

* Refactor and improve error handling. (#49)

* Replace `raw_call` with the new `submit` API. (#48)

The `raw_call` method is hereby removed in favor of the new `submit` method
that has an extended ABI capable of returning a transaction's revert status
and logged events.

Co-authored-by: Michael Birch <[email protected]>
Co-authored-by: Arto Bendiken <[email protected]>

* Add benchmarks for common EVM operations. (#41)

* Merge branch 'master' into improved-evm-token-logic

* Update error handling to `master`

* fix missing import

* cargo fmt

* Ensure ETH transfers return an execution result. (#48)

* Update to `master`

* fix str types

Co-authored-by: Frank Braun <[email protected]>
Co-authored-by: Michael Birch <[email protected]>
Co-authored-by: Arto Bendiken <[email protected]>
mrLSD added a commit that referenced this pull request May 5, 2021
* Added prover & extended types

* Mode Borsh args from types to parameters

* Added fungible tokens

* Added eth-connector

* Modify assert for fee

* Fix formatting

* Extend eth-conenctor with EVM token logic

* Changed eth-connector deposit logic

* Added changes for ETH deposit/withdraw and Engine changes

* Mint ETH-tokens

* Added: transfer_eth

* ETH withdraw basic method

* eth-withdraw validation structure & modified Deposit-eth fields

* eth transfer and withdraw logic

* eip712 message verifier - started encoding

* added encode-packed

* virefy EIP712 message for withdraw

* Changed EIP712 message fields

* Modify logs for EIP712 messages

* Test EIP712

* Tests EIP712

* Integration tests for encode_withdraw_eip712

* Integration tests for encode_withdraw_eip712

* EIP712-Withdraw: improvements and fixes.

* EIP712-Withdraw: fixed encoding rules and order.
* EIP712-Withdraw: `verify_withdraw_eip712` returns `true` only if the
  sender address equals to the address of message signer.
* EIP712-Withdraw: update tests.
* EIP712-Withdraw: refactoring.
* ethabit::encode_token_packed: use right-padded encoding for `Address`.
* WithdrawFromEthCallArgs: fixed `amount` type conversion.

* Extend tests for eth-connector

* eth-connector test deposit & balance & total_supply

* Imporved tests

* FT tests

* Fixed verify_transfer_eip712

* Change test_withdraw_near

* Tests for: ft_transfer, ft_transfer_call

* test_eth_deposit_balance_total_supply

* test and ifx: deposit_eth, withdraw_near

* References in fungible token (#29)

* Use references in fungible_token to avoid cloning

* cargo fmt

* Fix: hide logging behind feature flag

* Remove eth-conenctor transfer methods and deposit for new design

* Completed Deposit logic

* Fix clippy; added comments; improved ft_transfer_call

* Extend external functions for eth-connector

* Added deploy_evm_token

* Added ft_on_transfer logic

* Changed ft_on_transfer & remove json depends

* Changed deposit logic and fixed transfers

* Added register relayer

* Added message coder for ft_transfer_call

* ft_on_transfer - added logic for erc20

* Impoved ft_on_transfer

* ft_on_transfer: call erc20 contract adn send fee to Relayer. Added logs

* eth-connector: Removed unsued methods

* tests: deposit & fixed init test

* tests: depoist, withdraw

* tests: fix test_withdraw_near

* Eth-connector: never skip bridge call.

* Tests: fix ft_transfer_call

* ft_transfer_call - changed gas amountr

* Fixed: test_eth_deposit_balance_total_supply, test_ft_transfer

* Added: test_ft_transfer_call_near_eth

* Clippy fix

* Added test_ft_transfer_call_erc20

* Added test_ft_transfer_call_erc20

* tests: ft_transfer_call for ERC20 changes

* Fix finish_deposit - promise flow when failed for ft_transfer_call

* added: test_deposit_with_same_proof

* Improved EVM token master branch update (#50)

* Link to docs in the README. (#18)

* Change deprecated `u64::max_value` to `u64::MAX`. (#38)

* Support custom error messages. (#40)

* Implement `begin_chain` for evm-bully. (#30)

* Implement a faucet method. (#39)

* Implement all Istanbul HF precompiles. (#21)

* Check and increment nonces. (#42)

* Fix the RIPEMD160 and ModExp precompiles. (#44)

* Implement a first draft of `COINBASE` and `GASLIMIT`. (#47)

* Refactor and improve error handling. (#49)

* Replace `raw_call` with the new `submit` API. (#48)

The `raw_call` method is hereby removed in favor of the new `submit` method
that has an extended ABI capable of returning a transaction's revert status
and logged events.

Co-authored-by: Michael Birch <[email protected]>
Co-authored-by: Arto Bendiken <[email protected]>

* Add benchmarks for common EVM operations. (#41)

* Merge branch 'master' into improved-evm-token-logic

* Update error handling to `master`

* fix missing import

* cargo fmt

* Ensure ETH transfers return an execution result. (#48)

* Update to `master`

* fix str types

Co-authored-by: Frank Braun <[email protected]>
Co-authored-by: Michael Birch <[email protected]>
Co-authored-by: Arto Bendiken <[email protected]>

* Update src/connector.rs misspel

Co-authored-by: Joshua J. Bouw <[email protected]>

* Update src/connector.rs change constants error

Co-authored-by: Joshua J. Bouw <[email protected]>

Co-authored-by: Septen <[email protected]>
Co-authored-by: Michael Birch <[email protected]>
Co-authored-by: Joshua J. Bouw <[email protected]>
Co-authored-by: Frank Braun <[email protected]>
Co-authored-by: Arto Bendiken <[email protected]>
@bowenwang1996
Copy link

@birchmd I see. Do we need any other host functions that have not been implemented in nearcore yet?

@birchmd birchmd deleted the evm-benchmark branch May 5, 2021 19:52
@birchmd
Copy link
Member Author

birchmd commented May 5, 2021

@bowenwang1996 As far as I know everything in that nearcore PR is needed. I'm not sure how often each precompile is used in real Eth contracts, so I don't know if some could be considered higher priority than others. But I do know that the EVM contract itself uses ecrecover to validate transactions it receives which is why it is very important it can be executed efficiently.

I don't know if there are other host functions which are needed and not implemented. Maybe @artob or @joshuajbouw would know if there is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collect benchmarking data for runtime improvements
6 participants