Byzantium Fork Rules #123

pipermerriam · 2017-10-18T22:23:33Z

What was wrong?

Need support for the Byzantium fork.

How was it fixed?

Cute Animal Picture

put a cute animal picture here.

pipermerriam · 2017-11-12T02:59:30Z

evm/chains/chain.py

        else:
            raise VMNotFound("No vm available for block #{0}".format(block_number))

+        vm_class = base_vm_class.configure(chaindb=self.chaindb)


TODO: undo this. No longer necessary.

pipermerriam · 2017-11-16T03:36:59Z

maybe green? This is going to get split into about 5 pull requests with some minor refactors extracted into their own pull requests.

pipermerriam

Just me annotating the pull request so that I have an indicator on whether I've pulled all the things out.

pipermerriam · 2017-11-16T03:45:36Z

evm/chains/chain.py

+            'IMPORTED_BLOCK: number %s | hash %s',
+            imported_block.number,
+            encode_hex(imported_block.hash),
+        )


TODO: extract

pipermerriam · 2017-11-16T03:46:07Z

evm/constants.py

-GAS_SUICIDE_NEWACCOUNT = 25000
+GAS_SELFDESTRUCT = 0
+GAS_SELFDESTRUCT_EIP150 = 5000
+GAS_SELFDESTRUCT_NEWACCOUNT = 25000


TODO: extract

pipermerriam · 2017-11-16T03:46:16Z

evm/constants.py

@@ -110,7 +116,8 @@
 DIFFICULTY_MINIMUM = 131072

 FRONTIER_DIFFICULTY_ADJUSTMENT_CUTOFF = 13
-HOMESTEAD_DIFF_ADJUSTMENT_CUTOFF = 10
+HOMESTEAD_DIFFICULTY_ADJUSTMENT_CUTOFF = 10


TODO: extract

pipermerriam · 2017-11-16T03:46:37Z

evm/logic/block.py

@@ -3,9 +3,7 @@

 def blockhash(computation):
    block_number = computation.stack.pop(type_hint=constants.UINT256)
-


pipermerriam · 2017-11-16T03:47:28Z

evm/logic/call.py

+            else:
+                code = state_db.get_code(to)
+
+        child_msg_kwargs = {


TODO: move back into else block

pipermerriam · 2017-11-16T03:54:47Z

evm/vm/forks/spurious_dragon/opcodes.py

-        logic_fn=system.suicide_eip161,
-        mnemonic=mnemonics.SUICIDE,
-        gas_cost=constants.GAS_SUICIDE_EIP150,
+    opcode_values.SELFDESTRUCT: as_opcode(


TODO: extract

pipermerriam · 2017-11-16T03:55:11Z

tests/core/vm/test_computation.py

@@ -167,7 +161,7 @@ def test_add_log_entry_raises_if_data_isnt_in_bytes(computation):
 def test_add_log_entry(computation):
    # Adds log entry to log entries
    computation.add_log_entry(CANONICAL_ADDRESS_A, [1, 2, 3], b'')
-    assert computation.log_entries == [(b'\x0fW.R\x95\xc5\x7f\x15\x88o\x9b&>/m-l{^\xc6',
+    assert computation.log_entries == [(b'\x0fW.R\x95\xc5\x7f\x15\x88o\x9b&>/m-l\x7b^\xc6',


TODO: extract

pipermerriam · 2017-11-16T03:55:27Z

tests/core/vm/test_computation.py

    assert computation.get_log_entries() == ()


-def test_get_gas_refund(computation, create_error):
+def test_get_gas_refund(computation):


TODO: extract

pipermerriam · 2017-11-16T03:55:42Z

tests/json-fixtures/test_blockchain.py

        )
-    elif 'TransitionTests/bcHomesteadToEIP150' in fixture_path:
+    elif network == 'Homestead':


TODO: extract

pipermerriam · 2017-11-16T03:55:52Z

tests/json-fixtures/test_state.py

@@ -185,8 +190,8 @@ def test_state_fixtures(fixture, fixture_vm_class):

    try:
        computation = vm.apply_transaction(transaction)
-    except ValidationError:
-        transaction_error = True
+    except ValidationError as err:


TODO: extract

gsalgado

Looking good! I have a few questions/suggestions, but nothing major

gsalgado · 2017-11-20T08:43:05Z

evm/constants.py

+# ByzantiumVM
+#
+BYZANTIUM_MAINNET_BLOCK = 4370000
+BYZANTIUM_ROPSTEN_BLOCK = 1700000


BYZANTIUM_ROPSTEN_BLOCK is not used anywhere. Should it be in RopstenChain?

Yeah, I'm thinking it makes sense to migration the various fork-and-chain-specific block numbers into the fork or chain modules themselves.

gsalgado · 2017-11-20T08:43:54Z

evm/exceptions.py

+
+class OutOfBoundsRead(VMError):
+    """
+    Error raised to indicate an attempt was made to read data beyond then


s/then/the?

gsalgado · 2017-11-20T08:46:18Z

evm/logic/call.py

@@ -105,14 +109,18 @@ def __call__(self, computation):
            if child_computation.error:


Wouldn't it be better to explicitly check for None here instead?

I don't think so or I'm not understanding your suggestion.

I meant if child_computation.error is not None:, just in case at some point we have an error class that for some reason implements __bool__() and returns False there, this would no longer work as expected. I can't think why we'd have an error class that would do that, but in general I prefer to explicitly check for None

👍 I'm going to file this away in a "Good First Issue" ticket.

gsalgado · 2017-11-20T08:47:22Z

evm/logic/call.py

@@ -146,6 +154,7 @@ def get_call_params(self, computation):
            memory_output_start_position,
            memory_output_size,
            True,  # should_transfer_value,
+            computation.msg.is_static,  # is_static


I'd drop this comment as it doesn't add anything

gsalgado · 2017-11-20T08:55:12Z

evm/logic/call.py

@@ -105,14 +109,18 @@ def __call__(self, computation):
            if child_computation.error:
                computation.stack.push(0)
            else:
+                computation.stack.push(1)
+
+            if not child_computation.error or not child_computation.error.zeros_return_data:


I wonder if it'd make sense to abstract this into a method on Computation, so that we don't need to reach so many levels down into the exception's attributes. Something like Computation.should_return_data()?

+1 but going to do this as part of a "good first issue" ticket

gsalgado · 2017-11-20T09:09:48Z

evm/precompiles/ecpairing.py

+)
+
+
+ZERO = (bn128.FQ2.one(), bn128.FQ2.one(), bn128.FQ2.zero())


I find this an odd name choice for a 3-tuple with seemingly non-zero items, but maybe I'm missing something?

This code is a refactored version of vitalik's implementation in pyethereum. I'm not familiar enough with what is actually going on here to know what a better name would be :(

gsalgado · 2017-11-20T09:27:06Z

evm/vm/forks/byzantium/headers.py

+def compute_byzantium_difficulty(parent_header, timestamp):
+    """
+    https://github.com/ethereum/EIPs/issues/100
+    TODO: figure out how to know about uncles in this context...


Is this still TODO? If so, it'd be nice if you could expand on what exactly you mean

This was from before I'd put in the optimization to just check if parent_header.uncles_hash != EMPTY_UNCLE_HASH and was thinking that I'd actually need to be able to fetch the uncles to know how many there were.

gsalgado · 2017-11-20T09:33:59Z

evm/vm/forks/byzantium/opcodes.py

+from evm.vm.forks.spurious_dragon.opcodes import SPURIOUS_DRAGON_OPCODES
+
+
+def no_static(opcode_fn):


The name here had me confused at first, but then I realized it's used to ensure a given opcode_fn is not passed a computation with a static msg, right? Maybe rename it to ensure_no_static()?

gsalgado · 2017-11-20T09:35:45Z

evm/vm/forks/byzantium/opcodes.py

+    @functools.wraps(opcode_fn)
+    def inner(computation):
+        if computation.msg.is_static:
+            raise WriteProtection("Cannot modify state while inside of a STATICCALL context")


Also, the exception msg here is quite misleading, as it's saying we cannot modify the state when the msg is actually static? Or have I gotten this backwards?

I think the txt is correct.

"Since we are inside of a STATICCALL context, the state cannot be modified

gsalgado · 2017-11-20T09:41:46Z

evm/vm/message.py

@@ -33,6 +33,7 @@ class Message(object):
    create_address = None

    should_transfer_value = None
+    is_static = None


Why does it default to None? To force all message types to define it?

Same convention that I think has raised questions previously. This is merely a pattern I use to make it easy to know what properties an object has.

It you have strong feelings about this pattern I'm all ears because it's something that grew organically and I'm not sure it's a good pattern.

Not really, I just find it odd for bool attributes to have a default of None, but I think it's more of a gut feeling than anything justifiable

pipermerriam added the PR state: WIP label Oct 18, 2017

pipermerriam force-pushed the piper/byzantium branch 4 times, most recently from 121b572 to 61701d7 Compare October 20, 2017 16:23

pipermerriam force-pushed the piper/byzantium branch from 3562916 to 6ffd4af Compare November 11, 2017 00:01

pipermerriam commented Nov 12, 2017

View reviewed changes

pipermerriam commented Nov 16, 2017

View reviewed changes

pipermerriam force-pushed the piper/byzantium branch 5 times, most recently from b7b1bda to fc77ba5 Compare November 17, 2017 21:23

Byzantium fork rules

d58f6a7

pipermerriam force-pushed the piper/byzantium branch from fc77ba5 to d58f6a7 Compare November 17, 2017 21:23

pipermerriam added Needs Review and removed PR state: WIP labels Nov 17, 2017

pipermerriam requested a review from gsalgado November 17, 2017 22:11

gsalgado approved these changes Nov 20, 2017

View reviewed changes

pipermerriam added 2 commits November 20, 2017 06:47

minor PR cleanup

df89614

unpack result before serializing in ECMUL and ECADD

1a815fd

pipermerriam merged commit dd00cba into master Nov 20, 2017

pipermerriam deleted the piper/byzantium branch November 20, 2017 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Byzantium Fork Rules #123

Byzantium Fork Rules #123

pipermerriam commented Oct 18, 2017 •

edited

Loading

pipermerriam Nov 12, 2017

pipermerriam commented Nov 16, 2017

pipermerriam left a comment

pipermerriam Nov 16, 2017

pipermerriam Nov 16, 2017

pipermerriam Nov 16, 2017

pipermerriam Nov 16, 2017

pipermerriam Nov 16, 2017

pipermerriam Nov 16, 2017

pipermerriam Nov 16, 2017

pipermerriam Nov 16, 2017

pipermerriam Nov 16, 2017

pipermerriam Nov 16, 2017

gsalgado left a comment

gsalgado Nov 20, 2017

pipermerriam Nov 20, 2017

gsalgado Nov 20, 2017

gsalgado Nov 20, 2017

pipermerriam Nov 20, 2017

gsalgado Nov 20, 2017

pipermerriam Nov 20, 2017

gsalgado Nov 20, 2017

gsalgado Nov 20, 2017

pipermerriam Nov 20, 2017

gsalgado Nov 20, 2017

pipermerriam Nov 20, 2017

gsalgado Nov 20, 2017

pipermerriam Nov 20, 2017 •

edited

Loading

gsalgado Nov 20, 2017

gsalgado Nov 20, 2017

pipermerriam Nov 20, 2017

gsalgado Nov 20, 2017

pipermerriam Nov 20, 2017

gsalgado Nov 20, 2017

		@@ -3,9 +3,7 @@

		def blockhash(computation):
		block_number = computation.stack.pop(type_hint=constants.UINT256)

		@@ -105,14 +109,18 @@ def __call__(self, computation):
		if child_computation.error:

		)


		ZERO = (bn128.FQ2.one(), bn128.FQ2.one(), bn128.FQ2.zero())

		from evm.vm.forks.spurious_dragon.opcodes import SPURIOUS_DRAGON_OPCODES


		def no_static(opcode_fn):

Byzantium Fork Rules #123

Byzantium Fork Rules #123

Conversation

pipermerriam commented Oct 18, 2017 • edited Loading

What was wrong?

How was it fixed?

Cute Animal Picture

Choose a reason for hiding this comment

pipermerriam commented Nov 16, 2017

pipermerriam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsalgado left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam commented Oct 18, 2017 •

edited

Loading

pipermerriam Nov 20, 2017 •

edited

Loading