Update signing data according to EIP-155

[tryice3]

Fixes #123 by replacing v with chainId.

As an additional fix, it would be nice to remove the hardcoded hash(false) from the sign (privateKey) function and use the hash as input.

[holgerd77]

@danjm not completely getting the whole picture yet. Updated the branch, this is now breaking on the respective tests?

[danjm]

@holgerd77 Last week I felt this PR was more or less correct... now I am less certain. I will explain the issues as I understand them so that you or others can comment.

Background info and findings:

• Prior to this PR, we default v to 0x1c.
• v is used when calling serialize(), hash() and verifySignature()
• notes on serialize()
  • serialize() returns the rlp encoding of the the raw transaction.
  • serialize() is not used in index.js or fake.js
  • serialize() is not required to meet specifications of the yellow paper or EIPs
  • serialize() appears to be a convenience method for developers
  • serialize() is not tested on its own, and it functioning correctly is not required to pass the tests from ethereum/tests.
• hash() has its own code for modifying v according to EIP155, which is conditionally applied if chainId > 0
  • verifySignature() calls hash() and will use the v value set in the hash() call
• hash() and verifySignature() will use the default v value if chainId > 0 === false
• v is overwritten when the transaction is signed (i.e. when sign() is called)

All of the above appears to be correct. (With the exception of chainId > 0 being the condition to apply the rules of EIP155, something I fix in #132).

The problem experienced in #123 is a problem with serialize() only. In particular, in the case where the EIP155 conditions are true, serialize() will not return the same rlp enconded tx as is used in hash().

Now, this does not affect the correct functioning of hashing, signing, signature verification, etc. As said above, it appears to be a convenience method for developers.

Of course, we still would like it to function correctly. The question is, what should it do exactly? I think the answer to that question is: it should return the rlp encoding of the raw tx that is ultimately used by hash().

This PR attempts to make this happen by applying the rule hash() uses to when setting v to the place where we set the default value for v. However, it is unclear that the default value applied in this PR is correct or necessary. Furthermore, EIP155 also specifies values for r and s, which this PR does not address (of course, those values don't affect the stringified version of serialize(). Finally, this rule should really only be applied if we are on the spurious dragon hard fork or later.

I might suggest an alternative to this PR, to be implemented after #132 is merged, that does the following: (1) move the code for applying the EIP155 changes to v, r and s out of hash() and to its own method (2) use this method when setting the this.raw, which in turn requires (3) some sort of refactor of the use of ethutil.defineProperties() to take into account the EIP155 changes, and also to make it clearer to the reader of ethereumjs-tx code exactly what that does

[danjm]

Closing this in favour of https://github.com/ethereumjs/ethereumjs-tx/pull/143

The description of that PR thoroughly explains why.