Conversation
cmichi
force-pushed
the
cmichi-fix-option-ptr-bug
branch
2 times, most recently
from
1f29702 to
2ca833a
Compare
cmichi
force-pushed
the
cmichi-fix-option-ptr-bug
branch
from
2ca833a to
69cd40e
Compare
Codecov Report
@@ Coverage Diff @@
## master #570 +/- ##
===========================================
- Coverage 82.47% 67.75% -14.73%
===========================================
Files 155 155
Lines 6820 6826 +6
===========================================
- Hits 5625 4625 -1000
- Misses 1195 2201 +1006
Continue to review full report at Codecov.
|
Thanks for fixing! This is exactly what I thought was wrong. :) |
This reverts commit 69cd40e.
Robbepop
left a comment
Robbepop
left a comment
There was a problem hiding this comment.
Thanks for the bug fix and clippy suggestion improvement.
| } | ||
|
|
||
| #[test] | ||
| fn regression_test_for_issue_570() -> ink_env::Result<()> { |
There was a problem hiding this comment.
The entire test can be heavily simplified. You only need to push a (Option, i32) tuple with values (None, 1) to the contract storage and then read it again in the next step and assert that the first value is None and the second value is 1. Done.
There was a problem hiding this comment.
Or am I missing something?
There was a problem hiding this comment.
So that's a clever idea, I guess you thought the 1 would be written out to the first pointer and on the next pull_spread we would try to read a Some(...) there and fail. It doesn't work unfortunately because of this: push_spread for Option writes self.is_some() as u8 in the first ptr slot and increments the pointer. Hence on the next pull_spread we immediately return the None and continue pulling the i32. So no failure would occur here.
There was a problem hiding this comment.
Using a tuple also doesn't simplify the test significantly, because for each tuple element the entire push_spread recursion is called. So storage is basically overwritten until I introduce LazyCell into the tuple. (Note that in my test only the Option is pushed in step 2).
I mean the whole test could also be simplified way more drastically to something like
assert_eq!(ptr_after_pushing_none, ptr_after_pushing_some);
assert_eq!(ptr_after_pulling_none, ptr_after_pulling_some);
(I only thought of this now)
There was a problem hiding this comment.
Thanks for the elaboration!
Robbepop
left a comment
Robbepop
left a comment
There was a problem hiding this comment.
Thanks a lot for this fix! We will probably release an ink! 3.0-rc3 soon with it.
| } | ||
|
|
||
| #[test] | ||
| fn regression_test_for_issue_570() -> ink_env::Result<()> { |
There was a problem hiding this comment.
Thanks for the elaboration!
3 participants
Closes #568.
The bug was in the the implementation of
SpreadLayoutforOption:On
push_spread,clear_spread,pull_spreadno further forwarding to the inner valuepush_spread,clear_spread,pull_spreadfunctions happened if theOptionwasNone.This is problematic because these methods advance the ptr by one ‒ which happened if the
OptionwasSome(…). ForNoneit didn't happen, thus causing havoc for all objects that subsequently tried to push, pull or clear their content from storage.