Skip to content

Commit

Permalink
Merge bitcoin#30083: kernel: Remove batchpriority from kernel library
Browse files Browse the repository at this point in the history
d4b17c7 kernel: Remove batchpriority from kernel library (TheCharlatan)

Pull request description:

  The current usage of ScheduleBatchPriority is not transparent. Once the thread scheduling is changed, it remains unchanged for the remainder of the thread's lifetime. So move the call from `ImportBlocks` to the init code where it is clearer that its effect lasts for the entire lifetime of the thread.

  Users of the kernel library might not expect `ImportBlocks` to have an influence on the thread it is called in. Particularly since it is only a compile time option and cannot be controlled at runtime. With this patch users of the kernel library can now freely choose their own scheduling policy.

  This PR is easier reviewed with `git diff --color-moved-ws=ignore-all-space --color-moved=dimmed-zebra`

  ---
  This PR is part of the [libbitcoinkernel project](bitcoin#27587).

ACKs for top commit:
  maflcko:
    ACK d4b17c7 📭
  ryanofsky:
    Code review ACK d4b17c7, just added suggested comment since last review
  hebasto:
    ACK d4b17c7, I have reviewed the code and it looks OK.

Tree-SHA512: cafedecd9affad58ddd7f30f68bba71291ca951bb186ff4b2da04b7f21f0b26e5e3143846d032b9e391bd5ce6c7466b97aa3758d2a85ebd7353eb8b69139641a
  • Loading branch information
ryanofsky committed May 14, 2024
2 parents 7fcf4e9 + d4b17c7 commit dbb3113
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 57 deletions.
1 change: 0 additions & 1 deletion src/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -974,7 +974,6 @@ libbitcoinkernel_la_SOURCES = \
txdb.cpp \
txmempool.cpp \
uint256.cpp \
util/batchpriority.cpp \
util/chaintype.cpp \
util/check.cpp \
util/feefrac.cpp \
Expand Down
2 changes: 2 additions & 0 deletions src/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
#include <txdb.h>
#include <txmempool.h>
#include <util/asmap.h>
#include <util/batchpriority.h>
#include <util/chaintype.h>
#include <util/check.h>
#include <util/fs.h>
Expand Down Expand Up @@ -1741,6 +1742,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
}

chainman.m_thread_load = std::thread(&util::TraceThread, "initload", [=, &chainman, &args, &node] {
ScheduleBatchPriority();
// Import blocks
ImportBlocks(chainman, vImportFiles);
if (args.GetBoolArg("-stopafterblockimport", DEFAULT_STOPAFTERBLOCKIMPORT)) {
Expand Down
109 changes: 53 additions & 56 deletions src/node/blockstorage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1175,69 +1175,66 @@ class ImportingNow

void ImportBlocks(ChainstateManager& chainman, std::vector<fs::path> vImportFiles)
{
ScheduleBatchPriority();

{
ImportingNow imp{chainman.m_blockman.m_importing};

// -reindex
if (fReindex) {
int nFile = 0;
// Map of disk positions for blocks with unknown parent (only used for reindex);
// parent hash -> child disk position, multiple children can have the same parent.
std::multimap<uint256, FlatFilePos> blocks_with_unknown_parent;
while (true) {
FlatFilePos pos(nFile, 0);
if (!fs::exists(chainman.m_blockman.GetBlockPosFilename(pos))) {
break; // No block files left to reindex
}
AutoFile file{chainman.m_blockman.OpenBlockFile(pos, true)};
if (file.IsNull()) {
break; // This error is logged in OpenBlockFile
}
LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile);
chainman.LoadExternalBlockFile(file, &pos, &blocks_with_unknown_parent);
if (chainman.m_interrupt) {
LogPrintf("Interrupt requested. Exit %s\n", __func__);
return;
}
nFile++;
ImportingNow imp{chainman.m_blockman.m_importing};

// -reindex
if (fReindex) {
int nFile = 0;
// Map of disk positions for blocks with unknown parent (only used for reindex);
// parent hash -> child disk position, multiple children can have the same parent.
std::multimap<uint256, FlatFilePos> blocks_with_unknown_parent;
while (true) {
FlatFilePos pos(nFile, 0);
if (!fs::exists(chainman.m_blockman.GetBlockPosFilename(pos))) {
break; // No block files left to reindex
}
WITH_LOCK(::cs_main, chainman.m_blockman.m_block_tree_db->WriteReindexing(false));
fReindex = false;
LogPrintf("Reindexing finished\n");
// To avoid ending up in a situation without genesis block, re-try initializing (no-op if reindexing worked):
chainman.ActiveChainstate().LoadGenesisBlock();
AutoFile file{chainman.m_blockman.OpenBlockFile(pos, true)};
if (file.IsNull()) {
break; // This error is logged in OpenBlockFile
}
LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile);
chainman.LoadExternalBlockFile(file, &pos, &blocks_with_unknown_parent);
if (chainman.m_interrupt) {
LogPrintf("Interrupt requested. Exit %s\n", __func__);
return;
}
nFile++;
}

// -loadblock=
for (const fs::path& path : vImportFiles) {
AutoFile file{fsbridge::fopen(path, "rb")};
if (!file.IsNull()) {
LogPrintf("Importing blocks file %s...\n", fs::PathToString(path));
chainman.LoadExternalBlockFile(file);
if (chainman.m_interrupt) {
LogPrintf("Interrupt requested. Exit %s\n", __func__);
return;
}
} else {
LogPrintf("Warning: Could not open blocks file %s\n", fs::PathToString(path));
WITH_LOCK(::cs_main, chainman.m_blockman.m_block_tree_db->WriteReindexing(false));
fReindex = false;
LogPrintf("Reindexing finished\n");
// To avoid ending up in a situation without genesis block, re-try initializing (no-op if reindexing worked):
chainman.ActiveChainstate().LoadGenesisBlock();
}

// -loadblock=
for (const fs::path& path : vImportFiles) {
AutoFile file{fsbridge::fopen(path, "rb")};
if (!file.IsNull()) {
LogPrintf("Importing blocks file %s...\n", fs::PathToString(path));
chainman.LoadExternalBlockFile(file);
if (chainman.m_interrupt) {
LogPrintf("Interrupt requested. Exit %s\n", __func__);
return;
}
} else {
LogPrintf("Warning: Could not open blocks file %s\n", fs::PathToString(path));
}
}

// scan for better chains in the block chain database, that are not yet connected in the active best chain
// scan for better chains in the block chain database, that are not yet connected in the active best chain

// We can't hold cs_main during ActivateBestChain even though we're accessing
// the chainman unique_ptrs since ABC requires us not to be holding cs_main, so retrieve
// the relevant pointers before the ABC call.
for (Chainstate* chainstate : WITH_LOCK(::cs_main, return chainman.GetAll())) {
BlockValidationState state;
if (!chainstate->ActivateBestChain(state, nullptr)) {
chainman.GetNotifications().fatalError(strprintf(_("Failed to connect best block (%s)."), state.ToString()));
return;
}
// We can't hold cs_main during ActivateBestChain even though we're accessing
// the chainman unique_ptrs since ABC requires us not to be holding cs_main, so retrieve
// the relevant pointers before the ABC call.
for (Chainstate* chainstate : WITH_LOCK(::cs_main, return chainman.GetAll())) {
BlockValidationState state;
if (!chainstate->ActivateBestChain(state, nullptr)) {
chainman.GetNotifications().fatalError(strprintf(_("Failed to connect best block (%s)."), state.ToString()));
return;
}
} // End scope of ImportingNow
}
// End scope of ImportingNow
}

std::ostream& operator<<(std::ostream& os, const BlockfileType& type) {
Expand Down

0 comments on commit dbb3113

Please sign in to comment.