Add panic after continuing storage iteration after a storage mutation

docs/language/accounts.mdx

@@ -641,8 +641,28 @@ as is the behavior when a path is added or removed from storage.
 
 <Callout type="warning">
 The order of iteration is undefined. Do not rely on any particular behaviour.
+
 Saving to or removing from storage during iteration can cause the order in which values are stored to change arbitrarily. 
-Continuing to iterate after such an operation may cause iteration to repeat some elements or skip others. 
+
+Continuing to iterate after such an operation will cause Cadence to panic and abort execution. In order to avoid such errors,
+we recommend not modifying storage during iteration. If you do, return `false` from the iteration callback to cause iteration 
+to end after the mutation like so:
+
+```cadence
+account.save(1, to: /storage/foo1)
+account.save(2, to: /storage/foo2)
+account.save(3, to: /storage/foo3)
+account.save("qux", to: /storage/foo4)
+
+account.forEachStored(fun (path: StoragePath, type: Type): Bool {
+    if type == Type<String>() {
+        account.save("bar", to: /storage/foo5)
+        // returning false here ends iteration after storage is modified, preventing a panic
+        return false
+    }
+    return true
+})
+```
 </Callout>
 
 ## Storage limit

runtime/interpreter/errors.go

@@ -825,3 +825,16 @@ func (DuplicateKeyInResourceDictionaryError) IsUserError() {}
 func (e DuplicateKeyInResourceDictionaryError) Error() string {
 	return "duplicate key in resource dictionary"
 }
+
+// StorageMutatedDuringIterationError
+type StorageMutatedDuringIterationError struct {
+	LocationRange
+}
+
+var _ errors.UserError = StorageMutatedDuringIterationError{}
+
+func (StorageMutatedDuringIterationError) IsUserError() {}
+
+func (e StorageMutatedDuringIterationError) Error() string {
+	return "storage iteration continued after modifying storage"
+}

runtime/interpreter/interpreter.go

@@ -355,6 +355,8 @@ type Interpreter struct {
 	atreeValueValidationEnabled    bool
 	atreeStorageValidationEnabled  bool
 	tracingEnabled                 bool
+	inStorageIteration             bool
+	storageMutatedDuringIteration  bool
 	// TODO: ideally this would be a weak map, but Go has no weak references
 	referencedResourceKindedValues       ReferencedResourceKindedValues
 	invalidatedResourceValidationEnabled bool
@@ -688,11 +690,13 @@ var BaseActivation = func() *VariableActivation {
 func NewInterpreter(program *Program, location common.Location, options ...Option) (*Interpreter, error) {
 
 	interpreter := &Interpreter{
-		Program:           program,
-		Location:          location,
-		Globals:           map[string]*Variable{},
-		resourceVariables: map[ResourceKindedValue]*Variable{},
-		baseActivation:    BaseActivation,
+		Program:                       program,
+		Location:                      location,
+		Globals:                       map[string]*Variable{},
+		resourceVariables:             map[ResourceKindedValue]*Variable{},
+		baseActivation:                BaseActivation,
+		inStorageIteration:            false,
+		storageMutatedDuringIteration: false,
 	}
 
 	interpreter.activations = NewVariableActivations(interpreter)
@@ -2766,14 +2770,18 @@ func (interpreter *Interpreter) NewSubInterpreter(
 		WithMemoryGauge(interpreter.memoryGauge),
 	}
 
-	return NewInterpreter(
+	subInterp, err := NewInterpreter(
 		program,
 		location,
 		append(
 			defaultOptions,
 			options...,
 		)...,
 	)
+	subInterp.inStorageIteration = interpreter.inStorageIteration
+	subInterp.storageMutatedDuringIteration = interpreter.storageMutatedDuringIteration
+
+	return subInterp, err
 }
 
 func (interpreter *Interpreter) storedValueExists(
@@ -2808,6 +2816,7 @@ func (interpreter *Interpreter) writeStored(
 ) {
 	accountStorage := interpreter.Storage.GetStorageMap(storageAddress, domain, true)
 	accountStorage.WriteValue(interpreter, identifier, value)
+	interpreter.recordStorageMutation()
 }
 
 type ValueConverterDeclaration struct {
@@ -3710,6 +3719,12 @@ func (interpreter *Interpreter) storageAccountPaths(addressValue AddressValue, g
 	return interpreter.accountPaths(addressValue, getLocationRange, common.PathDomainStorage, PrimitiveStaticTypeStoragePath)
 }
 
+func (interpreter *Interpreter) recordStorageMutation() {
+	if interpreter.inStorageIteration {
+		interpreter.storageMutatedDuringIteration = true
+	}
+}
+
 func (interpreter *Interpreter) newStorageIterationFunction(addressValue AddressValue, domain common.PathDomain, pathType sema.Type) *HostFunctionValue {
 	address := addressValue.ToAddress()
 
@@ -3732,6 +3747,12 @@ func (interpreter *Interpreter) newStorageIterationFunction(addressValue Address
 
 			invocationTypeParams := []sema.Type{pathType, sema.MetaType}
 
+			inIteration := inter.inStorageIteration
+			inter.inStorageIteration = true
+			defer func() {
+				inter.inStorageIteration = inIteration
+			}()
+
 			for key, value := storageIterator.Next(); key != "" && value != nil; key, value = storageIterator.Next() {
 				pathValue := NewPathValue(inter, domain, key)
 				runtimeType := NewTypeValue(inter, value.StaticType(inter))
@@ -3753,6 +3774,18 @@ func (interpreter *Interpreter) newStorageIterationFunction(addressValue Address
 				if !shouldContinue {
 					break
 				}
+
+				// it is not safe to check this at the beginning of the loop (i.e. on the next invocation of the callback)
+				// because if the mutation performed in the callback reorganized storage such that the iteration pointer is now
+				// at the end, we will not invoke the callback again but will still silently skip elements of storage. In order
+				// to be safe, we perform this check here to effectively enforce that users return `false` from their callback
+				// in all cases where storage is mutated
+				if inter.storageMutatedDuringIteration {
+					panic(StorageMutatedDuringIterationError{
+						LocationRange: getLocationRange(),
+					})
+				}
+
 			}
 
 			return NewVoidValue(inter)