Page MenuHomePhabricator

partial gArgs refactor
AbandonedPublic

Authored by Mengerian on Jun 30 2018, 21:12.

Details

Reviewers
deadalnix
matiu
Group Reviewers
Restricted Project
Summary

This is a small first step to refactor Config / gArgs / ChainParams / ChainBaseParam aiming to remove globals
and implicit states.

The idea is to eventually have something like:

auto &config = make_unique<Config &>(Config(argc, argv));

at bitcoind / bitcoin-cli / bitcoin-tx.

Then, config will have all parameters available, with getters, something like:

config.getDataDir();
config.consensus.getDiskMagic();

We could decide wheter to pass config everywhere needed, or to have global
(or similar) instance in the future.

For testing, we could do:

auto &config = make_unique<Config &>(TestConfig());

with adds setters methods for parameters, and other test's needs.

As a first step, we want to create methods on ArgsManager to access args settings
so we remove the param name constants (like "-datadir"), and help messages that
are spread all over the code.

Test Plan

test/functional/test_bitcoin.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
gargs
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 2791
Build 3692: Bitcoin ABC Buildbot (legacy)
Build 3691: arc lint + arc unit

Event Timeline

I don't understand where this is going. Passing gArgs down instead of accessing the global object actually help toward removing global, but I don't see how this API changes anything except reducing the generality of gArgs.

The idea might be to start moving away from the GetArg() stuff all over the codebase, and instead validate and set proper config variables at launch.

The idea might be to start moving away from the GetArg() stuff all over the codebase, and instead validate and set proper config variables at launch.

yes.

I want to try to unify gArgs/Config/ChainParams/ChainBaseParam in the future, remove all that globals, and have a nice Config object. But in order to do that, I feel we need to make the API more specific and less error prune.

IMHO, doing things like:

return nMedianTimePast >= gArgs.GetArg("-magneticanomalyactivationtime",
                                       config.GetChainParams()
                                           .GetConsensus()
                                           .magneticAnomalyActivationTime);

in validation.cpp should be enhanced to:

return nMedianTimePast >= gArgs.GetMagneticAnomalyActivationTime();

and then:

return nMedianTimePast >= config.consensus.GetMagneticAnomalyActivationTime();

What you think?

Adds PersistMempoolSet and StopAfterBlockImport flags to refactor

deadalnix requested changes to this revision.Jul 12 2018, 11:51

As discussed offline, this is specializing gArgs on specific arguments that our software is using, which is poor separation fo concerns. Knowing what parameter are used is the role of the config.

This revision now requires changes to proceed.Jul 12 2018, 11:51
Mengerian abandoned this revision.
Mengerian added a reviewer: matiu.