utils.serializeTransaction ignores “chain ID” part of “v”

[3sGgpQ8H]

When utils.serializeTransaction(tx, sig) is used to serialize signed transaction, the function could distort sig.v value passed: strip “chain ID” part from passed sig.v value and replace it with value derived from passed “tx.chainIdparameter. This is counter-intuitive and error prone. Moreover, this logic is applied even iftxdoes not havechainId` field at all (the function serialized pre EIP-155 transaction in such case).

In my opinion, once sig.v value is explicitly provided by the user, this value should go into serialized transaction as is. If this value does not match with tx.chainId value, explicitly provided, then exception should be thrown. There should be some way to clearly distinguish situations when chain ID is not provided by the user from other situations, when user explicitly provided that he wants to serialized pre EIP-155 transaction with no chain ID.

With current implementation, if one has all nine parts of a signed transaction and wants to serialize this transaction, he has to manually calculate chain ID from v and pass ten parameters to utils.serializeTransaction(tx, sig), otherwise his v value will most probably be distorted (replaced with 27 or 28, effectively stripping chain ID information).

[ricmoo]

I think this makes sense for the most part, I’ll think about it more and look into it tomorrow for v5. It is true that of chainId and v disagree, there should be an error.

For the signature, there should be no need to compute the v manually though, since you can just ensure recoveryParam is correct and it will determine the v for you. Or it should at least. I’ll build out a larger suite of tests tomorrow to follow the various allowed combinations. I added a bunch of tests recently to v5 to make sure the splitSignature could recreate a full signature from valid fragments (e.g. defining vs should be able to reconstruct v, s and recoveryParam). But it looks like the chainId is in fact ignored when serializing.

[ricmoo]

The logic I'm thinking right now:

• If there is no chainId in the transaction, but the signature has a v which is over 28, then it will be serialized as EIP-155 with the computed chainId.
• If there is any mismatch between the transaction.chainId and signature, the world ends and an exception is thrown.

Does that seem ifair?

[3sGgpQ8H]

• If there is no chainId in the transaction, but the signature has a v which is over 28, then it will be serialized as EIP-155 with the computed chainId.

Absence of chainId in transaction may mean either that user does not want to pass chain ID separately as it is already encoded into v, or that user explicitly wants to encode pre EIP-155 transaction. In the former case proposed behavior is OK, while in the latter case an exception should be thrown. I suggest to treat undefined chainId as “unspecified”, and false/null/"" chainId as explicit pre EIP-155 intention.

• If there is any mismatch between the transaction.chainId and signature, the world ends and an exception is thrown.

This sounds fine.

[ricmoo]

Hmmm... I don't really like undefined and null having different meanings. I do it sometimes, but very rarely, and usually in contained/internal ways...

I think it is ok that if chainId is unspecified and the siguature uses a canonical v, that no EIP-155 is expected, but if chainId or non-canonical v is specified to use EIP-155, no?

If a chainId of 0 is provided, then non-EIP-155 is forced and a v mismatch will throw.

Basically, I never want to "guess" what is intended, but I do want to make sure a person can be as expressive as desired and to have sane defaults.

Did you check out the changes? :)

[3sGgpQ8H]

When I wrote false/null/"" I didn't mean that all these value should mean the same. I meant that there should be special value for chainId to explicitly specify that the user wants to encode pre EIP-155 transaction, and this value should differ from "unspecified" chain ID. Is zero is not a valid chain ID in terms of EIP-155, then zero is probably the best choice for such value.

[ricmoo]

I think the new incarnation of serializeTransaction addresses the concerns, so I'm going to close this, but if you disagree, please feel free to re-open.

Thanks! :)