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

Disable EIP-98 transition by default #9955

Merged
merged 2 commits into from
Nov 25, 2018
Merged

Disable EIP-98 transition by default #9955

merged 2 commits into from
Nov 25, 2018

Conversation

sorpaas
Copy link
Collaborator

@sorpaas sorpaas commented Nov 22, 2018

This PR replaces the default value of eip98Transition to BlockNumber::max_value, to reduce the chance of chain mis-configs. Note that:

  • If a chain has EIP-658 enabled at any point, then it "disables" EIP-98. So we don't need to specify them in poacore and poasokol.
  • This leaves only Kovan configs need to be changed.
  • Removing existing "eip98Transition": "0xffffffffffffffff" is entirely optional.

For release note, we need to specify that:

For custom chain config, if you don't have eip98Transition specifically set, and you don't have eip658Transition enabled at block 0, then you need to add "eip98Transition": 0.

@sorpaas sorpaas added A0-pleasereview 🤓 Pull request needs code review. M2-config 📂 Chain specifications and node configurations. A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 22, 2018
@sorpaas sorpaas added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Nov 23, 2018
@sorpaas sorpaas changed the title Remove EIP-98 configurations Disable EIP-98 transition by default Nov 23, 2018
Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

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

LGTM. Indeed release note is very important, maybe a [Breaking change] tag or any kind of emphasis should be added in the release to make it super explicit.

@sorpaas sorpaas added B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 23, 2018
@5chdn 5chdn merged commit 34d22a3 into master Nov 25, 2018
@5chdn 5chdn deleted the sp-remove-eip98 branch November 25, 2018 18:59
@5chdn 5chdn added this to the 2.3 milestone Nov 25, 2018
@stone212
Copy link

stone212 commented Oct 2, 2019

@sorpaas

@5chdn Can you please help explain the language used here:

If a chain has EIP-658 enabled at any point, then it "disables" EIP-98. So we don't need to specify them in poacore and poasokol.

The phrase "we don't need to specify them in poacore and poasokol" is confusing for a few reasons.

  1. "don't need to" or must not?

  2. is this only for PoA chains? If I have a private blockchain with PoW and EIP-658, is it necessary to add eip98transition? I just do not understand the language of this release note.

@jam10o-new
Copy link
Contributor

jam10o-new commented Oct 2, 2019

@stone212 the two eips are mutually exclusive - they should never both be enabled on the same chain - there is no special case for Aura.

@stone212
Copy link

stone212 commented Oct 2, 2019

@joshua-mir Okay so if I have eip658Transition (and I do) in my chain spec, I should not have eip98transition (and I do not) correct?

Because eip98transition is dis-abled by default after 2.3.0 so eip658Transition should be happy on a chain running Parity 2.3.x with no mention for eip98transition. Do I understand this?

@stone212
Copy link

stone212 commented Oct 2, 2019

@joshua-mir

there is no special case for Aura.

What is this "Aura" you keep talking about? I do not know what this is.

@jam10o-new
Copy link
Contributor

Aura is our PoA consensus engine.

Because eip98transition is dis-abled by default after 2.3.0 so eip658Transition should be happy on a chain running Parity 2.3.x. Do I understand this?

Correct.

@stone212
Copy link

stone212 commented Oct 2, 2019

@joshua-mir

Aura is our PoA consensus engine.

Okay but now this goes back in a circle to my earlier question, the vague language. The release notes above make it seem like this issue only applies to PoA. So let me ask again and mention PoW again:

Because eip98transition is dis-abled by default after 2.3.0 so eip658Transition should be happy on a chain running Parity 2.3.x and using PoW / ethash. Do I continue to understand this?

Original question: If I have a private blockchain with PoW and EIP-658, is it necessary to add eip98transition

@jam10o-new
Copy link
Contributor

Yes, you're not misunderstanding anything.

The reason why there are no PoW networks mentioned in the comments above is because very few people run private PoW networks. Unless a param is in the params section within the engine section of your config, it is unrelated to the consensus engine of your chain.

@stone212
Copy link

stone212 commented Jan 25, 2020

@joshua-mir

Yes, you're not misunderstanding anything.

I am getting confused by the language. Please, a yes or no:

If I have a private blockchain with PoW and EIP-658 (at a block that is past but is not block 0), is it necessary to add eip98transition at 0 to enable sync with Parity 2.4.x?

The confusion comes from this phrase from above:

For custom chain config, if you don't have eip98Transition specifically set, and you don't have eip658Transition enabled at block 0, then you need to add "eip98Transition": 0.

We have a chain with "eip658Transition": "352300", and that means we "don't have eip658Transition enabled at block 0" so that implies we must " add "eip98Transition": 0."

Do you see why I'm confused?

This is an old issue for us, but we have not had workers since September so I am returning to it now.

And a follow-up question, is if not doing this could cause pre-2.4 versions to not sync with 2.4.x? Thank you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. M2-config 📂 Chain specifications and node configurations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants