There appear to be a few more of these globals that are only used once. I could do more of them, if this sort of change is welcome.
Reference: #17383 (comment)
There appear to be a few more of these globals that are only used once. I could do more of them, if this sort of change is welcome.
Reference: #17383 (comment)
4727@@ -4729,6 +4728,7 @@ bool LoadExternalBlockFile(const CChainParams& chainparams, FILE* fileIn, FlatFi
4728
4729 void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams)
4730 {
4731+ static bool fCheckBlockIndex = gArgs.GetBoolArg("-checkblockindex", chainparams.DefaultConsistencyChecks());
0validation.cpp:4731:73: error: use of undeclared identifier 'chainparams'
1
2 static bool fCheckBlockIndex = gArgs.GetBoolArg("-checkblockindex", chainparams.DefaultConsistencyChecks());
chainparams
is not available in the scope, you should give a CChainParams
argument in the function (like in the other functions) or use a global variable
chainparams
instead of only the consensusParams
subset.
static
), however I’d still argue that this should be parsed at the time of construction (during initialization), so that parse errors can be returned to the caller.
4725@@ -4727,8 +4726,9 @@ bool LoadExternalBlockFile(const CChainParams& chainparams, FILE* fileIn, FlatFi
4726 return nLoaded > 0;
4727 }
4728
4729-void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams)
4730+void CChainState::CheckBlockIndex(const CChainParams& chainparams)
4731 {
4732+ static bool fCheckBlockIndex = gArgs.GetBoolArg("-checkblockindex", chainparams.DefaultConsistencyChecks());
In commit “Move CheckBlockIndex to validation.cpp” (340acd0692de198dbcc8a8ecdc48744d4f741c11)
Using a static local variable here isn’t great because it makes makes the method harder to test and reuse. For example, it would be impossible to write a unit test that invokes this fucntion with different fCheckBlockIndex
values. I haven’t looked at all the other variables you want to extend this to, but using static locals could create real problems in other cases.
Would suggest making this a CChainState object member and initializing it in the constructor as an alternative to leaving it as a global, and in general avoiding static locals in application logic outside of specialized init code.
Also, it’d be good to clarify the goal and advantages of this PR. It’s not obvious that it’s better to parse config values when they are first used instead of in AppInitParameterInteraction. For example an advantage of parsing in AppInitParameterInteraction is that if the user specifies an invalid or unparseable value, they can get error feedback immediately, and code that runs later doesn’t need to deal with errors.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
I’d still argue that this should be parsed at the time of construction… @ryanofsky It’s not obvious that it’s better to parse config values when they are first used instead of in AppInitParameterInteraction…
I think that AppInitParameterInteraction()
should be used for checking for parameter interactions. In cases where a parameter is just being read and stored in a global, it’s better to keep that in the translation unit where it’s used to avoid polluting the global namespace. Parsing and storing parameters in their own components also reduce code churn hotspots and reduces merge conflicts and rebases.
Would suggest making this a CChainState object…
This seems reasonable, although note g_chainstate
has’t yet been initialized at the point that we call AppInitParameterInteraction
.
This seems reasonable, although note g_chainstate has’t yet been initialized at the point that we call AppInitParameterInteraction.
I think we agree that AppInitParameterInteraction
is the wrong place for this, so obviously this code would need to be called when the chainstate is initialized.
Ok, would be good to look at specific cases, but the general tradeoff with this PR seems to be better error feedback for users and potentially simpler error handling in code vs “polluting the global namespace” and “code churn hotspots”, and “merge conflicts and rebases”.
This seems reasonable, although note
g_chainstate
has’t yet been initialized at the point that we callAppInitParameterInteraction
.
Yeah, sorry I was suggesting two different things. My first preference would be to keep parsing in AppInitParameterInteraction so it can happen as early as possible. But if that’s a losing proposition, CChainState member would be preferable to static local.
If there are a lot of related options, it might make sense to introduce a struct similar to the coin control struct which could hold parsed options and be passed around where needed.
If there are a lot of related options, it might make sense to introduce a struct similar to the coin control struct which could hold parsed options and be passed around where needed.
We already do that for connman: https://dev.visucore.com/bitcoin/doxygen/struct_c_connman_1_1_options.html
I think we agree that
AppInitParameterInteraction
is the wrong place for this
I don’t agree AppInitParameterInteraction
is the wrong place to parse and validate options. Or, I don’t think it’s the wrong time to validate and parse options. If relevant parsing were moved into a ValidationParameterInteraction
function to be more modular, that would seem good too.
If relevant parsing were moved into a ValidationParameterInteraction function to be more modular, that would seem good too.
I agree this would be good. We do something similar for wallet: https://github.com/bitcoin/bitcoin/blob/a6f6333ba253cda83221ee529810cacf930e413f/src/init.cpp#L1116
fCheckBlockIndex
value to the members of Validation.cpp
and added a function ValidationParameterInteraction(const CChainParams& chainparams)
function to validation.h
and validation.cpp
which I’m calling from init.cpp::AppInitParameterInteraction
.
The global fCheckBlockIndex is only used once and has no interaction.
Code review ACK eec8608577323536b55e75268a66b8c5657529d0. Seems good. Param checking is now split between:
EDIT: Removed ack. Seems to have bug pointed out by Marco below that test code isn’t calling ValidationParameterInteraction so gArgs.ForceSetArg("-checkblockindex", "1");
has no effect and checks are being skipped during testing.
Code review ACK eec8608. Seems good. Param checking is now split between:
* InitParameterInteraction * AppInitParameterInteraction * WalletInit::ParameterInteraction * ValidationParameterInteraction
Great, thanks. I’m thinking that I’ll add another commit to move fCheckpointsEnabled
in the same style next.
Could you add a motivation to the OP explaining the change, please. This is still a global (while hidden from other compilation untis) and it seems that the only way for tests to set this is to force set the argument and then call ValidationParameterInteraction
.
It doesn’t look like the tests are currently calling ValidationParameterInteraction
. That looks like a regression and raises the question if the change and bundled complication is worth it.
If the long-term goal is to limit parameter parsing to a single function per module FooParameteriInteraction
, then maybe gArgs
should be passed in to ValidationParameterInteraction
, so that it some day can be removed (or limited to live in init.cpp).
Could you add a motivation to the OP explaining the change, please. This is still a global (while hidden from other compilation untis) and it seems that the only way for tests to set this is to force set the argument and then call
ValidationParameterInteraction
.
I assume that is actually the goal of the PR: to limit access to the global so it’s internal to validation.cpp and outside code (including test code) can’t access it without setting a up a real configuration and initializing it with ValidationParameterInteraction
.
I don’t know if this a good goal or not. I’m +0. Maybe isolating these global variables more will reduce merge conflicts and so on as suggested #17434 (comment).
It doesn’t look like the tests are currently calling
ValidationParameterInteraction
. That looks like a regression and raises the question if the change and bundled complication is worth it.
That does seem like a bug. Could be fixed by test code calling ValidationParameterInteraction, or by changing the default value from false to true:
ValidationParameterInteraction
could also take an option to set up all relevant variables for testing without the test framework having to poke specific variables.
If the long-term goal is to limit parameter parsing to a single function per module
FooParameteriInteraction
, then maybegArgs
should be passed in toValidationParameterInteraction
, so that it some day can be removed (or limited to live in init.cpp).
This would be reasonable. Could be partially implemented here. Implementing it fully might expand this PR too much to non-validation code.
🐙 This pull request conflicts with the target branch and needs rebase.