From a4c3b5cab4185305cca4da6bccc920a015ffc792 Mon Sep 17 00:00:00 2001
From: Callum Waters <cmwaters19@gmail.com>
Date: Tue, 10 May 2022 17:34:53 +0200
Subject: [PATCH] validate block before we persist it (#8493)

---
 internal/blocksync/reactor.go | 58 +++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 26 deletions(-)

diff --git a/internal/blocksync/reactor.go b/internal/blocksync/reactor.go
index 0bf0561d3..bf9845370 100644
--- a/internal/blocksync/reactor.go
+++ b/internal/blocksync/reactor.go
@@ -517,8 +517,16 @@ func (r *Reactor) poolRoutine(ctx context.Context, stateSynced bool, blockSyncCh
 			// NOTE: We can probably make this more efficient, but note that calling
 			// first.Hash() doesn't verify the tx contents, so MakePartSet() is
 			// currently necessary.
-			if err = state.Validators.VerifyCommitLight(chainID, firstID, first.Height, second.LastCommit); err != nil {
-				err = fmt.Errorf("invalid last commit: %w", err)
+			err = state.Validators.VerifyCommitLight(chainID, firstID, first.Height, second.LastCommit)
+
+			if err == nil {
+				// validate the block before we persist it
+				err = r.blockExec.ValidateBlock(ctx, state, first)
+			}
+
+			// If either of the checks failed we log the error and request for a new block
+			// at that height
+			if err != nil {
 				r.logger.Error(
 					err.Error(),
 					"last_commit", second.LastCommit,
@@ -545,37 +553,35 @@ func (r *Reactor) poolRoutine(ctx context.Context, stateSynced bool, blockSyncCh
 						return
 					}
 				}
-			} else {
-				r.pool.PopRequest()
+				return
+			}
 
-				// TODO: batch saves so we do not persist to disk every block
-				r.store.SaveBlock(first, firstParts, second.LastCommit)
+			r.pool.PopRequest()
 
-				var err error
+			// TODO: batch saves so we do not persist to disk every block
+			r.store.SaveBlock(first, firstParts, second.LastCommit)
 
-				// TODO: Same thing for app - but we would need a way to get the hash
-				// without persisting the state.
-				state, err = r.blockExec.ApplyBlock(ctx, state, firstID, first)
-				if err != nil {
-					// TODO: This is bad, are we zombie?
-					panic(fmt.Sprintf("failed to process committed block (%d:%X): %v", first.Height, first.Hash(), err))
-				}
+			// TODO: Same thing for app - but we would need a way to get the hash
+			// without persisting the state.
+			state, err = r.blockExec.ApplyBlock(ctx, state, firstID, first)
+			if err != nil {
+				panic(fmt.Sprintf("failed to process committed block (%d:%X): %v", first.Height, first.Hash(), err))
+			}
 
-				r.metrics.RecordConsMetrics(first)
+			r.metrics.RecordConsMetrics(first)
 
-				blocksSynced++
+			blocksSynced++
 
-				if blocksSynced%100 == 0 {
-					lastRate = 0.9*lastRate + 0.1*(100/time.Since(lastHundred).Seconds())
-					r.logger.Info(
-						"block sync rate",
-						"height", r.pool.height,
-						"max_peer_height", r.pool.MaxPeerHeight(),
-						"blocks/s", lastRate,
-					)
+			if blocksSynced%100 == 0 {
+				lastRate = 0.9*lastRate + 0.1*(100/time.Since(lastHundred).Seconds())
+				r.logger.Info(
+					"block sync rate",
+					"height", r.pool.height,
+					"max_peer_height", r.pool.MaxPeerHeight(),
+					"blocks/s", lastRate,
+				)
 
-					lastHundred = time.Now()
-				}
+				lastHundred = time.Now()
 			}
 		}
 	}