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

Staking: Adding dest field to Rewarded event to allow easier / historical reward tracking #129

Closed
rossbulat opened this issue Aug 20, 2023 · 7 comments
Labels
I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@rossbulat
Copy link

rossbulat commented Aug 20, 2023

Teams have began exploring a better mechanism of estimating staking rewards, which inevitably involves indexing chain state and collecting a historical amount of data.

It is becoming apparent that this is expensive and cumbersome to do with the current block data being emitted. Notably, it is very non-trivial to track to what address(es) staking rewards are actually sent to: the final destination, rather than the stash account subject to the reward.

To further complicate this tracking, the upcoming Split payout destination will allow rewards to go to potentially multiple addresses, one part to compound bond, the other part as free balance.

One potential way we can streamline these issues is to have a destination field in the Rewarded event, emitted when payouts are made to stakers (validators & nominators). Another suggestion has been just to include the final payout destination address, but again, the Split variant complicates this as there will most likely 2 distinct addresses in most cases. Including the RewardDestination (or upcoming PayoutDestination) in the event would give clients tracking the event enough information to successfully track exactly where staking rewards are going to, in real-time, as the event is emitted.

Beyond an additional space requirement for the additional field, I can not foresee any unwanted impacts by adding this field. It would aid in tracking reward data, and ultimately lead to better means of estimating APY (which we very much need in place of the rough estimations we are using).

@rossbulat rossbulat changed the title Staking Adding destination field to Rewarded event to allow easier reward tracking Staking: Adding destination field to Rewarded event to allow easier reward tracking Aug 20, 2023
@Ank4n
Copy link
Contributor

Ank4n commented Aug 22, 2023

To add to this, the target account can be inferred today by reading the Payee storage item for each Rewarded event (which is a lot of RPC calls).

I am in support of adding explicitly which account has been rewarded, seems like a important information missing from the event. Though I should mention that payout_stakers is already a heavy operation and this would make it more expensive by (cost of adding a field * 512). Hopefully, once we have paged rewards, we could reduce the number of payouts in a single call.

@rossbulat
Copy link
Author

rossbulat commented Aug 22, 2023

Indeed, I feel a lot is hanging on the paged rewards PR, for this and for the split payout destination. I have no strong opinion about whether to hold off until paged payouts are merged or not, but my intuition is that as long as payout_stakers still leaves a modest amount of space for other block data, we should be good.

@joepetrowski
Copy link
Contributor

which is a lot of RPC calls

We already have tools like Sidecar that aggregate these calls so that users (front ends, custodians, etc.) can make a single query and get all this info. Like you said it's already a heavy on-chain operation, and referencing only the stash makes sense (that is what generated the rewards). Other information can easily be looked up with the stash account as key.

@juangirini juangirini transferred this issue from paritytech/substrate Aug 24, 2023
@the-right-joyce the-right-joyce added I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework. and removed J0-enhancement labels Aug 25, 2023
@rossbulat rossbulat changed the title Staking: Adding destination field to Rewarded event to allow easier reward tracking Staking: Adding destination field to Rewarded event to allow easier / historical reward tracking Aug 31, 2023
@jonasW3F
Copy link

which is a lot of RPC calls

We already have tools like Sidecar that aggregate these calls so that users (front ends, custodians, etc.) can make a single query and get all this info. Like you said it's already a heavy on-chain operation, and referencing only the stash makes sense (that is what generated the rewards). Other information can easily be looked up with the stash account as key.

Integrating this field into the event offers a distinct advantage: it enables any entity with access to the block data to retrieve all staking rewards for any account throughout the duration the data is stored. In contrast, as far as I understand it, the sidecar solution can only retrieve rewards up tohistory_depth. While it might seem practical to set up the sidecar in a monitoring-fashion that constantly collects the recent rewards, the addresses to be watched need to be known. This approach limits the potential real-world setups and falls short when dealing with addresses that haven't been previously identified and rewards fall outside of history_depth.

Adding this field to the event data also make setups feasible where the (block) data provider is not necessarily the same entity as the one doing the queries, because it alleviates the coordination effort on agreeing which addresses to watch. Further, it would allow for processes that protect the anonymity, because the entity scraping the rewards from the blocks would not need to disclose their addresses (after receiving all block data).

This update seems to be very useful for me, but I cannot judge the technical overhead this creates.

@joepetrowski
Copy link
Contributor

joepetrowski commented Aug 31, 2023

as far as I understand it, the sidecar solution can only retrieve rewards up to history_depth

No, this is not how this works at all. You are thinking of pending rewards (also keyed by stash), and that's a limitation of what is stored in state, not what Sidecar can fetch.

Sidecar can look up any state at any block height. You simply see the event that a stash was rewarded at block hash H, and look up the payment destination preferences for stash at H.

The runtime is not for application logic like this, especially when it's so easy to look up this info.

Adding this field to the event data also make setups feasible where the (block) data provider is not necessarily the same entity as the one doing the queries, because it alleviates the coordination effort on agreeing which addresses to watch. Further, it would allow for processes that protect the anonymity, because the entity scraping the rewards from the blocks would not need to disclose their addresses (after receiving all block data).

I don't understand this at all. Everything about staking is linked to a stash address. Of course if you see the payment destination in an event you can link it to the stash.

@AlistairStewart
Copy link

Like you said it's already a heavy on-chain operation, and referencing only the stash makes sense (that is what generated the rewards). Other information can easily be looked up with the stash account as key.

But doesn't the chain already need to lookup the payout address to do the payout? So this should not increase the weight of the payout function. If it did have to read more state, then this would clearly be bad, but it doesn't.

@joepetrowski
Copy link
Contributor

Fair enough RE weight, but it does add the bytes to every event. I still have the opinion that stakers should always be identified by their stash account. Everything can be derived from that.

@rossbulat rossbulat changed the title Staking: Adding destination field to Rewarded event to allow easier / historical reward tracking Staking: Adding dest field to Rewarded event to allow easier / historical reward tracking Sep 18, 2023
rossbulat pushed a commit that referenced this issue Sep 18, 2023
Addresses #129.

Returns `Self:payee()` from `make_payout` in a tuple alongside an
imbalance & adds it to `Rewarded` event.
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this issue Mar 25, 2024
…tytech#1602)

Addresses paritytech#129.

Returns `Self:payee()` from `make_payout` in a tuple alongside an
imbalance & adds it to `Rewarded` event.
lexnv pushed a commit that referenced this issue Apr 3, 2024
fix: `rpc_methods` return value
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 8, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 8, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 10, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 10, 2024
jonathanudd pushed a commit to jonathanudd/polkadot-sdk that referenced this issue Apr 10, 2024
claravanstaden pushed a commit to claravanstaden/polkadot-sdk that referenced this issue Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
Status: Done
Development

No branches or pull requests

7 participants