-
Notifications
You must be signed in to change notification settings - Fork 47
Prototype the validator rewards feature. #1162
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| // SPDX-License-Identifier: MIT OR Apache-2.0 | ||
| pragma solidity ^0.8.23; | ||
|
|
||
| import {SubnetID} from "../structs/Subnet.sol"; | ||
|
|
||
| /// @title ValidatorRewarder interface. | ||
| /// | ||
| /// @dev Implement this interface and supply the address of the implementation contract at subnet creation to process | ||
| /// subnet summaries at this level, and disburse rewards to validators based on their block production activity. | ||
| /// | ||
| /// This interface will be called by the subnet actor when a relayer presents a | ||
| interface IValidatorRewarder { | ||
| /// @notice Called by the subnet manager contract to instruct the rewarder to process the subnet summary and | ||
| /// disburse any relevant rewards. | ||
| /// The | ||
| /// @dev This method should revert if the summary is invalid; this will cause the | ||
| function disburseRewards(SubnetID memory id, ActivitySummary memory summary) external; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,31 @@ struct BottomUpCheckpoint { | |
| uint64 nextConfigurationNumber; | ||
| /// @dev Batch of messages to execute. | ||
| IpcEnvelope[] msgs; | ||
| /// @dev A commitment to the summary of our chain activity since the previous checkpoint and this one. | ||
| bytes32 summary; | ||
| /// @dev Summaries relayed upwards from descendants of this subnet. | ||
| /// NOTE: Not merkelized to keep it simple, but we will merkelize later to scale better. | ||
| RelayedSummary[] relayedSummaries; | ||
| } | ||
|
|
||
| struct ActivitySummary { | ||
| /// @dev The block range the activity summary spans; these are the local heights of the start and the end, inclusive. | ||
| uint256[2] blockRange; | ||
| /// @dev The validators whose activity we're reporting about. | ||
| address[] validators; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious why 2 arrays instead of a mapping?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand the arrays if when distributing the rewards we have to process them all but I agree with @cryptoAtwill, validators could claim their own rewards, so a mapping would make sense.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a DTO, so mapping cannot, but I have updated the data struct slightly for storage |
||
| /// @dev The number of blocks committed by each validator in the position they appear in the validators array. | ||
| /// If there is a configuration change applied at this checkpoint, this carries information about the _old_ validator set. | ||
| uint64[] blocksCommitted; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we have a metadata bytes typed member here to allow to pass additional info? or are there other alternatives for customization that are still in the making?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice suggestion @JulissaDantes. @raulk, in particular, we have additional information we'd like to propagate up the hierarchy. I know we had talked about specific "zones" for this type of information in the checkpoints at one point. What is your thinking here at this stage?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At least for solidity, there is no generic, then perhaps only bytes array? To let the subnet/fendermint and parent IValidatorRewarder handle the bytes deserialization + distribution. What the framework does is just checking:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is the direction I wanted to move things in. This was the idea:
@carsonfarmer @JulissaDantes In my head we could defer this feature, but it sounds like it would be beneficial to you guys to have it now, right? If so, I think we can simplify the above on several fronts to try to meet the time constraints while delivering the feature. How about:
Open points:
|
||
| } | ||
|
|
||
| event ActivitySummaryCommitted(bytes32 indexed commitment, ActivitySummary summary); | ||
|
|
||
| struct RelayedSummary { | ||
| /// @dev The subnet IDs whose activity is being relayed. | ||
| SubnetID subnet; | ||
| /// @dev The commitment to the summary, so it can be presented later by the relayer. | ||
| /// A blake2b hash of the summary generated by abi.encode'ing the ActivitySummary and hashing it via the Eth precompile. | ||
| bytes32 commitment; | ||
| } | ||
|
|
||
| /// @notice A batch of bottom-up messages for execution. | ||
|
|
||
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.
this will cause thewhat will it cause? And what criteria makes a summary invalid?