VM: Strip zeros when putting contract storage in StateManager #880
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Right, tests are failing, but reading the errors seems like this is a case of "the tests are wrong". We might want to consider switching the tests to TypeScript at some point too, because looks like numbers get fed into |
|
@jochem-brouwer yes, that was exactly what I was thinking when reviewing #849 (switching to TypeScript), probably rather sooner than later since this should give direct benefits on reliability. //cc @ryanio |
jochem-brouwer
force-pushed
the
storage-slots-32-bytes
branch
from
cea7a9c to
aab700d
Compare
| throw new Error('Storage key cannot be longer than 32 bytes') | ||
| } | ||
|
|
||
| value = unpadBuffer(value) |
There was a problem hiding this comment.
You are writing in the comments that a value will be left-padded and then you actually unpad (I read the semantics of this function as: "remove the padding", correct me if I am wrong) here, how does this go together?
There was a problem hiding this comment.
Oops this is a comment from when I was still thinking that storage slots should always be 32 bytes; this is thus not the case, the leading zeros are stripped. Will update
jochem-brouwer
changed the title
jochem-brouwer
force-pushed
the
storage-slots-32-bytes
branch
from
ecc948f to
0b33df7
Compare
|
this LGTM! nice additional tests. it just needs an update to ts now, then can approve and merge. |
jochem-brouwer
force-pushed
the
storage-slots-32-bytes
branch
3 times, most recently
from
55a711a to
ae8f63c
Compare
|
Rebased and updated the spelling/grammar errors in the docs, should be ready @ryanio 😄 |
|
Nvm should rebase |
|
@jochem-brouwer are you doing this "right now"? Otherwise should be also ok to use the "Update branch" function (rebase is a bit better of course) and I would do a review during the next hour or so. |
|
Rebasing it now! @holgerd77 |
…lues in putContractStorage
fix test; update comment lint fix test fix grammar Co-authored-by: Ryan Ghods <ryan@ryanio.com> fix spelling Co-authored-by: Ryan Ghods <ryan@ryanio.com>
jochem-brouwer
force-pushed
the
storage-slots-32-bytes
branch
from
ae8f63c to
21e43e6
Compare
|
This was an easy rebase, no conflicts 😄 Ready for review @holgerd77 I thought GitHub said there were conflicting files but doesn't seem to be. |
| t.end() | ||
| }) | ||
|
|
||
| it('should strip storage values lower than 32 bytes', async (t) => { |
There was a problem hiding this comment.
This test description is a bit misleading (stripping is logically not connected to the byte length) respectively also not correct (stripping is also done for 32-byte length. Can you please adjust?
There was a problem hiding this comment.
I have edited this name in 5217f50, hope this is better. I think this name was from when I still expected storage values to always be 32 bytes and I did not edit it right when I realized that was wrong.
|
|
||
| await stateManager.putContractStorage(address, key0, value0) | ||
| const slot0 = await stateManager.getContractStorage(address, key0) | ||
| t.ok(slot0.equals(expect0), 'value of 31 bytes padded correctly') |
There was a problem hiding this comment.
Really nit, but I would think this test would be a bit better readable if the respective execution part of these two cases are combined with the value setup (so everything with *0 together and everything with *1 together)
There was a problem hiding this comment.
Fixed -> 5217f50
Note: I have edited this test a little bit (it did unpadZeros in previous commits on this particular test, but none of the test cases actually had leading zeros)
|
Thanks for the review @holgerd77, had no time this afternoon to take a further look but have fixed (?) it now 😄 |
There was a problem hiding this comment.
Ok, this looks good now, thanks @jochem-brouwer, great find! Sorry that this took a bit long, will merge now.
Closes #875
Had a bit of a wrong understanding here, storage slot values should be stripped of leading zeros, which is now enforced in
putContractStorage. Since putting >32 bytes storage values is not possible in EVM, this is now also disallowed. If we strip leading zeros this means that any zero-byte will immediately be detected as "delete value", both in StateManager but also in MPT, which is nice.