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

Fix contract call fee issue #3158

Closed
MaksymZavershynskyi opened this issue Aug 13, 2020 · 10 comments
Closed

Fix contract call fee issue #3158

MaksymZavershynskyi opened this issue Aug 13, 2020 · 10 comments
Assignees
Labels
A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) C-bug Category: This is a bug P-critical Priority: critical

Comments

@MaksymZavershynskyi
Copy link
Contributor

We have discovered that we were underpricing contract call fee due to a bug in param estimator tool that was hidden by a bug introduced in this PR: https://github.com/nearprotocol/nearcore/pull/2845/files#diff-a811f7fb6a4892a2b6cc0098f7f68616R567. It turns out 300KiB contracts would cost >100Tgas to compile which is prohibitively expensive for our partners. According to @willemneal this is not only a Wasmer issue, but Wasmtime+lightbeam have the same CPU cost.

We need to fix it urgently and delay Mainnet launch until we do so CC @SkidanovAlex , @bowenwang1996 Things that we need to do:

  • @olonho or @evgenykuzyakov to independently double check @willemneal 's results that Wasmtime+lightbeam and Wasmer+singlepass produce almost exactly the same number of CPU instructions during compilation. This coincidence looks suspicious for two completely different backends with different architectures;
  • @olonho, @willemneal, @evgenykuzyakov need to investigate whether there are low-hanging fruits in Wasmer+singlepass compilation. E.g. why does compilation cost so much, isn't it supposed to be JIT?
  • @evgenykuzyakov should start working on implementing [Proposal] Differentiate cold and hot contracts NEPs#97 . Good news, this might boost our contract call TPS significantly overall.
@MaksymZavershynskyi MaksymZavershynskyi added C-bug Category: This is a bug P-critical Priority: critical A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) labels Aug 13, 2020
@mikedotexe
Copy link

Please advise if this affects if/how we should communicate to launch partners when you have a free moment.

@evgenykuzyakov
Copy link
Collaborator

Please comment on near/NEPs#97 (comment)
Does this work?

@frol
Copy link
Collaborator

frol commented Aug 14, 2020

Consider testing the rewrite of wasmer (wasmer-reborn): https://github.com/wasmerio/wasmer-reborn

@bowenwang1996
Copy link
Collaborator

@frol looks like the repo doesn't exist

@frol
Copy link
Collaborator

frol commented Aug 14, 2020

It seems they had made it private again... Should we ask them to share it with us? 😃

@willemneal
Copy link
Contributor

It seems risky to try a new VM that is less tested. We could revisit when it's stabilized.

@bowenwang1996
Copy link
Collaborator

@frol looks like it's already merged wasmerio/wasmer@518e0f3

@MaksymZavershynskyi
Copy link
Contributor Author

Setting estimate to 5 -- how much time it is going to take in this cycle.

evgenykuzyakov pushed a commit that referenced this issue Oct 12, 2020
Move `RuntimeConfig` from `Runtime` initialization to `ApplyState` object.
This makes `Runtime` completely stateless and allows to execute transitions based on different configs.
It allows to upgrade `RuntimeConfig` with the new fees based on the protocol version for the current block.

This change should be NOOP and doesn't change the protocol. 

NEP: near/NEPs#120
Issue: #3158

# Test plan:

- [X] CI
- [x] http://nayduck.eastus.cloudapp.azure.com:3000/#/run/553
olonho pushed a commit that referenced this issue Oct 15, 2020
Move `RuntimeConfig` from `Runtime` initialization to `ApplyState` object.
This makes `Runtime` completely stateless and allows to execute transitions based on different configs.
It allows to upgrade `RuntimeConfig` with the new fees based on the protocol version for the current block.

This change should be NOOP and doesn't change the protocol. 

NEP: near/NEPs#120
Issue: #3158

# Test plan:

- [X] CI
- [x] http://nayduck.eastus.cloudapp.azure.com:3000/#/run/553
olonho pushed a commit that referenced this issue Oct 19, 2020
Move `RuntimeConfig` from `Runtime` initialization to `ApplyState` object.
This makes `Runtime` completely stateless and allows to execute transitions based on different configs.
It allows to upgrade `RuntimeConfig` with the new fees based on the protocol version for the current block.

This change should be NOOP and doesn't change the protocol. 

NEP: near/NEPs#120
Issue: #3158

# Test plan:

- [X] CI
- [x] http://nayduck.eastus.cloudapp.azure.com:3000/#/run/553
chefsale pushed a commit that referenced this issue Oct 19, 2020
Move `RuntimeConfig` from `Runtime` initialization to `ApplyState` object.
This makes `Runtime` completely stateless and allows to execute transitions based on different configs.
It allows to upgrade `RuntimeConfig` with the new fees based on the protocol version for the current block.

This change should be NOOP and doesn't change the protocol. 

NEP: near/NEPs#120
Issue: #3158

# Test plan:

- [X] CI
- [x] http://nayduck.eastus.cloudapp.azure.com:3000/#/run/553
@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
Copy link
Collaborator

Superseded by #4401

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) C-bug Category: This is a bug P-critical Priority: critical
Projects
None yet
Development

No branches or pull requests

7 participants