Skip to content

Prototype the validator rewards feature.#1162

Closed
raulk wants to merge 1 commit intomainfrom
raulk/prototype-rewards
Closed

Prototype the validator rewards feature.#1162
raulk wants to merge 1 commit intomainfrom
raulk/prototype-rewards

Conversation

@raulk
Copy link
Copy Markdown
Contributor

@raulk raulk commented Oct 2, 2024

This PR prototypes the Validator Rewards feature (#1079). I tried to keep it as simple as possible for now. I've authored the data structures and inserted TODO(rewards) comments everywhere I think we need to extend the implementation, but there may be other spots that I haven't covered.

IValidatorRewarder

This is the interface that should be implemented to disburse validator rewards for a subnet on its parent.

Summaries and commitments

The ActivitySummary is the central object that is created by the subnet and committed in the checkpoint in a hash. The full payload is emitted as an event on the local chain so that the relayer can pick it up and submit it to the root network once the commitment has fully travelled upwards.

Subnet Actor storage

There are new fields added to specify the address of a ValidatorRewarder, and to track commitments to summaries that are pending forwarding and presentation.

Relayer

The relayer should be augmented to watch multiple subnets at once. It will also track summaries that are emitted locally and will present them in the root chain once the relevant commitment has travelled all the way up.

Fendermint

In maybe_create_checkpoint we should craft the ValidatorSummary object by querying the chain in CometBFT. That said, this is not the ideal solution as it likely adds latency. Instead, we should keep a cache (queue) that's updated when new blocks are committed, and pruned when new checkpoints are generated. However, that might be more complex, so maybe we start simple and only implement the cache if we observe performance issues?

@raulk raulk changed the title Rrototype the validator rewards feature. Prototype the validator rewards feature. Oct 2, 2024
@cryptoAtwill
Copy link
Copy Markdown
Contributor

cryptoAtwill commented Oct 3, 2024

@raulk Regarding fendermint construct ActivitySummary, I think querying the chain might be too slow, imaging 600 bottom up checkpoint period. Instead, maybe we can create a solidity contract to track this, end_block hook. An fvm actor will also do but there might be more type conversion here and there. Solidity contract could be more convenient.

@cryptoAtwill
Copy link
Copy Markdown
Contributor

@raulk So in summary, the high level flow diagram is:

  • Fendermint mines a new block
  • The validator that proposed this is recorded
  • When a bottom up checkpoint is created, create ActivitySummary as well and attach it to bottom up checkpoint.
  • Quorum formation in fendermint, emit Activity event
  • Relayer push the rewards distribution with ActivitySummary

A few questions I have:

  1. Actually why not ask validators to claim? They can always query fendermint to obtain ActivitySummary and claim their rewards in batch. So relayer does not really have to worry about gas. Compared to the validator rewards, the gas imposed on validators should be quite minimal.
  2. RelayedActivity seems not clear how it's used. Currently validators are coming from parent, so as for the use case now, it really does not seems to be used. Maybe I'm missing sth.

/// @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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this will cause the what will it cause? And what criteria makes a summary invalid?

/// @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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just curious why 2 arrays instead of a mapping?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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

address[] validators;
/// @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;
Copy link
Copy Markdown
Contributor

@JulissaDantes JulissaDantes Oct 4, 2024

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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:

  • Validator has claimed before?
  • Does the claim actually exists?
  • The lifecycle of the validator reward

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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:

  1. We add a region in checkpoints to carry extensible subnet activity reports in the form of { namespace/type/discriminator: bytes, payload: bytes }[] -- we can call this checkpoint extensions.
  2. IPC ships with a set of default subnet activity reporters as extensions, e.g. the block production one here.
  3. Users can plug in custom subnet activity reporters to, e.g. report on quality of service, retrieval rate, scoring, etc.
  4. Ideally the reporters are Wasm actors, so they can compose easily and be upgraded through governance in the future. However, to truly make this design client/node-agnostic, users would likely need to add custom syscalls to access the metadata actors will want to report on.
  5. We'd need a master actor to orchestrate "hooks". The extend_checkpoint hook would be called by the IPC node, injecting the raw Checkpoint and expecting an array of extension fragments in return. The client would assemble them into the Checkpoint and submit that on chain for local quorum signature.

@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:

  1. We don't bother with developing the checkpoint extensions as Wasm-land actors for now; we just add well-bounded hook callbacks in Fendermint with the intention to migrate them to actors in the future.
  2. We have the node orchestrate the hooks instead of a master actor.

Open points:

  1. Namespacing.
  2. Parent execution.

@raulk
Copy link
Copy Markdown
Contributor Author

raulk commented Oct 22, 2024

Integrated into #1181.

@raulk raulk closed this Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants