backport: partial bitcoin#34156 restore cleanup ownership

[PastaPastaPasta]

Issue being fixed or feature implemented

A filesystem exception while checking a restore backup can reach failure cleanup before the destination directory was created. An authorized restorewallet request can consequently delete an existing node-writable destination. Cleanup must only remove a directory owned by the current restore attempt.

What was done?

Partially adapts Bitcoin PR bitcoin#34156, specifically commit 4ed0693a3f2a427ef9e7ad016930ec29fa244995: track successful destination creation and gate cleanup on that ownership. Set an explicit load-failure status in the exception handler so RPC callers do not inspect an uninitialized status.

Dash still rejects existing destinations and always loads the restored wallet. The copied-file tracking, empty-directory assertion, and nonrecursive removal in upstream's commit are omitted: failed BDB verification can create .walletlock, database/, and db.log in a newly created directory, so Dash retains recursive cleanup exclusively for that owned directory. The remaining migration/restore-in-existing-directory changes in Bitcoin bitcoin#34156 are outside this partial backport. No upstream tests are claimed as backported; adapted functional coverage exercises Dash's restore behavior.

How Has This Been Tested?

On macOS arm64 with the existing depends-backed BDB/SQLite/Qt build:

• make -j1 passed.
• wallet_backup.py --legacy-wallet and wallet_backup.py --descriptors passed, including restored balances, missing-backup/existing-destination controls, and new relative/absolute temporary-directory preservation checks.
• The new preservation regression failed with the baseline binary because the nested sentinel was deleted after the early filesystem exception. The fixed code preserves it and returns a wallet error.
• A truncated, recognized BDB backup fails verification and the newly created restore directory is removed, retaining Dash's auxiliary-file cleanup.
• test_dash --run_test=walletdb_tests, whitespace lint, targeted repository Python lint, and git diff --check passed.

The filesystem exception regression uses an overlong path component in temporary test data. It does not operate on an existing real wallet; filesystem-error details can vary by platform.

Breaking Changes

None. Existing restore destinations remain rejected.

Checklist:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation (no documentation change required)
• I have assigned this pull request to a milestone

This pull request was created by Codex.

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 13 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: f356a2ea-265c-4d3b-9555-a4668f077ae6

📥 Commits

Reviewing files that changed from the base of the PR and between f35a24c and b3e27d9.

📒 Files selected for processing (2)

• src/wallet/wallet.cpp
• test/functional/wallet_backup.py

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

✅ Final review complete — no blockers (commit b3e27d9) · triage: low · Phase 2 only (queue backlog)

[thepastaclaw]

Final validation — Phase 2 only (queue backlog)

The restore cleanup guard is set only after successful destination creation, preserves pre-existing destinations on early filesystem exceptions, and supplies a defined failure status to RPC error handling. The omitted upstream restore and migration tests exercise behavior explicitly excluded from this partial backport, so the supplied suggestion is not actionable. Static verification found no in-scope defects; no builds or tests were run locally, and the supplied CI snapshot shows passing lint and several builds with additional builds and test jobs pending.

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: low by gpt-6.1-sol (effort low) — The diff is a small, contained restore-cleanup fix that gates directory deletion on successful creation and initializes failure status, with focused regression coverage.
• Phase 1 reviewers: not run (skipped for throughput: 18 PRs queued, above the 10 limit)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — backport-reviewer (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer

Out-of-scope follow-up suggestions (1)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Declared partial backport omits upstream restore and migration test transformations — INTENTIONAL_EXCLUSION: The PR description explicitly states that the remaining migration/restore-in-existing-directory changes in Bitcoin bitcoin#34156 are outside this partial backport and that no upstream tests are claimed as backported. Commit b3e27d9 independently records those exclusions. Inspection confirms that f011e0f tests existing-directory restoration, unnamed restoration, and unnamed pruned-restore failure, while 36093bd and b7c34d0 test excluded unnamed migration recovery paths. Dash retains existing-destination rejection and unconditional wallet loading at both base and head; the new sentinel-preservation and corrupt-BDB cases cover the selected ownership fix and retained recursive cleanup. The exclusions are already documented, so requesting their documentation again or tests for excluded behavior would expand the stated scope.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.