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

[Qt] implements concept for different disk sizes on intro #13216

Merged
merged 1 commit into from Jan 12, 2019

Conversation

Projects
None yet
9 participants
@marcoagner
Copy link
Contributor

marcoagner commented May 11, 2018

Fixes #13213.
Mostly, I layed out the concept to open the PR for refinement and getting feedback if the approach is okay. Changes are expected.

Two points:

  • The values for both new consts TESTNET_BLOCK_CHAIN_SIZE and TESTNET_CHAIN_STATE_SIZE is certainly not optimal; I just checked the size of my testnet3 related dirs and set them to little bit higher values. Which values should be used?
  • Should we do something like this to regtest? Or these "niceties" do not matter when on regtest?

Thanks!

@marcoagner marcoagner changed the title [qt] implements concept for different disk sizes on intro [Qt] implements concept for different disk sizes on intro May 11, 2018

@fanquake fanquake added the GUI label May 11, 2018

@GrumpyKiitty

This comment has been minimized.

Copy link
Contributor

GrumpyKiitty commented May 11, 2018

Tested on Ubuntu 16.04: Image
Nano nit: '200GB'/'15GB' should be '200 GB'/'15 GB' for the sake of consistency.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented May 11, 2018

Thanks for addressing this issue.

We generally try to avoid having explicit branching on the network type. Instead, I would suggest adding a variable to CChainParams (which is initialized differently in the different network versions).

@marcoagner

This comment has been minimized.

Copy link
Contributor

marcoagner commented May 11, 2018

Okay, thanks! This will be cleaner.
I will update this PR with the CChainParams approach.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented May 11, 2018

I will update this PR with the CChainParams approach.

Don't forget to update the release process document as well.

@promag

This comment has been minimized.

Copy link
Member

promag commented May 12, 2018

Concept ACK.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented May 12, 2018

Concept ACK

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented May 14, 2018

utACK,... maybe simplify the code according to #13229

@marcoagner

This comment has been minimized.

Copy link
Contributor

marcoagner commented May 14, 2018

@jonasschnelli, if #13229 is acceptable, I prefer dropping this PR in favor of #13229. No need to re-do the work here and your approach seems better.

Just as a heads up, it seems that #13229 also explicitly branches the network type (see @sipa's suggestion above). I'm new to the code base and I can't tell for sure if using CChainParams for this Qt issue will be better for the rest of the code as a whole.

What do you think?

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented May 23, 2018

I think it is fine to put it in chainparams, if you call it m_assumed_size (similar to "assumed valid").

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented May 25, 2018

@jonasschnelli, if #13229 is acceptable, I prefer dropping this PR in favor of #13229. No need to re-do the work here and your approach seems better.

Please continue with this and feel free to grab code from 13229...

I had the feeling that this would be GUI only and not appropriate to put Into the chainparams holder... but @sipa and @MarcoFalke think its better there which I guess they are right...

@marcoagner marcoagner force-pushed the marcoagner:fix_testnet_disk_usage_msg branch 3 times, most recently from 2dfd5b6 to d9ac4a2 May 28, 2018

@@ -23,6 +23,7 @@ Before every major release:

* Update hardcoded [seeds](/contrib/seeds/README.md), see [this pull request](https://github.com/bitcoin/bitcoin/pull/7415) for an example.
* Update [`BLOCK_CHAIN_SIZE`](/src/qt/intro.cpp) to the current size plus some overhead.

This comment has been minimized.

@sipa

sipa May 28, 2018

Member

This line can be removed now.

This comment has been minimized.

@marcoagner

marcoagner May 28, 2018

Contributor

ooops, okay

@@ -317,6 +321,8 @@ class CRegTestParams : public CChainParams {
pchMessageStart[3] = 0xda;
nDefaultPort = 18444;
nPruneAfterHeight = 1000;
m_assumed_blockchain_size = 200;

This comment has been minimized.

@sipa

sipa May 28, 2018

Member

This sounds a bit excessive for regtest.

This comment has been minimized.

@marcoagner

marcoagner May 28, 2018

Contributor

do you have any suggestion for this value? thanks

This comment has been minimized.

@sipa

sipa May 28, 2018

Member

0 or 1? :)

It doesn't matter, and it's also unpredictable, as it depends on what blocks are created (regtest is purely for testing purposes, and all blocks are typically explicitly created in the test; there is no real network).

@@ -12,6 +12,7 @@

#include <qt/guiutil.h>

#include <chainparams.h>

This comment has been minimized.

@sipa

sipa May 28, 2018

Member

You can't include core files directly (this is part of ongoing work to separate the two more cleanly); you'll need to go through the node interface (see the include below).

This comment has been minimized.

@marcoagner

marcoagner May 28, 2018

Contributor

I see! Thank you, I will fix this as soon as I get back.

@marcoagner marcoagner force-pushed the marcoagner:fix_testnet_disk_usage_msg branch from d9ac4a2 to 071689a May 29, 2018

@marcoagner

This comment has been minimized.

Copy link
Contributor

marcoagner commented May 29, 2018

Updated do address @sipa's review.
My only comment for now is that the intro message is a bit funny warning that you need at least 0GB... even though that's accurate. :)
The message already didn't totally reflect regtest's reality anyway but let me know if anybody thinks this should be addressed for some reason I can't think of.

@marcoagner marcoagner force-pushed the marcoagner:fix_testnet_disk_usage_msg branch from 071689a to 0c1ee2f Jun 29, 2018

@marcoagner marcoagner force-pushed the marcoagner:fix_testnet_disk_usage_msg branch from 0c1ee2f to 82b5faa Sep 28, 2018

@DrahtBot DrahtBot removed the Needs rebase label Sep 28, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Sep 28, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15114 (Qt: Replace remaining 0 with nullptr by Empact)
  • #15112 (build: Optionally enable -Wzero-as-null-pointer-constant by Empact)

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.

/* Total required space (in GB) depending on user choice (prune, not prune) */
static uint64_t requiredSpace;

/* Check free space asynchronously to prevent hanging the UI thread.
/* Total required space (in GB) depending on user choice (prune, not prune) */

This comment has been minimized.

@practicalswift

practicalswift Oct 7, 2018

Member

Please note that the comment is already opened with /* on the line above :-)

/* Minimum free space (in GB) needed for data directory when pruned; Does not include prune target */
static const uint64_t CHAIN_STATE_SIZE = 3;
/* Total required space (in GB) depending on user choice (prune, not prune) */
static uint64_t requiredSpace;

This comment has been minimized.

@practicalswift

practicalswift Oct 7, 2018

Member

Please note that requiredSpace is redefined four lines below :-)

/* Use selectParams here to guarantee Params() can be used by node interface */
try {
node.selectParams(gArgs.GetChainName());
} catch(std::exception) {

This comment has been minimized.

@practicalswift

practicalswift Oct 7, 2018

Member

Should catch by reference instead of value. Also missing space before ( :-)

This comment has been minimized.

@marcoagner

marcoagner Oct 7, 2018

Contributor

Except this one, other issues were caused by a (obviously bad) merge conflict resolution - bad. I'll take a lot more care so it doesn't happen again.
Thank you for taking the time to review it.

implements different disk sizes for different networks on intro
- Creates m_assumed_blockchain_size and m_assumed_chain_state_size on CChainParams.
- Implements access to CChainParams' m_assumed_blockchain_size and m_assumed_chain_state_size on node interface.
- Implements m_assumed_blockchain_size and m_assumed_chain_state_size on qt/intro via node interface.
- Updates release process document with the new CChainParam's values.

@marcoagner marcoagner force-pushed the marcoagner:fix_testnet_disk_usage_msg branch from 82b5faa to 9d0e528 Oct 7, 2018

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Oct 23, 2018

Concept ACK. Code review ACK for non-Qt code 9d0e528

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Oct 23, 2018

Re-utACK 9d0e528

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 23, 2018

utACK 9d0e528 non-qt code.

@marcoagner

This comment has been minimized.

Copy link
Contributor

marcoagner commented Jan 11, 2019

Should anything else be done to have this merged?

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jan 12, 2019

Tested ACK 9d0e528

@jonasschnelli jonasschnelli merged commit 9d0e528 into bitcoin:master Jan 12, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonasschnelli added a commit that referenced this pull request Jan 12, 2019

Merge #13216: [Qt] implements concept for different disk sizes on intro
9d0e528 implements different disk sizes for different networks on intro (marcoagner)

Pull request description:

  Fixes #13213.
  Mostly, I layed out the concept to open the PR for refinement and getting feedback if the approach is okay. Changes are expected.

  Two points:
  - The values for both new consts `TESTNET_BLOCK_CHAIN_SIZE` and `TESTNET_CHAIN_STATE_SIZE` is certainly not optimal; I just checked the size of my testnet3 related dirs and set them to little bit higher values. Which values should be used?
  - Should we do something like this to regtest? Or these "niceties" do not matter when on regtest?

  Thanks!

Tree-SHA512: 8ae87a29fa8356b899e7a823c76cde793d9126b4ee59554d7a2a8edb088fe42a19976b34c06c2fd4a98a727e1e4971dd983f42b6093ea6caa255b45004e22bb4
@marcoagner

This comment has been minimized.

Copy link
Contributor

marcoagner commented Jan 12, 2019

Thank you for testing, @jonasschnelli.

@MarcoFalke
Copy link
Member

MarcoFalke left a comment

Some post-merge observations

try {
node.selectParams(gArgs.GetChainName());
} catch (const std::exception&) {
return false;

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 13, 2019

Member

Note that this will hide all error messages from selectParams

@@ -107,6 +107,8 @@ class CMainParams : public CChainParams {
pchMessageStart[3] = 0xd9;
nDefaultPort = 8333;
nPruneAfterHeight = 100000;
m_assumed_blockchain_size = 200;

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 13, 2019

Member

This used to be 220?

This comment has been minimized.

@marcoagner

marcoagner Jan 13, 2019

Contributor

Well spotted, thanks! This was 200 when the PR was first made but changed down the line with this maintenance for 0.17: 78dae8c. Do you think this should be addressed here or could wait for the next maintenance (since the release process now asks for this to be changed)?

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 14, 2019

Member

Could bump it to 240 before 0.18 is branched, so we don't forget about it?

This comment has been minimized.

@marcoagner

marcoagner Jan 16, 2019

Contributor

Yes, thanks. Like this? -> #15183

/* If current default data directory does not exist, let the user choose one */
Intro intro;
Intro intro(0, node.getAssumedBlockchainSize(), node.getAssumedChainStateSize());

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 13, 2019

Member

s/0/nullptr/

This comment has been minimized.

@marcoagner

marcoagner Jan 16, 2019

Contributor

Is another PR for this okay?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment