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

niklasad1 · 2019-09-12T14:25:27Z

This PR tries to close #11014 and close #11063

To summarize the important change with this PR is that the chain spec format for builtins which support old format (with single pricing/activation) and new format as map to support multiple pricing/activations follows:

"builtin": {
    "name": "single_activation_old_format",
    "pricing": { "linear": {"base": 60, "word": 12 } },
     "activate_at": 0
}

"builtin": {
    "name": "multi_prices_and_activation",
    "pricing": {
        "4370000": {
            "info": "Hardfork X enable EIP ZZ",
            "price": {
                "linear": {
                        "base": 500,
                        "word": 0
                }
            }
        },
        "36028797018963967": { 
           "info": "Hardfork Y enable EIP ZZ", 
            "price": {
                "linear": {
                        "base": 150,
                        "word": 0
                }
            }
        }
    }     
}

dvdplm · 2019-09-13T07:09:33Z

Would something like this work?

"builtin": {
	"name": "single_activation",
	"pricing": {
		"linear": {"base": 60, "word": 12 }, "activate_at": 87
	}
}
"builtin": {
	"name": "multi_prices_and_activation",
	"pricing": [
		{ "linear": {"base": 60, "word": 12 }, "activate_at": 123 },
		{ "modexp": {"divisor": 3.14 }, "activate_at": 456123 }
	]
}

niklasad1 · 2019-09-13T07:35:28Z

Would something like this work?

Yes, except the floating-point/fraction part. I'm not really sure if it makes sense to change price model on the same builtin but I guess it is more future-proof than just changing values if it is needed.

dvdplm · 2019-09-13T08:44:40Z

makes sense to change price model on the same builtin but I guess it is more future-proof than just changing values if it is needed.

It is most likely completely non-sensical, but yeah, the world is a crazy place!

ethcore/builtin/src/lib.rs

json/src/spec/spec.rs

ethcore/res/ethereum/classic.json

ethcore/builtin/src/lib.rs

dvdplm

Good stuff here, thank you for taking it on.
On the json formatting side I think we should be using hex consistently. I have some comments/questions around the new Single/Multi.

ethcore/res/authority_round.json

ethcore/res/authority_round_block_reward_contract.json

ethcore/res/ethereum/callisto.json

json/src/spec/builtin.rs

json/src/spec/spec.rs

json/src/spec/builtin.rs

ethcore/builtin/src/lib.rs

json/src/spec/builtin.rs

ethcore/res/spec_backward_compability.json

ethcore/builtin/src/lib.rs

Have an enum to deserialize either a builtin of a single price or several prices

util/network-devp2p/src/host.rs

ordian

I'm not sure about omitting is_active check in some places, but other than that looks good.

niklasad1 · 2019-10-31T12:40:25Z

I'm not sure about omitting is_active check in some places, but other than that looks good.

Fair enough, I can revert those to be on the safe side, however, machine calls is_active three times instead of one IIRC. I can handle that in another PR that doesn't have to be backported

dvdplm

minor nits

ethcore/machine/src/executive.rs

ethcore/builtin/src/lib.rs

dvdplm · 2019-10-31T13:02:30Z

ethcore/builtin/src/lib.rs

 	pub fn execute(&self, input: &[u8], output: &mut BytesRef) -> Result<(), &'static str> {
 		self.native.execute(input, output)
 	}

 	/// Whether the builtin is activated at the given block number.
+	#[inline]


Just curious: did you measure that these #[inline]s matter or is it hunch-based optimization?

I think those were already inlined thus I didn't see any measurable difference IIRC, do you want the annotations removed?

No, it's fine, I was just curious.

…11039) * [builtin]: impl new builtin type Have an enum to deserialize either a builtin of a single price or several prices * [builtin]: style cleanup * [builtin]: fix tests * [builtin]: replace boxing with wrapper enum * cleanup * fix: make it backward compatible with old builtin * fix: update chain specs * fix: revert use of `type alias` on enum The CI doesn't use the latest rust. This commit reverts that change * fix: builtin tests * fix: revert use of `type alias` on enum * [basic-authority]: update test-chainspec * fix failing tests * [builtin]: multi-prices add `info field` It might be hard to read chain specs with several activations points. This commit introduces a `info` field which may be used to write some information about the current activation such as: `Istanbul hardfork EIP-1108` or something similar. * fix: bad rebase Co-Authored-By: David <dvdplm@gmail.com> * fix(grumbles): make it backward compatible * grumbles: resolve `NOTE` * revert chain specs changes * rename test Co-Authored-By: David <dvdplm@gmail.com> * [builtin docs]: price -> Fixed price Co-Authored-By: Andronik Ordian <write@reusable.software> * [json]: address naming grumbles InnerPricing -> PricingInner PriceWithActivationAt -> PricingAt * docs: revert changes for `AltBn128ConstOperations` * [json]: usize -> u64 Use explicit types to cope with platform dependent issues for `usize` * grumble: simplify `spec_backward_compability.json` * docs: add issue link to `TODO` * [builtin]: replace `match` with `map` * [builtin]: add deprecation message `eip1108` params * nits * [json spec tests]: fix json indentation * [json docs]: fix typos * [json]: `compability layer` + deser to BTreeMap Previously we had to match `Pricing::Single` and `PricingMulti` which this fixes. It does by introducing a compability layer and into() implemenentation. In addition, I switched the deserialization to `BTreeMap` instead of `Vec`. That changes the format of the chain spec again * [json]: rename `BuiltinCombat` -> `BuiltinCompat` * Update ethcore/builtin/src/lib.rs Co-Authored-By: David <dvdplm@gmail.com> * [json builtin]: improve docs Co-Authored-By: David <dvdplm@gmail.com> * [json builtin]: improve docs Co-Authored-By: David <dvdplm@gmail.com> * chore(builtin): sort dependencies * [json builtin]: deprecate `eip1108` params * [machine]: add bench for calling builtin contract * [machine]: reduce calls to `Builtin::is_active` * [builtin]: fix nits * [json]: revert breakage of chain specs * [json builtin]: remove `eip1108` params * [chain specs]: update to new format * [machine]: revert changes * [devp2p]: revert change * [builtin]: doc nits

According to openethereum/parity-ethereum#11039

* ethcore-builtin (#10850) * ethcore-builtin * rename ethcore-builtin Impl to Implementation * [builtin]: support `multiple prices and activations` in chain spec (#11039) * [builtin]: impl new builtin type Have an enum to deserialize either a builtin of a single price or several prices * [builtin]: style cleanup * [builtin]: fix tests * [builtin]: replace boxing with wrapper enum * cleanup * fix: make it backward compatible with old builtin * fix: update chain specs * fix: revert use of `type alias` on enum The CI doesn't use the latest rust. This commit reverts that change * fix: builtin tests * fix: revert use of `type alias` on enum * [basic-authority]: update test-chainspec * fix failing tests * [builtin]: multi-prices add `info field` It might be hard to read chain specs with several activations points. This commit introduces a `info` field which may be used to write some information about the current activation such as: `Istanbul hardfork EIP-1108` or something similar. * fix: bad rebase Co-Authored-By: David <dvdplm@gmail.com> * fix(grumbles): make it backward compatible * grumbles: resolve `NOTE` * revert chain specs changes * rename test Co-Authored-By: David <dvdplm@gmail.com> * [builtin docs]: price -> Fixed price Co-Authored-By: Andronik Ordian <write@reusable.software> * [json]: address naming grumbles InnerPricing -> PricingInner PriceWithActivationAt -> PricingAt * docs: revert changes for `AltBn128ConstOperations` * [json]: usize -> u64 Use explicit types to cope with platform dependent issues for `usize` * grumble: simplify `spec_backward_compability.json` * docs: add issue link to `TODO` * [builtin]: replace `match` with `map` * [builtin]: add deprecation message `eip1108` params * nits * [json spec tests]: fix json indentation * [json docs]: fix typos * [json]: `compability layer` + deser to BTreeMap Previously we had to match `Pricing::Single` and `PricingMulti` which this fixes. It does by introducing a compability layer and into() implemenentation. In addition, I switched the deserialization to `BTreeMap` instead of `Vec`. That changes the format of the chain spec again * [json]: rename `BuiltinCombat` -> `BuiltinCompat` * Update ethcore/builtin/src/lib.rs Co-Authored-By: David <dvdplm@gmail.com> * [json builtin]: improve docs Co-Authored-By: David <dvdplm@gmail.com> * [json builtin]: improve docs Co-Authored-By: David <dvdplm@gmail.com> * chore(builtin): sort dependencies * [json builtin]: deprecate `eip1108` params * [machine]: add bench for calling builtin contract * [machine]: reduce calls to `Builtin::is_active` * [builtin]: fix nits * [json]: revert breakage of chain specs * [json builtin]: remove `eip1108` params * [chain specs]: update to new format * [machine]: revert changes * [devp2p]: revert change * [builtin]: doc nits * manually adjust spec * [chain specs]: activate `Istanbul` on mainnet (#11228) * [chains spec]: activate istanbul at mainnet * Activate `Block >= 9,069,000` on the Ethereum mainnet * Enable Blake2 compression function `F` precompile * [chain specs]: add comments for EIP1108 activation * [chainspec mainnet]: enable Istanbul transitions * [chainspec mainnet]: simply configuration * ethcore/res: add mordor testnet configuration (#11200) * ethcore/res: add mordor testnet configuration * ethcore/spec: add mordor testnet configuration * parity/cli: add mordor testnet configuration * parity/config: fix tests * ethcore/res: update mordor spec with agharta hardfork block 301243 * ethcore/res: update kotti with agharta block 1705549 * ethcore/res: update morden with agharta block 5000381 * ethcore/res: multiple prices and activations for mordor testnet * fix mordor spec json * fix mordor spec json * fix bad merge * [ethereum tests]: revert `725dbc73a725dbc73a`

…11039) * [builtin]: impl new builtin type Have an enum to deserialize either a builtin of a single price or several prices * [builtin]: style cleanup * [builtin]: fix tests * [builtin]: replace boxing with wrapper enum * cleanup * fix: make it backward compatible with old builtin * fix: update chain specs * fix: revert use of `type alias` on enum The CI doesn't use the latest rust. This commit reverts that change * fix: builtin tests * fix: revert use of `type alias` on enum * [basic-authority]: update test-chainspec * fix failing tests * [builtin]: multi-prices add `info field` It might be hard to read chain specs with several activations points. This commit introduces a `info` field which may be used to write some information about the current activation such as: `Istanbul hardfork EIP-1108` or something similar. * fix: bad rebase Co-Authored-By: David <dvdplm@gmail.com> * fix(grumbles): make it backward compatible * grumbles: resolve `NOTE` * revert chain specs changes * rename test Co-Authored-By: David <dvdplm@gmail.com> * [builtin docs]: price -> Fixed price Co-Authored-By: Andronik Ordian <write@reusable.software> * [json]: address naming grumbles InnerPricing -> PricingInner PriceWithActivationAt -> PricingAt * docs: revert changes for `AltBn128ConstOperations` * [json]: usize -> u64 Use explicit types to cope with platform dependent issues for `usize` * grumble: simplify `spec_backward_compability.json` * docs: add issue link to `TODO` * [builtin]: replace `match` with `map` * [builtin]: add deprecation message `eip1108` params * nits * [json spec tests]: fix json indentation * [json docs]: fix typos * [json]: `compability layer` + deser to BTreeMap Previously we had to match `Pricing::Single` and `PricingMulti` which this fixes. It does by introducing a compability layer and into() implemenentation. In addition, I switched the deserialization to `BTreeMap` instead of `Vec`. That changes the format of the chain spec again * [json]: rename `BuiltinCombat` -> `BuiltinCompat` * Update ethcore/builtin/src/lib.rs Co-Authored-By: David <dvdplm@gmail.com> * [json builtin]: improve docs Co-Authored-By: David <dvdplm@gmail.com> * [json builtin]: improve docs Co-Authored-By: David <dvdplm@gmail.com> * chore(builtin): sort dependencies * [json builtin]: deprecate `eip1108` params * [machine]: add bench for calling builtin contract * [machine]: reduce calls to `Builtin::is_active` * [builtin]: fix nits * [json]: revert breakage of chain specs * [json builtin]: remove `eip1108` params * [chain specs]: update to new format * [machine]: revert changes * [devp2p]: revert change * [builtin]: doc nits

* master: (21 commits) ropsten #6631425 foundation #8798209 (#11201) Update list of bootnodes for xDai chain (#11236) ethcore/res: add mordor testnet configuration (#11200) [chain specs]: activate `Istanbul` on mainnet (#11228) [builtin]: support `multiple prices and activations` in chain spec (#11039) Insert explicit warning into the panic hook (#11225) Snapshot restoration overhaul (#11219) Fix docker centos build (#11226) retry on gitlab system failures (#11222) Update bootnodes. (#11203) Use provided usd-per-eth value if an endpoint is specified (#11209) Use a lock instead of atomics for snapshot Progress (#11197) [informant]: `MillisecondDuration` -> `as_millis()` (#11211) Step duration map configuration parameter ported from the POA Network fork (#10902) Upgrade jsonrpc to latest (#11206) [export hardcoded sync]: use debug for `H256` (#11204) Pause pruning while snapshotting (#11178) Type annotation for next_key() matching of json filter options (#11192) Crypto primitives removed from ethkey (#11174) Made ecrecover implementation trait public (#11188) ...

* ropsten #6631425 foundation #8798209 (#11201) * [stable] builtin, istanbul and mordor testnet backports (#11234) * ethcore-builtin (#10850) * [builtin]: support `multiple prices and activations` in chain spec (#11039) * [chain specs]: activate `Istanbul` on mainnet (#11228) * ethcore/res: add mordor testnet configuration (#11200) * Update list of bootnodes for xDai chain (#11236) * ethcore: remove `test-helper feat` from build (#11047) * Secret store: fix Instant::now() related race in net_keep_alive (#11155) (#11159) * [stable]: backport #10691 and #10683 (#11143) * Fix compiler warning (that will become an error) (#10683) * Refactor Clique stepping (#10691) * Add Constantinople eips to the dev (instant_seal) config (#10809) * Add cargo-remote dir to .gitignore (?) * Insert explicit warning into the panic hook (#11225) * Fix docker centos build (#11226) * Update MIX bootnodes. (#11203) * Use provided usd-per-eth value if an endpoint is specified (#11209) * Add new line after writing block to hex file. (#10984) * Type annotation for next_key() matching of json filter options (#11192) (but no `FilterOption` in 2.5 so…) * Upgrade jsonrpc to latest (#11206) * [CI] check evmbin build (#11096) * Correct EIP-712 encoding (#11092) * [client]: Fix for incorrectly dropped consensus messages (#11086) * Fix block detail updating (#11015) * Switching sccache from local to Redis (#10971) * Made ecrecover implementation trait public (#11188) * [dependencies]: jsonrpc `14.0.1` (#11183) * [receipt]: add `sender` & `receiver` to `RichReceipts` (#11179) * [ethcore/builtin]: do not panic in blake2pricer on short input (#11180) * util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175) * ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172) * Change how RPCs eth_call and eth_estimateGas handle "Pending" (#11127) * Cleanup stratum a bit (#11161) * Upgrade to jsonrpc v14 (#11151) * SecretStore: expose restore_key_public in HTTP API (#10241)

* [CI] check evmbin build (#11096) * Correct EIP-712 encoding (#11092) * [client]: Fix for incorrectly dropped consensus messages (#11082) (#11086) * Update hardcoded headers (foundation, classic, kovan, xdai, ewc, ...) (#11053) * Add cargo-remote dir to .gitignore (?) * Update light client headers: ropsten 6631425 foundation 8798209 (#11201) * Update list of bootnodes for xDai chain (#11236) * ethcore/res: add mordor testnet configuration (#11200) * [chain specs]: activate Istanbul on mainnet (#11228) * [builtin]: support multiple prices and activations in chain spec (#11039) * [receipt]: add sender & receiver to RichReceipts (#11179) * [ethcore/builtin]: do not panic in blake2pricer on short input (#11180) * Made ecrecover implementation trait public (#11188) * Fix docker centos build (#11226) * Update MIX bootnodes. (#11203) * Insert explicit warning into the panic hook (#11225) * Use provided usd-per-eth value if an endpoint is specified (#11209) * Cleanup stratum a bit (#11161) * Add Constantinople EIPs to the dev (instant_seal) config (#10809) (already backported) * util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175) * ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172) * Type annotation for next_key() matching of json filter options (#11192) * Upgrade jsonrpc to latest (#11206) * [dependencies]: jsonrpc 14.0.1 (#11183) * Upgrade to jsonrpc v14 (#11151) * Switching sccache from local to Redis (#10971) * Snapshot restoration overhaul (#11219) * Add new line after writing block to hex file. (#10984) * Pause pruning while snapshotting (#11178) * Change how RPCs eth_call and eth_estimateGas handle "Pending" (#11127) * Fix block detail updating (#11015) * Make InstantSeal Instant again #11186 * Filter out some bad ropsten warp snapshots (#11247)

This was referenced Sep 17, 2019

Validating Aura node crashes on startup with 2.5.8 #11063

Closed

[chainspecs]: make eip1108 optional #11065

Closed

ordian added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M2-config 📂 Chain specifications and node configurations. M4-core ⛓ Core client code / Rust. labels Sep 19, 2019

ordian added this to the 2.7 milestone Sep 19, 2019

niklasad1 force-pushed the na-builtin-refactor branch 6 times, most recently from a5c52b8 to 8db3971 Compare October 1, 2019 15:21

niklasad1 marked this pull request as ready for review October 1, 2019 15:28

niklasad1 added A0-pleasereview 🤓 Pull request needs code review. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Oct 1, 2019

dvdplm reviewed Oct 3, 2019

View reviewed changes

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

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

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

ordian reviewed Oct 4, 2019

View reviewed changes

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

ethcore/res/ethereum/classic.json Show resolved Hide resolved

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

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

ordian reviewed Oct 4, 2019

View reviewed changes

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

dvdplm previously requested changes Oct 4, 2019

View reviewed changes

niklasad1 force-pushed the na-builtin-refactor branch 2 times, most recently from bceeac1 to 4510485 Compare October 4, 2019 17:06

ordian reviewed Oct 6, 2019

View reviewed changes

niklasad1 added 3 commits October 6, 2019 21:15

[builtin]: impl new builtin type

96495f6

Have an enum to deserialize either a builtin of a single price or several prices

[builtin]: style cleanup

5d88fc5

[builtin]: fix tests

dd9f23d

niklasad1 added 2 commits October 31, 2019 10:59

[json builtin]: remove eip1108 params

fed8433

[chain specs]: update to new format

0e83090

niklasad1 commented Oct 31, 2019

View reviewed changes

util/network-devp2p/src/host.rs Outdated Show resolved Hide resolved

ordian approved these changes Oct 31, 2019

View reviewed changes

[machine]: revert changes

0bc1153

dvdplm approved these changes Oct 31, 2019

View reviewed changes

niklasad1 added 3 commits October 31, 2019 14:18

[devp2p]: revert change

1bd8290

[builtin]: doc nits

279342c

Merge remote-tracking branch 'origin/master' into na-builtin-refactor

2be2534

ordian merged commit 13729a0 into master Oct 31, 2019

ordian deleted the na-builtin-refactor branch October 31, 2019 16:09

varasev added a commit to poanetwork/posdao-contracts that referenced this pull request Nov 4, 2019

New pricing transitions in spec.json

1a8c66e

According to openethereum/parity-ethereum#11039

rjl493456442 mentioned this pull request Nov 5, 2019

cmd/puppeth: update chain spec of parity ethereum/go-ethereum#20241

Merged

This was referenced Nov 5, 2019

v2.5.10 stable #11239

Merged

v2.6.5 beta #11240

Merged

varasev mentioned this pull request Nov 22, 2019

(Sokol) Update spec for Istanbul poanetwork/poa-chain-spec#128

Merged

This was referenced Dec 4, 2019

(xDai) Update spec for Istanbul poanetwork/poa-chain-spec#130

Merged

(Core) Update spec for Istanbul poanetwork/poa-chain-spec#131

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

niklasad1 commented Sep 12, 2019 •

edited by ordian

Loading

dvdplm commented Sep 13, 2019 •

edited

Loading

niklasad1 commented Sep 13, 2019

dvdplm commented Sep 13, 2019

dvdplm left a comment

ordian left a comment

niklasad1 commented Oct 31, 2019 •

edited

Loading

dvdplm left a comment

dvdplm Oct 31, 2019

niklasad1 Oct 31, 2019 •

edited

Loading

dvdplm Oct 31, 2019

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

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

Conversation

niklasad1 commented Sep 12, 2019 • edited by ordian Loading

dvdplm commented Sep 13, 2019 • edited Loading

niklasad1 commented Sep 13, 2019

dvdplm commented Sep 13, 2019

dvdplm left a comment

Choose a reason for hiding this comment

ordian left a comment

Choose a reason for hiding this comment

niklasad1 commented Oct 31, 2019 • edited Loading

dvdplm left a comment

Choose a reason for hiding this comment

dvdplm Oct 31, 2019

Choose a reason for hiding this comment

niklasad1 Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

dvdplm Oct 31, 2019

Choose a reason for hiding this comment

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

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

niklasad1 commented Sep 12, 2019 •

edited by ordian

Loading

dvdplm commented Sep 13, 2019 •

edited

Loading

niklasad1 commented Oct 31, 2019 •

edited

Loading

niklasad1 Oct 31, 2019 •

edited

Loading