Do not send checkpoint if node is syncing (#1982)

* Fix * Comments fix
0xPolygon · Oct 12, 2023 · c27d222 · c27d222
1 parent 83e2a53
commit c27d222
Show file tree

Hide file tree

Showing 2 changed files with 107 additions and 71 deletions.
diff --git a/consensus/polybft/checkpoint_manager.go b/consensus/polybft/checkpoint_manager.go
@@ -126,6 +126,12 @@ func (c *checkpointManager) submitCheckpoint(latestHeader *types.Header, isEndOf
 		return err
 	}
 
+	if lastCheckpointBlockNumber > latestHeader.Number {
+		// node is out of sync (haven't reached the tip of the chain), so even though it is a proposer,
+		// it would checkpoint block that is already checkpointed and transaction would fail anyway
+		return nil
+	}
+
 	c.logger.Debug("submitCheckpoint invoked...",
 		"latest checkpoint block", lastCheckpointBlockNumber,
 		"checkpoint block", latestHeader.Number)

diff --git a/consensus/polybft/checkpoint_manager_test.go b/consensus/polybft/checkpoint_manager_test.go
@@ -35,87 +35,117 @@ func TestCheckpointManager_SubmitCheckpoint(t *testing.T) {
 		epochSize   = 2
 	)
 
-	var aliases = []string{"A", "B", "C", "D", "E"}
-
-	validators := validator.NewTestValidatorsWithAliases(t, aliases)
-	validatorsMetadata := validators.GetPublicIdentities()
-	txRelayerMock := newDummyTxRelayer(t)
-	txRelayerMock.On("Call", mock.Anything, mock.Anything, mock.Anything).
-		Return("2", error(nil)).
-		Once()
-	txRelayerMock.On("SendTransaction", mock.Anything, mock.Anything).
-		Return(&ethgo.Receipt{Status: uint64(types.ReceiptSuccess)}, error(nil)).
-		Times(4) // send transactions for checkpoint blocks: 4, 6, 8 (pending checkpoint blocks) and 10 (latest checkpoint block)
-
-	backendMock := new(polybftBackendMock)
-	backendMock.On("GetValidators", mock.Anything, mock.Anything).Return(validatorsMetadata)
-
-	var (
-		headersMap  = &testHeadersMap{}
-		epochNumber = uint64(1)
-		dummyMsg    = []byte("checkpoint")
-		idx         = uint64(0)
-		header      *types.Header
-		bitmap      bitmap.Bitmap
-		signatures  bls.Signatures
-	)
+	t.Run("submit checkpoint happy path", func(t *testing.T) {
+		t.Parallel()
 
-	validators.IterAcct(aliases, func(t *validator.TestValidator) {
-		bitmap.Set(idx)
-		signatures = append(signatures, t.MustSign(dummyMsg, bls.DomainCheckpointManager))
-		idx++
-	})
+		var aliases = []string{"A", "B", "C", "D", "E"}
+
+		validators := validator.NewTestValidatorsWithAliases(t, aliases)
+		validatorsMetadata := validators.GetPublicIdentities()
+
+		txRelayerMock := newDummyTxRelayer(t)
+		txRelayerMock.On("Call", mock.Anything, mock.Anything, mock.Anything).
+			Return("2", error(nil)).
+			Once()
+		txRelayerMock.On("SendTransaction", mock.Anything, mock.Anything).
+			Return(&ethgo.Receipt{Status: uint64(types.ReceiptSuccess)}, error(nil)).
+			Times(4) // send transactions for checkpoint blocks: 4, 6, 8 (pending checkpoint blocks) and 10 (latest checkpoint block)
+
+		backendMock := new(polybftBackendMock)
+		backendMock.On("GetValidators", mock.Anything, mock.Anything).Return(validatorsMetadata)
+
+		var (
+			headersMap  = &testHeadersMap{}
+			epochNumber = uint64(1)
+			dummyMsg    = []byte("checkpoint")
+			idx         = uint64(0)
+			header      *types.Header
+			bitmap      bitmap.Bitmap
+			signatures  bls.Signatures
+		)
+
+		validators.IterAcct(aliases, func(t *validator.TestValidator) {
+			bitmap.Set(idx)
+			signatures = append(signatures, t.MustSign(dummyMsg, bls.DomainCheckpointManager))
+			idx++
+		})
 
-	signature, err := signatures.Aggregate().Marshal()
-	require.NoError(t, err)
+		signature, err := signatures.Aggregate().Marshal()
+		require.NoError(t, err)
 
-	for i := uint64(1); i <= blocksCount; i++ {
-		if i%epochSize == 1 {
-			// epoch-beginning block
-			checkpoint := &CheckpointData{
-				BlockRound:  0,
-				EpochNumber: epochNumber,
-				EventRoot:   types.BytesToHash(generateRandomBytes(t)),
-			}
-			extra := createTestExtraObject(validatorsMetadata, validatorsMetadata, 3, 3, 3)
-			extra.Checkpoint = checkpoint
-			extra.Committed = &Signature{Bitmap: bitmap, AggregatedSignature: signature}
-			header = &types.Header{
-				ExtraData: extra.MarshalRLPTo(nil),
+		for i := uint64(1); i <= blocksCount; i++ {
+			if i%epochSize == 1 {
+				// epoch-beginning block
+				checkpoint := &CheckpointData{
+					BlockRound:  0,
+					EpochNumber: epochNumber,
+					EventRoot:   types.BytesToHash(generateRandomBytes(t)),
+				}
+				extra := createTestExtraObject(validatorsMetadata, validatorsMetadata, 3, 3, 3)
+				extra.Checkpoint = checkpoint
+				extra.Committed = &Signature{Bitmap: bitmap, AggregatedSignature: signature}
+				header = &types.Header{
+					ExtraData: extra.MarshalRLPTo(nil),
+				}
+				epochNumber++
+			} else {
+				header = header.Copy()
 			}
-			epochNumber++
-		} else {
-			header = header.Copy()
+
+			header.Number = i
+			header.ComputeHash()
+			headersMap.addHeader(header)
 		}
 
-		header.Number = i
-		header.ComputeHash()
-		headersMap.addHeader(header)
-	}
+		// mock blockchain
+		blockchainMock := new(blockchainMock)
+		blockchainMock.On("GetHeaderByNumber", mock.Anything).Return(headersMap.getHeader)
+
+		validatorAcc := validators.GetValidator("A")
+		c := &checkpointManager{
+			key:              wallet.NewEcdsaSigner(validatorAcc.Key()),
+			rootChainRelayer: txRelayerMock,
+			consensusBackend: backendMock,
+			blockchain:       blockchainMock,
+			logger:           hclog.NewNullLogger(),
+		}
 
-	// mock blockchain
-	blockchainMock := new(blockchainMock)
-	blockchainMock.On("GetHeaderByNumber", mock.Anything).Return(headersMap.getHeader)
+		err = c.submitCheckpoint(headersMap.getHeader(blocksCount), false)
+		require.NoError(t, err)
+		txRelayerMock.AssertExpectations(t)
 
-	validatorAcc := validators.GetValidator("A")
-	c := &checkpointManager{
-		key:              wallet.NewEcdsaSigner(validatorAcc.Key()),
-		rootChainRelayer: txRelayerMock,
-		consensusBackend: backendMock,
-		blockchain:       blockchainMock,
-		logger:           hclog.NewNullLogger(),
-	}
+		// make sure that expected blocks are checkpointed (epoch-ending ones)
+		for _, checkpointBlock := range txRelayerMock.checkpointBlocks {
+			header := headersMap.getHeader(checkpointBlock)
+			require.NotNil(t, header)
+			require.True(t, isEndOfPeriod(header.Number, epochSize))
+		}
+	})
 
-	err = c.submitCheckpoint(headersMap.getHeader(blocksCount), false)
-	require.NoError(t, err)
-	txRelayerMock.AssertExpectations(t)
+	t.Run("checkpoint not submitted when node is syncing", func(t *testing.T) {
+		t.Parallel()
 
-	// make sure that expected blocks are checkpointed (epoch-ending ones)
-	for _, checkpointBlock := range txRelayerMock.checkpointBlocks {
-		header := headersMap.getHeader(checkpointBlock)
-		require.NotNil(t, header)
-		require.True(t, isEndOfPeriod(header.Number, epochSize))
-	}
+		var aliases = []string{"A"}
+
+		validators := validator.NewTestValidatorsWithAliases(t, aliases)
+
+		txRelayerMock := newDummyTxRelayer(t)
+		txRelayerMock.On("Call", mock.Anything, mock.Anything, mock.Anything).
+			Return("20", error(nil)).
+			Once()
+
+		c := &checkpointManager{
+			key:              wallet.NewEcdsaSigner(validators.GetValidator("A").Key()),
+			rootChainRelayer: txRelayerMock,
+			logger:           hclog.NewNullLogger(),
+		}
+
+		err := c.submitCheckpoint(&types.Header{Number: 19 /*lower than what Call returns*/}, false)
+		require.NoError(t, err)
+		// since transaction will not be sent to CheckpointManager, we only expect that Call
+		// will called on tx relayer, and nothing else in submitCheckpoint will be executed
+		txRelayerMock.AssertExpectations(t)
+	})
 }
 
 func TestCheckpointManager_abiEncodeCheckpointBlock(t *testing.T) {