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

Update EWF's chains with Istanbul transition block numbers#11482

Merged
dvdplm merged 11 commits into
openethereum:masterfrom
ngyam:master
Apr 24, 2020
Merged

Update EWF's chains with Istanbul transition block numbers#11482
dvdplm merged 11 commits into
openethereum:masterfrom
ngyam:master

Conversation

@ngyam
Copy link
Copy Markdown
Contributor

@ngyam ngyam commented Feb 12, 2020

Adding the transiton blocknumbers for the istanbul updates on

  • Energy Web's Volta testnet and
  • Energy Web Chain (EWC) mainnet

ngyam added 2 commits February 12, 2020 10:29
@ngyam
update(ewf-chainspec): istanbul fork transition blocks
ed220dd 
- volta and ewc chains
@ngyam
Merge pull request #2 from ngyam/update/ewf_istanbul_chainspecs
338bc47 
update(ewf-chainspec): istanbul fork transition blocks
@parity-cla-bot
Copy link
Copy Markdown

parity-cla-bot commented Feb 12, 2020

It looks like @ngyam signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

niklasad1
niklasad1 reviewed Feb 12, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

Looks good to me, are the block_numbers published anywhere for us the check?

@dvdplm dvdplm requested a review from ngotchac February 12, 2020 11:19
@ngyam
Copy link
Copy Markdown
Contributor Author

ngyam commented Feb 12, 2020

They are also here in our repo: https://github.com/energywebfoundation/ewf-chainspec
Would it suffice? There will be some blogpost publications but not sure if this week.

dvdplm
dvdplm approved these changes Feb 12, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

LGTM

Would it be possible to use the same files in both repos? Would make checking significantly easier.

@ngyam
Copy link
Copy Markdown
Contributor Author

ngyam commented Feb 12, 2020

@dvdplm our validator install scripts pull the chainspec from this repo, and they no_serve_light = true, so we decided not to include the hardcodedSync field to keep it cleaner. That should be the only difference.

Another public source: https://energyweb.atlassian.net/wiki/spaces/EWF/pages/916291657/Istanbul+Hardfork

niklasad1
niklasad1 previously approved these changes Feb 13, 2020
View reviewed changes
niklasad1
niklasad1 reviewed Feb 13, 2020
View reviewed changes
Comment thread ethcore/res/ethereum/ewc.json
}
},
"0x0000000000000000000000000000000000000009": {
"builtin": {
Copy link
Copy Markdown
Collaborator

@niklasad1 niklasad1 Feb 13, 2020

Choose a reason for hiding this comment

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

one question though, why has the other builtins balance = 1?

Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm Feb 18, 2020

Choose a reason for hiding this comment

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

@ngyam ^^^?

Copy link
Copy Markdown
Contributor Author

@ngyam ngyam Feb 19, 2020

Choose a reason for hiding this comment

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

good observation, not sure if I can give a sound answer to that because I didn't do that part back then. Maybe to be on the safe side of the init of those accounts (if it even makes sense).
As far as I know it's not necessary. @niklasad1

Copy link
Copy Markdown
Collaborator

@niklasad1 niklasad1 Feb 19, 2020

Choose a reason for hiding this comment

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

Ok, @dvdplm explained it well: #11108 (comment)

Note, that changing balance will likely end up with a different state root but I just wanted to highlight this because we had a few bugs related this in the past.

We really should document this somewhere but I guess it is not documented because JSON doesn't support comments.

Copy link
Copy Markdown
Contributor Author

@ngyam ngyam Feb 19, 2020

Choose a reason for hiding this comment

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

thanks for the clarification. So in theory all should be good to go without adding 1 wei to this account balance, right?

Copy link
Copy Markdown
Contributor Author

@ngyam ngyam Feb 19, 2020

Choose a reason for hiding this comment

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

I don't want to change the chainspec if not necessary

Copy link
Copy Markdown
Contributor Author

@ngyam ngyam Feb 19, 2020

Choose a reason for hiding this comment

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

but if there is a slight change that otherwise there is a risk of some bugs, then of course it makes sense

Copy link
Copy Markdown
Contributor Author

@ngyam ngyam Feb 19, 2020

Choose a reason for hiding this comment

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

update: we decided to add this "missing" 1 wei just to be safe. I update the PR tomorrow

@ngyam
Copy link
Copy Markdown
Contributor Author

ngyam commented Feb 21, 2020

@niklasad1 @dvdplm

  • added balance 1 wei to the blake2 bultin
  • delayed fork dates by 1 week so that our validators have more time to update

@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. M2-config 📂 Chain specifications and node configurations. labels Feb 21, 2020
@ngyam
Copy link
Copy Markdown
Contributor Author

ngyam commented Feb 21, 2020

Please don't merge it yet, it still has a chance to change according to our governance.

@niklasad1 niklasad1 added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Feb 21, 2020
@niklasad1 niklasad1 self-requested a review February 21, 2020 16:07
@ngyam
Copy link
Copy Markdown
Contributor Author

ngyam commented Apr 3, 2020

The forks already went down so the changes I made are the final one:

@ngyam
Copy link
Copy Markdown
Contributor Author

ngyam commented Apr 17, 2020

@dvdplm @niklasad1 @ngotchac ping?

niklasad1
niklasad1 approved these changes Apr 17, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM

@niklasad1 niklasad1 added A8-looksgood 🦄 Pull request is reviewed well. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Apr 18, 2020
@dvdplm dvdplm merged commit 4f3a128 into openethereum:master Apr 24, 2020
dvdplm added a commit that referenced this pull request Apr 28, 2020
@dvdplm
Merge branch 'dp/fix/memory-leak-when-warp-syncing' into dp/chore/upg…
e93113e 
…rade-to-rocksdb-0.14

* dp/fix/memory-leak-when-warp-syncing:
  Drain the transaction overlay
  New default paths (#11641)
  Update EWF's chains with Istanbul transition block numbers (#11482)
  add openethereum supplementary bootnodes, those are not active right now, but will be in case the network needs more power (#11650)
  Remove Parity bootnodes (#11644)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ngotchac ngotchac Awaiting requested review from ngotchac

2 more reviewers

@dvdplm dvdplm dvdplm approved these changes

@niklasad1 niklasad1 niklasad1 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A8-looksgood 🦄 Pull request is reviewed well. M2-config 📂 Chain specifications and node configurations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ngyam @parity-cla-bot @dvdplm @niklasad1 @ordian