Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Update upstream tests for Istanbul #1852

Closed
wants to merge 9 commits into from
Closed

[WIP] Update upstream tests for Istanbul #1852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fe74e9b
update upstream tests for Istanbul
voith Sep 18, 2019
c023144
updated fixtures and handle case when postState is a string
voith Sep 22, 2019
d1b657e
add more gas for sstore_uint64 tests
voith Sep 22, 2019
b3eb09c
change ByzantiumToConstantinopleAt5 to ByzantiumToConstantinopleFixAt5
voith Sep 22, 2019
b413fa9
update gas for create_empty_contract
voith Sep 22, 2019
4b96de7
mark randomStatetest94 as slow tests
voith Sep 23, 2019
0d0c58c
add istanbul blockchain tests to CI
voith Sep 23, 2019
cbaeba7
Some fixtures do not have post state. Added a normalizer `normalize_p…
voith Sep 26, 2019
cd3e634
mark some tests as incorrect
voith Oct 2, 2019
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
7 changes: 7 additions & 0 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,12 @@ jobs:
- image: circleci/python:3.6
environment:
TOXENV: py36-native-blockchain-homestead
py36-native-blockchain-istanbul:
<<: *common
docker:
- image: circleci/python:3.6
environment:
TOXENV: py36-native-blockchain-istanbul
py36-native-blockchain-petersburg:
<<: *common
docker:
Expand Down Expand Up @@ -171,6 +177,7 @@ workflows:
- py36-native-blockchain-constantinople
- py36-native-blockchain-frontier
- py36-native-blockchain-homestead
- py36-native-blockchain-istanbul
- py36-native-blockchain-petersburg
- py36-native-blockchain-tangerine_whistle
- py36-native-blockchain-spurious_dragon
Expand Down
4 changes: 4 additions & 0 deletions eth/chains/mainnet/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
MAINNET_CHAIN_ID,
BYZANTIUM_MAINNET_BLOCK,
PETERSBURG_MAINNET_BLOCK,
ISTANBUL_MAINNET_BLOCK,
TANGERINE_WHISTLE_MAINNET_BLOCK,
HOMESTEAD_MAINNET_BLOCK,
SPURIOUS_DRAGON_MAINNET_BLOCK,
Expand All @@ -34,6 +35,7 @@
ByzantiumVM,
FrontierVM,
HomesteadVM,
IstanbulVM,
PetersburgVM,
SpuriousDragonVM,
TangerineWhistleVM,
Expand Down Expand Up @@ -84,6 +86,7 @@ class MainnetHomesteadVM(MainnetDAOValidatorVM):
SPURIOUS_DRAGON_MAINNET_BLOCK,
BYZANTIUM_MAINNET_BLOCK,
PETERSBURG_MAINNET_BLOCK,
ISTANBUL_MAINNET_BLOCK,
)
MAINNET_VMS = (
FrontierVM,
Expand All @@ -92,6 +95,7 @@ class MainnetHomesteadVM(MainnetDAOValidatorVM):
SpuriousDragonVM,
ByzantiumVM,
PetersburgVM,
IstanbulVM,
)

MAINNET_VM_CONFIGURATION = tuple(zip(MAINNET_FORK_BLOCKS, MAINNET_VMS))
Expand Down
6 changes: 6 additions & 0 deletions eth/chains/mainnet/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,9 @@
# Petersburg Block
#
PETERSBURG_MAINNET_BLOCK = BlockNumber(7280000)

#
# Istanbul Block
#
# FIXME: Replace this with the correct blocknumber when it's decided
ISTANBUL_MAINNET_BLOCK = BlockNumber(7280728)
15 changes: 12 additions & 3 deletions eth/tools/_utils/normalization.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,8 +376,17 @@ def normalize_account_state(account_state: FixtureAccountState) -> AccountState:
}


def normalize_post_state(postate: FixtureAccountState) -> AccountState:
# poststate might not be present in some fixtures
# https://github.com/ethereum/tests/issues/637#issuecomment-534072897
if postate is None:
return {}
else:
return normalize_account_state(postate)


@to_dict
def normalize_post_state(post_state: Dict[str, Any]) -> Iterable[Tuple[str, bytes]]:
def normalize_post_state_hash(post_state: Dict[str, Any]) -> Iterable[Tuple[str, bytes]]:
yield 'hash', decode_hex(post_state['hash'])
if 'logs' in post_state:
yield 'logs', decode_hex(post_state['logs'])
Expand All @@ -393,7 +402,7 @@ def normalize_statetest_fixture(fixture: Dict[str, Any],
normalized_fixture = {
'env': normalize_environment(fixture['env']),
'pre': normalize_account_state(fixture['pre']),
'post': normalize_post_state(post_state),
'post': normalize_post_state_hash(post_state),
'transaction': normalize_unsigned_transaction(
fixture['transaction'],
post_state['indexes'],
Expand Down Expand Up @@ -540,7 +549,7 @@ def normalize_blockchain_fixtures(fixture: Dict[str, Any]) -> Dict[str, Any]:
'genesisBlockHeader': normalize_block_header(fixture['genesisBlockHeader']),
'lastblockhash': decode_hex(fixture['lastblockhash']),
'pre': normalize_account_state(fixture['pre']),
'postState': normalize_account_state(fixture['postState']),
'postState': normalize_post_state(fixture.get('postState')),
'network': fixture['network'],
}

Expand Down
7 changes: 6 additions & 1 deletion eth/tools/fixtures/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
FrontierVM,
HomesteadVM as BaseHomesteadVM,
SpuriousDragonVM,
IstanbulVM,
)


Expand Down Expand Up @@ -131,6 +132,10 @@ def chain_vm_configuration(fixture: Dict[str, Any]) -> Iterable[Tuple[int, Type[
return (
(0, PetersburgVM),
)
elif network == 'Istanbul':
return (
(0, IstanbulVM),
)
elif network == 'FrontierToHomesteadAt5':
HomesteadVM = BaseHomesteadVM.configure(support_dao_fork=False)
return (
Expand All @@ -156,7 +161,7 @@ def chain_vm_configuration(fixture: Dict[str, Any]) -> Iterable[Tuple[int, Type[
(0, SpuriousDragonVM),
(5, ByzantiumVM),
)
elif network == 'ByzantiumToConstantinopleAt5':
elif network == 'ByzantiumToConstantinopleFixAt5':
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks suspicious, the previous name ByzantiumToConstantinopleAt5 was correctly reflected with the selected VMs ByzantiumVM and ConstantinopleVM. But now the transition seems to go from Byzantium to ConstantinopleFix where ConstantinopleFix is the ethereum/test way to refer to PetersburgVM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this. I need to verify this again. But this was something that I had changed just to get the tests passing. Since the tests didn't complain, I didn't forget to verify this change. If the tests have in fact been renamed then I need to change line 167 to PetersburgVM 👍 . I will verify this and make sure that I haven't missed any case(since tests didn't raise ValueError, I assume I didn't).

Copy link
Contributor

@veox veox Oct 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I've grepped fixtures, and there are no longer instances of string ByzantiumToConstantinopleAt5 there.

The upstream test checks for main-net transition from ByzantiumVM to PetersburgVM, it seems. These two files (just one generated test file):

fixtures/BlockchainTests/TransitionTests/bcByzantiumToConstantinopleFix/ConstantinopleFixTransition.json:        "network" : "ByzantiumToConstantinopleFixAt5",
fixtures/src/BlockchainTestsFiller/TransitionTests/bcByzantiumToConstantinopleFix/ConstantinopleFixTransitionFiller.json:	    "network" : "ByzantiumToConstantinopleFixAt5",

return (
(0, ByzantiumVM),
(5, ConstantinopleVM),
Expand Down
2 changes: 1 addition & 1 deletion fixtures
Open in desktop
Submodule fixtures updated from 6b8570 to b5eb99
4 changes: 2 additions & 2 deletions scripts/benchmark/checks/deploy_dos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@
)

FIRST_TX_GAS_LIMIT = 367724
SECOND_TX_GAS_LIMIT = 62050
THIRD_TX_GAS_LIMIT = 104789
SECOND_TX_GAS_LIMIT = 63042
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to increase gas price(+992) here in order to get the tests passing. Don't know how updating upstream tests affects benchmark tests. Is this expected or should I dig into this? cc @cburgdorf

Copy link
Contributor

Choose a reason for hiding this comment

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

Istanbul reprices some opcodes so I'm relatively sure that this change in the tx gas limit is legit. 👍

THIRD_TX_GAS_LIMIT = 105781
FORTH_TX_GAS_LIMIT = 21272
FIFTH_TX_GAS_LIMIT = 21272

Expand Down
11 changes: 11 additions & 0 deletions tests/core/chain-object/test_contract_call.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
ByzantiumVM,
ConstantinopleVM,
PetersburgVM,
IstanbulVM,
)


Expand Down Expand Up @@ -213,6 +214,16 @@ def test_get_transaction_result(
'useLotsOfGas()',
OutOfGas,
),
(
IstanbulVM,
'doRevert()',
Revert,
),
(
IstanbulVM,
'useLotsOfGas()',
OutOfGas,
),
),
)
def test_get_transaction_result_revert(
Expand Down
7 changes: 5 additions & 2 deletions tests/core/tester/test_generate_vm_configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class Forks(enum.Enum):
Byzantium = 'Byzantium'
Constantinople = 'Constantinople'
Petersburg = 'Petersburg'
Istanbul = 'Istanbul'


class CustomFrontierVM(FrontierVM):
Expand All @@ -29,7 +30,7 @@ class CustomFrontierVM(FrontierVM):
(
tuple(),
{},
((0, Forks.Petersburg),),
((0, Forks.Istanbul),),
),
(
((0, 'tangerine-whistle'), (1, 'spurious-dragon')),
Expand Down Expand Up @@ -116,7 +117,8 @@ class CustomFrontierVM(FrontierVM):
(1, 'homestead'),
(2, 'tangerine-whistle'),
(3, 'byzantium'),
(5, 'petersburg')
(5, 'petersburg'),
(6, 'istanbul'),
),
{},
(
Expand All @@ -125,6 +127,7 @@ class CustomFrontierVM(FrontierVM):
(2, Forks.TangerineWhistle),
(3, Forks.Byzantium),
(5, Forks.Petersburg),
(6, Forks.Istanbul)
),
),
),
Expand Down
9 changes: 9 additions & 0 deletions tests/json-fixtures/test_blockchain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,10 @@
('GeneralStateTests/stStaticCall/static_Return50000_2_d0g0v0.json', 'static_Return50000_2_d0g0v0_ConstantinopleFix'), # noqa: E501
('GeneralStateTests/stSystemOperationsTest/CallRecursiveBomb0_OOG_atMaxCallDepth_d0g0v0.json', 'CallRecursiveBomb0_OOG_atMaxCallDepth_d0g0v0_Frontier'), # noqa: E501
('GeneralStateTests/stSystemOperationsTest/CallRecursiveBomb0_OOG_atMaxCallDepth_d0g0v0.json', 'CallRecursiveBomb0_OOG_atMaxCallDepth_d0g0v0_Homestead'), # noqa: E501
('ValidBlocks/bcStateTests/randomStatetest94.json', 'randomStatetest94_Homestead'), # noqa: E501
('ValidBlocks/bcStateTests/randomStatetest94.json', 'randomStatetest94_Byzantium'), # noqa: E501
('ValidBlocks/bcStateTests/randomStatetest94.json', 'randomStatetest94_Constantinople'), # noqa: E501
('ValidBlocks/bcStateTests/randomStatetest94.json', 'randomStatetest94_ConstantinopleFix'), # noqa: E501
}


Expand All @@ -183,12 +187,14 @@
('GeneralStateTests/stRevertTest/RevertInCreateInInit_d0g0v0.json', 'RevertInCreateInInit_d0g0v0_Byzantium'), # noqa: E501
('GeneralStateTests/stRevertTest/RevertInCreateInInit_d0g0v0.json', 'RevertInCreateInInit_d0g0v0_Constantinople'), # noqa: E501
('GeneralStateTests/stRevertTest/RevertInCreateInInit_d0g0v0.json', 'RevertInCreateInInit_d0g0v0_ConstantinopleFix'), # noqa: E501
('GeneralStateTests/stRevertTest/RevertInCreateInInit.json', 'RevertInCreateInInit_d0g0v0_Istanbul'), # noqa: E501

# The CREATE2 variant seems to have been derived from the one above - it, too,
# has a "synthetic" state, on which py-evm flips.
# * https://github.com/ethereum/py-evm/pull/1181#issuecomment-446330609
('GeneralStateTests/stCreate2/RevertInCreateInInitCreate2_d0g0v0.json', 'RevertInCreateInInitCreate2_d0g0v0_Constantinople'), # noqa: E501
('GeneralStateTests/stCreate2/RevertInCreateInInitCreate2_d0g0v0.json', 'RevertInCreateInInitCreate2_d0g0v0_ConstantinopleFix'), # noqa: E501
('GeneralStateTests/stCreate2/RevertInCreateInInitCreate2.json', 'RevertInCreateInInitCreate2_d0g0v0_Istanbul'), # noqa: E501

# Four variants have been specifically added to test a collision type
# like the above; therefore, they fail in the same manner.
Expand All @@ -202,6 +208,9 @@
('GeneralStateTests/stSStoreTest/InitCollision_d0g0v0.json', 'InitCollision_d0g0v0_ConstantinopleFix'), # noqa: E501
('GeneralStateTests/stSStoreTest/InitCollision_d1g0v0.json', 'InitCollision_d1g0v0_ConstantinopleFix'), # noqa: E501
('GeneralStateTests/stSStoreTest/InitCollision_d3g0v0.json', 'InitCollision_d3g0v0_ConstantinopleFix'), # noqa: E501
('GeneralStateTests/stSStoreTest/InitCollision.json', 'InitCollision_d0g0v0_Istanbul'), # noqa: E501
('GeneralStateTests/stSStoreTest/InitCollision.json', 'InitCollision_d1g0v0_Istanbul'), # noqa: E501
('GeneralStateTests/stSStoreTest/InitCollision.json', 'InitCollision_d3g0v0_Istanbul'), # noqa: E501
}


Expand Down
4 changes: 2 additions & 2 deletions tests/json-fixtures/test_transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,9 @@ def fixture_transaction_class(fixture_data):
return ByzantiumTransaction
elif fork_name == ForkName.Constantinople:
return ConstantinopleTransaction
elif fork_name == "Petersburg":
elif fork_name == "ConstantinopleFix":
return PetersburgTransaction
elif fork_name == ForkName.Istanbul:
elif fork_name == "Istanbul":
return IstanbulTransaction
elif fork_name == ForkName.Metropolis:
pytest.skip("Metropolis Transaction class has not been implemented")
Expand Down
3 changes: 2 additions & 1 deletion tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
envlist=
py{36,37}-{core,database,transactions,vm}
py36-benchmark
py36-native-blockchain-{frontier,homestead,tangerine_whistle,spurious_dragon,byzantium,constantinople,petersburg,metropolis,transition}
py36-native-blockchain-{frontier,homestead,tangerine_whistle,spurious_dragon,byzantium,constantinople,petersburg,istanbul,metropolis,transition}
py{36,37}-lint
py36-docs

Expand All @@ -28,6 +28,7 @@ commands=
native-blockchain-byzantium: pytest {posargs:tests/json-fixtures/test_blockchain.py --fork Byzantium}
native-blockchain-constantinople: pytest {posargs:tests/json-fixtures/test_blockchain.py --fork Constantinople}
native-blockchain-petersburg: pytest {posargs:tests/json-fixtures/test_blockchain.py --fork ConstantinopleFix}
native-blockchain-istanbul: pytest {posargs:tests/json-fixtures/test_blockchain.py --fork Istanbul}
native-blockchain-metropolis: pytest {posargs:tests/json-fixtures/test_blockchain.py --fork Metropolis}
native-blockchain-transition: pytest {posargs:tests/json-fixtures/test_blockchain.py -k BlockchainTests/TransitionTests}
lint: flake8 {toxinidir}/eth {toxinidir}/tests {toxinidir}/scripts
Expand Down