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

Factory contract update needed due to increased deployment cost #135

Closed
jakmeier opened this issue Mar 9, 2022 · 11 comments
Closed

Factory contract update needed due to increased deployment cost #135

jakmeier opened this issue Mar 9, 2022 · 11 comments
Labels
C-discussion Category: Discussion, leading to research or enhancement C-Testing Unit testing / integration testing P-Critical Priority: Critical S-L2 Size: Minor 10-20 code line change

Comments

@jakmeier
Copy link

jakmeier commented Mar 9, 2022

Deployment costs per byte of code are going to be increased by a factor of ~5 with this change to NEAR protocol: near/nearcore#6397
I believe this will break your "Upgrade from the factory" mechanism.

Probably this will happen with protocol version 53, which would be released on testnet by March 23 and on mainnet by April 20.

I listed all the indirect deployments from the past that would fail with the new cost. Deployments from sputnik-dao.near showed up 269 times and sputnikdao.near 274 times. For example this transaction would fail with the new deployment costs: https://explorer.near.org/transactions/7z58HAEdoAvYjP5mHZWuaa3tMyi9nx6gpznuSgsQSsBK#6H2d52MiMBbWZyAjp2UVghDeUPPU3j3vCr6aVVuNYzUS

Briefly going through your code, the culprit seems to be in inside pub fn create_contract(...), where sys::promise_batch_action_deploy_contract(promise_id, u64::MAX as _, 0); is going to use much more gas. This means that the amount of gas attached to the promises afterwards will not be available.

sys::promise_batch_action_deploy_contract(promise_id, u64::MAX as _, 0);
// call `new` with given arguments.
sys::promise_batch_action_function_call(
promise_id,
new_method.len() as _,
new_method.as_ptr() as _,
args.len() as _,
args.as_ptr() as _,
&NO_DEPOSIT as *const u128 as _,
CREATE_CALL_GAS.0,
);
// attach callback to the factory.
let _ = sys::promise_then(
promise_id,
factory_account_id.len() as _,
factory_account_id.as_ptr() as _,
callback_method.len() as _,
callback_method.as_ptr() as _,
callback_args.len() as _,
callback_args.as_ptr() as _,
&NO_DEPOSIT as *const u128 as _,
ON_CREATE_CALL_GAS.0,
);
sys::promise_return(promise_id);

I assume these constants all need to be reconsidered:

const CREATE_CALL_GAS: Gas = Gas(75_000_000_000_000);
/// Gas allocated on the callback.
const ON_CREATE_CALL_GAS: Gas = Gas(10_000_000_000_000);
/// Leftover gas after creating promise and calling update.
const GAS_UPDATE_LEFTOVER: Gas = Gas(20_000_000_000_000);

How much more gas will be used? Assuming a size of 521kB, as I see it today for sputnikdao2.wasm, the gas cost on deployment alone will go from 7.5Tgas to 37.6Tgas.

Please check if my understanding is correct that this will break factory upgrade and if you can update it before the protocol version 53 is released. I am happy to assist and available to answer any questions you might have.

@TrevorJTClarke TrevorJTClarke added P-Critical Priority: Critical C-discussion Category: Discussion, leading to research or enhancement S-L2 Size: Minor 10-20 code line change C-Testing Unit testing / integration testing labels Mar 14, 2022
@TrevorJTClarke
Copy link
Contributor

@jakmeier thank you for the thorough breakdown. I was unaware of the protocol change. If possible, can you link me or describe why this change is occurring?

We will discuss & make changes appropriate to handle this change. Luckily you caught it before we finalize the next set of sputnik upgrades.

@ctindogaru
Copy link
Collaborator

ctindogaru commented Mar 14, 2022

Since all the gas costs for the deployment are increasing by a factor of 5, does the gas upper limit change as well? Currently it's set to 300 Tgas, as far as I know.

@jakmeier
Copy link
Author

@TrevorJTClarke The change is necessary because deploying a contract also requires it to be compiled. This compilation cost has not been properly charged for. Unfortunately, there is no comprehensive issue with all the details that I could link you to, sorry. The initial motivation came from near/nearcore#6201 but the nasty details have been discussed through other channels. I can share some more details in PM if you or someone else is interested. (You can find me on near.zulipchat.com or drop me a mail at jakob@near.org)

@ctindogaru You are right, 300Tgas is the limit since very recently, near/nearcore#6221 increased it from 200Tgas to 300Tgas. No further increasing is planned right now. I believe that it is not necessary since we have a hard limit of 4MiB on contract sizes and it is still possible to fit that in 300Tgas. However, if you have concerns that 300Tgas is too limiting for some use cases, I am very interested to hear about them.

@ctindogaru
Copy link
Collaborator

I think 300Tgas should be enough, we just need to make sure that all the GAS costs are updated accordingly. Will create a PR soon to fix the gas costs.

Thank you for giving us a heads up regarding all of this!

@TrevorJTClarke
Copy link
Contributor

requires it to be compiled

@jakmeier can you elaborate on this? Looked at the other issues, i guess its a dumb question but I dont understand the inner workings of neard/runtime enough to know what the compilation is doing, wasm goes to something else? or it is stored in disk in a special way?

@jakmeier
Copy link
Author

Not a dumb question at all! It could also be implemented differently but this is how it works today.

A WASM module goes through a couple of steps before we can run it.

  • Validation: Make sure it is valid according to WASM spec
  • Instrumentation: A few things we add to the code to work nicely in the NEAR Runtime. Mainly things to account for the dynamic gas cost for each operation executed during a function call.
  • Compilation: Generate machine code that can be executed on a CPU (i.e. x86)

Going through all these steps on every function call would be inefficient. Inefficient = we have to charge a lot of gas. Therefore, we put as much work as possible in the deployment phase, to make function calls cheaper. (We could also do it only on the first function call, but we don't want to charge different amount of gas for the first function call compared to calls later on, as we think it would be more confusing for users.)

So, upon deployment, we store WASM code in the account storage. (On the blockchain, you also have to stake some tokens to cover the storage for that code.) Additionally, we generate final machine code and store it in a cache that is local to the validator node. (No storage staking required, storing this is essentially for free from a user perspective.) The validator can then reliably read the native code from this cache when a function is called.

I hope that helps, but feel free to ask more questions.

@TrevorJTClarke
Copy link
Contributor

@jakmeier TYSM!!!! That was extremely helpful. Cheers!

@ctindogarus4f
Copy link
Contributor

ctindogarus4f commented Mar 15, 2022

amazing insights, thank you a lot for the detailed answer!

@ctindogaru
Copy link
Collaborator

ctindogaru commented Mar 24, 2022

https://explorer.near.org/transactions/7z58HAEdoAvYjP5mHZWuaa3tMyi9nx6gpznuSgsQSsBK#6H2d52MiMBbWZyAjp2UVghDeUPPU3j3vCr6aVVuNYzUS

Created #147 to address this. However, the gas errors can be avoided even without #147 being merged. The user just needs to supply more gas to cover the new deployment costs. So instead of 200 TGas, the user should provide 300 TGas to be 100% sure that the transaction succeeds.

The PR optimises how the gas is spent on the calls, so the transaction will succeed even if the user provides only 200 TGas.

@ctindogaru
Copy link
Collaborator

Fixed in #147

@TrevorJTClarke
Copy link
Contributor

So instead of 200 TGas, the user should provide 300 TGas to be 100% sure that the transaction succeeds

I know this is a late comment, but wanted to note that this is not the case.
See the scripts file for full testing using max gas. The upgrade flows do not work when trying with max gas, since the constraint is within the already deployed code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-discussion Category: Discussion, leading to research or enhancement C-Testing Unit testing / integration testing P-Critical Priority: Critical S-L2 Size: Minor 10-20 code line change
Projects
Status: Ready For QA
Development

No branches or pull requests

4 participants