Correctly support signing and serializing of EIP155 valid transaction

[danjm]

This PR is branched off of the branch for #131

This fixes #123 and is an improvement over #124. It also fixes an additional issue introduced in #131 that was necessary to get the transaction tests passing.

The first commit in this PR adds two failing tests, one of which is strictly based off the example in https://github.com/ethereum/EIPs/blob/master/EIPS/eip-155.md. This is the example used in the explanation of #123. The second commit fixes these tests.

Further explanation

Consider the test added in this PR:

t.test('Verify EIP155 Signature before and after signing with private key', function (st) {
    // Inputs and expected results for this test are taken directly from the example in https://github.com/ethereum/EIPs/blob/master/EIPS/eip-155.md
    var txRaw = [
        '0x09',
        '0x4a817c800',
        '0x5208',
        '0x3535353535353535353535353535353535353535',
        '0x0de0b6b3a7640000',
        '0x'
    ]
    var privateKey = Buffer.from('4646464646464646464646464646464646464646464646464646464646464646', 'hex')
    var pt = new Transaction(txRaw, { chain: 1 })
    st.equal(pt.serialize().toString('hex'), 'ec098504a817c800825208943535353535353535353535353535353535353535880de0b6b3a764000080018080')
    pt.sign(privateKey)
    st.equal(pt.hash(false).toString('hex'), 'daf5a779ae972f972197303d7b574746c7ef83eadac0f2791ad23db92e4c8e53')
    st.equal(pt.serialize().toString('hex'), 'f86c098504a817c800825208943535353535353535353535353535353535353535880de0b6b3a76400008025a028ef61340bd939bc2195fe537567866003e1a15d3c71ff63e1590620aa636276a067cbe9d8997f761aecb703304b3800ccf555c9f3dc64214b297fb1966a3b6d83')
    st.end()
})

Note that there are three assertions. The first, which checks the serialized tx before signing, currently fails on master. #124 would fix that assertion, but break signing which relies on our current behaviour of defaulting to v to 0x1c even when chainIds are passed. This behaviour is tested by the 'sign tx with chainId specified in params' test in test/api.js, and is consistent with part of EIP-155 that says "The currently existing signature scheme using v = 27 and v = 28 remains valid and continues to operate under the same rules as it does now."

This PR provides a backwards compatible fix of the first assertion by taking advantage of the new common and chain options. Developers who need to correctly serialize transactions with a chainId before they are signed will be able to do so if the chainId is indicated through those options. The v value used for serialization will be set by those if provided.

After #131, the 3rd assertion in the above test fails. This is because that PR changed the conditions which determine whether the EIP155 alterations to signature values (v, r, s) would be applied to the hash used when signing. It replaced the simple check as to whether chainId was being used (chaindId > 0) with the check defined in the EIP (v is chainId*2 + 35 or 36, and the current block > 2,675,000). This was necessary to get all transaction tests to pass.

However, this had the unintended side effect of using the incorrect hash when signing EIP155 transactions with chainId's but no explicit v, r and s values. This side effect is not caught by the transaction tests because those are all already signed. Basically, transactions seeking replay protection need to be signed with the v, r and s values specified under EIP155 rules, even when not provided a v at time of creation. The EIP155 example implies that a transaction should be signed with a hash created with a v = chainId, if chainId is explicitly provided, even when v is not explicitly provided. This PR fixes the 3rd assertion in the above test by ensuring this happens.

The other test added in this PR checks that signing is done correctly when using a chain other than mainnet.

[danjm]

One question that came to when when writing the above PR description: what should happen when v and chainId are provided to the transaction constructor, but they are incompatible? I am inclined to think that an error should be thrown. But would appreciate other opinions.

[coveralls]

Coverage increased (+0.03%) to 98.969% when pulling eb7ae3c on fix-signing-eip155-transactions into 4d75731 on update-official-transaction-tests.

[holgerd77]

Hi Dan, just only came to read this, thanks for this fantastic write-up, this is really an outstanding and super-transparent summary! 😀🤗

I would also suggest/tend to throw with incompatible v and chainId.

[holgerd77]

Once you've merged #131 can you the here so that one can better see the change diff?

[holgerd77]

What's the status of this (typo in the above comment: "the"-> "rebase"), also can #131 be merged? I would really like to get this over the finish line and do an in-between release, one doesn't even dare to look any more when we started this work.

[alcuadrado]

This PR seems correct. I don't get why it changes getSenderPublicKey(). That change also seems right, as having the public key cached could lead to errors if v, r or s were changed after calling verifySignature().

Regarding incompatible v and chainId values: now the transactions fail to validate in that case, which is good, but I also think throwing an error would be better.

[holgerd77]

Hi @danjm I've now merged #131. Can you rebase and address the comment from @alcuadrado so that we can also merge here, there is more and more work building in top of your changes, and otherwise things get a bit too messy. 🙂

[danjm]

I've rebased this branch.

@alcuadrado To be clear, you don't think further changes are needed to this PR, correct?

[alcuadrado]

That's correct @danjm, thanks for rebasing this.