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

[FEATURE REQUEST] RPC functions for better staking pool support #176

Closed
redmaner opened this issue Jul 29, 2020 · 11 comments
Closed

[FEATURE REQUEST] RPC functions for better staking pool support #176

redmaner opened this issue Jul 29, 2020 · 11 comments
Assignees
Labels
enhancement New feature or request

Comments

@redmaner
Copy link
Contributor

redmaner commented Jul 29, 2020

Hi team Nimiq,

During the Albatross devnet we explored the possibilities to start working on a staking pool. Although most of the groundwork has already been laid out to support pool staking, we miss a few features to really make a resilient high available staking pool possible. This is a feature request for these missing features. We invite you to consider adding these features in the future. The features below will help all staking pools in the future, not just ours.

Feature 1: RPC function for rewards

The current devnet version of Albatross doesn't support functions or mechanics to reliably determine the reward of a validator for a given block or epoch. Considering that rewards are inherents, and not regular transactions it's cumbersome to calculate the rewards ourselfs in a reliable way.

Now that the new reward has been merged with pull request #166 we request to expose an RPC function that allows the retrieval of rewards for validators in a given epoch. We have come up with two different solutions, both would suffice. Other solutions are possible too.

Solution 1

listRewardsByEpoch(epochNumber int)

This would return a list of the validators for the given epoch, including their public key, their reward address and the reward each validator received for the given epoch.

Solution 2
A different solution would be to list the reward / validator for a specific block

getRewardByBlock(blockNumber int) 

This would return the validator for a block, including his public key, his reward address and the reward he has received for the given block.

UPDATE: 30 march 2021 --> Solution 3 (preferred solution)
Considering the inclusion of checkpoint blocks and batches in the current state of Albatross, the previously proposed solution 1 no longer makes sense. In that regard a solution 3 would suffice for the same reasons discussed before:

getBlockRewardForBatch(batchNumber int)

Returns block reward for given batch. This would probably expose the following function: block_reward_for_batch. On basis of the current state of Albatross I think this is now the preferred solution, although solution 2 might also still suffice.

Solution 4
Another solution could be:

getRewardsForBatch(batchNumber int)

Which returns a list of validators that validated blocks during the given batch, combined with their total reward for the batch and their reward address.

Feature 2: listStakes for a specific epoch

The devnet version of Albatross contains an RPC function to list the stakers / validators for the next epoch. We would like an RPC function that returns the same data for a specific epoch:

listStakesByEpoch(epochNumber int)

Why are we requesting functions taking specific epochs or blocks?

You might be wondering why we specifically ask for functions that take specific epochs / blocks, and not functions that return the data for the latest block or epoch. The reason for this is that we want to be independent of the consensus. The current listStakes function on the devnet is a good example. It returns the stakers for the next epoch. This can be problematic in case the pool goes offline for whatever reason, but the node itself keeps running. In this case the pool is missing viable information that can only be retrieved again by resyncing the consensus. This is fine on a devnet, but in three years when we are in production resyncing the entire consensus will be a costly operation. By having functions that can retrieve data for specific blocks or epochs we can continue where we left off in case of planned or unplanned outtage.

Thanks in advance for your consideration.

Best regards,
@maestroi / @redmaner

@paberr
Copy link
Member

paberr commented Jul 30, 2020

Hi,

first of all, thank you for the detailed explanation!
The first feature seems to be quite straightforward to implement and we'll add it onto our list of TODOs.
The second one, I need a bit more information for.

Feature 1: RPC function for rewards

There already should be everything in place to be able to implement this feature.

To calculate the total reward for an epoch, we need the transactions and timestamps of the epoch's macro block and the preceding epoch's macro block. Both should be easily retrievable from the blockchain (even with the current version of macro sync that we're planning).
To calculate the individual rewards, we additionally need the validators of the epoch and the slash set, which are also stored within the macro blocks.

Feature 2: RPC function for stakes

Okay, here I will need some more information what exactly you plan to retrieve. For now, I assume you want something similar to the listStakes we currently have.
Unfortunately, this is information that can change per block. When you say you would like to get this information for a specific epoch, do you mean the snapshot that is used to select the validators for this epoch (i.e. the state before the previous macro block)?

In general, this kind of information can only be computed by temporarily reverting the transactions that affect the staking contract. This is quite some heavy operation. A naive implementation could just open a database transaction and revert all blocks before retrieving the staking contract and aborting the database transaction. This approach would currently fail as we disallow to revert macro blocks. A more efficient implementation would probably only revert what affects the staking contract, which, however, is more difficult to implement.

Or do you actually mean you want to be able to retrieve the validators of the epoch and their relative stake (i.e., the voting power)?
This is actually part of the macro blocks and thus quite easy to compute.

Perhaps, if you tell us what specific information in the stakes you are looking for, we could find a more efficient solution though. :)

@redmaner
Copy link
Contributor Author

redmaner commented Jul 30, 2020

Hi, thanks for your reply! Nice to hear feature 1 is clear, and can be worked on in the future. I tried to elabore further on feature 2 below.

Feature 2

Allow me to elaborate further on feature 2. The idea of a pool is to stake in the name of other wallets. Other wallets delegate their stake of a certain power to the pool. As a pool I need to know if I am a validator during a certain epoch, and if that is the case which wallets have delegated their stake to me and with which power. This information is then used by the pool to pay out all the wallets that have delegated their stake to the pool.

Currently (on devnet) the listStakes() RPC call returns a list of all active and inactive validators and a list of inactive stakes. All active validatators contain a map of stakes, which holds the addresses of all wallets that have delegated their stake to the validator and their respective power in Luna. This is the information that we require, which is not in the macro block itself (which kinda makes sense). For now we can only retrieve this information for the next epoch. What would really help us, is if it would be possible to retrieve this information for past epochs or blocks.

Hopefully the above gives a better understanding of what we are searching for.

@paberr
Copy link
Member

paberr commented Aug 1, 2020

Ok thanks, that makes it clearer.
Then, what I wrote above applies and retrieving the general list of stakes at a specified point in time is very expensive from a computational point of view.

In my opinion, there are two ways to go from here that make possible what you are looking for:

  1. Move this functionality to the outside (i.e., into the pool implementation) and store the list of stakes for your pool as you go. What I mean by that is that we could expose events that notify you when, e.g., a macro block happened. Then, you can retrieve the list of stakes for that macro block and store it in your database. (Potentially, the event itself could actually already include the list of stakes, but we would have to see what makes sense; also see below.)

  2. We provide a RPC function (which is still taking up some computation) that retrieves the stakes by reverting only transactions affecting the staking contract. Such a function should not be called regularly but only if actually needed.
    Also, implementing this would certainly take some time.

Overall, I think option 1 is perhaps the better way to go as I am a bit at unease with exposing computational heavy functions via RPC. :) It is also the easier way to implement it and allows you to retrieve the information for the past very efficiently.
I'm thinking of some way to subscribe to stake changes for a particular validator public key for example. So, you would get all of the changes while they occur and can keep your own history in a database.
This should be possible via a websocket RPC.

I'd be interested in whether option would 1 work for you. Or do you foresee issues with that?

@redmaner
Copy link
Contributor Author

redmaner commented Aug 3, 2020

Hi, thanks for your reply and thinking with me. I agree with your described solutions. Currently we have implemented solution 1, which works fine. We requested feature 2 mostly as a nice to have feature. It is especially useful in case something goes wrong on the pool side, and staking contract information is required from a specific timeframe. I do agree with your statement that exposing a heavy computational function via RPC is undesirable from multiple points of view, especially from a security and (network) stability point of view. So let's not do that.

As long as we can still have feature 1, where we can obtain the amount of rewards received for validating a block / epoch we should be fine.

Thanks again for your consideration. I'll edit the request to only include feature 1.

@paberr paberr added the enhancement New feature or request label Aug 5, 2020
@jgraef jgraef self-assigned this Sep 16, 2020
@Eligioo
Copy link
Member

Eligioo commented Jan 15, 2021

Hi all, first of all great discussion and proposal! As terminology has changed and Checkpoints/Election blocks were introduced in #183, I think listRewardsByEpoch(epochNumber int) isn't flexible enough anymore. That's why I want to propose to rename listRewardsByEpoch to listRewardsByBatch for feature 1. Since batches will be between two consecutive macro blocks (checkpoint or election), e.g. pools will be able to process payouts on a higher frequency since they don't have to wait for an epoch to end (as an epoch will take around 6-12 h).

@redmaner
Copy link
Contributor Author

redmaner commented Jan 18, 2021

Hi all, first of all great discussion and proposal! As terminology has changed and Checkpoints/Election blocks were introduced in #183, I think listRewardsByEpoch(epochNumber int) isn't flexible enough anymore. That's why I want to propose to rename listRewardsByEpoch to listRewardsByBatch for feature 1. Since batches will be between two consecutive macro blocks (checkpoint or election), e.g. pools will be able to process payouts on a higher frequency since they don't have to wait for an epoch to end (as an epoch will take around 6-12 h).

Good point. However, my preference has actually changed to solution 2: getRewardByBlock(blockNumber int) returning the reward for the given block, the validator's public key and the validator's reward address. With the new CheckpointBlocks/ElectionBlocks I think getting the reward by block gives the most flexibility. You can create every batch you want, leaving it up to the client (being a pool, or anything else) to retrieve the information he needs at his preferred batch / interval.

@redmaner
Copy link
Contributor Author

redmaner commented Mar 30, 2021

@jgraef @Eligioo I've updated the issue with a solution 3, getBlockRewardForBatch() and a solution 4 getRewardsForBatch() based on the current state of Albatross. As already mentioned by @Eligioo solution 1 no longer makes sense.

Maybe this could also make the list for #229 so spamming @sisou as well ;)

@Eligioo
Copy link
Member

Eligioo commented Mar 30, 2021

If getRewardsForBatch(batchNumber int) would be to intensive because you can potentially have a lot of validators, an option could be something like getBatchReward(batch, validator_id). In the context of pools, for solution 3 the number of minted blocks needs to be tracked.

@sisou
Copy link
Member

sisou commented Mar 30, 2021

There is technically no longer a per-block reward in Albatross. Batch rewards are calculated based on time passed since the last batch, and the rewards are according to the emission curve formula.

Furthermore, the rewards paid out to each validator in a macro block are already stored in the history_store as inherents. They "just" need to be indexed by batch-number or macro-block-number and made gettable.

@redmaner
Copy link
Contributor Author

redmaner commented Mar 30, 2021

Furthermore, the rewards paid out to each validator in a macro block are already stored in the history_store as inherents. They "just" need to be indexed by batch-number or macro-block-number and made gettable.

If they are gettable by either batch_number or macro_block_number that would be perfect.

@sisou
Copy link
Member

sisou commented Apr 14, 2021

RPC method getTransactionsByBlockNumber was added in ecc9e98 and allows getting reward inherents for macro blocks. Additionally, getBatchInherents was added in 127e4b5 that allows to also retrieve slash inherents during a batch.

Both of these methods are also already available on the lab-test branch.

Please note that there is work underway to convert reward inherents into transactions that would be included in the macro block, so the output format of these methods will likely change in the future: de5a958.

@sisou sisou closed this as completed Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants