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

murchandamus · 2019-11-10T18:28:31Z

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)

cvengler · 2019-11-11T08:55:35Z

src/validation.cpp

@@ -4729,6 +4728,7 @@ bool LoadExternalBlockFile(const CChainParams& chainparams, FILE* fileIn, FlatFi

 void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams)
 {
+    static bool fCheckBlockIndex = gArgs.GetBoolArg("-checkblockindex", chainparams.DefaultConsistencyChecks());


validation.cpp:4731:73: error: use of undeclared identifier 'chainparams' 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

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

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

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.

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

ryanofsky · 2019-11-11T18:54:19Z

src/validation.cpp

 {
+    static bool fCheckBlockIndex = gArgs.GetBoolArg("-checkblockindex", chainparams.DefaultConsistencyChecks());


In commit "Move CheckBlockIndex to validation.cpp" (340acd0)

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 · 2019-11-11T19:40:49Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

fuzz: Disable debug log file #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 · 2019-11-11T19:43:22Z

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

maflcko · 2019-11-11T19:53:56Z

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 · 2019-11-11T19:59:00Z

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.

maflcko · 2019-11-11T20:21:52Z

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 · 2019-11-11T20:51:15Z

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 · 2019-11-11T20:57:24Z

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:

bitcoin/src/init.cpp

Line 1116 in a6f6333

if (!g_wallet_init_interface.ParameterInteraction()) return false;

murchandamus · 2019-11-11T22:46:26Z

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.

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

ryanofsky

~~Code review ACK eec8608~~. 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.

murchandamus · 2019-11-12T02:44:21Z

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.

maflcko · 2019-11-12T03:09:42Z

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 · 2019-11-12T04:02:17Z

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:

bitcoin/src/validation.cpp

Line 118 in eec8608

bool fCheckBlockIndex = false;

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 · 2020-04-15T23:47:12Z

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

maflcko · 2020-04-25T14:03:26Z

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

fanquake added the Refactoring label Nov 10, 2019

cvengler reviewed Nov 11, 2019

View reviewed changes

murchandamus force-pushed the trimGlobals branch from 1b1e421 to 340acd0 Compare November 11, 2019 18:25

ryanofsky reviewed Nov 11, 2019

View reviewed changes

murchandamus force-pushed the trimGlobals branch 3 times, most recently from 4092aa1 to 43d7bab Compare November 11, 2019 23:01

Move CheckBlockIndex to validation.cpp

eec8608

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

murchandamus force-pushed the trimGlobals branch from 43d7bab to eec8608 Compare November 11, 2019 23:04

ryanofsky approved these changes Nov 11, 2019

View reviewed changes

DrahtBot mentioned this pull request Feb 12, 2020

Add fee_est tool for debugging fee estimation code #10443

Closed

DrahtBot mentioned this pull request Mar 12, 2020

refactor: Add params to node context #18322

Closed

DrahtBot mentioned this pull request Apr 9, 2020

fuzz: Disable debug log file #18571

Merged

DrahtBot added the Needs rebase label Apr 15, 2020

fanquake closed this Jul 26, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

murchandamus commented Nov 10, 2019

cvengler Nov 11, 2019

murchandamus Nov 11, 2019

maflcko Nov 11, 2019

maflcko Nov 11, 2019

maflcko Nov 11, 2019

ryanofsky Nov 11, 2019

DrahtBot commented Nov 11, 2019 •

edited

Loading

jnewbery commented Nov 11, 2019

maflcko commented Nov 11, 2019

ryanofsky commented Nov 11, 2019

maflcko commented Nov 11, 2019

ryanofsky commented Nov 11, 2019

jnewbery commented Nov 11, 2019

murchandamus commented Nov 11, 2019

ryanofsky left a comment •

edited

Loading

murchandamus commented Nov 12, 2019

maflcko commented Nov 12, 2019

ryanofsky commented Nov 12, 2019

DrahtBot commented Apr 15, 2020

maflcko commented Apr 25, 2020

		{
		static bool fCheckBlockIndex = gArgs.GetBoolArg("-checkblockindex", chainparams.DefaultConsistencyChecks());

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

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

Conversation

murchandamus commented Nov 10, 2019

cvengler Nov 11, 2019

Choose a reason for hiding this comment

murchandamus Nov 11, 2019

Choose a reason for hiding this comment

maflcko Nov 11, 2019

Choose a reason for hiding this comment

maflcko Nov 11, 2019

Choose a reason for hiding this comment

maflcko Nov 11, 2019

Choose a reason for hiding this comment

ryanofsky Nov 11, 2019

Choose a reason for hiding this comment

DrahtBot commented Nov 11, 2019 • edited Loading

Conflicts

jnewbery commented Nov 11, 2019

maflcko commented Nov 11, 2019

ryanofsky commented Nov 11, 2019

maflcko commented Nov 11, 2019

ryanofsky commented Nov 11, 2019

jnewbery commented Nov 11, 2019

murchandamus commented Nov 11, 2019

ryanofsky left a comment • edited Loading

Choose a reason for hiding this comment

murchandamus commented Nov 12, 2019

maflcko commented Nov 12, 2019

ryanofsky commented Nov 12, 2019

DrahtBot commented Apr 15, 2020

maflcko commented Apr 25, 2020

DrahtBot commented Nov 11, 2019 •

edited

Loading

ryanofsky left a comment •

edited

Loading