refactor: Move stopafterblockimport option out of blockstorage

TheCharlatan commented at 10:40 am on July 8, 2023: contributor

This has the benefit of moving this StartShutdown call out of the blockstorage file and thus out of the kernel’s responsibility. The user can now decide if he wants to start shutdown / interrupt after a block import or not.

This also simplifies #28048, making it one fewer shutdown call to handle.

DrahtBot commented at 10:40 am on July 8, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	MarcoFalke, ryanofsky
Stale ACK	furszy

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28051 (Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly by ryanofsky)
#28048 (kernel: Remove StartShutdown calls from validation code by ryanofsky)
#27607 (index: make startup more efficient by furszy)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Refactoring on Jul 8, 2023

in src/init.cpp:1659 in 5867bb1ae7 outdated

1655@@ -1657,7 +1656,12 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1656     }
1657 
1658     chainman.m_load_block = std::thread(&util::TraceThread, "loadblk", [=, &chainman, &args] {
1659-        ThreadImport(chainman, vImportFiles, ShouldPersistMempool(args) ? MempoolPath(args) : fs::path{});
1660+        auto return_after_import = args.GetBoolArg("-stopafterblockimport", false);

furszy commented at 2:55 pm on July 8, 2023:

tiny nit: A bit more readable if it uses bool instead of auto. (and also, maybe a better name for the variable would be stop_after_import)

furszy approved

furszy commented at 2:58 pm on July 8, 2023: member

ACK 5867bb1a

bitcoinfinancier approved

DrahtBot added the label Needs rebase on Jul 10, 2023

ryanofsky approved

ryanofsky commented at 6:01 pm on July 10, 2023: contributor

Code review ACK 5867bb1ae7d1d345a5fd3b341d88ff777b6ef404. This looks good, and now can be rebased and simplified because #27607 is merged

TheCharlatan force-pushed on Jul 11, 2023

TheCharlatan commented at 9:29 am on July 11, 2023: contributor

Rebased 5867bb1ae7d1d345a5fd3b341d88ff777b6ef404 -> f87c21c1827fa532749ec17a2f44322378046b93 (blockImportReturn_0 -> blockImportReturn_1, compare)

Fixed conflict with #27607.
Dropped boolean argument for making the function return early.

in src/init.cpp:566 in f87c21c182 outdated

562@@ -564,7 +563,7 @@ void SetupServerArgs(ArgsManager& argsman)
563     argsman.AddArg("-checkmempool=<n>", strprintf("Run mempool consistency checks every <n> transactions. Use 0 to disable. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
564     argsman.AddArg("-checkpoints", strprintf("Enable rejection of any forks from the known historical chain until block %s (default: %u)", defaultChainParams->Checkpoints().GetHeight(), DEFAULT_CHECKPOINTS_ENABLED), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
565     argsman.AddArg("-deprecatedrpc=<method>", "Allows deprecated RPC method(s) to be used", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
566-    argsman.AddArg("-stopafterblockimport", strprintf("Stop running after importing blocks from disk (default: %u)", DEFAULT_STOPAFTERBLOCKIMPORT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
567+    argsman.AddArg("-stopafterblockimport", strprintf("Stop running after importing blocks from disk (default: %u)", false), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);

MarcoFalke commented at 9:53 am on July 11, 2023:

Any reason to change this? Is there any need to remove the named constant?

refactor: Move stopafterblockimport handling out of blockstorage

This has the benefit of moving the StartShutdown call out of the
blockstorage file and thus out of the kernel's responsibility. The user
can now decide if he wants to start shutdown / interrupt after a block
import or not.

462390c85f

TheCharlatan force-pushed on Jul 11, 2023

DrahtBot removed the label Needs rebase on Jul 11, 2023

TheCharlatan commented at 10:06 am on July 11, 2023: contributor

Updated f87c21c1827fa532749ec17a2f44322378046b93 -> 462390c85f1174b3cf83dfb0f74922049e5cb8af (blockImportReturn_1 -> blockImportReturn_2, compare)

Addressed @MarcoFalke’s comment, re-introduced named constant.

MarcoFalke approved

MarcoFalke commented at 10:11 am on July 11, 2023: member

lgtm ACK 462390c85f1174b3cf83dfb0f74922049e5cb8af 🗝

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK 462390c85f1174b3cf83dfb0f74922049e5cb8af  🗝
379pOWJJhRKkMSczcP5SRwipfYj5m6V7y44rmI0Q0c7UbllpH+RG/Ei49ay9gaV/dBEYrjhKR0IAUOAi4s+THDw==

DrahtBot requested review from furszy on Jul 11, 2023

DrahtBot requested review from ryanofsky on Jul 11, 2023

ryanofsky approved

ryanofsky commented at 1:29 pm on July 11, 2023: contributor

Code review ACK 462390c85f1174b3cf83dfb0f74922049e5cb8af. Just has been rebased and is a simpler change after #27607

ryanofsky merged this on Jul 11, 2023

ryanofsky closed this on Jul 11, 2023

sidhujag referenced this in commit 6491748930 on Jul 12, 2023

refactor: Move stopafterblockimport option out of blockstorage #28053

Reviews

Conflicts