Skip to content
This repository was archived by the owner on Jul 1, 2021. It is now read-only.

Conversation

cburgdorf
Copy link
Contributor

@cburgdorf cburgdorf commented Nov 6, 2019

What was wrong?

Trinity needs to be upgraded to the latest py-evm version to:

  1. Support the Istanbul fork
  2. Allow using the new Clique Consensus engine

How was it fixed?

  • setup.py voodoo
  • mainnet.json vodoo
  • Using new IstanbulVM where appropriate
  • updated fixtures to latest version (because latest py-evm is incompatible with previous fixtures) and resolved some minor issues.

Cute Animal Picture

put a cute animal picture link inside the parentheses

@cburgdorf cburgdorf force-pushed the christoph/chore/upgrade-evm branch 13 times, most recently from 8a9383b to 3dce761 Compare November 7, 2019 09:29
@cburgdorf cburgdorf requested a review from carver November 7, 2019 14:41
'randomStatetest94_Homestead',
'randomStatetest94_Byzantium',
'randomStatetest94_Constantinople',
'randomStatetest94_ConstantinopleFix',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These ☝️ four tests kept crashing CI (they work locally) and they were also put into the slow tests on the Py-EVM side.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, veox said something about the tests having high RAM usage.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cburgdorf That's correct. Those tests need more than 4gb of RAM(discovered here). And they were marked as slow in this commit.

await validate_accounts(rpc, chain_fixture['postState'])
# Fixtures do not include the `postState` field when its size would be past a certain limit.
# https://github.com/ethereum/tests/issues/637#issuecomment-534072897
if chain_fixture.get('postState', None):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carver I was a bit worried if we are losing any safety here but it seems that we are doing the same on the Py-EVM side and I think that we are still safe here because even if we do not validate the postState for these special cases we do validate the lastblockhash.

Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ for the link to the convo. After reading it, I agree

ALL_VMS_BY_FORK = {
vm_class.fork: vm_class
for vm_class in ALL_VMS
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just dead code

@cburgdorf cburgdorf force-pushed the christoph/chore/upgrade-evm branch from 3dce761 to 977f7f8 Compare November 7, 2019 14:49
'ShanghaiLove_Homestead',
'ShanghaiLove_Frontier',
'DelegateCallSpam',
'walletReorganizeOwners',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't get confused, I just resorted this list alphabetically (in a separate commit), so there are really only four new tests in that list.

@cburgdorf cburgdorf force-pushed the christoph/chore/upgrade-evm branch from 977f7f8 to e382cfc Compare November 7, 2019 14:52
await validate_accounts(rpc, chain_fixture['postState'])
# Fixtures do not include the `postState` field when its size would be past a certain limit.
# https://github.com/ethereum/tests/issues/637#issuecomment-534072897
if chain_fixture.get('postState', None):
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ for the link to the convo. After reading it, I agree

@cburgdorf cburgdorf force-pushed the christoph/chore/upgrade-evm branch from 5b8bfde to bba5162 Compare November 8, 2019 08:39
@cburgdorf cburgdorf merged commit ff43c30 into ethereum:master Nov 8, 2019
@carver
Copy link
Contributor

carver commented Nov 20, 2019

Closed #1186

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants