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

[Qt]: fixes m_assumed_blockchain_size variable value #15183

Merged
merged 1 commit into from Feb 14, 2019

Conversation

Projects
None yet
7 participants
@marcoagner
Copy link
Contributor

commented Jan 16, 2019

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

Tagging 0.18, this should be done prior to a major release: https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md#release-process

Bump testnet as well?

@laanwj laanwj added the GUI label Jan 16, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

Yes, probably best to do this just before branching off 0.18, to have the most up-to-date number.

@marcoagner

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2019

I will then keep this open (if there's no problem, ofc) and rebase it just before branching with the fix/bump of m_assumed_blockchain_size for both mainnet and testnet. Thanks.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

Sounds good to me!

@laanwj laanwj changed the title [Qt]: fixes m_assumed_blockchain_size variable value: [Qt]: fixes m_assumed_blockchain_size variable value Feb 6, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

FYI planned split-off date for the 0.18 branch is 2019-03-01, if you have time please update it on that date or a few days before that

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

I think this can be done right now, we have to add an overhead anyway. Being off by one GB either way won't hurt.

@marcoagner

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

Thank you for the heads up.
I'll be able to update this in a few days when I come back home and access my full node drives to check the exact values plus overhead by myself. (unless somebody suggests one right now, ofc - which happens to be most of the work here haha)

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

Concept ACK. Maybe also add any useful commands to the release process doc.

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

Being off by one GB either way won't hurt.

Also good point. You'll want to round up anyhow.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

ACK bd2893b (240GB seems fine, could also bump testnet?)

@promag

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

Concept ACK.

Maybe also add any useful commands to the release process doc

@Sjors what do you mean?

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

@promag I mean the release process doc could use instructions on how you actually calculate the size correctly.

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

I simply take the size of a synced ~/.bitcoin and do nothing special to compute it. After all it's used as a guideline for how much space the user needs on their drive, not strictly just the blockchain. I agree documenting that would make sense.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Can we also bump Testnet within the same PR?

@marcoagner marcoagner force-pushed the marcoagner:fix_m_assumed_blockchain_size branch from bd2893b to bb50c90 Feb 14, 2019

fixes m_assumed_blockchain_size variables values:
This commit was a fix to `m_assumed_blockchain_size` reverted from
3fc2063's 220 to 9d0e528's 200 since work on 9d0e528 was being done in
parallel and ended up reverting `m_assumed_blockchain_size`.

This commits is now a intended to be a bump of
`m_assumed_blockchain_size` for both mainnet and testnet for new
reasonable values.

@marcoagner marcoagner force-pushed the marcoagner:fix_m_assumed_blockchain_size branch from bb50c90 to 8c3fdd3 Feb 14, 2019

@marcoagner

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

8c3fdd3 now bumps the variable for both mainnet and testnet.
Testnet is now bumped to 30GB; any disagreement to this value?

As for @Sjors's suggestion, it seems proper to document this in another PR (I can update the release process doc for this and open the PR, ofc).

@fanquake

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

utACK 8c3fdd3

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Feb 14, 2019

Merge bitcoin#15183: [Qt]: fixes m_assumed_blockchain_size variable v…
…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

@MarcoFalke MarcoFalke merged commit 8c3fdd3 into bitcoin:master Feb 14, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.