Don't edit Chainparams after initialization #13311

jtimon · 2018-05-23T13:26:21Z

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

This is a refactor and doesn't change functionality.

Related to #8994

skeees · 2018-05-23T17:45:46Z

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

sipa · 2018-05-24T17:42:09Z

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

jtimon · 2018-05-27T10:36:54Z

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 · 2018-05-27T20:01:35Z

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.

maflcko · 2018-05-30T13:58:47Z

Needs rebase due to merge of #13341

jtimon · 2018-06-16T20:42:49Z

Rebased and separated MOVEONLY commit.

instagibbs

utACK with ultra minor nit

instagibbs · 2018-07-12T00:07:25Z

src/chainparams.cpp


 #include <assert.h>

-#include <chainparamsseeds.h>
+#include <boost/algorithm/string/classification.hpp>
+#include <boost/algorithm/string/split.hpp>


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

instagibbs · 2018-07-12T00:11:19Z

src/chainparams.cpp


 #include <assert.h>

-#include <chainparamsseeds.h>
+#include <boost/algorithm/string/classification.hpp>


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

instagibbs · 2018-07-12T00:14:10Z

src/test/test_bitcoin.cpp

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


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

jtimon · 2018-07-19T05:41:05Z

rebased

jtimon · 2018-07-19T22:36:52Z

Fixed nit in #13311 (comment)

sipa · 2018-07-19T23:25:04Z

src/chainparams.cpp

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


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?

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

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

Could make this const ArgsManager& args

ajtowns

utACK 7067657731404c28ff87e46ec390992500ca13be

ajtowns · 2018-07-19T23:11:44Z

src/chainparams.cpp

+        }
+    }
+}
+
 static std::unique_ptr<CChainParams> globalChainParams;


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)

ajtowns · 2018-07-19T23:23:40Z

src/chainparams.cpp

 {
-    globalChainParams->UpdateVersionBitsParameters(d, nStartTime, nTimeout);
+    SelectBaseParams(network);
+    globalChainParams = CreateChainParams(network, gArgs);


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.

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

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?

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.

promag · 2018-07-20T01:13:49Z

Concept ACK.

jtimon · 2018-07-20T01:31:34Z

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

practicalswift · 2018-09-18T15:05:17Z

src/test/test_bitcoin.cpp

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


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]

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'

Cool! A false positive then. Thanks for investigating.

practicalswift · 2018-09-18T15:06:54Z

src/chainparams.cpp

+        }
+        bool found = false;
+        for (int j=0; j<(int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++j) {
+            if (vDeploymentParams[0].compare(VersionBitsDeploymentInfo[j].name) == 0) {


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]

practicalswift · 2018-09-18T15:07:05Z

src/chainparams.cpp

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


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 · 2018-09-19T16:03:21Z

Fixed 2 of the 3 clang warnings.

maflcko

utACK bd74fb9e4c271bd901dfbb5ad1929b44fcadd735

maflcko · 2018-09-19T18:46:41Z

src/chainparams.cpp

+        std::vector<std::string> vDeploymentParams;
+        boost::split(vDeploymentParams, strDeployment, boost::is_any_of(":"));
+        if (vDeploymentParams.size() != 3) {
+            throw std::runtime_error("Version bits parameters malformed, expecting deployment:start:end");


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.

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

practicalswift · 2018-09-23T08:20:47Z

src/chainparams.cpp

+            throw std::runtime_error(strprintf("Invalid nTimeout (%s)", vDeploymentParams[2]));
+        }
+        bool found = false;
+        for (int j=0; j<(int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++j) {


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

jtimon · 2018-09-23T20:59:59Z

Fixed latest style nit.

promag · 2018-09-23T23:25:37Z

utACK 6fa901f.

maflcko · 2018-09-24T20:45:54Z

re-utACK 6fa901f

6fa901f Don't edit Chainparams after initialization (Jorge Timón) 980b38f MOVEONLY: Move versionbits info out of versionbits.o (Jorge Timón) Pull request description: This encapsulates the "-vbparams" option, which is only meant for regtest, directly on CRegTestParams. This is a refactor and doesn't change functionality. Related to #8994 Tree-SHA512: 79771d729a63a720e743a9c77d5e2d80369f072d66202a43c1304e83a7d0ef7c6103d4968a03aea9666cc89a7203c618da972124a677b38cfe62ddaeb28f9f5d

06f1b42 Don't edit Chainparams after initialization (Jorge Timón) 875a47c MOVEONLY: Move versionbits info out of versionbits.o (Jorge Timón) Pull request description: This is a backport of bitcoin/bitcoin#13311 since it's already merged, merging this in elements-0.17 should at least make the next rebase slightly easier. And since we're heavily touching chainparams in elements, I think it's a good thing. Tree-SHA512: 177f6396a34ecf87b609b39a108cc2b1e5b49bd8d12b705a4ae0241c7a5007db754ff01d6d045ee1e6a219ec6c6767663cd98558752c83961c4aff3378aef5a2

5f23531 CRegTestParams: Use `args` instead of `gArgs`. (Kiminuo) Pull request description: This PR is a very minor follow-up to bitcoin#13311. I believe that `gArgs` was just overlooked at the modified line. ACKs for top commit: MarcoFalke: cr ACK 5f23531 Tree-SHA512: f4e4ed6b23fca60e88825b502f20a1341ee2e4429bc8a2a7e419057adb643abda11be2061fe7ee076931657736e629aff88fd2c33737c84c330dc9d64f368c30

6fa901f Don't edit Chainparams after initialization (Jorge Timón) 980b38f MOVEONLY: Move versionbits info out of versionbits.o (Jorge Timón) Pull request description: This encapsulates the "-vbparams" option, which is only meant for regtest, directly on CRegTestParams. This is a refactor and doesn't change functionality. Related to bitcoin#8994 Tree-SHA512: 79771d729a63a720e743a9c77d5e2d80369f072d66202a43c1304e83a7d0ef7c6103d4968a03aea9666cc89a7203c618da972124a677b38cfe62ddaeb28f9f5d

* Merge bitcoin#13311: Don't edit Chainparams after initialization 6fa901f Don't edit Chainparams after initialization (Jorge Timón) 980b38f MOVEONLY: Move versionbits info out of versionbits.o (Jorge Timón) Pull request description: This encapsulates the "-vbparams" option, which is only meant for regtest, directly on CRegTestParams. This is a refactor and doesn't change functionality. Related to bitcoin#8994 Tree-SHA512: 79771d729a63a720e743a9c77d5e2d80369f072d66202a43c1304e83a7d0ef7c6103d4968a03aea9666cc89a7203c618da972124a677b38cfe62ddaeb28f9f5d * Resolve Merge with bitcoin#13311 * Incorporated review changes * Apply suggestions from code review * Update src/chainparams.cpp * Update src/chainparams.cpp Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> Co-authored-by: MarcoFalke <falke.marco@gmail.com> Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>

jtimon force-pushed the b17-regtest-only-params branch from 4b5e16a to 6414f5b Compare May 23, 2018 13:30

fanquake added the Refactoring label May 23, 2018

maflcko added the Needs rebase label Jun 6, 2018

DrahtBot added Needs rebase and removed Needs rebase labels Jun 6, 2018

jtimon force-pushed the b17-regtest-only-params branch from 6414f5b to c3d5082 Compare June 16, 2018 20:42

DrahtBot removed the Needs rebase label Jun 19, 2018

DrahtBot mentioned this pull request Jun 19, 2018

[net,mempool] Call AcceptToMemoryPool() asynchronously in p2p #13413

Closed

instagibbs approved these changes Jul 12, 2018

View reviewed changes

DrahtBot mentioned this pull request Jul 12, 2018

[refactor] Fix the chainparamsbase -> util circular dependency #13639

Closed

DrahtBot added the Needs rebase label Jul 12, 2018

jtimon force-pushed the b17-regtest-only-params branch from c3d5082 to 54c256d Compare July 19, 2018 05:12

jtimon force-pushed the b17-regtest-only-params branch from b2ad565 to 7067657 Compare July 19, 2018 22:35

sipa reviewed Jul 19, 2018

View reviewed changes

ajtowns reviewed Jul 19, 2018

View reviewed changes

DrahtBot removed the Needs rebase label Jul 20, 2018

jtimon force-pushed the b17-regtest-only-params branch from 7067657 to ca040b5 Compare July 20, 2018 01:29

DrahtBot mentioned this pull request Jul 25, 2018

Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency #13751

Closed

This was referenced Aug 1, 2018

Include tinyformat as a subtree #13842

Closed

Include tinyformat as a subtree #13845

Closed

practicalswift reviewed Sep 18, 2018

View reviewed changes

jtimon force-pushed the b17-regtest-only-params branch from 18d5f26 to bd74fb9 Compare September 19, 2018 16:02

maflcko reviewed Sep 19, 2018

View reviewed changes

This was referenced Sep 21, 2018

Allow all mempool txs to be replaced after a configurable timeout (default 6h) #10823

Closed

Add fee_est tool for debugging fee estimation code #10443

Closed

practicalswift reviewed Sep 23, 2018

View reviewed changes

jtimon added 2 commits September 23, 2018 22:55

MOVEONLY: Move versionbits info out of versionbits.o

980b38f

Don't edit Chainparams after initialization

6fa901f

jtimon force-pushed the b17-regtest-only-params branch from bd74fb9 to 6fa901f Compare September 23, 2018 20:59

DrahtBot mentioned this pull request Sep 24, 2018

Use non-throwing type-safe ChainType where possible #14309

Closed

maflcko merged commit 6fa901f into bitcoin:master Sep 24, 2018

jtimon mentioned this pull request Oct 10, 2018

[0.17] Don't edit Chainparams after initialization ElementsProject/elements#427

Merged

Bushstar mentioned this pull request Oct 11, 2018

Updates from bitcoin/master FeatherCoin/Feathercoin#366

Merged

kiminuo mentioned this pull request Jun 2, 2021

CRegTestParams: Use args instead of gArgs. #22135

Merged

rkarthik2k21 added a commit to rkarthik2k21/dash that referenced this pull request Aug 25, 2021

Resolve Merge with bitcoin#13311

187ffd0

rkarthik2k21 mentioned this pull request Aug 25, 2021

merge bitcoin#13311: Don't edit Chainparams after initialization dashpay/dash#4362

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Don't edit Chainparams after initialization #13311

Don't edit Chainparams after initialization #13311

Conversation

jtimon commented May 23, 2018

skeees commented May 23, 2018

sipa commented May 24, 2018

jtimon commented May 27, 2018

sipa commented May 27, 2018

maflcko commented May 30, 2018

jtimon commented Jun 16, 2018

instagibbs left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimon commented Jul 19, 2018 • edited Loading

jtimon commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Empact Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajtowns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

promag commented Jul 20, 2018

jtimon commented Jul 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimon commented Sep 19, 2018

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimon Sep 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimon commented Sep 23, 2018

promag commented Sep 23, 2018

maflcko commented Sep 24, 2018

instagibbs left a comment •

edited

Loading

jtimon commented Jul 19, 2018 •

edited

Loading

Empact Aug 20, 2018 •

edited

Loading

jtimon Sep 20, 2018 •

edited

Loading