Skip to content
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

test: add light client sync test for forced update before update timeout #4127

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

bshastry
Copy link
Contributor

This PR adds a test case test_light_client_sync_no_force_update that verifies light client force update behavior before timeout threshold is reached. The test:

  • Creates a valid but partial update (missing finalized header)
  • Advances state to just before UPDATE_TIMEOUT threshold
  • Verifies force update does not occur prematurely

This complements existing timeout tests by explicitly covering the following negative case (first branch in process_light_client_store_force_update evaluates to False because current slot is at the timeout threshold) that was previously uncovered

def process_light_client_store_force_update(store: LightClientStore, current_slot: Slot) -> None:
    if (
        current_slot > store.finalized_header.beacon.slot + UPDATE_TIMEOUT
        and store.best_valid_update is not None
    ):
        # Forced best update when the update timeout has elapsed.
        # Because the apply logic waits for `finalized_header.beacon.slot` to indicate sync committee finality,
        # the `attested_header` may be treated as `finalized_header` in extended periods of non-finality
        # to guarantee progression into later sync committee periods according to `is_better_update`.
        if store.best_valid_update.finalized_header.beacon.slot <= store.finalized_header.beacon.slot:
            store.best_valid_update.finalized_header = store.best_valid_update.attested_header
        apply_light_client_update(store, store.best_valid_update)
        store.best_valid_update = None

@bshastry
Copy link
Contributor Author

Might I trouble @etan-status for a review again since the PR covers light client spec? 😅

yield from emit_force_update(test, spec, state)
# Store should remain unchanged since timeout wasn't reached
assert test.store.finalized_header.beacon.slot == finalized_state.slot

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Finish test
yield from finish_lc_sync_test(test)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to ensure that yaml files are created for client implementations to be tested against this.

@with_light_client
@spec_state_test_with_matching_config
@with_presets([MINIMAL], reason="too slow")
def test_light_client_sync_no_force_update(spec, state):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would recommend moving this up by a function (above run_lc_sync_test_upgraded_store_with_legacy_data), so that the test_..._with_legacy_data wrappers are located next to the run_... definition.

Copy link
Contributor

@etan-status etan-status left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the test to also cover the test of force update before timeout is reached.

PR generally looks good to me once the comments are addressed. If you could upload the generated test zip I can cross-check against Nimbus implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants