Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Add EIP-1283 disable transition #10214

Merged
merged 1 commit into from
Jan 21, 2019
Merged

Add EIP-1283 disable transition #10214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion ethcore/src/spec/spec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ pub struct CommonParams {
pub eip1052_transition: BlockNumber,
/// Number of first block where EIP-1283 rules begin.
pub eip1283_transition: BlockNumber,
/// Number of first block where EIP-1283 rules end.
pub eip1283_disable_transition: BlockNumber,
/// Number of first block where EIP-1014 rules begin.
pub eip1014_transition: BlockNumber,
/// Number of first block where dust cleanup rules (EIP-168 and EIP169) begin.
Expand Down Expand Up @@ -189,7 +191,7 @@ impl CommonParams {
schedule.have_return_data = block_number >= self.eip211_transition;
schedule.have_bitwise_shifting = block_number >= self.eip145_transition;
schedule.have_extcodehash = block_number >= self.eip1052_transition;
schedule.eip1283 = block_number >= self.eip1283_transition;
schedule.eip1283 = block_number >= self.eip1283_transition && !(block_number >= self.eip1283_disable_transition);
if block_number >= self.eip210_transition {
schedule.blockhash_gas = 800;
}
Expand Down Expand Up @@ -300,6 +302,10 @@ impl From<ethjson::spec::Params> for CommonParams {
BlockNumber::max_value,
Into::into,
),
eip1283_disable_transition: p.eip1283_disable_transition.map_or_else(
BlockNumber::max_value,
Copy link
Contributor

@holiman holiman Jan 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that the disable-eip will be by default disabled (in other words: eip1283 will be enabled), so unless otherwise specified, eip-1283 will be present. How will this work out for mainnet clients? Will you include the fork numbers for Constantinople+Fix later on in this PR?

Copy link
Contributor

@cheme cheme Jan 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eip 1283 is disabled by default too (eip1283 activation transition also default to max block size).

Copy link
Contributor

@holiman holiman Jan 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but if someone sets constantinople without setting constantinople-disable, it will not default to mainnet behaviour.

Might be fine, but I chose to default to 'if nil: use same as Constantinople' for the geth PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

abut the json tag name, it applies camelCase to rust field name so it is indeed eip1283DisableTransition in the json.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no way to "set Constantinople" in Parity Ethereum. You either enable EIP-1283 or you don't (default). You either disable it or you don't (default). So a default chain does neither have a 1283-transition nor a 1283-disable-transition.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You either enable EIP-1283 or you don't (default)

Not quite true. You either

  1. Define it, as disabled,
  2. Define it, as enabled,
  3. Don't define it.

In the case where you have set a blocknumber for EIP-1283, but haven't defined eip1283_disable_transition, it will default use 1283. Which is what I'm talking about.

Just clarifying what I meant, not trying to argue that you should do it the way I proposed...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not understanding your comment. if you define something, than you are not using defaults.

  • eip1283Transition is default BlockNumber::max_value, hence disabled
  • eip1283DisableTransition is default BlockNumber::max_value, hence disabled

So if you don't define 1283, there won't be 1283 by default. You only get it if you set eip1283Transition explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. So if you have a chain spec which your colleague gave you a month ago, which has eip1283Transition defined as (in two wooks), and update to the latest parity client, it will roll out the version with 1283, because eip1283_disable_transition is not defined.

An alternative approach would be to have it at 0 unless otherwise defined.

Into::into,
),
eip1014_transition: p.eip1014_transition.map_or_else(
BlockNumber::max_value,
Into::into,
Expand Down
3 changes: 3 additions & 0 deletions json/src/spec/params.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ pub struct Params {
pub eip1052_transition: Option<Uint>,
/// See `CommonParams` docs.
pub eip1283_transition: Option<Uint>,
/// See `CommonParams` docs.
pub eip1283_disable_transition: Option<Uint>,
/// See `CommonParams` docs.
pub eip1014_transition: Option<Uint>,
/// See `CommonParams` docs.
pub dust_protection_transition: Option<Uint>,
Expand Down