Clarify storage layout. #10860

chriseth · 2021-01-28T08:52:53Z

bshastry · 2021-01-28T09:49:01Z

docs/internals/layout_in_storage.rst

 by the above rules, state variables from different contracts do share the same storage slot.

-The elements of structs and arrays are stored after each other, just as if they were given explicitly.
+The elements of structs and arrays are stored after each other, just as if they were given


Does stored after each other mean stored in separate storage slots or packed tightly as per rules above?

I hoped that "just as if they were given..." would clarify that question...

Hmm... it is still a bit vague at least for me. Perhaps just as if they were declared outside the struct?

Actually, I also still find this sentence a bit confusing... isn't this covered by the "rules above" - or respectively wouldn't it be covered, if the contiguous items that need less than 32 bytes in front of those rules was dropped?

docs/internals/layout_in_storage.rst

bshastry · 2021-01-28T09:54:32Z

docs/internals/layout_in_storage.rst

-Due to their unpredictable size, mapping and dynamically-sized array types use a Keccak-256 hash
-computation to find the starting position of the value or the array data.
-These starting positions are always full stack slots.
+Due to their unpredictable size, mappings and dynamically-sized array types cannot be stored


Suggested change

Due to their unpredictable size, mappings and dynamically-sized array types cannot be stored

Due to their unknown size, mappings and dynamically-sized array types cannot be stored

Hm, not sure about this. If you don't know the size of something but can put a reasonable upper bound on it, you might still store it in-place.

In the case of mappings and dynamically-sized arrays isn't the reason they are not stored in between because their size is unknown (because this value set at runtime)? I agree that in the hypothetical case where some data structure is bounded but exact size unknown, one could still store "in between" but that isn't the case here iiuc

What about "Since their size is not fixed, mappings..."?

bshastry · 2021-01-28T09:56:58Z

docs/internals/layout_in_storage.rst

-So for the following contract snippet
-the position of ``data[4][9].b`` is at ``keccak256(uint256(9) . keccak256(uint256(4) . uint256(1))) + 1``::
-
+For mappings, the slot stays empty, but it is needed so that two mappings after each other will use a


Stays empty meaning zero initialized?

Yes, but this is undefined. It's just not used for anything.

Should we include this info in the docs? Something like reading from this slot is undefined?

I think this paragraph is enough documentation.

docs/internals/layout_in_storage.rst

bshastry · 2021-01-28T10:05:00Z

docs/internals/layout_in_storage.rst

+rule recursively. The location of ``x[i][j]`` for ``x`` being ``uint32[][]`` is
+computed as follows (again, assuming ``x`` itself is stored at slot ``p``):
+The slot is ``keccak256(keccak2569(p) + i) + floor(j / 8)`` (8 items per slot) and
+the element can be obtained from the slot data ``v`` using ``(v >> ((j % 8) * 2)) & 0xffff``.


Let's say the uint32 values are 1, 2, 3. Each value is stored in 4 byte groups (32-bit) so the last 12 bytes look like 0x000100020003. So, isn't the formula to obtain data (v >> ((j % 8) * 8)) & 0xffffffff?

Right! Is the forumla useful?

Depends. If an "advanced" user wants to exploit this to reduce gas usage, maybe?

It's useful, I'm trying to look at internal contract state /nonpublic storage on chain so understanding that formula is necessary if you want to introspect values smaller than 32bytes

Hi I have a question.

what does v in (v >> ((j % 8) * 2)) & 0xffff refer to?

Also, why are we &ing with 0xffff? Is not anding with 0xffff same as the value that we are anding?

docs/internals/layout_in_storage.rst

bshastry · 2021-01-28T10:06:37Z

docs/internals/layout_in_storage.rst

+
+The value of a mapping key
+``k`` is located at ``keccak256(k . p)`` where ``.`` is concatenation. The hash is always a hash of
+64 bytes. If the key type is shorter, it is padded with zeros according to its alignment.


Suggested change

64 bytes. If the key type is shorter, it is padded with zeros according to its alignment.

64 input (`k.p`) bytes. If the key type is shorter, it is padded with zeros according to its alignment.

Maybe the whole sentence should be rephrased - we should not use k in keccak256(k . p) if it is modified in some cases.

docs/internals/layout_in_storage.rst

relyt29 · 2021-01-28T18:09:15Z

docs/internals/layout_in_storage.rst

+computed to compress it to 32 bytes.
+
+If the mapping value is a
+non-value type, the positions are found by adding an offset to ``keccak256(k . p)``.


might be worth putting a ref to https://docs.soliditylang.org/en/v0.8.1/types.html#reference-types for convenience here, I had to google to make sure I knew what a non-value type was (though I was definitely assuming it was non-primitives e.g. uint etc)

relyt29 · 2021-01-28T18:11:37Z

This is much much better now, you addressed pretty much all of the concerns I had in the issue / confusing bits. This is much easier to follow, especially the part about p. Thanks

relyt29 · 2021-01-28T20:21:39Z

I'm still not sure / a bit confused around "adding an offset" for non value types. Maybe the example should add nested structs as well, e.g. foo[4].bar[3].baz

for a sample contract like so

pragma solidity ^0.4.0;

contract C {
  struct Disco { 
          uint a; 
          uint b; 
          mapping(uint => Techno) bar;
  }
  struct Techno {
         uint g;
         uint baz; 
 }
  uint x;
  mapping(uint => Disco) foo;
}

am I right in my understanding that the location for foo[4].bar[3].baz should be (for all integer numbers below assume uint256(integer) so 31 bytes of leading zeros, . is concatenation + is arithmetic addition)

keccak ( 0x03 . ( keccak( 0x04 . 0x01 ) + 0x02) ) ) + 0x01

chriseth · 2021-02-02T10:02:11Z

Made some modifications, please re-review.

chriseth · 2021-02-02T10:02:23Z

@relyt29 yes, your computation is correct.

docs/internals/layout_in_storage.rst

bshastry

Took a quick look at the text part and it looks good to me (tiny change suggested). Would be nice to have an ack from @relyt29 as well.

ekpyron

Some minor comments, but generally it reads good and is definitely an improvement! (So I'll approve regardless of those minor comments)

ekpyron · 2021-02-03T16:45:24Z

docs/internals/layout_in_storage.rst

+Except for dynamically-sized arrays and mappings (see below), data is stored
+contiguously item after item starting with the first state variable,
+which is stored in slot ``0``. For each variable,
+a size in bytes is determined according to its type.


Suggested change

a size in bytes is determined according to its type.

its size in bytes is determined according to its type.

?

is 32 the size of a mapping?

Fair enough... the required size? Just "a size" sounds a bit weird to me. But also fine ;-).

Yeah, I even wanted to suggest the size myself but I saw that @ekpyron already made a suggestion so I removed it.

It could say something like the size of the space taken in the contiguous block but that's way too long.

ekpyron · 2021-02-03T16:57:45Z

docs/internals/layout_in_storage.rst

 by the above rules, state variables from different contracts do share the same storage slot.

-The elements of structs and arrays are stored after each other, just as if they were given explicitly.
+The elements of structs and arrays are stored after each other, just as if they were given


Actually, I also still find this sentence a bit confusing... isn't this covered by the "rules above" - or respectively wouldn't it be covered, if the contiguous items that need less than 32 bytes in front of those rules was dropped?

docs/internals/layout_in_storage.rst

ekpyron · 2021-02-03T17:07:26Z

docs/internals/layout_in_storage.rst

+non-value type, the location is the starting slot of the data. If the value is of struct type,
+for example, you have to add an offset corresponding the the struct member to reach the member.
+
+Let us compute the storage location of ``data[4][9].c`` in the following contract.


Maybe it's nicer to say

As an example, consider the following contract: <Contract> Let us compute the storage location of...

otherwise one needs to look/scroll down and then move back to understand this... but maybe fine...

docs/internals/layout_in_storage.rst

chriseth · 2021-02-03T17:51:46Z

Rebased.

docs/internals/layout_in_storage.rst

cameel · 2021-02-03T19:48:41Z

docs/internals/layout_in_storage.rst

+The value corresponding to a mapping key ``k`` is located at ``keccak256(h(k) . p)``
+where ``.`` is concatenation and ``h`` is a function that is applied to the key depending on its type:
+
+- for value types, ``h`` pads the value to 32 bytes according to its alignment


We could clarify this a bit more:

Suggested change

- for value types, ``h`` pads the value to 32 bytes according to its alignment

- for value types, ``h`` pads the value to 32 bytes according to its alignment (value types are generally right aligned except for fixed bytes types).

BTW, is there anything other than bytesXX that is left-aligned?

function types, I think.

Is that documented anywhere? Or is it considered an implementation detail?

Actually this document does not mention function types at all and I don't think we ever explain how much space they take (at least not Types > Function Types). They don't take a full slot, right?

We discussed this on Discord. We do not have the size of function type documented anywhere (and maybe some other value types too, we need to check) but that's a separate issue.

docs/internals/layout_in_storage.rst

ekpyron

Reapproving. (There's still https://github.com/ethereum/solidity/pull/10860/files#r569580367, but I'd be fine with merging as is.)

EDIT: Oh, wow, a few of @cameel's points just now are worth fixing, though!

chriseth · 2021-02-04T13:19:06Z

These two are still open:

docs/internals/layout_in_storage.rst

chriseth mentioned this pull request Jan 28, 2021

Docs: "Layout of State Variables in Storage" "Mappings and Dynamic Arrays" paragraph incredibly confusing and poorly written #10859

Closed

chriseth force-pushed the clarifyStorageLayout branch from 6f88691 to e4e3434 Compare January 28, 2021 08:55

bshastry reviewed Jan 28, 2021

View reviewed changes

relyt29 reviewed Jan 28, 2021

View reviewed changes

chriseth force-pushed the clarifyStorageLayout branch from 0c06f01 to a9dfc53 Compare February 2, 2021 10:02

bshastry reviewed Feb 2, 2021

View reviewed changes

docs/internals/layout_in_storage.rst Outdated Show resolved Hide resolved

bshastry previously approved these changes Feb 2, 2021

View reviewed changes

chriseth dismissed bshastry’s stale review via 4a25027 February 2, 2021 11:04

chriseth force-pushed the clarifyStorageLayout branch from 4a25027 to a89322b Compare February 2, 2021 11:05

ekpyron previously approved these changes Feb 3, 2021

View reviewed changes

cameel reviewed Feb 3, 2021

View reviewed changes

docs/internals/layout_in_storage.rst Show resolved Hide resolved

docs/internals/layout_in_storage.rst Show resolved Hide resolved

chriseth dismissed ekpyron’s stale review via 08d3317 February 3, 2021 17:47

chriseth force-pushed the clarifyStorageLayout branch from e8a9a3f to 77c7afd Compare February 3, 2021 17:51

cameel reviewed Feb 3, 2021

View reviewed changes

ekpyron previously approved these changes Feb 3, 2021

View reviewed changes

chriseth dismissed ekpyron’s stale review via 409b08b February 4, 2021 12:20

chriseth force-pushed the clarifyStorageLayout branch from 26eb07b to 1db87bd Compare February 4, 2021 13:18

cameel previously approved these changes Feb 4, 2021

View reviewed changes

chriseth dismissed cameel’s stale review via 827035f February 4, 2021 15:00

cameel reviewed Feb 4, 2021

View reviewed changes

docs/internals/layout_in_storage.rst Outdated Show resolved Hide resolved

chriseth force-pushed the clarifyStorageLayout branch from 827035f to 514340f Compare February 4, 2021 15:12

cameel previously approved these changes Feb 4, 2021

View reviewed changes

Clarify storage layout.

61b5e8e

chriseth dismissed cameel’s stale review via 61b5e8e February 4, 2021 15:26

chriseth force-pushed the clarifyStorageLayout branch from 514340f to 61b5e8e Compare February 4, 2021 15:26

chriseth merged commit 2fb2788 into develop Feb 4, 2021

chriseth deleted the clarifyStorageLayout branch February 4, 2021 15:26

	Due to their unpredictable size, mappings and dynamically-sized array types cannot be stored
	Due to their unknown size, mappings and dynamically-sized array types cannot be stored

	64 bytes. If the key type is shorter, it is padded with zeros according to its alignment.
	64 input (`k.p`) bytes. If the key type is shorter, it is padded with zeros according to its alignment.

	a size in bytes is determined according to its type.
	its size in bytes is determined according to its type.

	- for value types, ``h`` pads the value to 32 bytes according to its alignment
	- for value types, ``h`` pads the value to 32 bytes according to its alignment (value types are generally right aligned except for fixed bytes types).

Clarify storage layout. #10860

Clarify storage layout. #10860

Uh oh!

Conversation

chriseth commented Jan 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sssubik Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

relyt29 commented Jan 28, 2021

Uh oh!

relyt29 commented Jan 28, 2021

Uh oh!

chriseth commented Feb 2, 2021

Uh oh!

chriseth commented Feb 2, 2021

Uh oh!

Uh oh!

bshastry left a comment

Choose a reason for hiding this comment

Uh oh!

ekpyron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

sssubik Mar 25, 2022 •

edited

Loading

ekpyron left a comment •

edited

Loading