system: use %LOCALAPPDATA% as default datadir on windows

pinheadmz commented at 8:44 pm on February 8, 2023: member

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 commented at 8:44 pm on February 8, 2023: contributor

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 commented at 9:17 pm on February 8, 2023: member

cc @sipsorcery

hebasto added the label Windows on Feb 8, 2023

unknown approved

unknown commented at 11:27 pm on February 9, 2023: none

utACK https://github.com/bitcoin/bitcoin/pull/27064/commits/a2bfd52cf5585677e50cd35a277e8fe2ee0f261f

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 commented at 4:08 pm on February 16, 2023: member

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

DrahtBot added the label Needs rebase on Apr 21, 2023

achow101 requested review from jarolrod on Apr 25, 2023

achow101 requested review from hebasto on Apr 25, 2023

pinheadmz force-pushed on Apr 26, 2023

pinheadmz commented at 1:07 pm on April 26, 2023: member

Push to 60cd07b781bfe46262a5d66c97e257e0dd378f5c:

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

DrahtBot removed the label Needs rebase on Apr 26, 2023

hebasto commented at 3:56 pm on May 22, 2023: member

Concept ACK.

in src/common/args.cpp:695 in 60cd07b781 outdated

692 #ifdef WIN32
693     // Windows
694-    return GetSpecialFolderPath(CSIDL_APPDATA) / "Bitcoin";
695+    // Check for existence of datadir in old location and keep it there
696+    fs::path legacy_path = GetSpecialFolderPath(CSIDL_APPDATA) / "Bitcoin";
697+    if (fs::exists(legacy_path / "chainstate"))

hebasto commented at 4:00 pm on May 22, 2023:

Update the PR description:

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

?

pinheadmz commented at 7:25 pm on May 24, 2023:

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 commented at 10:48 am on May 23, 2023: member

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

pinheadmz commented at 7:43 pm on May 24, 2023: member

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 commented at 8:08 pm on June 22, 2023: member

Why would we prefer local over roaming explicitly?

pinheadmz commented at 8:16 pm on June 22, 2023: member

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 requested review from ryanofsky on Sep 20, 2023

achow101 requested review from achow101 on Sep 20, 2023

achow101 commented at 3:56 pm on September 26, 2023: member

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 force-pushed on Oct 3, 2023

pinheadmz commented at 12:16 pm on October 3, 2023: member

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: https://github.com/bitcoin/bitcoin/pull/27409/commits/8d777c067058fa600be0195231ec86e07a398fc9

…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 added the label CI failed on Nov 13, 2023

DrahtBot removed the label CI failed on Nov 13, 2023

DrahtBot added the label CI failed on Jan 13, 2024

somecodingwitch approved

DrahtBot commented at 7:51 am on April 5, 2024: contributor

Could rebase for fresh CI, if still relevant?

achow101 commented at 9:37 pm on April 15, 2024: member

crACK c941712c269d34e6496db35424e567c7f6ba34e8

Have not yet tested on a Windows machine.

DrahtBot requested review from hebasto on Apr 15, 2024

achow101 removed review request from achow101 on Apr 15, 2024

pinheadmz force-pushed on Apr 16, 2024

pinheadmz commented at 6:26 pm on April 16, 2024: member

Could rebase for fresh CI, if still relevant?

rebased on master

DrahtBot removed the label CI failed on Apr 16, 2024

in doc/bitcoin-conf.md:62 in c59b1e45db outdated

58@@ -59,7 +59,7 @@ The `includeconf=<file>` option in the `bitcoin.conf` file can be used to includ
59 
60 Operating System | Data Directory | Example Path
61 -- | -- | --
62-Windows | `%APPDATA%\Bitcoin\` | `C:\Users\username\AppData\Roaming\Bitcoin\bitcoin.conf`
63+Windows | `%APPDATA%\Bitcoin\` | `C:\Users\username\AppData\Local\Bitcoin\bitcoin.conf`

hebasto commented at 2:42 pm on April 29, 2024:

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

in doc/release-notes/release-notes-27064.md:7 in c59b1e45db outdated

0@@ -0,0 +1,7 @@
1+Files
2+-----
3+
4+The default data directory on Windows has been moved from `C:\Users\Username\AppData\Roaming\Bitcoin`
5+to `C:\Users\Username\AppData\Local\Bitcoin`. Bitcoin Core will check the existence
6+of the old directory first and continue to use that directory for backwards
7+compatability if it is present.

hebasto commented at 2:42 pm on April 29, 2024:

typo:

0compatibility if it is present.

in src/common/args.cpp:709 in c59b1e45db outdated

706     // Windows
707-    return GetSpecialFolderPath(CSIDL_APPDATA) / "Bitcoin";
708+    // Check for existence of datadir in old location and keep it there
709+    fs::path legacy_path = GetSpecialFolderPath(CSIDL_APPDATA) / "Bitcoin";
710+    if (fs::exists(legacy_path))
711+        return legacy_path;

hebasto commented at 2:43 pm on April 29, 2024:

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

hebasto approved

hebasto commented at 2:43 pm on April 29, 2024: member

ACK c59b1e45db7b62ba0e0c833e6517045d3dfeacd0, tested on Windows 11 Pro 23H2.

DrahtBot requested review from achow101 on Apr 29, 2024

system: use %LOCALAPPDATA% as default datadir on windows 855dd8d592

doc: add release-notes-27064.md 84900ac34f

pinheadmz force-pushed on Apr 30, 2024

pinheadmz commented at 3:04 pm on April 30, 2024: member

@hebasto thanks for the review, I addressed your comments

hebasto approved

hebasto commented at 3:16 pm on April 30, 2024: member

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

achow101 commented at 8:09 pm on May 1, 2024: member

ACK 84900ac34f6888b7a851d0a6a5885192155f865c

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

BenWestgate commented at 10:20 pm on May 22, 2024: contributor

crACK https://github.com/bitcoin/bitcoin/commit/84900ac34f6888b7a851d0a6a5885192155f865c

Have not tested on a Windows machine.

achow101 merged this on May 23, 2024

achow101 closed this on May 23, 2024

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

Code Coverage

Reviews

Conflicts