-
Notifications
You must be signed in to change notification settings - Fork 39
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
feat: subnet activity rollups and validator rewards (phase 1) #1181
Conversation
Co-authored-by: Eric Tu <[email protected]> Co-authored-by: Mikers <[email protected]> Co-authored-by: raulk <[email protected]> Co-authored-by: Shashank Trivedi <[email protected]>
…pc into simple-genesis
Co-authored-by: Karel Moravec <[email protected]>
Co-authored-by: Karel Moravec <[email protected]>
…pc into simple-genesis
Co-authored-by: raulk <[email protected]>
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.
Some initial comments.
Ok(()) | ||
} | ||
|
||
pub fn purge_validator_block_committed(&mut self, rt: &impl Runtime) -> Result<(), ActorError> { |
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.
Any reason we don't just store an empty HAMT? It's cheaper than iterating over all entries and deleting each.
let v = if let Some(v) = validators.get(validator)? { | ||
*v + 1 | ||
} else { | ||
1 | ||
}; |
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.
Since this returns an option, it's more idiomatic to just use unwrap_or to set the initial value.
use serde::{Deserialize, Serialize}; | ||
|
||
pub type BlockCommittedMap<BS> = Map2<BS, Address, BlockCommitted>; | ||
pub type BlockCommitted = u64; |
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 type alias is kinda redundant IMO.
Full review developing here: #1205. |
Co-authored-by: cryptoAtwill <[email protected]> Co-authored-by: cryptoAtwill <[email protected]>
// TODO(rewards): enable the relayer to watch multiple subnets at once. | ||
|
||
// TODO(rewards): add a new flag --process-summaries to activate processing summaries on all subnets. | ||
// Enabling this mode makes the relayer watch for ActivitySummaryCommitted events, and stores the summaries in a database. | ||
// It then tracks which summaries have been committed to the root (right now we only support submitting to the L1), to chase | ||
// after those and present them via SubnetActor#submitSummary in order to trigger reward payout. | ||
|
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.
Let's open issues to add these TODOs to the backlog.
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.
Added issue: #1215, remove the todo.
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.
Stale/leaked from previous changes.
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.
Stale/leaked from previous changes.
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.
Stale/leaked from previous changes.
// TODO(rewarder): step 1. call fvm ActivityTrackerActor::get_summary to generate the summary | ||
// TODO(rewarder): step 2. update checkpoint.activities with that in step 1 | ||
// TODO: (if there is more time, should wrap param checkpoint with another data structure) | ||
// TODO(rewarder): step 3. call fvm ActivityTrackerActor::purge_activities to purge the activities | ||
// TODO(rewarder): step 4. emit validator details as event |
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.
@cryptoAtwill is this still relevant?
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 is for using fvm actor to derive commitment, we have pushed this to later phases, not relevant now, I can create an issue to track this
return compressed; | ||
} | ||
|
||
function newCompressedSummary( |
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 is only used above and once elsewhere. I don't think it's worth having it.
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.
ok, I removed it.
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 file has leaked. This is already covered in gas_market
.
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.
The ID of this actor cannot be 99, as that conflicts with the burnt funds actor.
impl<'a, DB: Blockstore + Clone + 'static> ActorActivityTracker<'a, DB> { | ||
fn apply_implicit_message(&mut self, msg: FvmMessage) -> anyhow::Result<ApplyRet> { | ||
let (apply_ret, _) = self.executor.execute_implicit(msg)?; | ||
if let Some(err) = apply_ret.failure_info { | ||
anyhow::bail!("failed to apply activity tracker messages: {}", err) | ||
} else { | ||
Ok(apply_ret) | ||
} | ||
} | ||
} |
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 is duplicated code, no? Do we actually need it? It seems it's only used once and kinda superfluous.
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 method is used repeatedly in gas market and activity actor, I think it's worth making this a util function in FvmExecState
, a function that executes the message and makes sure the execution is ok.
impl MerkleProofGen { | ||
pub fn pack_validator(v: &ValidatorData) -> Vec<String> { | ||
vec![format!("{:?}", v.validator), v.blocks_committed.to_string()] | ||
} | ||
|
||
pub fn root(&self) -> Hash { | ||
self.tree.root() | ||
} | ||
|
||
pub fn new(values: &[ValidatorData]) -> anyhow::Result<Self> { | ||
let values = values.iter().map(Self::pack_validator).collect::<Vec<_>>(); | ||
|
||
let tree = StandardMerkleTree::of(&values, &VALIDATOR_SUMMARY_FIELDS) | ||
.context("failed to construct Merkle tree")?; | ||
Ok(MerkleProofGen { tree }) | ||
} | ||
} |
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.
Code like this already exists in ipc/provider/.../evm/manager.rs
. Looks like we can refactor into an Activity helper of some kind?
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 created a MerkleGen
util struct that can be used repeatedly, a simple wrapper.
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 is good enough to merge now. We'll land the docs + ERC20 minter ValidatorRewarder in separate PRs, before making a release.
Integrates these stacked PRs:
Subnet activity rollups (Phase 1)
This PR introduces the notion of subnet activity rollups that piggyback on bottom-up checkpoints. This is a mechanism to synthesise and bubble up information to the parent about some activity that occurred inside a subnet. The parent can choose to either relay this information up the hierarchy, or consume the incoming rollup for some effect (e.g. rewarding validators by minting a token, rebalancing the validator set based on performance metrics).
Consensus summary
Activity rollups contain subject-scoped summaries. These can be protocol-defined, or in user-defined (in the future). The protocol-defined summary we add is the consensus summary. This summary collects datapoints from the consensus layer that are relevant to export/surface to ancestors. Namely, the first attribute we care about is the number of blocks committed per validator. This will predictably be used by subnet creators to reward validators at the parent through, e.g. token inflation.
Compressed vs. full rollups and summaries
To avoid saturating checkpoints with activity data, we publish full rollups locally within the subnet as events only. In checkpoints, rollups travel in a compressed form, usually by creating commitments to data rather than embedding the data itself. This limits the footprint and spares space for xnet messages.
In the case of the consensus summary, checkpoints only carry short aggregated stats, and the root of a Merkle tree of the full data as a commitment.
Processing activity rollups and ValidatorRewarder
When an activity rollup arrives to the parent within a checkpoint, the SubnetActor uses the LibActivity library to process it. Currently, we extract the consensus summary and store it so that validators can come later and claim rewards by presenting a claim, which consists of the data leaf and a Merkle proof of inclusion in the root commitiment. The library tracks in state which validators have claimed per subnet and checkpoint height to prevent duplicate claims.
LibActivity is responsible for validating claims, but not for disbursing rewards. For this, it calls the configured user-defined ValidatorRewarder on its
notifyClaimValid
function, passing in the validated data leaf.We provide an example
ValidatorRewarderMap
implementation that merely tallies the number of blocks committed per validator.Activity actor and Merkle tree commitments
The new
activity-tracker
built-in actor acts as an accumulator of subnet activity. It is called atABCI::end_block
time to report the validator that committed the block. When a checkpoint is emitted, we pull and reset the accumulated data out of the actor, populate a Merkle tree with the entries, and we set its root hash in the consensus summary within the activity rollup.We emit the full activity rollup as a local event in the subnet, for stakeholders (e.g. validators) to monitor and present any relevant payloads up in the hierarchy to execute the claim.
Secured by subnet consensus
Activity rollups present the same degree of security as all other vertical IPC interactions. This is because checkpoints are (a) published in the subnet during state transition and therefore secured by consensus, and (b) checkpoints are signed by a quorum of validators containing activity rollups are signed by a subnet quorum.
Claiming interfaces
The subnet actor is augmented with methods to perform single and batch claims. The validator (or another party acting on its behalf) must submit the checkpoint height, the relevant data leaf, and the merkle inclusion proof.
The subnet actor will verify the merkle inclusion proof for the pending checkpoint height, and call the ValidatorRewarder if it succeeds, marking the fragment as claimed in its state, so it cannot be double claimed.
A validator can claim rewards from multiple checkpoints, across multiple subnets rooted at that subnet actor, in a single call. Thus, it is rational for the validator to wait until enough rewards have been accumulated to offset and amortize the gas costs of the claim process.
Relayer enhancements
The relayer is enhanced with a new operating mode and task with the job of claiming rewards on behalf of a validator. Validators can use this new mode to watch a subnet for validator activity matching address, and to subsequently submit any relevant claims to the appropriate parent network (normally the root) to withdraw their corresponding validator rewards.
Unimplemented (phase 2)
Activity summary routing / relaying
At every level, subnets can decide whether they consume activity summaries from descendants, or they relay them up the chain. Usually, deeper subnets will want to relay these all the way up to the root, to provide a single entrypoint for validators operating in a subtree hierarchy to claim a canonical ERC20 token seated at the rootnet.