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.

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

Fix transaction execution #313

Merged
merged 8 commits into from
Jan 27, 2018
Merged

Conversation

jannikluhn
Copy link
Contributor

Replaces #276 (I suck at rebasing and thought rewriting this would be faster -- probably mistakenly, but well...).

  1. transaction access list is converted to prefix list at initialization (that's the format used in the rest of the code)
  2. state accessibility is checked during transaction execution (both during pre- and post-processing and for all opcodes), using computation.state_db when available.
  3. access lists are passed down from transaction to message and from message to child message (see also Deduplication of Message.gas_price and Message.origin #262)
  4. test_sharding_vm and user account contract tests have been updated to provide transactions with correct access lists

When fixing 3) I noticed the use of prepare_child_message was not consistent: BaseComputation has methods prepare_child_message and prepare_child_sharding_message. ShardingMessage has an additional prepare_child_message.

What I did was removing all prepare_child_sharding_messages and implementing ShardingComputation.prepare_child_message. So now only the computation objects have a prepare_child_message, and the sharding specific functionality (passing on access list and sighash) is implemented in ShardingComputation. I don't see anything wrong with this, just mentioning it explicitly because it's probably the only noteworthy difference to #276.

Cute Animal Picture

put a cute animal picture link inside the parentheses

@@ -145,31 +144,6 @@ def prepare_child_message(self,
)
return child_message

def prepare_child_sharding_message(self,
Copy link
Member

Choose a reason for hiding this comment

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

this looks like a good change.

@@ -161,6 +164,10 @@ class BaseShardingTransaction(rlp.Serializable):
('code', binary),
]

def __init__(self, chain_id, shard_id, to, data, gas, gas_price, access_list, code):
super().__init__(chain_id, shard_id, to, data, gas, gas_price, access_list, code)
Copy link
Member

Choose a reason for hiding this comment

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

You could *args, **kwargs both this call and the __init__ signature here since you aren't using any of these variables in the local context.

@hwwhww
Copy link
Contributor

hwwhww commented Jan 27, 2018

Maybe update tox.ini for testing tests/auxiliary in this PR?

@jannikluhn
Copy link
Contributor Author

Will do that in a separate PR (in my experience, fixing Travis requires some trial and error and I'd like to keep this one clean).

@jannikluhn jannikluhn merged commit 76a8bdd into ethereum:sharding Jan 27, 2018
@NIC619 NIC619 mentioned this pull request Jan 29, 2018
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants