[pos] Add epoch argument to vp::pos::rewards

[joel-u410]

Describe your changes

This adds an epoch argument to the rewards() function in vp::pos. It does require a RPC node update in order to work, but it's backwards compatible. Tested with previous build of namadan and results are same as before:

With node on main and client on this branch:

$ ./target/debug/namadac rewards --source tnam1qrtypl0yazfamk8ptmgp608msl8wzrkpmqwppc7w --validator tnam1qxsx2ezu89gx252kwwluqp7hadyp285tkczhaqg0
Current rewards available for claim: 4625.955882 NAM

With both client and node on this branch:

$ ./target/debug/namadac rewards --source tnam1qrtypl0yazfamk8ptmgp608msl8wzrkpmqwppc7w --validator tnam1qxsx2ezu89gx252kwwluqp7hadyp285tkczhaqg0
Current rewards available for claim: 4625.955882 NAM

$ ./target/debug/namadac rewards --source tnam1qrtypl0yazfamk8ptmgp608msl8wzrkpmqwppc7w --validator tnam1qxsx2ezu89gx252kwwluqp7hadyp285tkczhaqg0 --epoch 60
Rewards for epoch 60: 4625.955882 NAM

$ ./target/debug/namadac rewards --source tnam1qrtypl0yazfamk8ptmgp608msl8wzrkpmqwppc7w --validator tnam1qxsx2ezu89gx252kwwluqp7hadyp285tkczhaqg0 --epoch 59
Rewards for epoch 59: 3887.536527 NAM

$ ./target/debug/namadac rewards --source tnam1qrtypl0yazfamk8ptmgp608msl8wzrkpmqwppc7w --validator tnam1qxsx2ezu89gx252kwwluqp7hadyp285tkczhaqg0 --epoch 58
Rewards for epoch 58: 3148.755426 NAM

Checklist before merging

• If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
  • This will require 2 reviewers to approve the changes
• If this PR requires changes to the docs or specs, a corresponding PR is opened in the namada-docs repo
  • Relevant PR if applies: (coming soon)
• If this PR affects services such as namada-indexer or namada-masp-indexer, a corresponding PR is opened in that repo
  • Relevant PR if applies: feat: backfill epoch rewards namada-indexer#235

[codecov]

Codecov Report

Attention: Patch coverage is 13.79310% with 50 lines in your changes missing coverage. Please review.

Project coverage is 74.42%. Comparing base (2ae305c) to head (89df61f).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/sdk/src/queries/vp/pos.rs	12.28%	50 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4196      +/-   ##
==========================================
- Coverage   74.46%   74.42%   -0.05%     
==========================================
  Files         342      342              
  Lines      107773   107826      +53     
==========================================
- Hits        80257    80252       -5     
- Misses      27516    27574      +58     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[joel-u410]

Testing that this behaves as expected for a validator that claimed rewards in epoch 51:

$ ./target/debug/namadac rewards --validator tnam1qyf80vw4e8aa2efuk4jsg9lpwxrntsj2dql4tssa --epoch 50
Rewards for epoch 50: 2.179855 NAM

$ ./target/debug/namadac rewards --validator tnam1qyf80vw4e8aa2efuk4jsg9lpwxrntsj2dql4tssa --epoch 51
Rewards for epoch 51: 4.363707 NAM

$ ./target/debug/namadac rewards --validator tnam1qyf80vw4e8aa2efuk4jsg9lpwxrntsj2dql4tssa --epoch 52
Rewards for epoch 52: 2.897536 NAM

[joel-u410]

Of particular note, this code accounts for the rewards_counter when specifying an epoch by:

• Subtracting the current counter that was included by query_reward_tokens
• Adding the historical counter queried from storage at the height of epoch start

[brentstone]

Mind adding some unit tests here to demonstrate the behavior too @joel-u410?

[joel-u410]

Mind adding some unit tests here to demonstrate the behavior too @joel-u410?

For sure, I can look into that.

[joel-u410]

@brentstone @tzemanovic -- I've added a unit test that demonstrates querying for rewards with the epoch argument.

Please take a look: 8cb563f

Note that my test initially would not pass because the MockDB didn't provide a proper implementation for read_subspace_val_with_height . I added a very basic implementation (which does seem to work!) in this commit: 3c47541

[tzemanovic]

hi @joel-u410, thanks for the PR!

Do I understand correctly that you'd like to know the rewards distributed at the given epoch? The rewards query current returns unclaimed rewards, which accumulate and may be claimed at any point so I think there are some limitations with getting this information out of it. Specifically, when the rewards are claimed, the delegation bond amounts and validator commission rewards counter will go back to 0, but from this query endpoint it won't be very apparent what's happened. Because of this, I'm tempted to say that this functionality may be better built into an indexer, but if this is useful despite the limitations we could still add it, but I think we might need to better document how this work (I think one might expect that this returns the rewards obtained in the given epoch, but really it is accumulated unclaimed rewards)

[tzemanovic]

yeah, that's right :/

[joel-u410]

Is there any way to easily detect when this situation would apply (i.e. quickly determine whether there was a reward claim for source + validator in the current epoch)?

If there was, I could add that to the conditions here to avoid falsely short-circuiting in that case.

I'd like to keep this short-circuit here if at all possible because it significantly improves performance for the indexer when it's processing an older epoch.

[tzemanovic]

yeah, see get_last_reward_claim_epoch

[tzemanovic]

this is the last outstanding thing, other than that lgtm!

[tzemanovic]

btw, sry the CI is not working for external PRs atm - will workaround it before it's fixed once this is rdy

[joel-u410]

@tzemanovic - Great thanks for the review! I’ll address the comments and push an update soon.

(I think one might expect that this returns the rewards obtained in the given epoch, but really it is accumulated unclaimed rewards)

Yes, I’m aware this is returning accumulated unclaimed rewards, and in fact the reason I’m making the change here to specify an epoch is in support of namada-indexer for two reasons:

• currently it just queries it without an epoch, and I want to tighten that up to be specific about which epoch it’s querying
• then it can also backfill this data for past epochs

all of this then will make it possible for the indexer to calculate the rewards earned in each epoch, by comparing these unclaimed values by epoch and also taking into account any claim transactions that occurred in each epoch.

I agree some better documentation would be helpful. Where should that go?

[tzemanovic]

I agree some better documentation would be helpful. Where should that go?

we could add this to the rewards cli about string

[joel-u410]

@tzemanovic all comments addressed; should be ready to merge!