Add "recovery" to ECDSA Signature Interface

[ScottyPoi]

Fixes Issue #1961

Adds recovery: bigint as a value in ECDSA Signature interface.

recovery is calculated from v and chainId, or just v if no chainId is provided, and EIP-155 Replay Protection is enabled.

Updates signature.spec.ts test in util and eip-3074-authcall.spec.ts in vm to use new interface/API

Resubmit of PR #1995 and PR #2012

[codecov[bot]]

Codecov Report

Merging #2051 (32a4de4) into master (06dc4d9) will decrease coverage by 0.01%. The diff coverage is 95.45%.

Flag	Coverage Δ
block	83.81% <ø> (ø)
blockchain	80.54% <ø> (ø)
client	77.07% <100.00%> (+<0.01%)	:arrow_up:
common	95.31% <ø> (ø)
devp2p	82.44% <ø> (+0.12%)	:arrow_up:
ethash	90.81% <ø> (ø)
evm	40.72% <ø> (ø)
statemanager	84.55% <ø> (ø)
trie	81.13% <ø> (ø)
tx	92.18% <100.00%> (-0.02%)	:arrow_down:
util	86.17% <93.75%> (-1.05%)	:arrow_down:
vm	78.05% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

[ScottyPoi]

@holgerd77

Finally got everything to pass, without touching anything outside of util

Could use a sanity check but I think this is done.

[holgerd77]

Rebased this via UI.

[ScottyPoi]

@holgerd77

I did another round on this and have everything passing.

• I needed to tweak something in LegacyTransaction to allow the tx tests to pass.
• Changing this in tx caused some client tests to fail
• To fix these, I made changes to miner.ts and pendingBlock.ts in the client.
  • Both of these changes had to do with setHardforkByBlockNumber

I think this is likely the wrong approach, but I haven't found another way to get around this.

[ScottyPoi]

To achieve the same result without changing any defaults, I created a config option called disableMinerHardforkByBlockNumber, and applied it to the failing tests in miner.spec.ts and pendingBlock.spec.ts

The way the tests were set up, with custom chains and custom hardforks, the miner would fail to pass the custom hardfork information down to the miner, so the miner would change it back to chainstart every time.

[holgerd77]

Thanks for the efforts, really appreciate the contiuous work! 🙏 Nevertheless I am still not getting why this needs to affect the upper level libraries in any way. We are adding just an additional field to Util - recovery - with v just staying exactly the same as it was before (that's at least how it should be). Why (the hell) does this lead to changes in something like LegacyTransaction and even a new client option??? 🤔

This should just be some additional convenience for the Util method users who can then choose to either use the v value or recovery depending on their needs.

I would put this on hold for now, I do not have the energy/ressources to properly review this in time before our final releases so this will likely not make it in, so be it. Eventually it is possible to later do in a non-breaking way.

[ScottyPoi]

I would put this on hold for now, I do not have the energy/ressources to properly review this in time before our final releases so this will likely not make it in, so be it. Eventually it is possible to later do in a non-breaking way.

Ok. I got in over my head with this one, to be honest. I will move on.

[holgerd77]

Thanks, no worries, this happens, we just have to be honest about it things are ready or not and not wave things through which just need more thought it polishing. 🙂