Skip to content

[Qt]: fixes m_assumed_blockchain_size variable value #15183

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.

Sign up for GitHub

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

Jump to bottom
Merged
maflcko merged 1 commit into from
Feb 14, 2019
Merged

[Qt]: fixes m_assumed_blockchain_size variable value #15183

maflcko merged 1 commit into from
Feb 14, 2019

Conversation

@marcoagner
Copy link
Contributor

@marcoagner marcoagner commented Jan 16, 2019 •
edited
Loading

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.

@marcoagner marcoagner mentioned this pull request Jan 16, 2019
Merged
@maflcko maflcko added this to the 0.18.0 milestone Jan 16, 2019
@maflcko
Copy link
Member

maflcko 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
Copy link
Member

laanwj 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
Copy link
Contributor Author

marcoagner commented Jan 17, 2019 •
edited
Loading

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
Copy link
Member

laanwj 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
Copy link
Member

laanwj 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

@maflcko
Copy link
Member

maflcko 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
Copy link
Contributor Author

marcoagner 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
Copy link
Member

Sjors commented Feb 11, 2019

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

@laanwj
Copy link
Member

laanwj commented Feb 12, 2019

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

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

@maflcko
Copy link
Member

maflcko commented Feb 12, 2019

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

@promag
Copy link
Contributor

promag commented Feb 12, 2019

Concept ACK.

Maybe also add any useful commands to the release process doc

@Sjors what do you mean?

@Sjors
Copy link
Member

Sjors commented Feb 13, 2019

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

@laanwj
Copy link
Member

laanwj 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
Copy link
Contributor

jonasschnelli commented Feb 14, 2019

Can we also bump Testnet within the same PR?

@marcohextor
fixes m_assumed_blockchain_size variables values:
8c3fdd3 
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
Copy link
Contributor Author

marcoagner commented Feb 14, 2019 •
edited
Loading

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
Copy link
Member

fanquake commented Feb 14, 2019

utACK 8c3fdd3

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Feb 14, 2019
Merge bitcoin#15183: [Qt]: fixes m_assumed_blockchain_size variable v…
38989ab 
…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
@maflcko maflcko merged commit 8c3fdd3 into bitcoin:master Feb 14, 2019
@marcoagner marcoagner mentioned this pull request Feb 21, 2019
Merged
laanwj added a commit that referenced this pull request Sep 30, 2019
@laanwj
Merge #15459: doc: add how to calculate blockchain and chainstate siz…
981ec9b 
…e variables to release process

eb4c43e doc: documents how to calculate m_assumed_blockchain_size and m_assumed_chain_state_size on the release process. (marcoagner)

Pull request description:

  Regarding [this](#15183 (comment)) on #15183.
  Added an "Additional information" section for this which seems reasonable to me but may not be the best place for this. Also, let me know if anything else should be documented here (like more details).

ACKs for top commit:
  laanwj:
    ACK eb4c43e

Tree-SHA512: 7e6fc46740daa01dd9be5a8da7846e7a9f7fa866bf31fdc2cb252f90c698cfd6ef954f9588f7abcebda2355ec2b2a380635e14a164e53e77d38abefa3e2cc698
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 2, 2019
@laanwj
Merge bitcoin#15459: doc: add how to calculate blockchain and chainst…
8ce6b10 
…ate size variables to release process

eb4c43e doc: documents how to calculate m_assumed_blockchain_size and m_assumed_chain_state_size on the release process. (marcoagner)

Pull request description:

  Regarding [this](bitcoin#15183 (comment)) on bitcoin#15183.
  Added an "Additional information" section for this which seems reasonable to me but may not be the best place for this. Also, let me know if anything else should be documented here (like more details).

ACKs for top commit:
  laanwj:
    ACK eb4c43e

Tree-SHA512: 7e6fc46740daa01dd9be5a8da7846e7a9f7fa866bf31fdc2cb252f90c698cfd6ef954f9588f7abcebda2355ec2b2a380635e14a164e53e77d38abefa3e2cc698
humbleDasher pushed a commit to humbleDasher/dash that referenced this pull request Dec 15, 2021
@laanwj
Merge bitcoin#15459: doc: add how to calculate blockchain and chainst…
91310cf 
…ate size variables to release process

eb4c43e doc: documents how to calculate m_assumed_blockchain_size and m_assumed_chain_state_size on the release process. (marcoagner)

Pull request description:

  Regarding [this](bitcoin#15183 (comment)) on bitcoin#15183.
  Added an "Additional information" section for this which seems reasonable to me but may not be the best place for this. Also, let me know if anything else should be documented here (like more details).

ACKs for top commit:
  laanwj:
    ACK eb4c43e

Tree-SHA512: 7e6fc46740daa01dd9be5a8da7846e7a9f7fa866bf31fdc2cb252f90c698cfd6ef954f9588f7abcebda2355ec2b2a380635e14a164e53e77d38abefa3e2cc698
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

GUI

Projects

None yet

Milestone

0.18.0

Development

Successfully merging this pull request may close these issues.

8 participants

@marcoagner @maflcko @laanwj @Sjors @promag @jonasschnelli @fanquake @marcohextor