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

Add average uptime calculation #97

Merged
merged 8 commits into from
Nov 9, 2024
Merged

Add average uptime calculation #97

merged 8 commits into from
Nov 9, 2024

Conversation

thaarok
Copy link
Collaborator

@thaarok thaarok commented Nov 7, 2024

  • Updated and refactored version of Add average uptime for validators #83
  • Use Q1.30 number representation for the normalised uptime (fits into uint32)
  • Used math lemma for the average
  • Used unsigned integers only to simplify checks/verification
  • Disabled too inactive validators included but disabled by default. (I would prefer to include it into the audit.)

@thaarok thaarok requested review from b-scholz and jmpike November 7, 2024 14:52
b-scholz
b-scholz previously approved these changes Nov 8, 2024
// Is also the minimum number of epochs necessary for deactivation of offline validators.
uint32 public averageUptimeEpochsThreshold;

// Minimum average uptime in Q1.30 format; acceptable bounds [0,0.9]
Copy link

Choose a reason for hiding this comment

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

Changing from int32 to uint32 means we have UQ1.31 (and not Q1.30). Perhaps, you want to replace any occurrence in the comments/source code from Q1.30 to UQ1.31. Also, the constant 1 << 30 needs to change to 1 << 31. Ideally, you don't want to have the number 1 << 31 in the source code - perhaps you can define a constant with the value 1 << 31.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually we have already Decimal.unit() in the SFC for this purpose.
Reworked to use Decimal.unit() instead.


function updateAverageUptimeEpochsThreshold(uint32 v) external virtual onlyOwner {
if (v < 10) {
revert ValueTooSmall();
Copy link

Choose a reason for hiding this comment

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

In the comment, you may want to mention that an epoch lasts approximately 6 minutes, and the duration 6-minute * threshold must exceed the permissible downtime with a safety margin so that validators going through a maintenance cycle are not automatically culled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we should fix and mention the epoch duration in SFC source, as it depends on the network configuration. We may use very different epochs configuration in the future.

Copy link

Choose a reason for hiding this comment

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

ok.

contracts/sfc/ConstantsManager.sol Outdated Show resolved Hide resolved
contracts/sfc/SFC.sol Outdated Show resolved Hide resolved
contracts/sfc/ConstantsManager.sol Outdated Show resolved Hide resolved
contracts/sfc/SFC.sol Outdated Show resolved Hide resolved
contracts/sfc/SFC.sol Outdated Show resolved Hide resolved
contracts/sfc/SFC.sol Outdated Show resolved Hide resolved
contracts/sfc/SFC.sol Outdated Show resolved Hide resolved
contracts/sfc/SFC.sol Outdated Show resolved Hide resolved
@thaarok
Copy link
Collaborator Author

thaarok commented Nov 8, 2024

Rewritten to use Unit.decimal() (1e18) instead of (1<<30) to keep fixed-point representation consistent across usages in the SFC.
Edit: keeping the remainder to ensure stability as requested by @b-scholz .

b-scholz
b-scholz previously approved these changes Nov 8, 2024
contracts/sfc/SFC.sol Outdated Show resolved Hide resolved

function updateAverageUptimeEpochsThreshold(uint32 v) external virtual onlyOwner {
if (v < 10) {
revert ValueTooSmall();
Copy link

Choose a reason for hiding this comment

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

ok.

Copy link
Collaborator

@jmpike jmpike left a comment

Choose a reason for hiding this comment

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

LGTM
Thank you.

@thaarok thaarok merged commit e482950 into main Nov 9, 2024
2 checks passed
@thaarok thaarok deleted the jk/average-uptime branch November 9, 2024 07:16
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.

3 participants