Skip to content

Commit fb5ff3c

Browse files
tcoratgerclaude
andauthored
refactor: drop model_copy in favour of in-place mutation (leanEthereum#789)
* refactor: drop model_copy in favour of in-place mutation StrictBaseModel was previously frozen, which forced every state update to go through model_copy(update={...}) and produced a verbose functional style throughout the codebase. The frozen constraint was introduced as groundwork for formal verification, which we are not yet using; in the meantime the indirection hurts readability. Changes: - src/lean_spec/base.py: drop frozen=True from StrictBaseModel and update the docstring (no longer claims immutability). - .claude/rules/ssz-patterns.md: drop "immutability" from the SSZModel principle bullet so the rule matches the new behavior. - All 115 model_copy(update=...) call sites across src/, tests/, and packages/testing/ converted to direct field assignment. Intermediate dict-building variables inlined where the result was used only once. Exceptions: - The three JustifiedSlots collection helpers (with_justified, extend_to_slot, shift_window) still return new instances, but via type(self)(data=...) instead of model_copy. This preserves the return-new contract that callers rely on for SSZ collection methods. - The api_endpoint test fixture's "return self.model_copy(update=handler( store, self))" pattern becomes a setattr loop over handler's output dict, then return self. - MockStore in tests/lean_spec/node/chain/test_service.py loses its hand-rolled model_copy method (no longer needed; it's a plain dataclass and is mutable by default). just check passes (ruff lint + format, ty type check, codespell, mdformat). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: restore hashability and pure-function semantics where required Dropping frozen globally lost two side effects the codebase relied on: hashability of value types used as dict keys / set elements, and the "returns a new state" contract that callers of process_slots and state_transition were depending on. Both are restored surgically. Hashability: - Re-freeze the value types used as dict keys / set elements: Checkpoint, AttestationData, Attestation, Signature, TypeOneMultiSignature, TypeTwoMultiSignature, KeyPair, ValidatorKeyPair, Eth2Data. Per-class model_config override leaves the stateful types (Store, State, Block, BlockHeader, ENR, etc.) unfrozen and mutable. - Add explicit __hash__ on Signature, TypeOneMultiSignature, and TypeTwoMultiSignature: their nested list-bearing fields break the auto-derived hash, so the SSZ-encoded bytes drive the hash instead. Pure-function semantics: - process_slots now deepcopies its input at entry, matching the existing docstring ("returns a new state with slot == target_slot"). This makes state_transition and build_block trivially pure. - Slot assertion relaxed from < to <= so process_slots is idempotent under repeated calls with the same target (build_block needs this). - Two Checkpoint mutations in process_block_header replaced with construction now that Checkpoint is frozen again. Test fixture in BlockSpec.build_signed_block_with_store deepcopies the store at entry: the simulation pipeline (on_tick, on_gossip_attestation, aggregate, accept_new_attestations) mutates store directly, so the caller's store needs an explicit barrier. Test fixes: - _replace_head_at_slot and _add_block_at_slot in test_service.py construct new Block instances instead of mutating the original (the AST conversion had broken them). - Corrupted-proof tests construct new TypeOneMultiSignature rather than mutating .proof on a frozen instance. - test_combined_path_rejects_{depth_mismatch,odd_depth} construct new HashSubTree to avoid polluting the prf_trees fixture across tests. - Dropped test_frozen_rejects_assignment on StrictBaseModel (the base itself is no longer frozen by design). - Dropped the "store is not store_before" identity check that was only meaningful when on_gossip_attestation returned a new store. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(spec): restore strict process_slots assertion Loosening the assertion to state.slot <= target_slot was a workaround for the mutation cascade through build_block. Now that process_slots deepcopies its input at entry, the caller's state.slot is never advanced across repeated calls, so the original strict inequality holds again. The spec filler test test_process_slots_target_equal_to_state_slot_rejected was relying on the strict form. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(forks/lstar): restore Checkpoint frozen override after merge The merge from main clobbered the per-class `frozen=True` model_config override on Checkpoint. That override was added in 24a0e7b alongside the same override on eight other value types used as dict keys / set members; Checkpoint was the only one that moved files in the merge (the deleted lean_spec/types/checkpoint.py from leanEthereum#790 collided with the new home spec/forks/lstar/containers.py from leanEthereum#785) and so it was the only one whose override was lost. Without the override AttestationData (which embeds Checkpoint) is no longer hashable, and on_gossip_attestation crashes when it inserts into store.attestation_signatures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 708b82f commit fb5ff3c

45 files changed

Lines changed: 424 additions & 617 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.claude/rules/ssz-patterns.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ When creating SSZ types, follow these established patterns:
2929
- These use Pydantic models with a `data` field for contents
3030
- Example: `MyList(data=[1, 2, 3])` *has* a list of data with SSZ serialization
3131

32-
**Key principle**: If the type conceptually *holds* or *contains* other data, use SSZModel for consistent validation and immutability.
32+
**Key principle**: If the type conceptually *holds* or *contains* other data, use SSZModel for consistent validation and SSZ encoding.
3333

3434
## Modular Architecture
3535

packages/testing/src/consensus_testing/test_fixtures/api_endpoint.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -283,4 +283,6 @@ def make_fixture(self) -> "ApiEndpointTest":
283283
genesis_time=self.genesis_params.get("genesisTime", 0),
284284
anchor_slot=self.genesis_params.get("anchorSlot", 0),
285285
)
286-
return self.model_copy(update=handler(store, self))
286+
for k, v in handler(store, self).items():
287+
setattr(self, k, v)
288+
return self

packages/testing/src/consensus_testing/test_fixtures/fork_choice.py

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -233,22 +233,14 @@ def make_fixture(self) -> Self:
233233
for i, validator in enumerate(self.anchor_state.validators):
234234
idx = ValidatorIndex(i)
235235
attestation_pubkey, proposal_pubkey = key_manager.get_public_keys(idx)
236-
validator = validator.model_copy(
237-
update={
238-
"attestation_pubkey": attestation_pubkey.encode_bytes(),
239-
"proposal_pubkey": proposal_pubkey.encode_bytes(),
240-
}
241-
)
236+
validator.attestation_pubkey = attestation_pubkey.encode_bytes()
237+
validator.proposal_pubkey = proposal_pubkey.encode_bytes()
242238
updated_validators.append(validator)
243239

244240
# Updating validators changes the state root.
245241
# We must also update the anchor block to match.
246-
self.anchor_state = self.anchor_state.model_copy(
247-
update={"validators": Validators(data=updated_validators)}
248-
)
249-
self.anchor_block = self.anchor_block.model_copy(
250-
update={"state_root": hash_tree_root(self.anchor_state)}
251-
)
242+
self.anchor_state.validators = Validators(data=updated_validators)
243+
self.anchor_block.state_root = hash_tree_root(self.anchor_state)
252244

253245
# Store initialization
254246
#

packages/testing/src/consensus_testing/test_fixtures/gossipsub_handler.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,8 +159,8 @@ class GossipsubHandlerTest(BaseConsensusFixture):
159159

160160
def make_fixture(self) -> "GossipsubHandlerTest":
161161
"""Produce the completed fixture with expected outputs filled in."""
162-
expected = asyncio.run(self._execute())
163-
return self.model_copy(update={"expected": expected})
162+
self.expected = asyncio.run(self._execute())
163+
return self
164164

165165
async def _execute(self) -> dict[str, Any]:
166166
"""

packages/testing/src/consensus_testing/test_fixtures/justifiability.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,8 @@ def make_fixture(self) -> "JustifiabilityTest":
4646
delta = self.slot - self.finalized_slot
4747
justifiable = s.is_justifiable_after(f)
4848

49-
output = {
49+
self.output = {
5050
"delta": delta,
5151
"isJustifiable": justifiable,
5252
}
53-
return self.model_copy(update={"output": output})
53+
return self

packages/testing/src/consensus_testing/test_fixtures/networking_codec.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,8 @@ def make_fixture(self) -> "NetworkingCodecTest":
9696
output = self._make_decode_failure()
9797
case _:
9898
raise ValueError(f"Unknown codec: {self.codec_name}")
99-
return self.model_copy(update={"output": output})
99+
self.output = output
100+
return self
100101

101102
def _make_decode_failure(self) -> dict[str, Any]:
102103
"""Assert that decoding `input.bytes` with `input.decoder` raises.

packages/testing/src/consensus_testing/test_fixtures/poseidon_permutation.py

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,5 @@ def make_fixture(self) -> "PoseidonPermutationTest":
6060
input_state = [Fp(v) for v in state_ints]
6161
output_state = engine.permute(input_state)
6262

63-
return self.model_copy(
64-
update={"output": {"outputState": [str(int(fp)) for fp in output_state]}}
65-
)
63+
self.output = {"outputState": [str(int(fp)) for fp in output_state]}
64+
return self

packages/testing/src/consensus_testing/test_fixtures/slot_clock.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,8 @@ def make_fixture(self) -> "SlotClockTest":
6161
result = self._make_total_intervals()
6262
case _:
6363
raise ValueError(f"Unknown operation: {self.operation}")
64-
output = {"config": config, **result}
65-
return self.model_copy(update={"output": output})
64+
self.output = {"config": config, **result}
65+
return self
6666

6767
def _make_from_unix_time(self) -> dict[str, Any]:
6868
"""Convert unix timestamp to interval count since genesis."""

packages/testing/src/consensus_testing/test_fixtures/ssz.py

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -98,12 +98,9 @@ def make_fixture(self) -> "SSZTest":
9898

9999
root = hash_tree_root(self.value)
100100

101-
return self.model_copy(
102-
update={
103-
"serialized": "0x" + ssz_bytes.hex(),
104-
"root": "0x" + root.hex(),
105-
}
106-
)
101+
self.serialized = "0x" + ssz_bytes.hex()
102+
self.root = "0x" + root.hex()
103+
return self
107104

108105
def _make_decode_failure(self) -> "SSZTest":
109106
"""Run the decode-failure path: assert decoding `raw_bytes` raises.
@@ -138,9 +135,6 @@ def _make_decode_failure(self) -> "SSZTest":
138135
f"{type(exception_raised).__name__}: {exception_raised}"
139136
)
140137

141-
return self.model_copy(
142-
update={
143-
"serialized": "0x" + raw.hex(),
144-
"root": "",
145-
}
146-
)
138+
self.serialized = "0x" + raw.hex()
139+
self.root = ""
140+
return self

packages/testing/src/consensus_testing/test_fixtures/state_transition.py

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -283,24 +283,16 @@ def _build_block_from_spec(
283283
)
284284
for fa in spec.forced_attestations
285285
]
286-
block = block.model_copy(
287-
update={
288-
"body": block.body.model_copy(
289-
update={
290-
"attestations": AggregatedAttestations(
291-
data=[*block.body.attestations.data, *forced]
292-
)
293-
}
294-
)
295-
}
286+
block.body.attestations = AggregatedAttestations(
287+
data=[*block.body.attestations.data, *forced]
296288
)
297289

298290
# The body changed, so re-run the transition to get the correct
299291
# post-state and state root.
300292
if post_state is not None:
301293
post_state = LstarSpec().process_slots(state, spec.slot)
302294
post_state = LstarSpec().process_block(post_state, block)
303-
block = block.model_copy(update={"state_root": hash_tree_root(post_state)})
295+
block.state_root = hash_tree_root(post_state)
304296

305297
return block, post_state
306298

0 commit comments

Comments
 (0)