Tx: Usability Improvements

[holgerd77]

Ok, this should significantly improve tx library usability after reports from users like Web3.js (@GregTheGreek) and others requesting a tx instantiation more independently from Common. This PR now bumps the tx default HF to london and allows for the derivation of a compatible Common from the chain ID (for typed txs) or from the v value (for signed legacy txs respecting EIP-155).

A long reasoning has been done on the chat, will just copy over from there:

---

We very likely do not want to remove Common completely from the library. Common is our shared view on the network and hardfork state. In the tx library there is some fundamental need of an awareness of the hardfork. There are HF checks all over the place and the outcome of various methods are simply not defined (e.g. getSenderPublicKey() with the additional signature checks from Homestead or getMessageToSign() with EIP-155) if the tx doesn't know about the HF context.

That said: I have the impression that the current problems very much arise of a combination of new specifics of the new typed transactions and the non-breaking release we have done now.

So the thing is: we do allow for an instantiation of a tx without a Common passed in. If no Common is passed in there is then a default Common (with a default HF which we likely want to keep (it was in discussion to remove and always require an input) after this discussion //cc @jochem#8569-brouwer) instantiated which is just used internally and you are not needed to be aware of. With the typed txs this is just not working right now due to the following circumstances:

1. Due to backwards compatibility reasons - we didn't want to do breaking releases again - we remained on istanbul for the default HF. This HF is not yet compatible with typed txs and therefore a default Common is "throwing by default in this case", lol 😜
2. Typed txs are already coming with a chainId and this is not playing well with just setting the default chain in Common to mainnet (as we are doing now)

So my suggestion would be:
I thought a bit longer on 1.. The main reason to have this fixed specifically for tx library is due to unexpected behavior changes (like e.g. EIP-155) in the future. On a closer look one can state though: this was the case for legacy txs. For typed tx it can very well be assumed that tx functionality will remain stable and not change over time (this is one reason typed txs were introduced, otherwise there would just be a new tx type introduced). At the same time we switched over to explicitly copy the Common instance on tx instantiation to isolate - in the case of blocks and txs - Common from the rest of the system and have this stable within block and tx context.

With both these things given I would cautiously suggest that we upgrade the default HF (without doing a breaking release) - just for the default common on both new typed tx types (2930 and 1559) - to london (and simply: enable them). I would then - again, cautiously - think that this should not have substantial side effects (and - if someone wants to have more complex contexts, it is still possible to explicitly pass in a Common).

For 2. I would then suggest that we continue to throw if a typed tx is instantiated with a chainId deviating from the chain ID from a passed in Common instance (actually: are we doing this right now? 😛) and in turn instantiate the default Common for typed txs directly with the chainId passed in.

This would be generally more fitting and it would then be possible to just instantiate a e.g. 1559 tx with:

const tx = FeeMarketEIP1559Transaction.fromTxData({
    chainId: 5,
    //...
})

without any additional context. 😀

One thing to at least consider here: we would with this introduce (even more) slightly differing behavior between legacy txs and typed txs. Not sure if this is a thing/a big deal though.

[codecov]

Codecov Report

Merging #1292 (5c7add1) into master (dc77c19) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Flag	Coverage Δ
block	86.20% <ø> (-0.25%)	⬇️
blockchain	84.36% <ø> (ø)
client	84.48% <ø> (ø)
common	88.25% <ø> (-0.46%)	⬇️
devp2p	83.82% <ø> (+0.21%)	⬆️
ethash	82.83% <ø> (ø)
tx	89.49% <100.00%> (+1.14%)	⬆️
vm	81.35% <ø> (ø)

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

[jochem-brouwer]

I think in general I agree with this approach, I am just thinking if there are any dangerous side-effects by defaulting typed txs to london somewhere downstream.

[GregTheGreek]

This does solve our problem 👍

FWIW: We could just enable berlin for the time being

[holgerd77]

Ok, after some continued discussion in the chat I've adopted the default HF setup again to some more differentiated setting following the following rule: "The default HF is the default HF from Common if the tx type is active on that HF or the first greater HF where the tx is active".

This seems to be the solution with the most benefits and the least risk for side effects to me. People staying in their existing setup can with this be assured that the tx types which have been active before doesn't change on behavior (in this case: legacy txs). The typed txs are instantiated with the first rule set which exists for them. Since there just are no e.g. 1559 txs which might want to be examined which have been submitted on a network with a HF setup before london, there is no risk that these are tricked into some non-fitting rule set.

On the upstream side - e.g. in VM.runTx() it is ensured anyhow that the respective EIPs are in place, see e.g. this code part from VM.runTx():

// Is it an Access List transaction?
    if (!this._common.isActivatedEIP(2930)) {
      await state.revert()
      throw new Error('Cannot run transaction: EIP 2930 is not activated.')
    }
    if (opts.reportAccessList && !('generateAccessList' in state)) {
      await state.revert()
      throw new Error(
        'StateManager needs to implement generateAccessList() when running with reportAccessList option'
      )
    }
    if (opts.tx.transactionType === 2 && !this._common.isActivatedEIP(1559)) {
      await state.revert()
      throw new Error('Cannot run transaction: EIP 1559 is not activated.')
    }

I guess I am very much confident now that we can go with this, would open this again for review (if there is some sufficient agreement would be nice if we can still release this this week together with the other 1559 related changes. Guess after that there will be another 1559 related round of releases, maybe in 1-2 weeks or so, just as some side note).

[ryanio]

nice, I enjoyed following this :) looks really good, and 100% diff coverage! woot.

just two small things from review i will add in a following commit.

[holgerd77]

Ah, didn't know about this syntactic possibility. Nice. 😄