Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow setting nMinimumChainWork on command line #10357

Merged
merged 2 commits into from Sep 6, 2017

Conversation

sdaftuar
Copy link
Member

@sdaftuar sdaftuar commented May 8, 2017

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.

@gmaxwell
Copy link
Contributor

gmaxwell commented May 8, 2017

Yea, sure, conceptack as a hidden setting.

@sdaftuar
Copy link
Member Author

sdaftuar commented May 9, 2017

I just realized this doesn't quite work cleanly with #10345; closing for now.

@sdaftuar sdaftuar closed this May 9, 2017
@sdaftuar sdaftuar reopened this Jun 8, 2017
Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • if it's not a hex value, then uint256S() will fail silently (in fact it's worse than this - uint256S() will return the int for as much of the prefix is valid hex, so if I set -minimumchainwork to 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).
  • it accepts hex values with or without leading 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.

src/init.cpp Outdated
@@ -973,6 +976,11 @@ bool AppInitParameterInteraction()
else
LogPrintf("Validating signatures for all blocks.\n");

nMinimumChainWork = UintToArith256(uint256S(GetArg("-minimumchainwork", chainparams.GetConsensus().nMinimumChainWork.GetHex())));
if (nMinimumChainWork != UintToArith256(chainparams.GetConsensus().nMinimumChainWork)) {
LogPrintf("Setting nMinimumChainWork=%s (%s default value)\n", nMinimumChainWork.GetHex(), (nMinimumChainWork < UintToArith256(chainparams.GetConsensus().nMinimumChainWork)) ? "below" : "above");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest:

  • always log the nMinimumChainWork and default value (even if it hasn't been overwritten in config)
  • issue a stronger warning in the log if -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."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/init.cpp Outdated
@@ -355,6 +355,9 @@ std::string HelpMessage(HelpMessageMode mode)
strUsage += HelpMessageOpt("-maxorphantx=<n>", strprintf(_("Keep at most <n> unconnectable transactions in memory (default: %u)"), DEFAULT_MAX_ORPHAN_TRANSACTIONS));
strUsage += HelpMessageOpt("-maxmempool=<n>", strprintf(_("Keep the transaction memory pool below <n> megabytes (default: %u)"), DEFAULT_MAX_MEMPOOL_SIZE));
strUsage += HelpMessageOpt("-mempoolexpiry=<n>", strprintf(_("Do not keep transactions in the mempool longer than <n> hours (default: %u)"), DEFAULT_MEMPOOL_EXPIRY));
if (showDebug) {
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()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest "Minimum work assumed to exist on a valid chain in hex"

def __init__(self):
super().__init__()
self.setup_clean_chain = True
self.num_nodes = 4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default number of nodes in the base class is 4, so you don't need to override this here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.extra_args = [[], ["-minimumchainwork=0x65"], ["-minimumchainwork=0xff"], ["-minimumchainwork=0x01ff"]]
self.node_min_work = [0, 101, 255, 511]

def setup_network(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Start building a chain on node0. node2 shouldn't be able to sync until node1's
# minchainwork is exceeded; node3 shouldn't be able to sync until node2's minchainwork
# is exceeded.
cumulative_chain_work = 2 # Genesis block's work
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use REGTEST_WORK_PER_BLOCK instead of hardcoding this?


starting_blockcount = self.nodes[i+1].getblockcount()

num_blocks_to_generate = int((self.node_min_work[i] - cumulative_chain_work) / 2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this 2 refers to work per block? In that case, replace with REGTEST_WORK_PER_BLOCK

@sdaftuar sdaftuar force-pushed the 2017-05-chainwork-commandline branch from 87fb774 to f1041ef Compare June 13, 2017 21:37
@sdaftuar
Copy link
Member Author

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

@jnewbery
Copy link
Contributor

tested ACK f1041ef. 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:

  • the help text saying the number should be hex
  • the log message always printing out the minimum chain work in hex

@laanwj
Copy link
Member

laanwj commented Jun 26, 2017

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 :(

@laanwj laanwj self-assigned this Jun 28, 2017
@sdaftuar sdaftuar force-pushed the 2017-05-chainwork-commandline branch from f1041ef to d76adbe Compare June 29, 2017 17:39
@sdaftuar
Copy link
Member Author

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?

@sdaftuar
Copy link
Member Author

sdaftuar commented Aug 3, 2017

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.

@laanwj
Copy link
Member

laanwj commented Aug 22, 2017

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.

@sdaftuar sdaftuar force-pushed the 2017-05-chainwork-commandline branch from d76adbe to 317cc11 Compare August 31, 2017 17:08
@sdaftuar
Copy link
Member Author

Rebased.

@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) separately addressing how that code is organized; let me know if you disagree.

@laanwj
Copy link
Member

laanwj commented Aug 31, 2017 via email

Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (str.size() > 2 && *str.begin() == '0' && *(str.begin()+1) == 'x') {
starting_location = 2;
}
for (auto it: str.substr(starting_location)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

for (auto it: str.substr(starting_location)) {
if (HexDigit(it) < 0) return false;
}
return (str.size() > starting_location);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@meshcollider
Copy link
Contributor

utACK 317cc11

@sdaftuar sdaftuar force-pushed the 2017-05-chainwork-commandline branch from 6bf6ea8 to 6f801a5 Compare September 5, 2017 16:50
@sdaftuar
Copy link
Member Author

sdaftuar commented Sep 5, 2017

@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: sdaftuar@317cc11

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.
@sdaftuar sdaftuar force-pushed the 2017-05-chainwork-commandline branch from 6f801a5 to eac64bb Compare September 5, 2017 19:09
@sdaftuar
Copy link
Member Author

sdaftuar commented Sep 5, 2017

Actually this needed a rebase due to the test framework changes.

@laanwj
Copy link
Member

laanwj commented Sep 6, 2017

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.

@laanwj
Copy link
Member

laanwj commented Sep 6, 2017

utACK eac64bb

@laanwj laanwj merged commit eac64bb into bitcoin:master Sep 6, 2017
laanwj added a commit that referenced this pull request Sep 6, 2017
eac64bb [qa] Test nMinimumChainWork (Suhas Daftuar)
0311836 Allow setting nMinimumChainWork on command line (Suhas Daftuar)

Pull request description:

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

Tree-SHA512: fe4d8f4f289697615c98d8760f1cc74c076110310ea0b5b875fcab78c127da9195b4eb84148aebacc7606c246e5773d3f13bd5d9559d0a8bffac20a3a28c62df
@maflcko
Copy link
Member

maflcko commented Sep 6, 2017

post merge utACK eac64bb

@maflcko maflcko added this to the 0.15.0.2 milestone Nov 2, 2017
@maflcko
Copy link
Member

maflcko commented Nov 2, 2017

Tagging for backport, as the 0.15.0.2 tests rely on this.

@morcos morcos mentioned this pull request Nov 2, 2017
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 2, 2017
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 2, 2017
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.

Github-Pull: bitcoin#10357
Rebased-From: eac64bb
sipa added a commit that referenced this pull request Nov 11, 2017
1141364 [trivial] (whitespace only) fix getblockchaininfo alignment (John Newbery)
bd9c181 [rpc] Add initialblockdownload to getblockchaininfo (John Newbery)

Pull request description:

  Exposing whether the node is in IBD would help for testing, and may be useful in general, particularly for developers.

  First discussed in #10357 here: #10357 (review)

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

   This PR currently implements the simplest way of doing this: adding an `initialblockdownload` field to `getblockchaininfo`. Other approaches we could take:

  1. add a new debug RPC method that exposes `IBD` and potentially other information.
  2. add a parameter to `getblockchaininfo`, eg `debug_info`, which would cause it to return debug information including IBD
  3. add a query string to the url `?debug=true` which would cause RPCs to return additional debug information.

  I quite like the idea of (3). Feedback on these and other approaches very much welcomed!

  @sdaftuar @laanwj

Tree-SHA512: a6dedd47f8c9bd38769cc597524466250041136feb33500644b9c48d0ffe4e3eeeb2587b5bbc6420364ebdd2667df807fbb50416f9a7913bbf11a14ea86dc0d4
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 20, 2019
eac64bb [qa] Test nMinimumChainWork (Suhas Daftuar)
0311836 Allow setting nMinimumChainWork on command line (Suhas Daftuar)

Pull request description:

  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 bitcoin#10345, which proposes a new use of `nMinimumChainWork`.

Tree-SHA512: fe4d8f4f289697615c98d8760f1cc74c076110310ea0b5b875fcab78c127da9195b4eb84148aebacc7606c246e5773d3f13bd5d9559d0a8bffac20a3a28c62df
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 23, 2019
eac64bb [qa] Test nMinimumChainWork (Suhas Daftuar)
0311836 Allow setting nMinimumChainWork on command line (Suhas Daftuar)

Pull request description:

  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 bitcoin#10345, which proposes a new use of `nMinimumChainWork`.

Tree-SHA512: fe4d8f4f289697615c98d8760f1cc74c076110310ea0b5b875fcab78c127da9195b4eb84148aebacc7606c246e5773d3f13bd5d9559d0a8bffac20a3a28c62df
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 23, 2019
eac64bb [qa] Test nMinimumChainWork (Suhas Daftuar)
0311836 Allow setting nMinimumChainWork on command line (Suhas Daftuar)

Pull request description:

  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 bitcoin#10345, which proposes a new use of `nMinimumChainWork`.

Tree-SHA512: fe4d8f4f289697615c98d8760f1cc74c076110310ea0b5b875fcab78c127da9195b4eb84148aebacc7606c246e5773d3f13bd5d9559d0a8bffac20a3a28c62df
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 24, 2019
eac64bb [qa] Test nMinimumChainWork (Suhas Daftuar)
0311836 Allow setting nMinimumChainWork on command line (Suhas Daftuar)

Pull request description:

  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 bitcoin#10345, which proposes a new use of `nMinimumChainWork`.

Tree-SHA512: fe4d8f4f289697615c98d8760f1cc74c076110310ea0b5b875fcab78c127da9195b4eb84148aebacc7606c246e5773d3f13bd5d9559d0a8bffac20a3a28c62df
codablock pushed a commit to codablock/dash that referenced this pull request Sep 26, 2019
eac64bb [qa] Test nMinimumChainWork (Suhas Daftuar)
0311836 Allow setting nMinimumChainWork on command line (Suhas Daftuar)

Pull request description:

  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 bitcoin#10345, which proposes a new use of `nMinimumChainWork`.

Tree-SHA512: fe4d8f4f289697615c98d8760f1cc74c076110310ea0b5b875fcab78c127da9195b4eb84148aebacc7606c246e5773d3f13bd5d9559d0a8bffac20a3a28c62df
codablock pushed a commit to codablock/dash that referenced this pull request Sep 29, 2019
eac64bb [qa] Test nMinimumChainWork (Suhas Daftuar)
0311836 Allow setting nMinimumChainWork on command line (Suhas Daftuar)

Pull request description:

  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 bitcoin#10345, which proposes a new use of `nMinimumChainWork`.

Tree-SHA512: fe4d8f4f289697615c98d8760f1cc74c076110310ea0b5b875fcab78c127da9195b4eb84148aebacc7606c246e5773d3f13bd5d9559d0a8bffac20a3a28c62df
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 17, 2020
1141364 [trivial] (whitespace only) fix getblockchaininfo alignment (John Newbery)
bd9c181 [rpc] Add initialblockdownload to getblockchaininfo (John Newbery)

Pull request description:

  Exposing whether the node is in IBD would help for testing, and may be useful in general, particularly for developers.

  First discussed in bitcoin#10357 here: bitcoin#10357 (review)

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

   This PR currently implements the simplest way of doing this: adding an `initialblockdownload` field to `getblockchaininfo`. Other approaches we could take:

  1. add a new debug RPC method that exposes `IBD` and potentially other information.
  2. add a parameter to `getblockchaininfo`, eg `debug_info`, which would cause it to return debug information including IBD
  3. add a query string to the url `?debug=true` which would cause RPCs to return additional debug information.

  I quite like the idea of (3). Feedback on these and other approaches very much welcomed!

  @sdaftuar @laanwj

Tree-SHA512: a6dedd47f8c9bd38769cc597524466250041136feb33500644b9c48d0ffe4e3eeeb2587b5bbc6420364ebdd2667df807fbb50416f9a7913bbf11a14ea86dc0d4
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 22, 2020
1141364 [trivial] (whitespace only) fix getblockchaininfo alignment (John Newbery)
bd9c181 [rpc] Add initialblockdownload to getblockchaininfo (John Newbery)

Pull request description:

  Exposing whether the node is in IBD would help for testing, and may be useful in general, particularly for developers.

  First discussed in bitcoin#10357 here: bitcoin#10357 (review)

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

   This PR currently implements the simplest way of doing this: adding an `initialblockdownload` field to `getblockchaininfo`. Other approaches we could take:

  1. add a new debug RPC method that exposes `IBD` and potentially other information.
  2. add a parameter to `getblockchaininfo`, eg `debug_info`, which would cause it to return debug information including IBD
  3. add a query string to the url `?debug=true` which would cause RPCs to return additional debug information.

  I quite like the idea of (3). Feedback on these and other approaches very much welcomed!

  @sdaftuar @laanwj

Tree-SHA512: a6dedd47f8c9bd38769cc597524466250041136feb33500644b9c48d0ffe4e3eeeb2587b5bbc6420364ebdd2667df807fbb50416f9a7913bbf11a14ea86dc0d4
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 22, 2020
1141364 [trivial] (whitespace only) fix getblockchaininfo alignment (John Newbery)
bd9c181 [rpc] Add initialblockdownload to getblockchaininfo (John Newbery)

Pull request description:

  Exposing whether the node is in IBD would help for testing, and may be useful in general, particularly for developers.

  First discussed in bitcoin#10357 here: bitcoin#10357 (review)

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

   This PR currently implements the simplest way of doing this: adding an `initialblockdownload` field to `getblockchaininfo`. Other approaches we could take:

  1. add a new debug RPC method that exposes `IBD` and potentially other information.
  2. add a parameter to `getblockchaininfo`, eg `debug_info`, which would cause it to return debug information including IBD
  3. add a query string to the url `?debug=true` which would cause RPCs to return additional debug information.

  I quite like the idea of (3). Feedback on these and other approaches very much welcomed!

  @sdaftuar @laanwj

Tree-SHA512: a6dedd47f8c9bd38769cc597524466250041136feb33500644b9c48d0ffe4e3eeeb2587b5bbc6420364ebdd2667df807fbb50416f9a7913bbf11a14ea86dc0d4
barrystyle pushed a commit to PACGlobalOfficial/PAC that referenced this pull request Jan 22, 2020
eac64bb [qa] Test nMinimumChainWork (Suhas Daftuar)
0311836 Allow setting nMinimumChainWork on command line (Suhas Daftuar)

Pull request description:

  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 bitcoin#10345, which proposes a new use of `nMinimumChainWork`.

Tree-SHA512: fe4d8f4f289697615c98d8760f1cc74c076110310ea0b5b875fcab78c127da9195b4eb84148aebacc7606c246e5773d3f13bd5d9559d0a8bffac20a3a28c62df
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
1141364 [trivial] (whitespace only) fix getblockchaininfo alignment (John Newbery)
bd9c181 [rpc] Add initialblockdownload to getblockchaininfo (John Newbery)

Pull request description:

  Exposing whether the node is in IBD would help for testing, and may be useful in general, particularly for developers.

  First discussed in bitcoin#10357 here: bitcoin#10357 (review)

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

   This PR currently implements the simplest way of doing this: adding an `initialblockdownload` field to `getblockchaininfo`. Other approaches we could take:

  1. add a new debug RPC method that exposes `IBD` and potentially other information.
  2. add a parameter to `getblockchaininfo`, eg `debug_info`, which would cause it to return debug information including IBD
  3. add a query string to the url `?debug=true` which would cause RPCs to return additional debug information.

  I quite like the idea of (3). Feedback on these and other approaches very much welcomed!

  @sdaftuar @laanwj

Tree-SHA512: a6dedd47f8c9bd38769cc597524466250041136feb33500644b9c48d0ffe4e3eeeb2587b5bbc6420364ebdd2667df807fbb50416f9a7913bbf11a14ea86dc0d4
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants