fix: fix state leak in modifiers; add tests to catch this

Author: fselmo

packages/testing/src/execution_testing/test_types/block_access_list/modifiers.py

@@ -28,10 +28,9 @@ def _remove_field_from_accounts(
 ) -> Callable[[BlockAccessList], BlockAccessList]:
     """Abstracted helper to remove a field from specified accounts."""
     len_addresses = len(addresses)
-    found_addresses = set()
 
     def transform(bal: BlockAccessList) -> BlockAccessList:
-        nonlocal found_addresses
+        found_addresses: set[Address] = set()
         new_root = []
         for account_change in bal.root:
             if account_change.address in addresses:
@@ -70,11 +69,10 @@ def _modify_field_value(
     Abstracted helper to modify a field value for a specific account and
     transaction.
     """
-    found_address = False
-    found_index = False
 
     def transform(bal: BlockAccessList) -> BlockAccessList:
-        nonlocal found_address, found_index
+        found_address = False
+        found_index = False
         new_root = []
         for account_change in bal.root:
             if account_change.address == address:
@@ -261,13 +259,12 @@ def swap_bal_indices(
     idx1: int, idx2: int
 ) -> Callable[[BlockAccessList], BlockAccessList]:
     """Swap block access indices throughout the BAL, modifying ordering."""
-    nonce_indices = {idx1: False, idx2: False}
-    balance_indices = nonce_indices.copy()
-    storage_indices = nonce_indices.copy()
-    code_indices = nonce_indices.copy()
 
     def transform(bal: BlockAccessList) -> BlockAccessList:
-        nonlocal nonce_indices, balance_indices, storage_indices, code_indices
+        nonce_indices = {idx1: False, idx2: False}
+        balance_indices = nonce_indices.copy()
+        storage_indices = nonce_indices.copy()
+        code_indices = nonce_indices.copy()
         new_root = []
         for account_change in bal.root:
             new_account = account_change.model_copy(deep=True)
@@ -399,10 +396,8 @@ def append_change(
     else:
         raise TypeError(f"Unsupported change type: {type(change)}")
 
-    found_address = False
-
     def transform(bal: BlockAccessList) -> BlockAccessList:
-        nonlocal found_address
+        found_address = False
         new_root = []
         for account_change in bal.root:
             if account_change.address == account:
@@ -441,10 +436,9 @@ def append_storage(
       slot_changes
     - If change provided and slot new: creates new BalStorageSlot
     """
-    found_address = False
 
     def transform(bal: BlockAccessList) -> BlockAccessList:
-        nonlocal found_address
+        found_address = False
         new_root = []
         for account_change in bal.root:
             if account_change.address == address:
@@ -491,10 +485,9 @@ def duplicate_account(
     address: Address,
 ) -> Callable[[BlockAccessList], BlockAccessList]:
     """Duplicate an account entry in the BAL."""
-    address_present = False
 
     def transform(bal: BlockAccessList) -> BlockAccessList:
-        nonlocal address_present
+        address_present = False
         new_root = []
         for account_change in bal.root:
             new_root.append(account_change)
@@ -528,15 +521,14 @@ def _duplicate_in_field(
     When sub_field and sub_match_fn are provided, find the parent entry
     via match_fn then duplicate within sub_field using sub_match_fn.
     """
-    found = False
 
     def _copy(entry: Any) -> Any:
         if hasattr(entry, "model_copy"):
             return entry.model_copy(deep=True)
         return ZeroPaddedHexNumber(entry)
 
     def transform(bal: BlockAccessList) -> BlockAccessList:
-        nonlocal found
+        found = False
         new_root = []
         for account_change in bal.root:
             if account_change.address == address:
@@ -677,10 +669,9 @@ def insert_storage_read(
     Useful for testing that a key must not appear in both
     storage_changes and storage_reads.
     """
-    found_address = False
 
     def transform(bal: BlockAccessList) -> BlockAccessList:
-        nonlocal found_address
+        found_address = False
         new_root = []
         for account_change in bal.root:
             if account_change.address == address:

packages/testing/src/execution_testing/test_types/tests/test_block_access_list_modifiers.py

@@ -1,5 +1,7 @@
 """Unit tests for BAL modifier functions."""
 
+from typing import Callable
+
 import pytest
 
 from execution_testing.base_types import Address
@@ -13,6 +15,8 @@
     BlockAccessList,
 )
 from execution_testing.test_types.block_access_list.modifiers import (
+    append_change,
+    append_storage,
     duplicate_account,
     duplicate_balance_change,
     duplicate_code_change,
@@ -25,7 +29,9 @@
     modify_code,
     modify_nonce,
     modify_storage,
+    remove_nonces,
     reorder_accounts,
+    swap_bal_indices,
 )
 
 ALICE = Address(0xA)
@@ -304,3 +310,61 @@ def test_reorder_accounts_out_of_range_raises(
     """Raise when indices are not a valid permutation (skipped index)."""
     with pytest.raises(ValueError, match="valid permutation"):
         reorder_accounts([0, 2])(sample_bal)
+
+
+_EMPTY_BAL = BlockAccessList([])
+_ALICE_ONLY_BAL = BlockAccessList(
+    [BalAccountChange(address=ALICE, nonce_changes=[])]
+)
+
+
+@pytest.mark.parametrize(
+    "modifier_factory, missing_bal",
+    [
+        pytest.param(
+            lambda: remove_nonces(ALICE), _EMPTY_BAL, id="remove_nonces"
+        ),
+        pytest.param(
+            lambda: swap_bal_indices(1, 1), _EMPTY_BAL, id="swap_bal_indices"
+        ),
+        pytest.param(
+            lambda: append_change(
+                ALICE, BalNonceChange(block_access_index=2, post_nonce=5)
+            ),
+            _EMPTY_BAL,
+            id="append_change",
+        ),
+        pytest.param(
+            lambda: append_storage(CONTRACT, slot=7, read=True),
+            _EMPTY_BAL,
+            id="append_storage",
+        ),
+        pytest.param(
+            lambda: duplicate_account(ALICE),
+            _EMPTY_BAL,
+            id="duplicate_account",
+        ),
+        pytest.param(
+            lambda: duplicate_nonce_change(ALICE, 1),
+            _ALICE_ONLY_BAL,
+            id="duplicate_nonce_change",
+        ),
+        pytest.param(
+            lambda: insert_storage_read(CONTRACT, 99),
+            _EMPTY_BAL,
+            id="insert_storage_read",
+        ),
+    ],
+)
+def test_reused_callable_does_not_carry_found_state(
+    sample_bal: BlockAccessList,
+    modifier_factory: Callable[
+        [], Callable[[BlockAccessList], BlockAccessList]
+    ],
+    missing_bal: BlockAccessList,
+) -> None:
+    """A modifier's found-state must not persist across calls."""
+    modifier = modifier_factory()
+    modifier(sample_bal)
+    with pytest.raises(ValueError, match="not found"):
+        modifier(missing_bal)