Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion dot/core/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,12 +127,16 @@ func (s *Service) StorageRoot() (common.Hash, error) {
}

// HandleBlockImport handles a block that was imported via the network
func (s *Service) HandleBlockImport(block *types.Block, state *rtstorage.TrieState) error {
func (s *Service) HandleBlockImport(block *types.Block, state *rtstorage.TrieState, announce bool) error {
err := s.handleBlock(block, state)
if err != nil {
return fmt.Errorf("handling block: %w", err)
}

if !announce {
return nil
}

bestBlockHash := s.blockState.BestBlockHash()
isBestBlock := bestBlockHash.Equal(block.Header.Hash())

Expand Down
6 changes: 6 additions & 0 deletions dot/network/notifications_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package network

import (
"errors"
"fmt"
"reflect"
"testing"
"time"
Expand Down Expand Up @@ -196,6 +197,11 @@ func TestCreateNotificationsMessageHandler_BlockAnnounceHandshake(t *testing.T)

err = handler(stream, testHandshake)
require.ErrorIs(t, err, errCannotValidateHandshake)

expectedErrorMessage := fmt.Sprintf("handling handshake: %s from peer %s using protocol %s: genesis hash mismatch",
errCannotValidateHandshake, testPeerID, info.protocolID)
Comment thread
EclesioMeloJunior marked this conversation as resolved.
require.EqualError(t, err, expectedErrorMessage)
Comment thread
qdm12 marked this conversation as resolved.

data := info.peersData.getInboundHandshakeData(testPeerID)
require.NotNil(t, data)
require.True(t, data.received)
Expand Down
49 changes: 32 additions & 17 deletions dot/sync/chain_processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ type chainProcessor struct {
ctx context.Context
cancel context.CancelFunc

chainSync ChainSync

// blocks that are ready for processing. ie. their parent is known, or their parent is ahead
// of them within this channel and thus will be processed first
readyBlocks *blockQueue
Expand All @@ -42,24 +44,35 @@ type chainProcessor struct {
telemetry telemetry.Client
}

func newChainProcessor(readyBlocks *blockQueue, pendingBlocks DisjointBlockSet,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chainProcessorConfig and chainProcessor are basically the same struct apart from ctx and cancel. chainProcessorConfig isn't useful apart from providing better indentation.
I would suggest to remove chainProcessorConfig and modify newChainProcessor everywhere to provide better indentation.

Like given below, (here and everywhere newChainProcessor is used)

func newChainProcessor(
	readyBlocks *blockQueue, 
	pendingBlocks DisjointBlockSet,
	blockState BlockState, 
	storageState StorageState,
	transactionState TransactionState, 
	babeVerifier BabeVerifier,
	finalityGadget FinalityGadget, 
	blockImportHandler BlockImportHandler, 
	telemetry telemetry.Client) 
*chainProcessor {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ repeating myself, but these are out of scope changes, we should have them in a separate ez PR.

Eclesio's argument was:

I will keep it as it is because we already use a configuration struct for the chain_sync so adding this configuration struct for chain_processor keep them in the same pattern. I have changed to accept a value instead of a pointer.

Although I agree we should standardize our codebase as we progress through it, I agree with Kishan's suggestion especially for unexported/internal constructors.

I would also add in my very opinionated opinion that config structs should be reserved for the public Go API (so exported public constructors) and every field can be set to a default (so the user can pass an empty struct). It also allows to add fields without breaking the constructor signature (so it stays retro-compat).

For internal constructors, it's far easier to have plain and simple compulsory named arguments which would detect compilation problems when adding an argument.

blockState BlockState, storageState StorageState,
transactionState TransactionState, babeVerifier BabeVerifier,
finalityGadget FinalityGadget, blockImportHandler BlockImportHandler, telemetry telemetry.Client) *chainProcessor {
type chainProcessorConfig struct {
readyBlocks *blockQueue
pendingBlocks DisjointBlockSet
syncer ChainSync
blockState BlockState
storageState StorageState
transactionState TransactionState
babeVerifier BabeVerifier
finalityGadget FinalityGadget
blockImportHandler BlockImportHandler
telemetry telemetry.Client
}

func newChainProcessor(cfg chainProcessorConfig) *chainProcessor {
ctx, cancel := context.WithCancel(context.Background())

return &chainProcessor{
ctx: ctx,
cancel: cancel,
readyBlocks: readyBlocks,
pendingBlocks: pendingBlocks,
blockState: blockState,
storageState: storageState,
transactionState: transactionState,
babeVerifier: babeVerifier,
finalityGadget: finalityGadget,
blockImportHandler: blockImportHandler,
telemetry: telemetry,
readyBlocks: cfg.readyBlocks,
pendingBlocks: cfg.pendingBlocks,
chainSync: cfg.syncer,
blockState: cfg.blockState,
storageState: cfg.storageState,
transactionState: cfg.transactionState,
babeVerifier: cfg.babeVerifier,
finalityGadget: cfg.finalityGadget,
blockImportHandler: cfg.blockImportHandler,
telemetry: cfg.telemetry,
}
}

Expand Down Expand Up @@ -109,6 +122,8 @@ func (s *chainProcessor) processBlockData(bd *types.BlockData) error {
return fmt.Errorf("failed to check block state has body for hash %s: %w", bd.Hash, err)
}

// while in bootstrap mode we don't need to broadcast block announcements
announceImportedBlock := s.chainSync.syncState() == tip
if hasHeader && hasBody {
// TODO: fix this; sometimes when the node shuts down the "best block" isn't stored properly,
// so when the node restarts it has blocks higher than what it thinks is the best, causing it not to sync
Expand Down Expand Up @@ -149,7 +164,7 @@ func (s *chainProcessor) processBlockData(bd *types.BlockData) error {
return err
}

if err := s.blockImportHandler.HandleBlockImport(block, state); err != nil {
if err := s.blockImportHandler.HandleBlockImport(block, state, announceImportedBlock); err != nil {
logger.Warnf("failed to handle block import: %s", err)
}

Expand All @@ -170,7 +185,7 @@ func (s *chainProcessor) processBlockData(bd *types.BlockData) error {
Body: *bd.Body,
}

if err := s.handleBlock(block); err != nil {
if err := s.handleBlock(block, announceImportedBlock); err != nil {
logger.Debugf("failed to handle block number %d: %s", block.Header.Number, err)
return err
}
Expand Down Expand Up @@ -201,7 +216,7 @@ func (s *chainProcessor) handleBody(body *types.Body) {
}

// handleHeader handles blocks (header+body) included in BlockResponses
func (s *chainProcessor) handleBlock(block *types.Block) error {
func (s *chainProcessor) handleBlock(block *types.Block, announceImportedBlock bool) error {
parent, err := s.blockState.GetHeader(block.Header.ParentHash)
if err != nil {
return fmt.Errorf("%w: %s", errFailedToGetParent, err)
Expand Down Expand Up @@ -233,7 +248,7 @@ func (s *chainProcessor) handleBlock(block *types.Block) error {
return fmt.Errorf("failed to execute block %d: %w", block.Header.Number, err)
}

if err = s.blockImportHandler.HandleBlockImport(block, ts); err != nil {
if err = s.blockImportHandler.HandleBlockImport(block, ts, announceImportedBlock); err != nil {
return err
}

Expand Down
Loading