-
-
Notifications
You must be signed in to change notification settings - Fork 790
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
VIP: Improve storage variable layout #769
Comments
As we've discussed this across many meetings, I have marked this as approved ;) |
@daejunpark I started working on the above, can we just update the spec, the resulting lll comes out as follows (just order of concat is different):
Also please mention that list and structs also need to use the new method ;) |
@jacqueswww that seems more natural to me. Indeed, I have no good idea why Solidity uses the swapped order. It would be great if you know Solidity developers and can ask them to confirm that there is no critical reason for choosing the swapped order. I'll update the spec as you suggested. Thanks! |
@jacqueswww I think you can keep the current layout scheme for lists and structs. Below is a quick reference of Lists:
Structs:
Maps:
In general: (
NOTE:
Rationale: The number of elements (or fields) of a list (or a struct) is expected to be much smaller than the size of the storage address space. Thus, even if the elements (or the fields) are stored in a consecutive region of the storage, it is very unlikely the region is overlapped with the other regions, since the starting locations of the regions will be well distributed (well spread across the entire storage) by Question. Could you confirm that the above scheme for lists and structs is the same with the current implementation? |
@daejunpark I actually went and did the work on all structures yesterday. The other one to consider is a ByteArray, which uses the same scheme. Are we 100% it's safe to have those structures not to use the |
@jacqueswww I discussed with @yzhang90, we think that it seems to be safe, but not 100% sure to be honest (because I'm not an expert for the kaccak256 hash). Indeed, the above scheme is similar to that of Solidity. I think we need to have an extra discussion and confirmation of its safety with other experts (e.g., Solidity developers). Do you think the Solidity gitter channel is a good place for it? If so, I can initiate the discussion there. (BTW, you're so quick to develop. Sorry for making you go back and forth. I think you can leave what you already implemented for now, just in case you change the scheme again.) |
I agree, let's get this sorted before I develop it further ;) gitter channel should help yes. Also look for @chriseth in our channel directly I know he works on solidity as well. |
Just for a record, copy the gitter communication here: Daejun Park @daejunpark 12:16 (It seems weird to ask about Solidity in Vyper channel, but this will help to improve the Vyper's layout scheme.) The current scheme, for example, works as follows:
Now, I have two questions: Q1. Is there a critical reason that the location scheme of dynamic arrays is different from that of mappings? In other word, what will happen if Q2. For a map entry location, is there any critical difference (in terms of security or hash distribution) between the following two? chriseth @chriseth 13:21 for the collisions see https://chriseth.github.io/notes/talks/safe_solidity/#/8 For Q2, I hope that keccak256 ensures that the order does not matter. |
@jacqueswww It turns out (thanks to @chriseth ) that the current scheme for lists is somewhat necessary, otherwise there is no reason to have lists in addition to maps. But we need to have a compile-time check to reject a very large list, which can be problematic for the same reason we discussed before. It would be good to check the size of structs as well (although it is very unlikely to declare such a large struct). So, are lists, structs, and maps all we have? |
In Solidity we issue warnings for large statically-sized arrays and we plan to remove the ability to arbitrarily increase the size of dynamically-sized arrays. If you have structures whose size in storage scales linearly with the amount of symbols required in source code, you should be fine, since there is still a lot of space in 2**256. |
Perhaps to clarify a little more: If you can only increase the length of a dynamically-sized structure by a single element, then the gas costs for that operation keep the structure small enough until the end of the universe. |
Thanks @chriseth for your help! Now, we're clear what to do. |
Preamble
Simple Summary
Improve the layout of dynamically-sized state variables in the storage, adopting that of Solidity.
Specification
A map entry
m[k]
is stored at the locationhash(k . index(m))
, where ".
" is byte-concatenation.A nested map entry's location can be defined recursively. For example,
m[k1][k2]
is stored athash(k2 . hash(k1 . index(m)))
A formal specification can be found here (in progress).
Below is a quick reference of
loc
that computes the storage location.Lists:
Structs:
Maps:
In general: (
E
is an arbitrary nested data structure, e.g., a list of structs of maps ...):NOTE:
#
denotes thekeccak256
hash..
is byte-concatenation.index
conversion for the list namea
, the struct names
, the struct field namesx
andy
, and the map namem
.Rationale:
The number of elements (or fields) of a list (or a struct) is expected to be much smaller than the size of the storage address space. Thus, even if the elements (or the fields) are stored in a consecutive region of the storage, it is very unlikely the region is overlapped with the other regions, since the starting locations of the regions will be well distributed (well spread across the entire storage) by
keccak256
.Motivation
The main difference from the current scheme is the use of
.
instead of+
to compute the offset. This change is critical for avoiding potential collisions, since.
is injective, while+
is not due to the modulo arithmetic. (Note that this is orthogonal to the potential hash collision ofkeccak256
.)The text was updated successfully, but these errors were encountered: