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

ISSUE-87: Return early if now == rho #99

Closed
wants to merge 2 commits into from
Closed

ISSUE-87: Return early if now == rho #99

wants to merge 2 commits into from

Conversation

godsflaw
Copy link
Contributor

@godsflaw godsflaw commented Nov 8, 2019

This is fixes #87

gas saving are 40,053 if called in the same block.

@@ -138,7 +138,8 @@ contract Pot is LibNote {

// --- Savings Rate Accumulation ---
function drip() external note returns (uint tmp) {
require(now >= rho, "Pot/invalid-now");
if (now == rho) return chi;
require(now > rho, "Pot/invalid-now");

Choose a reason for hiding this comment

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

Unrelated to this PR, shouldn't this be invalid-rho?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

time is relative... Sorry, been a long day.

@gbalabasquer gbalabasquer requested a review from a team November 8, 2019 01:51
@kmbarry1
Copy link
Contributor

kmbarry1 commented Nov 8, 2019

Just to accurately represent the calculus behind this, using the following numbers (which others measured, I have not checked them):

savings of optimization when rho == now: 40,053 gas
cost of optimization when rho != now: 310 gas

one finds that the odds of calling drip when rho == now need to be about 1/130 for this "optimization" to actually be an improvement, on average.

Making this change represents a certain level of faith that the system will reach a scale where this condition holds so real gas savings accrue. Although my priors aren't terribly well-informed, I suspect that this will be a net loss at least for some period of time (I'm expecting << 1 drip call per block, at least initially, e.g. on the order of a few per hour).

Of course, EVM storage operations tend to get more expensive with each hard fork, so it is more likely that the threshold become more favorable over time than not.

Note: above numbers were for Jug.drip.

@livnev
Copy link
Contributor

livnev commented Nov 8, 2019

@kmbarry1 I would recommend repeating the calculus under EIP 2200, since that is what really matters.

@brianmcmichael
Copy link
Contributor

It's sufficiently complex and the rewards so minimal as to be unlikely, but as MCD scales this mechanism could theoretically be utilized by miners to bundle these transactions (or unbundle them) to consume less (or capture additional) gas for each call. I don't think this side-effect is an argument for or against, but thought it was worth pointing out the observation.

@MicahZoltu
Copy link

MicahZoltu commented Nov 8, 2019

I think something that isn't being considered in this discussion is that if this change is not made, then anyone who calls drip MUST do if (pot.rho() != now) pot.drip(). If they don't, they risk a failed transaction. It is significantly more expensive for an external caller to do that check than it is to do it internally, especially since internally you are already loading rho from storage.

Given that all callers must first do the check, I believe this change is a gas reduction in all cases except the theoretical (but likely never encountered) case where the caller knows they are at the top of the block or otherwise has some guarantee that no transactions before it have triggered a pot.drip.

@gbalabasquer
Copy link
Contributor

why do they risk a failed transaction? If equals the tx still passes without this change, just consumes more gas.

@MicahZoltu
Copy link

Ah, sorry. Was thinking of something else. I shouldn't comment on GitHub late at night.

@MicahZoltu
Copy link

It is possible that if the DSR is high enough, bots will drip regularly as they move capital in/out of pot so they get the savings rate, but they need DAI to do their bot stuff. This is a potentiality where this function is hit with some regularity, enough to make the gas savings worth it.

@godsflaw
Copy link
Contributor Author

godsflaw commented Nov 8, 2019

Sorry to say, while we think the gains here are worthy, we are just taking too much risk to include this now. We have done multiple audits, a bug bounty, and formal verification of the system and after much deliberation we believe even a small change like this can invalidate those results and threaten our confidence in the system. Since the pot and jug are peripherally connected we can add this again later, perhaps with other optimizations, and let governance vote on upgrading these contracts after audits and formal verification.

@godsflaw godsflaw closed this Nov 8, 2019
@gbalabasquer gbalabasquer deleted the ISSUE-87 branch February 5, 2021 12:02
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.

Return early if now == rho.
6 participants