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

Don't include stakers with expired stakes in the missing_stakers results for StakingEscrowAgent.partition_stakers_by_activity() #2764

Merged
merged 2 commits into from
Jul 30, 2021

Conversation

derekpierre
Copy link
Member

Type of PR:

  • Bugfix
  • Feature
  • Documentation
  • Other

Required reviews:

  • 1
  • 2
  • 3

What this does:
Ignores stakers with expired stakes.

Issues fixed/closed:
Fixes #2760 .

Why it's needed:
Including stakers with expired stakes causes an overstating of the number of inactive stakers on the network i.e. stakers who haven't committed to the current period.

@derekpierre derekpierre added Bug 🐛 Broken functionality Staking labels Jul 28, 2021
@derekpierre derekpierre requested a review from vzotova July 28, 2021 14:22
@derekpierre
Copy link
Member Author

derekpierre commented Jul 28, 2021

@vzotova does the StakingEscrow contract need to be updated to really fix this i.e. the contract should do the pruning of its stakers list from time to time? Otherwise, with this agent-specific fix, we end up with some inconsistencies eg.

>>> active_stakers, pending_stakers, missing_stakers = staking_agent.partition_stakers_by_activity()
>>> staking_agent.get_staker_population()
2129
>>> len(active_stakers) + len(pending_stakers) + len(missing_stakers)
638

2129 != 638.

It is also inefficient to do the pruning on the agent side, since every call to the partition function requires excessive staker checks for expired stakes, as opposed to the list in the contract being already correct/corrected.

Interested to get your thoughts here.

@vzotova
Copy link
Member

vzotova commented Jul 28, 2021

I'd prefer not to spend gas from anyone to make state clearer - just have this stakers and calculate real numbers offline. But it definitely possible

…lts for StakingEscrowAgent.partition_stakers_by_activity()
@derekpierre
Copy link
Member Author

@vzotova some times for you 😅 :

Using infura:

# partition function before checking for expired stakes on mainnet
473.30053091049194 s
493.7947690486908 s

# partition function when checking for expired stakes on mainnet
875.4592299461365 s
820.4715950489044 s
768.975784778595 s

@derekpierre
Copy link
Member Author

Ignore the numbers above, I was using the wrong web3 provider:

Using infura:

# before fix
262.1745069026947 s
266.5320801734924 s

# after fix
480.71446204185486 s
455.59283089637756 s

@derekpierre derekpierre changed the title [WIP] Don't include stakers with expired stakes in the missing_stakers results for StakingEscrowAgent.partition_stakers_by_activity() Don't include stakers with expired stakes in the missing_stakers results for StakingEscrowAgent.partition_stakers_by_activity() Jul 29, 2021
@derekpierre derekpierre marked this pull request as ready for review July 29, 2021 13:40
@derekpierre derekpierre merged commit 614e4fa into nucypher:main Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🐛 Broken functionality
Projects
None yet
3 participants