Tx initialization on the block library crashes on initialization with only chain set on Common

[holgerd77]

When experimenting with the ethereumjs-client code I stumbled upon an error that block initialization crashes when a block is initialized with a Common instance with only the chain and no hardfork set.

I triggered a test run here which shows the "Error: Method called with neither a hardfork set nor provided by param" error being triggered in this case.

This is due to an undefined HF state in the created txs which leads to undefined behavior in places like here (the this._common.gteHardfork('spuriousDragon') comparison throws (correctly) because it needs a base HF to compare against).

The problem is already described with two TODO entries in the block constructor (I think from @alcuadrado):

// TODO: Compute the hardfork based on this block's number. It can be implemented right now
// because the block number is not immutable, so the Common can get out of sync.
const hardfork = opts.hardfork ? opts.hardfork : null
// addition by @holgerd77: "can" is likely meant as "can't" here 

// ...

// parse transactions
for (let i = 0; i < rawTransactions.length; i++) {
  // TODO: Pass the common object instead of the options. It can't be implemented right now
  // because the hardfork may be `null`. Read the above TODO for more info.
  const tx = new Transaction(rawTransactions[i], opts)
  this.transactions.push(tx)
}

I think with this current implementation behavior of a runtime crash we should rethink the weight of the immutability argument here. I would therefore suggest to internally in the block class (so: not change the common instance) use a HF computed by the block number (this might need an additional method in Common) if no HF is passed on block initialization and also pass this on to the created txs.

I think while technically possible the context of changing the block number after object creation is unlikely and generally non-recommended style, since on a block all data is highly interdependent and changing single data pieces is very likely bringing things out of sync.

Side note: there is another bug to fix here which is the Common object created by default (when no instance is provided) not being passed on to the txs currently (since opts is passed there). This also leads to a potentially inconsistent state.

[holgerd77]

Update: this rather in addition might need some discussion if to only set the HF internally or directly on the Common instance, first comment only points out first possibility

[alcuadrado]

// addition by @holgerd77: "can" is likely meant as "can't" here

I remember having this same issue, so those TODOs are probably mine. And yes, it should be "can't".

[holgerd77]

@ryanio ah, thanks for reopening. Good that you noticed!

[ryanio]

@holgerd77 np! i am working on this now

[ryanio]

i'm a little stuck because the block number is not available when initializing common for Block.

first we have to initialize BlockHeader to get block number, but that also needs a common passed into it (here).

after BlockHeader is initialized we could possibly use _getHardfork on BlockHeader but it is currently private.

we could also introduce a static method on common, but we would still need to pass in blockNumber, perhaps something like:

  /**
   * Gets the hardfork based on the block's number.
   * @param chain String ('mainnet') or Number (1) chain
   * @param blockNumber Block number
   * @returns String identifier for hardfork ('byzantium')
   */
  static getHardforkForBlock(chain: string | number, blockNumber: number): string {
    const targetChain = Common._getChainParams(chain)
    let hardforks = targetChain.hardforks.filter(hardfork => hardfork.block && hardfork.block >= blockNumber)
    hardforks = hardforks.sort((a, b) => (a.block! > b.block!) ? 1 : -1)
    return hardforks[0].name
  }

[alcuadrado]

I think a static method like that would kill the possibility of using custom Common objects, right?

Testing networks, like Ganache or Buidler EVM, requires using custom Commons.

[ryanio]

@alcuadrado ah didn't know about custom Commons, but if a common object is passed in then it should skip that part of the initialization (wouldn't reach here).

but yeah I'm not too familiar with how this constructor is used across dependant libs so it's a bit difficult for me to see the solution.

[alcuadrado]

@alcuadrado ah didn't know about custom Commons, but if a common object is passed in then it should skip that part of the initialization (wouldn't reach here).

but yeah I'm not too familiar with how this constructor is used across dependant libs so it's a bit difficult for me to see the solution.

That's a good point. It should work then.

BTW, I always have the impression that accepting chain and hardfork in the constructors don't really work well with Common. I think I proposed dropping those some time ago.

[ryanio]

that would be nice as it would simplify a lot of our constructors across the monorepo.

[holgerd77]

@ryanio The BlockHeader has no problems with a Common instance without a hardfork set. So this should be no problem I guess?

I would say the most tricky part here is rather the mutability of the Common instance. So if Common is managed for e.g. by the client and the HF is later changed on a HF update or something, the HF from a block - respectively a transaction - would change as well.

I would argue that on the level of blocks and tx - since chain and fork are bound to the identity of these items - should rather internally use a copy of the Common instance pass and therefore detach from the original one - or alternatively - we should suggest for the usage of the API to pass a Common instance copy - not sure, think I would prefer the first variant.

The block constructor could then try to set the hardfork based by block number, eventually a new method on Common is needed for that like Common.setHardforkByBlockNumber?

Then the transactions can be initialized based on this modified Common block copy with an own respective copy (maybe in this case one copy for all transactions is enough for resource reasons?).

As a side note: I would also think a transaction should throw on initialization when no HF is provided along a Common instance or generally with the constructor, since with the logic in functions like Tx.validateV a transactions behavior is not deterministically defined without a HF set.

[holgerd77]

@alcuadrado @ryanio Also one thought, not completely thought through though yet: it might also be worth to think if we want to switch to static methods for Common for all the methods which can be used in a stateless context, so that it eventually gets easier to use Common without the need for initialization.

[holgerd77]

Another idea, partly complementary: we can pass in a flag to the block constructor to decide if HF is implicitly determined by the block number or by the Common instance passed.

[alcuadrado]

I have the feeling that the transaction class shouldn't have a common object, but take on as parameters whenever necessary (i.e. on validation and signing methods).

The information that Common represents is inherent to Block, and not to Transaction.

[holgerd77]

@alcuadrado hmm, I am not completely convinced here. There are lots of use cases where a tx is used without the scope of an encompassing block but where there is nevertheless the need/want for a clear and non-changing context state for the transaction. Taking this statefulness away here seems not the right solution to me but rather limiting the use cases.

[alcuadrado]

The issue I see is that signing and validation are two separate actions that can use different Commons. A user/dev can sign a tx thinking that it will be executed/validated with HF rules X, and it ends up being executed/validated with HF rules X+1.

I think this is important if the client is to be more complete.

[holgerd77]

Fixed by #863, will close.