-
Notifications
You must be signed in to change notification settings - Fork 26
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
Prorated validation rewards #603
base: main
Are you sure you want to change the base?
Conversation
function claimValidationRewards( | ||
bytes32 validationID, | ||
uint32 messageIndex | ||
) external nonReentrant { | ||
PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage(); | ||
|
||
Validator memory validator = getValidator(validationID); | ||
if (validator.status != ValidatorStatus.Active) { | ||
revert InvalidValidatorStatus(validator.status); | ||
} | ||
|
||
// Non-PoS validators are required to boostrap the network, but are not eligible for rewards. | ||
if (!_isPoSValidator(validationID)) { | ||
revert ValidatorNotPoS(validationID); | ||
} | ||
|
||
// Rewards can only be claimed by the validator owner. | ||
if ($._posValidatorInfo[validationID].owner != _msgSender()) { | ||
revert UnauthorizedOwner(_msgSender()); | ||
} | ||
|
||
// Check that minimum stake duration has passed. | ||
uint64 claimTime = uint64(block.timestamp); | ||
if (claimTime < validator.startedAt + $._posValidatorInfo[validationID].minStakeDuration) { | ||
revert MinStakeDurationNotPassed(claimTime); | ||
} | ||
|
||
// The claim's uptime is the difference between the total uptime and the minimum possible uptime from the last claim. | ||
// We use the minimum uptime to get a lower bound on the required uptime for this claim | ||
uint64 totalUptime = _updateUptime(validationID, messageIndex); | ||
uint64 uptime = totalUptime - $._posValidatorInfo[validationID].lastClaimMinUptime; | ||
|
||
// If no rewards have yet been claimed, use the validator's start time | ||
uint64 lastClaimTime = $._posValidatorInfo[validationID].lastClaimTime; | ||
if (lastClaimTime == 0) { | ||
lastClaimTime = validator.startedAt; | ||
} | ||
// Validate the uptime for this claim. Given that all previous claims have been similarly validated, | ||
// this is equivalent to validating the uptime of the entire validation period up to this point, due | ||
// to the linearity of the uptime threshold calculation. | ||
if (!_validateSufficientUptime(uptime, lastClaimTime, claimTime)) { | ||
revert ValidatorIneligibleForRewards(validationID); | ||
} | ||
|
||
uint256 reward = $._rewardCalculator.calculateReward({ | ||
stakeAmount: weightToValue(validator.startingWeight), | ||
validatorStartTime: lastClaimTime, | ||
stakingStartTime: lastClaimTime, | ||
stakingEndTime: claimTime, | ||
uptimeSeconds: uptime, | ||
initialSupply: 0, | ||
endSupply: 0 | ||
}); | ||
|
||
$._posValidatorInfo[validationID].lastClaimMinUptime = | ||
(claimTime - validator.startedAt) * UPTIME_REWARDS_THRESHOLD_PERCENTAGE / 100; | ||
$._posValidatorInfo[validationID].lastClaimTime = claimTime; | ||
_reward($._posValidatorInfo[validationID].owner, reward); | ||
|
||
emit ValidationRewardsClaimed(validationID, reward); | ||
} |
Check notice
Code scanning / Slither
Block timestamp Low
Dangerous comparisons:
- claimTime < validator.startedAt + $._posValidatorInfo[validationID].minStakeDuration
function _validateSufficientUptime( | ||
uint64 uptimeSeconds, | ||
uint64 periodStartTime, | ||
uint64 periodEndTime | ||
) internal pure returns (bool) { | ||
// Equivalent to uptimeSeconds/(periodEndTime - periodStartTime) < UPTIME_REWARDS_THRESHOLD_PERCENTAGE/100 | ||
// Rearranged to prevent integer division truncation. | ||
if ( | ||
uptimeSeconds * 100 | ||
< (periodEndTime - periodStartTime) * UPTIME_REWARDS_THRESHOLD_PERCENTAGE | ||
) { | ||
return false; | ||
} | ||
return true; | ||
} |
Check notice
Code scanning / Slither
Block timestamp Low
Dangerous comparisons:
- uptimeSeconds * 100 < (periodEndTime - periodStartTime) * UPTIME_REWARDS_THRESHOLD_PERCENTAGE
Unfortunately we're bumping against the contract size limit again. A quick experiment consolidating custom errors yielded marginal gains, but changing the Draft PR here: #604 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just left one suggestion for simplification
// Equivalent to uptimeSeconds/(periodEndTime - periodStartTime) < UPTIME_REWARDS_THRESHOLD_PERCENTAGE/100 | ||
// Rearranged to prevent integer division truncation. | ||
if ( | ||
uptimeSeconds * 100 | ||
< (periodEndTime - periodStartTime) * UPTIME_REWARDS_THRESHOLD_PERCENTAGE | ||
) { | ||
return false; | ||
} | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Equivalent to uptimeSeconds/(periodEndTime - periodStartTime) < UPTIME_REWARDS_THRESHOLD_PERCENTAGE/100 | |
// Rearranged to prevent integer division truncation. | |
if ( | |
uptimeSeconds * 100 | |
< (periodEndTime - periodStartTime) * UPTIME_REWARDS_THRESHOLD_PERCENTAGE | |
) { | |
return false; | |
} | |
return true; | |
// Equivalent to uptimeSeconds/(periodEndTime - periodStartTime) >= UPTIME_REWARDS_THRESHOLD_PERCENTAGE/100 | |
// Rearranged to prevent integer division truncation. | |
return uptimeSeconds * 100 | |
>= (periodEndTime - periodStartTime) * UPTIME_REWARDS_THRESHOLD_PERCENTAGE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From an incentive perspective, I wonder if we should introduce a minimum claim period to prevent a validator from claiming their rewards too frequently after the initial minimum stake period. Gas already does this to a certain degree, but if the staked amount is large, it may still be worth it for the validator operator to claim frequently if we are not providing them a compounding rewards curve.
) internal returns (bool) { | ||
uint32 messageIndex, | ||
bool requireSufficientUptime | ||
) internal { | ||
PoSValidatorManagerStorage storage $ = _getPoSValidatorManagerStorage(); | ||
|
||
Validator memory validator = _initializeEndValidation(validationID); | ||
|
||
// Non-PoS validators are required to boostrap the network, but are not eligible for rewards. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Non-PoS validators are required to boostrap the network, but are not eligible for rewards. | |
// Non-PoS validators are required to bootstrap the network, but are not eligible for rewards. |
}); | ||
|
||
$._posValidatorInfo[validationID].lastClaimMinUptime = | ||
(claimTime - validator.startedAt) * UPTIME_REWARDS_THRESHOLD_PERCENTAGE / 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that we want to "consume" from the totalUptime only what is needed to claim the reward. This allows the next claim to be more forgiving. However, it is now possible for claimTime - lastClaimTime to be less than totalUptime - lastClaimMinUptime in the next claim (i.e., an uptime that is longer than the claim period). This is not a problem, but seems like it is breaking an invariant at first glance. It may be worth documenting this.
So I've tried to formalize what I'm about to say, but my brain doesn't quite want to get there, but hopefully I can explain it well enough. We don't need to do the uptime normalization at all. The calculation we're doing here is actually just equivalent to checking if the current uptime percentage over the whole period is greater than I think the following approach would work better: Just track the rewards the staker has claimed so far. Calculate the total rewards for the entire staking period thus far, and then subtract and rewards that have already been claimed. This would have the added benefit of accounting for non-linear reward curves. Obviously we wouldn't want to mix this feature with a "compounding" rewards curve, but the non-linearities could include things like the starting and ending token supply. Let me know if you want me to throw together a quick example of what that would look like |
I love this approach and summary. I'm not sure what formalization you are looking for but the way I'm thinking about it right now is that the rewards are really just a time integral of the reward curve function. Right now the function is just a zero slope curve with a static reward precentage. But this could be replaced with any curve as long as the function itself doesn't change which it doesn't in a non-compounding case. Given that, your observation that unreedemed rewards are equivalent to current eligible rewards minus previously claimed rewards holds true. The only potential formalization here would be to show that uptime percentage check is additive which it is. |
I implemented this approach here. It is indeed much simpler than the equivalent logic in this PR. |
Why this should be merged
Fixes #526
How this works
Adds the function
claimValidationRewards
toPoSValidatorManager
that allows rewards to be claimed from an active validation without ending the validation. There's no interaction with the P-Chain, so the core of this feature is making sure that uptimes are properly accounted for.But how do we calculate the uptime for a floating claim interval when uptimes are provided relative to the start of the overall validation period? When processing a claim, we need to somehow offset the supplied uptime to match the period of time since the last claim. A key point is that
claimValidationRewards
reverts if the supplied uptime is not sufficient for rewards (i.e. is < 80%). After processing a claim, we store the claim time and the smallest sufficient uptime needed for that claim (not the supplied uptime). On the next claim, we offset the supplied uptime by this amount to normalize the uptime to the new interval since the last claim. This normalization to windows within the overall validation period yields the same uptime-based descisions since that calculation is linear. Note that If the rewards curve is also linear, then the rewards from multiple claims will match the rewards from a single claim over the same time period as well.The uptime percentage for a given claim interval$i > 0$ is
where
As an example, for$i=1$ this reduces to $U_1/(t_1 - t_0)$ , which is the uptime percentage for an interval starting from the validation start time.
Finally, this change moves the uptime threshold calculation to$U_{min_i}$ .
PoSValidatorManager
, since it it used to calculateuptimeSeconds
is left in theIRewardsCalculator
interface to allow for rewards curves that wish to scale rewards based on uptime.How this was tested
CI, new unit tests
How is this documented
TODO