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

kernel: Remove batchpriority from kernel library #30083

Merged
merged 1 commit into from
May 14, 2024
Merged

kernel: Remove batchpriority from kernel library #30083

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
1 change: 0 additions & 1 deletion src/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -975,7 +975,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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,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 @@ -1735,6 +1736,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
View file
Open in desktop
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
Copy link
Contributor

Choose a reason for hiding this comment

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

In commit "kernel: Remove batchpriority from kernel library" (04ffe40)

I think it might be useful to keep the // End scope of ImportNow comment here. If someone is adding new code at this point, it's possible they might want to add it with importing set to false, or importing set to true, so the comment would be a reminder that the value will change at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want to preserve the comment or the scoping? I was on the fence about this, but decided to drop the scoping after the introduced RAII wrappers for the ECC context, which don't scope either: https://github.com/bitcoin/bitcoin/pull/29252/files#diff-dbfadb3e0332664bff298a329b1d27065d2decbbe43fd391388af18f5861c114R17

Besides, I don't think it is likely that his function will grow. It is finally scoped in a reasonably limited way now. If a more functionality is added, it should probably go in another function and removing the scoping as done here encourages that.

Copy link
Contributor

@ryanofsky ryanofsky May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

re: #30083 (comment)

Whatever you prefer is good. I like getting rid of the nested scope, and the current PR is fine. I was just suggesting keeping the comment as a reminder that the import variable would be change to false at that point.

}
// End scope of ImportingNow
}

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