Skip to content

Conversation

@0xahzam
Copy link

@0xahzam 0xahzam commented Jul 27, 2025

Problem

The votor module contains inconsistent arithmetic operation patterns.

Specifically:

  • Slot arithmetic mixes saturating_add() and checked_add() for identical operations across files
  • Timer arithmetic inconsistently uses saturating_ vs checked_ operations
  • Multiple .unwrap() calls provide no debugging context when panics occur

Summary of Changes

  • Standardize slot arithmetic to consistently use checked_add/checked_sub with overflow detection
  • Standardize timer arithmetic to consistently use checked_ operations
  • Replace .unwrap() calls with descriptive .expect() messages for better debugging
  • Preserve saturating_ operations for stats/stake calculations where overflow clamping is desired

Files modified:

  • certificate_pool.rs: 3 slot arithmetic standardizations
  • certificate_pool/parent_ready_tracker.rs: 5 slot arithmetic standardizations
  • certificate_pool_service.rs: 1 slot arithmetic + error message improvement
  • event_handler.rs: 1 slot arithmetic standardization
  • skip_timer.rs: 6 timer arithmetic standardizations + error messages

Rationale: Slot and timer overflow indicates consensus logic bugs. Better to crash explicitly with clear error messages than silently corrupt calculations.

Fixes #287

jstarry and others added 30 commits March 12, 2025 20:27
PohService needs to set `use_alpenglow_tick_producer` flag on startup  (anza-xyz#59)
Co-authored-by: yihau <[email protected]>
Co-authored-by: carllin <[email protected]>
Co-authored-by: Ashwin Sekar <[email protected]>
…all fashion. (anza-xyz#69)

Co-authored-by: yihau <[email protected]>
Co-authored-by: carllin <[email protected]>
Co-authored-by: Ashwin Sekar <[email protected]>
Copy link
Contributor

@ksn6 ksn6 left a comment

Choose a reason for hiding this comment

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

Really appreciate the contribution here! One minor fix, and we're all set :)

@ksn6
Copy link
Contributor

ksn6 commented Jul 30, 2025

@0xahzam - once merge conflicts are resolved, this should be good to land in master.

@0xahzam
Copy link
Author

0xahzam commented Jul 31, 2025

@ksn6 perfect! thanks, should I try to resolve the conflict or someone else will pick it up?

@ksn6
Copy link
Contributor

ksn6 commented Aug 1, 2025

@0xahzam - feel free to take a look at resolving the conflict; these should hopefully be pretty straightforward - don't hesitate to reach out if you run into any issues!

@0xahzam 0xahzam force-pushed the ahzam/fix-arithmetic-operations branch 2 times, most recently from fb6141b to 8995797 Compare August 2, 2025 17:53
@0xahzam
Copy link
Author

0xahzam commented Aug 2, 2025

@ksn6 I think it should be fine now

@waldrobi
Copy link

waldrobi commented Sep 8, 2025

Is this PR still needed?
Can we close if not

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.

Check arithmetic operations and handle errors appropriately