Applied comments from PR and minor cleanup

internal/blocksync/pool.go

@@ -238,7 +238,7 @@ func (pool *BlockPool) PopRequest() {
 		// to smooth the block sync rate and the unit represents the number of blocks per second.
 		// -1 because the start height is assumed to be 1  @jmalicevic ToDo, verify it is still OK when
 		// starting height is not 1
-		if (pool.height-pool.startHeight-1)%100 == 0 && pool.height != pool.startHeight {
+		if (pool.height-pool.startHeight+1)%100 == 0 {
 			newSyncRate := 100 / time.Since(pool.lastHundredBlockTimeStamp).Seconds()
 			if pool.lastSyncRate == 0 {
 				pool.lastSyncRate = newSyncRate

internal/blocksync/reactor.go

@@ -110,16 +110,17 @@ func NewReactor(
 	eventBus *eventbus.EventBus,
 ) *Reactor {
 	r := &Reactor{
-		logger:      logger,
-		stateStore:  stateStore,
-		blockExec:   blockExec,
-		store:       *store,
-		consReactor: consReactor,
-		blockSync:   newAtomicBool(blockSync),
-		chCreator:   channelCreator,
-		peerEvents:  peerEvents,
-		metrics:     metrics,
-		eventBus:    eventBus,
+		logger:           logger,
+		stateStore:       stateStore,
+		blockExec:        blockExec,
+		store:            *store,
+		consReactor:      consReactor,
+		blockSync:        newAtomicBool(blockSync),
+		chCreator:        channelCreator,
+		peerEvents:       peerEvents,
+		metrics:          metrics,
+		eventBus:         eventBus,
+		lastTrustedBlock: &TrustedBlockData{},
 	}
 
 	r.BaseService = *service.NewBaseService(logger, "BlockSync", r)
@@ -545,10 +546,10 @@ func (r *Reactor) poolRoutine(ctx context.Context, stateSynced bool, blockSyncCh
 			)
 
 			// TODO(sergio, jmalicevic): Should we also validate against the extended commit?
-			if r.lastTrustedBlock == nil || r.lastTrustedBlock.block == nil {
-				if r.initialState.LastBlockHeight != 0 {
-					r.lastTrustedBlock = &TrustedBlockData{r.store.LoadBlock(r.initialState.LastBlockHeight), r.store.LoadSeenCommit()}
-					if r.lastTrustedBlock == nil {
+			if r.lastTrustedBlock.block == nil {
+				if state.LastBlockHeight != 0 {
+					r.lastTrustedBlock = &TrustedBlockData{r.store.LoadBlock(state.LastBlockHeight), r.store.LoadSeenCommit()}
+					if r.lastTrustedBlock.block == nil {
 						panic("Failed to load last trusted block")
 					}
 				}
@@ -556,7 +557,7 @@ func (r *Reactor) poolRoutine(ctx context.Context, stateSynced bool, blockSyncCh
 			var err error
 			// Check for nil on block to avoid the case where we failed the block validation before persistence
 			// but passed the verification before and created a lastTrustedBlock object
-			if r.lastTrustedBlock != nil && r.lastTrustedBlock.block != nil {
+			if r.lastTrustedBlock.block != nil {
 				err = VerifyNextBlock(newBlock, newBlockID, verifyBlock, r.lastTrustedBlock.block, r.lastTrustedBlock.commit, state.Validators)
 			} else {
 				oldHash := r.initialState.Validators.Hash()
@@ -566,7 +567,7 @@ func (r *Reactor) poolRoutine(ctx context.Context, stateSynced bool, blockSyncCh
 						"new hash ", newBlock.ValidatorsHash,
 					)
 
-					peerID := r.pool.RedoRequest(r.lastTrustedBlock.block.Height + 1)
+					peerID := r.pool.RedoRequest(state.LastBlockHeight)
 					if serr := blockSyncCh.SendError(ctx, p2p.PeerError{
 						NodeID: peerID,
 						Err:    ErrValidationFailed{},
@@ -580,11 +581,11 @@ func (r *Reactor) poolRoutine(ctx context.Context, stateSynced bool, blockSyncCh
 				// If we ran state sync before blocksync, we can trust the state we have in the store and we will not end up here
 				// If we ran block sync or consensus, we will be able to load the last block from our store
 				// We use the validator set provided in the gensis vile, to verify the new block, using the lastCommit of verifyBlock
-				if r.initialState.LastBlockHeight != 0 {
+				if state.LastBlockHeight != 0 {
 					panic("Cannot be here unles we start from genesis")
 				}
 				err = state.Validators.VerifyCommitLight(newBlock.ChainID, newBlockID, newBlock.Height, verifyBlock.LastCommit)
-				r.lastTrustedBlock = &TrustedBlockData{}
+
 			}
 			if err != nil {
 

internal/blocksync/reactor_test.go

@@ -424,9 +424,11 @@ func TestReactor_NonGenesisSync(t *testing.T) {
 
 	valSet, privVals := factory.ValidatorSet(ctx, t, 4, 30)
 	genDoc := factory.GenesisDoc(cfg, time.Now(), valSet.Validators, factory.ConsensusParams())
-	maxBlockHeight := int64(101)
+	// Has to be 100 + the max starting height because we calculate the sync rate based on every 100 blocks processed
+	// Otherwise the test will time out
+	maxBlockHeight := int64(151)
 
-	rts := setup(ctx, t, genDoc, privVals, []int64{maxBlockHeight, 2}) // 2, 0}) //50, 4, 0})
+	rts := setup(ctx, t, genDoc, privVals, []int64{maxBlockHeight, 2, 50, 0})
 	require.Equal(t, maxBlockHeight, rts.reactors[rts.nodes[0]].store.Height())
 	rts.start(ctx, t)
 
@@ -439,7 +441,7 @@ func TestReactor_NonGenesisSync(t *testing.T) {
 					continue
 				}
 				matching = matching && rts.reactors[rts.nodes[idx]].GetRemainingSyncTime() > time.Nanosecond &&
-					rts.reactors[rts.nodes[idx]].pool.getLastSyncRate() > 0.001
+					rts.reactors[rts.nodes[idx]].pool.getLastSyncRate() > 0.0001
 
 				if !matching {
 					height, _, _ := rts.reactors[rts.nodes[idx]].pool.GetStatus()