Util: Allow v to be 0 or 1 for EIP1559 transactions

[ernestognw]

Description

Currently, there are 4 functions using the calculateSigRecovery method, but it only allows for values to be 27, 28 or chainId * 2 + (35 | 36), introducing incompatibility for EIP1559 signed transactions, which is avoidable by just extracting the yParity from the v value (as stated in #1597).

The so-called functions are:

• ecrecover
• toRpcSig
• toCompactSig
• isValidSignature

However, the hack of extracting the yParity out of v works in most of the cases except for the toRpcSign function, which creates its result from the v param, making impossible to just toRpcSign(v - 27, r, s), since subtracting 27 actually changes the result.

Another related problem is that the fromRpcSig function might receive any signature but if v is < 27, it'll add 27 anyway. This is fixable once called since you can just subtract the 27 if you know what you're doing, but I think the docs will need at least to state that the function doesn't have enough information to know if adding 27 or not.

Proposed solution

• Update the calculateSigRecovery function to allow 0 and 1 values
• Update fromRpcSig docs to state that is adding 27 for < 27 values in any case

[holgerd77]

Hi there,
thanks for reporting, we - somewhat - know of the flaws of the current signature code in the respective Utils package. We are currently working on breaking releases on most of the monorepo packages - relatively imminent, to be ready in 4-6 weeks I would say - and one of the remaining not-yet-tackled parts in actually the reworking of the Util signature module, see #1717, and then: "Util" -> "Signing / Hashing Functionality Removal" respectively also the "In Discussion" parts.

Our current stronger tendency is to completely remove this package and directly rely on/use the ethereum-cryptography package - which bundles the underlying crypto - and then encapsulate the remaining and less relevant legacy part directly in the LegacyTransaction class of the Tx package.

You are very very invited to comment on that plan and give your thoughts.

Also of interest is your context of reporting: do you have an imminent problem where you need a solution? Or did you just - generally - want to improve on things?

Thanks a lot!

[holgerd77]

(have done some post-submit edits, better to read directly on the website)

[ernestognw]

Hi there, thanks for reporting, we - somewhat - know of the flaws of the current signature code in the respective Utils package. We are currently working on breaking releases on most of the monorepo packages - relatively imminent, to be ready in 4-6 weeks I would say - and one of the remaining not-yet-tackled parts in actually the reworking of the Util signature module, see #1717, and then: "Util" -> "Signing / Hashing Functionality Removal" respectively also the "In Discussion" parts.

Hi, thanks for the fast response.
I've already seen what's in #1717 before opening this, but I had the feeling that it's not clear the direction. You've already replaced the hash function in favor of ethereum-cryptography but #1845 seems to be focused on ecsign and ecrecover, but no clue about toRpcSig, which is the one I actually need (more detail below).

Our current stronger tendency is to completely remove this package and directly rely on/use the ethereum-cryptography package - which bundles the underlying crypto - and then encapsulate the remaining and less relevant legacy part directly in the LegacyTransaction class of the Tx package.

You are very very invited to comment on that plan and give your thoughts.

Overall I agree with moving towards ethereumjs. My only comment is to also add more context to #1845 about what's the plan for the rest of the functions. Although may not be that important, removing the module will break backwards compatibility, and having a clear guide on where to get the same functionality would be great. Particularly with the case of fromRpcSig and toRpcSig that seems to be in need of more discussion (#1717)

Also of interest is your context of reporting: do you have an imminent problem where you need a solution? Or did you just - generally - want to improve on things?

Thanks a lot!

Of course! We're making use of this in OpenZeppelin Defender, and some changes I'm currently introducing related to EIP1559 support are breaking because we can't serialize signed EIP1559 transactions because of v being 0 or 1, which throws an error when passing it through toRpcSig.

So, the hack of v + 27 to make it work is actually changing the serialized result.

Right now, I went for this approach:

...
 try {
    return toRpcSig(v, r, s, chainId);
  } catch (error) {
    if (error.message === 'Invalid signature v value' && (v == 0 || v == 1)) {
      return Buffer.concat([setLengthLeft(r, 32), setLengthLeft(s, 32), toBuffer(v)]);
    } else throw error;
  }
...

It's working, but I decided to open the PR to have the 1 and 0 included in the package.
Looking forward to hear your thoughts!

[holgerd77]

Thanks a lot for submitting and also for the extensive explanations here.

I do have a tendency to take this in respectively merge the PR, since this already sufficiently tackles substantial parts of the legacy signature code problems.

It also lead me to the following evolved opinion on how to proceed with this whole task, will also present this to the team as a suggestion: I do have the impression that we overload us with these big last minute refactoring tasks.

The signature code in Util is also more complex and sensitive than the hash code, where a replacement was easier. So my suggestion is that we take this here in - already for master - and as some future-solidifying for the develop releases as well. On top we do some of the (small) refactoring tasks from the v6 list of this code part.

We then keep the signature code instead of directly removing and take this one round slower. It is e.g. a possibility that we - during the v6 release lifecycle - internally move over to direct ethereum-cryptography usage and - along transition - also make deprecation decisions on the Util signature functions.

This seems more appropriate to me on such sensitive code parts, people have more time to adjust (everyone can still directly use ethereum-cryptography if wanted) and we free us from some hastily - somewhat big - last minute decisions so late in the release process where we actually already had (and partly still have) enough on this release round! 😋

Does this make sense? 🙂

[ernestognw]

Makes sense to me. We recently upgraded from ethereumjs-tx to @ethereumjs/tx and the most difficult part was to check the function/method equivalences so we don't break the system, especially since we have tests for it, but they're not exhaustive for every combination between chainId, EIP1559Txs, LegacyTxs, etc.

The Util's signature module is less of a headache in my opinion, as there are not too many functions in there, and the critical ones are already identified and you've planned ahead for them, but I do think that removing it completely will put somebody in a difficult position to upgrade to v6.

We then keep the signature code instead of directly removing and take this one round slower. It is e.g. a possibility that we - during the v6 release lifecycle - internally move over to direct ethereum-cryptography usage and - along transition - also make deprecation decisions on the Util signature functions.

Agree a 100% percent with this. Deprecating and slowly replacing with ethereum-cryptography is the softest transition from my perspective.