EIP 1108: Reduce alt_bn128 precompile gas costs #11008

niklasad1 · 2019-08-29T15:11:01Z

Attempt to close #10998 and implement https://eips.ethereum.org/EIPS/eip-1108

Warning, this is not an elegant PR because ethcore/builtins reads the values directly from JSON and I tried to implement it as best effort.

If this is good enough, I will update the chain spec accordingly

niklasad1 · 2019-08-29T15:39:59Z

ethcore/builtin/src/lib.rs

+				Box::new(AltBn128ConstOperations {
+						price: pricer.price,
+						eip1108_transition_price: pricer.eip1108_transition_price,
+						eip1108_transition_at: b.eip1108_transition.map_or(u64::max_value(), Into::into)


deny-by-default

niklasad1 · 2019-08-29T15:40:10Z

ethcore/builtin/src/lib.rs

+						base: pricer.base,
+						pair: pricer.pair,
+					},
+					eip1108_transition_at: b.eip1108_transition.map_or(u64::max_value(), Into::into),


deny-by-default

dvdplm

minor comments&feedback

ethcore/builtin/src/lib.rs

ethcore/types/src/engines/params.rs

json/src/spec/builtin.rs

dvdplm · 2019-08-29T20:08:09Z

If this is good enough, I will update the chain spec accordingly

Can you elaborate on this? What further changes to the chainspec do we need?

As for "good enough" I'm interested in hearing if Wei has a better idea. I think juggling these transitions is always a bit inelegant but maybe he has a better approach.

@niklasad1 how would you refactor the Pricer trait to avoid the extra param?

sorpaas · 2019-08-29T20:25:10Z

@dvdplm A better approach I can think of maybe something like this: we define a new type builtinMulti (as a field in json::Account). It takes a hashmap, where keys are block number, and values are Builtin. We then activate/deactivate builtins based on the block number. The resulting config will look something like this:

"accounts": {
  "0000000000000000000000000000000000000008": { "builtinMulti": {
    "0": { "name": "alt_bn128_pairing", "activate_at": 2000000, "pricing": { "alt_bn128_pairing": { "base": 100000, "pair": 80000 } } } },
    "12345678": { "name": "alt_bn128_pairing", "activate_at": 2000000, "pricing": { "alt_bn128_pairing": { "base": 900, "pair": 1000 } } } },
  }
}

However, this will probably take some time to refactor, and given our time constraints, I think @niklasad1's approach should be good enough!

niklasad1 · 2019-08-30T07:27:26Z

@dvdplm

Can you elaborate on this? What further changes to the chainspec do we need?

We need to update all other chain specs with the eip1108_transition params

As for "good enough" I'm interested in hearing if Wei has a better idea. I think juggling these transitions is always a bit inelegant but maybe he has a better approach.

Yes, it is not pretty I agree

how would you refactor the Pricer trait to avoid the extra param?

I think it should be possible to revert it completely if we could handle it within JSON (builtinMulti as Wei suggested). Otherwise, I have no good idea without reworking the entire thing.

dvdplm · 2019-08-30T14:12:30Z

update all other chain specs with the eip1108_transition params

Right, of course. Ok, overall lgtm. Holler when you're done with this and I'll give it a once-over again. :)

niklasad1 · 2019-08-30T19:38:48Z

I'm done 👍

dvdplm · 2019-08-30T20:07:10Z

ethcore/builtin/src/lib.rs

@@ -38,9 +38,11 @@ trait Implementation: Send + Sync {
 }

 /// A gas pricing scheme for built-in contracts.
+// NOTE(niklasad1): maybe refactor this trait because only `bn128_operations` are using


Please add an issue and include a link here.

Co-Authored-By: David <dvdplm@gmail.com>

dvdplm · 2019-09-02T09:05:40Z

@sorpaas looks good to you? Let's merge this! :)

* master: EIP 1108: Reduce alt_bn128 precompile gas costs (openethereum#11008) Fix deadlock in `network-devp2p` (openethereum#11013)

* master: EIP 1108: Reduce alt_bn128 precompile gas costs (#11008) Fix deadlock in `network-devp2p` (#11013) Implement EIP-1283 reenable transition, EIP-1706 and EIP-2200 (#10191)

…he-right-place * master: Extract snapshot to own crate (#11010) Edit publish-onchain.sh to use https (#11016) EIP 1108: Reduce alt_bn128 precompile gas costs (#11008) Fix deadlock in `network-devp2p` (#11013) Implement EIP-1283 reenable transition, EIP-1706 and EIP-2200 (#10191) EIP 1884 Re-pricing of trie-size dependent operations (#10992)

holiman · 2019-09-06T06:50:45Z

Wow, nice job on the PR number; 11008 for 1108!

dvdplm · 2019-09-06T07:08:31Z

Glad someone finally noticed! ;)

* feat: implement eip1108 * doc nit: price per point pair Co-Authored-By: David <dvdplm@gmail.com> * fix: test-build * fix: update chain specs * fix: basic_authority.json chain spec * fix: change `eip_transition == 0x7fffffffffffff` * add issue link to `TODO`

* add more tx tests (#11038) * Fix parallel transactions race-condition (#10995) * Add blake2_f precompile (#11017) * [trace] introduce trace failed to Ext (#11019) * Edit publish-onchain.sh to use https (#11016) * Fix deadlock in network-devp2p (#11013) * EIP 1108: Reduce alt_bn128 precompile gas costs (#11008) * xDai chain support and nodes list update (#10989) * EIP 2028: transaction gas lowered from 68 to 16 (#10987) * EIP-1344 Add CHAINID op-code (#10983) * manual publish jobs for releases, no changes for nightlies (#10977) * [blooms-db] Fix benchmarks (#10974) * Verify transaction against its block during import (#10954) * Better error message for rpc gas price errors (#10931) * Fix fork choice (#10837) * Fix compilation on recent nightlies (#10991)

* add more tx tests (#11038) * Fix parallel transactions race-condition (#10995) * Add blake2_f precompile (#11017) * [trace] introduce trace failed to Ext (#11019) * Edit publish-onchain.sh to use https (#11016) * Fix deadlock in network-devp2p (#11013) * EIP 1108: Reduce alt_bn128 precompile gas costs (#11008) * xDai chain support and nodes list update (#10989) * EIP 2028: transaction gas lowered from 68 to 16 (#10987) * EIP-1344 Add CHAINID op-code (#10983) * manual publish jobs for releases, no changes for nightlies (#10977) * [blooms-db] Fix benchmarks (#10974) * Verify transaction against its block during import (#10954) * Better error message for rpc gas price errors (#10931) * tx-pool: accept local tx with higher gas price when pool full (#10901) * Fix fork choice (#10837) * Cleanup unused vm dependencies (#10787) * Fix compilation on recent nightlies (#10991)

phahulin · 2019-09-16T14:20:44Z

In future releases it might be a good idea to provide instructions about required updates in chainspec or other breaking changes in Changelog, for those who run custom networks

dvdplm · 2019-09-16T19:13:39Z

@phahulin I agree, we did not handle that optimally. Apologies. OTOH it's non-trivial to keep track of exactly what changes affect which (advanced) users; do you have any concrete suggestions for us, other than writing more detailed changelogs?

niklasad1 force-pushed the na-eip1108 branch 2 times, most recently from b045765 to 7bf1f51 Compare August 29, 2019 15:17

niklasad1 commented Aug 29, 2019

View reviewed changes

niklasad1 marked this pull request as ready for review August 29, 2019 15:41

niklasad1 requested review from sorpaas and dvdplm August 29, 2019 15:41

niklasad1 added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Aug 29, 2019

dvdplm reviewed Aug 29, 2019

View reviewed changes

ethcore/builtin/src/lib.rs Show resolved Hide resolved

ethcore/types/src/engines/params.rs Outdated Show resolved Hide resolved

json/src/spec/builtin.rs Outdated Show resolved Hide resolved

dvdplm approved these changes Aug 30, 2019

View reviewed changes

niklasad1 mentioned this pull request Aug 31, 2019

ethcore/builtins: refactor Pricer trait and introduce JSON Address: builtinMulti #11014

Closed

niklasad1 and others added 7 commits August 31, 2019 10:31

feat: implement eip1108

674f8c7

doc nit: price per point pair

c9afedb

Co-Authored-By: David <dvdplm@gmail.com>

fix: test-build

44b2644

fix: update chain specs

f88d313

fix: basic_authority.json chain spec

593c8ba

fix: change eip_transition == 0x7fffffffffffff

011073c

add issue link to TODO

148235d

niklasad1 force-pushed the na-eip1108 branch from 0e8ae07 to 148235d Compare August 31, 2019 08:31

sorpaas approved these changes Sep 2, 2019

View reviewed changes

sorpaas merged commit a9cb572 into master Sep 2, 2019

sorpaas deleted the na-eip1108 branch September 2, 2019 09:38

ordian added this to the 2.7 milestone Sep 2, 2019

ordian added A8-looksgood 🦄 Pull request is reviewed well. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 2, 2019

ordian added a commit to ordian/parity that referenced this pull request Sep 2, 2019

Merge branch 'master' into ao-move-memdiff-to-trace-executed

afb70cd

* master: EIP 1108: Reduce alt_bn128 precompile gas costs (openethereum#11008) Fix deadlock in `network-devp2p` (openethereum#11013)

Shadowfiend mentioned this pull request Sep 3, 2019

EIP-1108 PRs on Parity and go-ethereum keep-network/keep-core#963

Closed

2 tasks

This was referenced Sep 12, 2019

v2.6.3-beta #11040

Merged

v2.5.8-stable #11041

Merged

niklasad1 mentioned this pull request Sep 19, 2019

[chainspecs]: make eip1108 optional #11065

Closed

niklasad1 mentioned this pull request Oct 7, 2019

[builtin]: support multiple prices and activations in chain spec #11039

Merged

niklasad1 mentioned this pull request Nov 1, 2019

[chain specs]: activate Istanbul on mainnet #11228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP 1108: Reduce alt_bn128 precompile gas costs #11008

EIP 1108: Reduce alt_bn128 precompile gas costs #11008

niklasad1 commented Aug 29, 2019 •

edited

Loading

niklasad1 Aug 29, 2019

niklasad1 Aug 29, 2019

dvdplm left a comment

dvdplm commented Aug 29, 2019

sorpaas commented Aug 29, 2019

niklasad1 commented Aug 30, 2019

dvdplm commented Aug 30, 2019

niklasad1 commented Aug 30, 2019

dvdplm Aug 30, 2019

dvdplm commented Sep 2, 2019

holiman commented Sep 6, 2019

dvdplm commented Sep 6, 2019

phahulin commented Sep 16, 2019

dvdplm commented Sep 16, 2019

EIP 1108: Reduce alt_bn128 precompile gas costs #11008

EIP 1108: Reduce alt_bn128 precompile gas costs #11008

Conversation

niklasad1 commented Aug 29, 2019 • edited Loading

niklasad1 Aug 29, 2019

Choose a reason for hiding this comment

niklasad1 Aug 29, 2019

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

dvdplm commented Aug 29, 2019

sorpaas commented Aug 29, 2019

niklasad1 commented Aug 30, 2019

dvdplm commented Aug 30, 2019

niklasad1 commented Aug 30, 2019

dvdplm Aug 30, 2019

Choose a reason for hiding this comment

dvdplm commented Sep 2, 2019

holiman commented Sep 6, 2019

dvdplm commented Sep 6, 2019

phahulin commented Sep 16, 2019

dvdplm commented Sep 16, 2019

niklasad1 commented Aug 29, 2019 •

edited

Loading