Fix UTXO diff child error (#2084)

* Avoid creating the chain iterator if high hash is actually low hash

* Always use iterator in nextPruningPointAndCandidateByBlockHash

* Initial failing test

* Minimal failing test + some comments

* go lint

* Add simpler tests with two different errors

* Missed some error checks

* Minor

* A workaround patch for preventing the missing utxo child diff bug

* Make sure we fully resolve virtual

* Move ResolveVirtualWithMaxParam to test consensus

* Mark virtual not updated and loop in batches

* Refactor: remove VirtualChangeSet from functions return values

* Remove workaround comments

* If block has no body, virtual is still considered updated

* Remove special error ErrReverseUTXODiffsUTXODiffChildNotFound

Co-authored-by: Ori Newman <orinewman1@gmail.com>

app/protocol/flowcontext/blocks.go

@@ -107,7 +107,7 @@ func (f *FlowContext) AddBlock(block *externalapi.DomainBlock) error {
 		return protocolerrors.Errorf(false, "cannot add header only block")
 	}
 
-	_, err := f.Domain().Consensus().ValidateAndInsertBlock(block, true)
+	err := f.Domain().Consensus().ValidateAndInsertBlock(block, true)
 	if err != nil {
 		if errors.As(err, &ruleerrors.RuleError{}) {
 			log.Warnf("Validation failed for block %s: %s", consensushashing.BlockHash(block), err)

app/protocol/flowcontext/orphans.go

@@ -141,7 +141,7 @@ func (f *FlowContext) unorphanBlock(orphanHash externalapi.DomainHash) (bool, er
 	}
 	delete(f.orphans, orphanHash)
 
-	_, err := f.domain.Consensus().ValidateAndInsertBlock(orphanBlock, true)
+	err := f.domain.Consensus().ValidateAndInsertBlock(orphanBlock, true)
 	if err != nil {
 		if errors.As(err, &ruleerrors.RuleError{}) {
 			log.Warnf("Validation failed for orphan block %s: %s", orphanHash, err)

app/protocol/flows/v5/blockrelay/handle_relay_invs.go

@@ -321,7 +321,7 @@ func (flow *handleRelayInvsFlow) readMsgBlock() (msgBlock *appmessage.MsgBlock,
 
 func (flow *handleRelayInvsFlow) processBlock(block *externalapi.DomainBlock) ([]*externalapi.DomainHash, error) {
 	blockHash := consensushashing.BlockHash(block)
-	_, err := flow.Domain().Consensus().ValidateAndInsertBlock(block, true)
+	err := flow.Domain().Consensus().ValidateAndInsertBlock(block, true)
 	if err != nil {
 		if !errors.As(err, &ruleerrors.RuleError{}) {
 			return nil, errors.Wrapf(err, "failed to process block %s", blockHash)

app/protocol/flows/v5/blockrelay/ibd.go

@@ -488,7 +488,7 @@ func (flow *handleIBDFlow) processHeader(consensus externalapi.Consensus, msgBlo
 		log.Debugf("Block header %s is already in the DAG. Skipping...", blockHash)
 		return nil
 	}
-	_, err = consensus.ValidateAndInsertBlock(block, false)
+	err = consensus.ValidateAndInsertBlock(block, false)
 	if err != nil {
 		if !errors.As(err, &ruleerrors.RuleError{}) {
 			return errors.Wrapf(err, "failed to process header %s during IBD", blockHash)
@@ -654,7 +654,7 @@ func (flow *handleIBDFlow) syncMissingBlockBodies(highHash *externalapi.DomainHa
 				return err
 			}
 
-			_, err = flow.Domain().Consensus().ValidateAndInsertBlock(block, false)
+			err = flow.Domain().Consensus().ValidateAndInsertBlock(block, false)
 			if err != nil {
 				if errors.Is(err, ruleerrors.ErrDuplicateBlock) {
 					log.Debugf("Skipping IBD Block %s as it has already been added to the DAG", blockHash)
@@ -705,7 +705,7 @@ func (flow *handleIBDFlow) resolveVirtual(estimatedVirtualDAAScoreTarget uint64)
 			}
 			log.Infof("Resolving virtual. Estimated progress: %d%%", percents)
 		}
-		_, isCompletelyResolved, err := flow.Domain().Consensus().ResolveVirtual()
+		isCompletelyResolved, err := flow.Domain().Consensus().ResolveVirtual()
 		if err != nil {
 			return err
 		}

app/protocol/flows/v5/blockrelay/ibd_with_headers_proof.go

@@ -280,7 +280,7 @@ func (flow *handleIBDFlow) processBlockWithTrustedData(
 		blockWithTrustedData.GHOSTDAGData = append(blockWithTrustedData.GHOSTDAGData, appmessage.GHOSTDAGHashPairToDomainGHOSTDAGHashPair(data.GHOSTDAGData[index]))
 	}
 
-	_, err := consensus.ValidateAndInsertBlockWithTrustedData(blockWithTrustedData, false)
+	err := consensus.ValidateAndInsertBlockWithTrustedData(blockWithTrustedData, false)
 	return err
 }
 

domain/consensus/consensus.go

@@ -61,17 +61,18 @@ type consensus struct {
 	blocksWithTrustedDataDAAWindowStore model.BlocksWithTrustedDataDAAWindowStore
 
 	consensusEventsChan chan externalapi.ConsensusEvent
+	virtualNotUpdated   bool
 }
 
-func (s *consensus) ValidateAndInsertBlockWithTrustedData(block *externalapi.BlockWithTrustedData, validateUTXO bool) (*externalapi.VirtualChangeSet, error) {
+func (s *consensus) ValidateAndInsertBlockWithTrustedData(block *externalapi.BlockWithTrustedData, validateUTXO bool) error {
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
-	virtualChangeSet, _, err := s.blockProcessor.ValidateAndInsertBlockWithTrustedData(block, validateUTXO)
+	_, _, err := s.blockProcessor.ValidateAndInsertBlockWithTrustedData(block, validateUTXO)
 	if err != nil {
-		return nil, err
+		return err
 	}
-	return virtualChangeSet, nil
+	return nil
 }
 
 // Init initializes consensus
@@ -193,21 +194,47 @@ func (s *consensus) BuildBlockTemplate(coinbaseData *externalapi.DomainCoinbaseD
 
 // ValidateAndInsertBlock validates the given block and, if valid, applies it
 // to the current state
-func (s *consensus) ValidateAndInsertBlock(block *externalapi.DomainBlock, shouldValidateAgainstUTXO bool) (*externalapi.VirtualChangeSet, error) {
+func (s *consensus) ValidateAndInsertBlock(block *externalapi.DomainBlock, shouldValidateAgainstUTXO bool) error {
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
-	virtualChangeSet, blockStatus, err := s.blockProcessor.ValidateAndInsertBlock(block, shouldValidateAgainstUTXO)
+	_, err := s.validateAndInsertBlockNoLock(block, shouldValidateAgainstUTXO)
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
+func (s *consensus) validateAndInsertBlockNoLock(block *externalapi.DomainBlock, updateVirtual bool) (*externalapi.VirtualChangeSet, error) {
+	// If virtual is in non-updated state, and the caller requests updating virtual -- then we must first
+	// resolve virtual so that the new block can be fully processed properly
+	if updateVirtual && s.virtualNotUpdated {
+		for s.virtualNotUpdated {
+			// We use 10000 << finality interval. See comment in `ResolveVirtual`.
+			// We give up responsiveness of consensus in this rare case.
+			_, err := s.resolveVirtualNoLock(10000) // Note `s.virtualNotUpdated` is updated within the call
+			if err != nil {
+				return nil, err
+			}
+		}
+	}
+
+	virtualChangeSet, blockStatus, err := s.blockProcessor.ValidateAndInsertBlock(block, updateVirtual)
 	if err != nil {
 		return nil, err
 	}
 
+	// If block has a body, and yet virtual was not updated -- signify that virtual is in non-updated state
+	if !updateVirtual && blockStatus != externalapi.StatusHeaderOnly {
+		s.virtualNotUpdated = true
+	}
+
 	err = s.sendBlockAddedEvent(block, blockStatus)
 	if err != nil {
 		return nil, err
 	}
 
-	err = s.sendVirtualChangedEvent(virtualChangeSet, shouldValidateAgainstUTXO)
+	err = s.sendVirtualChangedEvent(virtualChangeSet, updateVirtual)
 	if err != nil {
 		return nil, err
 	}
@@ -861,36 +888,41 @@ func (s *consensus) PopulateMass(transaction *externalapi.DomainTransaction) {
 	s.transactionValidator.PopulateMass(transaction)
 }
 
-func (s *consensus) ResolveVirtual() (*externalapi.VirtualChangeSet, bool, error) {
+func (s *consensus) ResolveVirtual() (bool, error) {
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
 	// In order to prevent a situation that the consensus lock is held for too much time, we
 	// release the lock each time resolve 100 blocks.
 	// Note: maxBlocksToResolve should be smaller than finality interval in order to avoid a situation
 	// where UpdatePruningPointByVirtual skips a pruning point.
-	virtualChangeSet, isCompletelyResolved, err := s.consensusStateManager.ResolveVirtual(100)
+	return s.resolveVirtualNoLock(100)
+}
+
+func (s *consensus) resolveVirtualNoLock(maxBlocksToResolve uint64) (bool, error) {
+	virtualChangeSet, isCompletelyResolved, err := s.consensusStateManager.ResolveVirtual(maxBlocksToResolve)
 	if err != nil {
-		return nil, false, err
+		return false, err
 	}
+	s.virtualNotUpdated = !isCompletelyResolved
 
 	stagingArea := model.NewStagingArea()
 	err = s.pruningManager.UpdatePruningPointByVirtual(stagingArea)
 	if err != nil {
-		return nil, false, err
+		return false, err
 	}
 
 	err = staging.CommitAllChanges(s.databaseContext, stagingArea)
 	if err != nil {
-		return nil, false, err
+		return false, err
 	}
 
 	err = s.sendVirtualChangedEvent(virtualChangeSet, true)
 	if err != nil {
-		return nil, false, err
+		return false, err
 	}
 
-	return virtualChangeSet, isCompletelyResolved, nil
+	return isCompletelyResolved, nil
 }
 
 func (s *consensus) BuildPruningPointProof() (*externalapi.PruningPointProof, error) {

domain/consensus/consensus_test.go

@@ -27,7 +27,7 @@ func TestConsensus_GetBlockInfo(t *testing.T) {
 		newHeader := invalidBlock.Header.ToMutable()
 		newHeader.SetTimeInMilliseconds(0)
 		invalidBlock.Header = newHeader.ToImmutable()
-		_, err = consensus.ValidateAndInsertBlock(invalidBlock, true)
+		err = consensus.ValidateAndInsertBlock(invalidBlock, true)
 		if !errors.Is(err, ruleerrors.ErrTimeTooOld) {
 			t.Fatalf("Expected block to be invalid with err: %v, instead found: %v", ruleerrors.ErrTimeTooOld, err)
 		}
@@ -55,7 +55,7 @@ func TestConsensus_GetBlockInfo(t *testing.T) {
 			t.Fatalf("consensus.BuildBlock with an empty coinbase shouldn't fail: %v", err)
 		}
 
-		_, err = consensus.ValidateAndInsertBlock(validBlock, true)
+		err = consensus.ValidateAndInsertBlock(validBlock, true)
 		if err != nil {
 			t.Fatalf("consensus.ValidateAndInsertBlock with a block straight from consensus.BuildBlock should not fail: %v", err)
 		}

domain/consensus/finality_test.go

@@ -33,7 +33,7 @@ func TestFinality(t *testing.T) {
 				return nil, err
 			}
 
-			_, err = consensus.ValidateAndInsertBlock(block, true)
+			err = consensus.ValidateAndInsertBlock(block, true)
 			if err != nil {
 				return nil, err
 			}
@@ -201,7 +201,7 @@ func TestBoundedMergeDepth(t *testing.T) {
 				return nil, false // fo some reason go doesn't recognize that t.Fatalf never returns
 			}
 
-			_, err = consensus.ValidateAndInsertBlock(block, true)
+			err = consensus.ValidateAndInsertBlock(block, true)
 			if err == nil {
 				return block, false
 			} else if errors.Is(err, ruleerrors.ErrViolatingBoundedMergeDepth) {
@@ -213,7 +213,7 @@ func TestBoundedMergeDepth(t *testing.T) {
 		}
 
 		processBlock := func(consensus testapi.TestConsensus, block *externalapi.DomainBlock, name string) {
-			_, err := consensus.ValidateAndInsertBlock(block, true)
+			err := consensus.ValidateAndInsertBlock(block, true)
 			if err != nil {
 				t.Fatalf("TestBoundedMergeDepth: %s got unexpected error from ProcessBlock: %+v", name, err)
 
@@ -225,7 +225,7 @@ func TestBoundedMergeDepth(t *testing.T) {
 			if err != nil {
 				t.Fatalf("TestBoundedMergeDepth: Failed building block: %+v", err)
 			}
-			_, err = consensus.ValidateAndInsertBlock(block, true)
+			err = consensus.ValidateAndInsertBlock(block, true)
 			if err != nil {
 				t.Fatalf("TestBoundedMergeDepth: Failed Inserting block to consensus: %v", err)
 			}
@@ -268,7 +268,7 @@ func TestBoundedMergeDepth(t *testing.T) {
 					t.Fatalf("GetBlockHeader: %+v", err)
 				}
 
-				_, err = tcSyncee.ValidateAndInsertBlock(block, true)
+				err = tcSyncee.ValidateAndInsertBlock(block, true)
 				if err != nil {
 					t.Fatalf("ValidateAndInsertBlock %d: %+v", i, err)
 				}
@@ -556,7 +556,7 @@ func TestFinalityResolveVirtual(t *testing.T) {
 				block.Header = mutableHeader.ToImmutable()
 			}
 
-			_, err = tcAttacker.ValidateAndInsertBlock(block, true)
+			err = tcAttacker.ValidateAndInsertBlock(block, true)
 			if err != nil {
 				panic(err)
 			}
@@ -583,14 +583,14 @@ func TestFinalityResolveVirtual(t *testing.T) {
 		t.Logf("Side chain tip (%s) blue score %d", sideChainTipHash, sideChainTipGHOSTDAGData.BlueScore())
 
 		for _, block := range sideChain {
-			_, err := tc.ValidateAndInsertBlock(block, false)
+			err := tc.ValidateAndInsertBlock(block, false)
 			if err != nil {
 				panic(err)
 			}
 		}
 
 		for i := 0; ; i++ {
-			_, isCompletelyResolved, err := tc.ResolveVirtual()
+			isCompletelyResolved, err := tc.ResolveVirtual()
 			if err != nil {
 				panic(err)
 			}

domain/consensus/model/externalapi/consensus.go

@@ -5,8 +5,8 @@ type Consensus interface {
 	Init(skipAddingGenesis bool) error
 	BuildBlock(coinbaseData *DomainCoinbaseData, transactions []*DomainTransaction) (*DomainBlock, error)
 	BuildBlockTemplate(coinbaseData *DomainCoinbaseData, transactions []*DomainTransaction) (*DomainBlockTemplate, error)
-	ValidateAndInsertBlock(block *DomainBlock, shouldValidateAgainstUTXO bool) (*VirtualChangeSet, error)
-	ValidateAndInsertBlockWithTrustedData(block *BlockWithTrustedData, validateUTXO bool) (*VirtualChangeSet, error)
+	ValidateAndInsertBlock(block *DomainBlock, shouldValidateAgainstUTXO bool) error
+	ValidateAndInsertBlockWithTrustedData(block *BlockWithTrustedData, validateUTXO bool) error
 	ValidateTransactionAndPopulateWithConsensusData(transaction *DomainTransaction) error
 	ImportPruningPoints(pruningPoints []BlockHeader) error
 	BuildPruningPointProof() (*PruningPointProof, error)
@@ -48,7 +48,7 @@ type Consensus interface {
 	Anticone(blockHash *DomainHash) ([]*DomainHash, error)
 	EstimateNetworkHashesPerSecond(startHash *DomainHash, windowSize int) (uint64, error)
 	PopulateMass(transaction *DomainTransaction)
-	ResolveVirtual() (*VirtualChangeSet, bool, error)
+	ResolveVirtual() (bool, error)
 	BlockDAAWindowHashes(blockHash *DomainHash) ([]*DomainHash, error)
 	TrustedDataDataDAAHeader(trustedBlockHash, daaBlockHash *DomainHash, daaBlockWindowIndex uint64) (*TrustedDataDataDAAHeader, error)
 	TrustedBlockAssociatedGHOSTDAGDataBlockHashes(blockHash *DomainHash) ([]*DomainHash, error)

domain/consensus/model/testapi/test_consensus.go

@@ -49,6 +49,8 @@ type TestConsensus interface {
 		*externalapi.VirtualChangeSet, error)
 	UpdatePruningPointByVirtual() error
 
+	ResolveVirtualWithMaxParam(maxBlocksToResolve uint64) (bool, error)
+
 	MineJSON(r io.Reader, blockType MineJSONBlockType) (tips []*externalapi.DomainHash, err error)
 	ToJSON(w io.Writer) error
 

domain/consensus/processes/blockprocessor/validate_and_insert_block.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"github.com/kaspanet/kaspad/domain/consensus/model"
 	"github.com/kaspanet/kaspad/domain/consensus/model/externalapi"
-	"github.com/kaspanet/kaspad/domain/consensus/processes/consensusstatemanager"
 	"github.com/kaspanet/kaspad/domain/consensus/ruleerrors"
 	"github.com/kaspanet/kaspad/domain/consensus/utils/consensushashing"
 	"github.com/kaspanet/kaspad/domain/consensus/utils/multiset"
@@ -165,12 +164,7 @@ func (bp *blockProcessor) validateAndInsertBlock(stagingArea *model.StagingArea,
 
 	if reversalData != nil {
 		err = bp.consensusStateManager.ReverseUTXODiffs(blockHash, reversalData)
-		// It's still not known what causes this error, but we can ignore it and not reverse the UTXO diffs
-		// and harm performance in some cases.
-		// TODO: Investigate why this error happens in the first place, and remove the workaround.
-		if errors.Is(err, consensusstatemanager.ErrReverseUTXODiffsUTXODiffChildNotFound) {
-			log.Errorf("Could not reverse UTXO diffs while resolving virtual: %s", err)
-		} else if err != nil {
+		if err != nil {
 			return nil, externalapi.StatusInvalid, err
 		}
 	}

domain/consensus/processes/blockprocessor/validate_and_insert_block_test.go

@@ -79,7 +79,7 @@ func TestBlockStatus(t *testing.T) {
 			disqualifiedBlock.Header.PruningPoint(),
 		)
 
-		_, err = tc.ValidateAndInsertBlock(disqualifiedBlock, true)
+		err = tc.ValidateAndInsertBlock(disqualifiedBlock, true)
 		if err != nil {
 			t.Fatalf("ValidateAndInsertBlock: %+v", err)
 		}
@@ -106,7 +106,7 @@ func TestBlockStatus(t *testing.T) {
 			disqualifiedBlock.Header.PruningPoint(),
 		)
 
-		_, err = tc.ValidateAndInsertBlock(invalidBlock, true)
+		err = tc.ValidateAndInsertBlock(invalidBlock, true)
 		if err == nil {
 			t.Fatalf("block is expected to be invalid")
 		}
@@ -139,11 +139,11 @@ func TestValidateAndInsertErrors(t *testing.T) {
 		if err != nil {
 			t.Fatalf("AddBlock: %+v", err)
 		}
-		_, err = tc.ValidateAndInsertBlock(blockWithStatusInvalid, true)
+		err = tc.ValidateAndInsertBlock(blockWithStatusInvalid, true)
 		if err == nil {
 			t.Fatalf("Test ValidateAndInsertBlock: Expected an error, because the block is invalid.")
 		}
-		_, err = tc.ValidateAndInsertBlock(blockWithStatusInvalid, true)
+		err = tc.ValidateAndInsertBlock(blockWithStatusInvalid, true)
 		if err == nil || !errors.Is(err, ruleerrors.ErrKnownInvalid) {
 			t.Fatalf("Expected block to be invalid with err: %v, instead found: %v", ruleerrors.ErrKnownInvalid, err)
 		}
@@ -155,12 +155,12 @@ func TestValidateAndInsertErrors(t *testing.T) {
 		if err != nil {
 			t.Fatalf("AddBlock: %+v", err)
 		}
-		_, err = tc.ValidateAndInsertBlock(block, true)
+		err = tc.ValidateAndInsertBlock(block, true)
 		if err != nil {
 			t.Fatalf("ValidateAndInsertBlock: %+v", err)
 		}
 		// resend the same block.
-		_, err = tc.ValidateAndInsertBlock(block, true)
+		err = tc.ValidateAndInsertBlock(block, true)
 		if err == nil || !errors.Is(err, ruleerrors.ErrDuplicateBlock) {
 			t.Fatalf("Expected block to be invalid with err: %v, instead found: %v", ruleerrors.ErrDuplicateBlock, err)
 		}
@@ -173,12 +173,12 @@ func TestValidateAndInsertErrors(t *testing.T) {
 			t.Fatalf("AddBlock: %+v", err)
 		}
 		onlyHeader.Transactions = []*externalapi.DomainTransaction{}
-		_, err = tc.ValidateAndInsertBlock(onlyHeader, true)
+		err = tc.ValidateAndInsertBlock(onlyHeader, true)
 		if err != nil {
 			t.Fatalf("AddBlock: %+v", err)
 		}
 		// resend the same header.
-		_, err = tc.ValidateAndInsertBlock(onlyHeader, true)
+		err = tc.ValidateAndInsertBlock(onlyHeader, true)
 		if err == nil || !errors.Is(err, ruleerrors.ErrDuplicateBlock) {
 			t.Fatalf("Expected block to be invalid with err: %v, instead found: %v", ruleerrors.ErrDuplicateBlock, err)
 		}