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

Simplify transaction's constructor and EIP155 handling #153

Closed alcuadrado closed 5 years ago

alcuadrado commented 5 years ago

This PR implements the changes discussed in the TS migration PR and in fixes #149.

A summary of the changes:

  1. chainId can't be provided in the data object.
  2. Only opts.common, or opts.chain and opts.hardfork can be used.
  3. The default hardfork is now 'petersburg'.
  4. The chain id is always obtained from the internal common object now. It isn't computed in the constructor.
  5. The constructor throws if the v value is present, indicates that EIP155 was enabled, and the chain id it indicates doesn't match the one of the internal common object.
  6. No default v is set. If a transaction isn't signed, it would be an empty buffer.
  7. Transactions are always signed with EIP155 after Spurios Dragon.
  8. If v is changed after construction, and it doesn't match the one of the internal common object, we throw a clear message in tx.verifySignature() instead of just letting it return false. its value is validated in its setter.

I think (7) and (8) require some extra comments:

(7) This is different from the previous implementation, where by default EIP155 wasn't used. We'd need an extra param to implement that. That would also make the logic to decide when to use EIP155 would be more complex, and it's already fairly complex. IMO it's better to use EIP155 by default, giving the users replay-protection by default. This EIP has been active for a long time now.

(8) Maybe this should be done in the v setter. What do you think? It would require a call to Object.defineProperty in the constructor, as anything in the prototype is shadowed by defineProperties. Update: See next comment

alcuadrado commented 5 years ago

(8) Maybe this should be done in the v setter. What do you think? It would require a call to Object.defineProperty in the constructor, as anything in the prototype is shadowed by defineProperties.

I just updated this PR with an implementation of this logic. I think it's better to validate v in its setter than throwing a possible unexpected error in a boolean function.

To implement this I had to change the hash method because it was temporarily modifying the transaction with invalid values. Now it's a pure function.

In doing the modification to hash, I noticed that all the fields have allowLess: true except v. I'm not sure where to check if this is correct or just an error, but I'm pretty sure it's wrong. Also, as we have already discussed in #151, this is RLP-related logic, which IMO shouldn't be part of the internal representation of these objects.

alcuadrado commented 5 years ago

Update: I fixed a bug when signing already signed txs. If it was previously signed without EIP155, tx.hash(false) would not generate an EIP155 hash, but sign would assume it did.

holgerd77 commented 5 years ago

Thanks for the super solid work and the great summary, I would generally go along with all changes proposed/implemented, just as some first estimation. :smile:

alcuadrado commented 5 years ago

Update: I fixed a bug when signing already signed txs. If it was previously signed without EIP155, tx.hash(false) would not generate an EIP155 hash, but sign would assume it did.

Woke up thinking that this change isn't right. Signing signed and unsigned transactions should have the same logic to decide whether to use EIP155 or not, but _implementsEIP155 works differently for signed and unsigned transactions.

I see two ways of fixing this:

  1. Split the hash method in three: one to get the tx id, one to get the message to sign, and finally one to get the message to verify the signature.
  2. Remove any previous signature before signing.

I'll make a commit with the second alternative, as the first one needs further discussion, and can be addressed as part of #151.

UPDATE: Done. UPDATE2: I implemented (1) in #154 to get an idea of how it would look.

alcuadrado commented 5 years ago

I'll make a commit with the second alternative, as the first one needs further discussion, and can be addressed as part of #151.

While testing this I realized that toJSON wasn't included in the TS migration, so I added it back.

I didn't reimplement its behavior, as defineProperties will overwrite it anyway. I'm not happy with that though, but duplicating its logic didn't seem right either. I think I just don't like defineProperties 😅

holgerd77 commented 5 years ago

Signing signed and unsigned transactions should have the same logic to decide whether to use EIP155 or not

Would agree here, taking 2. should be a valid fix for now.

toJSON wasn't included in the TS migration, so I added it back

Ok.

holgerd77 commented 5 years ago

@s1na @alcuadrado After this has been merged (@alcuadrado feel free to merge) I think we are in a good working state on the library. I would very much tend to do now do a major release on this, I think the changes already done are heavy enough and further refactoring/simplification work started and discussed by you guys is still a bit far off respectively shouldn't be overrushed and we should take our time here and do a subsequent likely major release on this once ready.

Are you going along with this?

alcuadrado commented 5 years ago

@s1na @alcuadrado After this has been merged (@alcuadrado feel free to merge) I think we are in a good working state on the library. I would very much tend to do now do a major release on this, I think the changes already done are heavy enough and further refactoring/simplification work started and discussed by you guys is still a bit far off respectively shouldn't be overrushed and we should take our time here and do a subsequent likely major release on this once ready.

Are you going along with this?

Completely agree. I created #154 as a place to explore different ideas, not to be merged and released now.