Don't edit Chainparams after initialization #13311

jtimon commented at 1:26 PM on May 23, 2018: contributor

This encapsulates the "-vbparams" option, which is only meant for regtest, directly on CRegTestParams.

This is a refactor and doesn't change functionality.

jtimon force-pushed on May 23, 2018

fanquake added the label Refactoring on May 23, 2018

skeees commented at 5:45 PM on May 23, 2018: contributor

to ease review, might be good to split this into a moveonly commit and then a subsequent one with changes if thats possible?

sipa commented at 5:42 PM on May 24, 2018: member

Why move the version bits information to a separate file? This introduces a circular dependency between chainparams and versionbitsinfo,

jtimon commented at 10:36 AM on May 27, 2018: contributor

to ease review, might be good to split this into a moveonly commit and then a subsequent one with changes if thats possible?

Sure, but I would like to wait for some concept acks first.

Why move the version bits information to a separate file? This introduces a circular dependency between chainparams and versionbitsinfo,

No, it does not. After this, versionbits depends onchainparams, which depends on versionbitsinfo, which depends on consensus/params. The separation is precisely to avoid a circular dependency between chainparams and versionbits.

sipa commented at 8:01 PM on May 27, 2018: member

No, it does not. After this, versionbits depends onchainparams, which depends on versionbitsinfo, which depends on consensus/params. The separation is precisely to avoid a circular dependency between chainparams and versionbits.

My apologies! I confused chainparams and consensus/params when reading this.

MarcoFalke commented at 1:58 PM on May 30, 2018: member

Needs rebase due to merge of #13341

MarcoFalke added the label Needs rebase on Jun 6, 2018

DrahtBot removed the label Needs rebase on Jun 6, 2018

DrahtBot added the label Needs rebase on Jun 6, 2018

jtimon force-pushed on Jun 16, 2018

jtimon commented at 8:42 PM on June 16, 2018: contributor

Rebased and separated MOVEONLY commit.

DrahtBot removed the label Needs rebase on Jun 19, 2018

in src/chainparams.cpp:18 in c3d5082ce3 outdated

  14 |  
  15 |  #include <assert.h>
  16 |  
  17 | -#include <chainparamsseeds.h>
  18 | +#include <boost/algorithm/string/classification.hpp>
  19 | +#include <boost/algorithm/string/split.hpp>

instagibbs commented at 12:07 AM on July 12, 2018:

NB: it's still used in init.cpp, only for mempool replacement

in src/chainparams.cpp:17 in c3d5082ce3 outdated

  13 | +#include <versionbitsinfo.h>
  14 |  
  15 |  #include <assert.h>
  16 |  
  17 | -#include <chainparamsseeds.h>
  18 | +#include <boost/algorithm/string/classification.hpp>

instagibbs commented at 12:11 AM on July 12, 2018:

NB: it's still used in init.cpp, only for mempool replacement

in src/test/test_bitcoin.cpp:60 in c3d5082ce3 outdated

  54 | @@ -55,6 +55,9 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName)
  55 |          InitSignatureCache();
  56 |          InitScriptExecutionCache();
  57 |          fCheckBlockIndex = true;
  58 | +        // CreateAndProcessBlock() does not support building SegWit blocks, so don't activate in these tests.
  59 | +        // TODO: fix the code to support SegWit blocks.
  60 | +        gArgs.ForceSetArg("-vbparams", "segwit:0:999999999999");

instagibbs commented at 12:14 AM on July 12, 2018:

nit: would be nice to keep using Consensus::BIP9Deployment::NO_TIMEOUT

instagibbs approved

instagibbs commented at 12:23 AM on July 12, 2018: member

utACK with ultra minor nit

DrahtBot added the label Needs rebase on Jul 12, 2018

jtimon force-pushed on Jul 19, 2018

jtimon commented at 5:41 AM on July 19, 2018: contributor

rebased

jtimon force-pushed on Jul 19, 2018

jtimon commented at 10:36 PM on July 19, 2018: contributor

Fixed nit in #13311 (review)

in src/chainparams.cpp:400 in 7067657731 outdated

 395 | +            }
 396 | +        }
 397 | +    }
 398 | +}
 399 | +
 400 |  static std::unique_ptr<CChainParams> globalChainParams;

ajtowns commented at 11:11 PM on July 19, 2018:

Could change this to unique_ptr<const CChainParams> to better ensure that params aren't changed after initialisation? (Means CreateChainParams() return value also needs to change, but that seems to work fine)

in src/chainparams.cpp:427 in 7067657731 outdated

 429 | -void UpdateVersionBitsParameters(Consensus::DeploymentPos d, int64_t nStartTime, int64_t nTimeout)
 430 | +void SelectParams(const std::string& network)
 431 |  {
 432 | -    globalChainParams->UpdateVersionBitsParameters(d, nStartTime, nTimeout);
 433 | +    SelectBaseParams(network);
 434 | +    globalChainParams = CreateChainParams(network, gArgs);

ajtowns commented at 11:23 PM on July 19, 2018:

Why not reference gArgs directly in CreateChainParams() when using REGTEST? This is the only place that actually calls CreateChainParams with the args parameter and the only place that calls it with chain==REGTEST. That way you don't need to have two different ways of calling CreateChainParams.

jtimon commented at 1:23 AM on July 20, 2018:

I can remove the new way of calling CreateChainParams, but the idea was that some unittests could create their own custom ArgsManager instead of using gArgs. This would only serve for testing functions that explicitly take a CChainParams instead of relaying on Params().

promag commented at 10:51 PM on August 19, 2018:

Tend to agree with @ajtowns.

but the idea was that some unittests could create their own custom ArgsManager instead of using gArgs

If needed it can mutate gArgs? Or this can be done when needed?

jtimon commented at 8:20 PM on August 20, 2018:

I guess one can mutate gArgs to a given value for a given test and then back to its original value afterwards. Or as you say, make this change afterwards when it's going to be used.

in src/chainparams.cpp:279 in 7067657731 outdated

 275 | @@ -280,7 +276,7 @@ class CTestNetParams : public CChainParams {
 276 |   */
 277 |  class CRegTestParams : public CChainParams {
 278 |  public:
 279 | -    CRegTestParams() {
 280 | +    CRegTestParams(ArgsManager& args) {

sipa commented at 11:25 PM on July 19, 2018:

Is it necessary to pass in an entire ArgsManager object? It seems only the -vbparams value is used. What about just passing in the vbparams value?

jtimon commented at 11:26 PM on July 19, 2018:

Well, the point of that is to make it easy to add further params, like in #8994 (probably not all of them are interesting for testing).

Empact commented at 11:23 PM on August 20, 2018:

Better to just pass the needed arg IMO, and only couple these classes if preferable based on the eventual use. https://martinfowler.com/bliki/Yagni.html

ajtowns commented at 4:39 AM on August 27, 2018:

Could make this const ArgsManager& args

ajtowns commented at 11:27 PM on July 19, 2018: member

utACK 7067657731404c28ff87e46ec390992500ca13be

DrahtBot removed the label Needs rebase on Jul 20, 2018

promag commented at 1:13 AM on July 20, 2018: member

Concept ACK.

jtimon force-pushed on Jul 20, 2018

jtimon commented at 1:31 AM on July 20, 2018: contributor

Fixed nit in https://github.com/bitcoin/bitcoin/pull/13311/files#r203899444

laanwj added this to the "Blockers" column in a project

in src/chainparams.cpp:363 in ca040b5b76 outdated

 358 | +    void UpdateVersionBitsParameters(Consensus::DeploymentPos d, int64_t nStartTime, int64_t nTimeout)
 359 | +    {
 360 | +        consensus.vDeployments[d].nStartTime = nStartTime;
 361 | +        consensus.vDeployments[d].nTimeout = nTimeout;
 362 | +    }
 363 | +    void UpdateVersionBitsParametersFromArgs(ArgsManager& args);

ajtowns commented at 4:40 AM on August 27, 2018:

const ArgsManager& here as well

in src/chainparams.cpp:414 in ca040b5b76 outdated

 412 |          return std::unique_ptr<CChainParams>(new CMainParams());
 413 |      else if (chain == CBaseChainParams::TESTNET)
 414 |          return std::unique_ptr<CChainParams>(new CTestNetParams());
 415 |      else if (chain == CBaseChainParams::REGTEST)
 416 | -        return std::unique_ptr<CChainParams>(new CRegTestParams());
 417 | +        return std::unique_ptr<CChainParams>(new CRegTestParams(args));

ajtowns commented at 4:41 AM on August 27, 2018:

I still think dropping the args param and referencing gArgs here directly makes more sense. Otherwise, could make args be const here too.

l2a5b1 commented at 9:50 PM on September 1, 2018:

Yes, that makes sense.

By doing just that and dropping the ArgsManager& args argument from std::unique_ptr<const CChainParams> CreateChainParams(const std::string& chain, ArgsManager& args) we can remove std::unique_ptr<const CChainParams> CreateChainParams(const std::string& chain) as you also have pointed out here: #13311 (review).

ajtowns approved

ajtowns commented at 4:42 AM on August 27, 2018: member

utACK ca040b5b76ddeeace6da4f8111169ffd6234414a

in src/chainparams.cpp:369 in ca040b5b76 outdated

 365 |  
 366 | -static std::unique_ptr<CChainParams> globalChainParams;
 367 | +void CRegTestParams::UpdateVersionBitsParametersFromArgs(ArgsManager& args)
 368 | +{
 369 | +    // Allow overriding version bits parameters for testing
 370 | +    if (args.IsArgSet("-vbparams")) {

l2a5b1 commented at 3:52 PM on September 1, 2018:

This function can exit early if the -vbparams arg is not set.

void CRegTestParams::UpdateVersionBitsParametersFromArgs(ArgsManager& args)
{
    if (!args.IsArgSet("-vbparams")) return;

    for (const std::string& strDeployment : args.GetArgs("-vbparams")) {
        ...
    }
}

in src/chainparams.h:113 in ca040b5b76 outdated

 107 | @@ -107,7 +108,9 @@ class CChainParams
 108 |   * @returns a CChainParams* of the chosen chain.
 109 |   * @throws a std::runtime_error if the chain is not supported.
 110 |   */
 111 | -std::unique_ptr<CChainParams> CreateChainParams(const std::string& chain);
 112 | +std::unique_ptr<const CChainParams> CreateChainParams(const std::string& chain);
 113 | +/** Extended version taking custom args (only useful for regtest). */
 114 | +std::unique_ptr<const CChainParams> CreateChainParams(const std::string& chain, ArgsManager& args);

l2a5b1 commented at 9:00 PM on September 1, 2018:

I tend to agree with @ajtowns that we can do without std::unique_ptr<const CChainParams> CreateChainParams(const std::string& chain, ArgsManager& args).

If you want to keep it then maybe you can remove its declaration from chainparams.h. There is no need to expose it; it only uses internal linkage at the moment.

jtimon force-pushed on Sep 5, 2018

jtimon commented at 12:06 AM on September 5, 2018: contributor

Hopefully fixed all pending nits in the last commit to be squashed, please remind me if I left anything out. Also sorry about the wait, back from vacation.

ajtowns commented at 12:04 AM on September 6, 2018: member

Looks good to me, fwiw

DrahtBot added the label Needs rebase on Sep 6, 2018

jtimon force-pushed on Sep 7, 2018

jtimon commented at 10:09 PM on September 7, 2018: contributor

Of course, @DrahtBot , from now on I will rebase all my open PRs whenever you ask. Starting today with this PR. Thanks

DrahtBot removed the label Needs rebase on Sep 7, 2018

DrahtBot commented at 1:49 AM on September 10, 2018: member

Note to reviewers: This pull request conflicts with the following ones:

#14309 (scripted-diff: Use non-throwing type-safe ChainType where possible by MarcoFalke)
#14053 (Add address-based index (attempt 4?) by marcinja)
#14045 (refactor: Fix the chainparamsbase -> util circular dependency by Empact)
#13751 (Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency by 251Labs)
#10823 (Allow all mempool txs to be replaced after a configurable timeout (default 6h) by greenaddress)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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.

ken2812221 commented at 8:04 PM on September 12, 2018: contributor

utACK 18d5f26

in src/chainparams.cpp:404 in 18d5f2621f outdated

 402 |          return std::unique_ptr<CChainParams>(new CMainParams());
 403 |      else if (chain == CBaseChainParams::TESTNET)
 404 |          return std::unique_ptr<CChainParams>(new CTestNetParams());
 405 |      else if (chain == CBaseChainParams::REGTEST)
 406 | -        return std::unique_ptr<CChainParams>(new CRegTestParams());
 407 | +        return std::unique_ptr<CChainParams>(new CRegTestParams(gArgs));

promag commented at 9:19 PM on September 16, 2018:

Any reason to not use gArgs global in CRegTestParams constructor?

ajtowns commented at 12:54 PM on September 18, 2018:

The class only uses the global for initialisation; I think it makes sense to supply it as a parameter for initialisation rather than making the entire class dependent on the global. YMMV, FWIW, etc.

promag commented at 9:19 PM on September 16, 2018: member

utACK 18d5f26, with a little question.

jtimon commented at 10:37 PM on September 16, 2018: contributor

Any reason to use the global instead of a parameter? I think it's more encapsulated this way, but I tend to be alergic to globals ib general. If people prefer a global I guess I will have to change it.

On Sun, 16 Sep 2018, 23:21 João Barbosa, notifications@github.com wrote:

@promag commented on this pull request.

utACK 18d5f26 https://github.com/bitcoin/bitcoin/commit/18d5f2621f2ade428e631d34b95e23c8be862ec8, with a little question.

In src/chainparams.cpp https://github.com/bitcoin/bitcoin/pull/13311#discussion_r217928196:

{ if (chain == CBaseChainParams::MAIN) return std::unique_ptr<CChainParams>(new CMainParams()); else if (chain == CBaseChainParams::TESTNET) return std::unique_ptr<CChainParams>(new CTestNetParams()); else if (chain == CBaseChainParams::REGTEST)
   return std::unique_ptr<CChainParams>(new CRegTestParams());
   return std::unique_ptr<CChainParams>(new CRegTestParams(gArgs));
Any reason to not use gArgs global in CRegTestParams constructor?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/13311#pullrequestreview-155758424, or mute the thread https://github.com/notifications/unsubscribe-auth/AA9jStqUTYlVdHuFThL_C3Io272IQd7eks5ubsDAgaJpZM4UKcd3 .

ajtowns commented at 1:03 PM on September 18, 2018: member

utACK 18d5f2621f2ade428e631d34b95e23c8be862ec8

in src/test/test_bitcoin.cpp:64 in 18d5f2621f outdated

  58 | @@ -58,6 +59,9 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName)
  59 |      InitSignatureCache();
  60 |      InitScriptExecutionCache();
  61 |      fCheckBlockIndex = true;
  62 | +    // CreateAndProcessBlock() does not support building SegWit blocks, so don't activate in these tests.
  63 | +    // TODO: fix the code to support SegWit blocks.
  64 | +    gArgs.ForceSetArg("-vbparams", strprintf("segwit:0:%d", (int64_t)Consensus::BIP9Deployment::NO_TIMEOUT));

practicalswift commented at 3:05 PM on September 18, 2018:

2018-09-18 16:46:43 clang-tidy(pr=13311): src/test/test_bitcoin.cpp:64:61: warning: redundant cast to the same type [google-readability-casting]

jtimon commented at 4:02 PM on September 19, 2018:

If I remove that casting, I get:

test/qt_test_test_bitcoin_qt-test_bitcoin.o: In function `tinyformat::detail::FormatArg::FormatArg<long>(long const&)':
/home/jt/code/bitcoin/src/./tinyformat.h:508: undefined reference to `Consensus::BIP9Deployment::NO_TIMEOUT'

practicalswift commented at 4:11 PM on September 19, 2018:

Cool! A false positive then. Thanks for investigating.

in src/chainparams.cpp:377 in 18d5f2621f outdated

 373 | +        if (!ParseInt64(vDeploymentParams[2], &nTimeout)) {
 374 | +            throw std::runtime_error(strprintf("Invalid nTimeout (%s)", vDeploymentParams[2]));
 375 | +        }
 376 | +        bool found = false;
 377 | +        for (int j=0; j<(int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++j) {
 378 | +            if (vDeploymentParams[0].compare(VersionBitsDeploymentInfo[j].name) == 0) {

practicalswift commented at 3:06 PM on September 18, 2018:

2018-09-18 16:46:43 clang-tidy(pr=13311): src/chainparams.cpp:377:17: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [misc-string-compare]

in src/chainparams.cpp:271 in 18d5f2621f outdated

 267 | @@ -270,7 +268,7 @@ class CTestNetParams : public CChainParams {
 268 |   */
 269 |  class CRegTestParams : public CChainParams {
 270 |  public:
 271 | -    CRegTestParams() {
 272 | +    CRegTestParams(const ArgsManager& args) {

practicalswift commented at 3:07 PM on September 18, 2018:

2018-09-18 16:46:43 clang-tidy(pr=13311): src/chainparams.cpp:271:5: warning: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor]

jtimon force-pushed on Sep 19, 2018

jtimon commented at 4:03 PM on September 19, 2018: contributor

Fixed 2 of the 3 clang warnings.

in src/chainparams.cpp:366 in bd74fb9e4c outdated

 362 | +
 363 | +    for (const std::string& strDeployment : args.GetArgs("-vbparams")) {
 364 | +        std::vector<std::string> vDeploymentParams;
 365 | +        boost::split(vDeploymentParams, strDeployment, boost::is_any_of(":"));
 366 | +        if (vDeploymentParams.size() != 3) {
 367 | +            throw std::runtime_error("Version bits parameters malformed, expecting deployment:start:end");

MarcoFalke commented at 6:46 PM on September 19, 2018:

I don't like that this introduces more exceptions in code that can normally hit them by a mere typo. I'd prefer if we used exceptions only in exceptional circumstances, since it is not easy to follow the control flow with exceptions.

Note that the our other command line parsing is done with functions that return bool instead of void and append the error to a passed in string reference.

I see that the caller needs to throw, but that isn't reason enough to "poison" unrelated code.

jtimon commented at 8:47 AM on September 20, 2018:

I think there's absolutely no problem in initialization exceptions that will prevent the binary from starting, this one should be cached (depending on binary) on bitcoin-cli.cpp, bitcoin-tx.cpp or bitcoind.cpp, when SelectParams is called, just like https://github.com/bitcoin/bitcoin/blob/master/src/chainparamsbase.cpp#L42

MarcoFalke commented at 6:46 PM on September 19, 2018: member

utACK bd74fb9e4c271bd901dfbb5ad1929b44fcadd735

MOVEONLY: Move versionbits info out of versionbits.o 980b38f8a1

Don't edit Chainparams after initialization 6fa901fb47

in src/chainparams.cpp:376 in bd74fb9e4c outdated

 372 | +        }
 373 | +        if (!ParseInt64(vDeploymentParams[2], &nTimeout)) {
 374 | +            throw std::runtime_error(strprintf("Invalid nTimeout (%s)", vDeploymentParams[2]));
 375 | +        }
 376 | +        bool found = false;
 377 | +        for (int j=0; j<(int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++j) {

practicalswift commented at 8:20 AM on September 23, 2018:

2018-09-22 22:20:08 cpplint(pr=13311): src/chainparams.cpp:376:  Missing spaces around <  [whitespace/operators] [3]

jtimon force-pushed on Sep 23, 2018

jtimon commented at 8:59 PM on September 23, 2018: contributor

Fixed latest style nit.

promag commented at 11:25 PM on September 23, 2018: member

utACK 6fa901f.

MarcoFalke commented at 8:45 PM on September 24, 2018: member

re-utACK 6fa901f

MarcoFalke merged this on Sep 24, 2018

MarcoFalke closed this on Sep 24, 2018

MarcoFalke referenced this in commit 4dac24db23 on Sep 24, 2018

meshcollider removed this from the "Blockers" column in a project

jtimon referenced this in commit a31ab0ec26 on Oct 13, 2018

MarcoFalke referenced this in commit 0011167191 on Jun 3, 2021

sidhujag referenced this in commit 1bfa968792 on Jun 3, 2021

rkarthik2k21 referenced this in commit dae58c5554 on Aug 12, 2021

rkarthik2k21 referenced this in commit 90b9a42a9b on Aug 25, 2021

rkarthik2k21 referenced this in commit 187ffd0898 on Aug 25, 2021

UdjinM6 referenced this in commit f456edb07e on Sep 2, 2021

MarcoFalke locked this on Sep 8, 2021

Don't edit Chainparams after initialization #13311

utACK 18d5f26 https://github.com/bitcoin/bitcoin/commit/18d5f2621f2ade428e631d34b95e23c8be862ec8, with a little question.