system: use %LOCALAPPDATA% as default datadir on windows #27064

pinheadmz · 2023-02-08T20:44:10Z

This PR changes the default datadir location on Windows from C:\Users\Username\AppData\Roaming\Bitcoin to C:\Users\Username\AppData\Local\Bitcoin. This change only applies to fresh installs. To preserve backwards compatibility, on startup we check for the existence of C:\Users\Username\AppData\Roaming\Bitcoin\chainstate and if it is there, we continue using the "Roaming" directory as the default datadir location.

Note that in Windows 11 this change may be moot:

Roaming data and settings is no longer supported as of Windows 11. The recommended replacement is Azure App Service. Azure App Service is widely supported, well documented, reliable, and supports cross-platform/cross-ecosystem scenarios such as iOS, Android and web. Settings stored here no longer roam (as of Windows 11), but the settings store is still available.

DrahtBot · 2023-02-08T20:44:13Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, achow101, BenWestgate

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 · 2023-02-08T21:17:34Z

cc @sipsorcery

ghost

utACK a2bfd52

Note that in Windows 11 this change may be moot

Win 11 isn't used a lot though.

Usage: https://gs.statcounter.com/os-version-market-share/windows/desktop/worldwide

Failure discussion: https://techcommunity.microsoft.com/t5/windows-11/why-users-are-not-upgrading-to-windows-11-is-windows-11-a/m-p/3611663

pinheadmz · 2023-02-16T16:08:15Z

Would this change need a release-notes-27064.md file? It is kinda changing a configuration model

pinheadmz · 2023-04-26T13:07:50Z

Push to 60cd07b:

rebase on master
add PR release note
change how we detect "old" directory use from looking for blocks/ to looking for chainstate/

hebasto · 2023-05-22T15:56:53Z

Concept ACK.

hebasto · 2023-05-22T16:00:14Z

src/common/args.cpp

-    return GetSpecialFolderPath(CSIDL_APPDATA) / "Bitcoin";
+    // Check for existence of datadir in old location and keep it there
+    fs::path legacy_path = GetSpecialFolderPath(CSIDL_APPDATA) / "Bitcoin";
+    if (fs::exists(legacy_path / "chainstate"))


Update the PR description:

we check for the existence of C:\Users\Username\AppData\Roaming\Bitcoin\blocks

?

Good catch thanks. (fixed) First draft of this PR did check for blocks directory but of course that is unreliable because it could be customized by the user.

hebasto · 2023-05-23T10:48:24Z

This change ignores non-main network data. I'm OK with that. But it seems worth mentioning in the docs.

pinheadmz · 2023-05-24T19:43:51Z

This change ignores non-main network data.

Ah I see, because I'm checking for Bitcoin\chainstate specifically but if a user only ever used signet, there would be a nested signet directory in there too. Is there anything else I can check for that always gets written to that root? Or maybe just check if Bitcoin\ is empty at all?

I'm OK with that. But it seems worth mentioning in the docs.

Sure but I feel like that's not super clean either.

luke-jr · 2023-06-22T20:08:45Z

Why would we prefer local over roaming explicitly?

pinheadmz · 2023-06-22T20:16:19Z

Why would we prefer local over roaming explicitly?

From https://www.makeuseof.com/tag/appdata-roaming-vs-local/

In the end, Roaming and Local are functionally identical for a home PC running Windows. In a domain environment, data in the Roaming folder will stay with a user's profile if they move to a different computer.

In other words, 500 GB of blockchain data would be copied to a user's remote workstation as they move around. Roaming is just supposed to be for small files like profile preferences, etc. Not big data stores.

More discussion in #2391

achow101 · 2023-09-26T15:56:10Z

on startup we check for the existence of C:\Users\Username\AppData\Roaming\Bitcoin\chainstate

I think we should check for existence of Roaming\Bitcoin rather than any subdirectory. This would not only catch non-mainnet datadirs, but also avoid compatibility issues with users who are looking at older instructions. For example, it's conceivable that a user following a guide that tells them to use Roaming would create Roaming\Bitcoin\bitcoin.conf before their first startup, and thus expect any settings in that conf file to be picked up. Currently, that would not be the case.

pinheadmz · 2023-10-03T12:16:44Z

I think we should check for existence of Roaming\Bitcoin rather than any subdirectory.

revised, rebased on master and pushed thanks.

I tested this "in production" on my windows laptop but found writing a functional test to be impossible. I thought I could model a test after @ryanofsky commit here: 8d777c0

...but for default datadir we use windows' SHGetSpecialFolderPathW with CSIDL_APPDATA which override environment variables: https://learn.microsoft.com/en-us/windows/win32/shell/csidl#remarks

DrahtBot · 2024-04-05T07:51:10Z

Could rebase for fresh CI, if still relevant?

achow101 · 2024-04-15T21:37:23Z

crACK c941712

Have not yet tested on a Windows machine.

pinheadmz · 2024-04-16T18:26:38Z

Could rebase for fresh CI, if still relevant?

rebased on master

hebasto

ACK c59b1e4, tested on Windows 11 Pro 23H2.

hebasto · 2024-04-29T14:42:20Z

doc/bitcoin-conf.md

@@ -59,7 +59,7 @@ The `includeconf=<file>` option in the `bitcoin.conf` file can be used to includ

 Operating System | Data Directory | Example Path
 -- | -- | --
-Windows | `%APPDATA%\Bitcoin\` | `C:\Users\username\AppData\Roaming\Bitcoin\bitcoin.conf`
+Windows | `%APPDATA%\Bitcoin\` | `C:\Users\username\AppData\Local\Bitcoin\bitcoin.conf`


Suggested change

Windows | `%APPDATA%\Bitcoin\` | `C:\Users\username\AppData\Local\Bitcoin\bitcoin.conf`

Windows | `%LOCALAPPDATA%\Bitcoin\` | `C:\Users\username\AppData\Local\Bitcoin\bitcoin.conf`

hebasto · 2024-04-29T14:42:46Z

doc/release-notes/release-notes-27064.md

+The default data directory on Windows has been moved from `C:\Users\Username\AppData\Roaming\Bitcoin`
+to `C:\Users\Username\AppData\Local\Bitcoin`. Bitcoin Core will check the existence
+of the old directory first and continue to use that directory for backwards
+compatability if it is present.


typo:

Suggested change

compatability if it is present.

compatibility if it is present.

hebasto · 2024-04-29T14:43:26Z

src/common/args.cpp

+    if (fs::exists(legacy_path))
+        return legacy_path;


nit: Could add curly braces or put in a single line?

pinheadmz · 2024-04-30T15:04:43Z

@hebasto thanks for the review, I addressed your comments

hebasto

re-ACK 84900ac, only addressed feedback since my recent review.

achow101 · 2024-05-01T20:09:09Z

ACK 84900ac

Tested on Windows. %APPDATA%/Bitcoin continues to be used if it exists, %LOCALAPPDATA% if not.

BenWestgate · 2024-05-22T22:20:57Z

crACK 84900ac

Have not tested on a Windows machine.

hebasto added the Windows label Feb 8, 2023

ghost approved these changes Feb 9, 2023

View reviewed changes

DrahtBot mentioned this pull request Apr 4, 2023

move-only: Extract common/args from util/system #27419

Merged

DrahtBot added the Needs rebase label Apr 21, 2023

achow101 requested review from jarolrod and hebasto April 25, 2023 16:19

pinheadmz force-pushed the windows-local-datadir branch from a2bfd52 to d0b715a Compare April 26, 2023 13:01

DrahtBot removed the Needs rebase label Apr 26, 2023

hebasto reviewed May 22, 2023

View reviewed changes

achow101 requested review from ryanofsky and achow101 September 20, 2023 16:51

pinheadmz force-pushed the windows-local-datadir branch from 60cd07b to c941712 Compare October 3, 2023 12:13

DrahtBot added CI failed and removed CI failed labels Nov 13, 2023

DrahtBot added the CI failed label Jan 13, 2024

scarletquasar approved these changes Mar 22, 2024

View reviewed changes

DrahtBot requested a review from hebasto April 15, 2024 21:37

achow101 removed their request for review April 15, 2024 21:37

pinheadmz force-pushed the windows-local-datadir branch from c941712 to c59b1e4 Compare April 16, 2024 18:26

DrahtBot removed the CI failed label Apr 16, 2024

hebasto approved these changes Apr 29, 2024

View reviewed changes

DrahtBot requested a review from achow101 April 29, 2024 14:43

pinheadmz added 2 commits April 30, 2024 11:03

system: use %LOCALAPPDATA% as default datadir on windows

855dd8d

doc: add release-notes-27064.md

84900ac

pinheadmz force-pushed the windows-local-datadir branch from c59b1e4 to 84900ac Compare April 30, 2024 15:04

hebasto approved these changes Apr 30, 2024

View reviewed changes

achow101 merged commit 915d727 into bitcoin:master May 23, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

system: use %LOCALAPPDATA% as default datadir on windows #27064

system: use %LOCALAPPDATA% as default datadir on windows #27064

pinheadmz commented Feb 8, 2023 •

edited

DrahtBot commented Feb 8, 2023 •

edited

hebasto commented Feb 8, 2023

ghost left a comment

pinheadmz commented Feb 16, 2023

pinheadmz commented Apr 26, 2023

hebasto commented May 22, 2023

hebasto May 22, 2023

pinheadmz May 24, 2023 •

edited

hebasto commented May 23, 2023

pinheadmz commented May 24, 2023

luke-jr commented Jun 22, 2023

pinheadmz commented Jun 22, 2023

achow101 commented Sep 26, 2023

pinheadmz commented Oct 3, 2023

DrahtBot commented Apr 5, 2024

achow101 commented Apr 15, 2024

pinheadmz commented Apr 16, 2024

hebasto left a comment

hebasto Apr 29, 2024

hebasto Apr 29, 2024

hebasto Apr 29, 2024

pinheadmz commented Apr 30, 2024

hebasto left a comment

achow101 commented May 1, 2024

BenWestgate commented May 22, 2024

	Windows \| `%APPDATA%\Bitcoin\` \| `C:\Users\username\AppData\Local\Bitcoin\bitcoin.conf`
	Windows \| `%LOCALAPPDATA%\Bitcoin\` \| `C:\Users\username\AppData\Local\Bitcoin\bitcoin.conf`

	compatability if it is present.
	compatibility if it is present.

system: use %LOCALAPPDATA% as default datadir on windows #27064

system: use %LOCALAPPDATA% as default datadir on windows #27064

Conversation

pinheadmz commented Feb 8, 2023 • edited

DrahtBot commented Feb 8, 2023 • edited

Code Coverage

Reviews

Conflicts

hebasto commented Feb 8, 2023

ghost left a comment

Choose a reason for hiding this comment

pinheadmz commented Feb 16, 2023

pinheadmz commented Apr 26, 2023

hebasto commented May 22, 2023

hebasto May 22, 2023

Choose a reason for hiding this comment

pinheadmz May 24, 2023 • edited

Choose a reason for hiding this comment

hebasto commented May 23, 2023

pinheadmz commented May 24, 2023

luke-jr commented Jun 22, 2023

pinheadmz commented Jun 22, 2023

achow101 commented Sep 26, 2023

pinheadmz commented Oct 3, 2023

DrahtBot commented Apr 5, 2024

achow101 commented Apr 15, 2024

pinheadmz commented Apr 16, 2024

hebasto left a comment

Choose a reason for hiding this comment

hebasto Apr 29, 2024

Choose a reason for hiding this comment

hebasto Apr 29, 2024

Choose a reason for hiding this comment

hebasto Apr 29, 2024

Choose a reason for hiding this comment

pinheadmz commented Apr 30, 2024

hebasto left a comment

Choose a reason for hiding this comment

achow101 commented May 1, 2024

BenWestgate commented May 22, 2024

pinheadmz commented Feb 8, 2023 •

edited

DrahtBot commented Feb 8, 2023 •

edited

pinheadmz May 24, 2023 •

edited