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

Correct description of blocksdir default value #16010

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@kristapsk
Copy link
Contributor

commented May 11, 2019

I recently moved all my Bitcoin datadir except blocksdir from a slow storage to the fast one. By previous description of blocksdir default I expected that blocksdir value should be "old_datadir/blocks", but it actually should be "old_datadir", as it expects / creates "blocks" subdirectory under it, so it's default is actually <datadir>, not <datadir>/blocks. See also GetDataDir() in src/util/system.cpp.

@fanquake fanquake added the Docs label May 11, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented May 12, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15457 (Check std::system for -[alert|block|wallet]notify by Sjors)
  • #14364 (doc: Clarify -blocksdir usage by sangaman)
  • #12557 ([WIP] 64 bit iOS device support by Sjors)

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.

@promag

This comment has been minimized.

Copy link
Member

commented May 12, 2019

From

bitcoin/src/util/system.cpp

Lines 724 to 735 in e79bbb7

if (gArgs.IsArgSet("-blocksdir")) {
path = fs::system_complete(gArgs.GetArg("-blocksdir", ""));
if (!fs::is_directory(path)) {
path = "";
return path;
}
} else {
path = GetDataDir(false);
}
path /= BaseParams().DataDir();
path /= "blocks";
looks like the default is correct?

@kristapsk

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

I'm not saying default is not correct, I am saying it's current description in help is a lie.

@promag

This comment has been minimized.

Copy link
Member

commented May 12, 2019

Oh right! Didn't check history but is current behavior correct? I mean, is it normal to expect subdirectory blocks/ on the specified -blocksdir?

Edit: see #12828 (comment)

I think the help should say something like Specify root directory for blocks (*blk) and undo (*.rev) files (default: <datadir>).

@@ -380,7 +380,7 @@ void SetupServerArgs()
gArgs.AddArg("-version", "Print version and exit", false, OptionsCategory::OPTIONS);
gArgs.AddArg("-alertnotify=<cmd>", "Execute command when a relevant alert is received or we see a really long fork (%s in cmd is replaced by message)", false, OptionsCategory::OPTIONS);
gArgs.AddArg("-assumevalid=<hex>", strprintf("If this block is in the chain assume that it and its ancestors are valid and potentially skip their script verification (0 to verify all, default: %s, testnet: %s)", defaultChainParams->GetConsensus().defaultAssumeValid.GetHex(), testnetChainParams->GetConsensus().defaultAssumeValid.GetHex()), false, OptionsCategory::OPTIONS);
gArgs.AddArg("-blocksdir=<dir>", "Specify blocks directory (default: <datadir>/blocks)", false, OptionsCategory::OPTIONS);
gArgs.AddArg("-blocksdir=<dir>", "Specify blocks directory (default: <datadir>)", false, OptionsCategory::OPTIONS);

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 13, 2019

Member

Should this put blocks into some quotes to clarify this is a string literal and not a English word (that e.g. can be translated). Thankfully we no longer translate those, but could still make it clear?

Suggested change
gArgs.AddArg("-blocksdir=<dir>", "Specify blocks directory (default: <datadir>)", false, OptionsCategory::OPTIONS);
gArgs.AddArg("-blocksdir=<dir>", "Specify 'blocks' directory (default: <datadir>)", false, OptionsCategory::OPTIONS);
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented May 13, 2019

ACK

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Needs rebase
@kristapsk

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

#14364 got merged (noticed existence of such PR only after pushing this one), this isn't actual anymore.

@kristapsk kristapsk closed this May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.