VM: RunBlock() can sometimes throw and not revert the state modifications

[alcuadrado]

I discovered this edge-case while documenting how the VM methods modify the state.

If runBlock is run without generate: true and an invalid block is given, the state will be modified and an error will be thrown. This is counter-intuitive.

The current code is:

/* ... */
// Checkpoint state
  await state.checkpoint()
  let result
  try {
    result = await applyBlock.bind(this)(block, opts.skipBlockValidation)
  } catch (err) {
    await state.revert()
    throw err
  }

  // Persist state
  await state.commit()
  const stateRoot = await state.getStateRoot()

  // Given the generate option, either set resulting header
  // values to the current block, or validate the resulting
  // header values against the current block.
  if (generateStateRoot) {
    block.header.stateRoot = stateRoot
    block.header.bloom = result.bloom.bitvector
  } else {
    if (
      result.receiptRoot &&
      result.receiptRoot.toString('hex') !== block.header.receiptTrie.toString('hex')
    ) {
      throw new Error('invalid receiptTrie ')
    }
/* ... */

So it can throw after a commit.

It should probably be something like this:

// Checkpoint state
  await state.checkpoint()
  let result
  try {
    result = await applyBlock.bind(this)(block, opts.skipBlockValidation)

    const stateRoot = await state.getStateRoot()

    // Given the generate option, either set resulting header
    // values to the current block, or validate the resulting
    // header values against the current block.
    if (generateStateRoot) {
      block.header.stateRoot = stateRoot
      block.header.bloom = result.bloom.bitvector
    } else {
      if (
        result.receiptRoot &&
        result.receiptRoot.toString('hex') !== block.header.receiptTrie.toString('hex')
      ) {
        throw new Error('invalid receiptTrie ')
      }
      if (result.bloom.bitvector.toString('hex') !== block.header.bloom.toString('hex')) {
        throw new Error('invalid bloom ')
      }
      if (bufferToInt(block.header.gasUsed) !== Number(result.gasUsed)) {
        throw new Error('invalid gasUsed ')
      }
      if (stateRoot.toString('hex') !== block.header.stateRoot.toString('hex')) {
        throw new Error('invalid block stateRoot ')
      }
    }
  } catch (err) {
    await state.revert()
    throw err
  }

  await state.commit()

The problem is that state.getStateRoot() throws if there's an uncommitted checkpoint.

[s1na]

Good point. One difficulty I see with your solution is that currently getStateRoot requires state to be committed.

Throwing an error on validation seems also a bit extreme. runBlock could potentially be split into two, one that runs a block and verifies the results against a block header (i.e. opts.generate = false) and returns a boolean as result, and one that runs a block and produces a block header.

[alcuadrado]

Throwing an error on validation seems also a bit extreme. runBlock could potentially be split into two, one that runs a block and verifies the results against a block header (i.e. opts.generate = false) and returns a boolean as result, and one that runs a block and produces a block header.

This is an interesting idea, but it would mean running the block twice, right?

Another approach is returning a new Block on runBlock, not modifying the existing one. Then, you can compare it against the original one and check if their headers match.

I'm convinced that domain objects (i.e. Account, Block, Transaction) should be immutable, so I'm more inclined to the alternative I proposed.

[s1na]

This is an interesting idea, but it would mean running the block twice, right?

No, I just meant we have two methods for different use cases. You'd need either one or the other. Just as a side note, this approach still won't solve an error being thrown by getStateRoot if there are uncommitted checkpoints.

I'm convinced that domain objects (i.e. Account, Block, Transaction) should be immutable, so I'm more inclined to the alternative I proposed.

That also sounds good

[alcuadrado]

No, I just meant we have two methods for different use cases.

Oh, I see. Makes sense.

[holgerd77]

This came up in a newer issue as well (can't find the reference atm), where I proposed double-checkpointing to solve this (needs some validation), so to do two checkpoints before running the block, then commit towards the second CP to have the correct state root and - in case the state root check or other checks fail - revert to the initial checkpoint. This should be doable now with the reworked CP mechanism in the trie without (too much) penalty.

WDYT? 🤔

//cc @jochem-brouwer (since we had the discussion)

[holgerd77]

This should now be implementable with getStateRoot not throwing any more on uncommitted checkpoints, see #1209. On a first review of our current code and x-check with the proposed solution from @alcuadrado I would also think this is still an issue which should be addressed?

[holgerd77]

This needs re-newed validation, might still be a thing though.

[g11tech]

i think this should not be an issue anymore as far as i have observed in writing some testcases , may be i can add a test case