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

Make initial block index loading faster #1420

Merged
merged 1 commit into from
Mar 11, 2024
Merged

Make initial block index loading faster #1420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 34 additions & 2 deletions src/txdb.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,16 @@ bool CBlockTreeDB::LoadBlockIndexGuts(boost::function<CBlockIndex*(const uint256
pcursor->Seek(std::make_pair(DB_BLOCK_INDEX, uint256()));

// Load mapBlockIndex

// We need to check PoW for the last N blocks. To do so we can't just save a pointer to the last block and go back
// from it because of possible forks. This multimap is used to track the most recent blocks (by height) saved in
// the block index on disk
std::multimap<int, CBlockIndex*> lastNBlocks;
// lowest height of all the elements in lastNBlocks
int firstInLastNBlocksHeight = 0;

bool fCheckPoWForAllBlocks = GetBoolArg("-fullblockindexcheck", DEFAULT_FULL_BLOCKINDEX_CHECK);

while (pcursor->Valid()) {
boost::this_thread::interruption_point();
std::pair<char, uint256> key;
Expand Down Expand Up @@ -406,8 +416,22 @@ bool CBlockTreeDB::LoadBlockIndexGuts(boost::function<CBlockIndex*(const uint256

pindexNew->activeDisablingSporks = diskindex.activeDisablingSporks;

if (!CheckProofOfWork(pindexNew->GetBlockPoWHash(), pindexNew->nBits, consensusParams))
return error("LoadBlockIndex(): CheckProofOfWork failed: %s", pindexNew->ToString());
if (fCheckPoWForAllBlocks) {
if (!CheckProofOfWork(pindexNew->GetBlockPoWHash(), pindexNew->nBits, consensusParams))
return error("LoadBlockIndex(): CheckProofOfWork failed: %s", pindexNew->ToString());
}
else {
if (pindexNew->nHeight >= firstInLastNBlocksHeight) {
lastNBlocks.insert(std::pair<int, CBlockIndex*>(pindexNew->nHeight, pindexNew));
if (lastNBlocks.size() > DEFAULT_BLOCKINDEX_NUMBER_OF_BLOCKS_TO_CHECK) {
// pop the first element from the map
auto firstElement = lastNBlocks.begin();
auto elementToPop = firstElement++;
lastNBlocks.erase(elementToPop);
firstInLastNBlocksHeight = firstElement->first;
}
}
}

pcursor->Next();
} else {
Comment on lines 416 to 437
Copy link

Choose a reason for hiding this comment

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

📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [359-452]

The modifications to CBlockTreeDB::LoadBlockIndexGuts introduce a conditional check for Proof of Work (PoW) based on the -fullblockindexcheck flag and the number of most recent blocks. While this optimization aims to improve the initial block index loading speed, there are several areas that require attention:

  • Correctness: The logic for selectively checking PoW for the most recent blocks appears sound. However, ensure thorough testing, especially around edge cases such as blockchain reorganizations that might affect the set of the most recent blocks.
  • Performance: This change could significantly impact startup times. It would be beneficial to quantify this impact through benchmarking under various scenarios (e.g., with and without the flag enabled, with different values for the number of blocks to check).
  • Maintainability: The added complexity to LoadBlockIndexGuts increases the cognitive load for future maintainers. Consider refactoring to improve readability and maintainability. For example, extracting the logic for managing lastNBlocks into separate functions could make the code more modular and easier to understand.
  • Error Handling: Ensure that any failures in PoW checks lead to clear and actionable error messages, aiding in troubleshooting potential issues during startup.
+ // Refactor to improve maintainability
+ // Benchmark to quantify performance impact
+ // Ensure thorough testing, especially around edge cases
+ // Improve error handling and messaging

Expand All @@ -418,6 +442,14 @@ bool CBlockTreeDB::LoadBlockIndexGuts(boost::function<CBlockIndex*(const uint256
}
}

if (!fCheckPoWForAllBlocks) {
// delayed check for all the blocks
for (const auto &blockIndex: lastNBlocks) {
if (!CheckProofOfWork(blockIndex.second->GetBlockPoWHash(), blockIndex.second->nBits, consensusParams))
return error("LoadBlockIndex(): CheckProofOfWork failed: %s", blockIndex.second->ToString());
}
}

return true;
}

Expand Down
5 changes: 5 additions & 0 deletions src/txdb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ static const int64_t nMaxBlockDBAndTxIndexCache = 1024;
//! Max memory allocated to coin DB specific cache (MiB)
static const int64_t nMaxCoinsDBCache = 8;

//! By default don't check block index PoW on client startup
static const bool DEFAULT_FULL_BLOCKINDEX_CHECK = false;
//! If not doing full check of block index, check only N of the latest blocks
static const int DEFAULT_BLOCKINDEX_NUMBER_OF_BLOCKS_TO_CHECK = 10000;
Comment on lines +46 to +49
Copy link

Choose a reason for hiding this comment

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

The introduction of DEFAULT_FULL_BLOCKINDEX_CHECK and DEFAULT_BLOCKINDEX_NUMBER_OF_BLOCKS_TO_CHECK constants is a significant change aimed at optimizing the initial block index loading process. However, there are a few considerations and recommendations:

  • Documentation: It's crucial to document these constants, explaining their purpose, usage, and the rationale behind the default values. This will aid in maintainability and understanding for future developers.
  • Configuration Flexibility: While the PR introduces a mechanism to control the depth of the block index check via a flag, consider exposing DEFAULT_BLOCKINDEX_NUMBER_OF_BLOCKS_TO_CHECK as a configurable parameter (e.g., via a configuration file or command-line argument). This would provide operators with greater flexibility in tuning the system based on their specific needs or constraints.
  • Validation: Ensure that there is validation in place for the DEFAULT_BLOCKINDEX_NUMBER_OF_BLOCKS_TO_CHECK value, especially if it becomes user-configurable. For example, negative values should be disallowed, and there might be a reasonable upper limit to consider.
+ // Documentation for DEFAULT_FULL_BLOCKINDEX_CHECK
+ // Documentation for DEFAULT_BLOCKINDEX_NUMBER_OF_BLOCKS_TO_CHECK
+ // Consider making DEFAULT_BLOCKINDEX_NUMBER_OF_BLOCKS_TO_CHECK configurable
+ // Add validation for DEFAULT_BLOCKINDEX_NUMBER_OF_BLOCKS_TO_CHECK
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
//! By default don't check block index PoW on client startup
static const bool DEFAULT_FULL_BLOCKINDEX_CHECK = false;
//! If not doing full check of block index, check only N of the latest blocks
static const int DEFAULT_BLOCKINDEX_NUMBER_OF_BLOCKS_TO_CHECK = 10000;
//! By default don't check block index PoW on client startup
// Documentation for DEFAULT_FULL_BLOCKINDEX_CHECK
static const bool DEFAULT_FULL_BLOCKINDEX_CHECK = false;
//! If not doing full check of block index, check only N of the latest blocks
// Documentation for DEFAULT_BLOCKINDEX_NUMBER_OF_BLOCKS_TO_CHECK
// Consider making DEFAULT_BLOCKINDEX_NUMBER_OF_BLOCKS_TO_CHECK configurable
// Add validation for DEFAULT_BLOCKINDEX_NUMBER_OF_BLOCKS_TO_CHECK
static const int DEFAULT_BLOCKINDEX_NUMBER_OF_BLOCKS_TO_CHECK = 10000;


struct CDiskTxPos : public CDiskBlockPos
{
unsigned int nTxOffset; // after header
Expand Down
Loading