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

stakerRewards bug | Payouts #5859

Open
ironoa opened this issue Apr 22, 2024 · 19 comments
Open

stakerRewards bug | Payouts #5859

ironoa opened this issue Apr 22, 2024 · 19 comments

Comments

@ironoa
Copy link

ironoa commented Apr 22, 2024

stakerRewards call may be broken, not filtering out the already claimed rewards

Likely related, the ui seems to be showing something wrong (I've just manually claimed for era 6528, 6527, 6526, but yet they are shown there)
image

As a double check, If I try to claim again I get indeed the the already claimed error
image

@polkadot/api version: v10.13.1

@ironoa ironoa changed the title stakerRewards bug stakerRewards bug | Payouts Apr 22, 2024
@ironoa
Copy link
Author

ironoa commented Apr 22, 2024

I used to get the Rewards to be claimed like this, but now it seem to be deprecated and the note says to use EraInfo... any suggestion ?

@TarikGul
Copy link
Member

Known issue. I am working on a fix to get the staking pages sorted and fixed.

@ironoa
Copy link
Author

ironoa commented Apr 22, 2024

Thanks for the answer.

fyi maybe related: being connected with a kusama node on v1.10.0, performing this (this.api.derive.staking.query(validatorAddress,{withLedger:true})) I don't see the ClaimedRewards property, but just the legacyClaimedRewards. And the legacyClaimedRewards doesn't go past the 6,510 era => It may cause an issue here

@TarikGul
Copy link
Member

Yea you definitely brought up a great point with the claimedRewards. I have been trying to clean it up in the derives for the past 3 hours as this is the last blocker until I can start pushing all these fixes downstream to the UI, but the change from legacy_claimed_rewards to ::ClaimedRewards is such a PITA given the current logic in the api.

I'll find some solution though 🤞

@TarikGul
Copy link
Member

Fixed via #5862

@TarikGul
Copy link
Member

this.api.derive.staking.query(validatorAddress,{withLedger:true, withClaimedRewardsEras: true}) Will add a claimedRewardsEras field. Then you can combine legacyClaimedRewards with claimedRewardsEras in order to see all the eras

@ironoa
Copy link
Author

ironoa commented Apr 24, 2024

Thanks for the quick resolution, I indeed implemented it using this.api.derive.staking.query(validatorAddress,{withLedger:true, withClaimedRewardsEras: true})

For reference: https://github.com/w3f/polkadot-k8s-payouts/blob/v1.2.9/src/claimer.ts#L138-L171


Unfortunately, there seems to be a bug still: even if unclaimed rewards are spotted, one cannot claim anything before era 6514 (speaking about Kusama) due to an AlreadyClaimed error

For now, this is my temporary fix. However, it will prevent people from claiming rewards before era 6514.

UPDATE: Polkadot seems to have the same issue, one cannot claim before era 1420


Maybe related: paritytech/polkadot-sdk#1189

@TarikGul
Copy link
Member

@Ank4n Have you anything similar to this? Pjs api is just returning legacyClaimedRewards, and the new ClaimedRewards from the node so I would doubt any of the data is wrong from the api side? Is there a new specific way for a user to claim those older rewards?

@Ank4n
Copy link

Ank4n commented Apr 24, 2024

  • 6512, error

Older rewards should work as usual. After the upgrade, all claimed rewards for eras are noted in the new storage ClaimedRewards. Older rewards are always treated as of 1 page.

For era 6512 your validator claimed reward here: https://kusama.subscan.io/extrinsic/22806784-2.

I didn't check all the links you posted but I think it might already be claimed which is why you are seeing the error (correctly). You should see an entry in ClaimedRewards storage for that era and stash to double check.

@ironoa
Copy link
Author

ironoa commented Apr 25, 2024

  • 6512, error

Older rewards should work as usual. After the upgrade, all claimed rewards for eras are noted in the new storage ClaimedRewards. Older rewards are always treated as of 1 page.

For era 6512 your validator claimed reward here: https://kusama.subscan.io/extrinsic/22806784-2.

I didn't check all the links you posted but I think it might already be claimed which is why you are seeing the error (correctly). You should see an entry in ClaimedRewards storage for that era and stash to double check.

this.api.derive.staking.query(validatorAddress,{withLedger:true, withClaimedRewardsEras: true}) Will add a claimedRewardsEras field. Then you can combine legacyClaimedRewards with claimedRewardsEras in order to see all the eras

Thanks for checking. But then I guess the problem is that nor legacyClaimedRewards nor claimedRewardsEras are showing that for era 6512 the reward has been claimed (code here)

@TarikGul
Copy link
Member

Weirdly enough I do remember seeing something similar during testing where the era during the staking transition was missing within legacyClaimedRewards, and or claimedRewards. At the time I thought nothing of it since I assumed it was just unclaimed, but now it seems a bit weird.

I don't have the ability to verify what I saw right now since I am OOO for the next week. But definitely something to follow up on.

@Ank4n
Copy link

Ank4n commented Apr 25, 2024

  • 6512, error

Older rewards should work as usual. After the upgrade, all claimed rewards for eras are noted in the new storage ClaimedRewards. Older rewards are always treated as of 1 page.
For era 6512 your validator claimed reward here: https://kusama.subscan.io/extrinsic/22806784-2.
I didn't check all the links you posted but I think it might already be claimed which is why you are seeing the error (correctly). You should see an entry in ClaimedRewards storage for that era and stash to double check.

this.api.derive.staking.query(validatorAddress,{withLedger:true, withClaimedRewardsEras: true}) Will add a claimedRewardsEras field. Then you can combine legacyClaimedRewards with claimedRewardsEras in order to see all the eras

Thanks for checking. But then I guess the problem is that nor legacyClaimedRewards nor claimedRewardsEras are showing that for era 6512 the reward has been claimed (code here)

Probably bug with the derived api? If you query the storage directly you can see that it is claimed.

console.log((await api.query.staking.claimedRewards(6512, 'GaK38GT7LmgCpRSTRdDC2LeiMaV9TJmx8NmQcb9L3cJ3fyX')).length)
// returns 1

@ironoa
Copy link
Author

ironoa commented Apr 25, 2024

Probably bug with the derived api? If you query the storage directly you can see that it is claimed.

console.log((await api.query.staking.claimedRewards(6512, 'GaK38GT7LmgCpRSTRdDC2LeiMaV9TJmx8NmQcb9L3cJ3fyX')).length)
// returns 1
console.log((await api.query.staking.claimedRewards(6511, 'GaK38GT7LmgCpRSTRdDC2LeiMaV9TJmx8NmQcb9L3cJ3fyX')).length)
// returns 0

And it returns 0 for all the era previous to 6512 (which is not correct)

6511,error

@Ank4n
Copy link

Ank4n commented Apr 25, 2024

Before https://kusama.polkassembly.io/referenda/373, claimed rewards were saved in the storage item Ledger as a vec. After the upgrade it is stored in ClaimedRewards. Anything claimed after the upgrade is marked in the new storage.
6511 will be present either in the new storage or in the legacy_claimed_rewards.

The reason behind doing this lazy upgrade was that migrating (all ledgers * 84 eras) would be extremely huge. In 84 eras though, all non paged storage item would be stale and there will exist only one source of truth. Admittedly this is causing lot of confusion. More info here.

I wrote a small script that reads both old and new storage and figures out if era is claimed or not.

let stash = 'GaK38GT7LmgCpRSTRdDC2LeiMaV9TJmx8NmQcb9L3cJ3fyX';
// we need controller to read ledger.
let controller = (await api.query.staking.bonded(stash)).unwrap();
let era = 6512;

// calculate pages
let pageCount = 0;

// check if non paged exposure, then page is always 1
if ((await api.query.staking.erasStakers(era, stash)).total > 0) {
  pageCount = 1;
} else {
  let overview = (await api.query.staking.erasStakersOverview(era, stash));
  if (overview !== null && overview.isSome) {
    // if paged exposure, we get count from `ErasStakersOverview`.
    pageCount = overview.unwrap().pageCount;
  }
  else {
    console.log('no exposure found');
    return;
  }
}  

// get paged claimed rewards.
let pagedClaimedRewards = (await api.query.staking.claimedRewards(era, stash)).length == pageCount;

// get legacy claimed rewards.
let legacyClaimedRewards = (await api.query.staking.ledger(controller)).unwrap().legacyClaimedRewards.filter((e) => e == era).length == 1;

console.log(`page claimed: ${pagedClaimedRewards || legacyClaimedRewards}`);

@Ank4n
Copy link

Ank4n commented Apr 25, 2024

I think what would help is a runtime api to check if era reward is claimable given a validator stash and era. I will make one but might take sometime for it to be live on production networks.

@Ank4n
Copy link

Ank4n commented Apr 25, 2024

@ironoa This would help in future but I think it will take sometime to get it merged and live in Kusama and Polkadot. paritytech/polkadot-sdk#4301

Also happy to incorporate any other runtime suggestions that would help.

@ironoa
Copy link
Author

ironoa commented Apr 25, 2024

Before https://kusama.polkassembly.io/referenda/373, claimed rewards were saved in the storage item Ledger as a vec. After the upgrade it is stored in ClaimedRewards. Anything claimed after the upgrade is marked in the new storage.
6511 will be present either in the new storage or in the legacy_claimed_rewards.

To recap:

  • In case of era 6511
  • console.log((await api.query.staking.claimedRewards(6511, 'GaK38GT7LmgCpRSTRdDC2LeiMaV9TJmx8NmQcb9L3cJ3fyX')).length) // returns 0 => not working , expected due to runtime upgrade
  • legacyClaimedRewards from derive.staking is not working, likely a bug, right?
  • let legacyClaimedRewards = (await api.query.staking.ledger(controller)).unwrap().legacyClaimedRewards.filter((e) => e == era).length == 1;, the one you @Ank4n suggested, works

So I guess the only thing missing is to fix the legacyClaimedRewards from derive.staking, wdyt ?

@TarikGul
Copy link
Member

So I guess the only thing missing is to fix the legacyClaimedRewards from derive.staking, wdyt ?

Yea I will double check this when I am back to see if there is any under lying issues with how the derive parses the legacyClaimedRewards, portion here

@TarikGul
Copy link
Member

TarikGul commented May 8, 2024

So #5870 fixes the main issues with the eras being incorrect. The regression is fixed which means the withClaimedRewardsEras can be reactivated.

That being said I did notice an issue and I will reference polkadot as the chain since that is what I was testing with.

If i query claimedRewards for era 1419, and lets say accountId 12YP2b7L7gcHabZqE7vJMyF9eSZA9W68gnvb8BzTYx4MUxRo then I will get the necessary page info.

But when I query erasStakersOverview for 1419 with the same account there is no era.

Here is a code example:

        const x = await api.query.staking.claimedRewards(1419, '12YP2b7L7gcHabZqE7vJMyF9eSZA9W68gnvb8BzTYx4MUxRo')
        const y = await api.query.staking.erasStakersOverview(1419, '12YP2b7L7gcHabZqE7vJMyF9eSZA9W68gnvb8BzTYx4MUxRo')
        const z = await api.query.staking.ledger('12YP2b7L7gcHabZqE7vJMyF9eSZA9W68gnvb8BzTYx4MUxRo');
        console.log('claimedRewards: ', x.toJSON());
        console.log('Overview: ', y.isSome);
        console.log('LegacyClaimedRewards: ', z.isSome && z.unwrap().legacyClaimedRewards.toJSON())

This will output:

claimedRewards:  [ 0 ]
Overview:  false
LegacyClaimedRewards:  [
  1352, 1353, 1354, 1355, 1356, 1357, 1358,
  1359, 1360, 1361, 1362, 1363, 1364, 1365,
  1366, 1367, 1368, 1369, 1370, 1371, 1372,
  1373, 1374, 1375, 1376, 1377, 1378, 1379,
  1380, 1381, 1382, 1383, 1384, 1385, 1386,
  1387, 1388, 1389, 1390, 1391, 1392, 1393,
  1394, 1395, 1396, 1397, 1398, 1399, 1400,
  1401, 1402, 1403, 1404, 1405, 1406, 1407,
  1408, 1409, 1410, 1411, 1412, 1413, 1414,
  1415, 1416, 1417, 1418
]

That being said once the above PR goes in I think it's safe to say we can close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants