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

Introduce immutable variables to WASM contracts #1714

Closed
agryaznov opened this issue Feb 23, 2023 · 11 comments
Closed

Introduce immutable variables to WASM contracts #1714

agryaznov opened this issue Feb 23, 2023 · 11 comments
Assignees

Comments

@agryaznov
Copy link
Contributor

agryaznov commented Feb 23, 2023

It was decided that this feature should be introduced on ink! side instead of on pallet side.

Rationale

Solidity has the concept of immutable variables, which value can be set once in contract’s constructor and guaranteed to stay unchanged afterwards. OpenZeppelin has recently recommended ink! to introduce this feature too, to provide contract developers with more flexibility in designing and building secure and efficient smart contracts. In this issue we’ll try to think about how we can introduce the similar concept into ink! and Pallet Contracts.

Spec of changes on ink! side

TBD


(Original discussion): ways to solve it on the pallet side

WebAssembly has indeed the similar construct - immutable global. Its initializer lives in the Globals section of the Wasm module, and uses a constant initializing expression.

The constructor of Wasm contracts in the pallet_contracts is the deploy exported function. This function is called once at the time of contract instantiation on-chain. Contract instantiation is done after the re-instrumentation and uploading the contract Wasm blob on-chain, and it in general can be done in a separate (from uploading) extrinsic, when deploying a new contract instance of a code already stored on-chain. This approach allows for more optimal storage usage, and saves gas and storage deposit for the caller account. Changing the Global section of the module on the step of contract instantiation would require its code altering and re-uploading on chain, implying an additional costs for contract authors, and, more importantly, breaking these variables immutability of all other contract instances which have been deployed from this code before.

Another approach could be to use an imported global in the contract module (note: currently importing globals to Wasm contracts is forbidden). Pallet contracts could define the global and initialize it in the “env” module during the instantiation of the contract module to the Wasm engine. For that it would need to somehow get the initializing expression from the constructor (deploy exported function of a module, which is to be designed by contract developer in #[ink(constructor)]). One problem here is that we instantiate the module first to the Wasm engine, and only after that we call the exported deploy function. Another problem is that once the immutable variable is initialized, we need to store its value for the particular contract instance living at specific AccountID. And once we don't want to store a separate code blob for each contract instance, the likely solution here is to persist the immutable variables values to the contract instance's storage.

@athei
Copy link
Contributor

athei commented Mar 1, 2023

Immutable variables is a programming language feature. It is in the case of solidity and it should be in our case, too. I see no reason to bake this complexity into pallet-contracts. As a rule of thumb: If a feature can be implemented in user space this is where it should be implemented. Adding complexity (and policy) to privileged code should only be done with a very good reason.

In this case I don't see a big advantage in moving this logic to pallet-contracts: ink! can solve this problem by flagging certain fields as read only: We remove direct field access for the root storage struct and generate accessor functions (we leave out the setter for read only variables).

@HCastano
Copy link
Contributor

I agree with Alex here. Can you move this into the ink! repo and spec it out based on ink!'s perspective instead?

@agryaznov agryaznov transferred this issue from paritytech/substrate Mar 14, 2023
@agryaznov agryaznov changed the title [contracts] Introduce immutable variables to WASM contracts Introduce immutable variables to WASM contracts Mar 14, 2023
@SkymanOne SkymanOne self-assigned this Apr 12, 2023
@SkymanOne
Copy link
Contributor

Just checked. ink! contracts already support defining constants. With regard to immutables, we can indeed just mark certain fields with #[ink::immutable] and then extract the value assigned in the constructor. However, to actually bring cost benefits of immutable, we should ideally bake the values into the blob.

As solidity documentation states, upon construction (which I understand is instantiation) of the contract, the references to immutable variables are replaced with their actual values.

So I guess we do need to alter the code on-chain and re-instantiate it as we currently do not separate constructors from the main contract body as solidity does. Alternatively, we need to expose some functionality from pallet-contracts to save the state. Otherwise, we don't really bring any performance benefits with this feature

@athei
Copy link
Contributor

athei commented Apr 27, 2023

I don't think we are after a performance benefit here.

@SkymanOne
Copy link
Contributor

SkymanOne commented Apr 27, 2023

Why not? The whole point of immutables is that you don't need to make storage reads as values get "hard coded" into the binary.

@agryaznov
Copy link
Contributor Author

So I guess we do need to alter the code on-chain and re-instantiate it as we currently do not separate constructors from the main contract body as solidity does.

This won't work, because in pallet-contracts we store the contract code once, and then it is shared between all contract instances. If you change the contract's code by injecting new values for an immutable variable coming from next instance's constructor execution, it would break all previously deployed instances using the same code (because it would mutate the immutable variables of them). On contrary, in EVM world each contract instance uses brand new code persisted on-chain. And this comes with a huge waste of storage.

@ascjones
Copy link
Collaborator

Agree that we definitely don't want to do this in pallet-contracts.

ink! can solve this problem by flagging certain fields as read only: We remove direct field access for the root storage struct and generate accessor functions (we leave out the setter for read only variables).

This would introduce a footgun because you can still update "read only" fields by calling ink_env::set_contract_storage directly. So the benefits are limited because there is no guarantee of that value being truly read-only.

Easiest thing would be to tell people just to use a const and they just have to rebuild and upload a different version of the code for different values of the contract.

If people really don't want to modify a const in source code we could just recommend them to do something like:

const VERSION: &str= match option_env!("VERSION") {
    Some(v) => v,
    None => "dev",
};

and do VERSION=whatevs cargo contract build

Could even do some syntactic sugar to generate such a pattern e.g.

#[ink(const)]
const MY_CONST: u32 = 420;

In this case you could add the consts to the metadata and pass them as args during build e.g. contract build --const-my-const 69

Anyway just thinking out loud, but seems to me that const is the way to go because you have the read only guarantee and the perf benefits all in one. Downside is multiple versions of the same code deployed with different const values 🤷

@SkymanOne
Copy link
Contributor

One of benefits of immutables over consts is the ability to assign runtime specific data like caller's address or the balance. Consts do not allow this.

The workaround I have in mind is to get this data from the dry-run and modify the source locally before uploading

@SkymanOne
Copy link
Contributor

After a call with @agryaznov. We've come up with the next design proposal:

It was agreed that changing contract's Wasm blob is costly and does not bring many benefits. Using constant is already possible and does not require any changes.
As @ascjones correctly pointed out, updating storage fields is possible with ink_env::set_contract_storage. Therefore, in order to persist on-chain data as immutable and forbid update, we should introduce a new reserved branch in the storage tree that is restricted to be updated with ink_env::set_contract_storage and can only be written to during the execution of the constructor. This is the change that needs to be done on the pallet-contracts side.

On the ink! side, we shall introduce #[ink::immutable] attribute that can be used with storage fields to mark them as immutable. These fields must be initialised in a constructor and can not be updated after instantiation. The values immutables shall be saved to under the key of "immutable" branch.

In the end, immutable fields will behave almost the same as normal fields except that they can not be updated.

@athei
Copy link
Contributor

athei commented Apr 28, 2023

You are suggesting access control for storage items in pallet-contracts. While we can discuss this is should be separate from the programming language feature discussed in this issue.

@SkymanOne
Copy link
Contributor

Having discussed the potential solutions with core ink! team I am closing the solution as we currently can not provide any security guarantees to implement this feature.

Solutions considered

Modify the contract bundle by adding globals to the Wasm module

This was the original design proposed in this issue and was later discarded due to complication with gas metering and potential re-instrumentation of every deployable contract. This procedure would simply sky-rocket the gas for every instantiation. This is due to fundamental differences between ink! and solidity. Upon compilation, solidity produces a "creation" binary that is then executed on-chain to instantiate contract. ink!, on the other hand, produce the final deployable binary that gets instantiated on-chain. Therefore, we can not do on-chain instantiation of any constants or immutables.

Restrict the mutability with macros

This change does not involve any modifications on the pallet-contracts. It restricts the mutability of storage fields on the language level. The problem with that is this approach does not provide any security guarantees and creates a false sense of security as the contract storage can be overwritten with set_contract_storage() extrinsic

Add immutable branch in the storage tree

Can be a potential solution, that does not bring much of a cost benefit, but provides security guarantees for immutability as pallet-contract would restrict the overwrite of this branch with set_contract_storage(). It was advocated that this feature is a significant change to the pallet-contracts that would require future maintenance, and the runtime should not enforce policies but provide mechanisms.

In conclusion, it is advices for developers to adopt their own approach in maintaining the immutable storage fields in their contracts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants