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

Jump to bottom

Make block_number configurable in genesis header again #2063

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Make block_number configurable in genesis header again #2063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fb7cbb7
Adding test case
catoenm May 23, 2022
cbf02f8
Proper unit test
catoenm May 23, 2022
796c4f2
Reordering params
catoenm May 23, 2022
cf786c9
Linting
catoenm May 23, 2022
0ca5f68
Updating according to issue comments
catoenm May 23, 2022
67ac212
Fixing unit testing
catoenm May 23, 2022
47074a4
Linting
catoenm May 23, 2022
afadd3f
Fixing another small linting problem
catoenm May 24, 2022
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
11 changes: 6 additions & 5 deletions eth/_utils/headers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@
Tuple,
)

from eth_typing import (
Address,
)

from eth_typing import Address
from eth.abc import BlockHeaderAPI
from eth.constants import (
BLANK_ROOT_HASH,
Expand All @@ -21,6 +18,7 @@
GAS_LIMIT_USAGE_ADJUSTMENT_DENOMINATOR,
ZERO_ADDRESS,
)
from eth.exceptions import PyEVMError
from eth.typing import (
BlockNumber,
HeaderParams,
Expand Down Expand Up @@ -57,6 +55,7 @@ def fill_header_params_from_parent(
gas_limit: int,
difficulty: int,
timestamp: int,
block_number: int = None,
coinbase: Address = ZERO_ADDRESS,
nonce: bytes = None,
extra_data: bytes = None,
Expand All @@ -67,11 +66,13 @@ def fill_header_params_from_parent(

if parent is None:
parent_hash = GENESIS_PARENT_HASH
block_number = GENESIS_BLOCK_NUMBER
block_number = block_number if block_number else GENESIS_BLOCK_NUMBER
if state_root is None:
state_root = BLANK_ROOT_HASH
else:
parent_hash = parent.hash
if block_number:
raise PyEVMError("block_number cannot be configured if a parent header exists.")
block_number = BlockNumber(parent.block_number + 1)

if state_root is None:
Expand Down
55 changes: 55 additions & 0 deletions tests/core/vm/test_vm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,13 @@
MiningChain,
)
from eth.chains.mainnet import MAINNET_VMS
from eth.exceptions import PyEVMError
from eth.tools.builder.chain import api
from eth.tools.factories.transaction import (
new_transaction
)
from eth_typing import BlockNumber
from eth._utils.headers import fill_header_params_from_parent


@pytest.fixture(params=MAINNET_VMS)
Expand Down Expand Up @@ -191,3 +194,55 @@ def test_validate_gas_limit_too_high(noproof_consensus_chain):

with pytest.raises(ValidationError, match="[Gg]as limit"):
vm.validate_header(invalid_header, block1.header)


@pytest.mark.parametrize(
"custom_header_params,null_parent",
(
({
'gas_limit': 1,
'difficulty': 10,
'timestamp': 100,
'block_number': 1000
}, True),
({
'gas_limit': 1,
'difficulty': 10,
'timestamp': 100,
}, False),
({
'gas_limit': 1,
'difficulty': 10,
'timestamp': 100,
'block_number': 1000,
}, False)
)
)
def test_fill_header_params_from_parent(custom_header_params, null_parent,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The test cases look good here. We do try to avoid conditionals in our tests and instead split each test case out into their own test. With conditionals, it can be hard to read what test case ends where and to know which conditional is causing the test to pass over another without debugging the test, etc.

If we can split each test case out into its own test, with an explicit title of the thing it is testing, I think that would be ideal. We can probably factor out the parametrized test cases into a list and pass them into each test if they end up sharing similar info too... something like:

CUSTOM_HEADER_TEST_DATA = (
    {
         'gas_limit': 1,
         'difficulty': 10,
         ...
    },
    { ... },
    { ... },
)

@pytest.mark.parametrize('custom_header_params', CUSTOM_HEADER_TEST_DATA)
def test_case_1(custom_header_params):
     ...
     
@pytest.mark.parametrize('custom_header_params', CUSTOM_HEADER_TEST_DATA)
def test_case_2(custom_header_params):
     ...

Thoughts?

noproof_consensus_chain):
# Handle cases which want to specify a null parent.
block = noproof_consensus_chain.mine_block()
header = block.header if null_parent is False else None

# Cannot specify block number and a parent.
if 'block_number' in custom_header_params and header is not None:
with pytest.raises(PyEVMError, match="block_number cannot be configured if a parent header exists."):
new_header_params = fill_header_params_from_parent(header, **custom_header_params)
return
new_header_params = fill_header_params_from_parent(header, **custom_header_params)

# Compare fields which are copied no matter what.
trivial_fields = ['gas_limit', 'difficulty', 'timestamp']
for field in trivial_fields:
assert new_header_params[field] == custom_header_params[field]

# Check `block_number` and `parent_hash` cases.
if 'block_number' in custom_header_params:
assert new_header_params['block_number'] == custom_header_params['block_number']
assert new_header_params['parent_hash'] == constants.GENESIS_PARENT_HASH
elif header is None:
assert new_header_params['block_number'] == constants.GENESIS_BLOCK_NUMBER
assert new_header_params['parent_hash'] == constants.GENESIS_PARENT_HASH
else:
assert new_header_params['block_number'] == BlockNumber(header['block_number'] + 1)
assert new_header_params['parent_hash'] == header.hash