Fix Option pointer bug (#570)

* Advance ptr, even when 'None'

* Revert me: Add minimal contract which reproduces bug

* Fix typo: explicitely ➜ explicitly

* Add regression test

* Improve structure

* Revert "Revert me: Add minimal contract which reproduces bug"

This reverts commit 69cd40e.

* Fix clippy error 'result_unit_err'

* Fix clippy error 'result_unit_err'

* Fix typo: sucesful ➜ successful

* Improve comment

* Remove no-op

* Remove type aliases for V1/V2

crates/storage/src/lazy/lazy_cell.rs

@@ -523,7 +523,7 @@ mod tests {
                 // We prevent the intermediate instance from clearing the storage preemtively by wrapping
                 // it inside `ManuallyDrop`. The third step will clean up the same storage region afterwards.
                 //
-                // We explicitely do not touch or assert the value of `pulled_pair.0` in order to trigger
+                // We explicitly do not touch or assert the value of `pulled_pair.0` in order to trigger
                 // the bug.
                 let pulled_pair: (LazyCell<i32>, i32) =
                     SpreadLayout::pull_spread(&mut KeyPtr::from(root_key));
@@ -545,4 +545,57 @@ mod tests {
             Ok(())
         })
     }
+
+    #[test]
+    fn regression_test_for_issue_570() -> ink_env::Result<()> {
+        run_test::<ink_env::DefaultEnvironment, _>(|_| {
+            let root_key = Key::from([0x00; 32]);
+            {
+                // Step 1: Push two valid values one after the other to contract storage.
+                // The first value needs to be an `Option::None` value, since the bug was
+                // then messing up following pointers.
+                let v1: Option<u32> = None;
+                let v2: u32 = 13;
+                let mut ptr = KeyPtr::from(root_key);
+
+                SpreadLayout::push_spread(&v1, &mut ptr);
+                SpreadLayout::push_spread(&v2, &mut ptr);
+            }
+            {
+                // Step 2: Pull the values from the step before.
+                //
+                // 1. Change the first values `None` to `Some(...)`.
+                // 2. Push the first value again to contract storage.
+                //
+                // We prevent the intermediate instance from clearing the storage preemptively
+                // by wrapping it inside `ManuallyDrop`. The third step will clean up the same
+                // storage region afterwards.
+                let mut ptr = KeyPtr::from(root_key);
+                let pulled_v1: Option<u32> = SpreadLayout::pull_spread(&mut ptr);
+                let mut pulled_v1 = core::mem::ManuallyDrop::new(pulled_v1);
+
+                let pulled_v2: u32 = SpreadLayout::pull_spread(&mut ptr);
+                let pulled_v2 = core::mem::ManuallyDrop::new(pulled_v2);
+
+                assert_eq!(*pulled_v1, None);
+                assert_eq!(*pulled_v2, 13);
+
+                *pulled_v1 = Some(99u32);
+                SpreadLayout::push_spread(&*pulled_v1, &mut KeyPtr::from(root_key));
+            }
+            {
+                // Step 3: Pull the values again from the storage.
+                //
+                // If the bug with `Option` has been fixed in PR #520 we must be able to inspect
+                // the correct values for both entries.
+                let mut ptr = KeyPtr::from(root_key);
+                let pulled_v1: Option<u32> = SpreadLayout::pull_spread(&mut ptr);
+                let pulled_v2: u32 = SpreadLayout::pull_spread(&mut ptr);
+
+                assert_eq!(pulled_v1, Some(99));
+                assert_eq!(pulled_v2, 13);
+            }
+            Ok(())
+        })
+    }
 }

crates/storage/src/traits/impls/prims.rs

@@ -66,6 +66,8 @@ where
         <u8 as SpreadLayout>::push_spread(&(self.is_some() as u8), ptr);
         if let Some(value) = self {
             <T as SpreadLayout>::push_spread(value, ptr);
+        } else {
+            ptr.advance_by(<T as SpreadLayout>::FOOTPRINT);
         }
     }
 
@@ -76,12 +78,17 @@ where
         <u8 as SpreadLayout>::clear_spread(&0, ptr);
         if let Some(value) = self {
             <T as SpreadLayout>::clear_spread(value, ptr)
+        } else {
+            ptr.advance_by(<T as SpreadLayout>::FOOTPRINT);
         }
     }
 
     fn pull_spread(ptr: &mut KeyPtr) -> Self {
         match <u8 as SpreadLayout>::pull_spread(ptr) {
-            0u8 => None,
+            0u8 => {
+                ptr.advance_by(<T as SpreadLayout>::FOOTPRINT);
+                None
+            }
             1u8 => Some(<T as SpreadLayout>::pull_spread(ptr)),
             _ => unreachable!("invalid Option discriminant"),
         }

examples/multisig_plain/lib.rs

@@ -155,6 +155,14 @@ mod multisig_plain {
         pub gas_limit: u64,
     }
 
+    /// Errors that can occur upon calling this contract.
+    #[derive(Copy, Clone, Debug, PartialEq, Eq, scale::Encode, scale::Decode)]
+    #[cfg_attr(feature = "std", derive(::scale_info::TypeInfo))]
+    pub enum Error {
+        /// Returned if the call failed.
+        TransactionFailed,
+    }
+
     #[ink(storage)]
     pub struct MultisigPlain {
         /// Every entry in this map represents the confirmation of an owner for a
@@ -227,7 +235,7 @@ mod multisig_plain {
         /// the output in bytes. The Option is `None` when the transaction was executed through
         /// `invoke_transaction` rather than `evaluate_transaction`.
         #[ink(topic)]
-        result: Result<Option<Vec<u8>>, ()>,
+        result: Result<Option<Vec<u8>>, Error>,
     }
 
     /// Emitted when an owner is added to the wallet.
@@ -479,7 +487,10 @@ mod multisig_plain {
         /// Its return value indicates whether the called transaction was successful.
         /// This can be called by anyone.
         #[ink(message)]
-        pub fn invoke_transaction(&mut self, trans_id: TransactionId) -> Result<(), ()> {
+        pub fn invoke_transaction(
+            &mut self,
+            trans_id: TransactionId,
+        ) -> Result<(), Error> {
             self.ensure_confirmed(trans_id);
             let t = self.take_transaction(trans_id).expect(WRONG_TRANSACTION_ID);
             let result = build_call::<<Self as ::ink_lang::ContractEnv>::Env>()
@@ -491,7 +502,7 @@ mod multisig_plain {
                 )
                 .returns::<()>()
                 .fire()
-                .map_err(|_| ());
+                .map_err(|_| Error::TransactionFailed);
             self.env().emit_event(Execution {
                 transaction: trans_id,
                 result: result.map(|_| None),
@@ -502,13 +513,13 @@ mod multisig_plain {
         /// Evaluate a confirmed execution and return its output as bytes.
         ///
         /// Its return value indicates whether the called transaction was successful and contains
-        /// its output when sucesful.
+        /// its output when successful.
         /// This can be called by anyone.
         #[ink(message)]
         pub fn eval_transaction(
             &mut self,
             trans_id: TransactionId,
-        ) -> Result<Vec<u8>, ()> {
+        ) -> Result<Vec<u8>, Error> {
             self.ensure_confirmed(trans_id);
             let t = self.take_transaction(trans_id).expect(WRONG_TRANSACTION_ID);
             let result = build_call::<<Self as ::ink_lang::ContractEnv>::Env>()
@@ -520,7 +531,7 @@ mod multisig_plain {
                 )
                 .returns::<ReturnType<Vec<u8>>>()
                 .fire()
-                .map_err(|_| ());
+                .map_err(|_| Error::TransactionFailed);
             self.env().emit_event(Execution {
                 transaction: trans_id,
                 result: result.clone().map(Some),