Update EIP-2565 to clarify specification

[ineffectualproperty]

This moves EIP-2565 from 'Last Call' to 'Accepted'. The review period ended on 2020-07-31 and there have been no further feedback or suggested changes provided.

EDIT: Due to the recommended changes, the Last Call date has been extended

[eip-automerger]

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

• File assets/eip-2565/Complexity_Regression.png is not an EIP
• File assets/eip-2565/GQuad_Change.png is not an EIP
• File assets/eip-2565/Library_Benchmarks.png is not an EIP

[MicahZoltu]

This should have been caught during last call, but can you change the links to be relative links rather than absolute?

[EIP-198](./eip-198)

[MicahZoltu]

If you can make that one fix (can be part of this PR, or a separate one) I'll merge this. Feel free to ping me if I don't notice the change.

[ineffectualproperty]

@MicahZoltu thank you for the feedback! I've updated to include to relative links. Please let me know if there are any other changes I should make

[MicahZoltu]

I feel a bit bad since I didn't review this during Last Call, but I do in fact have some more feedback now that I'm reading through this EIP more thoroughly (in preparation to merge).

EIPs should be technical specifications, not discussions or history of discussions. It seems that the only client change in this EIP is to adjust the gas cost of the QGUADDIVSOR precompile, and that should be the focus of the simple summary, abstract, and specification. The motivation and rationale can go into why this change is a necessary and why you decided on the number 3, but they are not what this specification is about so they should be left out of the specification section.

---

Suggested Simple Summary:

Reduce gas cost of QGUADDIVSOR.

Suggested Abstract:

Reduce the gas cost of QGUADDIVSOR to 3.

Suggested Motivation:

[keep as is]

Suggested Specification:

As of FORK_BLOCK_NUMBER, set the gas cost of the QGUADDIVSOR precompile at address 0x0000000000000000000000000000000000000005 to 3.

Suggested Rationale:

[Go into all of the details you currently have in other sections about how you calculated the cost of the precompile, how it compares to other operations, etc. Don't include options 3 or 4 or mention "options", instead preferring to discuss the EIP as an assertion of a change rather than the suggestion of a change]

---

Recommend removing the References section. EIP-198 is already a dependency, so there is no need for yet another reference to it.

---

Those links to Google Doc spreadsheets will almost certainly not stand the test of time. Can we inline the data contained in them, or add them as an asset in ../assets/eip-2565/? In general we try to avoid any external links in EIPs and instead prefer data stored in this file or repository.

[axic]

There is a push to remove the Accepted status as it is misleading and does not tell the entire story.

Besides that this EIP still has options. I do not think something "final" can have options in the specification section. It has to be clear what is being proposed.

[holiman]

Even leaving the options aside, the EIP as written is counter-factual. The proposed change (lowering gquaddivisor) actually creates the opposite of the desired effect. So if it's final, and final can't be touched, then we're in a silly spot

[ineffectualproperty]

Hi @holiman - I saw your feedback you posted on the EthMagicians forum and will work to incorporate it. I agree that the EIP is unclear with the various options in it as it stands and will incorporate the feedback from you, @MicahZoltu, @axic , and others to make it clear (including updates test vectors in an appendix). I also recommend that we keep it in Last Call once those changes are pushed to give us additional time to provide further suggested changes as the EIP is integrated and deployed in YOLOv2. You are correct that if GQUADDIVISOR is the only change that is made then this would be a cost increase. The above suggestions from @MicahZoltu don't fully encapsulate the changes that are required so I will make sure that it is clarified and ping you once the EIP is updated. Thanks for all of your feedback as this is my first EIP so it has been a learning process as we have moved from evaluation various proposed options to finalizing the spec-focused change.

[ineffectualproperty]

Hi @MicahZoltu - I've update the EIP to make it more specification driven and hopefully clarify the proposed changes. I've also incorporated feedback provided by @holiman in #2955 and on 'ethereum-magicians'. Please let me know if anything remains unclear or could be improved.

[MicahZoltu]

My feedback still remains largely the same as my previous review. The vast majority of the text you have in this EIP currently should be moved to the Motivation and Rationale section. As a specification this EIP is incredibly simple. "Set gas cost of precompile X to Y". Motivation is a great place to go into details as to why you think this change is a good idea broadly speaking, and Rationale section is a great place to discuss the various options you considered and your methodology for coming up with Y.

The Simple Summary, Abstract, and Specification sections though should really focus solely on the specification itself and not discuss any of the other things.

Also, keep in mind that EIPs don't make suggestions, they make assertions. People can choose to follow the EIP or not if they want, but the EIP should be written to target people who are following it and trying to implement.

Something to keep in mind is that the mechanism by which you calculated the new gas cost is not part of the specification. It is very reasonable to include that in the rationale section, but it isn't appropriate for the summary/abstract/specification section.

Also keep in mind that EIPs that change gas prices generally don't have hard dependencies on historic gas prices. While the gas cost of precompile X may be Y right now on Ethereum mainnet, over on Ethereum Classic or some testnet it may have been Z. This specification doesn't care about the historic gas price of the precompile, so it shouldn't mention it in the specification. It may be relevant in the motivation or rationale section, but generally speaking we want EIPs to specify single moments in time without care for what came before or what may come after.

[holiman]

If you want to, you can also add an "Implementations" section, pointing to ethereum/go-ethereum#21607 which implements this

[holiman]

I also agree with the general comments that @MicahZoltu made

[ineffectualproperty]

@MicahZoltu - I moved more of the text to the Rationale section and tightened up the specification. @holiman I added a section to include the Geth implementation. Please let me know if there is any thing else you'd like me to change.

[MicahZoltu]

@ineffectualproperty I created a PR to hopefully better express what I was getting at since going back and forth is proving to be a slow process. 😄 In the process, I also learned that this change is more complicated than I originally thought, I didn't realize that the ModExp precompile didn't have a fixed cost, which was greatly confusing the conversation above and I apologize for that.

[ineffectualproperty]

Hey @MicahZoltu . Thanks for creating the PR! No worries on the confusion, I will review and incorporate your suggestions shortly.

[ineffectualproperty]

Hey @MicahZoltu - I merged your PR and fixed the TODOs. I also added a link to executable Python that shows both the EIP-198 and EIP-2565 pricing. Please let me know if you have any more feedback.

[MicahZoltu]

Suggest changing the title to:

title: ModExp Gas Cost

This is looking good! A couple minor suggestions below and the title suggestion above is all that I think is required to merge this as Last Call. I would still really like to see the images updated with the legend label wording change I mentioned, but if that is particularly difficult I can accept it as-is.

[ineffectualproperty]

@MicahZoltu - Merged your suggested changes, updated the EIP title, and fixed the image titles/legends. Thanks once again for all the feedback!

[MicahZoltu]

@ineffectualproperty somehow there is a merge conflict. I'm guessing you made some changes directly that were merged and are now in conflict?

[ineffectualproperty]

@MicahZoltu - resolved the merge conflict.

[holiman]

I realize I'm a bit late here, but @karalabe reacted and I agree, about this portion:

def calculate_multiplication_complexity(base_length, modulus_length):
    max_length = max(base_length, modulus_length)
    words = math.ceil(max_length / 8)
    return words**2

def calculate_iteration_count(exponent_length, exponent):
    iteration_count = 0
    if exponent_length <= 32 and exponent == 0: iteration_count = 0
    elif exponent_length <= 32: iteration_count = exponent.bit_length() - 1
    elif exponent_length > 32: iteration_count = (8 * (exponent_length - 32)) + ((exponent & (2**256 - 1)).bit_length() - 1)
    return max(iteration_count, 1)

def calculate_gas_cost(base_length, modulus_length, exponent_length, exponent):
    multiplication_complexity = calculate_multiplication_complexity(base_length, modulus_length)
    iteration_count = calculate_iteration_count(exponent_length, exponent)
    return max(200, math.floor(multiplication_complexity * iteration_count / 3))

That chunk of code is not present in the original EIP-98 [https://eips.ethereum.org/EIPS/eip-198]. So on a 'casual read', it's not apparent that EIP-2565 only changes three specific things in the original, and indeed, even when reading the two side by side, it's not apparent that they are identical.

I would prefer it if the same description was used, as was used in the original EIP. I know that @MicahZoltu objected to phrasing it like "this is the original: ... and this EIP changes X, Y and Z as follows" -- but from a client implementors perspective, it's definitely very useful to know the relation between the two.

As I see it, the EIP-2565 spec now contains two definitions, the one above, and the later part, which describes the three changes to the original formula. Has anyone verified (as in, formally verified, or fuzztest-verified) that the two descriptions are identical?

[holiman]

I would prefer if this EIP was rewritten to use the same form as EIP-98:

ADJUSTED_EXPONENT_LENGTH is defined as follows.

    If length_of_EXPONENT <= 32, and all bits in EXPONENT are 0, return 0
    If length_of_EXPONENT <= 32, then return the index of the highest bit in EXPONENT (eg. 1 -> 0, 2 -> 1, 3 -> 1, 255 -> 7, 256 -> 8).
    If length_of_EXPONENT > 32, then return 8 * (length_of_EXPONENT - 32) plus the index of the highest bit in the first 32 bytes of EXPONENT (eg. if EXPONENT = \x00\x00\x01\x00.....\x00, with one hundred bytes, then the result is 8 * (100 - 32) + 253 = 797). If all of the first 32 bytes of EXPONENT are zero, return exactly 8 * (length_of_EXPONENT - 32).

[MicahZoltu]

I would prefer if this EIP was rewritten to use the same form as EIP-98:

from a client implementors perspective, it's definitely very useful to know the relation between the two.

I believe this comes back to the question of whether EIPs should be written for future developers or current developers. As I'm sure many are aware, I'm an advocate that EIPs should be written for future developers. I look up RFCs all the time decades after they were written, and if they were written targeting an audience of "people who were around and reading them when the RFC was written" they would be nearly unintelligible for me without a bunch of digging through history and following the chain of changes.

I would prefer it if the same description was used, as was used in the original EIP.

@holiman Would you be similarly satisfied if we made a non-normative change to EIP-98 to have it use similar syntax as this EIP for its specification? I'm trying to understand if the root of the issue here is in the syntactic choice, or just ensuring that both EIPs use the same syntactic choices.

[holiman]

I'm trying to understand if the root of the issue here is in the syntactic choice, or just ensuring that both EIPs use the same syntactic choices.

@MicahZoltu I'm worried about whether the two descriptions are in absolute agreement. So I would prefer if the original description from EIP-98 was used.

[tkstanczak]

Hi, my intuition is that the complexity of the calculation is defined by the last 32 bits more than the first 32 bits, hence the more granular calculation there? (length -32) * 8 (ignoring zeros) + last_32_very detailed.

Now I also think that maybe the author felt that the last n - 32 bits may be assumed to be filled and we do not care about bits set so much but for the first 32 bits it defines the number of iterations much more...

Would be great to confirm with authors as the spec is not entirely clear here but it may have consequence for potential attacks?

I have changed it on nethermind to match Geth's calculation (which may or may not be correct interpretation of the '&' logic in the spec.

[holiman]

I still think this EIP should be updated to be more clear about the formula. Both @tkstanczak, @karalabe and myself found it very hard to read, as opposed to the earlier phrasing from EIP-198.

[ineffectualproperty]

I can revert @MicahZoltu 's suggested changes to the specification so that is consistent with EIP-198 in the next day or two which should hopefully clear up any confusion

[MicahZoltu]

This discussion should really be happening over in the discussions-to link so anyone who has subscribed to that discussion can see it, rather than only the people who were actively involved in this PR (which I believe is just the 3 of us).