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

Re-evaluate EVM host functions using new param estimator #4778

Closed
MaksymZavershynskyi opened this issue Sep 1, 2021 · 2 comments
Closed
Assignees
Labels
A-contract-runtime Area: contract compilation and execution, virtual machines, etc P-low Priority: low T-Aurora Team: issues relevant to the Aurora team T-contract-runtime Team: issues relevant to the contract runtime team

Comments

@MaksymZavershynskyi
Copy link
Contributor

@matklad is working on new param estimator and he has an intermediate result. It is much more intuitive and diagnosable. We should re-evaluate EVM host functions using this param estimator and see if the fees are getting reduced.

@MaksymZavershynskyi MaksymZavershynskyi added A-contract-runtime Area: contract compilation and execution, virtual machines, etc T-contract-runtime Team: issues relevant to the contract runtime team T-Aurora Team: issues relevant to the Aurora team labels Sep 1, 2021
@MaksymZavershynskyi MaksymZavershynskyi added the P-low Priority: low label Sep 1, 2021
@joshuajbouw joshuajbouw self-assigned this Sep 10, 2021
@joshuajbouw
Copy link
Member

As per discussion with @matklad a week ago (I should've updated this, thats on me), he ran into an issue and will let me know when this is ready for us to re-run the param estimator.

@artob artob assigned matklad and unassigned joshuajbouw, birchmd and mfornet Sep 22, 2021
@matklad
Copy link
Contributor

matklad commented Sep 22, 2021

The costs we use today are:

Ripemd160Base                                     284_558_362
Ripemd160Block                                    226_702_528
EcrecoverBase                               1_121_789_875_000

My understanding is that they were estimating using this code (the numbers line up with the ones from #4548).

The costs I get locally with the new estimator using our standard approach to estimation (via wasm contract) are:

Ripemd160Base                                     690_977_200
Ripemd160Block                                    309_200_758
EcrecoverBase                                  92_940_662_819

Command to reproduce this numbers locally:

# switch to in-progress estimator branch 
$ git clone git@github.com:matklad/nearcore.git && cd nearcore
$ git switch --detach 13faa28d4fce661ec674ab8cb458fc7a711c52fe

$ cargo run --release -p runtime-params-estimator --features required -- --docker --v2 --full \
  --metrics-to-measure Ripemd160Base,Ripemd160Block,EcrecoverBase

These "new estimator" costs roughly match the "old estimator" costs from #4548.

The ecrecover cost is substantially lower in my estimates (10x) than what we currently use. This is explained in #4548 (comment), the new cost is correct, the old cost is too big due to erroneous benchmark. So,

action item for aurora team: lower EcrecoverBase cost to ~93*10**9 (per multiplier). See #4795 for cost lowering process (this requires a protocol upgrade).

The ripemd costs are higher in my estimate. That's because "wasm contract" approach is conservative -- it effectively summs the costs of hash computation and reading the input. Manually adding those costs to our current costs (while accounting for block/byte distinction) gives numbers similar to my estimates. So, while the estimator gives a higher number, this is OK, as it is, by design, an overestimation. Note that, we do charge both Ripemd160Base and ReadMemoryBase when running the host function, and the same for per byte costs. To sum up, the cost we currently use for Ripemd is correct, it's just more tight than most other hash function costs.

Action items for contract runtime team:

  • evaluate if it's time for us to replace conservative estimates from "wam contract" with something more tight.
  • take a closer look at ReadMemoryBase cost -- in my quick tests, it seems significantly cheaper than what we have today. In fact, it seems that it is 3x cheaper, and what we have today is divisible by 3, so maybe we accidentally safely multiplied the cost twice.

I am going to close this issue, feel free to open a follow up for EcrecoverBase upgrade!

@matklad matklad closed this as completed Sep 22, 2021
near-bulldozer bot pushed a commit that referenced this issue Oct 8, 2021
Stabilize features lowering costs for new release:
* #4795
* #4865

Quality control:

* We run param estimator several times and got consistent results: 
  * beginning of Sep 2021, my GCP instance https://hackmd.io/w6ODyKjUReuuofXTuqdyFQ
  * end of Sep 2021, @matklad instance #4778 (comment)

* The current fee values are explained:
  * Data receipt costs - investigated here #4482, the reason was relatively explained and the observed issue was fixed. Note that we don't know the exact root cause, but we assume that it is related to a separate fee (touching_trie_node) for taking store size into account. This fee is problematic because it assumes the constant height of a trie, but we treat it as a separate problem.
  * Ecrecover cost - follow links from this one: #4778 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contract-runtime Area: contract compilation and execution, virtual machines, etc P-low Priority: low T-Aurora Team: issues relevant to the Aurora team T-contract-runtime Team: issues relevant to the contract runtime team
Projects
None yet
Development

No branches or pull requests

5 participants