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

noot · 2021-11-02T12:52:22Z

Changes

deadlock was caused as GetBlockByHash() calls RLock(), GetBlockByHash() calls GetBlockBody() which previously also called RLock(). this was ok when there were no calls to the write lock Lock() simultaneously. however the deadlock scenario occured when one goroutine was calling GetBlockBody() (with 1 read-lock obtained) and another called SetFinalisedHash() (thus obtaining the write-lock). as a read-lock can no longer be obtained if 1. write-lock was obtained (even if it was freed) and 2. another read-lock was already obtained, this caused a deadlock
I ended up just removing the lock in GetBlockBody() to prevent multiple read-locks from being obtained. if anyone has a better method let me know :D

Tests

make gossamer
./bin/gossamer --chain polkadot

node can sync (up until it hits max memory :p) no more stalling/deadlocks

Issues

related to create gossamer devnet and document issues #1479

Primary Reviewer

@EclesioMeloJunior

…t/fix-bootstrap

codecov · 2021-11-02T13:26:48Z

Codecov Report

Merging #1959 (6cc0264) into development (7a66592) will decrease coverage by 0.03%.
The diff coverage is 38.46%.

@@               Coverage Diff               @@
##           development    #1959      +/-   ##
===============================================
- Coverage        60.27%   60.23%   -0.04%     
===============================================
  Files              180      180              
  Lines            18189    18189              
===============================================
- Hits             10963    10957       -6     
- Misses            5415     5418       +3     
- Partials          1811     1814       +3

Flag	Coverage Δ
unit-tests	`60.23% <38.46%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
dot/core/service.go	`52.91% <ø> (-0.78%)`	⬇️
lib/runtime/test_helpers.go	`16.88% <0.00%> (-0.23%)`	⬇️
dot/sync/chain_processor.go	`50.41% <40.00%> (-0.41%)`	⬇️
dot/state/block.go	`45.42% <100.00%> (-0.20%)`	⬇️
dot/state/block_finalisation.go	`45.29% <100.00%> (+0.47%)`	⬆️
dot/network/service.go	`69.09% <0.00%> (-1.17%)`	⬇️
dot/network/connmgr.go	`79.64% <0.00%> (+0.88%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 887be9e...6cc0264. Read the comment docs.

qdm12 · 2021-11-02T15:51:52Z

dot/sync/chain_processor.go

@@ -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)
+	}


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

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.

qdm12 · 2021-11-02T15:53:47Z

dot/state/block.go

@@ -360,9 +361,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()


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

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.

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

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

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

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

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)

EclesioMeloJunior · 2021-11-03T18:10:14Z

dot/state/block.go

@@ -360,9 +361,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()


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

…m memory

…/fix-bootstrap

# [0.6.0](v0.5.0...v0.6.0) (2021-12-03) ### Bug Fixes * **babe:** Fix extrinsic format in block. ([#1530](#1530)) ([1a03b2a](1a03b2a)) * **ci:** add missing go-version matrix to fix development branch CI ([#2037](#2037)) ([6babe76](6babe76)) * **cmd/cfg:** Use Babe Lead value from toml config ([#2032](#2032)) ([06aa3e3](06aa3e3)) * cmd/gossamer: Generate random name if --name flag not set ([#1506](#1506)) ([3c05a88](3c05a88)) * confirm block import notifier is closed properly ([#1736](#1736)) ([ad2d85e](ad2d85e)) * **docs:** improve build-spec usage docs ([#1706](#1706)) ([2e164b4](2e164b4)) * **dot/core:** Add only extrinsic during chain reorg. ([#1609](#1609)) ([29413d4](29413d4)) * **dot/core:** Batch process transaction message. ([#1780](#1780)) ([0064836](0064836)) * **dot/core:** check transaction Validity.Propagate field to determine whether to propagate tx ([#1643](#1643)) ([81f23cc](81f23cc)) * **dot/core:** Fix handle transaction message test. ([#1607](#1607)) ([58b8725](58b8725)) * **dot/network, lib/grandpa:** fix handshake decoding and grandpa message handler sigabort ([#1631](#1631)) ([887f72c](887f72c)) * **dot/network, lib/grandpa:** fix node sync, improve devnet finality ([bcc7935](bcc7935)) * **dot/network:** add nil checks in connManager ([#2069](#2069)) ([7f9c042](7f9c042)) * **dot/network:** Check for size when decoding leb128. ([#1634](#1634)) ([d082b9e](d082b9e)) * **dot/network:** check if peer supports protocol ([#1617](#1617)) ([6bf66a4](6bf66a4)) * **dot/network:** decrease DHT find peers interval for gssmr nodes ([#1703](#1703)) ([08516a0](08516a0)) * **dot/network:** fix bugs in notifications protocol handlers; add metrics for inbound/outbound streams ([#2010](#2010)) ([8c2993d](8c2993d)) * **dot/network:** fix dht connection on discovery on devnet ([#2059](#2059)) ([da065b8](da065b8)) * **dot/network:** fix discovery between gossamer nodes ([#1594](#1594)) ([f4c79d3](f4c79d3)) * **dot/network:** fix justification request at head logic ([#1510](#1510)) ([98d1413](98d1413)) * **dot/network:** fix memory allocations with `sizedBufferPool` ([#1963](#1963)) ([e0b126b](e0b126b)) * **dot/network:** Fix missing digest in header ([#2092](#2092)) ([21ea85e](21ea85e)) * **dot/network:** Fix notification handshake and reuse stream. ([#1545](#1545)) ([a632dc4](a632dc4)) * **dot/network:** fix receiving notifications messages from substrate peers ([#1517](#1517)) ([fdf3c53](fdf3c53)) * **dot/network:** fix stream manager tests ([#1683](#1683)) ([e02eca4](e02eca4)) * **dot/network:** implement a handshake timeout ([#1615](#1615)) ([87c2f63](87c2f63)) * **dot/network:** Implement time based handle transaction ([#1942](#1942)) ([dd08424](dd08424)) * **dot/network:** move low reputation peer removal from network ConnManager to peer scoring logic (dot/peerstate) ([#2068](#2068)) ([ac16285](ac16285)), closes [#2039](#2039) * **dot/network:** Return on EOF error while reading stream. ([#1733](#1733)) ([f447eac](f447eac)) * **dot/network:** split stored streams and handshakeData into inbound and outbound ([#1553](#1553)) ([637050b](637050b)) * **dot/network:** update notificationsProtocol handshakeData to sync.Map ([#1492](#1492)) ([22f7269](22f7269)) * **dot/node:** Start websocket server only with `--ws` flag ([#1671](#1671)) ([6ecef3b](6ecef3b)) * **dot/state, lib/babe, lib/trie:** improve syncing between gossamer authority nodes ([#1613](#1613)) ([ca99fbf](ca99fbf)) * **dot/state, lib/grandpa:** update justification and SignedVote handling in database ([#1682](#1682)) ([bbdcd6f](bbdcd6f)) * **dot/state,dot/network:** improve memory usage when syncing ([#1491](#1491)) ([3b2ad8d](3b2ad8d)) * **dot/state:** add StorageState Lock/Unlock API for usage by babe and sync ([#1700](#1700)) ([3c22ace](3c22ace)) * **dot/state:** fix deadlock, fixes bootstrap syncing ([#1959](#1959)) ([dd80c09](dd80c09)) * **dot/state:** fix usage of trie.Snapshot ([#1489](#1489)) ([3880a40](3880a40)) * **dot/state:** track runtime per-block, fix runtime upgrades differing between forks ([#1638](#1638)) ([e133884](e133884)) * **dot/state:** update `*state.BlockState.AddBlockToBlockTree` to store block in `unfinalisedBlocksMap` ([#2006](#2006)) ([55d997f](55d997f)) * **dot/sync:** add nil header checks ([#2099](#2099)) ([a7d4be0](a7d4be0)) * **dot/sync:** fix block request and response logic ([#1907](#1907)) ([9c6283e](9c6283e)) * **dot/sync:** fix creating block response, fixes node sync between gossamer nodes ([#1572](#1572)) ([1328c80](1328c80)) * **dot/telemetry:** refactor telemetry to reduce CPU usage ([#1597](#1597)) ([bc31ac7](bc31ac7)) * **dot/types:** *types.Body to be of type []types.Extrinsic ([#1807](#1807)) ([4c09715](4c09715)) * **dot/types:** fix max value for digest ([#1687](#1687)) ([48405e7](48405e7)) * **dot:** fix `TestNewNode` ([#2070](#2070)) ([42908d0](42908d0)) * fix edit link ([#1507](#1507)) ([5089327](5089327)) * fix Kusama sync; add storageState lock in core.HandleTransactionMessage ([#1783](#1783)) ([1d688e4](1d688e4)) * **lib/babe, lib/runtime/wasmer:** fixes for v0.9.8+ runtime ([#2075](#2075)) ([2f9f80c](2f9f80c)) * **lib/babe:** add `--babe-lead` flag, update epoch handling logic ([#1895](#1895)) ([7abcce6](7abcce6)) * **lib/babe:** add pre-runtime digest before calling initialize_block ([#1581](#1581)) ([c1b26d3](c1b26d3)) * **lib/babe:** always use 2/3 of slot to produce block, re-add potentially valid txs to queue ([#1679](#1679)) ([cf93ad3](cf93ad3)) * **lib/babe:** call AddBlock in BABE synchronously ([#1585](#1585)) ([86acc43](86acc43)) * **lib/babe:** fix BABE state storing after building block ([#1536](#1536)) ([1a3dea2](1a3dea2)) * **lib/babe:** fix err log ([#1801](#1801)) ([a96f06a](a96f06a)) * **lib/babe:** fix setting first slot of network, fix loading BABE epoch params ([#1640](#1640)) ([5c3dbfe](5c3dbfe)) * **lib/babe:** fix timing for transition between epochs ([#1636](#1636)) ([57027db](57027db)) * **lib/blocktree:** fix blocktree bug ([#2060](#2060)) ([c17b53a](c17b53a)) * **lib/blocktree:** fix potential nil pointer dereference in `HighestCommonAncestor`, core `handleBlocksAsync` ([#1993](#1993)) ([f7f4463](f7f4463)) * **lib/blocktree:** fix setting leaves after blocktree pruning ([#1605](#1605)) ([58c0854](58c0854)) * **lib/blocktree:** removes the inconsistency to choose a deepest leaf ([#2094](#2094)) ([43d68e3](43d68e3)) * **lib/crypto/ed25519:** update ed25519 to use go-schnorrkel bip39 derivation ([#1488](#1488)) ([dfb95d2](dfb95d2)) * **lib/genesis:** Update missing and incorrect fields in genesis file. ([#1681](#1681)) ([8207704](8207704)) * **lib/grandpa:** fix grandpa stall and various bugs ([#1708](#1708)) ([67c93f4](67c93f4)) * **lib/grandpa:** fix grandpa vote message switch ([#2095](#2095)) ([461890c](461890c)) * **lib/grandpa:** fix threshold checking to be strictly greater than 2/3 ([#1891](#1891)) ([66ffe51](66ffe51)) * **lib/grandpa:** use `defaultGrandpaInterval` if not set, fixes error on startup ([#1982](#1982)) ([75627b5](75627b5)) * **lib/runtime/life:** remove import C from life ([#1923](#1923)) ([ed507d2](ed507d2)) * **lib/runtime:** update HOST_API_TEST_RUNTIME_URL to reference specific commit ([#1885](#1885)) ([666ed06](666ed06)) * **log-levels:** do not ignore configuration file log levels ([#2016](#2016)) ([80879b2](80879b2)) * pending bubble hidden after block included ([#1592](#1592)) ([5826322](5826322)) * persist node name ([#1543](#1543)) ([88b88f2](88b88f2)) * **pprof:** only run pprof service if enabled ([#2073](#2073)) ([55669c5](55669c5)) * **release:** Trigger release when pushed to main branch. ([#1566](#1566)) ([d445c97](d445c97)) * **rpc/subscription:** subscribe runtime version notify when version changes ([#1686](#1686)) ([9a76d39](9a76d39)) * Staging CI workflow ([#2034](#2034)) ([84ec792](84ec792)) * **trie:** memory leak fix in `lib/trie` ([#2009](#2009)) ([0ad5eb7](0ad5eb7)) * update deprecated package ([#1603](#1603)) ([f195204](f195204)) * update go-schnorrkel version ([#1557](#1557)) ([b86c7ff](b86c7ff)) * update gssmr genesis to use pallet_babe::SameAuthoritiesForever ([#1696](#1696)) ([fb0a751](fb0a751)) * update HOST_API_TEST_RUNTIME_URL ([#1898](#1898)) ([2ef59a8](2ef59a8)) * **utils:** create a specific folder for database ([#1598](#1598)) ([8c67795](8c67795)) ### Features * add --chain dev option ([#1561](#1561)) ([04a2969](04a2969)) * Add properties and chainId on build-spec command ([#1520](#1520)) ([b18290c](b18290c)) * cmd: implement import-runtime subcommand ([#1483](#1483)) ([d82b2da](d82b2da)) * **cmd/gossamer:** implement --telemetry-url parameter ([#1890](#1890)) ([b202e89](b202e89)), closes [#1502](#1502) * **cmd:** implement offline pruning of state trie ([#1564](#1564)) ([af9c925](af9c925)) * **devnet:** Local Gossamer Devnet ([#2008](#2008)) ([a520001](a520001)) * **dot/network, lib/grandpa:** request justification on receiving NeighbourMessage, verify justification on receipt ([#1529](#1529)) ([e1f9f42](e1f9f42)) * **dot/network:** Add cache for network message. ([#1511](#1511)) ([accaf69](accaf69)) * **dot/network:** add propagate return bool to messageHandler func type to determine whether to propagate message or not ([#1555](#1555)) ([0d6f488](0d6f488)) * **dot/network:** implement persistent peers functionality ([#1512](#1512)) ([7850532](7850532)) * **dot/network:** implement streamManager to cleanup not recently used streams ([#1611](#1611)) ([ba861bf](ba861bf)) * **dot/network:** request block justifications when near head ([#1499](#1499)) ([ae7012b](ae7012b)) * **dot/peerset:** Implement peer scoring ([#1791](#1791)) ([1c989ad](1c989ad)) * **dot/rpc/modules:** add `system_addReservedPeer` and `system_removeReservedPeer` RPC call ([#1712](#1712)) ([dba5922](dba5922)) * **dot/rpc:** Add `system_localListenAddresses` RPC call ([#1689](#1689)) ([c981d2e](c981d2e)) * **dot/rpc:** Implement `childstate_getKeys` rpc call ([#1800](#1800)) ([9b2f41e](9b2f41e)) * **dot/rpc:** implement sync_state_genSyncSpec RPC call ([#1827](#1827)) ([2186caf](2186caf)) * **dot/state:** implement online pruning of historical state tries ([#1596](#1596)) ([3eb9399](3eb9399)) * **dot/sync:** implement codeSubstitutes ([#1635](#1635)) ([d87aaeb](d87aaeb)) * dot/telemetry: Implement basic telemetry connection ([#1497](#1497)) ([fcb4159](fcb4159)) * **dot/telemetry:** Added connection retry ([#1904](#1904)) ([579a791](579a791)) * **dot/telemetry:** Added more telemetry messages in grandpa client ([#2043](#2043)) ([2e57d15](2e57d15)), closes [#1841](#1841) [#1842](#1842) * **dot/telemetry:** implement notify.finalized telemetry interface ([#1877](#1877)) ([de1a60d](de1a60d)) * **dot/telemetry:** implement substrate_number_leaves metrics ([#1926](#1926)) ([69823c0](69823c0)) * **dot/telemetry:** implement telemetry message network_state ([#1618](#1618)) ([a81844e](a81844e)) * **flags:** read log levels from flags ([#1953](#1953)) ([9694e46](9694e46)) * implement ext_default_child_storage_storage_kill_version_2 ([#1799](#1799)) ([c2908ae](c2908ae)) * implement ext_offchain_index_set_version_1 for wasmer runtime ([#1739](#1739)) ([96c30a6](96c30a6)) * **lib/babe:** add check of types.ConfigData.SecondarySlots for disabling secondary verification ([#1910](#1910)) ([cd27ae4](cd27ae4)) * **lib/grandpa:** fully verify justifications using GrandpaState ([#1544](#1544)) ([028d25e](028d25e)) * **lib/grandpa:** Include equivocatory nodes while creating justification ([#1911](#1911)) ([aca86b6](aca86b6)) * **lib/grandpa:** send NeighbourMessage to peers ([#1558](#1558)) ([322ccf9](322ccf9)) * **lib/runtime/wasmer:** implement ext_default_child_storage_storage_kill_version_3 ([#1878](#1878)) ([a719a60](a719a60)) * **lib/runtime/wasmer:** implement ext_offchain_local_storage_version_1 ([#1821](#1821)) ([0f63b17](0f63b17)) * **lib/runtime:** Implement `ext_offchain_http_request_add_header_version_1` host function ([#1994](#1994)) ([0a30b3d](0a30b3d)) * **lib/runtime:** Implement `ext_offchain_http_request_start_version_1` host function ([#1947](#1947)) ([974b1fc](974b1fc)) * **lib/runtime:** Implement `trie_blake2_256_verify_proof` host function ([#1920](#1920)) ([506565d](506565d)) * **lib/trie:** Implement `verify_proof` function ([#1883](#1883)) ([67bb5ef](67bb5ef)) * **lib/trie:** Implement limit for trie.ClearPrefix ([#1905](#1905)) ([becec9e](becec9e)) * **lib/trie:** Parallel hash trie. ([#1657](#1657)) ([22827e7](22827e7)) * **pprof:** Pprof HTTP server service ([#1991](#1991)) ([ce24ea9](ce24ea9)) * **rpc/subscription:** implement state_unsubscribeStorage ([#1574](#1574)) ([7574f10](7574f10)) * **rpc:** Implement `childstate_getChildStorage` RPC call ([#1832](#1832)) ([3d949f2](3d949f2)) * **rpc:** Implement `childstate_getStorageHash` RPC call ([#1805](#1805)) ([e539bd3](e539bd3)) * **rpc:** Implement `childstate_getStorageSize` RPC call ([#1810](#1810)) ([a04deb6](a04deb6)) * **rpc:** Implement `payment_queryInfo` RPC call ([#1826](#1826)) ([7a5deec](7a5deec)) * **rpc:** Implement `state_getReadProof` rpc call ([#1768](#1768)) ([865f80f](865f80f)) * **runtime:** implement custom logging handler that print function name ([#1825](#1825)) ([2b1276d](2b1276d)) * **telemetry:** send telemetry messages when GRANDPA receieves commit or vote messages ([#2015](#2015)) ([7bf40e1](7bf40e1)), closes [#1840](#1840) [#1839](#1839) [#1838](#1838) * **telemetry:** send txpool.import telemetry msg ([#1966](#1966)) ([ffc81bf](ffc81bf)) ### Reverts * Revert "feat(dot/rpc) implement `author_hasSessionKeys` RPC call (#1704)" (#1714) ([65380fd](65380fd)), closes [#1704](#1704) [#1714](#1714)

github-actions · 2021-12-03T21:07:05Z

🎉 This PR is included in version 0.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

noot added 4 commits October 28, 2021 17:58

add blockQueue logs, increase cap

273e4ce

move stuff, add logs

08508d6

remove GetBlockBody lock

58d6d3d

cleanup

4b6de75

noot requested review from arijitAD, EclesioMeloJunior, edwardmack, jimjbrettj, kishansagathiya, omar391 and timwu20 as code owners November 2, 2021 12:52

noot added 2 commits November 2, 2021 08:54

cleanup

6318263

Merge branch 'development' of github.com:ChainSafe/gossamer into noo…

80a2106

…t/fix-bootstrap

noot self-assigned this Nov 2, 2021

noot added the Status: Review Needed label Nov 2, 2021

qdm12 approved these changes Nov 2, 2021

View reviewed changes

EclesioMeloJunior approved these changes Nov 3, 2021

View reviewed changes

noot added 3 commits November 3, 2021 15:53

update handleFinalisedBlock to get store block in db, then delete fro…

2424b06

…m memory

Merge branch 'development' of github.com:ChainSafe/gossamer into noot…

54879ba

…/fix-bootstrap

fix lint

6cc0264

qdm12 approved these changes Nov 3, 2021

View reviewed changes

noot merged commit dd80c09 into development Nov 3, 2021

noot deleted the noot/fix-bootstrap branch November 3, 2021 20:54

timwu20 pushed a commit to timwu20/gossamer that referenced this pull request Dec 6, 2021

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

13c92ea

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

noot commented Nov 2, 2021

codecov bot commented Nov 2, 2021 •

edited

Loading

qdm12 Nov 2, 2021

noot Nov 2, 2021

qdm12 Nov 2, 2021

noot Nov 2, 2021

qdm12 Nov 2, 2021

noot Nov 2, 2021

qdm12 Nov 2, 2021

EclesioMeloJunior Nov 3, 2021

noot Nov 3, 2021

EclesioMeloJunior Nov 3, 2021

github-actions bot commented Dec 3, 2021

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

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

Conversation

noot commented Nov 2, 2021

Changes

Tests

Issues

Primary Reviewer

codecov bot commented Nov 2, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 3, 2021

codecov bot commented Nov 2, 2021 •

edited

Loading