From bf0a28ffe5bb36f775caa12fdb5d9b791a8bc5e1 Mon Sep 17 00:00:00 2001 From: Manav Darji Date: Thu, 13 Jun 2024 14:45:30 +0530 Subject: [PATCH 1/5] eth, consensus: refactor whitelisting related logs and error handling --- consensus/bor/heimdall.go | 6 +- consensus/bor/heimdallapp/milestone.go | 23 +++---- consensus/bor/heimdallgrpc/milestone.go | 17 ++--- eth/backend.go | 18 ++--- eth/bor_checkpoint_verifier.go | 37 +++++------ eth/downloader/whitelist/finality.go | 23 +++++-- eth/downloader/whitelist/service.go | 2 + eth/handler_bor.go | 88 ++++++++++++------------- 8 files changed, 111 insertions(+), 103 deletions(-) diff --git a/consensus/bor/heimdall.go b/consensus/bor/heimdall.go index a093cc95da..5335e99bee 100644 --- a/consensus/bor/heimdall.go +++ b/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 - 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 + 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 + FetchMilestoneID(ctx context.Context, milestoneID string) error // Fetch the bool value whether milestone corresponding to the given id is in process in Heimdall Close() } diff --git a/consensus/bor/heimdallapp/milestone.go b/consensus/bor/heimdallapp/milestone.go index b7183b6498..8d9d7a0943 100644 --- a/consensus/bor/heimdallapp/milestone.go +++ b/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 { diff --git a/consensus/bor/heimdallgrpc/milestone.go b/consensus/bor/heimdallgrpc/milestone.go index 917e33771a..a156838548 100644 --- a/consensus/bor/heimdallgrpc/milestone.go +++ b/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 } diff --git a/eth/backend.go b/eth/backend.go index 12170fa238..e94deb7ce0 100644 --- a/eth/backend.go +++ b/eth/backend.go @@ -694,16 +694,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() @@ -711,13 +707,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) - cancel() + // Skip any error reporting here as it's handled in respective functions + _ = fn(ctx, ethHandler, bor) - if err != nil { - log.Warn(fmt.Sprintf("unable to handle %s", fnName), "err", err) - } + cancel() case <-closeCh: return } @@ -753,7 +747,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) } diff --git a/eth/bor_checkpoint_verifier.go b/eth/bor_checkpoint_verifier.go index b2fd2c2d21..bb65e737a5 100644 --- a/eth/bor_checkpoint_verifier.go +++ b/eth/bor_checkpoint_verifier.go @@ -12,12 +12,16 @@ import ( ) var ( - // errMissingBlocks is returned when we don't have the blocks locally, yet. - errMissingBlocks = errors.New("missing blocks") + // errMissingCurrentBlock is returned when we don't have the current block + // present locally. + errMissingCurrentBlock = errors.New("current block missing") - // errRootHash is returned when we aren't able to calculate the root hash - // locally for a range of blocks. - errRootHash = errors.New("failed to get local root hash") + // errChainOutOfSync is returned when we're trying to process a future + // checkpoint/milestone and we haven't reached at that number yet. + 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 @@ -30,10 +34,7 @@ var ( // errEndBlock is returned when we're unable to fetch a block locally. errTipConfirmationBlock = errors.New("failed to get tip confirmation block") - // errBlockNumberConversion is returned when we get err in parsing hexautil block number - errBlockNumberConversion = errors.New("failed to parse the block number") - - //Metrics for collecting the rewindLength + // rewindLengthMeter for collecting info about the length of chain rewinded 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) - return hash, errRootHash + log.Debug("Failed to calculate root hash of given block range while whitelisting checkpoint", "start", start, "end", end, "err", err) + 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)) } - } diff --git a/eth/downloader/whitelist/finality.go b/eth/downloader/whitelist/finality.go index 7ab684d88a..6d8f981321 100644 --- a/eth/downloader/whitelist/finality.go +++ b/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 -// against the local checkpoint entry -// todo: need changes +// IsValidChain checks the validity of chain by comparing it against the local +// whitelisted entry of checkpoint/milestone. 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 diff --git a/eth/downloader/whitelist/service.go b/eth/downloader/whitelist/service.go index 3c23e4a268..7e7e1985da 100644 --- a/eth/downloader/whitelist/service.go +++ b/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, diff --git a/eth/handler_bor.go b/eth/handler_bor.go index 9fbc096d3d..0cbadcea3a 100644 --- a/eth/handler_bor.go +++ b/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, errCheckpoint + return blockNum, blockHash, err } - 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 { - log.Warn("Failed to whitelist checkpoint", "err", err) + 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,66 @@ 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) { - log.Debug("Failed to fetch latest milestone for whitelisting", "err", err) - return num, hash, err - } - + err = reportCommonErrors("latest milestone", err, errMilestone) if err != nil { - log.Error("Failed to fetch latest milestone for whitelisting", "err", err) - return num, hash, errMilestone + return num, hash, err } 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 - // If verified, it returns the hash of the end block of the milestone. If not, - // it will return appropriate error. + // Verify if the milestone fetched can be added to the local whitelist entry or not. If verified, + // the hash of the end block of the milestone is returned else appropriate error is returned. _, 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) { - 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 - } + err = reportCommonErrors("latest no-ack milestone", err, nil) - return milestoneID, nil + return milestoneID, err } 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) { - log.Debug("Failed to fetch no-ack milestone by ID", "milestoneID", milestoneID, "err", err) - return err + if errors.Is(err, heimdall.ErrNotInRejectedList) { + log.Debug("MilestoneID not in rejected list", "milestoneID", milestoneID) } + err = reportCommonErrors("no-ack milestone by ID", err, nil, "milestoneID", milestoneID) + return err +} - // fixme: handle different types of errors - if errors.Is(err, ErrNotInRejectedList) { - log.Warn("MilestoneID not in rejected list", "milestoneID", milestoneID, "err", 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 } - if err != nil { - log.Error("Failed to fetch no-ack milestone by ID ", "milestoneID", milestoneID, "err", 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. + + 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), "err", err, ctx) + } else { + log.Warn(fmt.Sprintf("Failed to fetch %s", msg), "err", err, ctx) + } - return errMilestone + if wrapError != nil { + return fmt.Errorf("%w: %v", wrapError, err) } - return nil + return err } From f90910c2d8b040091fe4fd9d78e2834ae5fcea31 Mon Sep 17 00:00:00 2001 From: Manav Darji Date: Thu, 13 Jun 2024 14:45:39 +0530 Subject: [PATCH 2/5] core: fix lint --- core/blockchain_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/blockchain_test.go b/core/blockchain_test.go index 519c31fd28..dd80594ab1 100644 --- a/core/blockchain_test.go +++ b/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) } From b1c28de08d0ae684baaae1620211b3d1210739b2 Mon Sep 17 00:00:00 2001 From: Manav Darji Date: Thu, 13 Jun 2024 15:52:14 +0530 Subject: [PATCH 3/5] eth: fix tests, check against root error --- eth/handler_bor_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/eth/handler_bor_test.go b/eth/handler_bor_test.go index 7ceab1fc64..b062c9cdc7 100644 --- a/eth/handler_bor_test.go +++ b/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) From cdd8f1ad3ec304ab8c0a0cb69349b97330fcafc9 Mon Sep 17 00:00:00 2001 From: Manav Darji Date: Thu, 13 Jun 2024 15:52:34 +0530 Subject: [PATCH 4/5] eth: use ctx correctly while logging --- eth/handler_bor.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/eth/handler_bor.go b/eth/handler_bor.go index 0cbadcea3a..be06165b11 100644 --- a/eth/handler_bor.go +++ b/eth/handler_bor.go @@ -119,10 +119,12 @@ func reportCommonErrors(msg string, err error, wrapError error, ctx ...interface // 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), "err", err, ctx) + 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), "err", err, ctx) + log.Warn(fmt.Sprintf("Failed to fetch %s", msg), ctx...) } if wrapError != nil { From 8b9e9fd11017c16f40be9f6d04867d7efd8c7795 Mon Sep 17 00:00:00 2001 From: Manav Darji Date: Thu, 13 Jun 2024 15:52:56 +0530 Subject: [PATCH 5/5] eth: refactor comments and error message --- eth/bor_api_backend.go | 10 +++++----- eth/bor_checkpoint_verifier.go | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/eth/bor_api_backend.go b/eth/bor_api_backend.go index 9f909caa8e..93e93bb137 100644 --- a/eth/bor_api_backend.go +++ b/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()) diff --git a/eth/bor_checkpoint_verifier.go b/eth/bor_checkpoint_verifier.go index bb65e737a5..4d7a054374 100644 --- a/eth/bor_checkpoint_verifier.go +++ b/eth/bor_checkpoint_verifier.go @@ -31,7 +31,7 @@ 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") // rewindLengthMeter for collecting info about the length of chain rewinded