search

near-daos / sputnik-dao-contract

Smart contracts for https://app.astrodao.com
https://astrodao.com/
MIT License
108 stars 76 forks source link

Factory contract update needed due to increased deployment cost #135

Closed jakmeier closed 2 years ago

jakmeier commented 2 years ago

Deployment costs per byte of code are going to be increased by a factor of ~5 with this change to NEAR protocol: https://github.com/near/nearcore/pull/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. https://github.com/near-daos/sputnik-dao-contract/blob/4d6d889df976db6184ab8dc53428a2cd90ccca47/sputnikdao-factory2/src/factory_manager.rs#L134-L157

I assume these constants all need to be reconsidered: https://github.com/near-daos/sputnik-dao-contract/blob/4d6d889df976db6184ab8dc53428a2cd90ccca47/sputnikdao-factory2/src/factory_manager.rs#L10-L16

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 https://github.com/near/nearcore/issues/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, https://github.com/near/nearcore/pull/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

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 commented 2 years ago

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

ctindogarus4f commented 2 years ago

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

ctindogaru commented 2 years ago

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

Created https://github.com/near-daos/sputnik-dao-contract/pull/147 to address this. However, the gas errors can be avoided even without https://github.com/near-daos/sputnik-dao-contract/pull/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 commented 2 years ago

Fixed in #147

TrevorJTClarke commented 2 years ago

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.