Check assert time >= store.time in fork-choice tests
#3550
Conversation
| spec.on_tick(store, time) | ||
| test_steps.append({'tick': int(time)}) | ||
| output_store_checks(spec, store, test_steps) | ||
| if store.time < time: |
There was a problem hiding this comment.
Shouldn’t we omit this check and let it fail? in this case it can highlight an error in a test
There was a problem hiding this comment.
This is implementation dependant, time, in particular for spectests is very tricky. Prysm for example modifies genesis time on every tick check and it would be undefined behavior if it went back in time. I advocate for forkchoice tests to only fail on bad output from the API, that is, bad head, bad proposer boost roots, etc .
There was a problem hiding this comment.
Ah, I meant that if we’re adding assertion into the spec we can probably let this assertion cause an exception when the test isn’t properly design. Because if you set your time back this is probably something you’re doing wrong when writing the test, something that worth paying attention too instead of quietly omitting the problem
There was a problem hiding this comment.
If I understand correctly this is just ensuring that the tests don't get written in a way that time goes backwards, and that seems fair...
There was a problem hiding this comment.
@rolfyone is right; it was mainly to avoid righting useless steps in the existing test scripts.
About the failure cases, good point. If we want to cover it, we should add new test cases for on_tick. We do have the test format field to indicate if on_tick step is invalid: https://github.com/ethereum/consensus-specs/tree/dev/tests/formats/fork_choice#on_tick-execution-step
But I hear you @potuz that it could be implementation-dependant...
Do you guys think it is okay to revert the spec change and only make the test script mistake-proofing (avoid making time < store.time call)? I'm trying to find the balance between the abstract FC specs and useful tests.
There was a problem hiding this comment.
I think it makes more sense to have an assert store.time < time here so that the test writer sees very quickly that they've done something wrong, instead of it just silently doing nothing
There was a problem hiding this comment.
An assert in this place looks like a nice alternative to the assert in the on_tick
specs/phase0/fork-choice.md
Outdated
| @@ -623,6 +623,9 @@ def update_latest_messages(store: Store, attesting_indices: Sequence[ValidatorIn | |||
|
|
|||
| ```python | |||
| def on_tick(store: Store, time: uint64) -> None: | |||
| # Precondition | |||
| assert time >= store.time | |||
There was a problem hiding this comment.
I'm not certain we should add this unless we are going to actively test it. Our guiding principle is that asserts should be triggerable with invalid messages for spec and testing purposes. But this would only really be triggerable if the client's system was broken rather than them getting invalid messages
hwwhww
changed the title
assert time >= store.time in on_tickassert time >= store.time in FC tests
hwwhww
changed the title
assert time >= store.time in FC testsassert time >= store.time in fork-choice tests
To prevent other future bugs like #3548, this PR adds
assert time >= store.timetoon_tick.Note that we have stated "
on_tick(store, time)whenevertime > store.timewheretimeis the current Unix time" in fork choice specs. This PR is only to make it more explicit.It will remove some extra
on_ticksteps from test vectors.