ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.34k stars 5.77k forks source link

Consider dividing contract storage into regular and dedicated mapping part. #9955

Closed ekpyron closed 2 years ago

ekpyron commented 4 years ago

Motivation

Inspired by the whole fuzz about https://github.com/ethereum/solidity/pull/9746.

A problem with the "oversized objects" warnings is that the choice of sizes at which to start warning about likely collisions is rather arbitrary. Ideally, we would generally avoid any collision from being possible, so maybe it's worth thinking about how costly that would be.

An easy way to get rid of the problem of collisions and the problem of us not being able to clear mappings in storage would be the following:

Proposal

Partition storage into its lower 2^128 bytes and its higher 2^128 bytes, reserving the lower half for "regular" state variables and the higher half for mappings. That way we could specify that the lower half of storage (used for "regular" state variables) will always need to be cleared on deletion, whereas the upper half of storage (used for mappings) would always need cleaning on creation.

If I'm not missing anything, this should be feasible, but, of course, it won't be free.

Evaluation

Additional costs:

So I'd say this is comparably cheap.

Collision-safety should not be overly affected by reducing the hash from 256 bits to 255 bits.

Main drawback: This will break the existing storage layout of contracts. Maybe that drawback is sufficient for not doing this on its own, but it may be worth thinking about it again, in case we want to change the storage layout in some other way as well.

This would, of course, also affect a lot of existing attempts at upgradable contracts, resp. any manual construction of storage slots using inline assembly (i.e. when manually constructing slots, it'd be wise to apply the same masking).

cameel commented 4 years ago

Just wondering, is anyone ever likely to get that warning in a working contract? It's issued when your state variable takes more than 512 exabytes (= 2^64 slots) which surely would not fly even if the code was not being executed on a blockchain?

a3d4 commented 4 years ago

Inspired by the whole fuzz about #9746.

There are two cases: (A) oversized objects leading to collision warnings (B) oversized objects leading to compilation errors.

I think the PR #9746 (undoubtedly fuzzy), in its current state, focuses rather on (B), while this issue seems to be about (A).

ekpyron commented 4 years ago

Just wondering, is anyone ever likely to get that warning in a working contract? It's issued when your state variable takes more than 512 exabytes (= 2^64 slots) which surely would not fly even if the code was not being executed on a blockchain?

Since storage is implicitly zero-initialized, it's theoretically fine to declare contract C { uint256 x[2**120]; } and just index into x sparsely using your own method of managing the space. It's not that far-fetched to do this to basically try to create a cheaper version of a mapping(address => uint) that doesn't require hashing for example.

ekpyron commented 4 years ago

It might even be that uint256 balances[2**120]; with balances[uint120(someAddr)] while also having mapping(uint256 => uint256) usersCanWriteToArbitraryKeysInThis; is among the primary reasons for the warning we have for such cases :-).

ekpyron commented 2 years ago

I don't see us doing this without a larger storage redesign which may happen due to the verkle business, but I don't think this issue is helping, so I'm closing.