eth, consensus: refactor whitelisting related logs and improve error handling (#1268)

* eth, consensus: refactor whitelisting related logs and error handling

* core: fix lint

* eth: fix tests, check against root error

* eth: use ctx correctly while logging

* eth: refactor comments and error message

consensus/bor/heimdall.go

@@ -17,8 +17,8 @@ type IHeimdallClient interface {
 	FetchCheckpointCount(ctx context.Context) (int64, error)
 	FetchMilestone(ctx context.Context) (*milestone.Milestone, error)
 	FetchMilestoneCount(ctx context.Context) (int64, error)
-	FetchNoAckMilestone(ctx context.Context, milestoneID string) error //Fetch the bool value whether milestone corresponding to the given id failed in the Heimdall
+	FetchNoAckMilestone(ctx context.Context, milestoneID string) error // Fetch the bool value whether milestone corresponding to the given id failed in the Heimdall
-	FetchLastNoAckMilestone(ctx context.Context) (string, error)       //Fetch latest failed milestone id
+	FetchLastNoAckMilestone(ctx context.Context) (string, error)       // Fetch latest failed milestone id
-	FetchMilestoneID(ctx context.Context, milestoneID string) error    //Fetch the bool value whether milestone corresponding to the given id is in process in Heimdall
+	FetchMilestoneID(ctx context.Context, milestoneID string) error    // Fetch the bool value whether milestone corresponding to the given id is in process in Heimdall
 	Close()
 }

consensus/bor/heimdallapp/milestone.go

@@ -14,30 +14,31 @@ import (
 )
 
 func (h *HeimdallAppClient) FetchMilestoneCount(_ context.Context) (int64, error) {
-	log.Info("Fetching milestone count")
+	log.Debug("Fetching milestone count")
 
 	res := h.hApp.CheckpointKeeper.GetMilestoneCount(h.NewContext())
 
-	log.Info("Fetched Milestone Count")
+	log.Debug("Fetched Milestone Count", "res", int64(res))
 
 	return int64(res), nil
 }
 
 func (h *HeimdallAppClient) FetchMilestone(_ context.Context) (*milestone.Milestone, error) {
-	log.Info("Fetching Latest Milestone")
+	log.Debug("Fetching Latest Milestone")
 
 	res, err := h.hApp.CheckpointKeeper.GetLastMilestone(h.NewContext())
 	if err != nil {
 		return nil, err
 	}
 
-	log.Info("Fetched Latest Milestone")
+	milestone := toBorMilestone(res)
+	log.Debug("Fetched Latest Milestone", "milestone", milestone)
 
-	return toBorMilestone(res), nil
+	return milestone, nil
 }
 
 func (h *HeimdallAppClient) FetchNoAckMilestone(_ context.Context, milestoneID string) error {
-	log.Info("Fetching No Ack Milestone By MilestoneID", "MilestoneID", milestoneID)
+	log.Debug("Fetching No Ack Milestone By MilestoneID", "MilestoneID", milestoneID)
 
 	res := h.hApp.CheckpointKeeper.GetNoAckMilestone(h.NewContext(), milestoneID)
 	if res {
@@ -45,21 +46,21 @@ func (h *HeimdallAppClient) FetchNoAckMilestone(_ context.Context, milestoneID s
 		return nil
 	}
 
-	return fmt.Errorf("Still No Ack Milestone exist corresponding to MilestoneId:%v", milestoneID)
+	return fmt.Errorf("still no-ack milestone exist corresponding to milestoneID: %v", milestoneID)
 }
 
 func (h *HeimdallAppClient) FetchLastNoAckMilestone(_ context.Context) (string, error) {
-	log.Info("Fetching Latest No Ack Milestone ID")
+	log.Debug("Fetching Latest No Ack Milestone ID")
 
 	res := h.hApp.CheckpointKeeper.GetLastNoAckMilestone(h.NewContext())
 
-	log.Info("Fetched Latest No Ack Milestone ID")
+	log.Debug("Fetched Latest No Ack Milestone ID", "res", res)
 
 	return res, nil
 }
 
 func (h *HeimdallAppClient) FetchMilestoneID(_ context.Context, milestoneID string) error {
-	log.Info("Fetching Milestone ID ", "MilestoneID", milestoneID)
+	log.Debug("Fetching Milestone ID ", "MilestoneID", milestoneID)
 
 	res := chTypes.GetMilestoneID()
 
@@ -67,7 +68,7 @@ func (h *HeimdallAppClient) FetchMilestoneID(_ context.Context, milestoneID stri
 		return nil
 	}
 
-	return fmt.Errorf("Milestone corresponding to Milestone ID:%v doesn't exist in Heimdall", milestoneID)
+	return fmt.Errorf("milestone corresponding to milestoneID: %v doesn't exist in heimdall", milestoneID)
 }
 
 func toBorMilestone(hdMilestone *hmTypes.Milestone) *milestone.Milestone {

consensus/bor/heimdallgrpc/milestone.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"math/big"
 
+	"github.com/ethereum/go-ethereum/consensus/bor/heimdall"
 	"github.com/ethereum/go-ethereum/consensus/bor/heimdall/milestone"
 	"github.com/ethereum/go-ethereum/log"
 
@@ -48,14 +49,14 @@ func (h *HeimdallGRPCClient) FetchMilestone(ctx context.Context) (*milestone.Mil
 }
 
 func (h *HeimdallGRPCClient) FetchLastNoAckMilestone(ctx context.Context) (string, error) {
-	log.Info("Fetching latest no ack milestone Id")
+	log.Debug("Fetching latest no ack milestone Id")
 
 	res, err := h.client.FetchLastNoAckMilestone(ctx, nil)
 	if err != nil {
 		return "", err
 	}
 
-	log.Info("Fetched last no-ack milestone")
+	log.Debug("Fetched last no-ack milestone", "res", res.Result.Result)
 
 	return res.Result.Result, nil
 }
@@ -65,7 +66,7 @@ func (h *HeimdallGRPCClient) FetchNoAckMilestone(ctx context.Context, milestoneI
 		MilestoneID: milestoneID,
 	}
 
-	log.Info("Fetching no ack milestone", "milestoneaID", milestoneID)
+	log.Debug("Fetching no ack milestone", "milestoneID", milestoneID)
 
 	res, err := h.client.FetchNoAckMilestone(ctx, req)
 	if err != nil {
@@ -73,10 +74,10 @@ func (h *HeimdallGRPCClient) FetchNoAckMilestone(ctx context.Context, milestoneI
 	}
 
 	if !res.Result.Result {
-		return fmt.Errorf("Not in rejected list: milestoneID %q", milestoneID)
+		return fmt.Errorf("%w: milestoneID %q", heimdall.ErrNotInRejectedList, milestoneID)
 	}
 
-	log.Info("Fetched no ack milestone", "milestoneaID", milestoneID)
+	log.Debug("Fetched no ack milestone", "milestoneID", milestoneID)
 
 	return nil
 }
@@ -86,7 +87,7 @@ func (h *HeimdallGRPCClient) FetchMilestoneID(ctx context.Context, milestoneID s
 		MilestoneID: milestoneID,
 	}
 
-	log.Info("Fetching milestone id", "milestoneID", milestoneID)
+	log.Debug("Fetching milestone id", "milestoneID", milestoneID)
 
 	res, err := h.client.FetchMilestoneID(ctx, req)
 	if err != nil {
@@ -94,10 +95,10 @@ func (h *HeimdallGRPCClient) FetchMilestoneID(ctx context.Context, milestoneID s
 	}
 
 	if !res.Result.Result {
-		return fmt.Errorf("This milestoneID %q does not exist", milestoneID)
+		return fmt.Errorf("%w: milestoneID %q", heimdall.ErrNotInMilestoneList, milestoneID)
 	}
 
-	log.Info("Fetched milestone id", "milestoneID", milestoneID)
+	log.Debug("Fetched milestone id", "milestoneID", milestoneID)
 
 	return nil
 }

core/blockchain_test.go

@@ -4500,10 +4500,12 @@ func TestTxIndexer(t *testing.T) {
 	}
 	verify := func(db ethdb.Database, expTail uint64) {
 		tail := rawdb.ReadTxIndexTail(db)
+		//nolint:staticcheck
 		if tail == nil {
 			t.Fatal("Failed to write tx index tail")
 		}
 
+		//nolint:staticcheck
 		if *tail != expTail {
 			t.Fatalf("Unexpected tx index tail, want %v, got %d", expTail, *tail)
 		}

eth/backend.go

@@ -695,16 +695,12 @@ func retryHeimdallHandler(fn heimdallHandler, tickerDuration time.Duration, time
 		return
 	}
 
-	// first run for fetching milestones
+	// first run
 	firstCtx, cancel := context.WithTimeout(context.Background(), timeout)
-	err = fn(firstCtx, ethHandler, bor)
+	_ = fn(firstCtx, ethHandler, bor)
 
 	cancel()
 
-	if err != nil {
-		log.Warn(fmt.Sprintf("unable to start the %s service - first run", fnName), "err", err)
-	}
-
 	ticker := time.NewTicker(tickerDuration)
 	defer ticker.Stop()
 
@@ -712,13 +708,11 @@ func retryHeimdallHandler(fn heimdallHandler, tickerDuration time.Duration, time
 		select {
 		case <-ticker.C:
 			ctx, cancel := context.WithTimeout(context.Background(), timeout)
-			err := fn(ctx, ethHandler, bor)
+
+			// Skip any error reporting here as it's handled in respective functions
+			_ = fn(ctx, ethHandler, bor)
 
 			cancel()
-
-			if err != nil {
-				log.Warn(fmt.Sprintf("unable to handle %s", fnName), "err", err)
-			}
 		case <-closeCh:
 			return
 		}
@@ -754,7 +748,7 @@ func (s *Ethereum) handleMilestone(ctx context.Context, ethHandler *ethHandler,
 	// If the current chain head is behind the received milestone, add it to the future milestone
 	// list. Also, the hash mismatch (end block hash) error will lead to rewind so also
 	// add that milestone to the future milestone list.
-	if errors.Is(err, errMissingBlocks) || errors.Is(err, errHashMismatch) {
+	if errors.Is(err, errChainOutOfSync) || errors.Is(err, errHashMismatch) {
 		ethHandler.downloader.ProcessFutureMilestone(num, hash)
 	}
 

eth/bor_api_backend.go

@@ -53,16 +53,16 @@ func (b *EthAPIBackend) GetVoteOnHash(ctx context.Context, starBlockNr uint64, e
 		return false, errBorEngineNotAvailable
 	}
 
-	//Confirmation of 16 blocks on the endblock
+	// Confirmation of 16 blocks on the endblock
 	tipConfirmationBlockNr := endBlockNr + uint64(16)
 
-	//Check if tipConfirmation block exit
+	// Check if tipConfirmation block exit
 	_, err := b.BlockByNumber(ctx, rpc.BlockNumber(tipConfirmationBlockNr))
 	if err != nil {
 		return false, errTipConfirmationBlock
 	}
 
-	//Check if end block exist
+	// Check if end block exist
 	localEndBlock, err := b.BlockByNumber(ctx, rpc.BlockNumber(endBlockNr))
 	if err != nil {
 		return false, errEndBlock
@@ -75,12 +75,12 @@ func (b *EthAPIBackend) GetVoteOnHash(ctx context.Context, starBlockNr uint64, e
 
 	if !isLocked {
 		downloader.UnlockMutex(false, "", endBlockNr, common.Hash{})
-		return false, errors.New("Whitelisted number or locked sprint number is more than the received end block number")
+		return false, errors.New("whitelisted number or locked sprint number is more than the received end block number")
 	}
 
 	if localEndBlockHash != hash {
 		downloader.UnlockMutex(false, "", endBlockNr, common.Hash{})
-		return false, fmt.Errorf("Hash mismatch: localChainHash %s, milestoneHash %s", localEndBlockHash, hash)
+		return false, fmt.Errorf("hash mismatch: localChainHash %s, milestoneHash %s", localEndBlockHash, hash)
 	}
 
 	downloader.UnlockMutex(true, milestoneId, endBlockNr, localEndBlock.Hash())

eth/bor_checkpoint_verifier.go

@@ -12,12 +12,16 @@ import (
 )
 
 var (
-	// errMissingBlocks is returned when we don't have the blocks locally, yet.
+	// errMissingCurrentBlock is returned when we don't have the current block
-	errMissingBlocks = errors.New("missing blocks")
+	// present locally.
+	errMissingCurrentBlock = errors.New("current block missing")
 
-	// errRootHash is returned when we aren't able to calculate the root hash
+	// errChainOutOfSync is returned when we're trying to process a future
-	// locally for a range of blocks.
+	// checkpoint/milestone and we haven't reached at that number yet.
-	errRootHash = errors.New("failed to get local root hash")
+	errChainOutOfSync = errors.New("chain out of sync")
+
+	// errRootHash is returned when the root hash calculation for a range of blocks fails.
+	errRootHash = errors.New("root hash calculation failed")
 
 	// errHashMismatch is returned when the local hash doesn't match
 	// with the hash of checkpoint/milestone. It is the root hash of blocks
@@ -27,13 +31,10 @@ var (
 	// errEndBlock is returned when we're unable to fetch a block locally.
 	errEndBlock = errors.New("failed to get end block")
 
-	// errEndBlock is returned when we're unable to fetch a block locally.
+	// errEndBlock is returned when we're unable to fetch the tip confirmation block locally.
 	errTipConfirmationBlock = errors.New("failed to get tip confirmation block")
 
-	// errBlockNumberConversion is returned when we get err in parsing hexautil block number
+	// rewindLengthMeter for collecting info about the length of chain rewinded
-	errBlockNumberConversion = errors.New("failed to parse the block number")
-
-	//Metrics for collecting the rewindLength
 	rewindLengthMeter = metrics.NewRegisteredMeter("chain/autorewind/length", nil)
 )
 
@@ -57,14 +58,14 @@ func borVerify(ctx context.Context, eth *Ethereum, handler *ethHandler, start ui
 	currentBlock := eth.BlockChain().CurrentBlock()
 	if currentBlock == nil {
 		log.Debug(fmt.Sprintf("Failed to fetch current block from blockchain while verifying incoming %s", str))
-		return hash, errMissingBlocks
+		return hash, errMissingCurrentBlock
 	}
 
 	head := currentBlock.Number.Uint64()
 
 	if head < end {
 		log.Debug(fmt.Sprintf("Current head block behind incoming %s block", str), "head", head, "end block", end)
-		return hash, errMissingBlocks
+		return hash, errChainOutOfSync
 	}
 
 	var localHash string
@@ -77,15 +78,15 @@ func borVerify(ctx context.Context, eth *Ethereum, handler *ethHandler, start ui
 		localHash, err = handler.ethAPI.GetRootHash(ctx, start, end)
 
 		if err != nil {
-			log.Debug("Failed to get root hash of given block range while whitelisting checkpoint", "start", start, "end", end, "err", err)
+			log.Debug("Failed to calculate root hash of given block range while whitelisting checkpoint", "start", start, "end", end, "err", err)
-			return hash, errRootHash
+			return hash, fmt.Errorf("%w: %v", errRootHash, err)
 		}
 	} else {
 		// in case of milestone(isCheckpoint==false) get the hash of endBlock
 		block, err := handler.ethAPI.GetBlockByNumber(ctx, rpc.BlockNumber(end), false)
 		if err != nil {
 			log.Debug("Failed to get end block hash while whitelisting milestone", "number", end, "err", err)
-			return hash, errEndBlock
+			return hash, fmt.Errorf("%w: %v", errEndBlock, err)
 		}
 
 		localHash = fmt.Sprintf("%v", block["hash"])[2:]
@@ -93,7 +94,6 @@ func borVerify(ctx context.Context, eth *Ethereum, handler *ethHandler, start ui
 
 	//nolint
 	if localHash != hash {
-
 		if isCheckpoint {
 			log.Warn("Root hash mismatch while whitelisting checkpoint", "expected", localHash, "got", hash)
 		} else {
@@ -124,9 +124,9 @@ func borVerify(ctx context.Context, eth *Ethereum, handler *ethHandler, start ui
 		}
 
 		if isCheckpoint {
-			log.Warn("Rewinding chain due to checkpoint root hash mismatch", "number", rewindTo)
+			log.Info("Rewinding chain due to checkpoint root hash mismatch", "number", rewindTo)
 		} else {
-			log.Warn("Rewinding chain due to milestone endblock hash mismatch", "number", rewindTo)
+			log.Info("Rewinding chain due to milestone endblock hash mismatch", "number", rewindTo)
 		}
 
 		rewindBack(eth, head, rewindTo)
@@ -138,7 +138,7 @@ func borVerify(ctx context.Context, eth *Ethereum, handler *ethHandler, start ui
 	block, err := handler.ethAPI.GetBlockByNumber(ctx, rpc.BlockNumber(end), false)
 	if err != nil {
 		log.Debug("Failed to get end block hash while whitelisting", "err", err)
-		return hash, errEndBlock
+		return hash, fmt.Errorf("%w: %v", errEndBlock, err)
 	}
 
 	hash = fmt.Sprintf("%v", block["hash"])
@@ -170,5 +170,4 @@ func rewind(eth *Ethereum, head uint64, rewindTo uint64) {
 	} else {
 		rewindLengthMeter.Mark(int64(head - rewindTo))
 	}
-
 }

eth/downloader/whitelist/finality.go

@@ -18,6 +18,7 @@ type finality[T rawdb.BlockFinality[T]] struct {
 	Number   uint64      // Number , populated by reaching out to heimdall
 	interval uint64      // Interval, until which we can allow importing
 	doExist  bool
+	name     string // Name of the service (checkpoint or milestone)
 }
 
 type finalityService interface {
@@ -43,21 +44,33 @@ func (f *finality[T]) IsValidPeer(fetchHeadersByNumber func(number uint64, amoun
 	return isValidPeer(fetchHeadersByNumber, doExist, number, hash)
 }
 
-// IsValidChain checks the validity of chain by comparing it
+// IsValidChain checks the validity of chain by comparing it against the local
-// against the local checkpoint entry
+// whitelisted entry of checkpoint/milestone.
-// todo: need changes
 func (f *finality[T]) IsValidChain(currentHeader *types.Header, chain []*types.Header) (bool, error) {
 	// Return if we've received empty chain
 	if len(chain) == 0 {
 		return false, nil
 	}
 
-	res, err := isValidChain(currentHeader, chain, f.doExist, f.Number, f.Hash)
+	return isValidChain(currentHeader, chain, f.doExist, f.Number, f.Hash)
+}
 
-	return res, err
+// reportWhitelist logs the block number and hash if a new and unique entry is being inserted
+// and it doesn't log for duplicate/redundant entries.
+func (f *finality[T]) reportWhitelist(block uint64, hash common.Hash) {
+	msg := fmt.Sprintf("Whitelisting new %s from heimdall", f.name)
+	if !f.doExist {
+		log.Info(msg, "block", block, "hash", hash)
+	} else {
+		if f.Number != block && f.Hash != hash {
+			log.Info(msg, "block", block, "hash", hash)
+		}
+	}
 }
 
 func (f *finality[T]) Process(block uint64, hash common.Hash) {
+	f.reportWhitelist(block, hash)
+
 	f.doExist = true
 	f.Hash = hash
 	f.Number = block

eth/downloader/whitelist/service.go

@@ -60,6 +60,7 @@ func NewService(db ethdb.Database) *Service {
 				Hash:     checkpointHash,
 				interval: 256,
 				db:       db,
+				name:     "checkpoint",
 			},
 		},
 
@@ -70,6 +71,7 @@ func NewService(db ethdb.Database) *Service {
 				Hash:     milestoneHash,
 				interval: 256,
 				db:       db,
+				name:     "milestone",
 			},
 
 			Locked:                locked,

eth/handler_bor.go

@@ -3,6 +3,8 @@ package eth
 import (
 	"context"
 	"errors"
+	"fmt"
+	"strings"
 
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/consensus/bor"
@@ -18,8 +20,6 @@ var (
 	// errMilestone is returned when we are unable to fetch the
 	// latest milestone from the local heimdall.
 	errMilestone = errors.New("failed to fetch latest milestone")
-
-	ErrNotInRejectedList = errors.New("MilestoneID not in rejected list")
 )
 
 // fetchWhitelistCheckpoint fetches the latest checkpoint from it's local heimdall
@@ -32,19 +32,23 @@ func (h *ethHandler) fetchWhitelistCheckpoint(ctx context.Context, bor *bor.Bor,
 
 	// fetch the latest checkpoint from Heimdall
 	checkpoint, err := bor.HeimdallClient.FetchCheckpoint(ctx, -1)
+	err = reportCommonErrors("latest checkpoint", err, errCheckpoint)
 	if err != nil {
-		log.Debug("Failed to fetch latest checkpoint for whitelisting", "err", err)
+		return blockNum, blockHash, err
-		return blockNum, blockHash, errCheckpoint
 	}
 
-	log.Info("Got new checkpoint from heimdall", "start", checkpoint.StartBlock.Uint64(), "end", checkpoint.EndBlock.Uint64(), "rootHash", checkpoint.RootHash.String())
+	log.Debug("Got new checkpoint from heimdall", "start", checkpoint.StartBlock.Uint64(), "end", checkpoint.EndBlock.Uint64(), "rootHash", checkpoint.RootHash.String())
 
 	// Verify if the checkpoint fetched can be added to the local whitelist entry or not
 	// If verified, it returns the hash of the end block of the checkpoint. If not,
 	// it will return appropriate error.
 	hash, err := verifier.verify(ctx, eth, h, checkpoint.StartBlock.Uint64(), checkpoint.EndBlock.Uint64(), checkpoint.RootHash.String()[2:], true)
 	if err != nil {
+		if errors.Is(err, errChainOutOfSync) {
+			log.Info("Whitelisting checkpoint deferred", "err", err)
+		} else {
 			log.Warn("Failed to whitelist checkpoint", "err", err)
+		}
 		return blockNum, blockHash, err
 	}
 
@@ -64,72 +68,68 @@ func (h *ethHandler) fetchWhitelistMilestone(ctx context.Context, bor *bor.Bor,
 
 	// fetch latest milestone
 	milestone, err := bor.HeimdallClient.FetchMilestone(ctx)
-	if errors.Is(err, heimdall.ErrServiceUnavailable) {
+	err = reportCommonErrors("latest milestone", err, errMilestone)
-		log.Debug("Failed to fetch latest milestone for whitelisting", "err", err)
-		return num, hash, err
-	}
-
 	if err != nil {
-		log.Error("Failed to fetch latest milestone for whitelisting", "err", err)
+		return num, hash, err
-		return num, hash, errMilestone
 	}
 
 	num = milestone.EndBlock.Uint64()
 	hash = milestone.Hash
 
-	log.Info("Got new milestone from heimdall", "start", milestone.StartBlock.Uint64(), "end", milestone.EndBlock.Uint64(), "hash", milestone.Hash.String())
+	log.Debug("Got new milestone from heimdall", "start", milestone.StartBlock.Uint64(), "end", milestone.EndBlock.Uint64(), "hash", milestone.Hash.String())
 
-	// Verify if the milestone fetched can be added to the local whitelist entry or not
+	// Verify if the milestone fetched can be added to the local whitelist entry or not. If verified,
-	// If verified, it returns the hash of the end block of the milestone. If not,
+	// the hash of the end block of the milestone is returned else appropriate error is returned.
-	// it will return appropriate error.
 	_, err = verifier.verify(ctx, eth, h, milestone.StartBlock.Uint64(), milestone.EndBlock.Uint64(), milestone.Hash.String()[2:], false)
 	if err != nil {
+		if errors.Is(err, errChainOutOfSync) {
+			log.Info("Whitelisting milestone deferred", "err", err)
+		} else {
+			log.Warn("Failed to whitelist milestone", "err", err)
+		}
 		h.downloader.UnlockSprint(milestone.EndBlock.Uint64())
-		return num, hash, err
 	}
 
-	return num, hash, nil
+	return num, hash, err
 }
 
 func (h *ethHandler) fetchNoAckMilestone(ctx context.Context, bor *bor.Bor) (string, error) {
-	var (
-		milestoneID string
-	)
-
-	// fetch latest milestone
 	milestoneID, err := bor.HeimdallClient.FetchLastNoAckMilestone(ctx)
-	if errors.Is(err, heimdall.ErrServiceUnavailable) {
+	err = reportCommonErrors("latest no-ack milestone", err, nil)
-		log.Debug("Failed to fetch latest no-ack milestone", "err", err)
+
 	return milestoneID, err
-	}
-
-	if err != nil {
-		log.Error("Failed to fetch latest no-ack milestone", "err", err)
-		return milestoneID, errMilestone
-	}
-
-	return milestoneID, nil
 }
 
 func (h *ethHandler) fetchNoAckMilestoneByID(ctx context.Context, bor *bor.Bor, milestoneID string) error {
-	// fetch latest milestone
 	err := bor.HeimdallClient.FetchNoAckMilestone(ctx, milestoneID)
-	if errors.Is(err, heimdall.ErrServiceUnavailable) {
+	if errors.Is(err, heimdall.ErrNotInRejectedList) {
-		log.Debug("Failed to fetch no-ack milestone by ID", "milestoneID", milestoneID, "err", err)
+		log.Debug("MilestoneID not in rejected list", "milestoneID", milestoneID)
+	}
+	err = reportCommonErrors("no-ack milestone by ID", err, nil, "milestoneID", milestoneID)
+	return err
+}
+
+// reportCommonErrors reports common errors which can occur while fetching data from heimdall. It also
+// returns back the wrapped erorr if required to the caller.
+func reportCommonErrors(msg string, err error, wrapError error, ctx ...interface{}) error {
+	if err == nil {
+		return err
+	}
+
+	// We're skipping extra check to the `heimdall.ErrServiceUnavailable` error as it should not
+	// occur post HF (in heimdall). If it does, we'll anyways warn below as a normal error.
+
+	ctx = append(ctx, "err", err)
+
+	if strings.Contains(err.Error(), "context deadline exceeded") {
+		log.Warn(fmt.Sprintf("Failed to fetch %s, please check the heimdall endpoint and status of your heimdall node", msg), ctx...)
+	} else {
+		log.Warn(fmt.Sprintf("Failed to fetch %s", msg), ctx...)
+	}
+
+	if wrapError != nil {
+		return fmt.Errorf("%w: %v", wrapError, err)
+	}
+
 	return err
-	}
-
-	// fixme: handle different types of errors
-	if errors.Is(err, ErrNotInRejectedList) {
-		log.Warn("MilestoneID not in rejected list", "milestoneID", milestoneID, "err", err)
-		return err
-	}
-
-	if err != nil {
-		log.Error("Failed to fetch no-ack milestone by ID ", "milestoneID", milestoneID, "err", err)
-
-		return errMilestone
-	}
-
-	return nil
 }

eth/handler_bor_test.go

@@ -103,7 +103,7 @@ func fetchCheckpointTest(t *testing.T, heimdall *mockHeimdall, bor *bor.Bor, han
 	ctx := context.Background()
 
 	_, _, err := handler.fetchWhitelistCheckpoint(ctx, bor, nil, verifier)
-	require.Equal(t, err, errCheckpoint)
+	require.ErrorIs(t, err, errCheckpoint)
 
 	// create 4 mock checkpoints
 	checkpoints = createMockCheckpoints(4)
@@ -133,7 +133,7 @@ func fetchMilestoneTest(t *testing.T, heimdall *mockHeimdall, bor *bor.Bor, hand
 	ctx := context.Background()
 
 	_, _, err := handler.fetchWhitelistMilestone(ctx, bor, nil, verifier)
-	require.Equal(t, err, errMilestone)
+	require.ErrorIs(t, err, errMilestone)
 
 	// create 4 mock checkpoints
 	milestones = createMockMilestones(4)