Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(dot/state): fix deadlock, fixes bootstrap syncing #1959

Merged
merged 9 commits into from
Nov 3, 2021
Merged

fix(dot/state): fix deadlock, fixes bootstrap syncing #1959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
273e4ce
add blockQueue logs, increase cap
noot Oct 28, 2021
08508d6
move stuff, add logs
noot Nov 1, 2021
58d6d3d
remove GetBlockBody lock
noot Nov 2, 2021
4b6de75
cleanup
noot Nov 2, 2021
6318263
cleanup
noot Nov 2, 2021
80a2106
Merge branch 'development' of github.com:ChainSafe/gossamer into no…
noot Nov 2, 2021
2424b06
update handleFinalisedBlock to get store block in db, then delete fro…
noot Nov 3, 2021
54879ba
Merge branch 'development' of github.com:ChainSafe/gossamer into noot…
noot Nov 3, 2021
6cc0264
fix lint
noot Nov 3, 2021
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
1 change: 0 additions & 1 deletion dot/core/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,6 @@ func (s *Service) handleBlocksAsync() {
}

s.maintainTransactionPool(block)

case <-s.ctx.Done():
return
}
Expand Down
8 changes: 5 additions & 3 deletions dot/state/block.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,10 @@ func (bs *BlockState) getAndDeleteUnfinalisedBlock(hash common.Hash) (*types.Blo
return block.(*types.Block), true
}

func (bs *BlockState) deleteUnfinalisedBlock(hash common.Hash) {
bs.unfinalisedBlocks.Delete(hash)
}

// HasHeader returns if the db contains a header with the given hash
func (bs *BlockState) HasHeader(hash common.Hash) (bool, error) {
if bs.hasUnfinalisedBlock(hash) {
Expand Down Expand Up @@ -327,6 +331,7 @@ func (bs *BlockState) GetBlockByHash(hash common.Hash) (*types.Block, error) {
if err != nil {
return nil, err
}

return &types.Block{Header: *header, Body: *blockBody}, nil
}

Expand Down Expand Up @@ -360,9 +365,6 @@ func (bs *BlockState) HasBlockBody(hash common.Hash) (bool, error) {

// GetBlockBody will return Body for a given hash
func (bs *BlockState) GetBlockBody(hash common.Hash) (*types.Body, error) {
bs.RLock()
defer bs.RUnlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure GetBlockBody is not used anywhere else? As in, maybe we need to add locking on other callers using this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it isn't used anywhere else in the BlockState, so I think it's probably ok for now. but in the future it might be nice to do some lock overhaul on the BlockState since it could be improved imo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe unexport that method to make this clearer, if possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah sorry it is used in other spots in the codebase, so gotta leave it like this for now

Copy link
Contributor

Choose a reason for hiding this comment

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

So are you sure it won't data race on the block state if it's accessed elsewhere in the codebase?

Copy link
Member

Choose a reason for hiding this comment

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

currently, the GetBlockBody just access the unfinalisedBlocks field that is a *sync.Map so by default can prevent concurrent access to the data other than that I'm pretty sure we can remove the locks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah there's an edge case potentially with the handleFinalisedBlock function (just pushed a fix for it) previously that function was doing LoadAndDelete on the unfinalized blocks, then storing it in the db. so there was a potential race condition where one process was finalizing a block, and it was deleted from memory but not stored in the db yet, while another process calls GetBlockBody and isn't able to find it in memory or in the db. however my update to handleFinalisedBlock should fix this (it now gets from memory, stores in the db, then deletes from memory)


block, has := bs.getUnfinalisedBlock(hash)
if has {
return &block.Body, nil
Expand Down
4 changes: 3 additions & 1 deletion dot/state/block_finalisation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ func (bs *BlockState) handleFinalisedBlock(curr common.Hash) error {
continue
}

block, has := bs.getAndDeleteUnfinalisedBlock(hash)
block, has := bs.getUnfinalisedBlock(hash)
if !has {
return fmt.Errorf("failed to find block in unfinalised block map, block=%s", hash)
}
Expand All @@ -251,6 +251,8 @@ func (bs *BlockState) handleFinalisedBlock(curr common.Hash) error {
if err = batch.Put(headerHashKey(block.Header.Number.Uint64()), hash.ToBytes()); err != nil {
return err
}

bs.deleteUnfinalisedBlock(hash)
}

return batch.Flush()
Expand Down
17 changes: 8 additions & 9 deletions dot/sync/chain_processor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,6 @@ func (s *chainProcessor) processBlockData(bd *types.BlockData) error {
return ErrNilBlockData
}

err := s.blockState.CompareAndSetBlockData(bd)
if err != nil {
return fmt.Errorf("failed to compare and set data: %w", err)
}

hasHeader, _ := s.blockState.HasHeader(bd.Hash)
hasBody, _ := s.blockState.HasBlockBody(bd.Hash)
if hasHeader && hasBody {
Expand Down Expand Up @@ -164,8 +159,10 @@ func (s *chainProcessor) processBlockData(bd *types.BlockData) error {
return nil
}

logger.Debug("processing block data", "hash", bd.Hash)

if bd.Header != nil && bd.Body != nil {
if err = s.handleHeader(bd.Header); err != nil {
if err := s.handleHeader(bd.Header); err != nil {
return err
}

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

logger.Debug("processing block", "hash", bd.Hash)

if err = s.handleBlock(block); err != nil {
if err := s.handleBlock(block); err != nil {
logger.Error("failed to handle block", "number", block.Header.Number, "error", err)
return err
}
Expand All @@ -191,6 +186,10 @@ func (s *chainProcessor) processBlockData(bd *types.BlockData) error {
s.handleJustification(bd.Header, *bd.Justification)
}

if err := s.blockState.CompareAndSetBlockData(bd); err != nil {
return fmt.Errorf("failed to compare and set data: %w", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why moving CompareAndSetBlockData? There must be a reason but it's not totally mentioned in the description 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, sorry I should have mentioned. this should definitely be happening after the block checks happen, otherwise we might be storing some data in the database and the block turns out to be invalid, and then the extra block data is still stored.


return nil
}

Expand Down
13 changes: 9 additions & 4 deletions lib/runtime/test_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,6 @@ func GetRuntimeBlob(testRuntimeFilePath, testRuntimeURL string) (n int64, err er
if err != nil {
return 0, err
}
defer func() {
_ = out.Close()
}()

/* #nosec */
resp, err := http.Get(testRuntimeURL)
Expand All @@ -103,7 +100,15 @@ func GetRuntimeBlob(testRuntimeFilePath, testRuntimeURL string) (n int64, err er
}()

n, err = io.Copy(out, resp.Body)
return n, err
if err != nil {
return 0, err
}

if err = out.Close(); err != nil {
return 0, err
}

return n, nil
}

// TestRuntimeNetwork ...
Expand Down
2 changes: 1 addition & 1 deletion scripts/install-lint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ fi

if ! command -v golangci-lint &> /dev/null
then
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.41.0
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.43.0
fi

export PATH=$PATH:$(go env GOPATH)/bin