As discussed briefly here: https://botbot.me/freenode/bitcoin-core-dev/2017-02-28/?msg=81712308&page=4
This adds a hidden command line option for setting nMinimumChainWork, which allows us to test this parameter in our functional tests, as well as allowing for niche use cases like syncing nodes that are otherwise disconnected from the network.
See also #10345, which proposes a new use of nMinimumChainWork.
975@@ -973,6 +976,11 @@ bool AppInitParameterInteraction()
976 else
977 LogPrintf("Validating signatures for all blocks.\n");
978
979+ nMinimumChainWork = UintToArith256(uint256S(GetArg("-minimumchainwork", chainparams.GetConsensus().nMinimumChainWork.GetHex())));
980+ if (nMinimumChainWork != UintToArith256(chainparams.GetConsensus().nMinimumChainWork)) {
981+ LogPrintf("Setting nMinimumChainWork=%s (%s default value)\n", nMinimumChainWork.GetHex(), (nMinimumChainWork < UintToArith256(chainparams.GetConsensus().nMinimumChainWork)) ? "below" : "above");
I suggest:
-minimumchainwork is set lower than the default value eg “nMinimumChainWork is set below the default value. This node may sync to a chain which is not the known most-work blockchain.”
Regarding the stronger warning: it’s hard to exactly say what may happen if nMinimumChainWork is too low, as it’s just an anti-DoS threshold. There’s no reason to think that we would then sync to something that’s not the most-work blockchain, unless there were some other attack going on.
I decided that since this is already a hidden option it’s sufficient to just log a warning if the value is below the default, without specifying the risk.
354@@ -355,6 +355,9 @@ std::string HelpMessage(HelpMessageMode mode)
355 strUsage += HelpMessageOpt("-maxorphantx=<n>", strprintf(_("Keep at most <n> unconnectable transactions in memory (default: %u)"), DEFAULT_MAX_ORPHAN_TRANSACTIONS));
356 strUsage += HelpMessageOpt("-maxmempool=<n>", strprintf(_("Keep the transaction memory pool below <n> megabytes (default: %u)"), DEFAULT_MAX_MEMPOOL_SIZE));
357 strUsage += HelpMessageOpt("-mempoolexpiry=<n>", strprintf(_("Do not keep transactions in the mempool longer than <n> hours (default: %u)"), DEFAULT_MEMPOOL_EXPIRY));
358+ if (showDebug) {
359+ strUsage += HelpMessageOpt("-minimumchainwork", strprintf("Minimum work assumed to exist on a valid chain (default: %s, testnet: %s)", Params(CBaseChainParams::MAIN).GetConsensus().nMinimumChainWork.GetHex(), Params(CBaseChainParams::TESTNET).GetConsensus().nMinimumChainWork.GetHex()));
22+
23+class MinimumChainWorkTest(BitcoinTestFramework):
24+ def __init__(self):
25+ super().__init__()
26+ self.setup_clean_chain = True
27+ self.num_nodes = 4
If someone changed the default topology, it’d be unfortunate if a lot of tests didn’t break! (since the topology is fundamental assumption for how the test nodes should behave)
Anyway, this was just a style nit - I have no objection to keeping the setup_network() override.
26+ self.setup_clean_chain = True
27+ self.num_nodes = 4
28+ self.extra_args = [[], ["-minimumchainwork=0x65"], ["-minimumchainwork=0xff"], ["-minimumchainwork=0x01ff"]]
29+ self.node_min_work = [0, 101, 255, 511]
30+
31+ def setup_network(self):
40+
41+ def run_test(self):
42+ # Start building a chain on node0. node2 shouldn't be able to sync until node1's
43+ # minchainwork is exceeded; node3 shouldn't be able to sync until node2's minchainwork
44+ # is exceeded.
45+ cumulative_chain_work = 2 # Genesis block's work
REGTEST_WORK_PER_BLOCK instead of hardcoding this?
46+ for i in [1,2]:
47+ self.log.info("Testing relay across node %d (minChainWork = %d)", i, self.node_min_work[i])
48+
49+ starting_blockcount = self.nodes[i+1].getblockcount()
50+
51+ num_blocks_to_generate = int((self.node_min_work[i] - cumulative_chain_work) / 2)
2 refers to work per block? In that case, replace with REGTEST_WORK_PER_BLOCK
Looks good. I haven’t dug into why Travis is failing, but this builds and passes the test locally for me.
A bunch of nits inline, but one more general comment: consider adding some sanity checking for the value passed in for -minimumchainwork:
0<invalid hex char><valid hex string>, then uint256S() will return 0 and my minimum chain work will be 0. If I set -minimumchainwork to Ox<hex value> (ie mistype the leading 0 as O), then minimum chain work will be 0).0x. A user may pass in a minimumchainwork value in decimal and then be surprised that the actual minimum chain work is far higher than expected.Other than that, tested ACK 87fb774569a61d3a9c5b774f438d4f342bc7554e. I like the new test.
-minimumchainwork command line value start with “0x”, mostly because I struggled to (a) figure out how to concisely explain that to the user in the help message, and (b) I noticed that GetHex() already doesn’t print a “0x”, so it would be extra confusing that the defaults don’t match the specification if we required “0x.”
tested ACK https://github.com/bitcoin/bitcoin/pull/10357/commits/f1041efca442dd0b56dbfdd99dedcef333a90e75. Very nice work with the test coverage for the new IsHexNumber() util function.
As for not requiring a leading 0x, I think that’s fine. The only minor problem I could see is someone entering a decimal number and the parser interpreting it as hex. That’s now mitigated by:
nMinimumChainWork back to a global :(
My only remark on this is that it’s unfortunate that after all the work we did to move things into a consensus parameters structure and pass it around, this moves nMinimumChainWork back to a global :(
I agree – any suggestions for a better paradigm for user-configurable, chain-specific parameters?
I agree – any suggestions for a better paradigm for user-configurable, chain-specific parameters?
I wish we’d left some of the chain parameters mutable, at least during the init process, so they don’t have to migrate back to globals to override their value.
Perhaps this is too late for 0.15
Yes, needs rebase though.
69+{
70+ size_t starting_location = 0;
71+ if (str.size() > 2 && *str.begin() == '0' && *(str.begin()+1) == 'x') {
72+ starting_location = 2;
73+ }
74+ for (auto it: str.substr(starting_location)) {
auto it is confusing for me since it isn’t an iterator and substr is returning a string object. Obviously you can for loop over a string since it’s a sequence of chars. Perhaps this would be clearer:
0for (const signed char c : str.substr(starting_location)) {
1 if (HexDigit(c) < 0) return false;
72+ starting_location = 2;
73+ }
74+ for (auto it: str.substr(starting_location)) {
75+ if (HexDigit(it) < 0) return false;
76+ }
77+ return (str.size() > starting_location);
Return false for empty string or "0x". It may also be clearer to place this at the top of the function.
@sdaftuar Sure, I was just expressing my sadness there that the idea of well-encapsulated chain parameters didn’t work out. I certainly did not mean it as a blocker.
On Aug 31, 2017 10:14 AM, “Suhas Daftuar” notifications@github.com wrote:
Rebased.
@laanwj https://github.com/laanwj I’d prefer to not refactor the chain parameters here, and leave that work for a future PR – it seems that there are other PRs in progress (eg #8994 https://github.com/bitcoin/bitcoin/pull/8994) separately addressing how that code is organized; let me know if you disagree.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10357#issuecomment-326362670, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHutm05tOxNXegFru5ddPKZWIiV3T90ks5sdunugaJpZM4NT_9J .
A couple of nits on the IsHexNumber() function.
The test is ingenious at indirectly testing whether the node is in IBD. I wonder whether it would be far more straightforward to just add a field to getblockchaininfo to report whether the node is in IBD. That’s a two line changeset and makes testing this PR (and probably others) much simpler:
0diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
1index e6b7851..10c1d2c 100644
2--- a/src/rpc/blockchain.cpp
3+++ b/src/rpc/blockchain.cpp
4@@ -1131,6 +1131,7 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
5 " \"difficulty\": xxxxxx, (numeric) the current difficulty\n"
6 " \"mediantime\": xxxxxx, (numeric) median time for the current best block\n"
7 " \"verificationprogress\": xxxx, (numeric) estimate of verification progress [0..1]\n"
8+ " \"initialblockdownload\": xxxx, (bool) is this node in Initial Block Download mode\n"
9 " \"chainwork\": \"xxxx\" (string) total amount of work in active chain, in hexadecimal\n"
10 " \"pruned\": xx, (boolean) if the blocks are subject to pruning\n"
11 " \"pruneheight\": xxxxxx, (numeric) lowest-height complete block stored\n"
12@@ -1175,6 +1176,7 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
13 obj.push_back(Pair("difficulty", (double)GetDifficulty()));
14 obj.push_back(Pair("mediantime", (int64_t)chainActive.Tip()->GetMedianTimePast()));
15 obj.push_back(Pair("verificationprogress", GuessVerificationProgress(Params().TxData(), chainActive.Tip())));
16+ obj.push_back(Pair("initialblockdownload", IsInitialBlockDownload()));
17 obj.push_back(Pair("chainwork", chainActive.Tip()->nChainWork.GetHex()));
18 obj.push_back(Pair("pruned", fPruneMode));
@jnewbery Mostly addressed your nits I think (just renamed the confusing it variable to c, and added a comment). Previous version is here for comparison: https://github.com/sdaftuar/bitcoin/commit/317cc116a7c64e66525936927baba1bfdb4bfaf7
I think you’re totally right that we could simplify this (and possibly other) tests by just adding a way to know if a node is in IBD. I’d like to do that, but I’m not sure it makes sense to complicate this PR with discussion over how that information should be made available. Eg it’s not clear to me that the notion of being in IBD is worth exposing to the casual user, versus a hidden rpc call or something, since the definition has changed over time, and may continue to change in the future. But I still do agree that at least for testing purposes it would be far simpler to expose the field somehow…
Nodes don't consider themselves out of "initial block download" until
their active chain has more work than nMinimumChainWork.
While in initial block download, nodes won't relay blocks to their
peers, so test that this parameter functions as intended by verifying
that block relay only succeeds past a given node once its
nMinimumChainWork has been exceeded.
I think you’re totally right that we could simplify this (and possibly other) tests by just adding a way to know if a node is in IBD. I’d like to do that, but I’m not sure it makes sense to complicate this PR with discussion over how that information should be made available.
Yes, I think it makes sense to discuss (there’s arguments for and against it re: proper testing), but let’s not extend the scope of this PR with it.
