Contract creation transactions don't specify a salt

[jannikluhn]

What is wrong?

Contract creation transactions don't specify a salt (see https://github.com/ethereum/py-evm/blob/sharding/evm/vm/forks/sharding/vm_state.py#L60-L63). As a result, the address of the created contract is given by the bytecode only and thus it's impossible to deploy the same contract twice.

How can it be fixed

Specify the salt somehow. Potential candidates could be

• the first/last 32 bytes of transaction.code
• the first/last 32 bytes of transaction.data
• transaction.to (which is meaningless for contract creation transactions right now)

[NIC619]

One can append random bytes(which won't affect contract initialization process) to the bytecode as salt.

[NIC619]

@pipermerriam @hwwhww @jannikluhn
It seems cleaner if we just add salt to transaction format, as suggested by @vbuterin and to conform with suggested transaction format of main chain in this EIP.

[NIC619]

I will make the PR that together removes gas_price and add salt to Sharding transaction format once #285 (PAYGAS) is merged.

[jannikluhn]

What's wrong with reusing transaction.to/target for this? The salt actually defines the address (at least in part), so target isn't too forced. I think this would be better than adding another field which is empty for most transactions.

[NIC619]

Since transaction.to is where the contract will be deployed(EVM will check if transaction.to == generate_CREATE2_contract_address(*salt*, transaction.code) ), if we reuse transaction.to as salt it will result in cyclic dependency, i.e, transaction.to is used as salt for calculating address but also has to be the calculated address.

If I understand correct, transaction.target is not part of the formal transaction format. Hence it seems to me that it's not a reliable source as a salt.

[jannikluhn]

Yes, that check would have to be removed. But it's not necessary anyway, right now transaction.to is redundant information.

[NIC619]

If I understand correctly, transaction.to is still needed especially if it's not a contract deploying transaction?

[jannikluhn]

Yeah, sorry, transaction.to is only redundant for contract deployments. For normal transactions it's still required of course. But for those we don't need a salt.

[hwwhww]

Reusing transaction.to looks economical. 😄 But kind of confusing for the normal users, like the reason why V want to rename transaction.to to transaction.target. On the other hand, a little optimization in Transaction would improve a lot...

One thing different is the length of salt for randomness, would that be an issue?

Vote for reusing if we can well-documented that. Also, ping @vbuterin to take a look.

By the way, we have to open an issue for renaming BaseShardingTransaction.to to BaseShardingTransaction.target right?

[NIC619]

@hwwhww You can't reuse transaction.to as salt because that would result in cyclic dependency. IMO a salt field is more straightforward to users and they don't have to spend time on crafting the data which is to be reused.

I can update transaction.to along with other transaction fields that are to be added/removed.

[pipermerriam]

EIP232 proposes an alternate format for transactions which might be the right solution for this. The following is a simplified version of the proposed change.

txn = [version, data_elements]
if txn.version == 1:
    nonce, gas_price, gas, to, value, data, v, r, s = data_elements
elif txn.version == 2:
    nonce, gas_price, gas, to, value, data, v, r, s, chain_id = data_elements
elif txn.version == 3:
    gas, to, target, whatever, other_fields = data_elements

I know it adds one-more-protocol-change to sharding, but it is also the solution I've seen for handling this issue which will only get worse over time.

[hwwhww]

@NIC619 sorry for that I missed one comment...! You're right!

[vbuterin]

I'm ok with adding salt into the transaction; either adding it as an extra data field or the EIP 232 approach.

[jannikluhn]

@pipermerriam I'm not sure I understand how EIP232 would be applied here. Like this:

normal_transaction = [0, [chain_id, shard_id, to, data, gas, access_list]]
cc_transaction = [1, [chain_id, shard_id, code, data, salt, gas, access_list]]

? What I'd like about this is that we don't ever have any unused fields. What I wouldn't like is that it's called "version", "format" may be better. I think with some changes to rlp.Serializable to automate the "expansion" this approach could work quite well.

[pipermerriam]

I'm good with renaming version to something else. I agree that automating the inner expansion also makes a lot of sense and should make this API quite transparent, allowing us to even access the inner properties from a singular outer generic transaction class.

Here is a rough sketch of what I'm thinking of for this.

class EIP232Transaction(rlp.Serializable):
    fields = (
        ('version', ...),
        ('inner_data', sedes.binary),
    (
    transaction_formats = {
        1: TransactionFormatA,
        2: TransactionFormatB,
    }

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.txn_data = rlp.decode(self.inner_data, sedes=self.transaction_formats[self.version])

    def __getattr__(self, name):
        return getattr(self.txn_data, name)

We have an outer transaction class which contains a mapping of version numbers to inner transaction classes. Then, during initialization, we decode the outer data packet, and then lookup the appropriate inner transaction class and use it to decode the inner data packet. Then we put some convenience APIs on the outer class to allow direct property access to the inner transaction properties.

[NIC619]

EIP232 looks like a cleaner and better approach in the long term but I suppose it's not going to be as trivial to implement as adding a salt field? What about adding salt field for now(short term) and starts an issue about implementing EIP232?

[pipermerriam]

@NIC619 sounds good to me.

[hwwhww]

close via #422