Swap the wrong arguments in a call (#15639)

* Swap the wrong arguments in a call

I saw that the names of the passed arguments and the ones of the
function parameters don't match, so I suspect that it's a bug.

* Add changelog

* Add validation for the fillInForkChocieMissingBlocks checkpoints.

* Add test for checkpoint epoch validation in fillInForkChoiceMissingBlocks.

* Use a sentinel error rather than error string

---------

Co-authored-by: kasey <489222+kasey@users.noreply.github.com>
Co-authored-by: Preston Van Loon <preston@pvl.dev>

beacon-chain/blockchain/process_block.go

@@ -159,7 +159,7 @@ func (s *Service) onBlockBatch(ctx context.Context, blks []consensusblocks.ROBlo
 	}
 
 	// Fill in missing blocks
-	if err := s.fillInForkChoiceMissingBlocks(ctx, blks[0], preState.CurrentJustifiedCheckpoint(), preState.FinalizedCheckpoint()); err != nil {
+	if err := s.fillInForkChoiceMissingBlocks(ctx, blks[0], preState.FinalizedCheckpoint(), preState.CurrentJustifiedCheckpoint()); err != nil {
 		return errors.Wrap(err, "could not fill in missing blocks to forkchoice")
 	}
 

beacon-chain/blockchain/process_block_helpers.go

@@ -30,6 +30,10 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
+// ErrInvalidCheckpointArgs may be returned when the finalized checkpoint has an epoch greater than the justified checkpoint epoch.
+// If you are seeing this error, make sure you haven't mixed up the order of the arguments in the method you are calling.
+var ErrInvalidCheckpointArgs = errors.New("finalized checkpoint cannot be greater than justified checkpoint")
+
 // CurrentSlot returns the current slot based on time.
 func (s *Service) CurrentSlot() primitives.Slot {
 	return slots.CurrentSlot(s.genesisTime)
@@ -454,6 +458,9 @@ func (s *Service) ancestorByDB(ctx context.Context, r [32]byte, slot primitives.
 // This is useful for block tree visualizer and additional vote accounting.
 func (s *Service) fillInForkChoiceMissingBlocks(ctx context.Context, signed interfaces.ReadOnlySignedBeaconBlock,
 	fCheckpoint, jCheckpoint *ethpb.Checkpoint) error {
+	if fCheckpoint.Epoch > jCheckpoint.Epoch {
+		return ErrInvalidCheckpointArgs
+	}
 	pendingNodes := make([]*forkchoicetypes.BlockAndCheckpoints, 0)
 
 	// Fork choice only matters from last finalized slot.

beacon-chain/blockchain/process_block_test.go

@@ -375,6 +375,81 @@ func TestFillForkChoiceMissingBlocks_FinalizedSibling(t *testing.T) {
 	require.Equal(t, ErrNotDescendantOfFinalized.Error(), err.Error())
 }
 
+func TestFillForkChoiceMissingBlocks_ErrorCases(t *testing.T) {
+	tests := []struct {
+		name           string
+		finalizedEpoch primitives.Epoch
+		justifiedEpoch primitives.Epoch
+		expectedError  error
+	}{
+		{
+			name:           "finalized epoch greater than justified epoch",
+			finalizedEpoch: 5,
+			justifiedEpoch: 3,
+			expectedError:  ErrInvalidCheckpointArgs,
+		},
+		{
+			name:           "valid case - finalized equal to justified",
+			finalizedEpoch: 3,
+			justifiedEpoch: 3,
+			expectedError:  nil,
+		},
+		{
+			name:           "valid case - finalized less than justified",
+			finalizedEpoch: 2,
+			justifiedEpoch: 3,
+			expectedError:  nil,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			service, tr := minimalTestService(t)
+			ctx, beaconDB := tr.ctx, tr.db
+
+			st, _ := util.DeterministicGenesisState(t, 64)
+			require.NoError(t, service.saveGenesisData(ctx, st))
+
+			// Create a simple block for testing
+			blk := util.NewBeaconBlock()
+			blk.Block.Slot = 10
+			blk.Block.ParentRoot = service.originBlockRoot[:]
+			wsb, err := consensusblocks.NewSignedBeaconBlock(blk)
+			require.NoError(t, err)
+			util.SaveBlock(t, ctx, beaconDB, blk)
+
+			// Create checkpoints with test case epochs
+			finalizedCheckpoint := &ethpb.Checkpoint{
+				Epoch: tt.finalizedEpoch,
+				Root:  service.originBlockRoot[:],
+			}
+			justifiedCheckpoint := &ethpb.Checkpoint{
+				Epoch: tt.justifiedEpoch,
+				Root:  service.originBlockRoot[:],
+			}
+
+			// Set up forkchoice store to avoid other errors
+			fcp := &ethpb.Checkpoint{Epoch: 0, Root: service.originBlockRoot[:]}
+			state, blkRoot, err := prepareForkchoiceState(ctx, 0, service.originBlockRoot, service.originBlockRoot, [32]byte{}, fcp, fcp)
+			require.NoError(t, err)
+			require.NoError(t, service.cfg.ForkChoiceStore.InsertNode(ctx, state, blkRoot))
+
+			err = service.fillInForkChoiceMissingBlocks(
+				t.Context(), wsb, finalizedCheckpoint, justifiedCheckpoint)
+
+			if tt.expectedError != nil {
+				require.ErrorIs(t, err, tt.expectedError)
+			} else {
+				// For valid cases, we might get other errors (like block not being descendant of finalized)
+				// but we shouldn't get the checkpoint validation error
+				if err != nil && errors.Is(err, tt.expectedError) {
+					t.Errorf("Unexpected checkpoint validation error: %v", err)
+				}
+			}
+		})
+	}
+}
+
 // blockTree1 constructs the following tree:
 //
 //	/- B1