Merge pull request #291 from ava-labs/fix-build-block

Add option to skip write block in block verification

core/blockchain.go

@@ -1041,7 +1041,7 @@ func (bc *BlockChain) InsertChain(chain types.Blocks) (int, error) {
 	bc.chainmu.Lock()
 	defer bc.chainmu.Unlock()
 	for n, block := range chain {
-		if err := bc.insertBlock(block); err != nil {
+		if err := bc.insertBlock(block, true); err != nil {
 			return n, err
 		}
 	}
@@ -1050,12 +1050,16 @@ func (bc *BlockChain) InsertChain(chain types.Blocks) (int, error) {
 }
 
 func (bc *BlockChain) InsertBlock(block *types.Block) error {
+	return bc.InsertBlockManual(block, true)
+}
+
+func (bc *BlockChain) InsertBlockManual(block *types.Block, writes bool) error {
 	bc.blockProcFeed.Send(true)
 	defer bc.blockProcFeed.Send(false)
 
 	bc.wg.Add(1)
 	bc.chainmu.Lock()
-	err := bc.insertBlock(block)
+	err := bc.insertBlock(block, writes)
 	bc.chainmu.Unlock()
 	bc.wg.Done()
 
@@ -1079,7 +1083,7 @@ func (bc *BlockChain) gatherBlockLogs(hash common.Hash, number uint64, removed b
 	return logs
 }
 
-func (bc *BlockChain) insertBlock(block *types.Block) error {
+func (bc *BlockChain) insertBlock(block *types.Block, writes bool) error {
 	senderCacher.recover(types.MakeSigner(bc.chainConfig, block.Number(), new(big.Int).SetUint64(block.Time())), block.Transactions())
 
 	err := bc.engine.VerifyHeader(bc, block.Header())
@@ -1093,19 +1097,13 @@ func (bc *BlockChain) insertBlock(block *types.Block) error {
 		// snapshot layer and add a reference to the triedb, so we re-execute
 		// the block. Note that insertBlock should only be called on a block
 		// once if it returns nil
-
 		if bc.newTip(block) {
 			log.Debug("Setting head to be known block", "number", block.Number(), "hash", block.Hash())
 		} else {
 			log.Debug("Reprocessing already known block", "number", block.Number(), "hash", block.Hash())
 		}
 
-	// Pruning of the EVM is disabled, so we should never encounter this case.
-	// Because side chain insertion can have complex side effects, we error when
-	// we encounter it to prevent the accidental execution of these side effects.
-	//
-	// When supporting EVM pruning, we must re-enable this and ensure it is
-	// compatible with external consensus invariants.
+	// If an ancestor has been pruned, then this block cannot be acceptable.
 	case errors.Is(err, consensus.ErrPrunedAncestor):
 		return errors.New("side chain insertion is not supported")
 
@@ -1169,6 +1167,13 @@ func (bc *BlockChain) insertBlock(block *types.Block) error {
 		return err
 	}
 
+	// If [writes] are disabled, skip [writeBlockWithState] so that we do not write the block
+	// or the state trie to disk.
+	// Note: in pruning mode, this prevents us from generating a reference to the state root.
+	if !writes {
+		return nil
+	}
+
 	// Write the block to the chain and get the status.
 	// writeBlockWithState creates a reference that will be cleaned up in Accept/Reject
 	// so we need to ensure an error cannot occur later in verification, since that would

plugin/evm/block.go

@@ -206,16 +206,10 @@ func (b *Block) syntacticVerify() (params.Rules, error) {
 
 // Verify implements the snowman.Block interface
 func (b *Block) Verify() error {
-	if err := b.VerifyWithoutWrites(); err != nil {
-		return err
-	}
-
-	// InsertBlock must be the last step in verification to prevent a memory leak in the management
-	// of tries that the chain maintains an active reference to.
-	return b.vm.chain.InsertBlock(b.ethBlock)
+	return b.verify(true)
 }
 
-func (b *Block) VerifyWithoutWrites() error {
+func (b *Block) verify(writes bool) error {
 	rules, err := b.syntacticVerify()
 	if err != nil {
 		return fmt.Errorf("syntactic block verification failed: %w", err)
@@ -237,43 +231,31 @@ func (b *Block) VerifyWithoutWrites() error {
 	if err != nil {
 		return err
 	}
-	if atomicTx == nil {
-		return nil
-	}
-
-	// If the ancestor is unknown, then the parent failed verification when
-	// it was called.
-	// If the ancestor is rejected, then this block shouldn't be inserted
-	// into the canonical chain because the parent is will be missing.
-	if blkStatus := ancestorIntf.Status(); blkStatus == choices.Unknown || blkStatus == choices.Rejected {
-		return errRejectedParent
-	}
-	ancestor, ok := ancestorIntf.(*Block)
-	if !ok {
-		return fmt.Errorf("expected %s, parent of %s, to be *Block but is %T", ancestor.ID(), b.ID(), ancestorIntf)
-	}
-
-	parentState, err := vm.chain.BlockState(ancestor.ethBlock)
-	if err != nil {
-		return err
-	}
+	if atomicTx != nil {
+		// If the ancestor is unknown, then the parent failed verification when
+		// it was called.
+		// If the ancestor is rejected, then this block shouldn't be inserted
+		// into the canonical chain because the parent is will be missing.
+		if blkStatus := ancestorIntf.Status(); blkStatus == choices.Unknown || blkStatus == choices.Rejected {
+			return errRejectedParent
+		}
+		ancestor, ok := ancestorIntf.(*Block)
+		if !ok {
+			return fmt.Errorf("expected %s, parent of %s, to be *Block but is %T", ancestor.ID(), b.ID(), ancestorIntf)
+		}
 
-	if bonusBlocks.Contains(b.id) {
-		log.Info("skipping atomic tx verification on bonus block", "block", b.id)
-	} else {
-		utx := atomicTx.UnsignedAtomicTx
-		if err := utx.SemanticVerify(vm, atomicTx, ancestor, rules); err != nil {
-			return fmt.Errorf("invalid block due to failed semanatic verify: %w at height %d", err, b.Height())
+		if bonusBlocks.Contains(b.id) {
+			log.Info("skipping atomic tx verification on bonus block", "block", b.id)
+		} else {
+			utx := atomicTx.UnsignedAtomicTx
+			if err := utx.SemanticVerify(vm, atomicTx, ancestor, rules); err != nil {
+				return fmt.Errorf("invalid block due to failed semanatic verify: %w at height %d", err, b.Height())
+			}
 		}
 	}
 
-	// TODO: Because InsertChain calls Process, can't this invocation be removed?
 	bc := vm.chain.BlockChain()
-	_, _, _, err = bc.Processor().Process(b.ethBlock, parentState, *bc.GetVMConfig())
-	if err != nil {
-		return fmt.Errorf("invalid block due to failed processing: %w", err)
-	}
-	return nil
+	return bc.InsertBlockManual(b.ethBlock, writes)
 }
 
 // Bytes implements the snowman.Block interface

plugin/evm/vm.go

@@ -604,7 +604,10 @@ func (vm *VM) buildBlock() (snowman.Block, error) {
 	// Note: this is only called when building a new block, so caching
 	// verification will only be a significant optimization for nodes
 	// that produce a large number of blocks.
-	if err := blk.Verify(); err != nil {
+	// We call verify without writes here to avoid generating a reference
+	// to the blk state root in the triedb when we are going to call verify
+	// again from the consensus engine with writes enabled.
+	if err := blk.verify(false); err != nil {
 		vm.mempool.CancelCurrentTx()
 		return nil, fmt.Errorf("block failed verification due to: %w", err)
 	}

plugin/evm/vm_test.go

@@ -585,6 +585,11 @@ func TestBuildEthTxBlock(t *testing.T) {
 		t.Fatalf("Expected last accepted blockID to be the accepted block: %s, but found %s", blk2.ID(), lastAcceptedID)
 	}
 
+	ethBlk1 := blk1.(*chain.BlockWrapper).Block.(*Block).ethBlock
+	if ethBlk1Root := ethBlk1.Root(); vm.chain.BlockChain().HasState(ethBlk1Root) {
+		t.Fatalf("Expected blk1 state root to be pruned after blk2 was accepted on top of it in pruning mode")
+	}
+
 	// Clear the cache and ensure that GetBlock returns internal blocks with the correct status
 	vm.State.Flush()
 	blk2Refreshed, err := vm.GetBlockInternal(blk2.ID())