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

Sharding[VM,Block,Transaction,Message] and sharding transaction/message execution logic #238

Merged
merged 15 commits into from
Jan 7, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions evm/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,3 +171,9 @@
BALANCE_TRIE_PREFIX = b'\x01'
CODE_TRIE_PREFIX = b'\x02'
STORAGE_TRIE_PREFIX = b'\x03'


#
# Account Abstraction
#
ENTRY_POINT = 20 * b'\xff'
43 changes: 36 additions & 7 deletions evm/rlp/transactions.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,11 @@ def sender(self):
return self.get_sender()

@property
def intrensic_gas(self):
def intrinsic_gas(self):
"""
Convenience property for the return value of `get_intrensic_gas`
Convenience property for the return value of `get_intrinsic_gas`
"""
return self.get_intrensic_gas()
return self.get_intrinsic_gas()

# +-------------------------------------------------------------+
# | API that must be implemented by all Transaction subclasses. |
Expand All @@ -64,7 +64,7 @@ def validate(self):
Hook called during instantiation to ensure that all transaction
parameters pass validation rules.
"""
if self.intrensic_gas > self.gas:
if self.intrinsic_gas > self.gas:
raise ValidationError("Insufficient gas")
self.check_signature_validity()

Expand Down Expand Up @@ -96,7 +96,7 @@ def get_sender(self):
#
# Base gas costs
#
def get_intrensic_gas(self):
def get_intrinsic_gas(self):
"""
Compute the baseline gas cost for this transaction. This is the amount
of gas needed to send this transaction (but that is not actually used
Expand Down Expand Up @@ -153,9 +153,9 @@ class BaseShardingTransaction(rlp.Serializable):
fields = [
('chain_id', big_endian_int),
('shard_id', big_endian_int),
('target', address),
('to', address),
('data', binary),
('start_gas', big_endian_int),
('gas', big_endian_int),
('gas_price', big_endian_int),
('access_list', access_list_sedes),
('code', binary),
Expand All @@ -164,3 +164,32 @@ class BaseShardingTransaction(rlp.Serializable):
@property
def hash(self):
return keccak(rlp.encode(self))

#
# Validation
#
def validate(self):
"""
Hook called during instantiation to ensure that all transaction
parameters pass validation rules.
"""
if self.intrinsic_gas > self.gas:
raise ValidationError("Insufficient gas")

@property
def intrinsic_gas(self):
"""
Convenience property for the return value of `get_intrinsic_gas`
"""
return self.get_intrinsic_gas()

#
# Base gas costs
#
def get_intrinsic_gas(self):
"""
Compute the baseline gas cost for this transaction. This is the amount
of gas needed to send this transaction (but that is not actually used
for computation).
"""
raise NotImplementedError("Must be implemented by subclasses")
16 changes: 16 additions & 0 deletions evm/utils/address.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import rlp

from evm.validation import (
validate_is_bytes,
validate_length_lte,
)
from .keccak import (
keccak,
)
Expand All @@ -16,3 +20,15 @@ def force_bytes_to_address(value):

def generate_contract_address(address, nonce):
return keccak(rlp.encode([address, nonce]))[-20:]


def generate_create2_contract_address(salt, code):
"""
If contract is created by transaction, `salt` should be empty.
Copy link
Contributor

@jannikluhn jannikluhn Dec 28, 2017

Choose a reason for hiding this comment

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

Shouldn't the salt be specified by the transaction somehow (maybe the first 32 bytes of data)? Otherwise one can't deploy the same contract twice, or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The salt could be transaction nonce if nonce is not abstracted away eventually. Or if it is, one could simply append data to code and treat it as salt.

If contract is created by contract, `salt` is set by the creator contract.
"""
validate_length_lte(salt, 32, title="Salt")
validate_is_bytes(salt)
validate_is_bytes(code)

return keccak(pad_left(salt, 32, b'\x00') + code)[-20:]
3 changes: 3 additions & 0 deletions evm/vm/forks/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,6 @@
from .byzantium import ( # noqa: F401
ByzantiumVM,
)
from .sharding import ( # noqa: F401
ShardingVM,
)
2 changes: 1 addition & 1 deletion evm/vm/forks/frontier/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ def _execute_frontier_transaction(vm, transaction):
state_db.increment_nonce(transaction.sender)

# Setup VM Message
message_gas = transaction.gas - transaction.intrensic_gas
message_gas = transaction.gas - transaction.intrinsic_gas

if transaction.to == constants.CREATE_CONTRACT_ADDRESS:
contract_address = generate_contract_address(
Expand Down
6 changes: 3 additions & 3 deletions evm/vm/forks/frontier/transactions.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ def check_signature_validity(self):
def get_sender(self):
return extract_transaction_sender(self)

def get_intrensic_gas(self):
return _get_frontier_intrensic_gas(self.data)
def get_intrinsic_gas(self):
return _get_frontier_intrinsic_gas(self.data)

def get_message_for_signing(self):
return rlp.encode(FrontierUnsignedTransaction(
Expand Down Expand Up @@ -104,7 +104,7 @@ def as_signed_transaction(self, private_key):
)


def _get_frontier_intrensic_gas(transaction_data):
def _get_frontier_intrinsic_gas(transaction_data):
num_zero_bytes = transaction_data.count(b'\x00')
num_non_zero_bytes = len(transaction_data) - num_zero_bytes
return (
Expand Down
6 changes: 3 additions & 3 deletions evm/vm/forks/homestead/transactions.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ def validate(self):
super(HomesteadTransaction, self).validate()
validate_lt_secpk1n2(self.s, title="Transaction.s")

def get_intrensic_gas(self):
return _get_homestead_intrensic_gas(self)
def get_intrinsic_gas(self):
return _get_homestead_intrinsic_gas(self)

def get_message_for_signing(self):
return rlp.encode(HomesteadUnsignedTransaction(
Expand Down Expand Up @@ -60,7 +60,7 @@ def as_signed_transaction(self, private_key):
)


def _get_homestead_intrensic_gas(transaction):
def _get_homestead_intrinsic_gas(transaction):
num_zero_bytes = transaction.data.count(b'\x00')
num_non_zero_bytes = len(transaction.data) - num_zero_bytes
if transaction.to == CREATE_CONTRACT_ADDRESS:
Expand Down
173 changes: 173 additions & 0 deletions evm/vm/forks/sharding/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
from evm.constants import (
ENTRY_POINT,
)

from evm.exceptions import (
ContractCreationCollision,
)

from evm.vm.message import (
ShardingMessage,
)
from evm.vm.vm_state import (
VMState,
)

from evm.utils.address import (
generate_create2_contract_address,
)
from evm.utils.hexadecimal import (
encode_hex,
)
from evm.utils.keccak import (
keccak,
)

from ..byzantium import ByzantiumVM
from ..frontier.constants import (
REFUND_SELFDESTRUCT,
)
from .computation import ShardingComputation
from .validation import validate_sharding_transaction
from .blocks import ShardingBlock


def _execute_sharding_transaction(vm, transaction):
#
# 1) Pre Computation
#

# Validate the transaction
transaction.validate()

vm.validate_transaction(transaction)

gas_fee = transaction.gas * transaction.gas_price
with vm.state.state_db() as state_db:
# Buy Gas
state_db.delta_balance(transaction.to, -1 * gas_fee)

# Setup VM Message
message_gas = transaction.gas - transaction.intrinsic_gas

if transaction.code:
contract_address = generate_create2_contract_address(
b'',
transaction.code,
)
data = b''
code = transaction.code
is_create = True
else:
contract_address = None
data = transaction.data
code = state_db.get_code(transaction.to)
is_create = False

vm.logger.info(
(
"TRANSACTION: to: %s | gas: %s | "
"gas-price: %s | data-hash: %s | code-hash: %s"
),
encode_hex(transaction.to),
transaction.gas,
transaction.gas_price,
encode_hex(keccak(transaction.data)),
encode_hex(keccak(transaction.code)),
)

message = ShardingMessage(
gas=message_gas,
gas_price=transaction.gas_price,
to=transaction.to,
sender=ENTRY_POINT,
value=0,
data=data,
code=code,
is_create=is_create,
)

#
# 2) Apply the message to the VM.
#
if message.is_create:
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now message.is_create is True even if transaction.code is empty (as in this case message.code is set to the contract code in line 70). So I think there's something wrong here, but I'm not sure what's the best way to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah that's also true, it's my mistake. And I don't see a clear and simple way around this either. Perhaps have message.is_create set by execute_transaction and CREATE* opcodes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that sounds reasonable.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if I'm reading this right, but if what you're suggesting involves mutating the Message object I would like to find another way as I think Message should be treated as immutable.

Copy link
Contributor Author

@NIC619 NIC619 Dec 29, 2017

Choose a reason for hiding this comment

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

That makes sense. How about setting the message.is_create flag during message initialization? Since we can already pass in create_address, we can

  1. simply add a new is_create parameter
    OR
  2. tell by checking the value of create_address
    OR
  3. just replace create_address with is_create (since in Account Abstraction, instead of CREATE_CONTRACT_ADDRESS, to would be set as the address of the new contract. Setting storage_address to create_address would be the same as leaving it empty).

Choose a reason for hiding this comment

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

I'd say go for 2 or 3; keep it simple.

with vm.state.state_db(read_only=True) as state_db:
is_collision = state_db.account_has_code_or_nonce(contract_address)

if is_collision:
# The address of the newly created contract has collided
# with an existing contract address.
computation = vm.get_computation(message)
computation._error = ContractCreationCollision(
"Address collision while creating contract: {0}".format(
encode_hex(contract_address),
)
)
vm.logger.debug(
"Address collision while creating contract: %s",
encode_hex(contract_address),
)
else:
computation = vm.get_computation(message).apply_create_message()
else:
computation = vm.get_computation(message).apply_message()

#
# 2) Post Computation
#
# Self Destruct Refunds
num_deletions = len(computation.get_accounts_for_deletion())
if num_deletions:
computation.gas_meter.refund_gas(REFUND_SELFDESTRUCT * num_deletions)

# Gas Refunds
gas_remaining = computation.get_gas_remaining()
gas_refunded = computation.get_gas_refund()
gas_used = transaction.gas - gas_remaining
gas_refund = min(gas_refunded, gas_used // 2)
gas_refund_amount = (gas_refund + gas_remaining) * transaction.gas_price

if gas_refund_amount:
vm.logger.debug(
'TRANSACTION REFUND: %s -> %s',
gas_refund_amount,
encode_hex(message.to),
)

with vm.state.state_db() as state_db:
state_db.delta_balance(message.to, gas_refund_amount)

# Miner Fees
transaction_fee = (transaction.gas - gas_remaining - gas_refund) * transaction.gas_price
vm.logger.debug(
'TRANSACTION FEE: %s -> %s',
transaction_fee,
encode_hex(vm.block.header.coinbase),
)
with vm.state.state_db() as state_db:
state_db.delta_balance(vm.block.header.coinbase, transaction_fee)

# Process Self Destructs
with vm.state.state_db() as state_db:
for account, beneficiary in computation.get_accounts_for_deletion():
# TODO: need to figure out how we prevent multiple selfdestructs from
# the same account and if this is the right place to put this.
vm.logger.debug('DELETING ACCOUNT: %s', encode_hex(account))

# TODO: this balance setting is likely superflous and can be
# removed since `delete_account` does this.
state_db.set_balance(account, 0)
state_db.delete_account(account)

return computation


ShardingVM = ByzantiumVM.configure(
name='ShardingVM',
_computation_class=ShardingComputation,
_block_class=ShardingBlock,
_state_class=VMState,
# Method overrides
validate_transaction=validate_sharding_transaction,
execute_transaction=_execute_sharding_transaction,
)
21 changes: 21 additions & 0 deletions evm/vm/forks/sharding/blocks.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
from rlp.sedes import (
CountableList,
)
from evm.rlp.headers import (
BlockHeader,
)
from evm.vm.forks.homestead.blocks import (
HomesteadBlock,
)
from .transactions import (
ShardingTransaction,
)


class ShardingBlock(HomesteadBlock):
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that it should be class Collation(BaseCollation), which means BaseCollation is a new RLP object.
Ref: #172

If you're using ShardingBlock for tests currently, I think it's okay to keep it for now and we change to BaseCollation in other future task. But I don't see ShardingBlock being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, the name will be changed later but for now I keep it this way for simplicity.

I suppose with new transaction class, there would have to be new block class?
the new block class will be part of the ShardingVM

ShardingVM = ByzantiumVM.configure(
    name='ShardingVM',
    _block_class=ShardingBlock,
)

Copy link
Member

Choose a reason for hiding this comment

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

I think it was discussed but I can't recall the specifics.

What do you think about just writing an entirely separate Shard class so that we aren't overloading the block based API's, trying to cram the concepts of collations and blocks into the same API. While the two have a lot of similarities, I think they are going to be sufficiently different that we'll need a separate abstraction.

I have some deeper thoughts on the subject that I'll try to write-up since I'm starting to get the feeling that we will need a Shard class which mirrors the Chain class and than we'll want a ShardVM which mirrors the VM class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree that it's better to write a Shard class. But maybe it can be replaced later? My goal for this PR is to change Transaction related code. (sorry about the misleading ShardingBlock, I figured if I want to test the modification on Transaction I need to also set up VM and Block class for Sharding.)

transaction_class = ShardingTransaction
fields = [
('header', BlockHeader),
('transactions', CountableList(transaction_class)),
('uncles', CountableList(BlockHeader))
]
Loading