Util: Signature ecsign / ecrecover functionality

[holgerd77]

Some last minute thinking about the signature functionality for the breaking releases we now have in signature.ts in Util, especially regarding the role of the v and recovery value.

This was a bit triggered by stumbling upon #1597 which mentions the ecsign() function, which we haven't touched yet (regarding the aspect from above) during the up-till-now refactoring work.

So the thing is v is still returning the calculated values (so e.g. "+27" for legacy, "+ chainId + additional stuff" for typed tx), and people might or might not need rather the recovery value.

I wondered in a first round if it would make sense to just add plain recovery to the ECDSASignature interface and just return the plain recovery value in addition to what we return as v at the moment. Then people can choose what to use depending on their needs?

I have to admit though that I am still not 100% saddle-proof on the semantics of names, so on the very correct usage of v (that one in particular), recovery and yParity.

My current assumptions are:

• v: Legacy -> the "recovery + 27" stuff, Typed: the "recovery + chainId + stuff" stuff (is/can this still called v?)
• recovery == yParity (is this also 100% correct?)

Will circle in @jonathansmirnoff (from the issue above) here, everyone else welcome to join as well.

My current assumption is still that the suggestion from above (adding recovery to the interface) would help and would be very easy to be added (still breaking, so worth to do now).

[paulmillr]

recovery in secp256k1 is always 0 or 1, so I guess it can be == yParity. As a side note, @noble/secp256k1 signatures don't have recovery bit in them — instead, there's Point.fromSignature(msgHash: Hex, signature: Sig, recovery: number) method that creates ECDSA points (public keys) from signatures and their message hash.

[holgerd77]

I think we should still tackle, however too short-termed for Beta 1 so we should rather target Beta 2.

Open to be picked up.

Side note, had another closer look, for this to work (so: add recovery to ECDSASignature) particularly for fromRpcSig() the function calculateSigRecovery(v: bigint, chainId?: bigint): bigint helper function would need to be rewritten to work without the chainId and still deliver the same results. If I am not mistaken this should be possible by "solving" the v - (chainId * BigInt(2) + BigInt(35)) equation towards both 0 and 1 and see which one delivers pure integer results.

Correct me if I am wrong here anybody. 😋

[ScottyPoi]

We have a function ecrecover. I can refactor this function to calculate recovery if a chainId is not passed in, however the current tests are set up to expect this to fail when certain v values are used. If I let this method calculate recovery from v when no chainId is passed, than it no longer throws in those two test cases.

'should fail on an invalid signature (v = 29)'
and
'should fail on an invalid signature (v = 21)'

[holgerd77]

Have also removed the "urgent" label here and transformed this into a "normal" issue. 🙂 If this get's any feeling of "rush", in doubt rather leave.

[holgerd77]

(we can also add this in a non-breaking way later on by just making the recovery property optional in the interface (if added along a non-breaking release), then people can update to the specific version and use a ! in their code to indicate to TypeScript that the property is there. Not quite as clean and beautiful as doing this breaking but totally acceptable/ok as well)

[ScottyPoi]

#2051 Has been hanging out for a while. I've been picking at it, adding tests and removing extraneous code to improve the coverage results, but the tests all pass right now. Is this still something we want to merge, @holgerd77 ?

[holgerd77]

Is this still something we want to merge, @holgerd77

Yes, sure. Will have a look soon.