refactor: Remove almost all validation option globals

maflcko commented at 8:36 am on July 26, 2022: member

It seems preferable to assign globals to a class (in this case ChainstateManager), than to leave them dangling. This should clarify scope for code-readers, as well as clarifying unit test behaviour.

fanquake added the label Refactoring on Jul 26, 2022

glozow commented at 9:56 am on July 26, 2022: member

nice, Concept ACK

DrahtBot commented at 5:34 pm on July 26, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26251 (refactor: add cs_main.h by fanquake)
#25862 (refactor, kernel: Remove gArgs accesses from dbwrapper and txdb by ryanofsky)
#25740 (assumeutxo: background validation completion by jamesob)
#25193 (indexes: Read the locator’s top block during init, allow interaction with reindex-chainstate by mzumsande)

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.

in src/kernel/chainstatemanager_opts.h:17 in 1111ff1008 outdated

 9 #include <cstdint>
10 #include <functional>
11 
12 class CChainParams;
13 
14+using namespace std::chrono_literals;

ryanofsky commented at 8:45 pm on August 2, 2022:

In commit “Move ::nMaxTipAge into ChainstateManager” (1111ff10084e4446c2fbcaff65e6f0aa2a66a83e)

Was going to suggest it would be better to avoid using namespace at global level in a header file, since it means anything that directly or indirectly includes the header has now has std symbols in the global namespace.

But I guess src/qt/guiconstants.h and src/util/time.h already do this so maybe we don’t care.

maflcko commented at 7:14 am on August 3, 2022:

Jup, that was the goal of src/util/time.h. Happy to switch to using that include. Alternatively I can review a pull that changes this if there is need to and someone creates one.

ryanofsky commented at 3:34 pm on August 4, 2022:

Jup, that was the goal of src/util/time.h. Happy to switch to using that include. Alternatively I can review a pull that changes this if there is need to and someone creates one.

I’d go the opposite direction. I think it’s better to say using namespace std::chrono_literals; than include time.h, which just makes it harder to understand where the literals are coming from. IMO it would be best to delete using namespace std::chrono_literals; from time.h and add it explicitly to any file that use literals.

My initial reaction was an objection to having using namespace in a header file at all since it is common advice “Namespace using declarations should never appear in header files” for reasons described https://stackoverflow.com/a/37376171. But these reasons do not really apply to namespaces specifically reserved for literals like chrono_literals.

maflcko commented at 3:38 pm on August 4, 2022:

Happy to push any diff here or review a pull, if there is a motivation for the change.

maflcko commented at 9:43 am on August 22, 2022:

Removed the namespace (though, conceptually I did the opposite of what you asked for)

ryanofsky approved

ryanofsky commented at 8:53 pm on August 2, 2022: contributor

Code review ACK fa82b44ca5d059c8db604af9d76e2ae1c099554f

DrahtBot added the label Needs rebase on Aug 3, 2022

maflcko force-pushed on Aug 3, 2022

DrahtBot removed the label Needs rebase on Aug 3, 2022

glozow commented at 9:01 am on August 4, 2022: member

Seeing conflicts with a couple kernel PRs, so pinging @dongcarl to see if this interferes with anything

DrahtBot added the label Needs rebase on Aug 4, 2022

maflcko renamed this:
~~refactor: Remove all validation option globals~~
refactor: Remove almost all validation option globals
on Aug 4, 2022

maflcko force-pushed on Aug 5, 2022

DrahtBot removed the label Needs rebase on Aug 5, 2022

dongcarl commented at 11:14 pm on August 11, 2022: contributor

Concept and light Code Review ACK, very satisfying PR.

dongcarl commented at 5:58 pm on August 16, 2022: contributor

Code Review ACK fa738b8a988dd0e0700df9ca11a7819a2784546b

Shuffling between AppInitParameterInteraction and AppInitMain seem safe because the code being moved only logically depend on static variables and chainparams, which is const throughout and set before AppInitParameterInteraction and AppInitMain are called.

Followups:

Have ChainstateManager own an std::optional scriptcheckqueue so there’s a single (local) source of truth for whether or not to enable parallel script checks
Add a ApplyArgsManOptions for ChainstateManager::Options (already done in #25623, but needs to incorporate the new members introduced in this PR)

Marco: Any reason these commits are “Unverified”?

DrahtBot added the label Needs rebase on Aug 17, 2022

maflcko force-pushed on Aug 19, 2022

DrahtBot removed the label Needs rebase on Aug 19, 2022

maflcko commented at 2:02 pm on August 19, 2022: member

Followups:

Good idea.

Marco: Any reason these commits are “Unverified”?

Yes, this is a stick I threw in the mud so that you can’t trust Microsoft/GitHub to mark my commits as verified. If you care about this, you can verify it locally, but it seems no one does that anyway.

maflcko force-pushed on Aug 22, 2022

in src/init.cpp:1409 in fa6744c42e outdated

1403@@ -1403,6 +1404,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1404         const ChainstateManager::Options chainman_opts{
1405             .chainparams = chainparams,
1406             .adjusted_time_callback = GetAdjustedTime,
1407+            .max_tip_age = std::chrono::seconds{args.GetIntArg("-maxtipage", Ticks<std::chrono::seconds>(DEFAULT_MAX_TIP_AGE))},

ryanofsky commented at 5:59 pm on August 22, 2022:

In commit “Move ::nMaxTipAge into ChainstateManager” (fa6744c42e8bafd2769dd90e4b00af92e5dd4fe0)

Instead converting DEFAULT_MAX_TIP_AGE from a duration to an int64 then back to a duration, might be more type-safe to write this as:

0-            .max_tip_age = std::chrono::seconds{args.GetIntArg("-maxtipage", Ticks<std::chrono::seconds>(DEFAULT_MAX_TIP_AGE))},
1+            .max_tip_age = std::optional<std::chrono::seconds>{args.GetIntArg("-maxtipage")}.value_or(DEFAULT_MAX_TIP_AGE),

maflcko commented at 1:02 pm on August 26, 2022:

Thx, reworked

in src/validation.h:882 in fa6744c42e outdated

878@@ -881,6 +879,9 @@ class ChainstateManager
879      */
880     RecursiveMutex& GetMutex() const LOCK_RETURNED(::cs_main) { return ::cs_main; }
881 
882+    //! If the tip is older than this, the node is considered to be in initial block download.

ryanofsky commented at 6:04 pm on August 22, 2022:

In commit “Move ::nMaxTipAge into ChainstateManager” (fa6744c42e8bafd2769dd90e4b00af92e5dd4fe0)

If rebasing on #25905, would be nice to move this comment to the options struct to preserve it

maflcko commented at 10:09 am on August 26, 2022:

thx, rebased

in src/init.cpp:1352 in fa50c08f34 outdated

1350@@ -1365,6 +1351,20 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1351     } else {
1352         LogPrintf("Validating signatures for all blocks.\n");
1353     }
1354+    arith_uint256 nMinimumChainWork;
1355+    if (args.IsArgSet("-minimumchainwork")) {

ryanofsky commented at 6:25 pm on August 22, 2022:

In commit “Move ::nMinimumChainWork into ChainstateManager” (fa50c08f34ba5ae9f5d9c5df76542f87cb859f54)

Throughout the PR, instead of moving -checkblockindex/ / -checkpoints/ / -assumevalid/ / -minimumchainwork/ / -maxtipage options parsing code from AppInitParameterInteraction function to AppInitMain, I think it would be better to move it to a function that only parses arguments (node::ReadValidationArgs(const ArgsManager& args, ChainstateManagerOpts& options) or similar). As long as the code needs to move anyway, better to move it to a place where it can be indepedently called and tested and doesn’t complicate AppInitMain.

maflcko commented at 1:03 pm on August 26, 2022:

Thx, reworked

in src/kernel/chainstatemanager_opts.h:33 in fa39cf85d4 outdated

26@@ -27,6 +27,7 @@ namespace kernel {
27 struct ChainstateManagerOpts {
28     const CChainParams& chainparams;
29     const std::function<NodeClock::time_point()> adjusted_time_callback{nullptr};
30+    const bool parallel_script_checks{false};

ryanofsky commented at 6:40 pm on August 22, 2022:

In commit “Move ::g_parallel_script_checks into ChainstateManager” (fa39cf85d411e763d77f85d1e860e624e4a8e18b)

Not directly relevant to this PR (unless you want to add a todo comment), but since ChainstateManagerOpts is a kernel struct exposed to bitcoin-chainstate.cpp, probably it doesn’t make sense to expose a boolean parallel_script_checks kernel option which is separate option than the number of script verification threads. Better to have one option and avoid inconsistencies.

maflcko commented at 10:12 am on August 26, 2022:

Yes, leaving as independent change, that can be done before or after the changes in this pull request

maflcko commented at 1:03 pm on August 26, 2022:

nvm, reworked here

ryanofsky approved

ryanofsky commented at 6:44 pm on August 22, 2022: contributor

Code review ACK fa39cf85d411e763d77f85d1e860e624e4a8e18b. Good to remove globals. I made some suggestions, but none of them are critical. This could be rebased on top of #25905, but it would also not be difficult to rebase #25905 on top of this.

DrahtBot added the label Needs rebase on Aug 25, 2022

maflcko force-pushed on Aug 26, 2022

DrahtBot removed the label Needs rebase on Aug 26, 2022

maflcko force-pushed on Aug 26, 2022

in src/kernel/chainstatemanager_opts.h:35 in fa08f7aec6 outdated

24@@ -24,6 +25,8 @@ namespace kernel {
25 struct ChainstateManagerOpts {
26     const CChainParams& chainparams;
27     const std::function<NodeClock::time_point()> adjusted_time_callback{nullptr};
28+    /** Block hash whose ancestors we will assume to have valid scripts without checking them. */
29+    uint256 assumed_valid_block;

ryanofsky commented at 3:13 pm on August 26, 2022:

In commit “Move ::hashAssumeValid into ChainstateManager” (fa08f7aec6418abe78490b3d3350330e8959b336)

This is probably something that should be addressed in followup, rather than this PR, but I think as a general rule, libbitcoinkernel should try to have the same defaults as bitcoind, but in this case there’s a difference. By default bitcoind will set this to chainparams.GetConsensus().defaultAssumeValid and use that block, but libbitcoinkernel will leave this null and validate all blocks.

I think a good way to address this would be to change the type from uint256 to optional<uint256>, let IsNull() and block hash values keep their current meanings, and if the option is not set, let nullopt make validation code use the chainparams defaultAssumeValid value.

If you don’t want a change like that in this PR, I think it would be good to at least clarify the comment, by adding a note that caller can manually set this to CChainParams.GetConsensus().defaultAssumeValid() if they want default validation behavior. It might also be good to do set this in the bitcoin-chainstate example, though that would be a change in behavior there.

maflcko commented at 9:18 am on August 29, 2022:

I think it would be best if the libbitcoinkernel context initialization took care of initializing the value instead of turning all validation globals into tristates and all places where they are used, rely on ternary operators.

Thus, I think it is best to keep this as uint256.

ryanofsky commented at 7:24 pm on August 30, 2022:

re: #25704 (review)

I think it would be best if the libbitcoinkernel context initialization took care of initializing the value instead of turning all validation globals into tristates and all places where they are used, rely on ternary operators.

Thus, I think it is best to keep this as uint256.

I’d also try avoid using ternary operators in places where options are used and instead define accessor methods on chain or block classes to combine validation options & chain params as needed.

And I’m not suggesting turning all validation options into tristates. But if a validation option is supposed to override a chainparam value (or more generally, if a validation option is supposed to have a nonconstant default value), I think representing that option with std::optional is a better choice than hardcoding it with some incorrect default value different from the real default bitcoind would use. Using std::optional would avoid defining struct fields that have misleading default values, make validation code more self-contained by computing option values in kernel code instead of node or init code, and make kernel API simpler and harder to misuse.

In any case, this suggestion could considered for a followup, since this PR is just moving global variables into a struct, and doesn’t need to change variable types or libbitcoinkernel default behavior at the same time. The struct fields could be better documented to suggest better defaults, but it’s not like the old global variables had good documentation either :shrug:

maflcko commented at 7:34 am on September 7, 2022:

Ok, I can see how std::optional is helpful. Though, I’d prefer to at least wrap the ternary operators into a member function. Maybe I should add a

const uint256&  ChainstateManager::AssumedValidBlock() const { return m_settings.assumed_valid_block ? *m_settings.assumed_valid_block : GetConsensus().defaultAssumeValid; }

?

theuni commented at 8:21 pm on September 7, 2022:

FYI when @dongcarl and I discussed the generic pattern to use for overrides a few months back, I played around with (and liked) communicating via std::optional as well, like here: https://github.com/theuni/bitcoin/commit/7fad1352fb1aea12c41dfb8fccd8b220a58b694d

Though, I’d prefer to at least wrap the ternary operators into a member function. Maybe I should add a const uint256& ChainstateManager::AssumedValidBlock() const { return m_settings.assumed_valid_block ? *m_settings.assumed_valid_block : GetConsensus().defaultAssumeValid; }?

+1 for the convenient short-hand.

ryanofsky commented at 8:27 pm on September 7, 2022:

I’d prefer to at least wrap the ternary operators into a member function. Maybe I should add a const uint256& ChainstateManager::AssumedValidBlock() const { return m_settings.assumed_valid_block ? *m_settings.assumed_valid_block : GetConsensus().defaultAssumeValid; }?

Yes that’s what I meant by “define accessor methods on chain or block classes to combine validation options & chain params as needed”

maflcko commented at 7:45 pm on September 8, 2022:

Hmm, on a second though I am wondering if this is a good idea. std::optional has cmp functions that accept std::nullopt (https://en.cppreference.com/w/cpp/utility/optional/operator_cmp). Thus, it wouldn’t be a compile failure to not use the accessor method.

It might be better to “flatten” the options while storing them in m_options in the chainstatemanager constructor.

ryanofsky commented at 8:46 pm on September 8, 2022:

I’m not sure what issue you’re referring to with the comparison functions. I don’t think you ever need to call them.

In the AssumedValidBlock example above, you’re returning a uint256 not an optional<uint256>

maflcko commented at 5:57 am on September 9, 2022:

The compare functions are used to check the minimum chain work. But it may also happen for other types that can be compared. For example, if there is an optional<int> num, with a chainparams fallback. A dev might accidentally type if (m_options.num > 4) instead of if (Num() > 4), thus not using the fallback. Since the code compiles (and looks plausible), this would likely not be caught during review.

ryanofsky commented at 1:09 pm on September 9, 2022:

That’s fair, but what you are describing sounds like a reason to avoid std::optional class entirely in sensitive validation or wallet code for types that can be compared. It would apply just as much to internal state as external options (maybe even more) and would probably suggest that we should stop using std::optional in existing code.

I think it’s a reasonable concern, just not a concern that’s unique to this situation or extra compelling here. One way to address it would be to use std::variant instead of std::optional like:

 0struct UseChainDefault {};
 1
 2struct Options {
 3  std::variant<uint256, UseChainDefault> min_work = UseChainDefault{};
 4};
 5
 6uint256 GetMinWork()
 7{
 8    return std::holds_alternative<UseChainDefault>(m_options.min_work)
 9        ? m_params.GetConsensus().nMinimumChainWork
10        : std::get<uint256>(m_options.min_work);
11};

maflcko commented at 1:18 pm on September 9, 2022:

I am actually thinking that we should disallow the compare operator on std::optional. However, that is a separate topic.

Here, I’d simply “flatten” the optionals in the chainstatemanager constructor. Meaning the type will remain std::optional<bla>, however the optional is guaranteed to hold a value (either the previous value or a copy of the chain default value)

ryanofsky commented at 1:34 pm on September 9, 2022:

Oh, I wasn’t sure what you meant with flattening, but that’s a reasonable approach. I don’t love it because I think immutability is nice, so data is passed around verbatim, and you can set a value in one part of the code, and read it in another part of the code, and not have to worry about hidden code in the middle tweaking things. But this is nitpicking. The main goal is just to have an options struct that uses existing bitcoind default values and doesn’t introduce a new set of default behaviors.

dongcarl commented at 9:21 pm on September 9, 2022:

Got the summon from @theuni, skimmed the comments here… I don’t particularly care either way but I think an alternative is to discard the concept of ChainstateManagerOpts when we construct ChainstateManger:

0struct ChainstateManagerOpts {
1    const CChainParams& chainparams;
2...
3    std::optional<uint256> assumed_valid_block;
4...
5}

And have the ChainstateManager resolve this logic in its constructor, but without storing a m_options:

0ChainstateManager::ChainstateManager(Options options)
1    : m_effective_assume_valid_block{
2          options.assumed_valid_block.value_or(options.chainparams.GetConsensus().defaultAssumeValid)
3      }, ...

That way past the ChainstateManager ctor boundary there’s no confusion about what to use.

These *Opts structs don’t really need to be (and in a lot of cases shouldn’t be) stored as-is in their corresponding classes , classes can do smart things with them in their constructors and discard the structure.

ryanofsky commented at 11:01 pm on September 9, 2022:

These *Opts structs don’t really need to be (and in a lot of cases shouldn’t be) stored as-is in their corresponding classes

I disagree with this. Yes, it is possible to come up with different representations of the same options, transforming them from one state to another, with different representations optimized to take up less space or avoid recomputing values, or provide more type safety.

But I think this overcomplicates things, which is why I added the m_options struct in #25905 and am encouraging #25862 as an alternative to #25623. IMO, it is best to choose to choose one reasonable representation of options, stop transforming them from one state to another, come to peace with the fact that no single representation will be the most convenient or safest or most efficient form for every stage of the application, but that the simplicity of having one representation is better than the alternative of introducing boilerplate code in lots of places to copy or transform option values.

I think this is true for most options, allowing that there could be some exceptions.

maflcko commented at 12:30 pm on September 10, 2022:

Fixed

in src/node/chainstatemanager_args.cpp:17 in fa08f7aec6 outdated

11@@ -12,6 +12,13 @@
12 namespace node {
13 void ApplyArgsManOptions(const ArgsManager& args, ChainstateManager::Options& opts)
14 {
15+    opts.assumed_valid_block = uint256S(args.GetArg("-assumevalid", opts.chainparams.GetConsensus().defaultAssumeValid.GetHex()));
16+    if (!opts.assumed_valid_block.IsNull()) {
17+        LogPrintf("Assuming ancestors of block %s have valid signatures.\n", opts.assumed_valid_block.GetHex());

ryanofsky commented at 3:31 pm on August 26, 2022:

In commit “Move ::hashAssumeValid into ChainstateManager” (fa08f7aec6418abe78490b3d3350330e8959b336)

This is fine for now, but I wonder if ultimately we want to have log prints about important options in the command line parsing code, rather than places options are actually applied. These prints seem like they would be helpful to have in libitcoinkernel applications, not just in bitcoind, but now they are only in bitcoind. Could be better to move them someplace like LoadChainstate or InitializeChainstate later so they show up consistently when they are used.

maflcko commented at 10:56 am on August 29, 2022:

Thanks, done in https://github.com/bitcoin/bitcoin/pull/25951

in src/kernel/chainstatemanager_opts.h:31 in fa4d9f2229 outdated

26@@ -27,6 +27,7 @@ namespace kernel {
27 struct ChainstateManagerOpts {
28     const CChainParams& chainparams;
29     const std::function<NodeClock::time_point()> adjusted_time_callback{nullptr};
30+    bool check_block_index{false};

ryanofsky commented at 3:45 pm on August 26, 2022:

In commit “Move ::fCheckBlockIndex into ChainstateManager” (fa4d9f22291bbf23683b7daab20da63c6974af42)

The earlier comment about assumed_valid_block also applies to check_block_index. Instead of bitcoind and libbitcoinkernel using the same defaults, bitcoind defaults to chainparams.DefaultConsistencyChecks() while libbitcoinkernel defaults to false. Would be good to add a comment here mentioning that this could be set to the CChainParams.DefaultConsistencyChecks() value. Or to make this a tristate std::optional<bool> where true and false have current behavior and nullopt falls back to the chainparams.

maflcko commented at 10:58 am on August 29, 2022:

Same as above. I think it would be best to add a convenience function for libbitcoinkernel users to init the values here instead of using tristates everywhere.

ryanofsky commented at 7:26 pm on August 30, 2022:

Same as above

(thread is #25704 (review))

in src/validation.h:690 in fa4d9f2229 outdated

686@@ -688,7 +687,7 @@ class CChainState
687     /**
688      * Make various assertions about the state of the block index.
689      *
690-     * By default this only executes fully when using the Regtest chain; see: fCheckBlockIndex.
691+     * By default this only executes fully when using the Regtest chain; see: m_check_block_index.

ryanofsky commented at 3:47 pm on August 26, 2022:

In commit “Move ::fCheckBlockIndex into ChainstateManager” (fa4d9f22291bbf23683b7daab20da63c6974af42)

Stale comment, could say check_block_index option or m_options.check_block_index

maflcko commented at 11:11 am on August 29, 2022:

Thanks, fixed in the last force push

in src/kernel/chainstatemanager_opts.h:32 in facf86b84b outdated

26@@ -27,6 +27,11 @@ namespace kernel {
27 struct ChainstateManagerOpts {
28     const CChainParams& chainparams;
29     const std::function<NodeClock::time_point()> adjusted_time_callback{nullptr};
30+    /**
31+     * How many dedicated script-checking threads are running.
32+     * 0 indicates all script checking is done on the main threadMessageHandler thread.

ryanofsky commented at 3:57 pm on August 26, 2022:

In commit “Move ::g_parallel_script_checks into ChainstateManager” (facf86b84be761ae7c9885397c52d47d5574c32c)

Similar to earlier comments, I think this option exposes differences between bitcoind and libbitcoinkernel behavior that are possible footguns, and could be documented better.

I’d expand the comment to say that applications changing this value need to first call StartScriptCheckWorkerThreads with some number of threads, and then set this option to the same value.

In the future it would be good to change the kernel to read this option and call StartScriptCheckWorkerThreads directly. Also to change the option type from int to optional<int> so libbitcoinkernel applications and bitcoind can have the same default behavior.

maflcko commented at 11:05 am on August 29, 2022:

In the future it would be good to change the kernel to read this option and call StartScriptCheckWorkerThreads directly.

I wonder where this should be done, ideally. Doing it in the for loop that constructs the chainstatemanager and calls LoadChainstate, … may cause it being called several times, as it is a loop. Though, we only want to start the threads once.

ryanofsky commented at 7:41 pm on August 30, 2022:

re: #25704 (review)

I wonder where this should be done, ideally. Doing it in the for loop that constructs the chainstatemanager and calls LoadChainstate, … may cause it being called several times, as it is a loop. Though, we only want to start the threads once.

Maybe a simpler approach is to drop the script_check_threads variable and use scriptcheckqueue.m_worker_threads.size() in validation code instead.

maflcko commented at 5:34 am on September 7, 2022:

(this was done, so closing thread for now)

ryanofsky approved

ryanofsky commented at 4:08 pm on August 26, 2022: contributor

Code review ACK facf86b84be761ae7c9885397c52d47d5574c32c. This all looks good. It does expose some differences between default validation behavior in bitcoind vs default validation behavior using the kernel API that I think would be good to fix or document better (see comments below). But these differences already existed, and previously you would have to fix them by overridding obscure global variables, and now at least you can set them as options.

maflcko force-pushed on Aug 29, 2022

ryanofsky approved

ryanofsky commented at 7:46 pm on August 30, 2022: contributor

Code review ACK faf47bf54b8f73ebb523a81a7f5f782f7b82b1fb. Only change since last review is updating a code comment as suggested

maflcko referenced this in commit 36e1b52511 on Sep 1, 2022

DrahtBot added the label Needs rebase on Sep 1, 2022

sidhujag referenced this in commit e5ead4fe3d on Sep 1, 2022

maflcko force-pushed on Sep 2, 2022

DrahtBot removed the label Needs rebase on Sep 2, 2022

in src/init.cpp:941 in fa844d25eb outdated

923@@ -924,16 +924,6 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
924     fCheckBlockIndex = args.GetBoolArg("-checkblockindex", chainparams.DefaultConsistencyChecks());
925     fCheckpointsEnabled = args.GetBoolArg("-checkpoints", DEFAULT_CHECKPOINTS_ENABLED);
926 
927-    if (args.IsArgSet("-minimumchainwork")) {
928-        const std::string minChainWorkStr = args.GetArg("-minimumchainwork", "");
929-        if (!IsHexNumber(minChainWorkStr)) {
930-            return InitError(strprintf(Untranslated("Invalid non-hex (%s) minimum chain work value specified"), minChainWorkStr));

ryanofsky commented at 8:40 pm on September 6, 2022:

In commit “Move ::nMinimumChainWork into ChainstateManager” (fa844d25eb1cfb081b81a322ef9ba656ffba3de9)

One not great thing about this change is it delays validation of this option until after bitcoind daemonizes. So bitcoind service could appear to start successfully returning exit code 0, and then immediately fail. I think more ideally AppInitParameterInteraction would continue to validate options and report errors early.

It may be a good idea to add a util::Result<ChainstateManager::Options> MakeChainManOpts(ArgsManager&) helper function that could be called from both AppInitParameterInteraction and AppInitMain to validate options earlier and use them later.

maflcko commented at 7:19 am on September 7, 2022:

This is also true for all other settings, for example -maxmempool, -maxuploadtarget, … . AppInitParameterInteraction was only meant for setting interaction, not really for parsing. If there should be an effort to parse all settings before daemonization, I think this should be done separate from the changes here?

maflcko commented at 7:41 am on September 7, 2022:

MakeChainManOpts

An alternative would be to do the parsing only once and put the result into some kind of context (the existing kernel context or a new init context)?

ryanofsky commented at 1:55 pm on October 5, 2022:

re: #25704 (review)

Yes. Personally I would favor just calling the parsing function twice, if it avoids the need to track more application state. But both would be ways of providing earlier error feedback before daemonizing, so could be good for a future improvement.

maflcko commented at 4:43 pm on October 5, 2022:

Thanks, calling twice

ryanofsky approved

ryanofsky commented at 8:50 pm on September 6, 2022: contributor

Code review ACK fa158ab083c6f6d1e58a6aca10db5677695d803b. Main change is dropping the script_check_threads option and rebasing.

maflcko force-pushed on Sep 10, 2022

TheCharlatan approved

TheCharlatan commented at 2:03 pm on September 18, 2022: contributor

Code review ACK fa59bba008f72d8e73b171b549bd76682df811b5

DrahtBot added the label Needs rebase on Oct 4, 2022

maflcko force-pushed on Oct 5, 2022

DrahtBot removed the label Needs rebase on Oct 5, 2022

in src/validation.cpp:5244 in faa6743e59 outdated

5113@@ -5115,6 +5114,20 @@ void ChainstateManager::MaybeRebalanceCaches()
5114     }
5115 }
5116 
5117+/**
5118+ * Apply default chain params to nullopt members.

ryanofsky commented at 1:45 pm on October 5, 2022:

In commit “Move ::hashAssumeValid into ChainstateManager” (faa6743e59d201c1973c2f1f55fc6e43a1c807db)

Might be nice to move your comment “This helps to avoid coding errors…” into this commit, instead of later commit “Move ::nMinimumChainWork into ChainstateManager” (fa02eb1354ff16e91ab6b9bdb362be0de24f1750)

maflcko commented at 4:43 pm on October 5, 2022:

Thanks, added comment earlier

in src/validation.cpp:5128 in faa6743e59 outdated

5123+    return std::move(opts);
5124+}
5125+
5126+ChainstateManager::ChainstateManager(Options options) : m_options{Flatten(std::move(options))}
5127+{
5128+    Assert(m_options.adjusted_time_callback);

ryanofsky commented at 1:49 pm on October 5, 2022:

In commit “Move ::hashAssumeValid into ChainstateManager” (faa6743e59d201c1973c2f1f55fc6e43a1c807db)

Maybe move this Assert from ChainstateManager::ChainstateManager to the Flatten function. Seem like it might fit better there if goal of Flatten is to make sure every option has a value assigned.

maflcko commented at 4:43 pm on October 5, 2022:

Thanks, moved Assert

in src/checkqueue.h:202 in fa227bd60e outdated

198@@ -199,11 +199,15 @@ class CCheckQueue
199         WITH_LOCK(m_mutex, m_request_stop = false);
200     }
201 
202+    bool HasThreads()

aureleoules commented at 1:56 pm on October 5, 2022:

nit in fab08eff123b89ecd32a2db9995e084c2e6fb82c

0    bool HasThreads() const

maflcko commented at 4:43 pm on October 5, 2022:

Thanks, made const

aureleoules approved

aureleoules commented at 2:03 pm on October 5, 2022: member

ACK fa227bd60e739e6fb35619288e0f5ae535f52b9a Changes look good to me, left a nit.

in src/init.cpp:1385 in fa02eb1354 outdated

1381@@ -1392,7 +1382,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1382         .chainparams = chainparams,
1383         .adjusted_time_callback = GetAdjustedTime,
1384     };
1385-    ApplyArgsManOptions(args, chainman_opts);
1386+    if (const auto err{ApplyArgsManOptions(args, chainman_opts)}) {

ryanofsky commented at 2:06 pm on October 5, 2022:

In commit “Move ::nMinimumChainWork into ChainstateManager” (fa02eb1354ff16e91ab6b9bdb362be0de24f1750)

Maybe note slight change in behavior here in commit message: “Invalid non-hex” error will now be triggered later, after bitcoind has daemonized instead of before it daemonizes.

maflcko commented at 4:44 pm on October 5, 2022:

Thanks, but fixed in another way

in src/validation.h:862 in fa41ab525b outdated

858@@ -860,6 +859,10 @@ class ChainstateManager
859 
860     const CChainParams& GetParams() const { return m_options.chainparams; }
861     const Consensus::Params& GetConsensus() const { return m_options.chainparams.GetConsensus(); }
862+    bool IsCheckBlockIndex() const

ryanofsky commented at 2:10 pm on October 5, 2022:

In commit “Move ::fCheckBlockIndex into ChainstateManager” (fa41ab525b98df20bf84946cfcc9289dfb21113d)

“IsCheck” sounds like bad grammar, maybe “ShouldCheck” or “DoCheck” or just “Check” would be better.

maflcko commented at 4:44 pm on October 5, 2022:

Thanks, renamed

ryanofsky approved

ryanofsky commented at 2:27 pm on October 5, 2022: contributor

Code review ACK fa227bd60e739e6fb35619288e0f5ae535f52b9a. Left some suggestions, but could be merged as-is.

Main change since last review is rebasing and making assumed valid block, minimum chain work, and check block index options in kernel API match bitcoind defaults, using std::optional type and a Flatten function.

(Flatten isn’t my favorite approach but it solves problem of making kernel API straightforward to use and having kernel API defaults match bitcoind defaults. Preferred approach to doing this would have been to keep m_options fixed and just add accessor methods to merge option values and runtime defaults on demand. But difference between the approaches are internal to ChainManager, so not very significant.)

maflcko force-pushed on Oct 5, 2022

ryanofsky approved

ryanofsky commented at 5:25 pm on October 5, 2022: contributor

Code review ACK fa76987683be845955c6d993535a9ef768803540. Just suggested code and comment cleanups since last review, and moving the -minimumchainwork check back to AppInitParameterInteraction, before daemonization.

glozow requested review from TheCharlatan on Oct 6, 2022

DrahtBot added the label Needs rebase on Oct 13, 2022

Move ::nMaxTipAge into ChainstateManager faf44876db

Move ::hashAssumeValid into ChainstateManager

This changes the assumed valid block for the bitcoin-chainstate
executable. Previously it was uint256{}, now it is defaultAssumeValid.

fa29d0b57c

Move ::nMinimumChainWork into ChainstateManager

This changes the minimum chain work for the bitcoin-chainstate
executable. Previously it was uint256{}, now it is the chain's default
minimum chain work.

cccca83099

Move ::fCheckpointsEnabled into ChainstateManager fa43188d86

Move ::fCheckBlockIndex into ChainstateManager

This changes the flag for the bitcoin-chainstate executable. Previously
it was false, now it is the chain's default value (still false for the
main chain).

fa7c834b9f

Remove g_parallel_script_checks fa9ebec096

iwyu: Add missing includes aaaa7bd0ba

maflcko force-pushed on Oct 18, 2022

DrahtBot removed the label Needs rebase on Oct 18, 2022

ryanofsky approved

ryanofsky commented at 5:06 pm on October 18, 2022: contributor

Code review ACK aaaa7bd0ba60aa7114810d4794940882d987c0aa. No changes since last review, other than rebase

dergoegge commented at 5:20 pm on October 19, 2022: member

Concept ACK

aureleoules approved

aureleoules commented at 5:27 pm on October 19, 2022: member

reACK aaaa7bd0ba60aa7114810d4794940882d987c0aa

jamesob commented at 11:31 am on October 25, 2022: member

Concept ACK, will try to review today or tomorrow.

dergoegge commented at 9:22 am on October 26, 2022: member

Code review ACK aaaa7bd0ba60aa7114810d4794940882d987c0aa

maflcko merged this on Oct 26, 2022

maflcko closed this on Oct 26, 2022

maflcko deleted the branch on Oct 26, 2022

sidhujag referenced this in commit 7de7cb35e1 on Oct 27, 2022

hebasto commented at 12:08 pm on October 27, 2022: member

GCC throws multiple -Wmissing-field-initializers warnings with this code.

maflcko commented at 12:18 pm on October 27, 2022: member

Which version of GCC and what are the steps to reproduce?

hebasto commented at 12:21 pm on October 27, 2022: member

Which version of GCC and what are the steps to reproduce?

Debian 11 + gcc 10.2, Ubuntu 22.04 gcc 11.3:

0$ ./autogen.sh
1$ ./configure
2$ make clean
3$ make

maflcko commented at 9:14 am on October 28, 2022: member

Thanks, fixed in https://github.com/bitcoin/bitcoin/pull/26409

jamesob commented at 6:54 pm on November 9, 2022: member

There was a bug in here, though I certainly don’t think I would have caught it: https://github.com/bitcoin/bitcoin/pull/26477

bitcoin locked this on Nov 9, 2023

refactor: Remove almost all validation option globals #25704

Conflicts