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

Prorated validation rewards #603

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Prorated validation rewards #603

wants to merge 15 commits into from

Conversation

cam-schultz
Copy link
Contributor

Why this should be merged

Fixes #526

How this works

Adds the function claimValidationRewards to PoSValidatorManager 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 &gt; 0$ is

$(U_i-U_{min_i})/(t_i - t_{i-1})$

where

$U_{min_i} = α(t_{i-1} - t_{start})$ if $i&gt;1$, and

$U_{min_i} = 0$ if $i &lt;= 1$

  • $U_i$ is the uptime proof supplied for claim interval $i$. This is given in seconds since the start of the validation period, not the claim interval $i$
    • $U_i-U_{min_i}$ normalizes this to claim interval $i$
  • Claim interval $i$ is defined as the range $t_i - t_{i-1}$
  • $α$ is the uptime threshold percentage, which is $0.8$

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 PoSValidatorManager, since it it used to calculate $U_{min_i}$. uptimeSeconds is left in the IRewardsCalculator 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

Comment on lines 294 to 354
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

Comment on lines +356 to +370
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

@cam-schultz
Copy link
Contributor Author

Unfortunately we're bumping against the contract size limit again. A quick experiment consolidating custom errors yielded marginal gains, but changing the ValidatorMessages packing methods to external brought us back under the limit by a fair margin. Separating the large contract into multiple smaller contracts is the most scalable way to workaround the contract size limit, so this is a strategy we will likely need to employ anyway, despite the increased gas needed for delegatecall.

Draft PR here: #604

Copy link
Contributor

@geoff-vball geoff-vball left a 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

Comment on lines +361 to +369
// 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;
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
// 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

Copy link
Contributor

@bernard-avalabs bernard-avalabs left a 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.
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
// 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;
Copy link
Contributor

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.

@geoff-vball
Copy link
Contributor

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 α. You can throw in some numbers to check, but I'm having trouble explaining this any deeper at the moment.

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

@iansuvak
Copy link
Contributor

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 α. You can throw in some numbers to check, but I'm having trouble explaining this any deeper at the moment.

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.

Base automatically changed from validator-manager to main October 29, 2024 18:29
@cam-schultz
Copy link
Contributor Author

I implemented this approach here. It is indeed much simpler than the equivalent logic in this PR.

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

Successfully merging this pull request may close these issues.

Allow pro-rated rewards to be claimed without ending the validation.
4 participants