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.
Yea, sure, conceptack as a hidden setting.
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.
On further thought, perhaps I should add a warning if you set it above the default value? Because that could actually prevent you from syncing.
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()));
suggest "Minimum work assumed to exist on a valid chain in hex"
22 | + 23 | +class MinimumChainWorkTest(BitcoinTestFramework): 24 | + def __init__(self): 25 | + super().__init__() 26 | + self.setup_clean_chain = True 27 | + self.num_nodes = 4
default number of nodes in the base class is 4, so you don't need to override this here.
I ended up changing this test to use 3 nodes, but I wanted to fully specify the topology since the test depends on it -- eg if someone were to change the default I thought it would be unfortunate if the test then broke.
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):
The default test topology is a chain node0 <-> node1 <-> node2 <-> node3, so you don't need to override this method. You may want to move the comment into the module docstring.
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
Use 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)
I think this 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.
This needed a rebase.
@jnewbery Thanks for the review. I addressed or responded to most of your comments. I think the only thing I didn't do was require that the -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:
Concept ACK.
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 :(
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?
Perhaps this is too late for 0.15, but just wanted to mention that 0.15 will include another usage of nMinimumChainWork (#10345) that advanced users may sometimes want to work around.
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:
for (const signed char c : str.substr(starting_location)) {
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);
Perhaps add a comment. 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:
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index e6b7851..10c1d2c 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1131,6 +1131,7 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
" \"difficulty\": xxxxxx, (numeric) the current difficulty\n"
" \"mediantime\": xxxxxx, (numeric) median time for the current best block\n"
" \"verificationprogress\": xxxx, (numeric) estimate of verification progress [0..1]\n"
+ " \"initialblockdownload\": xxxx, (bool) is this node in Initial Block Download mode\n"
" \"chainwork\": \"xxxx\" (string) total amount of work in active chain, in hexadecimal\n"
" \"pruned\": xx, (boolean) if the blocks are subject to pruning\n"
" \"pruneheight\": xxxxxx, (numeric) lowest-height complete block stored\n"
@@ -1175,6 +1176,7 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
obj.push_back(Pair("difficulty", (double)GetDifficulty()));
obj.push_back(Pair("mediantime", (int64_t)chainActive.Tip()->GetMedianTimePast()));
obj.push_back(Pair("verificationprogress", GuessVerificationProgress(Params().TxData(), chainActive.Tip())));
+ obj.push_back(Pair("initialblockdownload", IsInitialBlockDownload()));
obj.push_back(Pair("chainwork", chainActive.Tip()->nChainWork.GetHex()));
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.
Actually this needed a rebase due to the test framework changes.
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.
utACK eac64bb
post merge utACK eac64bb7a3b6aba747403b23b3b1d3609843f8db
Tagging for backport, as the 0.15.0.2 tests rely on this.
Milestone
0.15.1