Skip to content

Bugfix: Correct first-run free space checks #29678

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
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Bugfix: Correct first-run free space checks #29678

wants to merge 3 commits into from
+4 −4

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Mar 19, 2024

It's not clear what m_assumed_*_size are actually set based on, but historically it was in GB, not GiB, and that's still used in the GUI which is more user-facing.

Could just as easily change the GUI if GiB is preferred.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 19, 2024 •
edited
Loading

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29678.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK hebasto, murchandamus, BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #31974 (Drop testnet3 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.

@Sjors
Copy link
Member

Sjors commented Mar 19, 2024 •
edited
Loading

I'm not sure what the latest convention is, cc @hebasto. A few years ago #15163 made it so RPC, code and GUI all use KiB / MiB / GiB.

(though for pruning it seems the config file uses MiB, but the GUI converts it to GB - yet there are translated error strings using MiB, confusing...)

This was referenced Apr 1, 2024
Merged
Merged
@luke-jr luke-jr mentioned this pull request May 28, 2024
Closed
@luke-jr luke-jr force-pushed the fix_init_lowdisk_warning_reqd branch from 279c4ee to 847ad93 Compare May 28, 2024 15:02
@DrahtBot DrahtBot mentioned this pull request May 30, 2024
Closed
24 tasks
@DrahtBot DrahtBot mentioned this pull request Jun 7, 2024
Merged
@fanquake
Copy link
Member

@hebasto can you follow up given the gui / translation Qs here. This also needs a rebase.

@hebasto
Copy link
Member

hebasto commented Jul 10, 2024

Indeed, there is inconsistency in GB/GiB unit usage.

For instance, the value of the m_assumed_blockchain_size variable in GiB is used with the "GB" units in the user-faced messages here:

bitcoin/src/init.cpp

Line 1707 in 45f757c

"Approximately %u GB of data will be stored in this directory."
and here:

bitcoin/src/qt/forms/intro.ui

Line 206 in 45f757c

<string>When you click OK, %1 will begin to download and process the full %4 block chain (%2 GB) starting with the earliest transactions in %3 when %4 initially launched.</string>

Concept ACK.

It would be nice to mention the second commit changes in the PR description, no?

Could just as easily change the GUI if GiB is preferred.

The GUI now uses "GB" (SI prefix) as a unit, based on the mindset of non-technical/non-CS users.

@GBKS What are modern guidelines in this regard?

@Sjors

I'm not sure what the latest convention is, cc @hebasto. A few years ago #15163 made it so RPC, code and GUI all use KiB / MiB / GiB.

(though for pruning it seems the config file uses MiB, but the GUI converts it to GB - yet there are translated error strings using MiB, confusing...)

For the GUI, the conversion to/from GB is still used:

bitcoin/src/qt/optionsmodel.h

Lines 26 to 34 in 45f757c

/**
* Convert configured prune target MiB to displayed GB. Round up to avoid underestimating max disk usage.
*/
static inline int PruneMiBtoGB(int64_t mib) { return (mib * 1024 * 1024 + GB_BYTES - 1) / GB_BYTES; }
/**
* Convert displayed prune target GB to configured MiB. Round down so roundtrip GB -> MiB -> GB conversion is stable.
*/
static inline int64_t PruneGBtoMiB(int gb) { return gb * GB_BYTES / 1024 / 1024; }

@GBKS
Copy link

GBKS commented Jul 11, 2024

The GUI now uses "GB" (SI prefix) as a unit, based on the mindset of non-technical/non-CS users.

GB is standard for general audiences, and GiB for technical, scientific, or engineering contexts. I'd recommend keeping GB in the GUI.

I am curious about use cases where it is essential for the user to know the GiB value in order to make decisions.

@hebasto hebasto mentioned this pull request Jul 15, 2024
Open
1 task
@luke-jr luke-jr force-pushed the fix_init_lowdisk_warning_reqd branch from 847ad93 to c452d6c Compare September 2, 2024 19:53
@achow101 achow101 requested a review from murchandamus October 15, 2024 15:40
murchandamus
murchandamus reviewed Oct 17, 2024
View reviewed changes
Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

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

All of these changes seem consistent to me.

Concept ACK on generally using GB to refer to 1,000,000,000 bytes and GiB to refer to 1024³ bytes everywhere.

BrandonOdiwuor
BrandonOdiwuor reviewed Nov 18, 2024
View reviewed changes
Copy link
Contributor

@BrandonOdiwuor BrandonOdiwuor left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Concept ACK using GB to represent 10003 bytes for consistency with GUI

@DrahtBot DrahtBot mentioned this pull request Mar 3, 2025
Draft
@maflcko
Copy link
Member

maflcko commented May 4, 2025

(close-open for fresh GitHub CI, I guess the Cirrus failure can be ignored for now)

@maflcko maflcko closed this May 4, 2025
@maflcko maflcko reopened this May 4, 2025
This was referenced Jun 12, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
2 more reviewers

@murchandamus murchandamus murchandamus left review comments

@BrandonOdiwuor BrandonOdiwuor BrandonOdiwuor left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
CI failed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@luke-jr @DrahtBot @Sjors @fanquake @hebasto @GBKS @maflcko @murchandamus @BrandonOdiwuor