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

eip-2565: rework it #2955

Closed
wants to merge 5 commits into from
Closed

eip-2565: rework it #2955

wants to merge 5 commits into from

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Sep 8, 2020

The EIP-2565 is totally out of sync with the spec on the discussions-to, and also does not make sense, as

  • the suggested option 2 actually makes the precompile more expensive (divisor 20 to 3),
  • The quoted formula from 198 is actually not correct,
  • It has two recommended options, but we're actually only going to implement one
  • It has a third option which is not consensus-related, and shouldn't be part of the EIP

Discussion at https://ethereum-magicians.org/t/eip-2565-big-integer-modular-exponentiation-eip-198-gas-cost/4150/5
This PR is not ready to merge, just a starting point for rewriting it.

Primarily, it needs filling on the gas-table of testcases, and I'd rather have the authors do that, to ensure that the values used for the benchmarkings/charts matches with what's in the testcases.

@eip-automerger
Copy link

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

Merging is up to the authors, but I left some general comments. I'm a fan of the removal of multiple options, EIPs should generally just be technical specifications, not discussions (that is what the discussions-to link is for).

EIPS/eip-2565.md Outdated
Comment on lines 28 to 32
The current gas pricing formula is defined in [EIP-198](eip-198.md) as:

```
floor(mult_complexity(max(length_of_MODULUS, length_of_BASE)) * max(ADJUSTED_EXPONENT_LENGTH, 1) / GQUADDIVISOR)
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Defining old behavior feels out of scope for a specification. Can we just leave this bit out, or move it to the rationale section or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the original specification in this case is rather small, I think it makes sense to include it here. It puts the specified change in context, so the reader can understand what the effect of the EIP is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, the description of the previous behaviour was present before this PR (but it was not correct). I'll leave it in, for now, but I guess the final say won't be up to me

EIPS/eip-2565.md Outdated Show resolved Hide resolved
EIPS/eip-2565.md Show resolved Hide resolved
EIPS/eip-2565.md Outdated Show resolved Hide resolved
Comment on lines +69 to +73

## Appendix

### Abandoned option : Modify ‘computational complexity’ function and add minimum gas cost

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider moving this to the rationale (that section is intended to represent why certain choices were made in this EIP) rather than a non-standard Appendix section.


## Rationale

### **Recommended** Option (1): Modify ‘computational complexity’ formula
A comparison of the current ‘complexity’ function and the proposed function described above can be found at the following [spreadsheet](https://docs.google.com/spreadsheets/d/1-xBzA-2-l2ZQDQ1eh3XXGZjcRSBQ_Hnp7NubXpbiSUY/edit?usp=sharing).

![Option 1 Graph](../assets/eip-2565/Complexity_Regression.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

There are no longer options, so the asset title "Option 1 Graph" no longer makes sense.

EIPS/eip-2565.md Outdated Show resolved Hide resolved
holiman and others added 4 commits September 8, 2020 14:56
Co-authored-by: Micah Zoltu <[email protected]>
Co-authored-by: Micah Zoltu <[email protected]>
Co-authored-by: Micah Zoltu <[email protected]>
Co-authored-by: Micah Zoltu <[email protected]>
@holiman
Copy link
Contributor Author

holiman commented Sep 8, 2020

Also, if the option that is currently "the one" is indeed the way I parsed and rewrote it, without a minimum 100 gascost, then geth seems to get borderline DoS behaviour on nagydani-1-qube, at 6mgas/s, and nagydani-2-qube at 13mgas/s (where a more appropriate value, on this particular machine would be 30-40 mgasps).

So please advice...

@holiman
Copy link
Contributor Author

holiman commented Sep 8, 2020

If I use a minimum value of 100, the worst case becomes nagydani-3-qube, at 15.9mgas/s, with 189 gas per op (well, assuming 189 is correct)

@axic
Copy link
Member

axic commented Sep 8, 2020

Maybe worth checking the feedback on #2892 also.

@holiman
Copy link
Contributor Author

holiman commented Sep 8, 2020

Ah, yes, indeed relevant!

@ineffectualproperty
Copy link
Contributor

Thanks for the start to re-working this EIP @holiman . If you could please keep this open for the next couple days I will incorporate your feedback into #2892 . Once that is done we can close this issue and focus on that PR. Appreciate you taking the time to provide some proposed fixes and helping with the process.

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.

5 participants