Spurious dragon fork rules: WIP #109

pipermerriam · 2017-09-30T04:03:49Z

fixes: #101

What was wrong?

No support for the Spurious Dragon hard fork.

How was it fixed?

Using the EIP's as a reference as well as pyethereum and goethereum, implemented the various fork rules under evm.vm.forks.spurious_dragon.SpuriousDragonVM

https://blog.ethereum.org/2016/11/18/hard-fork-no-4-spurious-dragon/

Cute Animal Picture

gsalgado

Great stuff! A few questions/suggestions but nothing major

gsalgado · 2017-10-19T07:37:02Z

evm/vm/forks/spurious_dragon/utils.py

+
+    for beneficiary in sorted(set(computation.accounts_to_delete.values())):
+        if computation.error and computation.is_origin_computation:
+            # Special case to account for geth+parity bug


Maybe include a link to the bug?

gsalgado · 2017-10-19T07:45:13Z

evm/vm/forks/spurious_dragon/transactions.py

+
+
+class SpuriousDragonUnsignedTransaction(HomesteadUnsignedTransaction):
+    def as_signed_transaction(self, private_key, chain_id=None):


ISTM that this method is never called with a chain_id argument. Can we get rid of it?

We can but we'll need to re-add it later when we implement the RPC API that performs transaction signing since we'll want to produce replay resistant transactions. Would you prefer we remove it until then?

Let's keep it, then

gsalgado · 2017-10-19T07:45:41Z

evm/vm/forks/spurious_dragon/transactions.py

+        if chain_id is None:
+            v, r, s = create_transaction_signature(self, private_key)
+        else:
+            v, r, s = create_eip155_transaction_signature(self, private_key)


If we do get rid of that argument, this function also becomes unnecessary, no?

Yes, but I'm inclined to keep it. This does however expose that this code path isn't covered since it's missing the chain_id argument. adding a test.

gsalgado · 2017-10-19T07:48:55Z

evm/vm/forks/spurious_dragon/blocks.py

+    transaction_class = SpuriousDragonTransaction
+    fields = [
+        ('header', BlockHeader),
+        ('transactions', CountableList(SpuriousDragonTransaction)),


Should you use transaction_class here?

Somehow I didn't know you could do this... 👍

gsalgado · 2017-10-19T07:52:51Z

evm/vm/forks/spurious_dragon/__init__.py

+
+    if computation.error:
+        vm.revert(snapshot)
+        return computation


We don't really need this early return here, but if you want to keep it, it'd be nice to do the sprinkle a few more in the else block below, mainly for consistency.

I think you're visually missing an indentation level. Agreed that this code block is difficult to read. Just not sure what to do about it right now (I took the liberty of removing the if vm.logger conditionals since all of our VM's have a logger.

Indeed, I thought the other return computation was not inside the else block

gsalgado · 2017-10-19T07:55:08Z

evm/vm/forks/homestead/__init__.py

@@ -24,7 +27,7 @@ def _apply_homestead_create_message(vm, message):
    computation = vm.apply_message(message)

    if computation.error:
-        vm.commit(snapshot)
+        vm.revert(snapshot)


This looks suspicious. Why was it needed?

Turns out it doesn't matter if it commits or reverts. I'm removing the line entirely.

gsalgado · 2017-10-19T07:57:10Z

evm/vm/computation.py

@@ -200,6 +191,15 @@ def add_log_entry(self, account, topics, data):
    #
    # Getters
    #
+    def get_accounts_for_deletion(self):
+        if self.error:
+            return tuple(dict().items())


return () instead?

gsalgado · 2017-10-19T08:04:55Z

evm/chains/tester/__init__.py

+        homestead_start_block is None
+    )
+    if no_declared_blocks:
+        yield (0, SpuriousDragonTesterVM)


We're going to have to change this every time we implement rules for a new fork, right? Is there any way we could keep a list of existing forks (in order) and always pick the last one here?

Yes, but I've been punting on writing a generic algorithm for this. Soon.

gsalgado · 2017-10-19T15:38:07Z

evm/utils/transactions.py

    else:
-        return (v - 35) // 2
+        return (v - EIP155_CHAIN_ID_OFFSET) // 2


Now you return the same value in both cases. I don't think that's what you intended?

gsalgado · 2017-10-19T15:38:40Z

evm/utils/transactions.py

@@ -79,16 +40,39 @@ def extract_signature_v(v):
        return 27


s/27/V_OFFSET?

gsalgado · 2017-10-19T15:41:34Z

tests/core/transaction-utils/test_transaction_signature_validation.py

+    elif request.param == 'Homestead':
+        return HomesteadTransaction
+    elif request.param == 'SpuriousDragon':
+        return SpuriousDragonTransaction


Why not use the classes in the params list? params=[FrontierTransaction, HomesteadTransaction, SpuriousDragonTransaction]

pipermerriam force-pushed the piper/spurious-dragon branch from 645437d to e222a3f Compare October 2, 2017 22:14

pipermerriam force-pushed the piper/spurious-dragon branch 3 times, most recently from e418a0f to 663726d Compare October 18, 2017 19:23

pipermerriam requested a review from gsalgado October 18, 2017 20:09

gsalgado approved these changes Oct 19, 2017

View reviewed changes

pipermerriam force-pushed the piper/spurious-dragon branch from c2238d5 to 3177cd7 Compare October 19, 2017 15:03

pipermerriam added 3 commits October 19, 2017 09:34

Spurious Dragon fork rules support

69a8385

PR feedback

acd45a0

revamp eip155 transactions signing

9d124f8

pipermerriam force-pushed the piper/spurious-dragon branch from 3889ec8 to 9d124f8 Compare October 19, 2017 15:35

gsalgado reviewed Oct 19, 2017

View reviewed changes

evm/utils/transactions.py Outdated

@@ -79,16 +40,39 @@ def extract_signature_v(v):

return 27

Copy link

Collaborator

gsalgado Oct 19, 2017

Choose a reason for hiding this comment

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

s/27/V_OFFSET?

gsalgado reviewed Oct 19, 2017

View reviewed changes

fix for chain id extraction from EIP155 signed transactions

948e6b6

pipermerriam force-pushed the piper/spurious-dragon branch from 700097f to 948e6b6 Compare October 19, 2017 15:53

pipermerriam merged commit 4fa0f23 into master Oct 19, 2017

pipermerriam deleted the piper/spurious-dragon branch October 19, 2017 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spurious dragon fork rules: WIP #109

Spurious dragon fork rules: WIP #109

pipermerriam commented Sep 30, 2017 •

edited

Loading

gsalgado left a comment

gsalgado Oct 19, 2017

gsalgado Oct 19, 2017

pipermerriam Oct 19, 2017

gsalgado Oct 19, 2017

gsalgado Oct 19, 2017

pipermerriam Oct 19, 2017

gsalgado Oct 19, 2017

pipermerriam Oct 19, 2017

gsalgado Oct 19, 2017

pipermerriam Oct 19, 2017

gsalgado Oct 19, 2017

gsalgado Oct 19, 2017

pipermerriam Oct 19, 2017

gsalgado Oct 19, 2017

gsalgado Oct 19, 2017

pipermerriam Oct 19, 2017

gsalgado Oct 19, 2017

gsalgado Oct 19, 2017

gsalgado Oct 19, 2017



		class SpuriousDragonUnsignedTransaction(HomesteadUnsignedTransaction):
		def as_signed_transaction(self, private_key, chain_id=None):

Spurious dragon fork rules: WIP #109

Spurious dragon fork rules: WIP #109

Conversation

pipermerriam commented Sep 30, 2017 • edited Loading

What was wrong?

How was it fixed?

Cute Animal Picture

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

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 Sep 30, 2017 •

edited

Loading