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

Update EIP-1108 with latest gas metering benchmarks #1987

Merged
merged 8 commits into from May 8, 2019

Conversation

Projects
None yet
6 participants
@zac-williamson
Copy link
Contributor

commented May 3, 2019

When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-X.md

We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met:

  • The PR edits only existing draft PRs.
  • The build passes.
  • Your Github username or email address is listed in the 'author' header of all affected PRs, inside .
  • If matching on email address, the email address is the one publicly listed on your GitHub profile.
@eip-automerger

This comment has been minimized.

Copy link
Collaborator

commented May 3, 2019

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

@zac-williamson zac-williamson force-pushed the AztecProtocol:master branch from 850cb3c to 119027b May 3, 2019

@Shadowfiend
Copy link
Contributor

left a comment

Awesome updates, thanks for adding a great mix of qualitative and quantitative justification!

Not sure what preferences are in this particular repo (@axic, perhaps?) but a couple of things I might tweak before merging:

  • Avoiding first person (we).
  • Clearer commit message, PR title, description.
  • Consistency in references to zk-SNARKs (typically I've seen zk-SNARK as the capitalization/punctuation of choice).

I left some suggested changes for the zk-SNARK and first person thing that should be easy to include. I actually also noticed the original EIP has a first person reference, which I'll probably fix once this PR goes in.

I'll give the EIP folks a day (or maybe the weekend <_<) to speak up if anything else needs tweaking and then probably merge in whatever is here.

Show resolved Hide resolved EIPS/eip-1108.md Outdated
Show resolved Hide resolved EIPS/eip-1108.md Outdated
Show resolved Hide resolved EIPS/eip-1108.md Outdated
Show resolved Hide resolved EIPS/eip-1108.md Outdated
@zac-williamson

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Heya @Shadowfiend

Thanks for the feedback! I've committed your changes, let me know if you have any other suggestions.

@zac-williamson zac-williamson changed the title updated EIP-1108 Update EIP-1108 with latest gas metering benchmarks May 3, 2019

zac-williamson and others added some commits May 3, 2019

EIP-1108 references latest pairing benchmarks
- Several optimizations have been made to the Parity bn create
- EIP-1108 references benchmarks that utilize this optimization
- Projected gas costs have been updated to reflect benchmark
- (benchmark at https://gist.github.com/zac-williamson/838419a3da179d47d31b25b586c15e53)
- Added descriptions of projects that would benefit from EIP-1108, and how they could utilize this EIP
Update EIPS/eip-1108.md
Co-Authored-By: zac-williamson <blorktronics@gmail.com>
Update EIPS/eip-1108.md
Co-Authored-By: zac-williamson <blorktronics@gmail.com>
Update EIPS/eip-1108.md
Co-Authored-By: zac-williamson <blorktronics@gmail.com>
Update EIPS/eip-1108.md
Co-Authored-By: zac-williamson <blorktronics@gmail.com>

@zac-williamson zac-williamson force-pushed the AztecProtocol:master branch from 5df7705 to 005e3f2 May 3, 2019

@zac-williamson

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

I've updated the PR and the commit message to be more descriptive of the changes in this PR

used by the [Parity client](https://github.com/paritytech/parity-ethereum) led to a
significant performance boost we
[benchmarked](https://gist.github.com/pdyraga/4649b74436940a01e8221d85e80bfeef)
and compared against the [previous
results](https://github.com/ethereum/benchmarking/blob/master/constantinople/analysis2.md).
results](https://gist.github.com/zac-williamson/838410a3da179d47d31b25b586c15e53).

This comment has been minimized.

Copy link
@Shadowfiend

Shadowfiend May 3, 2019

Contributor

Oh actually I think the link here is flipped---the [benchmarked] link needs to change, while the [previous results] link should continue to reflect the original analysis that set the current gas price.

This comment has been minimized.

Copy link
@zac-williamson

zac-williamson May 4, 2019

Author Contributor

Thanks for spotting that! I switched the links over


Fast elliptic curve cryptography is a keystone of a growing number of protocols built on top of Ethereum. To list a few:

[The AZTEC protocol](https://github.com/AztecProtocol/AZTEC) utilizes the elliptic curve precompiles to construct private tokens, with zero-knowledge transaction logic, via the [ERC1723](https://github.com/ethereum/EIPs/issues/1723) and [ERC1724](https://github.com/ethereum/EIPs/issues/1724) standard.

This comment has been minimized.

Copy link
@axic

axic May 3, 2019

Member

Can you make these into a bullet point list? It would look nicer that way after formatting.

This comment has been minimized.

Copy link
@zac-williamson

zac-williamson May 4, 2019

Author Contributor

Done

[The AZTEC protocol](https://github.com/AztecProtocol/AZTEC) utilizes the elliptic curve precompiles to construct private tokens, with zero-knowledge transaction logic, via the [ERC1723](https://github.com/ethereum/EIPs/issues/1723) and [ERC1724](https://github.com/ethereum/EIPs/issues/1724) standard.
[Matter Labs](https://github.com/matter-labs/matter-network) utilizes the precompiles to implement Ignis, a scaling solution with a throughput of 500txns per second
[Rollup](https://github.com/rollup/rollup) utilizes the precompiles to create L2 scaling solutions, where the correctness of transactions is gauranteed by main-net, without an additional consensus layer
[ZEther](https://crypto.stanford.edu/~buenz/papers/zether.pdf) uses precompiles `0x06` and `0x07` to construct confidential transactions

This comment has been minimized.

Copy link
@axic

axic May 3, 2019

Member

This is nitpicking, but above you define what precompiles the addresses 0x6 and 0x7 refer to. Maybe it is easier to read if you use ECADD and ECMUL here.

This comment has been minimized.

Copy link
@zac-williamson

zac-williamson May 4, 2019

Author Contributor

Pushed a change that uses ECADD and ECMUL


The gas costs for `ECADD` and `ECMUL` are updates to the costs listed in
EIP-196, while the gas costs for the pairing check are updates to the cost
listed in EIP-197. Updated gas costs have been adjusted to the less performant
client which is Parity, according to benchmarks<sup>[3]</sup>.
client which is Parity, according to benchmarks<sup>[3]</sup>. The updated gas costs are scaled relative to the `ecrecover` precompile.

This comment has been minimized.

Copy link
@axic

axic May 3, 2019

Member

Can you explain how it is relative to ecrecover?

This comment has been minimized.

Copy link
@zac-williamson

zac-williamson May 4, 2019

Author Contributor

I pushed a change that describes the ecrecover scaling. The ecrecover precompile took 116 microseconds to run in the benchmark. If we assume the ecrecover gas price of 3,000 gas is a fair price, we get a price of 25.86 gas per microsecond of an precompile algorithm's run-time.

The pairing precompile took 3,037 microseconds to compute 1 pairing, and 14,663 microseconds to compute 10 pairings. From this, the pairing algorithm has a fixed 'base' run-time of 1,745 microseconds, plus 1,292 microseconds per pairing. We can split the run-time into 'fixed cost' and 'linear cost per pairing' components because of the structure of the algorithm.

Then using a 'fair' price of 25.86 gas per microsecond, we get a gas formula of ~35,000 * k + 45,000 gas, where k is the number of pairings being computed.

n.b. of course, I am also making an explicit assumption that the run-time ratios between ecrecover and the pairing precompile are constant irrespective of the specs or architecture of the underlying machine. I think this is a reasonable assumption, as the arithmetic operations being performed are extremely similar (modular multiplications in 256 bit finite fields). However more benchmarks would be useful here.

This comment has been minimized.

Copy link
@axic

axic May 9, 2019

Member

@zac-williamson @Shadowfiend I think it would be nice to include the above text in the EIP. Can you add it?

This comment has been minimized.

Copy link
@Shadowfiend

Shadowfiend May 9, 2019

Contributor

I believe it's in there, right? @ https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1108.md#specification, ‘i.e. in the benchmark…’

This comment has been minimized.

Copy link
@axic

axic May 9, 2019

Member

Some of it is, but this is a more comprehensive description.

results](https://github.com/ethereum/benchmarking/blob/master/constantinople/analysis2.md).
results](https://gist.github.com/zac-williamson/838410a3da179d47d31b25b586c15e53).

## The Rationale Behind Lowering Gas Costs

This comment has been minimized.

Copy link
@axic

axic May 3, 2019

Member

There is a standard "rationale" section in EIPs. Also it seems the layout of this EIP follows an outdated old template. Have a look at the current one: https://github.com/ethereum/EIPs/blob/master/eip-X.md

This comment has been minimized.

Copy link
@zac-williamson

zac-williamson May 4, 2019

Author Contributor

Heya, thanks for pointing that out. I've pushed a change that uses the eip-X formatting

@axic

This comment has been minimized.

Copy link
Member

commented May 3, 2019

Oh, and please add adiscussions-to: field in the header. Usually authors use ethereum-magicians as a forum link. You could use https://ethereum-magicians.org/t/eip-1108-reduce-alt-bn128-precompile-gas-costs/3206 or create a new one. A github issue is also sufficient.

zac-williamson added some commits May 4, 2019

EIP-1108 - updated formatting
The formatting of the EIP has been modified to mirror EIP-X

Benchmarks referenced the wrong links, these have been swapped

Added example implementations of the bn128 curve

Expanded rationale behind scaling gas costs relative to `ecrecover`
@zac-williamson

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

Oh, and please add adiscussions-to: field in the header. Usually authors use ethereum-magicians as a forum link. You could use https://ethereum-magicians.org/t/eip-1108-reduce-alt-bn128-precompile-gas-costs/3206 or create a new one. A github issue is also sufficient.

Thanks for pointing that out, I've added a discussions-to field

EIP-196, while the gas costs for the pairing check are updates to the cost
listed in EIP-197. Updated gas costs have been adjusted to the less performant
client which is Parity, according to benchmarks<sup>[3]</sup>. The updated gas costs are scaled relative to the `ecrecover` precompile.
As no underlying algorithms are not being changed, there are no additional test cases to specify.

This comment has been minimized.

Copy link
@Shadowfiend

Shadowfiend May 6, 2019

Contributor

“As no underlying algorithms are being* changed.”, or we're changing ALL OF THE THINGS :)

This comment has been minimized.

Copy link
@tvanepps

tvanepps May 9, 2019

“As no underlying algorithms are being* changed.”, or we're changing ALL OF THE THINGS :)

Came here to point that language out =)

This comment has been minimized.

Copy link
@Shadowfiend

Shadowfiend May 9, 2019

Contributor

I'll push a PR that fixes it in a few.

@Shadowfiend

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Nitpick up there aside, anything preventing this from going in @axic? Want to help get #1988 in soon as well.

@Arachnid

This comment has been minimized.

Copy link
Collaborator

commented May 7, 2019

@Shadowfiend If you approve the PR, the bot will automatically merge it.

@Shadowfiend

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

Yep yep; since you had a lot more notes than I did I wanted to double-check before doing that :) I'm on it!

@Shadowfiend
Copy link
Contributor

left a comment

Let's rolllllll 🚀

@eip-automerger eip-automerger merged commit 44073f8 into ethereum:master May 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@axic

This comment has been minimized.

Copy link
Member

commented May 22, 2019

@Shadowfiend @zac-williamson this EIP doesn't have a copyright statement. Can you please add one? EIP-1 suggests CC0 - see the eip-x.md template.

@Shadowfiend

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

Opened #2067 to address both of the outstanding comments here @axic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.