Spec trace experimental prototype implementation - #1
Conversation
+ some vibe style improvements (this is not meant to be submitted, drafting this strictly for experimental purposes)
paths are still weird and need fixing
main structures in place, let's now examine the overall structure adn see if it's good enough to base the final implementation on
the test is still producing three identical state dumps though, to be reviewed
This enhances the test trace generator to include essential configuration details required for reproducing test vectors. Changes: - Adds a top-level `metadata` block to `trace.yaml` containing the fork, preset, and generator version. - Adds a `parameters` block to `trace.yaml` (under `context`) to capture simple test setup values (int, str, bool, None). - Updates `RecordingSpec` to merge static metadata (from init) and dynamic metadata (from `spec.meta()`) into a single dictionary. - Updates the `@record_spec_trace` decorator in `context.py` to extract these values from the test environment and pass them to the recorder. - Updates Pydantic models in `trace_models.py` to reflect the new schema.
This modifies the RecordingSpec wrapper to catch exceptions raised by spec functions. When an exception occurs, it is logged in the trace with an 'error' block, and the exception is re-raised to allow the test to fail as expected.
This patch improves the portability and readability of the generated trace files. Changes: - Forces conversion of int/str subclasses (e.g., Slot, Epoch) to primitive types to avoid python-specific YAML tags. - Excludes 'None' values from the generated trace.yaml to remove noise (e.g., "error: null" on successful steps).
This ensures that all byte objects (including subclasses like Root, BLSPubkey, etc.) are serialized as standard '0x...' hex strings instead of YAML binary blobs or Python objects. This is essential for cross-client compatibility.
This ensures that function return values are properly sanitized (converted to primitives or hex strings) before being written to the trace. Previously, subclasses of int (like ValidatorIndex) were leaking Python-specific YAML tags into the output.
This ensures that when a state object is mutated in-place, it is assigned a new version name (e.g., v1, v2) in the trace. It also removes redundant 'load_state' steps where the state did not change.
This replaces the sequential version numbering (v0, v1) for state objects with their actual Merkle root hash (hex encoded, no 0x prefix). It also removes redundant 'load_state' operations when the state has not changed.
IvanAnishchuk
changed the title
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an experimental prototype for spec tracing, which is a significant and well-structured addition. The core logic for intercepting spec calls, recording them, and managing artifacts is implemented using wrapt and Pydantic models. The code is generally of high quality, with good test coverage for the new tracing mechanism.
My review focuses on improving code clarity, removing unused code, and enhancing maintainability. I've suggested a few refactorings to make the code more explicit and easier to follow, and pointed out a misleading comment in one of the example tests. Overall, this is a great foundation for the new tracing feature.
| # We must re-assign the `state` variable, as `process_slot` | ||
| # is a pure function that returns a new, modified state. |
There was a problem hiding this comment.
This comment is misleading. In the pyspec implementation, process_slot mutates the state object in-place and does not return a new state. The comment suggests it's a pure function, which is incorrect in this context. Additionally, the code on line 25 does not re-assign the state variable. I recommend removing this comment to avoid confusion.
| # root_hex_str = root_hex.hex() | ||
|
|
||
| # Find the context variable for this state | ||
| state_name = None | ||
| for name, obj in proxy._model._name_to_obj.items(): | ||
| if obj.hash_tree_root() == root_hex: | ||
| state_name = name | ||
| break | ||
| assert state_name is not None |
There was a problem hiding this comment.
The way state_name is retrieved is a bit indirect. You can make this more direct and consistent with other tests in this file (e.g., test_argument_sanitization) by constructing the state_name from the known root hash. This also resolves the TODO on line 155, which can then be removed.
root_hex_str = root_hex.hex()
state_name = f"$context.states.{root_hex_str}"
# Check that the state was registered with the correct name
assert state_name in proxy._model._name_to_obj| if obj_type == "states": | ||
| current_root = obj.hash_tree_root().hex() | ||
| if not existing_name.endswith(f".{current_root}"): | ||
| pass # Root changed, re-register with new hash | ||
| else: | ||
| return existing_name | ||
| else: | ||
| return existing_name |
There was a problem hiding this comment.
The logic for handling state re-registration can be simplified to improve readability. The pass statement and the nested else make the control flow slightly harder to follow. You can refactor this to check for the case where the object should not be re-registered and return early, letting the re-registration logic be the default path.
| if obj_type == "states": | |
| current_root = obj.hash_tree_root().hex() | |
| if not existing_name.endswith(f".{current_root}"): | |
| pass # Root changed, re-register with new hash | |
| else: | |
| return existing_name | |
| else: | |
| return existing_name | |
| if obj_type == "states": | |
| current_root = obj.hash_tree_root().hex() | |
| if existing_name.endswith(f".{current_root}"): | |
| return existing_name | |
| # Root changed, fall through to re-register with new hash. | |
| else: | |
| return existing_name |
| self._self_config_data = {} | ||
| self._self_last_root = None | ||
|
|
||
| self._model = TraceModel(metadata=metadata or {}, context={"parameters": parameters or {}}) |
There was a problem hiding this comment.
The initialization of TraceModel is a bit implicit. Pydantic creates a ContextModel from the dictionary you provide for the context field. For better clarity and type safety, I recommend explicitly creating a ContextModel instance. This makes the code easier to understand and leverages the type system more effectively. To do this, you'll need to add ContextModel to the import from .trace_models (around line 18).
| self._model = TraceModel(metadata=metadata or {}, context={"parameters": parameters or {}}) | |
| self._model = TraceModel(metadata=metadata or {}, context=ContextModel(parameters=parameters or {})) |
| self._self_process_arg(state_obj, auto_artifact=True) | ||
|
|
||
| def _self_process_arg( | ||
| self, arg: Any, preferred_name: str | None = None, auto_artifact: bool = False |
There was a problem hiding this comment.
The auto_artifact parameter in _self_process_arg is defined but never used within the function. It appears to be a remnant of a previous implementation. To improve code clarity and remove dead code, I suggest removing this parameter from the function signature. You'll also need to remove it from all call sites within this file (lines 96, 112, 177, and 203).
| self, arg: Any, preferred_name: str | None = None, auto_artifact: bool = False | |
| self, arg: Any, preferred_name: str | None = None |
1 participant
(needs more work before submitting to eth-consensus-spec)
traces look like this currently (non-normative example)