Paths

Table of Contentst

partial gArgs refactor
AbandonedPublic
Actions

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 2777
Build 3664: Bitcoin ABC Buildbot (legacy)
Build 3663: arc lint + arc unit

Event Timeline

matiu created this revision.Jun 30 2018, 21:12

Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 30 2018, 21:12

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing started.

Harbormaster failed remote builds in B2777: Diff 4171!Jun 30 2018, 21:12

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing failed.

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.

In D1525#31277, @schancel wrote:

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

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B2784: Diff 4180!Jul 2 2018, 18:48

rebased

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B2791: Diff 4190!Jul 3 2018, 12:25

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 commandeered this revision.Oct 13 2023, 14:39

Mengerian abandoned this revision.

Mengerian added a reviewer: matiu.