-
Notifications
You must be signed in to change notification settings - Fork 1.1k
eip7495: add ProgressiveContainer specs and tests #4529
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This follows up on ethereum#4445 and ethereum#4480 to extend EIP-7495 support for containers, making the `ProgressiveContainer` type available to implementations in _features. - https://eips.ethereum.org/EIPS/eip-7495
Implement test runner for new consensus-spec test format for EIP-7495 ProgressiveContainer. New tests will automatically be picked up once they become available. - ethereum/consensus-specs#4529
I think as part of the test vectors we should have serialized binaries that were serialized with an active_field marked as 1, but the deserializer has it marked as 0 |
for i, modded_typ in enumerate(MODIFIED_PROGRESSIVE_CONTIANERS): | ||
serialized = serialize(container_case_fn(rng, mode, modded_typ)) | ||
try: | ||
_ = deserialize(typ, serialized) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etan-status are we testing when the container is modified but the deserializationn should work, or it shouldn't work always?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a filter, so that if we have a case where a serialization is valid for both modded_typ and typ, that we don't emit it as an 'invalid test case'.
like, we serialize an object of type 'modded_typ'. then we attempt to deserialize it as 'typ'.
- if this succeeds, then the case is not invalid
- if it fails, then the case is an example that we can emit as a test vector
yield ( | ||
f"{name}_{mode.to_name()}_modded_{i}", | ||
invalid_test_case(lambda serialized=serialized: serialized), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be an asseert False or something outside the exception if the exception is the expected behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both cases can happen. if two types are e.g.
class A(ProgessiveContainer(active_fields=[1])):
x: uint8
class B(ProgressiveContainer(active_fields=[0, 1])):
y: uint8
serialize(A()) can also successfully deserialize into B.
if the deserialization succeeds, it just means that that particular test vector is not relevant for 'invalid_test_case'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note it's the same concept as in ssz_container.py
Implement test runner for new consensus-spec test format for EIP-7495 ProgressiveContainer. New tests will automatically be picked up once they become available. - ethereum/consensus-specs#4529
This follows up on #4445 and #4480 to extend EIP-7495 support for containers, making the
ProgressiveContainer
type available to implementations in _features.