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

add_key_cost.function_call_cost_per_byte is not correctly calculated from #3037

Open
ailisp opened this issue Jul 24, 2020 · 9 comments
Open
Assignees
Labels
A-contract-runtime Area: contract compilation and execution, virtual machines, etc C-housekeeping Category: Refactoring, cleanups, code quality T-contract-runtime Team: issues relevant to the contract runtime team

Comments

@ailisp
Copy link
Member

ailisp commented Jul 24, 2020

We found #2621 introduced a typo, which cause add_key_cost.function_call_cost_per_byte 1000x more than correct. But besides that, there probably a copy-paste error in f024a28, this cause add_key_cost.function_call_cost_per_byte 10000x less than correct. As a result add_key_cost.function_call_cost_per_byte now is about 10x less than correct.

For correct I mean the result i got from rerun param estimator several time in between f024a289 and current master, in both cloud&locally, result is almostly not changed, so it's very likely correct

10x in add function call access key per byte cost is acceptable, we need to correct it some time in future, but must do it in a backward compatible way. In rust check protocol version and apply different fees.

@stale
Copy link

stale bot commented Jul 1, 2021

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label Jul 1, 2021
@bowenwang1996 bowenwang1996 added the A-contract-runtime Area: contract compilation and execution, virtual machines, etc label Jul 2, 2021
@stale stale bot removed the S-stale label Jul 2, 2021
@bowenwang1996 bowenwang1996 added the T-contract-runtime Team: issues relevant to the contract runtime team label Jul 2, 2021
@bowenwang1996
Copy link
Collaborator

@ailisp what is the status of this issue?

@ailisp
Copy link
Member Author

ailisp commented Jul 5, 2021

@bowenwang1996 Examined it's still not fixed. Most recent fee update to add_key_cost.function_call_cost_per_byte is this https://github.com/near/nearcore/commits/master/scripts/migrations/11-runtime-cost-adjustment.py, the number of this cost matches fees.rs, which is prior to this issue.
Should be fixable by run runtime param estimator and update cost in a protocol version upgrade I think

@bowenwang1996
Copy link
Collaborator

Okay. @Longarithm please keep this issue in mind when you recalculate the gas costs.

@stale
Copy link

stale bot commented Oct 3, 2021

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label Oct 3, 2021
@bowenwang1996
Copy link
Collaborator

@Longarithm is the recalculation done?

@stale stale bot removed the S-stale label Oct 3, 2021
@Longarithm Longarithm self-assigned this Oct 6, 2021
@Longarithm
Copy link
Member

@bowenwang1996 yes, together with data receipt costs: https://hackmd.io/w6ODyKjUReuuofXTuqdyFQ
I think nothing prevents us from updating add key costs. The costs difference in add_key_cost.function_call_cost_per_byte is ~6.25x, which is similar to what is stated in the issue. At the same time base costs can be reduced.

@stale
Copy link

stale bot commented Jan 4, 2022

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label Jan 4, 2022
@stale
Copy link

stale bot commented Apr 7, 2022

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label Apr 7, 2022
@akhi3030 akhi3030 removed the S-stale label Jul 8, 2022
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 C-housekeeping Category: Refactoring, cleanups, code quality T-contract-runtime Team: issues relevant to the contract runtime team
Projects
None yet
Development

No branches or pull requests

5 participants