-
Notifications
You must be signed in to change notification settings - Fork 644
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
Sharding[VM,Block,Transaction,Message] and sharding transaction/message execution logic #238
Conversation
) | ||
|
||
|
||
class ShardingBlock(HomesteadBlock): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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,
)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.)
evm/rlp/transactions.py
Outdated
@@ -151,7 +151,7 @@ class BaseShardingTransaction(rlp.Serializable): | |||
('shard_id', big_endian_int), | |||
('target', address), | |||
('data', binary), | |||
('start_gas', big_endian_int), | |||
('gas', big_endian_int), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you give an explanation for why change start_gas
to gas
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
start_gas
is only used in GasMeter
while other Transaction classes use gas
. So I change it for uniformity, but I have no strong preference for either one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been using the term gas
in the other Transaction
classes to be more inline with the conventions used elsewhere such as web3.py or the JSON-RPC spec. I could be convinced to change it if someone has a compelling argument.
evm/vm/forks/sharding/validation.py
Outdated
# TODO:Update transaction validation logic in Sharding | ||
# e.g. checking shard_id < SHARD_COUNT | ||
|
||
validate_frontier_transaction(evm, transaction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment for using validate_frontier_transaction
but validate_homestead_transaction
on purpose would be clearer. (validate_homestead_transaction
doesn't apply for sharding)
evm/rlp/transactions.py
Outdated
@@ -160,3 +160,32 @@ class BaseShardingTransaction(rlp.Serializable): | |||
@property | |||
def hash(self): | |||
return keccak(rlp.encode(self)) | |||
|
|||
@property | |||
def intrensic_gas(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
organization nitpick
This seems like it should be next to the get_intrensic_gas
method.
Please get tests passing (looks like a linting error?). |
@pipermerriam @hwwhww I update the code along with PR detail. Please have a look, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good to go after cleaning up the code duplication in ShardingMessage.__init__
. Let me know if I missed something that makes calling super()
not work.
evm/vm/message.py
Outdated
validate_canonical_address(sender, title="Message.sender") | ||
self.sender = sender | ||
|
||
validate_uint256(value, title="Message.value") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can eliminate the majority of the body of this function by inserting a super
call to the Messag.__init__
method and then only dealing with what looks like the only difference which is access_list
here in this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah you are right, I thought they differ in how they handle to
and sender
but the effects are actually the same.
evm/vm/message.py
Outdated
validate_canonical_address(to, title="Message.to") | ||
self.to = to | ||
|
||
if sender != ENTRY_POINT: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check shouldn't be required since the ENTRY_POINT
is a valid canonical address (at least I think it is)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah ENTRY_POINT
is a valid canonical address so we only validate if sender
is not ENTRY_POINT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silly me. I saw ==
even though it's clearly !=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
It's a bit unnerving for me to have the various transaction and message execution functions for shards untested but I assume that will come at some later point.
@pipermerriam I add tests for |
|
||
def generate_create2_contract_address(salt, code): | ||
""" | ||
If contract is created by transaction, `salt` should be empty. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
evm/vm/forks/sharding/validation.py
Outdated
# to be ECDSA and checks signatures accordingly. So it's skipped for | ||
# the reason that there won't be built-in signature scheme in Account | ||
# Abstraction. | ||
validate_frontier_transaction(evm, transaction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate_frontier_transaction
checks the balance of transaction.sender
which would fail for ShardingTransaction
s. It also ignores transaction.access_list
. So I think we can't use this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah you are right. I will update it. Thanks!
# | ||
# 2) Apply the message to the VM. | ||
# | ||
if message.is_create: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that sounds reasonable.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
- simply add a new
is_create
parameter
OR - tell by checking the value of
create_address
OR - just replace
create_address
withis_create
(since in Account Abstraction, instead ofCREATE_CONTRACT_ADDRESS
,to
would be set as the address of the new contract. Settingstorage_address
tocreate_address
would be the same as leaving it empty).
There was a problem hiding this comment.
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.
btw, @jannikluhn @pipermerriam I was thinking 1. adding nonce back to transaction format OR 2. completely abstract nonce away. I would prefer 1 and if nonce is eventually going away we can simply remove the codes altogether. |
@NIC619 my comment about testing was not intended to mean that you needed to provide those tests now (though I don't mind if you do), just to say that those code paths are un-tested. Maybe just a single smoke test for very basic transaction execution to know that there are no obvious major broken parts? I assume that the JSON fixture based tests will be updated at some point and then we'll use those to test. |
evm/vm/forks/sharding/validation.py
Outdated
# to be ECDSA and checks signatures accordingly. | ||
# `validate_frontier_transaction` checks for the balance of transaction | ||
# sender. | ||
# Both of which are in confilct with Account Abstraction implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this whole comment can be removed with the understanding that this is just a new validation function.
evm/vm/message.py
Outdated
|
||
@property | ||
def is_create(self): | ||
return self.code not in (None, b'') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, it'd be nice to have a single representation of empty. What do you think about changing message.code
to always be a bytestring so that b''
is the only empty value and we can just return bool(self.code)
evm/rlp/transactions.py
Outdated
Hook called during instantiation to ensure that all transaction | ||
parameters pass validation rules. | ||
""" | ||
if self.intrensic_gas > self.gas: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct spelling is "intrinsic".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pipermerriam I will update the naming in sharding branch along with this PR and update the one in master branch with a new PR.
We should wait for #244 to be merged as this will need to be refactored to the new architecture introduced in that PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be one un-used function. Otherwise this looks solid.
@@ -42,6 +44,27 @@ def _create_message(gas=1, | |||
) | |||
|
|||
|
|||
def _create_sharding_message( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't appear to be used anywhere.
3c0c378
to
b02b514
Compare
b02b514
to
519afdc
Compare
…new transaction format
…m Message to ShardMessage
…g validation logic
519afdc
to
2646a10
Compare
@pipermerriam rebased on #250 and updated to new computation logic(#244). If everything looks fine I will go ahead and merge this. |
What was wrong?
With new transaction format and to introduce ENTRY_POINT address(b'ffff….ffff'), some logics in
vm.execute_transaction
needed to be changed.Then CREATE2(#224), PAYGAS(#234) and PANIC opcodes can be built on top.
How was it fixed?
ShardingMessage
,ShardingTransaction
,ShardingVM
ShardingTransaction.intrinsic_gas
validate_sharding_transaction
ENTRY_POINT
generate_create2_contract_address
Cute Animal Picture