Fix a crash in GetMissingBlockBodyHashes (#1289)

* Remove the limit on the amount of hashes returned from antiPastHashesBetween.

* Guard against requests with a non-existing block hash.

* Move missing-block-hash guards to consensus.go.

* Ban a peer that doesn't send us all the requested headers during IBD.

* Extract blockHeap.ToSlice.

* Re-request headers in requestHeaders if we didn't receive the highHash.

app/protocol/flows/blockrelay/handle_request_headers.go

@@ -38,7 +38,12 @@ func (flow *handleRequestBlocksFlow) start() error {
 			return err
 		}
 
-		blockHashes, err := flow.Domain().Consensus().GetHashesBetween(lowHash, highHash)
+		// GetHashesBetween is a relatively heavy operation so we limit it.
+		// We expect that if the other peer did not receive all the headers
+		// they requested, they'd re-request a block locator and re-request
+		// headers with a higher lowHash
+		const maxBlueScoreDifference = 1 << 12
+		blockHashes, err := flow.Domain().Consensus().GetHashesBetween(lowHash, highHash, maxBlueScoreDifference)
 		if err != nil {
 			return err
 		}

app/protocol/flows/blockrelay/ibd.go

@@ -92,6 +92,8 @@ func (flow *handleRelayInvsFlow) runIBDIfNotRunning(highHash *externalapi.Domain
 }
 
 func (flow *handleRelayInvsFlow) syncHeaders(highHash *externalapi.DomainHash) error {
+	highHashReceived := false
+	for !highHashReceived {
 		log.Debugf("Trying to find highest shared chain block with peer %s with high hash %s", flow.peer, highHash)
 		highestSharedBlockHash, err := flow.findHighestSharedBlockHash(highHash)
 		if err != nil {
@@ -99,7 +101,20 @@ func (flow *handleRelayInvsFlow) syncHeaders(highHash *externalapi.DomainHash) e
 		}
 		log.Debugf("Found highest shared chain block %s with peer %s", highestSharedBlockHash, flow.peer)
 
-	return flow.downloadHeaders(highestSharedBlockHash, highHash)
+		err = flow.downloadHeaders(highestSharedBlockHash, highHash)
+		if err != nil {
+			return err
+		}
+
+		// We're finished once highHash has been inserted into the DAG
+		blockInfo, err := flow.Domain().Consensus().GetBlockInfo(highHash)
+		if err != nil {
+			return err
+		}
+		highHashReceived = blockInfo.Exists
+		log.Debugf("Headers downloaded from peer %s. Are further headers required: %t", flow.peer, !highHashReceived)
+	}
+	return nil
 }
 
 func (flow *handleRelayInvsFlow) findHighestSharedBlockHash(highHash *externalapi.DomainHash) (

domain/consensus/consensus.go

@@ -95,6 +95,11 @@ func (s *consensus) GetBlock(blockHash *externalapi.DomainHash) (*externalapi.Do
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
+	err := s.validateBlockHashExists(blockHash)
+	if err != nil {
+		return nil, err
+	}
+
 	return s.blockStore.Block(s.databaseContext, blockHash)
 }
 
@@ -102,6 +107,11 @@ func (s *consensus) GetBlockHeader(blockHash *externalapi.DomainHash) (*external
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
+	err := s.validateBlockHashExists(blockHash)
+	if err != nil {
+		return nil, err
+	}
+
 	return s.blockHeaderStore.BlockHeader(s.databaseContext, blockHash)
 }
 
@@ -145,20 +155,41 @@ func (s *consensus) GetBlockAcceptanceData(blockHash *externalapi.DomainHash) (e
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
+	err := s.validateBlockHashExists(blockHash)
+	if err != nil {
+		return nil, err
+	}
+
 	return s.acceptanceDataStore.Get(s.databaseContext, blockHash)
 }
 
-func (s *consensus) GetHashesBetween(lowHash, highHash *externalapi.DomainHash) ([]*externalapi.DomainHash, error) {
+func (s *consensus) GetHashesBetween(lowHash, highHash *externalapi.DomainHash,
+	maxBlueScoreDifference uint64) ([]*externalapi.DomainHash, error) {
+
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
-	return s.syncManager.GetHashesBetween(lowHash, highHash)
+	err := s.validateBlockHashExists(lowHash)
+	if err != nil {
+		return nil, err
+	}
+	err = s.validateBlockHashExists(highHash)
+	if err != nil {
+		return nil, err
+	}
+
+	return s.syncManager.GetHashesBetween(lowHash, highHash, maxBlueScoreDifference)
 }
 
 func (s *consensus) GetMissingBlockBodyHashes(highHash *externalapi.DomainHash) ([]*externalapi.DomainHash, error) {
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
+	err := s.validateBlockHashExists(highHash)
+	if err != nil {
+		return nil, err
+	}
+
 	return s.syncManager.GetMissingBlockBodyHashes(highHash)
 }
 
@@ -249,6 +280,15 @@ func (s *consensus) CreateBlockLocator(lowHash, highHash *externalapi.DomainHash
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
+	err := s.validateBlockHashExists(lowHash)
+	if err != nil {
+		return nil, err
+	}
+	err = s.validateBlockHashExists(highHash)
+	if err != nil {
+		return nil, err
+	}
+
 	return s.syncManager.CreateBlockLocator(lowHash, highHash, limit)
 }
 
@@ -256,6 +296,10 @@ func (s *consensus) FindNextBlockLocatorBoundaries(blockLocator externalapi.Bloc
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
+	if len(blockLocator) == 0 {
+		return nil, nil, errors.Errorf("empty block locator")
+	}
+
 	return s.syncManager.FindNextBlockLocatorBoundaries(blockLocator)
 }
 
@@ -266,13 +310,34 @@ func (s *consensus) GetSyncInfo() (*externalapi.SyncInfo, error) {
 	return s.syncManager.GetSyncInfo()
 }
 
-func (s *consensus) IsValidPruningPoint(block *externalapi.DomainHash) (bool, error) {
+func (s *consensus) IsValidPruningPoint(blockHash *externalapi.DomainHash) (bool, error) {
-	return s.pruningManager.IsValidPruningPoint(block)
+	err := s.validateBlockHashExists(blockHash)
+	if err != nil {
+		return false, err
+	}
+
+	return s.pruningManager.IsValidPruningPoint(blockHash)
 }
 
 func (s *consensus) GetVirtualSelectedParentChainFromBlock(blockHash *externalapi.DomainHash) (*externalapi.SelectedParentChainChanges, error) {
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
+	err := s.validateBlockHashExists(blockHash)
+	if err != nil {
+		return nil, err
+	}
+
 	return s.consensusStateManager.GetVirtualSelectedParentChainFromBlock(blockHash)
 }
+
+func (s *consensus) validateBlockHashExists(blockHash *externalapi.DomainHash) error {
+	exists, err := s.blockStatusStore.Exists(s.databaseContext, blockHash)
+	if err != nil {
+		return err
+	}
+	if !exists {
+		return errors.Errorf("block %s does not exists", blockHash)
+	}
+	return nil
+}

domain/consensus/model/block_heap.go

@@ -7,4 +7,5 @@ type BlockHeap interface {
 	Push(blockHash *externalapi.DomainHash) error
 	Pop() *externalapi.DomainHash
 	Len() int
+	ToSlice() []*externalapi.DomainHash
 }

domain/consensus/model/externalapi/consensus.go

@@ -11,7 +11,7 @@ type Consensus interface {
 	GetBlockInfo(blockHash *DomainHash) (*BlockInfo, error)
 	GetBlockAcceptanceData(blockHash *DomainHash) (AcceptanceData, error)
 
-	GetHashesBetween(lowHash, highHash *DomainHash) ([]*DomainHash, error)
+	GetHashesBetween(lowHash, highHash *DomainHash, maxBlueScoreDifference uint64) ([]*DomainHash, error)
 	GetMissingBlockBodyHashes(highHash *DomainHash) ([]*DomainHash, error)
 	GetPruningPointUTXOSet(expectedPruningPointHash *DomainHash) ([]byte, error)
 	PruningPoint() (*DomainHash, error)
@@ -22,6 +22,6 @@ type Consensus interface {
 	GetSyncInfo() (*SyncInfo, error)
 	Tips() ([]*DomainHash, error)
 	GetVirtualInfo() (*VirtualInfo, error)
-	IsValidPruningPoint(block *DomainHash) (bool, error)
+	IsValidPruningPoint(blockHash *DomainHash) (bool, error)
 	GetVirtualSelectedParentChainFromBlock(blockHash *DomainHash) (*SelectedParentChainChanges, error)
 }

domain/consensus/model/interface_processes_pruningmanager.go

@@ -5,5 +5,5 @@ import "github.com/kaspanet/kaspad/domain/consensus/model/externalapi"
 // PruningManager resolves and manages the current pruning point
 type PruningManager interface {
 	UpdatePruningPointByVirtual() error
-	IsValidPruningPoint(block *externalapi.DomainHash) (bool, error)
+	IsValidPruningPoint(blockHash *externalapi.DomainHash) (bool, error)
 }

domain/consensus/model/interface_processes_syncmanager.go

@@ -4,7 +4,7 @@ import "github.com/kaspanet/kaspad/domain/consensus/model/externalapi"
 
 // SyncManager exposes functions to support sync between kaspad nodes
 type SyncManager interface {
-	GetHashesBetween(lowHash, highHash *externalapi.DomainHash) ([]*externalapi.DomainHash, error)
+	GetHashesBetween(lowHash, highHash *externalapi.DomainHash, maxBlueScoreDifference uint64) ([]*externalapi.DomainHash, error)
 	GetMissingBlockBodyHashes(highHash *externalapi.DomainHash) ([]*externalapi.DomainHash, error)
 	CreateBlockLocator(lowHash, highHash *externalapi.DomainHash, limit uint32) (externalapi.BlockLocator, error)
 	FindNextBlockLocatorBoundaries(blockLocator externalapi.BlockLocator) (lowHash, highHash *externalapi.DomainHash, err error)

domain/consensus/processes/dagtraversalmanager/block_heap.go

@@ -114,6 +114,16 @@ func (bh *blockHeap) Len() int {
 	return bh.impl.Len()
 }
 
+// ToSlice copies this heap to a slice
+func (bh *blockHeap) ToSlice() []*externalapi.DomainHash {
+	length := bh.Len()
+	hashes := make([]*externalapi.DomainHash, length)
+	for i := 0; i < length; i++ {
+		hashes[i] = bh.Pop()
+	}
+	return hashes
+}
+
 // sizedUpBlockHeap represents a mutable heap of Blocks, sorted by their blueWork+hash, capped by a specific size.
 type sizedUpBlockHeap struct {
 	impl          upHeap

domain/consensus/processes/pruningmanager/pruningmanager.go

@@ -292,8 +292,8 @@ func (pm *pruningManager) deleteBlock(blockHash *externalapi.DomainHash) (alread
 	return false, nil
 }
 
-func (pm *pruningManager) IsValidPruningPoint(block *externalapi.DomainHash) (bool, error) {
+func (pm *pruningManager) IsValidPruningPoint(blockHash *externalapi.DomainHash) (bool, error) {
-	if *pm.genesisHash == *block {
+	if *pm.genesisHash == *blockHash {
 		return true, nil
 	}
 
@@ -308,7 +308,7 @@ func (pm *pruningManager) IsValidPruningPoint(block *externalapi.DomainHash) (bo
 		return false, err
 	}
 
-	isInSelectedParentChainOfHeadersSelectedTip, err := pm.dagTopologyManager.IsInSelectedParentChainOf(block,
+	isInSelectedParentChainOfHeadersSelectedTip, err := pm.dagTopologyManager.IsInSelectedParentChainOf(blockHash,
 		headersSelectedTip)
 	if err != nil {
 		return false, err
@@ -318,7 +318,7 @@ func (pm *pruningManager) IsValidPruningPoint(block *externalapi.DomainHash) (bo
 		return false, nil
 	}
 
-	ghostdagData, err := pm.ghostdagDataStore.Get(pm.databaseContext, block)
+	ghostdagData, err := pm.ghostdagDataStore.Get(pm.databaseContext, blockHash)
 	if err != nil {
 		return false, err
 	}

domain/consensus/processes/syncmanager/antipast.go

@@ -6,12 +6,12 @@ import (
 	"github.com/pkg/errors"
 )
 
-const maxHashesInAntiPastHashesBetween = 1 << 17
-
 // antiPastHashesBetween returns the hashes of the blocks between the
 // lowHash's antiPast and highHash's antiPast, or up to
-// maxHashesInAntiPastHashesBetween.
+// `maxBlueScoreDifference`, if non-zero.
-func (sm *syncManager) antiPastHashesBetween(lowHash, highHash *externalapi.DomainHash) ([]*externalapi.DomainHash, error) {
+func (sm *syncManager) antiPastHashesBetween(lowHash, highHash *externalapi.DomainHash,
+	maxBlueScoreDifference uint64) ([]*externalapi.DomainHash, error) {
+
 	lowBlockGHOSTDAGData, err := sm.ghostdagDataStore.Get(sm.databaseContext, lowHash)
 	if err != nil {
 		return nil, err
@@ -25,16 +25,17 @@ func (sm *syncManager) antiPastHashesBetween(lowHash, highHash *externalapi.Doma
 			lowBlockGHOSTDAGData.BlueScore(), highBlockGHOSTDAGData.BlueScore())
 	}
 
-	// In order to get no more then maxHashesInAntiPastHashesBetween
+	if maxBlueScoreDifference != 0 {
+		// In order to get no more then maxBlueScoreDifference
 		// blocks from the future of the lowHash (including itself),
 		// we iterate the selected parent chain of the highNode and
 		// stop once we reach
-	// highBlockBlueScore-lowBlockBlueScore+1 <= maxHashesInAntiPastHashesBetween.
+		// highBlockBlueScore-lowBlockBlueScore+1 <= maxBlueScoreDifference.
 		// That stop point becomes the new highHash.
 		// Using blueScore as an approximation is considered to be
 		// fairly accurate because we presume that most DAG blocks are
 		// blue.
-	for highBlockGHOSTDAGData.BlueScore()-lowBlockGHOSTDAGData.BlueScore()+1 > maxHashesInAntiPastHashesBetween {
+		for highBlockGHOSTDAGData.BlueScore()-lowBlockGHOSTDAGData.BlueScore()+1 > maxBlueScoreDifference {
 			highHash = highBlockGHOSTDAGData.SelectedParent()
 			var err error
 			highBlockGHOSTDAGData, err = sm.ghostdagDataStore.Get(sm.databaseContext, highHash)
@@ -42,12 +43,13 @@ func (sm *syncManager) antiPastHashesBetween(lowHash, highHash *externalapi.Doma
 				return nil, err
 			}
 		}
+	}
 
 	// Collect every node in highHash's past (including itself) but
 	// NOT in the lowHash's past (excluding itself) into an up-heap
 	// (a heap sorted by blueScore from lowest to greatest).
 	visited := hashset.New()
-	candidateHashes := sm.dagTraversalManager.NewUpHeap()
+	hashesUpHeap := sm.dagTraversalManager.NewUpHeap()
 	queue := sm.dagTraversalManager.NewDownHeap()
 	err = queue.Push(highHash)
 	if err != nil {
@@ -72,7 +74,7 @@ func (sm *syncManager) antiPastHashesBetween(lowHash, highHash *externalapi.Doma
 		if isCurrentAncestorOfLowHash {
 			continue
 		}
-		err = candidateHashes.Push(current)
+		err = hashesUpHeap.Push(current)
 		if err != nil {
 			return nil, err
 		}
@@ -88,17 +90,7 @@ func (sm *syncManager) antiPastHashesBetween(lowHash, highHash *externalapi.Doma
 		}
 	}
 
-	// Pop candidateHashes into a slice. Since candidateHashes is
+	return hashesUpHeap.ToSlice(), nil
-	// an up-heap, it's guaranteed to be ordered from low to high
-	hashesLength := maxHashesInAntiPastHashesBetween
-	if candidateHashes.Len() < hashesLength {
-		hashesLength = candidateHashes.Len()
-	}
-	hashes := make([]*externalapi.DomainHash, hashesLength)
-	for i := 0; i < hashesLength; i++ {
-		hashes[i] = candidateHashes.Pop()
-	}
-	return hashes, nil
 }
 
 func (sm *syncManager) missingBlockBodyHashes(highHash *externalapi.DomainHash) ([]*externalapi.DomainHash, error) {
@@ -134,7 +126,7 @@ func (sm *syncManager) missingBlockBodyHashes(highHash *externalapi.DomainHash)
 			lowHash, highHash)
 	}
 
-	hashesBetween, err := sm.antiPastHashesBetween(lowHash, highHash)
+	hashesBetween, err := sm.antiPastHashesBetween(lowHash, highHash, 0)
 	if err != nil {
 		return nil, err
 	}

domain/consensus/processes/syncmanager/syncmanager.go

@@ -54,11 +54,13 @@ func New(
 	}
 }
 
-func (sm *syncManager) GetHashesBetween(lowHash, highHash *externalapi.DomainHash) ([]*externalapi.DomainHash, error) {
+func (sm *syncManager) GetHashesBetween(lowHash, highHash *externalapi.DomainHash,
+	maxBlueScoreDifference uint64) ([]*externalapi.DomainHash, error) {
+
 	onEnd := logger.LogAndMeasureExecutionTime(log, "GetHashesBetween")
 	defer onEnd()
 
-	return sm.antiPastHashesBetween(lowHash, highHash)
+	return sm.antiPastHashesBetween(lowHash, highHash, maxBlueScoreDifference)
 }
 
 func (sm *syncManager) GetMissingBlockBodyHashes(highHash *externalapi.DomainHash) ([]*externalapi.DomainHash, error) {