search

ethereumjs / ethereumjs-tx

Project is in active development and has been moved to the EthereumJS VM monorepo.
https://github.com/ethereumjs/ethereumjs-vm/tree/master/packages/tx
Mozilla Public License 2.0
779 stars 235 forks source link

Should throw error if chainId not specified #137

Closed aaroncolaco closed 5 years ago

aaroncolaco commented 5 years ago

Based on EIP155 chainId is used for replay protection. web3.js includes it by connecting to the chain if not specified. ethereumjs-tx does not do that returns a tx even when chainId is unspecified.

Based on the code at index.js#L259 it appears as though the chainId is never included in the signature.

There are no tests I could find which excluded chainId.

The expectation was that an error would be thrown the same way it is if gas parameters are left out.

Submitting this issue after detecting this problem with @danoctavian

danjm commented 5 years ago

Thanks for giving consideration to this issue @aaroncolaco and @danoctavian

The EIP does not say that chainId is necessary. Instead it 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."

The chainId is not meant to be included in the signature, but to determine the v, r and s values that should be included in the signature. There are two open PRs which will ensure that this is done consistently and correctly: #131 and #143. These PRs also ensure that chainId is set if the chain is specified by the developer.

Also, the tests in test/api.js do not explicitly set chainId. In the constructor, if a v value is supplied that is high enough to calculate a positive chainId according to the EIP155 rules, then a chainId is given a non-zero value by default. Some developers may rely on this behaviour.

I don't think we should throw an error as it would be a breaking change, and enforce something that is not specified as necessary in the EIP or yellow paper.

Does this make sense?

alcuadrado commented 5 years ago

Hey @aaroncolaco and @danoctavian,

We changed the behavior of this library with respect to EIP155. It now enables the replay-protection mechanism by default, but users can opt-out from that. You can take a look at #149 and #152 to read more about the changes.

I'll close this issue as it's now outdated. Feel free to open a new one.