Tx: fix assignment of v,r,s in fromValuesArray #1077
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
| v: !v?.equals(emptyBuffer) ? new BN(v) : undefined, | ||
| r: !r?.equals(emptyBuffer) ? new BN(r) : undefined, | ||
| s: !s?.equals(emptyBuffer) ? new BN(s) : undefined, | ||
| v: v && !v.equals(emptyBuffer) ? new BN(v) : undefined, |
There was a problem hiding this comment.
This won't work if v is 0. This is problematic in EIP-2930.
There was a problem hiding this comment.
Although I am not sure if there exists legacy transactions with a v value of 0... They do exist on EIP-2930 transactions, so it can also be "fixed" there, but I am generally more in favor to be correct here; if one inputs any value (also for r and s) which is 0, then it should create a BN(0).
There was a problem hiding this comment.
ah hm okay, then maybe I shouldn't remove the validation for v to be zero in this commit? 14581b0
I think I'd prefer that tx v3 continues to support legacy transactions only, and then v4 can support EIP-2930. These accidental BN(0) bugs are not fun and I don't think v,r,s should ever be initialized with BN(0), only undefined for unsigned transactions, otherwise we need to add more checks around the library (like in isSigned). What do you think?
There was a problem hiding this comment.
Although I am not sure if there exists legacy transactions with a v value of 0... They do exist on EIP-2930
Are you maybe mixing this up with this v - 27 thing (never understood that completely), I can't imagine that EIP-2930 is changing on the signature format?
Maybe @alcuadrado can have a short look here as well
|
Ah, just seeing that @jochem-brouwer is also reviewing right now with some remarks, will then leave this to him to give an approval since he has generally some better insight here |
a1d9f2c to
1c6aee1
Compare
|
@holgerd77 yes, another bug fix release for this PR would be great, sorry for the troubles and thanks :) the first PR fixed one half of it (static constructor |
This PR is a follow-up fix to #1042, thanks to @jochem-brouwer for noticing this, where the logic in checking for values of
v,r,sinfromValuesArraywas flawed.This bug would have been caught by
_validateTxVa long time ago (prior to #1041) if the function did not return ifvwas zero. The proper behavior in this package is for those values to be undefined if unavailable, zero is not a valid value as it's not being checked anywhere else for zero, only undefined.