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

Gas reductions #1101

Merged
merged 3 commits into from
Apr 24, 2023
Merged

Gas reductions #1101

merged 3 commits into from
Apr 24, 2023

Conversation

kronosapiens
Copy link
Member

@kronosapiens kronosapiens commented Nov 1, 2022

Gas cost data

Looking at these data, and considering current patterns of app usage, I would say that the following functions would most benefit from gas optimization:

  • OneTxPayment.makePayment
  • VotingReputation.stakeMotion
  • VotingReputation.claimReward
  • VotingReputation.revealVote
  • VotingReputation.submitVote

After analysis, the biggest change here comes from removing the setting of SUBMIT_END and REVEAL_END timestamps in VotingReputation prior to the stakes being set. Previously, they had been redundantly set twice.

@kronosapiens
Copy link
Member Author

@area some initial directions above

@kronosapiens
Copy link
Member Author

After looking through OneTxPayment, it unfortunately doesn't look like there are many opportunities to reduce gas without making deeper changes in the contracts. The functionality is about as optimized as can be, as much of the gas costs can be attributed to the many storage variables which must be written (~15-20) and many inter-contract calls which are used to provide authentication and security. The most notable opportunity for improvement would be to allow for the created structs to be deleted after use, but that direction introduces new challenges.

@kronosapiens
Copy link
Member Author

kronosapiens commented Dec 3, 2022

Notes on VotingReputation:

  • By removing the revealPeriod variable and asserting that submitPeriod and revealPeriod are always the same length, we would eliminate an entire storage slot per-motion and the associated updates. I think this an attractive trade-off, and would go so far as to argue we shouldn't offer that degree of configuration until there is a clear demand for it.
  • We may not need to set SUBMIT_END and REVEAL_END when only one side fully stakes.
  • We could save a bit of gas by not letting users update their _voteSecrets once submitted.
  • A wacky thought but we could probably use a bloom filter to lower the gas costs of storing secrets - e.g. store multiple secrets in a single storage slot. Finger-in-the-air I would say we could easily store at least 10 secrets per slot safely, if not more (we can calculate this rigorously).

@kronosapiens
Copy link
Member Author

@area what do you think of my moving forward with the VotingReputation optimizations I suggested above?

@area
Copy link
Member

area commented Dec 7, 2022

By removing the revealPeriod variable and asserting that submitPeriod and revealPeriod are always the same length, we would eliminate an entire storage slot per-motion and the associated updates. I think this an attractive trade-off, and would go so far as to argue we shouldn't offer that degree of configuration until there is a clear demand for it.

When we originally talked about looking at gas optimisations, I thought we agreed we'd be looking at no function changes? Either way, as ever, this type of change has to go through product.

We may not need to set SUBMIT_END and REVEAL_END when only one side fully stakes.

I like this one. Increases the cost for the first objection, but for 'good' motions, reduces the cost, which is probably what we want to optimise. This might affect the frontend, though, so we'd need to check with them that it's okay.

We could save a bit of gas by not letting users update their _voteSecrets once submitted.

I think it's important that people are able to change their votes, otherwise they're incentivised to wait until the last minute to vote in case any new information comes to light (which then nullifies the benefits of the 'fast forward' functionality).

A wacky thought but we could probably use a bloom filter to lower the gas costs of storing secrets - e.g. store multiple secrets in a single storage slot. Finger-in-the-air I would say we could easily store at least 10 secrets per slot safely, if not more (we can calculate this rigorously).

I don't really understand what you're proposing here, so you're going to have to go in to a lot more detail for me to give feedback on this one (I suspect on a call).

@kronosapiens
Copy link
Member Author

Noting that the optimization regarding not setting submit & reveal times has reduced the cost for creating a motion by 5000 gas, but increased the average cost of staking by 1500 gas (probably because the variables are set for the first time, rather than being updated).

@kronosapiens kronosapiens force-pushed the maint/gas-reductions branch 2 times, most recently from e5ee7ef to c1c8ac4 Compare December 23, 2022 01:55
@area
Copy link
Member

area commented Jan 3, 2023

Noting that the optimization regarding not setting submit & reveal times has reduced the cost for creating a motion by 5000 gas, but increased the average cost of staking by 1500 gas (probably because the variables are set for the first time, rather than being updated).

Is this cost of staking only for motions that get objected to?

@kronosapiens
Copy link
Member Author

@arrenv question: how important is it that users be able to configure both the length of time for submitting votes, and for revealing votes, as separate values? One proposed gas optimization is collapsing them into a single value (e.g. 48-hours for each period, rather than allowing 48-hours to submit votes and 24-hours to reveal votes, or something like that). Would that be an acceptable optimization?

@arrenv
Copy link
Member

arrenv commented Jan 3, 2023

@arrenv question: how important is it that users be able to configure both the length of time for submitting votes, and for revealing votes, as separate values? One proposed gas optimization is collapsing them into a single value (e.g. 48-hours for each period, rather than allowing 48-hours to submit votes and 24-hours to reveal votes, or something like that). Would that be an acceptable optimization?

Is it a one time gas saving, or ongoing for each Motion?

@kronosapiens
Copy link
Member Author

@arrenv ongoing, as we would have to update only one storage slot, not two, as the motion moves through the lifecycle. That said, it wouldn't improve every transaction (e.g. it wont affect most vote submission/reveal transactions), but it would affect a handful.

@arrenv
Copy link
Member

arrenv commented Jan 3, 2023

@arrenv ongoing, as we would have to update only one storage slot, not two, as the motion moves through the lifecycle. That said, it wouldn't improve every transaction (e.g. it wont affect most vote submission/reveal transactions), but it would affect a handful.

In my opinion, I don't see a big implication in merging them from a user and governance standpoint, it would make the options slightly simpler when setting it up and as a user understanding the configuration.

However, how would this affect previous Colonies that already have it as it is currently. The UI would need to be able to handle both cases depending on the version, right?

@kronosapiens kronosapiens force-pushed the maint/gas-reductions branch 3 times, most recently from 2a23586 to 3c966e1 Compare February 16, 2023 00:13
@kronosapiens
Copy link
Member Author

@rdig in this PR we are proposing to skip some of the lifecycle timestamp updates, until they are strictly necessary. @area wanted to to ask if you were using these timestamps earlier in the flow, and if so whether the change would affect the front-end. Specifically, would it affect the front-end if we didn't set the submitEnd and revealEnd timestamps until after both stakes have been submitted? Currently we set them at every stage in the life-cycle.

@rdig
Copy link
Member

rdig commented Feb 28, 2023

Yes, it will impact the frontend somewhat but I can't recall to what extent as it was a long time ago that we've implemented that.

At any rate, if this change is needed / wanted for the contracts, we'll accommodate it in the front-end, especially now since we're in "rebuild" mode

@kronosapiens
Copy link
Member Author

Noting that @area is asking whether this would introduce upgrade issues to colonies who already have this extension installed

@area
Copy link
Member

area commented Apr 13, 2023

After a call with Raul there are no issues with the frontend and this change.

@kronosapiens
Copy link
Member Author

Reviewing the code, I do not see any upgrade issues with this change. Currently, SUBMIT_END and REVEAL_END values are only used if both sides are fully staked. Until both sides fully staked, the values of those variables are not relevant. If earlier colonies had them set, they would be of no consequence.

@kronosapiens kronosapiens force-pushed the maint/gas-reductions branch from b7ef4e2 to ad7a155 Compare April 14, 2023 02:30
@kronosapiens
Copy link
Member Author

I think we are ready to merge

@area
Copy link
Member

area commented Apr 18, 2023

Noting that the optimization regarding not setting submit & reveal times has reduced the cost for creating a motion by 5000 gas, but increased the average cost of staking by 1500 gas (probably because the variables are set for the first time, rather than being updated).

Is this cost of staking only for motions that get objected to?

Where did we end up here?

@kronosapiens
Copy link
Member Author

@area the higher cost comes during the objection, since when the objection stakes we phase into voting and must set the submit and reveal timestamps. The initial affirmative stake does not have a higher gas cost. We essentially defer the initial variable setting to the objection stake, and avoid the intermediate (and redundant) updates entirely.

@area area merged commit 29c7be9 into develop Apr 24, 2023
@area area deleted the maint/gas-reductions branch April 24, 2023 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants