Remove validation code #348

mhchia · 2018-02-04T08:54:18Z

What was wrong?

As the item#3 in ethereum/sharding#37 described, we remove the validation code logics from VMC, to reduce gas consumption.

How was it fixed?

Remove validation code construction
Modify add_header to accept parameters directly, instead of the rlp encoded bytes.
Verify msg.sender in add_header and withdraw, instead of using validation code.

Cute Animal Picture

hwwhww

A great downsizing for VMC. 👍

hwwhww · 2018-02-04T09:44:52Z

evm/vm/forks/sharding/contracts/validator_manager.v.py

@@ -46,8 +41,8 @@
 # Gas limit of the signature validation code
 sig_gas_limit: num


It's useless now.

Thank you for pointing out. Will remove it

hwwhww · 2018-02-04T10:17:20Z

evm/vm/forks/sharding/contracts/validator_manager.v.py

+        new_head_in_num = 0
+    raw_log(
+        [
+            sha3("CollationAdded(int128,bytes4096,bool,int128)"),


Are these data types matching the actual results?

Yes, actually it matches log signature CollationAdded(indexed int128 shard, bytes4096 collationHeader, bool isNewHead, int128 score).
The reason why I use int128(i.e. num in vyper) is it is more friendly for calculation. And the reason why I use raw_log instead of log.CollationAdded() is previously vyper doesn't support bytes type whose length > 32. Actually there is a TODO comment in line 67

Can the collationHeader argument from CollationAdded be unpacked?

So instead of using CollationAdded(indexed int128 shard, bytes4096 collationHeader, bool isNewHead, int128 score)
Use CollationAdded(indexed int128 shard, int128 expectedPeriodNumber, bytes32 periodStartPrevHash, bytes32 _parentCollationHash, bytes32 txListRoot, address collationCoinbase, bytes32 postStateRoot, bytes32 receiptRoot, int128 collationNumber, bool isNewHead, int128 score)

The benefits would be: 1 int less for the log: shard; easier to use with the ABI.

I think it's a good point. I will see if there are specific reasons for using CollationAdded(indexed int128 shard, bytes4096 collationHeader, bool isNewHead, int128 score). If not, we can change to that.
Thank you for the suggestion.

hwwhww · 2018-02-04T10:28:14Z

evm/vm/forks/sharding/vmc_handler.py

-                gas_price=None):
-        """deposit(validation_code_addr: address, return_addr: address) -> num
+    def deposit(self, gas=None, gas_price=None):
+        """deposit() -> num


I guess you mean the deposit function in VMC returns the index of validators mapping, but this docstring looks meaningless here. IMO it may be more helpful to describe the action of this function itself.

Will change it. Thank you!

hwwhww · 2018-02-04T10:28:42Z

evm/vm/forks/sharding/vmc_handler.py

-    def withdraw(self, validator_index, sig, gas=None, gas_price=None):
-        """withdraw(validator_index: num, sig: bytes <= 1000) -> bool
+    def withdraw(self, validator_index, gas=None, gas_price=None):
+        """withdraw(validator_index: num) -> bool


hwwhww · 2018-02-04T10:29:00Z

evm/vm/forks/sharding/vmc_handler.py

+                   receipt_root,
+                   collation_number,
+                   gas=None,
+                   gas_price=None):
        """add_header(header: bytes <= 4096) -> bool


hwwhww · 2018-02-04T10:40:22Z

tests/sharding/test_vmc_handler.py

    )
-    sig = sign(sighash, privkey)
-    return rlp.encode([
+    header_tuple = (


How about using evm.rlp.headers.CollationHeader to build this object?

Thank you for pointing it out!
I think it's a good idea to use evm.rlp.headers.CollationHeader to build the collation header from logs. I will change it.
But actually the result of mk_testing_colhdr is designed to be passed to VMC.add_header. How do you think in this condition?
Personally I prefer to keep this behavior here.
Will it help if I change the function name to mk_testing_colhdr_params?

I'm thinking when the collator calls VMC.add_header, the collator would have built a real collation before. So there may be a property in CollationHeader:

@property def add_header_params: # returns header fields except `sig` ...

But actually the result of mk_testing_colhdr is designed to be passed to VMC.add_header. How do you think in this condition?
Personally I prefer to keep this behavior here.
Will it help if I change the function name to mk_testing_colhdr_params?

The second output (header_hash) of mk_testing_colhdr is not the input of VMC.add_header but the input of next mk_testing_colhdr call.

header_hash can be another property of CollationHeader. In this way, this mk_testing_colhdr function could just return CollationHeader object, and these two properties could be accessed in test_vmc_contract_calls.

header_hash can be another property of CollationHeader. In this way, this mk_testing_colhdr function could just return CollationHeader object, and these two properties could be accessed in test_vmc_contract_calls.

It makes sense. I will change it. Thank you!

Hold on. So in this case should I add another header_hash, which simply returns keccak(to_bytes32(shard_id) ++ to_bytes32(expected_period_number) ++ ...), is that correct?
Since the header_hash is no more sha3(rlp_list([fields]).

hwwhww · 2018-02-04T10:42:20Z

tests/sharding/test_vmc_handler.py

    """
-    Deploy validation code of and with the privkey, and do deposit
+    Deposit a validator


More precisely, "do deposit in VMC to be a validator"?

Ok no problem

hwwhww · 2018-02-04T18:02:42Z

I think this PR also closes #325. ✌🏻

pipermerriam · 2018-02-05T16:28:09Z

evm/rlp/headers.py

+        )
+        header_hash = keccak(
+            b''.join(
+                (


whitespace nitpick

This should be a bit easier to read if you write this as:

b''.join(( thing_a, thing_b, ))

You're right. I will change it.

pipermerriam · 2018-02-05T16:30:55Z

evm/vm/forks/sharding/vmc_handler.py

@@ -44,27 +46,35 @@ class UnknownShard(Exception):
    pass


+@to_dict
+def deserialize_header_bytes(header_bytes):


This function feels like something that should be a classmethod on the CollationHeader class.

Thank you for this great suggestion! I will change it.

pipermerriam · 2018-02-05T16:31:17Z

evm/vm/forks/sharding/vmc_handler.py

+        start_index = idx * obj_size
+        field_bytes = header_bytes[start_index:(start_index + obj_size)]
+        if field_type == rlp.sedes.big_endian_int:
+            # remove the leading zeros, to avoid `not minimal length` error in deserialization


is this a bug in the rlp library? If so can you open an issue for it in that repository?

I'm not sure. It just requires the big_endian_int to be in minimal length (e.g. b'\x00\x01' should be b'\x01')
The error is like this

self = <rlp.sedes.big_endian_int.BigEndianInt object at 0x111d6bb38> serial = b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x06' def deserialize(self, serial): if self.l is not None and len(serial) != self.l: raise DeserializationError('Invalid serialization (wrong size)', serial) if self.l is None and len(serial) > 0 and serial[0:1] == ascii_chr(0): raise DeserializationError('Invalid serialization (not minimal ' > 'length)', serial) E rlp.exceptions.DeserializationError: Invalid serialization (not minimal length) /home/xxx/.pyenv/versions/py36env/lib/python3.6/site-packages/rlp/sedes/big_endian_int.py:40: DeserializationError

Will dig more into it to see if it is an issue.
Thank you:)

pipermerriam · 2018-02-05T16:32:26Z

evm/vm/forks/sharding/vmc_handler.py

@@ -158,6 +168,9 @@ def __init__(self, *args, default_privkey, **kwargs):

        super().__init__(*args, **kwargs)

+    def get_default_sender_address(self):


This function doesn't appear to be used anywhere.

Actually I used them in test_vmc_handler.py, to get the default_sender_address for doing transactions.
Should I remove them? I can replace them by pass the address in use as argument in those testing functions in test_vmc_handler.py

I must have missed it. disregard.

enriquefynn · 2018-02-05T19:23:39Z

evm/vm/forks/sharding/contracts/validator_manager.v.py

+        receipt_root,
+        as_bytes32(collation_number),
+    )
+    entire_header_hash = sha3(header_bytes)
    assert entire_header_hash != as_bytes32(0)


Assert is not needed anymore

You're right. Thank you for pointing out.

hwwhww · 2018-02-06T06:43:23Z

tests/sharding/test_vmc_handler.py

+def mk_initiating_transactions(sender_privkey,
+                               sender_starting_nonce,
+                               TransactionClass,
+                               gas_price):
    """Make transactions of createing initial contracts
    Including rlp_decoder, sighasher and validator_manager


No rlp_decoder and sighasher now, so the docstring needs to be updated.
Also createing -> creating.

No problem. Thank you for discovering it.

hwwhww · 2018-02-06T07:06:31Z

evm/vm/forks/sharding/vmc_handler.py

            ],
            gas=gas,
            gas_price=gas_price,
        )
        return tx_hash

-    def add_header(self, header, gas=None, gas_price=None):
-        """add_header(header: bytes <= 4096) -> bool
+    def add_header(self,


VMC.add_header takes a lot of parameters and combines add_header inputs and gas setting in the same level. What do you think about passing CollationHeader to this function and moving logic of add_header_send_tx to here so that the params would be clearer:

def add_header(self, collation_header, gas=None, gas_price=None)

Good idea! I will change it. Thank you.

mhchia · 2018-02-06T16:45:28Z

@enriquefynn I have changed the signature of log CollationAdded. Please check it out if you have spare time.
Thank you for the great suggestion!

pipermerriam · 2018-02-06T16:52:38Z

evm/rlp/headers.py

@@ -196,15 +207,57 @@ def __repr__(self):

    @property
    def hash(self):
-        return keccak(rlp.encode(self))
+        int_to_bytes32 = compose(


I'd be good with this living in evm.utils.numeric. I suspect there are other places in the code where we'd use it.

I have moved it to evm.utils.numeric, and added two tests for it.
Hope that would help.

pipermerriam · 2018-02-06T16:53:33Z

evm/rlp/headers.py

+        header_sedes = cls.get_sedes()
+        # assume all fields are padded to 32 bytes
+        obj_size = 32
+        assert len(header_bytes) == obj_size * len(header_sedes)


Assertions are generally bad for runtime code as they can be easily disabled. This should probably raise a ValidationError

Sorry for that. I have changed it. Thank you!

pipermerriam · 2018-02-06T16:54:41Z

evm/vm/forks/sharding/vmc_handler.py

@@ -158,6 +168,9 @@ def __init__(self, *args, default_privkey, **kwargs):

        super().__init__(*args, **kwargs)

+    def get_default_sender_address(self):


I must have missed it. disregard.

mhchia · 2018-02-07T16:12:55Z

@pipermerriam Could you help me to review the newly added commits if you have spare time? Since I added the tests for int_to_bytes32, not sure if they are okay for merging.
Really appreciate it. :)

pipermerriam · 2018-02-07T16:26:57Z

evm/rlp/headers.py

-        assert len(header_bytes) == obj_size * len(header_sedes)
+        if len(header_bytes) != obj_size * len(cls.fields):
+            raise ValidationError(
+                "Length of header bytes should be equal to obj_size * len(cls.fields)"


I would actually be very explicit here as it's frustrating to have to go dig when there is code mis-behaving.

raise Validation Error( "Expected header bytes to be of length: {0}. Got length {1} instead.\n- {2}".format( obj_size * len(cls.fields), len(header_bytes), encode_hex(header_bytes), ) )

Good error messages are soooo valuable when things start going pear shaped.

Thank you sooo much! I didn't think of this. :)
I have changed it and really learned a lesson.
I will keep reminding myself with this suggestion.

pipermerriam · 2018-02-07T16:27:57Z

evm/utils/numeric.py

+        pad32,
+    )
+    if len(value_bytes) > 32:
+        raise ValueError("value is too large")


Instead of doing post validation, how about just adding a pre-condition check that it's a valid uint256 and raising a ValueError if it isn't. (hint, I don't think we should use evm.validation.validate_uint256 here since it's outside of evm.utils. Instead, just inline the same validation code.

Btw, I suggest ValueError for the same reason, not importing from outside of evm.utils from within evm.utils.

Thank you for suggesting this.
Actually I'm curious how to know when to import one function from outside directory or just inline it?
I can guess in this case the validation is simple so the cost(DRY) is not expensive. But if it is in other case, is it not good to import function in another layer?

From `header <= 4096` to separate unpacked parameters

Besides, remove the call to `get_eriod_prevhash`

- Remove unneeded `sig_gas_limit` in VMC - Modify the docstrings in VMC functions - Modify `do_deposit`, return `vmc.default_sender_address` instead. And change its docstring - Add getter for `default_sender_address` in VMC - Change `genesis_colhdr_hash` to upper case

For the consistency with `add_header_constant_call`

- Fix coding style - Move `deserialize_header_bytes` and add `from_bytes` to `CollationHeader` - Add test for `CollationHeader.from_bytes` - Remove unneeded assert in VMC

Remove unused imports

`collation_header` changes from `bytes` to respective parameters Besides, add log for `Deposit` and `Withdraw`. TODO: - Determine the exact log signature of `Deposit` and `Withdraw - Determine the log signature of `TxToShard`

- Move `int_to_bytes32` to evm.utils.numeric - Raise `ValidationError` instead of AssertionError in `CollationHeader.from_bytes` - Add test for invalid length for `from_bytes`

- Change the error message, adding the value of variables - Copied verification code from `evm.validation.validate_uint256`

mhchia added Needs Review eth2.0 labels Feb 4, 2018

mhchia mentioned this pull request Feb 4, 2018

Modify doc due to the removal of validation code logics ethereum/sharding#70

Merged

hwwhww requested changes Feb 4, 2018

View reviewed changes

prestonvanloon mentioned this pull request Feb 5, 2018

Validator manager contract in solidity prysmaticlabs/prysm#4

Merged

2 tasks

mhchia force-pushed the sharding_vmc_modify_add_header branch from ac040f0 to b1bf62a Compare February 5, 2018 14:58

pipermerriam reviewed Feb 5, 2018

View reviewed changes

enriquefynn reviewed Feb 5, 2018

View reviewed changes

hwwhww approved these changes Feb 6, 2018

View reviewed changes

hwwhww force-pushed the sharding branch from 04a688f to 5ff6dd2 Compare February 6, 2018 10:38

mhchia force-pushed the sharding_vmc_modify_add_header branch 2 times, most recently from 255fb42 to 6eefd26 Compare February 6, 2018 14:12

pipermerriam approved these changes Feb 6, 2018

View reviewed changes

pipermerriam approved these changes Feb 7, 2018

View reviewed changes

mhchia force-pushed the sharding_vmc_modify_add_header branch from 5daaa31 to dd4c8d5 Compare February 8, 2018 12:49

mhchia added 12 commits February 10, 2018 14:58

Remove validation code logics from VMC

30e80bb

Modify the parameter of add_header

319634a

From `header <= 4096` to separate unpacked parameters

Remove rlp_decoder and sighasher

d5abc3f

Besides, remove the call to `get_eriod_prevhash`

Remove unneeded functions

e972774

Remove unneeded imports

1d6a8f3

Remove [DEPRECATED] and unneeded functions

66bffdb

Remove the field cycle from validators

9bdb81a

Remove msg_hash in withdraw

34a5741

Use CollationHeader for the header from logs

d8b8bc7

Rename send_add_header_tx to add_header_send_tx

37f89de

For the consistency with `add_header_constant_call`

Several changes reflecting the PR review

6dfa952

- Fix coding style - Move `deserialize_header_bytes` and add `from_bytes` to `CollationHeader` - Add test for `CollationHeader.from_bytes` - Remove unneeded assert in VMC

mhchia added 11 commits February 10, 2018 14:58

Take CollationHeader as param in VMC.add_header

33005dd

Modify the comment in add_header_constant_call

a3a9ef5

flake8

5877363

Remove unused imports

Change the signature of log CollationAdded

5210cbe

`collation_header` changes from `bytes` to respective parameters Besides, add log for `Deposit` and `Withdraw`. TODO: - Determine the exact log signature of `Deposit` and `Withdraw - Determine the log signature of `TxToShard`

Multiple modification

5b7fc73

- Move `int_to_bytes32` to evm.utils.numeric - Raise `ValidationError` instead of AssertionError in `CollationHeader.from_bytes` - Add test for invalid length for `from_bytes`

Modify TODO in get_eligible_proposer

2ae0e12

Add error handling and test for int_to_bytes32

a342227

Clean the contract, remove unused statements

bddc7f5

Reflect to PR

c40c22b

- Change the error message, adding the value of variables - Copied verification code from `evm.validation.validate_uint256`

flake8

f02de29

Remove ErrorType

ee0deb5

mhchia force-pushed the sharding_vmc_modify_add_header branch from 4d61d30 to ee0deb5 Compare February 10, 2018 06:58

mhchia merged commit d919d2d into ethereum:sharding Feb 10, 2018

hwwhww mentioned this pull request Feb 28, 2018

STUB: Remove shuffling cycle from VMC #325

Closed

		@@ -46,8 +41,8 @@
		# Gas limit of the signature validation code
		sig_gas_limit: num

		@@ -158,6 +168,9 @@ def __init__(self, args, default_privkey, *kwargs):

		super().__init__(args, *kwargs)

		def get_default_sender_address(self):

Remove validation code #348

Remove validation code #348

Conversation

mhchia commented Feb 4, 2018

What was wrong?

How was it fixed?

Cute Animal Picture

hwwhww 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

mhchia Feb 6, 2018 • edited

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

mhchia Feb 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwwhww Feb 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhchia Feb 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwwhww commented Feb 4, 2018

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

mhchia Feb 6, 2018 • edited

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

enriquefynn Feb 5, 2018 • edited

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

hwwhww Feb 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhchia commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhchia Feb 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhchia commented Feb 7, 2018

Choose a reason for hiding this comment

mhchia Feb 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhchia Feb 6, 2018 •

edited

mhchia Feb 4, 2018 •

edited

hwwhww Feb 4, 2018 •

edited

mhchia Feb 5, 2018 •

edited

mhchia Feb 6, 2018 •

edited

enriquefynn Feb 5, 2018 •

edited

hwwhww Feb 6, 2018 •

edited

mhchia Feb 7, 2018 •

edited

mhchia Feb 8, 2018 •

edited