Merge bitcoin/bitcoin#34370: [29.x] Fix #34222 backport bugs

65173944ed60df3b9cffca95932aed8720921478 QA: tool_wallet: Check that db.log is deleted with a lone legacy wallet, but not with a shared db environment (Luke Dashjr)
69a6b9b1152ba0bb3edab6d2a54509fd416b24c8 Bugfix: Wallet/Migration: Move backup into wallet directory when migrating from non-directory (Luke Dashjr)
cef01d0be5223e9d33efc897d7fbe5d0a08692c0 Wallet/Migration: Skip moving the backup file back and forth for no reason (Luke Dashjr)
60f529027c6eacbdc298fab50192f8c60d7082a1 Wallet/Migration: If loading the new watchonly or solvables wallet fails, log the correct wallet name in error message (Luke Dashjr)
7475d134f6a3a6039ab6b9d39706ade47c764aa8 Wallet/bdb: Safely and correctly list files only used by the single wallet (Luke Dashjr)

Pull request description:

ACKs for top commit:
  achow101:
    ACK 65173944ed60df3b9cffca95932aed8720921478
  furszy:
    light ACK 65173944ed60df3b9cffca95932aed8720921478

Tree-SHA512: c10fe00dde512ca78cd6939a748b3875d0b40e9714997aedfd939a1dffdc7eaa2fd1779f3972a34b1c1d9a97d8f1ee1e082c970de15ac0e2ef5d9bbf3dc1d89a

.github/actions/configure-docker/action.yml

@@ -16,7 +16,7 @@ runs:
 
     # This is required to allow buildkit to access the actions cache
     - name: Expose actions cache variables
-      uses: actions/github-script@v6
+      uses: actions/github-script@v8
       with:
         script: |
           Object.keys(process.env).forEach(function (key) {

.github/workflows/ci.yml

@@ -49,7 +49,7 @@ jobs:
     steps:
       - name: Determine fetch depth
         run: echo "FETCH_DEPTH=$((${{ github.event.pull_request.commits }} + 2))" >> "$GITHUB_ENV"
-      - uses: actions/checkout@v4
+      - uses: actions/checkout@v6
         with:
           ref: ${{ github.event.pull_request.head.sha }}
           fetch-depth: ${{ env.FETCH_DEPTH }}
@@ -125,7 +125,7 @@ jobs:
     steps:
       - &CHECKOUT
         name: Checkout
-        uses: actions/checkout@v5
+        uses: actions/checkout@v6
         with:
           # Ensure the latest merged pull request state is used, even on re-runs.
           ref: &CHECKOUT_REF_TMPL ${{ github.event_name == 'pull_request' && github.ref || '' }}
@@ -164,7 +164,7 @@ jobs:
           FILE_ENV: ${{ matrix.file-env }}
 
       - name: Save Ccache cache
-        uses: actions/cache/save@v4
+        uses: actions/cache/save@v5
         if: github.event_name != 'pull_request' && steps.ccache-cache.outputs.cache-hit != 'true'
         with:
           path: ${{ env.CCACHE_DIR }}
@@ -198,11 +198,15 @@ jobs:
     steps:
       - *CHECKOUT
 
-      - name: Configure Developer Command Prompt for Microsoft Visual C++
-        # Using microsoft/setup-msbuild is not enough.
-        uses: ilammy/msvc-dev-cmd@v1
-        with:
-          arch: x64
+      - name: Set up VS Developer Prompt
+        shell: pwsh -Command "$PSVersionTable; $PSNativeCommandUseErrorActionPreference = $true; $ErrorActionPreference = 'Stop'; & '{0}'"
+        run: |
+          $vswherePath = "${env:ProgramFiles(x86)}\Microsoft Visual Studio\Installer\vswhere.exe"
+          $installationPath = & $vswherePath -latest -property installationPath
+          & "${env:COMSPEC}" /s /c "`"$installationPath\Common7\Tools\vsdevcmd.bat`" -arch=x64 -no_logo && set" | foreach-object {
+            $name, $value = $_ -split '=', 2
+            echo "$name=$value" >> $env:GITHUB_ENV
+          }
 
       - name: Get tool information
         run: |
@@ -222,13 +226,13 @@ jobs:
           sed -i '1s/^/set(ENV{CMAKE_POLICY_VERSION_MINIMUM} 3.5)\n/' "${VCPKG_INSTALLATION_ROOT}/scripts/ports.cmake"
 
       - name: vcpkg tools cache
-        uses: actions/cache@v4
+        uses: actions/cache@v5
         with:
           path: C:/vcpkg/downloads/tools
           key: ${{ github.job }}-vcpkg-tools
 
       - name: Restore vcpkg binary cache
-        uses: actions/cache/restore@v4
+        uses: actions/cache/restore@v5
         id: vcpkg-binary-cache
         with:
           path: ~/AppData/Local/vcpkg/archives
@@ -239,7 +243,7 @@ jobs:
           cmake -B build --preset vs2022-static -DCMAKE_TOOLCHAIN_FILE="$env:VCPKG_INSTALLATION_ROOT\scripts\buildsystems\vcpkg.cmake" ${{ matrix.generate-options }}
 
       - name: Save vcpkg binary cache
-        uses: actions/cache/save@v4
+        uses: actions/cache/save@v5
         if: github.event_name != 'pull_request' && steps.vcpkg-binary-cache.outputs.cache-hit != 'true' && matrix.job-type == 'standard'
         with:
           path: ~/AppData/Local/vcpkg/archives
@@ -414,7 +418,7 @@ jobs:
       CONTAINER_NAME: "bitcoin-linter"
     steps:
       - name: Checkout
-        uses: actions/checkout@v5
+        uses: actions/checkout@v6
         with:
           ref: *CHECKOUT_REF_TMPL
           fetch-depth: 0

doc/release-notes.md

@@ -73,8 +73,10 @@ Notable changes
 
 ### Misc
 
+- #32513 ci: remove 3rd party js from windows dll gha job
 - #33508 ci: fix buildx gha cache authentication on forks
 - #33581 ci: Properly include $FILE_ENV in DEPENDS_HASH
+- #34344 ci: update GitHub Actions versions
 
 Credits
 =======
@@ -90,6 +92,8 @@ Thanks to everyone who directly contributed to this release:
 - furszy
 - Hennadii Stepanov
 - ismaelsadeeq
+- m3dwards
+- Padraic Slattery
 - Pieter Wuille
 - SatsAndSports
 - willcl-ark

src/wallet/bdb.cpp

@@ -16,6 +16,7 @@
 #include <util/strencodings.h>
 #include <util/translation.h>
 
+#include <set>
 #include <stdint.h>
 
 #include <db_cxx.h>
@@ -340,6 +341,53 @@ bool BerkeleyDatabase::Verify(bilingual_str& errorStr)
     return true;
 }
 
+std::vector<fs::path> BerkeleyDatabase::Files()
+{
+    std::vector<fs::path> files;
+    // If the wallet is the *only* file, clean up the entire BDB environment
+    constexpr auto build_files_list = [](std::vector<fs::path>& files, const std::shared_ptr<BerkeleyEnvironment>& env, const fs::path& filename) {
+        if (env->m_databases.size() != 1) return false;
+
+        const auto env_dir = env->Directory();
+        const auto db_subdir = env_dir / "database";
+        if (fs::exists(db_subdir)) {
+            if (!fs::is_directory(db_subdir)) return false;
+            for (const auto& entry : fs::directory_iterator(db_subdir)) {
+                const auto& path = entry.path().filename();
+                if (!fs::PathToString(path).starts_with("log.")) {
+                    return false;
+                }
+                files.emplace_back(entry.path());
+            }
+        }
+        const std::set<fs::path> allowed_paths = {
+            filename,
+            "db.log",
+            ".walletlock",
+            "database"
+        };
+        for (const auto& entry : fs::directory_iterator(env_dir)) {
+            const auto& path = entry.path().filename();
+            if (allowed_paths.contains(path)) {
+                files.emplace_back(entry.path());
+            } else if (fs::is_directory(entry.path())) {
+                // Subdirectories can't possibly be using this db env, and is expected if this is a non-directory wallet
+                // Do not include them in Files, but still allow the env cleanup
+            } else {
+                return false;
+            }
+        }
+        return true;
+    };
+    try {
+        if (build_files_list(files, env, m_filename)) return files;
+    } catch (...) {
+        // Give up building the comprehensive file list if any error occurs
+    }
+    // Otherwise, it's only really safe to delete the one wallet file
+    return {env->Directory() / m_filename};
+}
+
 void BerkeleyEnvironment::CheckpointLSN(const std::string& strFile)
 {
     dbenv->txn_checkpoint(0, 0, 0);

src/wallet/bdb.h

@@ -132,20 +132,7 @@ public:
     /** Return path to main database filename */
     std::string Filename() override { return fs::PathToString(env->Directory() / m_filename); }
 
-    std::vector<fs::path> Files() override
-    {
-        std::vector<fs::path> files;
-        files.emplace_back(env->Directory() / m_filename);
-        if (env->m_databases.size() == 1) {
-            files.emplace_back(env->Directory() / "db.log");
-            files.emplace_back(env->Directory() / ".walletlock");
-            files.emplace_back(env->Directory() / "database" / "log.0000000001");
-            files.emplace_back(env->Directory() / "database");
-            // Note that this list is not exhaustive as BDB may create more log files, and possibly other ones too
-            // However it should be good enough for the only calls to Files()
-        }
-        return files;
-    }
+    std::vector<fs::path> Files() override;
 
     std::string Format() override { return "bdb"; }
     /**

src/wallet/wallet.cpp

@@ -4532,7 +4532,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
         to_reload = LoadWallet(context, name, /*load_on_start=*/std::nullopt, options, status, error, warnings);
         if (!to_reload) {
             LogError("Failed to load wallet '%s' after migration. Rolling back migration to preserve consistency. "
-                     "Error cause: %s\n", wallet_name, error.original);
+                     "Error cause: %s\n", name, error.original);
             return false;
         }
         return true;
@@ -4581,6 +4581,12 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
         // First change to using SQLite
         if (!local_wallet->MigrateToSQLite(error)) return util::Error{error};
 
+        // In case we're migrating from file to directory, move the backup into it
+        this_wallet_dir = fs::absolute(fs::PathFromString(local_wallet->GetDatabase().Filename())).parent_path();
+        backup_path = this_wallet_dir / backup_filename;
+        fs::rename(res.backup_path, backup_path);
+        res.backup_path = backup_path;
+
         // Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails
         success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);
         if (!success) {
@@ -4634,9 +4640,6 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
     }
     if (!success) {
         // Migration failed, cleanup
-        // Before deleting the wallet's directory, copy the backup file to the top-level wallets dir
-        fs::path temp_backup_location = fsbridge::AbsPathJoin(GetWalletDir(), backup_filename);
-        fs::rename(backup_path, temp_backup_location);
 
         // Make list of wallets to cleanup
         std::vector<std::shared_ptr<CWallet>> created_wallets;
@@ -4680,15 +4683,12 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
         // Convert the backup file to the wallet db file by renaming it and moving it into the wallet's directory.
         // Reload it into memory if the wallet was previously loaded.
         bilingual_str restore_error;
-        const auto& ptr_wallet = RestoreWallet(context, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings, /*load_after_restore=*/was_loaded);
+        const auto& ptr_wallet = RestoreWallet(context, backup_path, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings, /*load_after_restore=*/was_loaded);
         if (!restore_error.empty()) {
             error += restore_error + _("\nUnable to restore backup of wallet.");
             return util::Error{error};
         }
 
-        // The wallet directory has been restored, but just in case, copy the previously created backup to the wallet dir
-        fs::rename(temp_backup_location, backup_path);
-
         // Verify that there is no dangling wallet: when the wallet wasn't loaded before, expect null.
         // This check is performed after restoration to avoid an early error before saving the backup.
         bool wallet_reloaded = ptr_wallet != nullptr;

test/functional/tool_wallet.py

@@ -410,17 +410,30 @@ class ToolWalletTest(BitcoinTestFramework):
         self.assert_raises_tool_error('Error: Checksum is not the correct size', '-wallet=badload', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump')
         assert not (self.nodes[0].wallets_path / "badload").is_dir()
         if not self.options.descriptors:
-            os.rename(self.nodes[0].wallets_path / "wallet.dat", self.nodes[0].wallets_path / "default.wallet.dat")
+            os.rename(self.nodes[0].wallets_path / "wallet.dat", self.nodes[0].wallets_path / "../default.wallet.dat")
+            (self.nodes[0].wallets_path / "db.log").unlink(missing_ok=True)
         self.assert_raises_tool_error('Error: Checksum is not the correct size', '-wallet=', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump')
         assert self.nodes[0].wallets_path.exists()
         assert not (self.nodes[0].wallets_path / "wallet.dat").exists()
+        if not self.options.descriptors:
+            assert not (self.nodes[0].wallets_path / "db.log").exists()
 
         self.log.info('Checking createfromdump with an unnamed wallet')
         self.do_tool_createfromdump("", "wallet.dump")
         assert (self.nodes[0].wallets_path / "wallet.dat").exists()
         os.unlink(self.nodes[0].wallets_path / "wallet.dat")
         if not self.options.descriptors:
-            os.rename(self.nodes[0].wallets_path / "default.wallet.dat", self.nodes[0].wallets_path / "wallet.dat")
+            os.rename(self.nodes[0].wallets_path / "../default.wallet.dat", self.nodes[0].wallets_path / "wallet.dat")
+
+            self.log.info('Checking createfromdump with multiple non-directory wallets')
+            assert not (self.nodes[0].wallets_path / "wallet.dat").is_dir()
+            assert (self.nodes[0].wallets_path / "db.log").exists()
+            os.rename(self.nodes[0].wallets_path / "wallet.dat", self.nodes[0].wallets_path / "test.dat")
+            self.assert_raises_tool_error('Error: Checksum is not the correct size', '-wallet=', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump')
+            assert not (self.nodes[0].wallets_path / "wallet.dat").exists()
+            assert (self.nodes[0].wallets_path / "test.dat").exists()
+            assert (self.nodes[0].wallets_path / "db.log").exists()
+            os.rename(self.nodes[0].wallets_path / "test.dat", self.nodes[0].wallets_path / "wallet.dat")
 
     def test_chainless_conflicts(self):
         self.log.info("Test wallet tool when wallet contains conflicting transactions")