chain: carry finalized heights through the plan, so recovery names them right

The recovery index records height→id for every finalized block, so a behind node can
name a gap the ledger's own window has pruned. But the engine's accept loop derived each
block's height from pendingBlocks — and a gap block is by definition no longer tracked
there, so its height read as 0. The index then filed every gap block under height 0 and
answered for none of them: the fix populated nothing it could read.

The fold already knows each step's height. Plan now carries AcceptHeights alongside
Accept, and the accept loop reads the height from there — authoritative for a tracked or
an untracked block alike. The recovery index records the true height.

Co-authored-by: Hanzo Dev <[email protected]>

engine/chain/catchup_settled_test.go

@@ -326,3 +326,45 @@ func TestSettledHeight_LowersToGenuineZero(t *testing.T) {
 		t.Fatalf("settledHeight = (%d,%v), want (0,true) — a genuine-zero VM under a ledger is a wedge the floor must expose", got, set)
 	}
 }
+
+// TestRecoveryIndex_RecordsTheRealHeightThroughTheFold guards the population half of the
+// recovery index (red pass-4 Q2): a block finalized through the real cert fold must land
+// in recoveredAt at its TRUE height, not 0. The height a gap block carries into the accept
+// loop comes from the Plan (AcceptHeights), because a gap block is no longer tracked and
+// deriving its height from pendingBlocks yields 0 — which would file it under the wrong
+// height and make the whole index inert for exactly the blocks it exists to serve.
+func TestRecoveryIndex_RecordsTheRealHeightThroughTheFold(t *testing.T) {
+	vs := newTestValidatorSet(5)
+	vm := newCatchupVM()
+	rt, chainID, _ := newCatchupRuntime(t, vs, 0, vm)
+
+	const N = uint64(500_000)
+	const k = 20
+	tip := newTestBlock(N, ids.Empty, "tip@N")
+	seedBehindAt(t, rt, vm, tip)
+	gap := buildGap(vm, tip, k)
+
+	// Finalize the gap through the real cert path — applyBranchFinalization runs, and its
+	// accept loop is what records recovery.
+	for _, blk := range gap {
+		cert := catchupCertFor(t, vs, chainID, blk, []int{0, 1, 2, 3}, 3)
+		if err := rt.AcceptCatchupBlock(context.Background(), blk.bytes, cert); err != nil {
+			t.Fatalf("finalize height %d: %v", blk.height, err)
+		}
+	}
+
+	// Every finalized height must be in the recovery index under its OWN height, mapping
+	// to its OWN id — never height 0.
+	if id, ok := rt.Transitive.recoveredOuterAt(0); ok {
+		t.Fatalf("recovery index has a bogus height-0 entry (%s) — a gap block was filed under 0", id)
+	}
+	for _, blk := range gap {
+		id, ok := rt.Transitive.recoveredOuterAt(blk.height)
+		if !ok {
+			t.Fatalf("recovery index missing height %d entirely", blk.height)
+		}
+		if id != blk.id {
+			t.Fatalf("recovery index height %d → %s, want %s", blk.height, id, blk.id)
+		}
+	}
+}

engine/chain/engine.go

@@ -4284,22 +4284,22 @@ func (t *Transitive) applyBranchFinalization(ctx context.Context, plan Plan, cer
 	}
 	t.mu.RLock()
 	path := make([]pathBlock, 0, len(plan.Accept))
-	for _, id := range plan.Accept {
+	for i, id := range plan.Accept {
+		// The height comes from the PLAN (the fold knows it for every step), not from
+		// pendingBlocks — a gap block is by definition no longer tracked, so deriving the
+		// height from pendingBlocks yields 0 and the recovery index would record it at the
+		// wrong height. AcceptHeights is populated in lockstep with Accept.
+		var h uint64
+		if i < len(plan.AcceptHeights) {
+			h = plan.AcceptHeights[i]
+		}
 		pending, ok := t.pendingBlocks[id]
 		if !ok || pending.Decided {
 			// Untracked or already applied: treat as accepted for floor purposes (the ledger
 			// is the truth) but nothing to Accept on the VM.
-			var h uint64
-			if ok && pending.ConsensusBlock != nil {
-				h = pending.ConsensusBlock.height
-			}
 			path = append(path, pathBlock{id: id, vmb: nil, height: h})
 			continue
 		}
-		var h uint64
-		if pending.ConsensusBlock != nil {
-			h = pending.ConsensusBlock.height
-		}
 		path = append(path, pathBlock{id: id, vmb: pending.VMBlock, height: h})
 	}
 	t.mu.RUnlock()

engine/chain/ledger.go

@@ -245,7 +245,12 @@ type Cert struct {
 //     all its descendants — rejectTransitively.
 type Plan struct {
 	Accept []ids.ID
-	Reject []ids.ID
+	// AcceptHeights is the height of each Accept entry, in the same order. The fold knows
+	// every step's height; the engine's accept loop otherwise recovers it from
+	// pendingBlocks and gets 0 for a block that is no longer tracked — which is exactly a
+	// gap block. Carrying it here lets the recovery index record the true height.
+	AcceptHeights []uint64
+	Reject        []ids.ID
 }
 
 // Finalize is THE finality function: a pure fold of a Cert into the ledger. No
@@ -294,7 +299,7 @@ func Finalize(led FinalityLedger, cert Cert, dag Ancestry) (FinalityLedger, Plan
 	// superseded here: a cert at the hint's height, even for a different canonical id
 	// than the hint guessed, finalizes cleanly (the hint never blocks a cert).
 	if !led.set {
-		return seedLedger(cert.Block, canonical, cert.Height), Plan{Accept: []ids.ID{cert.Block}}, nil
+		return seedLedger(cert.Block, canonical, cert.Height), Plan{Accept: []ids.ID{cert.Block}, AcceptHeights: []uint64{cert.Height}}, nil
 	}
 
 	// Below/at the frontier with no record for this height → stale or non-monotonic.
@@ -323,6 +328,7 @@ func Finalize(led FinalityLedger, cert Cert, dag Ancestry) (FinalityLedger, Plan
 		next.canonical = s.canonical
 		next.height = s.height
 		plan.Accept = append(plan.Accept, s.id)
+		plan.AcceptHeights = append(plan.AcceptHeights, s.height)
 	}
 	next.pruneBelowWindow() // keep byHeight (and the next clone) O(window), not O(chain height)
 	return next, plan, nil

engine/chain/ledger_test.go

@@ -58,7 +58,9 @@ func (d mapDAG) Children(id ids.ID) []ids.ID {
 	return out
 }
 
-func (d mapDAG) add(id, parent ids.ID, height uint64) { d[id] = dagNode{parent: parent, height: height} }
+func (d mapDAG) add(id, parent ids.ID, height uint64) {
+	d[id] = dagNode{parent: parent, height: height}
+}
 
 // seedFold finalizes a genesis at height 0 through the public fold (not the
 // seedLedger helper) so every test exercises Finalize end to end.
@@ -391,3 +393,34 @@ func BenchmarkFinalize_FlatCost(b *testing.B) {
 		})
 	}
 }
+
+// TestLedger_PlanCarriesAcceptHeights: the fold's plan reports the height of every
+// Accept entry, in the same order. The engine's accept loop reads these to record the
+// recovery index at the true height — a gap block is no longer tracked, so deriving its
+// height from pendingBlocks yields 0 and files it under the wrong height. This is the
+// authoritative source that makes the recovery index name the right heights.
+func TestLedger_PlanCarriesAcceptHeights(t *testing.T) {
+	g := ids.GenerateTestID()
+	a1 := ids.GenerateTestID()
+	a2 := ids.GenerateTestID()
+	a3 := ids.GenerateTestID()
+	dag := mapDAG{}
+	dag.add(a1, g, 1)
+	dag.add(a2, a1, 2)
+	dag.add(a3, a2, 3)
+
+	led := seedFold(t, g, dag)
+	_, plan, err := Finalize(led, Cert{Block: a3, Parent: a2, Height: 3}, dag)
+	if err != nil {
+		t.Fatalf("finalize A3: %v", err)
+	}
+	if len(plan.AcceptHeights) != len(plan.Accept) {
+		t.Fatalf("AcceptHeights len %d != Accept len %d — heights lost", len(plan.AcceptHeights), len(plan.Accept))
+	}
+	want := []uint64{1, 2, 3}
+	for i, h := range plan.AcceptHeights {
+		if h != want[i] {
+			t.Fatalf("AcceptHeights[%d]=%d, want %d (id %s)", i, h, want[i], plan.Accept[i])
+		}
+	}
+}