Move parsing from init.cpp to where values are needed

Xekyo commented at 6:28 pm on November 10, 2019: member

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)

fanquake added the label Refactoring on Nov 10, 2019

in src/validation.cpp:4731 in 1b1e4213b6 outdated

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());

emilengler commented at 8:55 am on November 11, 2019:

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

Xekyo commented at 6:26 pm on November 11, 2019:

Thank you for your review @emilengler. I’ve updated the CheckBlockIndex function header to take all of chainparams instead of only the consensusParams subset.

MarcoFalke commented at 6:29 pm on November 11, 2019:

Instead of parsing command line arguments every time this function is called, what about adding this as a member variable that is only parsed once (e.g. at the time of construction).

MarcoFalke commented at 7:06 pm on November 11, 2019:

Sorry, I was wrong this was parsed on every call (missed the 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.

MarcoFalke commented at 7:07 pm on November 11, 2019:

(See also the reply by @ryanofsky which says exactly the same)

Xekyo force-pushed on Nov 11, 2019

in src/validation.cpp:4731 in 340acd0692 outdated

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());

ryanofsky commented at 6:54 pm on November 11, 2019:

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.

DrahtBot commented at 7:40 pm on November 11, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18571 (fuzz: Disable debug log file by MarcoFalke)

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.

jnewbery commented at 7:43 pm on November 11, 2019: member

@MarcoFalke

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.

MarcoFalke commented at 7:53 pm on November 11, 2019: member

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.

ryanofsky commented at 7:59 pm on November 11, 2019: member

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 call AppInitParameterInteraction.

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.

MarcoFalke commented at 8:21 pm on November 11, 2019: member

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

ryanofsky commented at 8:51 pm on November 11, 2019: member

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.

jnewbery commented at 8:57 pm on November 11, 2019: member

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

Xekyo commented at 10:46 pm on November 11, 2019: member

Alright. I’ve moved the 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.

Xekyo force-pushed on Nov 11, 2019

Move CheckBlockIndex to validation.cpp

The global fCheckBlockIndex is only used once and has no interaction.

eec8608577

Xekyo force-pushed on Nov 11, 2019

ryanofsky approved

ryanofsky commented at 11:23 pm on November 11, 2019: member

~Code review ACK eec8608577323536b55e75268a66b8c5657529d0~. Seems good. Param checking is now split between:

InitParameterInteraction
AppInitParameterInteraction
- WalletInit::ParameterInteraction
- ValidationParameterInteraction

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.

Xekyo commented at 2:44 am on November 12, 2019: member

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.

MarcoFalke commented at 3:09 am on November 12, 2019: member

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).

ryanofsky commented at 4:02 am on November 12, 2019: member

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:

https://github.com/bitcoin/bitcoin/blob/eec8608577323536b55e75268a66b8c5657529d0/src/validation.cpp#L118

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 maybe gArgs should be passed in to ValidationParameterInteraction, 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.

DrahtBot commented at 11:47 pm on April 15, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on Apr 15, 2020

MarcoFalke commented at 2:03 pm on April 25, 2020: member

@Xekyo Are you still working on this? Otherwise I suggest to close

fanquake closed this on Jul 26, 2020

DrahtBot locked this on Feb 15, 2022

Move parsing from init.cpp to where values are needed #17434

Conflicts