blockchain: Refactor db main chain idx to blk idx. #1332

davecgh · 2018-07-02T20:55:11Z

This refactors the code that relies on the database main chain index to use the in-memory block index now that the full index is in memory.

This is a major optimization for several functions because they no longer have to first consult the database (which is incredibly slow in many cases due to the way leveldb splits all of the information across files) to perform lookups and determine if blocks are in the main chain.

It should also be noted that even though the main chain index is no longer used as of this commit, the code which writes the main chain index entries to the database in connectBlock, disconnectBlock, and
createChainState have not been removed because, once that is done, it will no longer be possible to downgrade and thus those changes must be performed along with a database migration and associated version bump.

An overview of the changes are as follows:

Update all code which previously used the db to use the main chain by height map instead
Update several internal functions which previously accepted the hash to instead accept the block node as the parameter
- Rename fetchMainChainBlockByHash to fetchMainChainBlockByNode
- Rename fetchBlockByHash to fetchBlockByNode
- Rename dbFetchBlockByHash to dbFetchBlockByNode
- Update all callers to use the new names and semantics
Optimize HeaderByHeight to use block index/height map instead of db
- Move to chain.go since it no longer involves database I/O
Optimize BlockByHash to use block index instead of db
- Move to chain.go since it no longer involves database I/O
Optimize BlockByHeight to use block index/height map instead of db
- Move to chain.go since it no longer involves database I/O
Optimize MainChainHasBlock to use block index instead of db
- Move to chain.go since it no longer involves database I/O
- Removed error return since it can no longer fail
Optimize BlockHeightByHash to use block index instead of db
- Move to chain.go since it no longer involves database I/O
Optimize BlockHashByHeight to use block index instead of db
- Move to chain.go since it no longer involves database I/O
Optimize HeightRange to use block index instead of db
- Move to chain.go since it no longer involves database I/O
Remove several unused functions related to the main chain index
- dbFetchHeaderByHash
- DBFetchHeaderByHeight
- dbFetchBlockByHeight
- DBFetchBlockByHeight
- dbMainChainHasBlock
- DBMainChainHasBlock
Rework IsCheckpointCandidate to use block index
Modify the upgrade code to expose the old dbFetchBlockByHeight function only in the local scope since upgrades require the ability to read the old format
Update indexers to fetch the blocks themselves while only querying chain for chain-related details

This is work towards #1145.

davecgh · 2018-07-02T22:34:34Z

blockchain/chain.go

 		return err
 	})
 	return block, err
 }

-// fetchBlockByHash returns the block with the given hash from all known sources
+// fetchBlockByNode returns the block with the given hash from all known sources


No longer takes a hash. Comment needs update.

davecgh · 2018-07-02T22:36:02Z

blockchain/chain.go

 // such as the internal caches and the database.  This function returns blocks
 // regardless or whether or not they are part of the main chain.
 //
 // This function is safe for concurrent access.
-func (b *BlockChain) fetchBlockByHash(hash *chainhash.Hash) (*dcrutil.Block, error) {
+func (b *BlockChain) fetchBlockByNode(node *blockNode) (*dcrutil.Block, error) {
 	// Check orphan cache.


The order of the main and orphan cache checks here should probably be reversed because the chances the block is in the main chain cache is much higher than being an orphan.

davecgh · 2018-07-02T23:27:16Z

blockchain/chain.go

+	}
+
+	// Return the block from either cache or the database.  Note that this is
+	// note using fetchMainChainBlockByNode since the main chain check has


s/note/not/

dajohi

tOK testnet2 miner

alexlyp · 2018-07-03T15:31:20Z

Running smooth on testnet and did a successful idb on mainnet

matheusd · 2018-07-04T11:06:57Z

So far, so good on testnet.

USER       PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
decred    1442  2.0 25.4 944872 521308 pts/2   Sl+  Jul02  45:36 ./dcrd_dave_test

raedah · 2018-07-06T06:04:21Z

Running mainnet and testnet instances for the last 4 days with no issues found.

davecgh · 2018-07-06T15:12:06Z

blockchain/chain.go

+	}
+
+	// There is nothing to do when the start and end heights are the same,
+	// so return now to avoid the chain lock and a database transaction.


Comment needs to be updated since a database transaction is no longer needed.

jrick · 2018-07-06T15:23:22Z

blockchain/chain.go

-// fetchBlockByHash returns the block with the given hash from all known sources
-// such as the internal caches and the database.  This function returns blocks
-// regardless or whether or not they are part of the main chain.
+// fetchBlockByNode returns the block assocaited with the given node all known


associated is misspelled

jrick · 2018-07-06T15:34:30Z

blockchain/chain.go

+// height.  The end height will be limited to the current main chain height.
+//
+// This function is safe for concurrent access.
+func (b *BlockChain) HeightRange(startHeight, endHeight int64) ([]chainhash.Hash, error) {


perhaps document that this is the half open range [startHeight, endHeight)

It says It is inclusive of the start height and exclusive of the end height already. I figured that would make it clear. I'll go ahead and expand on it a bit to include the blurb you suggest.

jrick · 2018-07-06T15:37:57Z

blockchain/chain.go

+	b.heightLock.RLock()
+	iterNode := b.mainNodesByHeight[endHeight-1]
+	b.heightLock.RUnlock()
+	for i := startHeight; i < endHeight; i++ {


this would read better to me if it iterated backwards until reaching the start height

I'd rather not change this since it has already been well tested manually and reversing it could easily introduce an off by one. Since the blocklocator bits no longer use this function, the only remaining caller is from RPC handlers for which there are, unfortunately, no automated tests.

jrick · 2018-07-06T15:41:59Z

blockchain/chainquery.go

 	// Generate the results sorted by descending height.
 	sort.Sort(sort.Reverse(nodeHeightSorter(chainTips)))
 	results := make([]dcrjson.GetChainTipsResult, len(chainTips))
+	b.chainLock.RLock()


what is being protected by this mutex (why is being held longer here now?)

The inMainChain flag on the node is protected by the chain lock.

davecgh · 2018-07-06T16:16:38Z

Rebased to latest master and squashed all review commits.

This refactors the code that relies on the database main chain index to use the in-memory block index now that the full index is in memory. This is a major optimization for several functions because they no longer have to first consult the database (which is incredibly slow in many cases due to the way leveldb splits all of the information across files) to perform lookups and determine if blocks are in the main chain. It should also be noted that even though the main chain index is no longer used as of this commit, the code which writes the main chain index entries to the database in connectBlock, disconnectBlock, and createChainState have not been removed because, once that is done, it will no longer be possible to downgrade and thus those changes must be performed along with a database migration and associated version bump. An overview of the changes are as follows: - Update all code which previously used the db to use the main chain by height map instead - Update several internal functions which previously accepted the hash to instead accept the block node as the parameter - Rename fetchMainChainBlockByHash to fetchMainChainBlockByNode - Rename fetchBlockByHash to fetchBlockByNode - Rename dbFetchBlockByHash to dbFetchBlockByNode - Update all callers to use the new names and semantics - Optimize HeaderByHeight to use block index/height map instead of db - Move to chain.go since it no longer involves database I/O - Optimize BlockByHash to use block index instead of db - Move to chain.go since it no longer involves database I/O - Optimize BlockByHeight to use block index/height map instead of db - Move to chain.go since it no longer involves database I/O - Optimize MainChainHasBlock to use block index instead of db - Move to chain.go since it no longer involves database I/O - Removed error return since it can no longer fail - Optimize BlockHeightByHash to use block index instead of db - Move to chain.go since it no longer involves database I/O - Optimize BlockHashByHeight to use block index instead of db - Move to chain.go since it no longer involves database I/O - Optimize HeightRange to use block index instead of db - Move to chain.go since it no longer involves database I/O - Remove several unused functions related to the main chain index - dbFetchHeaderByHash - DBFetchHeaderByHeight - dbFetchBlockByHeight - DBFetchBlockByHeight - dbMainChainHasBlock - DBMainChainHasBlock - Rework IsCheckpointCandidate to use block index - Modify the upgrade code to expose the old dbFetchBlockByHeight function only in the local scope since upgrades require the ability to read the old format - Update indexers to fetch the blocks themselves while only querying chain for chain-related details

davecgh mentioned this pull request Jul 2, 2018

Multi-peer Checklist #1145

Open

28 tasks

davecgh commented Jul 3, 2018

View reviewed changes

dajohi approved these changes Jul 3, 2018

View reviewed changes

davecgh mentioned this pull request Jul 3, 2018

blockchain: Remove main chain index from db. #1334

Merged

davecgh added this to the 1.3.0 milestone Jul 5, 2018

davecgh commented Jul 6, 2018

View reviewed changes

jrick reviewed Jul 6, 2018

View reviewed changes

davecgh force-pushed the blockchain_convert_mainchain_index branch from 9814565 to 1788cc3 Compare July 6, 2018 16:16

jrick approved these changes Jul 6, 2018

View reviewed changes

davecgh force-pushed the blockchain_convert_mainchain_index branch from 1788cc3 to c6b5f6d Compare July 6, 2018 16:44

davecgh merged commit c6b5f6d into decred:master Jul 6, 2018

davecgh deleted the blockchain_convert_mainchain_index branch July 6, 2018 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

blockchain: Refactor db main chain idx to blk idx. #1332

blockchain: Refactor db main chain idx to blk idx. #1332

davecgh commented Jul 2, 2018

davecgh Jul 2, 2018

davecgh Jul 2, 2018

davecgh Jul 2, 2018

dajohi left a comment

alexlyp commented Jul 3, 2018

matheusd commented Jul 4, 2018

raedah commented Jul 6, 2018 •

edited

davecgh Jul 6, 2018

davecgh Jul 6, 2018

jrick Jul 6, 2018

davecgh Jul 6, 2018

jrick Jul 6, 2018

davecgh Jul 6, 2018

davecgh Jul 6, 2018

jrick Jul 6, 2018

davecgh Jul 6, 2018

jrick Jul 6, 2018

davecgh Jul 6, 2018

davecgh commented Jul 6, 2018

blockchain: Refactor db main chain idx to blk idx. #1332

blockchain: Refactor db main chain idx to blk idx. #1332

Conversation

davecgh commented Jul 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dajohi left a comment

Choose a reason for hiding this comment

alexlyp commented Jul 3, 2018

matheusd commented Jul 4, 2018

raedah commented Jul 6, 2018 • edited

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

Choose a reason for hiding this comment

davecgh commented Jul 6, 2018

raedah commented Jul 6, 2018 •

edited