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

marcoagner · 2018-05-11T13:29:20Z

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!

ghost · 2018-05-11T17:32:45Z

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

sipa · 2018-05-11T17:33:26Z

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 · 2018-05-11T19:37:55Z

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

maflcko · 2018-05-11T20:22:01Z

I will update this PR with the CChainParams approach.

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

promag · 2018-05-12T16:24:01Z

Concept ACK.

practicalswift · 2018-05-12T20:39:06Z

Concept ACK

jonasschnelli · 2018-05-14T08:07:17Z

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

marcoagner · 2018-05-14T08:41:42Z

@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?

maflcko · 2018-05-23T15:40:50Z

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

jonasschnelli · 2018-05-25T08:54:09Z

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

sipa · 2018-05-28T18:09:44Z

doc/release-process.md

This line can be removed now.

ooops, okay

sipa · 2018-05-28T18:10:21Z

src/chainparams.cpp

This sounds a bit excessive for regtest.

do you have any suggestion for this value? thanks

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

sipa · 2018-05-28T18:10:55Z

src/qt/intro.cpp

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

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

marcoagner · 2018-05-29T10:27:22Z

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.

DrahtBot · 2018-09-28T20:19:43Z

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.

practicalswift · 2018-10-07T09:24:41Z

src/qt/intro.cpp

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

practicalswift · 2018-10-07T09:25:54Z

src/qt/intro.cpp

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

practicalswift · 2018-10-07T09:26:52Z

src/qt/intro.cpp

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

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.

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

sipa · 2018-10-23T02:14:43Z

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

jonasschnelli · 2018-10-23T07:18:06Z

Re-utACK 9d0e528

maflcko · 2018-10-23T20:20:43Z

utACK 9d0e528 non-qt code.

marcoagner · 2019-01-11T16:56:58Z

Should anything else be done to have this merged?

jonasschnelli · 2019-01-12T00:32:41Z

Tested ACK 9d0e528

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 · 2019-01-12T12:14:49Z

Thank you for testing, @jonasschnelli.

maflcko

Some post-merge observations

maflcko · 2018-10-24T11:59:09Z

src/qt/intro.cpp

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


Note that this will hide all error messages from selectParams

maflcko · 2019-01-13T20:51:37Z

src/chainparams.cpp

        pchMessageStart[3] = 0xd9;
        nDefaultPort = 8333;
        nPruneAfterHeight = 100000;
+        m_assumed_blockchain_size = 200;


This used to be 220?

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

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

Yes, thanks. Like this? -> #15183

maflcko · 2019-01-13T20:52:05Z

src/qt/intro.cpp

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


s/0/nullptr/

Is another PR for this okay?

…alue 8c3fdd3 fixes m_assumed_blockchain_size variables values: (marcoagner) Pull request description: This is used by Qt but I'm not sure if this is the right tag here. Please, edit the title if there's something better. `m_assumed_blockchain_size` (src/chainparams.cpp:CChainParams) was `BLOCK_CHAIN_SIZE` (src/qt/intro.cpp) and while the transition was being made by PR 13216 (merged commit: 9d0e528), 3fc2063 changed its value from 200 to 220, which 9d0e528 ended up reverting. So, as per MarcoFalke's suggestion (bitcoin#13216 (comment)), I'm bumping it to 240 before 0.18 is branched to avoid any confusion. Anything else (e.g. constexpr) that should/could be done here? Thanks. Tree-SHA512: 4319739b870a2b96a57f268f9edc7dd9f9eff5c4ca3b01863e6b861b9ca58c245416ce362dae54d1673e3d5b1c7f5a16e4031842af250e1b1f0a5109b75fb3c3

Summary: - 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. Backport of core [[ bitcoin/bitcoin#13216 | PR13216 ]]. Test Plan: ninja check-all Build the win64 client and run it on my windows machine. Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D5748

… on intro 9d0e528 implements different disk sizes for different networks on intro (marcoagner) Pull request description: Fixes bitcoin#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 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 added the GUI label May 11, 2018

fanquake mentioned this pull request May 14, 2018

[trivial] Qt: Add testnet chainsize constants and switch for intro #13229

Closed

sipa reviewed May 28, 2018

View reviewed changes

This was referenced Jul 27, 2018

0.17: Pre-branch maintenance #13780

Merged

Don't use zero as null pointer constant. Use nullptr (C++11). #13802

Closed

DrahtBot added the Needs rebase label Aug 8, 2018

DrahtBot removed the Needs rebase label Sep 28, 2018

DrahtBot mentioned this pull request Sep 28, 2018

Use non-throwing type-safe ChainType where possible #14309

Closed

practicalswift reviewed Oct 7, 2018

View reviewed changes

src/qt/intro.cpp Outdated

Copy link

Contributor

practicalswift Oct 7, 2018

Choose a reason for hiding this comment

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

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

practicalswift reviewed Oct 7, 2018

View reviewed changes

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

This was referenced Jan 13, 2019

Qt: Replace remaining 0 with nullptr #15114

Merged

build: Optionally enable -Wzero-as-null-pointer-constant #15112

Closed

maflcko reviewed Jan 13, 2019

View reviewed changes

marcoagner mentioned this pull request Jan 16, 2019

[Qt]: fixes m_assumed_blockchain_size variable value #15183

Merged

jtimon mentioned this pull request Jun 13, 2019

Testchains: Introduce custom chain whose constructor... #8994

Closed

12 tasks

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

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

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

Uh oh!

Conversation

marcoagner commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 11, 2018

Uh oh!

sipa commented May 11, 2018

Uh oh!

marcoagner commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 11, 2018

Uh oh!

promag commented May 12, 2018

Uh oh!

practicalswift commented May 12, 2018

Uh oh!

jonasschnelli commented May 14, 2018

Uh oh!

marcoagner commented May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 23, 2018

Uh oh!

jonasschnelli commented May 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoagner commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift Oct 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Oct 23, 2018

Uh oh!

jonasschnelli commented Oct 23, 2018

Uh oh!

maflcko commented Oct 23, 2018

Uh oh!

marcoagner commented Jan 11, 2019

Uh oh!

jonasschnelli commented Jan 12, 2019

Uh oh!

marcoagner commented Jan 12, 2019

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoagner Jan 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

marcoagner commented May 11, 2018 •

edited

Loading

marcoagner commented May 11, 2018 •

edited

Loading

marcoagner commented May 14, 2018 •

edited

Loading

marcoagner commented May 29, 2018 •

edited

Loading

DrahtBot commented Sep 28, 2018 •

edited

Loading

practicalswift Oct 7, 2018 •

edited

Loading

marcoagner Jan 13, 2019 •

edited

Loading