signature/address: support for high recovery and chain IDs

[jochem-brouwer]

In ethereumjs/ethereumjs-monorepo#1129 I noticed that ecrecover does not have support for very high recovery IDs (which happen on very high chain IDs). This PR allows to pass BNs or Buffers to ecrecover, to allow this support.

I will add a test to show that this works for high chain IDs by taking a YoloV3 transaction.

[codecov]

Codecov Report

Merging #290 (be76516) into master (fe518cf) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #290      +/-   ##
==========================================
+ Coverage   97.10%   97.13%   +0.02%     
==========================================
  Files          11       11              
  Lines         415      454      +39     
  Branches       71       79       +8     
==========================================
+ Hits          403      441      +38     
  Misses          3        3              
- Partials        9       10       +1     

Impacted Files	Coverage Δ
src/account.ts	94.64% <100.00%> (+0.14%)	⬆️
src/bytes.ts	97.46% <100.00%> (ø)
src/signature.ts	93.75% <100.00%> (-2.41%)	⬇️
src/types.ts	100.00% <100.00%> (+20.00%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe518cf...46415c1. Read the comment docs.

[alcuadrado]

Really curious about this PR. Is this because of EIP155? Some chainIDs don't fit in a number, so computing the V requires a big number?

[jochem-brouwer]

@alcuadrado Yep, this was a problem on the YoloV3 network, which has a chain id of 34180983699157880. If we don't use the logic as in this PR, then at some point when syncing, we recover the wrong sender address (probably because some rounding errors in the arithmetic).

I will add this transaction as test.

[alcuadrado]

You know what's extra curious about it? In most platforms that's not a BN! It fits in a uint64, but js 🥲

[jochem-brouwer]

Okay, isolated the failing transaction on Yolo and added it as test. Ready for review.

[holgerd77]

Yes, also thought after writing that we likely should leave BigInt out for now and extract to a separate issue.

Not sure, I know we want to get this released soon since the client work depends on this, on the other hand we might want to also adopt the other methods as well which are touched by this since this would better justify a release and I think this is something others would directly profit from.

Have written together a short list to get an overview on the task:

• Account: toChecksumAddress = function(hexAddress: string, eip1191ChainId?: number)
  • -> relatively easy, only string conversion, would benefit, just tested a larget string output: '2.3409328409382408e+42' 😜
• Account: `isValidChecksumAddress = function( hexAddress: string, eip1191ChainId?: number)
  • -> simple, just calls into toChecksumAddress
• Signature: ecsign = function(msgHash: Buffer, privateKey: Buffer, chainId?: number)
  • -> Can we do this with function overloading to not make this breaking (return with BN (or alternatively: always Buffer? when chainId is BN) (a bit tricky)
• Signature: toRpcSig = function(v: number, r: Buffer, s: Buffer, chainId?: number): string (v and chainId)
  • -> needs some function overloading in the subcalls
• isValidSignature = function(v: number, r: Buffer, s: Buffer, homesteadOrLater: boolean = true, chainId?: number) (v and chainId)
  • -> needs some function overloading in the subcalls (redundant)

I think this should be doable, WDYT? 🤔 If you want you can directly integrate here but I could also do tomorrow if you don't find the extra time.

For the function overloading cases I guess the most clean way of doing this is likely to always return the same type as the input type? This would then be slightly more work, but I guess it's better than always returning a BN when people are e.g. working in a Buffer context.

Monorepo integration: @ryanio has also already suggested this and therefore already did #289 in preparation. 😋 I don't have the capacity right now to do the integration and I also feel that the monorepo is not enough on stable ground right now to do such a heavy integration on top (CI/Coverage often failing and things). But this is definitely planned! 😄

[holgerd77]

(or @ryanio you can also directly pick this up if you want to! 😄 )

[jochem-brouwer]

Yeah this should not be so hard. So what is the plan here? For anything where it would make sense to allow Buffer/BN/string/number input we allow these? How do we internally want to handle this? I assume we will convert them all to a BN or a Buffer, depending on the situation?

In future PRs we can start to use BigInt or Uint8Arrays (as opposed to Buffer, for optimization)

[ryanio]

Handling BNLike would be nice from a dev ux perspective, like we do in the fromXData factories in our monorepo packages. Maybe we can write some kind of utility/formatter to consume BNLike and output a specific type (BN or Buffer/Uint8Array) so it can be reused in other places.

[holgerd77]

Yeah this should not be so hard. So what is the plan here? For anything where it would make sense to allow Buffer/BN/string/number input we allow these? How do we internally want to handle this? I assume we will convert them all to a BN or a Buffer, depending on the situation?

I think it makes sense to on this first round stay somewhat narrow here and not do all methods somehow accepting numbers, so e.g. for many of the buffer-related methods (setLengthLeft(),...) I would doubt that we need BNs there, or at least that would likely need some discussion. But on a first look I would say that these v respectively chainId related methods are safe candidates to do. So I would suggest we stick to the (I think complete in this regard) list from above.

Yes, I think in most cases, an internal conversion to BN should work well - e.g. for toChecksumAddress and similar methods. Not completely sure about the overload methods - e.g. ecsign - might be most practical to stay in the respective type if we choose a BN in-and-out, Buffer-in-and-out,... methodology, but this will likely becoming more clear when working on this.

[holgerd77]

Phew, this is a lot harder than I expected. Already had various iterations on this, I started by expanding the ecsign method. Will nevertheless continue to work here since I do think that we need this anyhow if we e.g. want to expand on PoA block signing and the like. And this is generally useful to have as a feature.

1. I first added the BNLike type both as an input type as well as a return type. This is not so beautiful since it reduces on type safety for the return type and also directly breaks typescript code which assumed before to get a number back
2. Then I switched to function overloads - I pushed the state of this work within 5c80ce8 so you can get an impression. This does work, tests also pass, but when having a somewhat distant look again I am thinking that this code is getting too complex and error prone for what it does
3. Then thought: maybe just always return a BN: but this is not so beautiful from a future BigInt support perspective
4. So I finally came to the conclusion (long way), also when looking at the ecrecover method: the right thing to do here

• Add just one additional interface ECDSASignatureBuffer
• Use this as a return type for all inputs except the existing ones (undefined, number) (and therefore generally return a Buffer - as in ecrecover - which can be easily converted to BN or `BigInt
• Mark the "return as number" as well as ECDSASignatureBuffer (this will be renamed to ECDSASignatureBuffer) as deprecated and switch to always returning a Buffer on the next breaking release

So I will introduce 4. on the next push.

Hmpfg.

[holgerd77]

Ok, this 24279bf is finally how I would keep it for this specific function, will continue with the other methods today or tomorrow.

[holgerd77]

Have added a planned-breaking-change note to #291

[holgerd77]

Ok, this is now ready for review. 😄

Have left out fromRpcSig() - from the doc comments I have the impression that this method is outdated and we can deprecate? Not sure about toRpcSig(), have adopted for now.

Otherwise I think the main v and chainId related functionality should be covered.

Side note: first planned to also include a release commit to get this out quicker (PR is needed for continued client work), but since this has now gotten so large I've omitted to not overload.

[holgerd77]

(or does someone feel uneasy here with opening to so many type inputs? we can alternatively also just limit to (the existing) number + Buffer)

[holgerd77]

(wonder since we have lately rather reduced type variety)

[holgerd77]

(I have some (but no strong) tendency to think that in this case it is ok)

[jochem-brouwer]

I'm assuming that originally we expected these strings to be 0x prefixed?

[ryanio]

yeah actually PrefixedHexString is simply a string type right now, maybe in the future typescript will give us some way to enforce that the first two characters are 0x.

[ryanio]

looks great, however instead of converting the types inside each method (seems repetitive) what if we introduced a helper method e.g. toType(from: any, to: TypeOutput.Buffer) where TypeOutput is an enum of [Number, Buffer, BN] (in the future can add BigInt)

Then we can also refactor out vAndChainIdTypeChecks and include the 0x-prefix and MAX_SAFE_INTEGER checks inside toType if it is given a number or string, so we automatically have that safety everywhere.

[jochem-brouwer]

Few comments here.

Fun fact, I cannot approve my own PR 😛

[holgerd77]

Hi @ryanio can you eventually finish this PR? I won't be able to finish today or tomorrow and would be nice if we get this done, I'm fine with all changes. 😀

[ryanio]

Hi @ryanio can you eventually finish this PR? I won't be able to finish today or tomorrow and would be nice if we get this done, I'm fine with all changes. 😀

yes for sure I can do in a few hours when I'm back on my computer 👍

[holgerd77]

(so finish === include eventual improvement suggestions, otherwise this should be mostly ready)

[ryanio]

ok I added a commit (243fd47) that adds a toType helper that includes the 0x-prefix and MAX_SAFE_INTEGER checks so I was able to reduce some of that duplicate code, let me know what you guys think.

I would appreciate some TypeScript guru help, I was able to get the toType return type based on the outputType enum with some fancy code here, however I wasn't able to get the return values inside the method to work seamlessly (I had to add as any to all the returns). Otherwise when using the toType method in the actual code as a helper it seems to be reporting the correct return type so that's exciting!

I might also want to add some independent tests for toType if we will be exporting it for use by consumers of the package, but I can do that in a follow-up PR if we want to merge this in. (edit: added in 88db47f)

(BTW @holgerd77, do you think it was okay to refactor out the "Performance optimization" here? I can add it back in, but opted for the new simplicity and built in checks introduced by the toType helper.)

[ryanio]

this lgtm now! I would appreciate another set of 👀 on my latest commits but otherwise 👍

[holgerd77]

Hi @ryanio,
thanks for these additions, this toType() method is really genius and a great helper. I have to say that I feel - a bit - uneasy nevertheless to have that much code with such specific functionality and walking on relatively unproven ground somewhat last-minute to a PR.

Maybe on a next round we want to do things like this in a separate PR and in doubt keep out of an outstanding release.

Will give this a go here nevertheless on this turn, I DO think this should be ok after having checked the code twice. Also checked that the types are working when toType is applied (was also somewhat irritated by these as any casts at first), so that's great! 😄

Ah, and refactoring out the performance optimization seems to be ok for me, thought this might be faster, but had no real insight here and wast just a precautious measure, eventually overblown.

Ok, will directly prepare a release here. 😄

[holgerd77]

This was intentional to split the BNLike type to these two overlays and not use the BNLike type here. Otherwise there is an ambiguity on the number type.

Update: I just checked in the code, I would have expected for TypeScript to assume ECDSASignature | ECDSASignatureBuffer as a return type when used with a number chainId, this is not the case though, seems TypeScript takes the first working signature.

Will therefore let this pass, feeling a little uneasy on this nevertheless.