Split ext_clear_storage out from ext_set_storage

[Robbepop]

This PR splits the

ext_set_storage(key_ptr: u32, value_non_null: u32, value_ptr: u32, value_len: u32);

into two APIs:

ext_set_storage(key_ptr: u32, value_ptr: u32, value_len: u32);
ext_clear_storage(key_ptr: u32);

Motivation

We perform this change to make it simpler for static analyzers to analyze the underlying code semantically.

The old API with its value_non_null parameter is theoretically impossible to analyze statically since the value_non_null parameter could be any runtime computed value. So it is potentially very hard for static analyzers to apply the correct gas costs in the case where we want to differentiate between setting and clearing contract storage entries.

Why do we want to differentiate between those two operations?

The operations have completely different semantics:

• The one API is updating a value of a storage entry,
• the other is clearing the value of a storage entry.

Also as described above this makes it far simpler for static analyzers (or in some cases even possible) to reason about the underlying code. It slightly reduces overhead in some situations but that should generally be negligible.

Open Questions

While working on this PR one particular question arosed:

How do we currently treat empty storage entries? E.g. when setting ext_set_storage with a value_len of 0? This semantically is different from having a cleaned-up contract storage entry and thus should be taken into account for rental fees.

Since we were not aware of any smart contract use cases that could even profit from creating or having occupied empty storage entries we also disallowed having them in the adjusted ext_set_storage. Upon trying to set a contract storage entry to be empty this function will trigger a host error and the contract will trap.

We might change the semantics of this particular scenario once we are aware of a proper smart contracts use case.

Task List of this PR

• Introduced ext_clear_storage
• Adjusted semantics of ext_set_storage
• Adjusted COMPLEXITY.md
• Adjusted SEAL tests

[parity-cla-bot]

It looks like @Robbepop signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[pepyakin]

As per offline discussion here is my take on this:

The current style API and the proposed style APIs are mostly the same. One can easily imagine how to implement the same primitives both on the current style APIs and the proposed style. I would argue that they do not have significant differences, besides a better amenability for the static analysis in more cases, namely when a the language that targets contracts

TBF, This

Having very low gas costs for this clean up process is vital in order to motivate such cleanup routines.

doesn't really apply since both the current style API and the proposed one will have the equivalent gas costs. That is, old ext_set_storage with value_non_null=1 should have the same cost as new ext_set_storage and old ext_set_storage with value_non_null=0 will have the same cost as new ext_clear_storage.
Apart from that, it is not clear if such an incentivisation will work at all. First and foremost, the prices should be dictated by the runtime characteristics and not by incentivisation. If it turns out that the runtime cost of clearing the value will be roughly equal to the cost of setting, well we won't be able to make the clearing cheaper than it should be by weight and I am not a fan of the idea of artificially increasing the cost of the set only for providing such incentivisation (since again I am not sure if it is going to work).

My original answer was that I am indifferent on this and was inclined to leave the status quo, but now I think I am more for the change. The reason is that upcoming change #4590 (and #3263) will allows us dropping the AccountDb abstraction and we will switch to using substrate API directly which is in contrast uses the separate style API.

Now, to the open questions part. I think the concern regarding interaction between zero sized entries and state rent is valid. However, I think it deserves it's own issue. (filled under #5107)

[pepyakin]

looks good!

[gavofyork]

The proposition that this PR is based on seems flawed. Storage rent should be all the incentivisation needed to ensure that storage clean-ups occur. Trying to warp the prices away from the true costs for the storage operations in order to try to incentivise cleaning up or disincentivise wasteful usage seems accordingly ill-considered.

[Robbepop]

The proposition that this PR is based on seems flawed. Storage rent should be all the incentivisation needed to ensure that storage clean-ups occur. Trying to warp the prices away from the true costs for the storage operations in order to try to incentivise cleaning up or disincentivise wasteful usage seems accordingly ill-considered.

You are right that the original proposition was flawed since we would never decouple gas prices of operations from their real performance impact on the system which the original proposition was based on. - Unfortunately I forgot to update after we had internal discussions about this. However, the code changes no longer reflect the original proposition.
However, there are other reasons for splitting this API up into 2 functions because they are semantically doing completely different things (free and set) and by having this split we can more easily analyze semantics of the underlying code statically.