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

Optimize votes lookups for recent checkpoints #3673

Merged
merged 13 commits into from Sep 4, 2022

Conversation

frangio
Copy link
Contributor

@frangio frangio commented Sep 2, 2022

This PR optimizes getPastVotes so that for recent checkpoints it has lower gas cost than the worst case.

The worst case runs in O(log n) time, but delegates can have large numbers of checkpoints: a quick search led us to find one with 8500 checkpoints, which adds around 43k gas to a vote.

In the context of governance, since there is a voting period, getPastVotes will be invoked with a relatively recent block which will be near the end of the checkpoints array.

We modify getPastVotes so that it first checks to see if the sought checkpoint is a recent checkpoint, if so does a binary search among those, and otherwise does a binary search on the rest of the array. We define the recent checkpoints as the last sqrt(n), where n is the number of checkpoints. Thus, the best case cost of getPastVotes is half that of the worst case, though it remains logarithmic.


Some other changes bundled here:

  • Changed ERC20Votes to use unsafe array accesses to save on bounds checks.
  • Renamed getAtRecentBlock to getAtProbablyRecentBlock, to indicate that the function works even for non-recent blocks.
  • Removed upperLookupRecent for other Trace/Checkpoints types, since we won't be using it and if the keys are not block numbers or timestamps it is not clear that they are worth it.

@frangio frangio requested a review from Amxx September 2, 2022 01:12
@Amxx
Copy link
Collaborator

Amxx commented Sep 2, 2022

In the provided graph, we can see the transition from the "lookup is recent and cheap" to "lookup is older and more expensive" when the offset reaches 7. Yes you say K=16. Do you have an explanation for that ? I would expect the transition to appen around K

@frangio
Copy link
Contributor Author

frangio commented Sep 2, 2022

I think I may have been mistaken and the chart was for K=8.

I've now modified the PR to use K=sqrt(N).

@frangio frangio requested review from Amxx and removed request for Amxx September 2, 2022 19:31
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, but wee need better coverrage.

@frangio
Copy link
Contributor Author

frangio commented Sep 3, 2022

Added tests and coverage is good now.

@Amxx Amxx merged commit e09ccd1 into OpenZeppelin:master Sep 4, 2022
@frangio frangio deleted the optim-recent branch September 5, 2022 13:48
ronhuafeng added a commit to ronhuafeng/openzeppelin-contracts that referenced this pull request Sep 9, 2022
JulissaDantes pushed a commit to JulissaDantes/openzeppelin-contracts that referenced this pull request Nov 3, 2022
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

Successfully merging this pull request may close these issues.

None yet

2 participants