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

Intro: Never change the prune checkbox after the user has touched it #658

Merged
merged 2 commits into from
Feb 12, 2024

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Aug 30, 2022

Re-PR from bitcoin/bitcoin#18729

Now includes a bugfix too (-prune=2+ disabled the checkbox, but -prune=0/1 did not; this behaviour is necessary since -prune overrides GUI settings)

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 21, 2022

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK hebasto
Stale ACK hernanmarino

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

Conflicts

No conflicts as of last run.

@hebasto hebasto added the UX All about "how to get things done" label Oct 26, 2022
Copy link
Contributor

@hernanmarino hernanmarino left a comment

Choose a reason for hiding this comment

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

cr ACK 76cb089 and tested ACK

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

Title and description mismatch of the PR confuse me, I tested bitcoin-qt before and after the change and I don't see the difference in the behaviour (from the UI point of view, haven't checked the proper prunning).
I've ticked & unticked the prune checkbox and set a value, verifying the value is persisted in the config. Also combined the previous starting qt with -prune param set to 0, 1 and 600.
Perhaps I've some config that bypass this change? Could you please clarify? Sorry if I miss anything too obvius here.

@hebasto
Copy link
Member

hebasto commented Dec 6, 2022

-prune=2+ disabled the checkbox

Looks like a bug in the master branch, as it should be like that:

$ ./src/bitcoind -prune=2
Error: Prune configured below the minimum of 550 MiB.  Please use a higher number.

@hebasto
Copy link
Member

hebasto commented Dec 6, 2022

@ryanofsky Could you be interesting in reviewing of this PR?

@hebasto
Copy link
Member

hebasto commented May 26, 2023

Closing due to a long period of inactivity here. Feel free to reopen.

@hebasto hebasto closed this May 26, 2023
@achow101
Copy link
Member

Reopening by request

@achow101 achow101 reopened this Jun 29, 2023
@luke-jr luke-jr force-pushed the intro_dont_change_user_prune branch from 76cb089 to 051b049 Compare June 29, 2023 23:23
@luke-jr
Copy link
Member Author

luke-jr commented Jun 29, 2023

Rebased

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 5, 2024

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK bee0ffb, both commits are improvements of the current behaviour. Tested on Ubuntu 23.10.

@hebasto hebasto merged commit 2afbacc into bitcoin-core:master Feb 12, 2024
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Feb 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI failed UX All about "how to get things done"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants