fix: stop throwing away all unverified proposals upon hitting a

verification error

Author: evan-forbes

consensus/propagation/catchup.go

@@ -187,30 +187,31 @@ func (blockProp *Reactor) applyCachedProposalIfAvailable() {
 		blockProp.Logger.Info("applying cached proposal from catchup",
 			"height", currentHeight, "round", cb.Proposal.Round, "peer", peer.peer.ID())
 
-		blockProp.handleCachedCompactBlock(cb)
-
-		// Clean up the cache entry for this peer
-		peer.DeleteUnverifiedProposal(currentHeight)
-		return
+		if blockProp.handleCachedCompactBlock(cb) {
+			// Clean up the cache entry for this peer only if we successfully applied it.
+			peer.DeleteUnverifiedProposal(currentHeight)
+			return
+		}
 	}
 }
 
 // handleCachedCompactBlock processes a verified cached compact block.
 // Similar to handleCompactBlock but skips validation (already verified) and triggers immediate catchup.
-func (blockProp *Reactor) handleCachedCompactBlock(cb *proptypes.CompactBlock) {
+// Returns true if the cached block was applied.
+func (blockProp *Reactor) handleCachedCompactBlock(cb *proptypes.CompactBlock) bool {
 	blockProp.Logger.Info("applying cached compact block", "height", cb.Proposal.Height, "round", cb.Proposal.Round)
 
 	// generate (and cache) the proofs from the partset hashes in the compact block
 	_, err := cb.Proofs()
 	if err != nil {
 		blockProp.Logger.Error("cached compact block has invalid proofs", "err", err.Error())
-		return
+		return false
 	}
 
 	// Send proposal to consensus reactor
 	select {
 	case <-blockProp.ctx.Done():
-		return
+		return false
 	case blockProp.proposalChan <- ProposalAndSrc{
 		Proposal: cb.Proposal,
 		From:     blockProp.self, // From self since it's from cache
@@ -221,20 +222,27 @@ func (blockProp *Reactor) handleCachedCompactBlock(cb *proptypes.CompactBlock) {
 	added := blockProp.AddProposal(cb)
 	if !added {
 		blockProp.Logger.Debug("cached proposal already exists", "height", cb.Proposal.Height, "round", cb.Proposal.Round)
-		return
 	}
 
-	// Mark as catchup to skip parity requests in retryWants
+	propFound := false
 	blockProp.pmtx.Lock()
-	if prop := blockProp.proposals[cb.Proposal.Height][cb.Proposal.Round]; prop != nil {
-		prop.catchup = true
+	if props, ok := blockProp.proposals[cb.Proposal.Height]; ok {
+		if prop := props[cb.Proposal.Round]; prop != nil {
+			// Mark as catchup to skip parity requests in retryWants
+			prop.catchup = true
+			propFound = true
+		}
 	}
 	blockProp.pmtx.Unlock()
+	if !propFound {
+		return false
+	}
 
 	// Recover any parts from mempool
 	blockProp.recoverPartsFromMempool(cb)
 
 	// Immediately trigger part requests (like AddCommitment)
 	blockProp.ticker.Reset(RetryTime)
 	go blockProp.retryWants()
+	return true
 }

consensus/propagation/catchup_test.go

@@ -321,6 +321,54 @@ func TestApplyCachedProposalIfAvailable_MultiPeer(t *testing.T) {
 	require.Nil(t, peer2Cache, "valid peer's cache entry should be deleted after successful apply")
 }
 
+// TestApplyCachedProposalIfAvailable_KeepOnFailure ensures we don't delete a cached proposal
+// if handleCachedCompactBlock fails to apply it.
+func TestApplyCachedProposalIfAvailable_KeepOnFailure(t *testing.T) {
+	p2pCfg := defaultTestP2PConf()
+	nodes := 2
+	reactors, _ := createTestReactors(nodes, p2pCfg, false, "")
+	n1 := reactors[0] // Will have a proposal that fails in handleCachedCompactBlock
+	n2 := reactors[1] // The node applying cached proposals
+
+	cleanup, _, sm := state.SetupTestCase(t)
+	t.Cleanup(func() {
+		cleanup(t)
+	})
+
+	// n2 at height 1, round 0, with proposer set
+	n2.SetHeightAndRound(1, 0)
+	n2.SetProposer(mockPubKey)
+
+	// Create a compact block with valid signatures but invalid parts hashes.
+	cbBad, _, _, _ := testCompactBlock(t, sm, 2, 0)
+	badHash := make([]byte, len(cbBad.PartsHashes[0]))
+	copy(badHash, cbBad.PartsHashes[0])
+	badHash[0] ^= 0xFF
+	cbBad.PartsHashes[0] = badHash
+	cbBad.SetProofCache(nil)
+	signBytes, err := cbBad.SignBytes()
+	require.NoError(t, err)
+	sig, err := mockPrivVal.SignRawBytes(TestChainID, CompactBlockUID, signBytes)
+	require.NoError(t, err)
+	cbBad.Signature = sig
+
+	// n2 receives proposal for height 2 while still at height 1 - should cache it.
+	n2.handleCompactBlock(cbBad, n1.self, false)
+
+	peer1 := n2.getPeer(n1.self)
+	require.NotNil(t, peer1.GetUnverifiedProposal(2), "peer1 should have cached proposal")
+
+	// Now n2 catches up to height 2 - applyCachedProposalIfAvailable runs and should fail to apply cbBad.
+	n2.Prune(1)
+	n2.SetHeightAndRound(2, 0)
+
+	_, _, has := n2.GetProposal(2, 0)
+	require.False(t, has, "proposal should not be applied when proofs are invalid")
+
+	// Cached proposal should remain since handleCachedCompactBlock failed.
+	require.NotNil(t, peer1.GetUnverifiedProposal(2), "cached proposal should remain after failed apply")
+}
+
 // TestApplyCachedProposalIfAvailable_WrongRound tests that cached proposals for
 // a different round are kept cached until we advance to that round.
 func TestApplyCachedProposalIfAvailable_WrongRound(t *testing.T) {