Remove parent lookup from issue

snow/engine/snowman/issuer.go

@@ -33,7 +33,7 @@ func (i *issuer) Execute(ctx context.Context, _ []ids.ID, abandoned []ids.ID) er
 	// If the parent block was abandoned, this block should be abandoned as
 	// well.
 	blkID := i.blk.ID()
-	i.t.removeFromPending(i.blk)
+	delete(i.t.pending, blkID)
 	i.t.addToNonVerifieds(i.blk)
 	return i.t.blocked.Abandon(ctx, blkID)
 }

snow/engine/snowman/transitive.go

@@ -257,7 +257,7 @@ func (t *Transitive) Put(ctx context.Context, nodeID ids.NodeID, requestID uint3
 		issuedMetric = t.metrics.issued.WithLabelValues(unknownSource)
 	}
 
-	if t.wasIssued(blk) {
+	if !t.shouldIssueBlock(blk) {
 		t.metrics.numUselessPutBytes.Add(float64(len(blkBytes)))
 	}
 
@@ -266,7 +266,7 @@ func (t *Transitive) Put(ctx context.Context, nodeID ids.NodeID, requestID uint3
 	// receive requests to fill the ancestry. dependencies that have already
 	// been fetched, but with missing dependencies themselves won't be requested
 	// from the vdr.
-	if _, err := t.issueFrom(ctx, nodeID, blk, issuedMetric); err != nil {
+	if err := t.issueFrom(ctx, nodeID, blk, issuedMetric); err != nil {
 		return err
 	}
 	return t.executeDeferredWork(ctx)
@@ -305,7 +305,7 @@ func (t *Transitive) PullQuery(ctx context.Context, nodeID ids.NodeID, requestID
 
 	// Try to issue [blkID] to consensus.
 	// If we're missing an ancestor, request it from [vdr]
-	if _, err := t.issueFromByID(ctx, nodeID, blkID, issuedMetric); err != nil {
+	if err := t.issueFromByID(ctx, nodeID, blkID, issuedMetric); err != nil {
 		return err
 	}
 
@@ -335,7 +335,7 @@ func (t *Transitive) PushQuery(ctx context.Context, nodeID ids.NodeID, requestID
 		return nil
 	}
 
-	if t.wasIssued(blk) {
+	if !t.shouldIssueBlock(blk) {
 		t.metrics.numUselessPushQueryBytes.Add(float64(len(blkBytes)))
 	}
 
@@ -346,7 +346,7 @@ func (t *Transitive) PushQuery(ctx context.Context, nodeID ids.NodeID, requestID
 	// receive requests to fill the ancestry. dependencies that have already
 	// been fetched, but with missing dependencies themselves won't be requested
 	// from the vdr.
-	if _, err := t.issueFrom(ctx, nodeID, blk, issuedMetric); err != nil {
+	if err := t.issueFrom(ctx, nodeID, blk, issuedMetric); err != nil {
 		return err
 	}
 
@@ -365,25 +365,23 @@ func (t *Transitive) Chits(ctx context.Context, nodeID ids.NodeID, requestID uin
 	)
 
 	issuedMetric := t.metrics.issued.WithLabelValues(pullGossipSource)
-
-	addedPreferred, err := t.issueFromByID(ctx, nodeID, preferredID, issuedMetric)
-	if err != nil {
+	if err := t.issueFromByID(ctx, nodeID, preferredID, issuedMetric); err != nil {
 		return err
 	}
 
 	var (
-		addedPreferredIDAtHeight = addedPreferred
+		preferredIDAtHeightShouldBlock bool
 		// Invariant: The order of [responseOptions] must be [preferredID] then
 		// (optionally) [preferredIDAtHeight]. During vote application, the
 		// first vote that can be applied will be used. So, the votes should be
 		// populated in order of decreasing height.
 		responseOptions = []ids.ID{preferredID}
 	)
 	if preferredID != preferredIDAtHeight {
-		addedPreferredIDAtHeight, err = t.issueFromByID(ctx, nodeID, preferredIDAtHeight, issuedMetric)
-		if err != nil {
+		if err := t.issueFromByID(ctx, nodeID, preferredIDAtHeight, issuedMetric); err != nil {
 			return err
 		}
+		preferredIDAtHeightShouldBlock = t.canDependOn(preferredIDAtHeight)
 		responseOptions = append(responseOptions, preferredIDAtHeight)
 	}
 
@@ -399,10 +397,10 @@ func (t *Transitive) Chits(ctx context.Context, nodeID ids.NodeID, requestID uin
 	// Wait until [preferredID] and [preferredIDAtHeight] have been issued to
 	// consensus before applying this chit.
 	var deps []ids.ID
-	if !addedPreferred {
+	if t.canDependOn(preferredID) {
 		deps = append(deps, preferredID)
 	}
-	if !addedPreferredIDAtHeight {
+	if preferredIDAtHeightShouldBlock {
 		deps = append(deps, preferredIDAtHeight)
 	}
 
@@ -682,16 +680,18 @@ func (t *Transitive) buildBlocks(ctx context.Context) error {
 		}
 
 		issuedMetric := t.metrics.issued.WithLabelValues(builtSource)
-		added, err := t.issueWithAncestors(ctx, blk, issuedMetric)
-		if err != nil {
+		if err := t.issueWithAncestors(ctx, blk, issuedMetric); err != nil {
 			return err
 		}
 
-		// issuing the block shouldn't have any missing dependencies
-		if added {
+		// TODO: Technically this may incorrectly log a warning if the block
+		// that was just built caused votes to be applied such that the block
+		// was rejected or was accepted along with one of its children. This
+		// should be cleaned up to never produce an invalid warning.
+		if t.canIssueChildOn(blk.ID()) {
 			t.Ctx.Log.Verbo("successfully issued new block from the VM")
 		} else {
-			t.Ctx.Log.Warn("built block with unissued ancestors")
+			t.Ctx.Log.Warn("block that was just built is not extendable")
 		}
 	}
 	return nil
@@ -717,11 +717,11 @@ func (t *Transitive) issueFromByID(
 	nodeID ids.NodeID,
 	blkID ids.ID,
 	issuedMetric prometheus.Counter,
-) (bool, error) {
+) error {
 	blk, err := t.getBlock(ctx, blkID)
 	if err != nil {
 		t.sendRequest(ctx, nodeID, blkID, issuedMetric)
-		return false, nil
+		return nil
 	}
 	return t.issueFrom(ctx, nodeID, blk, issuedMetric)
 }
@@ -734,22 +734,21 @@ func (t *Transitive) issueFrom(
 	nodeID ids.NodeID,
 	blk snowman.Block,
 	issuedMetric prometheus.Counter,
-) (bool, error) {
+) error {
 	// issue [blk] and its ancestors to consensus.
 	blkID := blk.ID()
-	for !t.wasIssued(blk) {
-		if err := t.issue(ctx, nodeID, blk, false, issuedMetric); err != nil {
-			return false, err
+	for t.shouldIssueBlock(blk) {
+		err := t.issue(ctx, nodeID, blk, false, issuedMetric)
+		if err != nil {
+			return err
 		}
 
+		// If we don't have this ancestor, request it from [nodeID]
 		blkID = blk.Parent()
-		var err error
 		blk, err = t.getBlock(ctx, blkID)
-
-		// If we don't have this ancestor, request it from [vdr]
-		if err != nil || !blk.Status().Fetched() {
+		if err != nil {
 			t.sendRequest(ctx, nodeID, blkID, issuedMetric)
-			return false, nil
+			return nil
 		}
 	}
 
@@ -758,14 +757,11 @@ func (t *Transitive) issueFrom(
 		delete(t.blkReqSourceMetric, req)
 	}
 
-	if !t.isDecided(blk) && !t.Consensus.Processing(blkID) {
-		return false, nil
+	// If this block isn't pending, make sure nothing is blocked on it.
+	if _, isPending := t.pending[blkID]; !isPending {
+		return t.blocked.Abandon(ctx, blkID)
 	}
-
-	// A dependency should never be waiting on a decided or processing block.
-	// However, if the block was marked as rejected by the VM, the dependencies
-	// may still be waiting. Therefore, they should abandoned.
-	return true, t.blocked.Abandon(ctx, blkID)
+	return nil
 }
 
 // issueWithAncestors attempts to issue the branch ending with [blk] to consensus.
@@ -775,46 +771,30 @@ func (t *Transitive) issueWithAncestors(
 	ctx context.Context,
 	blk snowman.Block,
 	issuedMetric prometheus.Counter,
-) (bool, error) {
+) error {
 	blkID := blk.ID()
 	// issue [blk] and its ancestors into consensus
-	status := blk.Status()
-	for status.Fetched() && !t.wasIssued(blk) {
+	for t.shouldIssueBlock(blk) {
 		err := t.issue(ctx, t.Ctx.NodeID, blk, true, issuedMetric)
 		if err != nil {
-			return false, err
+			return err
 		}
 		blkID = blk.Parent()
 		blk, err = t.getBlock(ctx, blkID)
 		if err != nil {
-			status = choices.Unknown
 			break
 		}
-		status = blk.Status()
 	}
 
-	// The block was issued into consensus. This is the happy path.
-	if status != choices.Unknown && (t.isDecided(blk) || t.Consensus.Processing(blkID)) {
-		return true, nil
-	}
-
-	// There's an outstanding request for this block.
-	// We can just wait for that request to succeed or fail.
+	// There's an outstanding request for this block. We can wait for that
+	// request to succeed or fail.
 	if t.blkReqs.HasValue(blkID) {
-		return false, nil
+		return nil
 	}
 
-	// We don't have this block and have no reason to expect that we will get it.
-	// Abandon the block to avoid a memory leak.
-	return false, t.blocked.Abandon(ctx, blkID)
-}
-
-// If the block has been decided, then it is marked as having been issued.
-// If the block is processing, then it was issued.
-// If the block is queued to be added to consensus, then it was issued.
-func (t *Transitive) wasIssued(blk snowman.Block) bool {
-	blkID := blk.ID()
-	return t.isDecided(blk) || t.Consensus.Processing(blkID) || t.pendingContains(blkID)
+	// If the block wasn't already issued, we have no reason to expect that it
+	// will be able to be issued.
+	return t.blocked.Abandon(ctx, blkID)
 }
 
 // Issue [blk] to consensus once its ancestors have been issued.
@@ -846,12 +826,10 @@ func (t *Transitive) issue(
 		issuedMetric: issuedMetric,
 	}
 
-	// block on the parent if needed
-	var (
-		parentID = blk.Parent()
-		deps     []ids.ID
-	)
-	if parent, err := t.getBlock(ctx, parentID); err != nil || !(t.isDecided(parent) || t.Consensus.Processing(parentID)) {
+	// We know that shouldIssueBlock(blk) is true. This means that parent is
+	// either the last accepted block or is not decided.
+	var deps []ids.ID
+	if parentID := blk.Parent(); !t.canIssueChildOn(parentID) {
 		t.Ctx.Log.Verbo("block waiting for parent to be issued",
 			zap.Stringer("blkID", blkID),
 			zap.Stringer("parentID", parentID),
@@ -956,12 +934,10 @@ func (t *Transitive) deliver(
 ) error {
 	// we are no longer waiting on adding the block to consensus, so it is no
 	// longer pending
-	t.removeFromPending(blk)
+	blkID := blk.ID()
+	delete(t.pending, blkID)
 
-	var (
-		parentID = blk.Parent()
-		blkID    = blk.ID()
-	)
+	parentID := blk.Parent()
 	if !t.canIssueChildOn(parentID) || t.Consensus.Processing(blkID) {
 		// If the parent isn't processing or the last accepted block, then this
 		// block is effectively rejected.
@@ -1026,7 +1002,7 @@ func (t *Transitive) deliver(
 			t.sendQuery(ctx, blkID, blk.Bytes(), push)
 		}
 
-		t.removeFromPending(blk)
+		delete(t.pending, blkID)
 		if err := t.blocked.Fulfill(ctx, blkID); err != nil {
 			return err
 		}
@@ -1036,7 +1012,7 @@ func (t *Transitive) deliver(
 	}
 	for _, blk := range dropped {
 		blkID := blk.ID()
-		t.removeFromPending(blk)
+		delete(t.pending, blkID)
 		if err := t.blocked.Abandon(ctx, blkID); err != nil {
 			return err
 		}
@@ -1057,16 +1033,6 @@ func (t *Transitive) deliver(
 	return nil
 }
 
-// Returns true if the block whose ID is [blkID] is waiting to be issued to consensus
-func (t *Transitive) pendingContains(blkID ids.ID) bool {
-	_, ok := t.pending[blkID]
-	return ok
-}
-
-func (t *Transitive) removeFromPending(blk snowman.Block) {
-	delete(t.pending, blk.ID())
-}
-
 func (t *Transitive) addToNonVerifieds(blk snowman.Block) {
 	// don't add this blk if it's decided or processing.
 	blkID := blk.ID()
@@ -1176,6 +1142,37 @@ func (t *Transitive) getProcessingAncestor(ctx context.Context, initialVote ids.
 	}
 }
 
+// shouldIssueBlock returns true if the provided block should be enqueued for
+// issuance. If the block is already decided, already enqueued, or has already
+// been issued, this function will return false.
+func (t *Transitive) shouldIssueBlock(blk snowman.Block) bool {
+	height := blk.Height()
+	lastAcceptedID, lastAcceptedHeight := t.Consensus.LastAccepted()
+	if height <= lastAcceptedHeight {
+		return false // block is either accepted or rejected
+	}
+
+	// This is guaranteed not to underflow because the above check ensures
+	// [height] > 0.
+	parentHeight := height - 1
+	parentID := blk.Parent()
+	if parentHeight == lastAcceptedHeight && parentID != lastAcceptedID {
+		return false // the parent was rejected
+	}
+
+	blkID := blk.ID()
+	_, isPending := t.pending[blkID]
+	return !isPending && // If the block is already pending, don't issue it again.
+		!t.Consensus.Processing(blkID) // If the block was previously issued, don't issue it again.
+}
+
+// canDependOn reports true if it is guaranteed for the provided block ID to
+// eventually either be fulfilled or abandoned.
+func (t *Transitive) canDependOn(blkID ids.ID) bool {
+	_, isPending := t.pending[blkID]
+	return isPending || t.blkReqs.HasValue(blkID)
+}
+
 // canIssueChildOn reports true if it is valid for a child of parentID to be
 // verified and added to consensus.
 func (t *Transitive) canIssueChildOn(parentID ids.ID) bool {