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: Cover signature_slot tiebreaker in is_better_update #4124

Merged
merged 2 commits into from
Feb 11, 2025

Conversation

bshastry
Copy link
Contributor

@bshastry bshastry commented Feb 6, 2025

The is_better_update function has a final tiebreaker that compares signature_slots when all other fields (participation, finality, etc.) are equal. This branch was previously uncovered because test updates always had equal slots.

This change adds a test case with a higher signature_slot to an otherwise identical update, ensuring we test the final return statement where earlier signature_slots are preferred.

The change is minimal and leverages existing test infrastructure by adding one additional update to create_test_update quality-ordered list.

Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me, but I'm not super familiar with light client stuff either. I would appreciate @etan-status' opinion on this change before merging.

The relevant tiebreaker (the final return statement). I might suggest adding a "Tiebreaker 3: ..." comment too.

# Tiebreaker 2: Prefer older data (fewer changes to best)
if new_update.attested_header.beacon.slot != old_update.attested_header.beacon.slot:
return new_update.attested_header.beacon.slot < old_update.attested_header.beacon.slot
return new_update.signature_slot < old_update.signature_slot

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 catching this, having the additional test coverage is useful. 🔥

@Ssaaddaamm

This comment has been minimized.

@bshastry bshastry force-pushed the test_lc_update_sig_slot branch from 6d8c689 to ea74634 Compare February 11, 2025 09:22
@bshastry
Copy link
Contributor Author

This looks reasonable to me, but I'm not super familiar with light client stuff either. I would appreciate @etan-status' opinion on this change before merging.

The relevant tiebreaker (the final return statement). I might suggest adding a "Tiebreaker 3: ..." comment too.

# Tiebreaker 2: Prefer older data (fewer changes to best)
if new_update.attested_header.beacon.slot != old_update.attested_header.beacon.slot:
return new_update.attested_header.beacon.slot < old_update.attested_header.beacon.slot
return new_update.signature_slot < old_update.signature_slot

Done! Pushed an update that may need a reapproval. Thank you 🙏

@bshastry
Copy link
Contributor Author

Thank you for your reviews @jtraglia and @etan-status

I have addressed your review comments

@jtraglia jtraglia merged commit 044f22d into ethereum:dev Feb 11, 2025
23 checks passed
etan-status added a commit to status-im/nimbus-eth2 that referenced this pull request Feb 11, 2025
Add `signature_slot` comparison to `is_better_update` LC data comparator
for robustness in edge cases, and sync style with latest specs.

- ethereum/consensus-specs#4124
tersec pushed a commit to status-im/nimbus-eth2 that referenced this pull request Feb 11, 2025
Add `signature_slot` comparison to `is_better_update` LC data comparator
for robustness in edge cases, and sync style with latest specs.

- ethereum/consensus-specs#4124
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.

4 participants