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

_evaluateVotes would run out of gas if there are too many voters #65

Closed
3esmit opened this issue Jun 30, 2023 · 2 comments · Fixed by #72
Closed

_evaluateVotes would run out of gas if there are too many voters #65

3esmit opened this issue Jun 30, 2023 · 2 comments · Fixed by #72
Labels
priority 0: urgent/blocker Feature/fix that has to be provided ASAP
Milestone

Comments

@3esmit
Copy link
Member

3esmit commented Jun 30, 2023

for (uint256 i = 0; i < votesByRoomID[votingRoom.roomNumber].length; i++) {

This assumes the blockchain have infinite processing capabilities for the tabulation loop, and splitting the processing in multiple transactions opens to double voting because the balances are not locked. This works fine for small rooms (with just a few voters), but when there are hundreds of voters, not just it would get extremely expansive for a single actor, it eventually would surpass the block gas limit, making the tabulation impossible.

@osmaczko
Copy link
Collaborator

osmaczko commented Jul 3, 2023

Thank you @3esmit for pinpointing the issue. As we discussed in today's meeting, the proposed solution is to make SNT a MiniMeToken. This will allow us to verify votes across multiple transactions using balanceOfAt(address, endOfVotingBlock).

Note:
This will require some minor modifications to our existing contracts and user interface, considering that we now have to account for the potential scenario where the finalization stage may be executed in multiple steps/transactions.

@osmaczko osmaczko added this to the beta release milestone Jul 3, 2023
@osmaczko osmaczko added the priority 0: urgent/blocker Feature/fix that has to be provided ASAP label Jul 3, 2023
@3esmit 3esmit mentioned this issue Jul 10, 2023
@osmaczko osmaczko removed this from the beta release milestone Sep 19, 2023
@John-44
Copy link
Collaborator

John-44 commented Sep 21, 2023

If any design work is needed for UI changes, @benjthayer is the person to ping (@benjthayer I can help give background on this as needed).

0x-r4bbit added a commit that referenced this issue Sep 27, 2023
This commit introduces a change that has been largely discussed in
#65

Generally the idea is the following:

- Prior to this commit, the amount of votes to be evaluated is
  unbounded, meaning we can potentially run out of gas when too many
  votes have to be evaluated in a single transaction.
- To account for that, vote processing has to be batched based on a
  `limit` that is provided when the contract is called.
- As a result of batching, finalization of votes can happen over
  multiple transactions which makes checking balances of voters tricky
  because evaluation happens at different blocks where balances might
  have changed.
- That's why we're also introducing the use of `MiniMeToken` over
  standard `ERC20` because it allows us to check balances of accounts at
  certain blocks.

This commit introduces the following changes in both `VotingContract`
and `FeaturedVotingContract`:

1. Evaluation of votes now always happens in three places: When a voting
   is initialized, when votes as casted and when a voting is being
   finalized.
2. When initializing a voting or casting votes, the votes are evaluated
   against the current block, front-ends can therefore show a good
   estimation of the current vote state.
3. When finalizaing a vote, all votes are (re)evaluated against the
   block at which the finalization has started. This means with
   finalization, all votes are evaluated again to ensure they represent
   the latest state.

In terms of code this means:

`_evaluateVotes()` requires and additional `limit`, as a result of
that `finializeVoting()` and `finalizeVotingRoom()` require a `limit`
as well. `castVotes()` derives the `limit` from the vote amount
that's being sent.
0x-r4bbit added a commit that referenced this issue Sep 28, 2023
This commit introduces a change that has been largely discussed in
#65

Generally the idea is the following:

- Prior to this commit, the amount of votes to be evaluated is
  unbounded, meaning we can potentially run out of gas when too many
  votes have to be evaluated in a single transaction.
- To account for that, vote processing has to be batched based on a
  `limit` that is provided when the contract is called.
- As a result of batching, finalization of votes can happen over
  multiple transactions which makes checking balances of voters tricky
  because evaluation happens at different blocks where balances might
  have changed.
- That's why we're also introducing the use of `MiniMeToken` over
  standard `ERC20` because it allows us to check balances of accounts at
  certain blocks.

This commit introduces the following changes in both `VotingContract`
and `FeaturedVotingContract`:

1. Evaluation of votes now always happens in three places: When a voting
   is initialized, when votes as casted and when a voting is being
   finalized.
2. When initializing a voting or casting votes, the votes are evaluated
   against the current block, front-ends can therefore show a good
   estimation of the current vote state.
3. When finalizaing a vote, all votes are (re)evaluated against the
   block at which the finalization has started. This means with
   finalization, all votes are evaluated again to ensure they represent
   the latest state.

In terms of code this means:

`_evaluateVotes()` requires and additional `limit`, as a result of
that `finializeVoting()` and `finalizeVotingRoom()` require a `limit`
as well. `castVotes()` derives the `limit` from the vote amount
that's being sent.
0x-r4bbit added a commit that referenced this issue Sep 28, 2023
This commit introduces a change that has been largely discussed in
#65

Generally the idea is the following:

- Prior to this commit, the amount of votes to be evaluated is
  unbounded, meaning we can potentially run out of gas when too many
  votes have to be evaluated in a single transaction.
- To account for that, vote processing has to be batched based on a
  `limit` that is provided when the contract is called.
- As a result of batching, finalization of votes can happen over
  multiple transactions which makes checking balances of voters tricky
  because evaluation happens at different blocks where balances might
  have changed.
- That's why we're also introducing the use of `MiniMeToken` over
  standard `ERC20` because it allows us to check balances of accounts at
  certain blocks.

This commit introduces the following changes in both `VotingContract`
and `FeaturedVotingContract`:

1. Evaluation of votes now always happens in three places: When a voting
   is initialized, when votes as casted and when a voting is being
   finalized.
2. When initializing a voting or casting votes, the votes are evaluated
   against the current block, front-ends can therefore show a good
   estimation of the current vote state.
3. When finalizaing a vote, all votes are (re)evaluated against the
   block at which the finalization has started. This means with
   finalization, all votes are evaluated again to ensure they represent
   the latest state.

In terms of code this means:

`_evaluateVotes()` requires and additional `limit`, as a result of
that `finializeVoting()` and `finalizeVotingRoom()` require a `limit`
as well. `castVotes()` derives the `limit` from the vote amount
that's being sent.
0x-r4bbit added a commit that referenced this issue Sep 28, 2023
This commit introduces a change that has been largely discussed in
#65

Generally the idea is the following:

- Prior to this commit, the amount of votes to be evaluated is
  unbounded, meaning we can potentially run out of gas when too many
  votes have to be evaluated in a single transaction.
- To account for that, vote processing has to be batched based on a
  `limit` that is provided when the contract is called.
- As a result of batching, finalization of votes can happen over
  multiple transactions which makes checking balances of voters tricky
  because evaluation happens at different blocks where balances might
  have changed.
- That's why we're also introducing the use of `MiniMeToken` over
  standard `ERC20` because it allows us to check balances of accounts at
  certain blocks.

This commit introduces the following changes in both `VotingContract`
and `FeaturedVotingContract`:

1. Evaluation of votes now always happens in three places: When a voting
   is initialized, when votes as casted and when a voting is being
   finalized.
2. When initializing a voting or casting votes, the votes are evaluated
   against the current block, front-ends can therefore show a good
   estimation of the current vote state.
3. When finalizaing a vote, all votes are (re)evaluated against the
   block at which the finalization has started. This means with
   finalization, all votes are evaluated again to ensure they represent
   the latest state.

In terms of code this means:

`_evaluateVotes()` requires and additional `limit`, as a result of
that `finializeVoting()` and `finalizeVotingRoom()` require a `limit`
as well. `castVotes()` derives the `limit` from the vote amount
that's being sent.
0x-r4bbit added a commit that referenced this issue Sep 29, 2023
This commit introduces a change that has been largely discussed in #65

Generally the idea is the following:

Prior to this commit, the amount of votes to be evaluated is unbounded, meaning we can potentially run out of gas when too many votes have to be evaluated in a single transaction.
To account for that, vote processing has to be batched based on a limit that is provided when the contract is called.
As a result of batching, finalization of votes can happen over multiple transactions which makes checking balances of voters tricky because evaluation happens at different blocks where balances might have changed.
That's why we're also introducing the use of MiniMeToken over standard ERC20 because it allows us to check balances of accounts at certain blocks.
This commit introduces the following changes in both VotingContract and FeaturedVotingContract:

Evaluation of votes now always happens in three places: When a voting is initialized, when votes are casted and when a voting is being finalized.
When initializing a voting or casting votes, the votes are evaluated against the current block, front-ends can therefore show a good estimation of the current vote state.
Only the "new" votes are evaluated, which should equal to the amount of votes being sent.
When finalizaing a vote, all votes are (re)evaluated against the block at which the finalization has started. This means with finalization, all votes are evaluated again to ensure they represent the latest state.
In terms of code this means:

_evaluateVotes() requires and additional limit, as a result of that finializeVoting() and finalizeVotingRoom() require a limit as well. castVotes() derives the limit from the vote amount that's being sent.
0x-r4bbit added a commit that referenced this issue Oct 4, 2023
This commit introduces a change that has been largely discussed in #65

Generally the idea is the following:

Prior to this commit, the amount of votes to be evaluated is unbounded, meaning we can potentially run out of gas when too many votes have to be evaluated in a single transaction.
To account for that, vote processing has to be batched based on a limit that is provided when the contract is called.
As a result of batching, finalization of votes can happen over multiple transactions which makes checking balances of voters tricky because evaluation happens at different blocks where balances might have changed.
That's why we're also introducing the use of MiniMeToken over standard ERC20 because it allows us to check balances of accounts at certain blocks.
This commit introduces the following changes in both VotingContract and FeaturedVotingContract:

Evaluation of votes now always happens in three places: When a voting is initialized, when votes are casted and when a voting is being finalized.
When initializing a voting or casting votes, the votes are evaluated against the current block, front-ends can therefore show a good estimation of the current vote state.
Only the "new" votes are evaluated, which should equal to the amount of votes being sent.
When finalizaing a vote, all votes are (re)evaluated against the block at which the finalization has started. This means with finalization, all votes are evaluated again to ensure they represent the latest state.
In terms of code this means:

_evaluateVotes() requires and additional limit, as a result of that finializeVoting() and finalizeVotingRoom() require a limit as well. castVotes() derives the limit from the vote amount that's being sent.
0x-r4bbit added a commit that referenced this issue Oct 4, 2023
This commit introduces a change that has been largely discussed in #65

Generally the idea is the following:

Prior to this commit, the amount of votes to be evaluated is unbounded, meaning we can potentially run out of gas when too many votes have to be evaluated in a single transaction.
To account for that, vote processing has to be batched based on a limit that is provided when the contract is called.
As a result of batching, finalization of votes can happen over multiple transactions which makes checking balances of voters tricky because evaluation happens at different blocks where balances might have changed.
That's why we're also introducing the use of MiniMeToken over standard ERC20 because it allows us to check balances of accounts at certain blocks.
This commit introduces the following changes in both VotingContract and FeaturedVotingContract:

Evaluation of votes now always happens in three places: When a voting is initialized, when votes are casted and when a voting is being finalized.
When initializing a voting or casting votes, the votes are evaluated against the current block, front-ends can therefore show a good estimation of the current vote state.
Only the "new" votes are evaluated, which should equal to the amount of votes being sent.
When finalizaing a vote, all votes are (re)evaluated against the block at which the finalization has started. This means with finalization, all votes are evaluated again to ensure they represent the latest state.
In terms of code this means:

_evaluateVotes() requires and additional limit, as a result of that finializeVoting() and finalizeVotingRoom() require a limit as well. castVotes() derives the limit from the vote amount that's being sent.
0x-r4bbit added a commit that referenced this issue Oct 4, 2023
This commit introduces a change that has been largely discussed in #65

Generally the idea is the following:

Prior to this commit, the amount of votes to be evaluated is unbounded, meaning we can potentially run out of gas when too many votes have to be evaluated in a single transaction.
To account for that, vote processing has to be batched based on a limit that is provided when the contract is called.
As a result of batching, finalization of votes can happen over multiple transactions which makes checking balances of voters tricky because evaluation happens at different blocks where balances might have changed.
That's why we're also introducing the use of MiniMeToken over standard ERC20 because it allows us to check balances of accounts at certain blocks.
This commit introduces the following changes in both VotingContract and FeaturedVotingContract:

Evaluation of votes now always happens in three places: When a voting is initialized, when votes are casted and when a voting is being finalized.
When initializing a voting or casting votes, the votes are evaluated against the current block, front-ends can therefore show a good estimation of the current vote state.
Only the "new" votes are evaluated, which should equal to the amount of votes being sent.
When finalizaing a vote, all votes are (re)evaluated against the block at which the finalization has started. This means with finalization, all votes are evaluated again to ensure they represent the latest state.
In terms of code this means:

_evaluateVotes() requires and additional limit, as a result of that finializeVoting() and finalizeVotingRoom() require a limit as well. castVotes() derives the limit from the vote amount that's being sent.
@osmaczko osmaczko linked a pull request Oct 13, 2023 that will close this issue
@osmaczko osmaczko added this to the beta release milestone Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority 0: urgent/blocker Feature/fix that has to be provided ASAP
Projects
None yet
3 participants