IIP-3: Allow messages without &self receiver; opt out of automatic storage loading

[ascjones]

Abstract

Proposing the addition of ink! messages which do not enforce reading and decoding the top level [ink(storage)] struct. This would provide additional flexibility to contract authors, and provides an "escape hatch" in case the data at the root key of the contract cannot be decoded into the top level storage struct.

Motivation

ink! messages are currently required to be methods with a &self or &mut self receiver. In both cases the top level contract storage struct is first loaded from storage and passed in as the self argument. For &mut self the storage struct is persisted following successful execution of the message logic.

One problem with this is that if the storage data at the root storage key of the contract cannot be successfully decoded into the storage struct, then the contract will be permanently* "bricked". This can happen either by the storage being corrupted via a direct write to set_contract_storage, or during migration to a new version of a contract via set_code_hash where the new version of the contract has an incompatible layout with the data at the root storage key.

The following example illustrates:

#[ink(storage)]
struct MyContract {
  value: u32,
}

#[ink(message)]
fn migrate_storage(&self) {
  const STORAGE_KEY: u32 = 0x00000000;
  let new_value = self.value as u64;
  ink::env::set_contract_storage(&STORAGE_KEY, &new_value);
}

#[ink(message)]
fn set_code(&mut self, code_hash: Hash) {
  // this message cannot be called because the storage will fail to decode.
  self.env().set_code_hash(&code_hash).unwrap()
}

The call to migrate_storage writes directly to the contract storage, upgrading the value from a u32 to a u64. A subsequent call to set_code (or any other message) would now fail because the u64 cannot be decoded into the original u32 (see #1897). Note that decoding errors can also occur with other any other invalid/corrupted data.

Therefore we need a way to be able to call messages such as set_code which do not have the constraint of pre-loading the storage level struct.

* The "bricked" contract could be fixed by calling the priveleged set_code extrinsic on pallet_contracts, but this would require the contract author coordinating with the sudo owner or via governance.

Specification

Rust allows "associated functions" in impl blocks which do not take a self receiver. These can be used for messages which do not load the root contract storage before message invocation. Using the example above, the set_code function becomes:

#[ink(message)]
fn set_code(code_hash: Hash) {
  ink::env::set_code_hash::<<Self as ink::env::ContractEnv>::Env>(&code_hash).unwrap()
}

This allows the contract code to be updated without first requiring to load the contract state, which would fail.

Considerations

Does this pattern open up any security vulnerabilities, or otherwise introduce a footgun for contract authors? Should we allow contracts to be bricked, and encourage better testing to avoid the eventuality instead?

Alternatively we could encourage writing all contracts using Lazy and Mapping values at the top level, which could avoid some of the problems by not expecting any data to be stored at the top level. Of course this would still be affected by storage corruption if any data happens to be written directly to the root storage key.

Rationale

The layout of the contract storage can change, either intentionally for migration or accidentally/maliciously by directly writing to contract storage via ink::env::set_contract_storage. In this and other cases where we do not want or need to load the root contract state before executing a message, ink! messages which do not accept a self instance of the root contract storage should be provided.

[SkymanOne]

My personal take. I think this would be a useful feature to have. I personally do not see any security implications with this feature other than a potential user error, but this can be fixed with extensive documentation.

On the other, hand, introducing this feature will highlight the issue addressed in #1969 (i.e. ink! does not provide immutability in messages intrinsically). Since it would be absolutely valid to have this:

#[ink(message)]
fn update_cell(value: u8) {
  Lazy::<u8, ManualKey<127>>::new().set(&value);
}

Therefore, as part of this PR, I believe we should then remove mutability field from the metadata and advertise ink! code as mutable. Even though this is beyond the scope of this issue, it is affected as well.

My sentiment has always been removing direct storage and code manipulation and provide an encapsulating interface that would handle storage access and code upgrades while inferring the mutability from the message it is called. But again, beyond the scope of this issue.

I think this feature would actually pave the way for the upgradeability interface which I will outline later this week.

On the final note, I think ink::env::set_code_hash::<<Self as ink::env::ContractEnv>::Env>(&code_hash).unwrap() can be turned into a proc macro set_code_hash!(&code_hash).unwrap() for shortness.

[ascjones]

Therefore, as part of this PR, I believe we should then remove mutability field from the metadata and advertise ink! code as mutable. Even though this is beyond the scope of this issue, it is affected as well.

There still exists the distinction that mutable messages will automatically persist the top level storage struct upon successful execution. It is a limitation that the mutability does not provide us with any guarantees, if a user chooses to use the underlying APIs, as @xermicus says #1969 (comment).

My sentiment has always been removing direct storage and code manipulation and provide an encapsulating interface that would handle storage access and code upgrades while inferring the mutability from the message it is called. But again, beyond the scope of this issue.

At the moment the ink codegen requires the public API for reading/writing from storage. Maybe there is a technique we can use to hide that API from being used directly by the contract author, but I can't think off the top of my head a way to prevent calling the under the covers methods entirely, if they are to be exposed to the codegen.

[SkymanOne]

It is a limitation that the mutability does not provide us with any guarantees, if a user chooses to use the underlying APIs

Indeed, that's why we should discourage the use of the interface in favour of higher-level APIs that should come in future.

At the moment the ink codegen requires the public API for reading/writing from storage. Maybe there is a technique we can use to hide that API from being used directly by the contract author, but I can't think off the top of my head a way to prevent calling the under the covers methods entirely, if they are to be exposed to the codegen.

This is obviously a massive refactoring of the ink_env crate which is not the main priority atm. Therefore, as mentioned earlier, we should simply provide a better interface that would be favourable to use instead of direct storage operations for upgradeability. I see the design described in this issue as a step forward to it, but only if it will not be used by the developer directly and will be encapsulated as part of some migration function in future.

It's okay to use it now, since it allows devs to "unscrew" the migration as I mentioned in #1909 (comment)

[ShadoySV]

I've just successfully tested an approach to upgrade contracts with breaking storage changes and it might be used to fix broken contracts using the proposal:

Intermediate contract version preparation

• Move the new (breaking contract) field into a struct
• Add the previous working version of the field into the struct
• Implement parity_scale_codec::Decode manually for the struct to decode only the working field
• Transform the working field value into the new field value in the upgrading code function (the function should be mutable)
• Implement parity_scale_codec::Encode manually for the struct to encode only the new field

Final contract version preparation

• Turn the struct into the new field back
• Remove transforming value code from the upgrading function

Contract upgrade itself

• Upgrade contracts to intermediate and then to final contract version in one transaction

[ascjones]

Thankyou @ShadoySV that is a nice idea for a workaround!