ProgPoW Review

[gcolvin]

This is a PR for review ahead of All Core Devs call 96.

We Do Not recommend that this Proposal be deployed at this time. Rather it is being offered in the spirit of Ben DiFrancesco's compromise which for our purposes we state simply as,

• This Proposal is not being proposed for deployment in any planned hardfork.
• This Proposal should be fully implemented and tested across major clients.
• Clients implementing this Proposal should be deployed and maintained on a testnet.
• This leaves open the possibility and threat of future deployment.

Incorporates @AndreaLanfranchi's implementation of the Kik exploit fix and a start at @bitsbetrippin client, exchange, and pool tracking tables.

The reference implementation is on @ifdefelse's repo at https://github.com/ifdefelse/ProgPOW

[MicahZoltu]

Recommend removing all but the first sentence of the Simple Summary. EIPs are technical standards, and the rest of the simple summary is not appropriate for a standard. Consider moving that text over to the discussion-to link.

Some of the motivation section feels like it will end up out of date or is speculative in nature, and not a good fit for a technical document (for example, the bullet points in the opening portion of the Motivation section). Recommend removing that section and just letting the technical merits of ProgPoW compared to other attempts speak for itself.

The specification section talks about different versions of ProgPoW it appears, but an EIP specification should specify one thing only, not multiple versions of a thing. From the looks of it, version 0.9.4 is the latest and that is what this EIP should specify without mention of previous iterations. If we want multiple "versions", then we should have multiple EIPs.

A lot of the specification section should probably be pruned and moved out to the discussion-to link or some working document. Things like discussion of audits that ProgPoW has undergone, iterations that have been through, testnets that have been deployed to, etc. all isn't something someone implementing ProgPoW from this spec in 10 years is going to care about. The audits specifically maybe could be added as a footnote to the EIP as they do hold future value, but they should be included in the ../assets folder and not as external links if possible.

Some portions of the specification section should be moved to the Rationale section, like the reasoning for the keccak input state being to protect against a particular class of attack.

Missing Security Considerations section, which may also be a good place to discuss the keccak input state thing.

The Example/Test Cases section doesn't actually have any examples or test cases and instead seems to have stuff more appropriate for rationale or motivation (depending on how it is worded).

Recommend removing all but the test vectors for the version this EIP specifies (details about earlier iterations aren't appropriate for a technical standard).

The implementation section opening lines (before the implementation table) should be removed and put in the discussion-to link. As has been discussede lately in EIPIP meetings, EIPs are not the place to discuss mainnet operations such as fork inclusion.

In the implementation section, I'm not sure what it means for an exchange or a pool to be "ready". Either they run the latest version of a client or they don't. These lists feel like some sort of politicing (I'm not sure to what end) so I recommend removing them and perhaps including them in the discussion-to link with some more context as to what that means.

[gcolvin]

Thanks for your thorough review, @MicahZoltu.

This EIP has been through a lot since it was first proposed in 2018, and non-technical issues have loomed larger than technical ones. So I'd rather leave the Simple Summary and Motivation sections be. The Simple Summary is what we are proposing upfront that the Core Devs do now, and needs to somewhere. And the Motivation is why we propose to do it now -- if we don't it will soon enough it be irrelevant.

The Specification should mostly be pruned as you suggest - the historical information can be found in the @ifdefelse repo. The material on the audits, the Kik exploits, and such should remain in the document - probably as Security Considerations - as they are important to those who have been following the proposal and want to judge whether changes based on the audits and Kik exploit are sound. I'm not sure whether copyright issues allow us to move the audit documents to ../assets.

I've reduced the Example/Test Cases section down to just the Test Cases for this latest version.

The tables in the Implementation section are meant for Michael Carter (@bitsbetrippin) to track the status of the EIP. I'll leave it to him to better explain their meaning -- but as I understand it they show that months ago most clients had implemented and tested the software and the pools and exchanges were ready to fork to it. A political statement, perhaps. I'll leave it to Michael and James Hancock (MadeofTin) to decide if they belong here or elsewhere. And the opening lines of the Implementation section are the same ones that you want removed from the Simple Summary. For most any other EIP I'd agree, but this is not an ordinary EIP. I have moved both these lines and the table to a non-canonical Deployment section for now.

[Souptacular]

I'm torn on whether or not political/historical information needs to be included in the EIP. I want to defer to the other editors, but I'm not sure how many other editors are going to see this (maybe @axic?). I'm leaning towards keeping in the parts about the history and deployment information since it's a net positive to have that information centralized so people don't have to scroll through Git commits and EthMag's forum posts to find the latest.

[lightclient]

Not an editor, but I also feel strongly that Standards Track EIPs should not include political/historical information. I would, however, welcome an informational EIP with this information with open arms.

[gcolvin]

I hear and share all apprehensions. These notes on deployment and history can be moved elsewhere as appropriate before the EIP is Final. This PR needs them so that reviewers can understand the EIP in context. There are many important readers in the community who cannot be expected to study Git repos and Magician's threads.

[lightclient]

@gcolvin this is still a draft PR, are you still working on this or are you waiting for it to be merged? It looks like this should be addressed at minimum: #2894 (comment).

[gcolvin]

@lightclient Yes, it's a Draft PR -- cannot be merged, still being worked on. The typo that @chfast caught is fixed downstream.

[gcolvin]

@MicahZoltu @Souptacular

@AndreaLanfranchi is healthy and looking at the Kik exploit again, and it's becoming the consensus of the authors that it and similar exploits are not at all practical to execute in the real world. Andrea can explain better. That leaves our proposal much simpler, with no technical changes since the audits. The Kik fix and light-evaluation mitigation will still need to be in the security section, but not in so much detail. Ethash is also vulnerable to the light-evaluation attack, so mitigating it a topic for separate discussion. For ProgPoW changing one constant is all that is required.

[MicahZoltu]

@gcolvin I believe we are all waiting for this to move out of draft before dedicating time to really giving it a deep dive. Also, I doubt my stance is going to change much, but others may have different views on the matter.

The motivation section also feels too long. As hinted previously, I think most of that should be moved to a discussion link. Ideally, the motivation section would give a high level overview of why someone may want to use this EIP but it doesn't need to go into extensive detail about all of the arguments for/against the EIP, all of the possible alternatives, etc. The EIP should really just be a technical standard and the purpose of the motivation section is to give a little bit of context to future readers, not a history lesson.

[MicahZoltu]

It is worth noting that there is now the https://github.com/ethereum/eth1.0-specs/ repository where it is more appropriate to discuss "what is going to go into a hard fork". A large amount of what is in this EIP I think may be more appropriate there.

[AndreaLanfranchi]

@gcolvin just wrote a detailed explanation about why Kik's finding is impossible to exploit and all the noise around has no foundation.

[MicahZoltu]

To be clear to readers who may not be familiar with the EIP process: I want to merge this EIP as a draft.

However, I am unwilling to go against my standard EIP merging process for this PR which is that EIPs are purely technical standards, not a place to record history, record discussions, have arguments, sell people on the merits of a thing, convince people to use a thing, etc. If this PR was just a technical specification of ProgPoW with a few paragraphs of motivation to give context and a rationale for why certain choices were made over alternatives then I would not push back against merging this at all.

It is worth noting that the other aspect of my standard EIP merging process is to defer to the larger EIP editor group even when I disagree, so if enough other active editors think we should merge this with all of the non-technical-stuff in it, then I'll go along with the group.

[gcolvin]

@MicahZoltu @Souptacular We authors have come to consensus that the Kik exploit is not just impractical to deploy, but impossible. And we can devise a similar vulnerability that avoids Kik's mistake, but is uses an even more impractical amount of resources. So we backed out the most recent changes, leaving a single specification.

I've moved a brief description of the vulnerabilities with links to full discussions to the Security Considerations section. We also describe fixes, and argue that they should not be deployed.

[gcolvin]

@MicahZoltu

However, I am unwilling to go against my standard EIP merging process for this PR which is that EIPs are purely technical standards, not a place ...

What would we need to change to meet your standard?

[MicahZoltu]

What would we need to change to meet your standard?

Things I would want to see changed to feel comfortable merging this and helping push this EIP through to final:

Remove everything from the Simple Summary except:

A new Proof-of-Work algorithm to replace Ethash that utilizes almost all parts of commodity GPUs.

Remove the versioning stuff in the specification (the table) and just state what this specification specifies. The values could be kept in a separate list as they are now but without the header row and with only one column, or they could just be inlined into the definitions above the table to reduce repetition in the EIP. Also remove the lead-in line for those tables:

The values of these parameters have been tweaked between the original version and the version proposed here for Ethereum adoption. See this medium post for details.

From Security Considerations remove:

We do not recommend implementing this fix at this time.

From Security Considerations remove:

This fix is available as a PR to the reference implementation. Again, we do not recommend implementing this fix at this time. Kik's vulnerability and others like it cannot be exploited now and likely never will be. It's better to deploy the audited code.

Remove the entire Implementation section except the first line.

Change the License and Copyright section to just Copyright (following EIP template) and remove the mention of the license of an externally linked product (that product should include a license file). We don't specify licenses for externally linked content anywhere else in the EIPs repository, and I don't think we should change that policy here or introduce such a policy.

---

It is worth noting that there is an upcoming change to the EIP process that will fully separate it from the Ethereum hardfork inclusion process, which means that this EIP could go all the way to final without ever needing to answer the question of "will this be included in Ethereum mainnet".

[gcolvin]

@MicahZoltu

From ... remove ...

Right. The notes on deployment will either become a recorded ACD Decision soon or else the proposal will be withdrawn -- either way they will be removed from the Draft.

I do think what remains about the Kik exploit is about right. Will look at trimming.

I'll leave the implementation notes to @bitsbetrippin and @MadeofTin.

It is worth noting that there is an upcoming change to the EIP process that ...

Yes. This proposal has endured more than one such...

[gcolvin]

@MicahZoltu I have decided exercise to self-merge this, as Martin is waiting on result, and I think we have resolved as much as we can. I've removed everything but a part of the Kik discussion that I think needs to remain. It was Accepted long ago, so this isn't like this is the first draft to be merge. Thanks again for your assistance!