Standard user account #230

jannikluhn · 2017-12-19T17:43:05Z

The standard user account is a contract parametrized by an address that

Validates that the transaction is signed with the private key corresponding to the address,
validates that the block number is in a certain range (given by the transaction),
stores a nonce that is incremented at every call,
pays for gas, and
calls another contract.

It also has additional methods to

extract the nonce and to
validate an arbitrary signed message against the owner address.

Once the PR is finished it should contain

the contract code both as Vyper LLL and compiled bytecode
extensive tests for the contract code
utility functions to construct transactions in a format understood by the contract
utility functions to deploy such a contract (which might be split into a main and a validation contract)

References:

pipermerriam

Looks good.

pipermerriam · 2017-12-19T18:19:13Z

evm/auxiliary/user_account.py

+    #
+
+    @property
+    def signature(self):


what do you think about renaming this to vrs instead of signature? I think that naming convention would match better with other portions of both this codebase and others (web3.py and eth-keys).

With the conventions we've been establishing, I would expect this to return a eth_keys.Signature object.

Do you prefer renaming it to vrs or making it return a Signature object? I have no preference in that regard.

I think vrs is my preference since this is a low-level API

pipermerriam · 2017-12-19T18:25:04Z

evm/auxiliary/user_account.py

+        self.data = self.data[:VALUE_SLICE.start] + b + self.data[VALUE_SLICE.stop:]
+
+    @property
+    def receiver(self):


This fragments the terminology that we use for the destination of a transaction. Elsewhere we're using to. Any thoughts on whether it'd be better to conform to this naming convention?

Not sure... We used to have to and sender. Now we have target, sender and receiver.

to and target refer to more or less the same thing I guess (the first account that is called)

sender changes meaning: it used to be the one who signed the transaction, now it's the "owner" of the target account (and just happens to put a signature in the data field)

receiver is the second hop (the account that's called by the target account)

So from a technical standpoint, I don't see a lot of overlap between original and new terms (sender and receiver are new concepts, and don't live at the protocol level at all). I could imagine renaming target to to for consistency (although I'd prefer target in a vacuum). Renaming receiver to to could be misleading on the protocol level, but would make sense from a high level point of view in most cases. I don't see a good alternative to sender.

Good explanation. I think you are right to leave this as-is and not change the names from target/sender/receiver

Does the name destination better capture the concept that receiver currently represents?

pipermerriam · 2017-12-19T18:28:01Z

evm/logic/context.py

+
+
+def sighash(computation):
+    pass


thoughts on changing this to a raise NotImplementedError?

pipermerriam · 2017-12-19T18:28:06Z

evm/logic/system.py

+
+
+def paygas(computation):
+    pass


thoughts on changing this to a raise NotImplementedError?

pipermerriam · 2017-12-19T18:29:22Z

evm/vm/forks/sharding/opcodes.py

+        mnemonic=mnemonics.PAYGAS,
+        gas_cost=0,
+    ),
+


pipermerriam · 2017-12-19T18:32:00Z

tests/auxiliary/user-account/test_user_account.py

+
+def test_valid_transfer(chain, tx1):
+    vm = chain.get_vm()
+    vm.apply_transaction(tx1)


Are there any assertions that we could make here?

Definitely. That's one of the reasons why it's still WiP (this test doesn't even pass yet)

pipermerriam · 2017-12-19T18:33:01Z

evm/auxiliary/user_account.py

+
+
+USER_ACCOUNT_CODE = decode_hex(
+    "0x3f595260805903608020600052602060006080600060006001610bb8f1506001600051"


Will you add a comment referencing the source code that produced this bytecode and how it can be reproduced/verified.

Yes, this is not much more than a placeholder for now. Ideally I'd like to put the source code there directly and compile it in a test for comparison, but this will probably not work because Viper needs Python 3.6.

#206 has the same issue, it's better not import Viper even in Python 3.6.

A (little complicated) suggestion of verifying Viper code:

A command for compiling Viper source code to bytecode.

If someone want to update the code, they have to upload source code and bytecode (can be in separate file or assigned to a global variable in this case)

Install Viper in CI testing.

Write a test for auto-compiling the source code and then comparing the compiled bytecode with evm.auxiliary.user_account.USER_ACCOUNT_CODE.

By the way, @pipermerriam what do you think about upgrading to Python 3.6?

Perhaps the cleanest solution would be to put the code plus tests in a separate repository. The repository would contain packages with the bytecode for different languages that can be imported into client implementations such as Py-EVM.

By the way, @pipermerriam what do you think about upgrading to Python 3.6?

I'm in favor: ethereum/py-trie#18

I'm indifferent to the actual solution for the bytecode creation as long as it's documented and independently verifiable.

jannikluhn · 2017-12-22T15:06:15Z

I've given up on testing the contract code for now, as it's not really possible at the moment. I'll come back to it once #238 is done, but for now I'd like to put this PR on hold.

pipermerriam

Would you mind taking a minute to flesh out the main description for this pull request.

pipermerriam · 2018-01-08T22:42:27Z

I think that a large part of this diff will go away once this is rebased.

jannikluhn · 2018-01-09T10:39:55Z

Updated the description. Sorry for the current state of the PR. Now that #238 is merged I'm back at writing tests/debugging the contract. In the meantime I've made a couple of changes in the contract. I'm going to push something later today.

jannikluhn · 2018-01-10T18:00:33Z

Still not completely finished, but almost there I hope.

To run the tests, cherry-pick 90d155d, resolve the merge conflicts (accept both), and replace this line with pass.

pipermerriam · 2018-01-11T17:26:03Z

Sighash is now merged so this can be rebased (as I assume it needs sighash in it to work)

pipermerriam · 2018-01-11T17:27:15Z

eh, should have said sighash is ready for merge :P

jannikluhn · 2018-01-11T17:41:50Z

Merged the sighash PR and rebased. This should be ready for review now.

I'm not particularly happy with some names in this one, especially ForwardingTransaction. I called it forwarding because the "payload" data is passed through to the actual "destination" contract. But it's specific to one particular user contract, others might require a different format.

Test-wise I'd like to see tests that make sure that everything works as expected even if the gas runs out at different points during the execution of the contract. But this can only be done once PAYGAS is properly implemented.

jannikluhn · 2018-01-11T17:59:59Z

Ooh, I missed one thing: access_list is left empty all the time. It should default to the whole storage space of the destination account and to the required parts of user and validation accounts (as suggested here). Tests are not failing because of #263 .

pipermerriam

Just a few suggestions for cleanup/readability/opinionated-code-style

pipermerriam · 2018-01-11T18:11:07Z

evm/auxiliary/user_account_contract/transaction.py

+EMPTY_DATA = b'\x00' * DESTINATION_SLICE.stop
+
+
+class ForwardingTransaction(ShardingTransaction):


Other name ideas

StandardAccountTransaction

ProxyTransaction

EIP86Transaction (or whatever the appropriate EIP number is)

StandardAccountTransaction was the name before I switched to ForwardingTransaction. I dismissed it because I'm not sure if "standard account" is a good term either. It should probably be rather called "standard user account", and even that assumes that this contract will actually become "the" standard. But StandardUserAccountTransaction strikes me as too long.

General note on EIPs: As far as I know there are no EIPs for the sharding protocol itself. At least at the moment, I don't know if that will change.

pipermerriam · 2018-01-11T18:17:00Z

evm/auxiliary/user_account_contract/transaction.py

+            self.sig_hash,
+        ])
+
+    def sign(self, private_key):


Was it intentional to deviate from the existing API for transaction signing which uses as_signed_transaction which returns a new object rather than mutating the existing object?

This question is from a broader question about the use of property setters.

if the use of property setters was to embed validation as part of setting the value, then I would suggest changing to use a single validate() method which is inline with the other transaction classes.

if the use of property setters was to allow mutation of the object, I would like to understand why since we've trended away from most forms of object mutation.

I added the setters to allow mutation. The other transaction objects are technically mutable as well and I wanted to mirror that. But if they aren't mutated in practice I'll be happy to remove them because all these setters look quite ugly. I'll also try to implement the as_signed_transaction pattern.

pipermerriam · 2018-01-11T18:22:37Z

tests/auxiliary/user-account/test_contract.py

+    "code": b"",
+}
+
+DEFAULT_TX_PARAMS = {**DEFAULT_BASE_TX_PARAMS, **{


This method of dictionary merging has always bugged me, mostly because it takes advantage of the implementation rather than using formal semantics or which keys take precedence. I admit it could be argued that the way key precedence is handled in this case is part of the formal semantics for how this works, but either way, it's always felt a bit like magic.

What do you think about making use of something like the following. (If you haven't figured it out by now, I have great love for the toolz/cytoolz library)

from cytoolz import ( merge, dissoc, ) DEFAUT_TX_PARAMS = merge( dissoc(DEFAULT_BASE_TX_PARAMS, 'code'), {...<the overrides>}, }

Zero mutation and explicit about dictionary merging.

Personally I don't prefer one way over the other, so I don't mind adopting this style.

pipermerriam · 2018-01-11T18:24:48Z

tests/auxiliary/user-account/test_contract.py

+        (v, r, s - 1),
+        (v, r, 0),
+
+        (27 if v == 28 else 28, r, SECPK1_N - s)


can we parametrize these using pytest.mark.parametrize so that they each run in isolation?

pipermerriam · 2018-01-11T18:27:36Z

tests/auxiliary/user-account/test_contract.py

+
+    logs = computation.get_log_entries()
+    assert len(logs) == 1
+    assert big_endian_to_int(logs[0][-1]) > 900 * 1000  # some gas will have been consumed earlier


A slightly easier to understand version of this assertion that I've used elsewhere is to assert that the delta is smaller than a certain value.

gas_used = big_endian_to_int(logs[0][-1]) assert transaction.gas - gas_used < 1000

hwwhww · 2018-01-12T09:16:48Z

tests/auxiliary/user-account/test_contract.py

+        block_number=10,
+        gas_limit=3141592,
+        timestamp=1422494849,
+        parent_hash=decode_hex("0000000000000000000000000000000000000000000000000000000000000000"),


You can use evm.constants.ZERO_HASH32.

pipermerriam · 2018-01-12T22:51:34Z

evm/auxiliary/user_account_contract/transaction.py

            gas=gas,
            gas_price=gas_price,
            access_list=access_list,
            code=b'',
        )
+
+    def create_unsigned_transaction(self):


To stick with the existing transaction API this should be a classmethod. This API exists to reduce the cumbersome import of both the signed and unsigned classes, allowing you to create unsigned transactions from the main transaction class. I'd be fine with this just being an *args, **kwargs passthrough to reduce size since there are 10+ arguments.

pipermerriam · 2018-01-12T22:58:38Z

tests/auxiliary/user-account/test_contract.py

+            s=s,
+        )
+        # don't detect invalidity before transaction execution
+        transaction.validate = lambda: None


I think a more appropriate way to check this would be to assemble a Message and then to run vm.get_computation(message).apply_message().

This should execute the transaction below the place where the call to transaction.validate() occurs, allowing you to directly test that the EVM code appropriately validates the signature without having to do any monkeypatching of the transaction object.

hwwhww · 2018-01-13T17:40:30Z

I was checking with @vbuterin about renaming some parameters and also mentioned about
ShardingTransaction.to or ShardingTransaction.target, he clarified his intention:

so the reason I wanted to rename it to target is that in real life, the transaction.to field will be the sender of the money (and/or function call)
as it would be the forwarding contract that controls the account from which the funds get sent and the receiver would be specified in the tx.data under some encoding
I think it would cause too much long-term risk of devs getting confused for transaction.to to be the sender
hence my idea to rename it target

jannikluhn · 2018-01-15T14:09:46Z

Addressed comments and rebased.

@vbuterin Do you have time to review the contract code? https://github.com/ethereum/py-evm/pull/230/files#diff-05178e170e27782a10a1498ec2aefdef

vbuterin · 2018-01-15T16:12:02Z

evm/auxiliary/user_account_contract/contract.py

+        ['if',
+            ['eq',
+                ['calldataload', CALLDATA_FUNCTION_ID],
+                NONCE_GETTER_ID],


Shouldn't NONCE_GETTER_ID be multiplied by 2^224? It's the first four bytes that are the function sig, so getting the first 32 bytes would return that with 28 zero bytes at the end.

BTW are there tests for all of this? If so, the tests should catch this.

You're right, will fix this.

Tests are here: https://github.com/jannikluhn/py-evm/blob/07587ac50734c62a7d8862c072fbcaeedaa93c97/tests/auxiliary/user-account/test_contract.py

They cheat a bit by importing the function ids, that's why they didn't catch this. Will add a test comparing against a manually generated one (using the Vyper code).

vbuterin · 2018-01-15T16:14:20Z

evm/auxiliary/user_account_contract/contract.py

+                ['calldataload', CALLDATA_FUNCTION_ID],
+                VALIDATION_CODE_GETTER_ID,
+                # return validation code address
+                ['mstore', 0, validation_code_address],


Now that I think about it, what would the validation code address be? We don't want to create two contracts per account. I would actually recommend just not bothering with this get_validation_code feature at this point to keep it simpler.

vbuterin · 2018-01-15T16:28:03Z

evm/auxiliary/user_account_contract/contract.py

+                # output data (discarded)
+                0,
+                0
+            ]],


Let's not do things this way. This would require calling 2 accounts to send from this account, which would blow up witness size by a factor of 2. IMO just do ecrecover here.

vbuterin · 2018-01-15T16:49:39Z

evm/auxiliary/user_account_contract/contract.py

+        ['sstore', STORAGE_NONCE, ['add', ['calldataload', CALLDATA_NONCE], 1]],
+
+        # Assert that we won't run out of gas from here on
+        ['assert', ['gt', 'gas', GAS_RESERVE + GAS_RESERVE_OFFSET]],


Is this line still necessary?

Depends: If we run out of gas after the PAYGAS call, the transaction can be replayed infinitely and the account would get drained. This line makes sure that this can never happen. If we trust the user to never create transactions with not enough gas, the check isn't necessary. But at least I would feel much safer with it.

jannikluhn · 2018-01-16T10:01:50Z

Anything else or can we merge?

jannikluhn · 2018-01-17T09:31:40Z

I'm also in favor of renaming to to target, but I think this should be done in a separate pull request.

jannikluhn · 2018-01-18T09:17:52Z

Rebased. Is this Ready-for-merge when tests have passed?

hwwhww · 2018-01-18T16:21:38Z

evm/auxiliary/user_account_contract/contract.py

+VALIDATION_CODE_GAS = 3500
+GAS_RESERVE = 4500  # amount of gas reserved for returning
+GAS_RESERVE_OFFSET = 200
+NONCE_GETTER_ID = 0x141b5b48


maybe add a comment to describe
# NONCE_GETTER_ID = big_endian_to_int(keccak(b"get_nonce()")[:4]) ?

pipermerriam

Good to merge (when tests are green)

hwwhww · 2018-01-19T08:51:59Z

They are green now 🎉

jannikluhn added eth2.0 PR state: WIP labels Dec 19, 2017

pipermerriam approved these changes Dec 19, 2017

View reviewed changes

pipermerriam force-pushed the sharding branch from 0b9e4fb to 62e4934 Compare January 4, 2018 19:31

jannikluhn mentioned this pull request Jan 8, 2018

Add SIGHASH opcode #226

Closed

pipermerriam reviewed Jan 8, 2018

View reviewed changes

jannikluhn force-pushed the standard-user-account branch from 91d225f to 095b7c1 Compare January 10, 2018 17:55

jannikluhn force-pushed the standard-user-account branch from 55215ce to f5d854a Compare January 11, 2018 17:39

jannikluhn added Needs Review and removed PR state: WIP labels Jan 11, 2018

pipermerriam approved these changes Jan 11, 2018

View reviewed changes

hwwhww reviewed Jan 12, 2018

View reviewed changes

pipermerriam requested changes Jan 12, 2018

View reviewed changes

jannikluhn force-pushed the standard-user-account branch 2 times, most recently from f7897d9 to 35476e8 Compare January 15, 2018 13:36

vbuterin reviewed Jan 15, 2018

View reviewed changes

pipermerriam approved these changes Jan 16, 2018

View reviewed changes

jannikluhn added 19 commits January 18, 2018 10:02

Add standard account contract code

12af1c5

Add helper transaction type

cb4b28f

Add tests for standard account contract

ce920d8

flake8

6eed710

Add PAYGAS stub

8081d7e

Check logs of subcalls in contract tests

5b74057

Move signature verification to validation contract

5007236

flake8

0a4d7a4

Make transaction immutable, add validation

ea73a78

Fix and clean up tests

41dbaaf

flake8

d6d9b33

Fix invalid signature test

b93e572

Obey correct create_signed_transaction interface

2adce49

Fix bug in contract.

920a02c

Create utils.py

840eaa1

Validate to/target in ShardingTransaction

97bf4a0

Parametrize signature test

c2776c9

Don't use validation code pattern in user contract

d6a38f5

Fix function ID check

2d305bf

jannikluhn force-pushed the standard-user-account branch from f6f8429 to 2d305bf Compare January 18, 2018 09:14

jannikluhn mentioned this pull request Jan 18, 2018

Check access restrictions during tx execution #276

Closed

hwwhww reviewed Jan 18, 2018

View reviewed changes

Add comment clarifying origin of NONCE_GETTER_ID

96ae4e1

pipermerriam approved these changes Jan 18, 2018

View reviewed changes

jannikluhn merged commit 081a851 into ethereum:sharding Jan 19, 2018

hwwhww mentioned this pull request Jan 20, 2018

Standard Account Contract #225

Closed

2 tasks



		USER_ACCOUNT_CODE = decode_hex(
		"0x3f595260805903608020600052602060006080600060006001610bb8f1506001600051"

		EMPTY_DATA = b'\x00' * DESTINATION_SLICE.stop


		class ForwardingTransaction(ShardingTransaction):

Standard user account #230

Standard user account #230

Conversation

jannikluhn commented Dec 19, 2017 • edited Loading

pipermerriam 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

jannikluhn Dec 20, 2017 • edited Loading

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

jannikluhn commented Dec 22, 2017

pipermerriam left a comment

Choose a reason for hiding this comment

pipermerriam commented Jan 8, 2018

jannikluhn commented Jan 9, 2018

jannikluhn commented Jan 10, 2018

pipermerriam commented Jan 11, 2018

pipermerriam commented Jan 11, 2018

jannikluhn commented Jan 11, 2018

jannikluhn commented Jan 11, 2018 • edited Loading

pipermerriam 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

hwwhww commented Jan 13, 2018

jannikluhn commented Jan 15, 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

Choose a reason for hiding this comment

jannikluhn commented Jan 16, 2018

jannikluhn commented Jan 17, 2018

jannikluhn commented Jan 18, 2018

Choose a reason for hiding this comment

pipermerriam left a comment

Choose a reason for hiding this comment

hwwhww commented Jan 19, 2018

jannikluhn commented Dec 19, 2017 •

edited

Loading

jannikluhn Dec 20, 2017 •

edited

Loading

jannikluhn commented Jan 11, 2018 •

edited

Loading