Skip to content

test: prune past Dash's first fastprune block file in wallet_migration - #13

Open
PastaPastaPasta wants to merge 4 commits into
knst:bp-wallet-descriptorsfrom
PastaPastaPasta:fix/8014-pruned-migration-test
Open

test: prune past Dash's first fastprune block file in wallet_migration#13
PastaPastaPasta wants to merge 4 commits into
knst:bp-wallet-descriptorsfrom
PastaPastaPasta:fix/8014-pruned-migration-test

Conversation

@PastaPastaPasta

@PastaPastaPasta PastaPastaPasta commented Nov 2, 2026

Copy link
Copy Markdown

Fix for the failing wallet_migration.py in dashpay#8014. This is a separate PR into bp-wallet-descriptors so nothing is pushed to your branch; merge, cherry-pick or squash it as you prefer.

Problem

unsynced_wallet_on_pruned_node_fails fails at 7db2696c on linux64, tsan and multiprocess (all 3 attempts) with No exception raised at the "Block not available (pruned data)" assert. CI's debug log has Prune (Manual): prune_height=250 removed 0 blk/rev pairs.

The test comes from Bitcoin 28.x (mine 450 blocks, prune to 250), but Dash's early regtest blocks are much smaller. In a local run the -fastprune block files hold:

file heights avg block size
blk00000 0..319 204 B
blk00001 320..475 416 B
blk00002 476..543 935 B
blk00003 544..569 1260 B

Manual pruning removes only whole files whose last height is at or below min(250, tip - 288) = 250. The wallet's next block (~120) sits in blk00000, which ends at 319, so no file is ever eligible.

Fix

Mine 600 blocks and prune to height 400. blk00000 (ends at ~320) is removed with about 80 blocks of margin; blk00001 stays. Test-only; no wallet code changes.

Testing

  • Fails at 7db2696c locally with the same CI symptom; passes with this commit.
  • wallet_migration.py 6/6 in parallel, plus wallet_backup.py and tool_wallet.py (--legacy-wallet and --descriptors), on macOS with a RAM-disk datadir.

🤖 Generated with Claude Code

knst and others added 4 commits November 2, 2026 20:02
@knst @fanquake
partial Merge bitcoin#34156: wallet: fix unnamed legacy wallet migrat…
e7bbeb4 
…ion failure

BACKPORT NOTE:
Code taken from the 28.x adaptation (bitcoin#34223: e1e9d71, e47af69,
1d46624, 75b59e5, fb4406e, 86eaf71, 77622e0) because master's
commits target the BERKELEY_RO-only migration path. Dropped test_pruned_wallet_backup
(needs bitcoin#30678). The backup filename keeps Dash's basename reduction for
path-style wallet names (see the TODO for bitcoin#32273) and only adopts
MigrationPrefixName for the unnamed wallet.

b7c34d0 test: coverage for migration failure when last sync is beyond prune height (furszy)
82caa81 wallet: migration, fix watch-only and solvables wallets names (furszy)
d70b159 wallet: improve post-migration logging (furszy)
f011e0f test: restorewallet, coverage for existing dirs, unnamed wallet and prune failure (furszy)
36093bd test: add coverage for unnamed wallet migration failure (furszy)
f4c7e28 wallet: fix unnamed wallet migration failure (furszy)
4ed0693 wallet: RestoreWallet failure, erase only what was created (furszy)

Pull request description:

  Minimal fix for bitcoin#34128.

  The issue occurs during the migration of a legacy unnamed wallet
  (the legacy "default" wallet). When the migration fails, the cleanup
  logic is triggered to roll back the state, which involves erasing the
  newly created descriptor wallets directories. Normally, this only
  affects the parent directories of named wallets, since they each
  reside in their own directories. However, because the unnamed
  wallet resides directly in the top-level `/wallets/` folder, this
  logic accidentally deletes the main directory.

  The fix ensures that only the wallet.dat file of the unnamed wallet
  is touched and restored, preserving the wallet in BDB format and
  leaving the main `/wallets/` directory intact.

  #### Story Line:
  bitcoin#32273 fixed a different set of issues and, in doing so, uncovered
  this one.
  Before the mentioned PR, backups were stored in the same directory
  as the wallet.dat file. On a migration failure, the backup was then
  copied to the top-level `/wallets/` directory. For the unnamed legacy
  wallet, the wallet directory is the `/wallets/` directory, so the source
  and destination paths were identical. As a result, we threw early in the
  `fs::copy_file` call ([here](https://github.com/bitcoin/bitcoin/blob/29.x/src/wallet/wallet.cpp#L4572)) because the file already existed, as we
  were trying to copy the file onto itself. This caused the cleanup logic
  to abort early on and never reach the removal line.

  #### Testing Notes:
  Cherry-pick the test commit on top of master and run it. You will
  see the failure and realize the reason by reading the test code.

ACKs for top commit:
  achow101:
    ACK b7c34d0
  davidgumberg:
    crACK bitcoin@b7c34d0
  w0xlt:
    ACK bitcoin@b7c34d0
  willcl-ark:
    ACK b7c34d0

Tree-SHA512: d0be14c0ed6417f999c3f2f429652c2407097d0cc18453c91653e57ae4b5375b327ad3b2553d9ea6ff46a3ae00cdbd5ab325b94eba763072c4fc5a773b85618b

Co-authored-by: merge-script <fanquake@gmail.com>
@knst @fanquake
Merge bitcoin#34226: wallet: test: Relative wallet failed migration c…
d1d602b 
…leanup

BACKPORT NOTE:
Single-node test layout as on 28.x (4d21972). The expected backup path uses the
last component of the resolved wallet path because Dash keeps the basename rule
for path-style wallet names (see the TODO for bitcoin#32273 in wallet.cpp).

eeaf28d wallet: test: Failed migration cleanup (David Gumberg)

Pull request description:

  Prior to bitcoin#34156, an issue existed where if migration of a wallet with a relative pathname failed, the relatively specified path where the legacy wallet is would be deleted. This issue predates bitcoin#32273, because the relative pathnames get stacked together, e.g. "../../", the copy conflict bug that caused migration to abort early instead of getting far enough to attempt clean-up that was fixed in bitcoin#32273 is avoided.

  This is a functional test demonstrating that we handle failed migration clean-up correctly for relatively-named wallets. To see the issue, you can backport this test onto 29.x: https://github.com/davidgumberg/bitcoin/tree/2026-01-07-rel-migration-test-backport

  I've also added an absolute path failed migration cleanup test. WRT this and bitcoin#34156, absolute paths exhibit similar behavior to unnamed wallets. Because of the name-conflict bug prior to bitcoin#32273 an absolute-path migration would fail no matter what because migration would attempt to copy a file to a destination that already exists. But after bitcoin#32273, absolute-path migration gets past there, and if it fails for some other reason, the same behavior that's fixed in bitcoin#34156 occurs where the directory containing the wallet file is deleted.

ACKs for top commit:
  achow101:
    ACK eeaf28d
  furszy:
    ACK eeaf28d
  rkrux:
    lgtm ACK eeaf28d

Tree-SHA512: ee366fe526d0328654a86c2e9e6f228ca81554c8f8a78c259fa7aab90f024f9e5694ecf3f1d188938355f4e6d351c5a6a8ad236701bdd0ce63005e5d42c15e15

Co-authored-by: merge-script <fanquake@gmail.com>
@knst @sedited
Merge bitcoin#34370: [29.x] Fix bitcoin#34222 backport bugs
7db2696 
BACKPORT NOTE:
These five commits have no master counterpart (they fix the BDB-era backport
bitcoin#34222 / bitcoin#34223). The 28.x copies in bitcoin#34620 (29abedc,
619480b, 6494072, c5d9f75, 242b9ae) were used as the code reference.
Dash has no load_after_restore (bitcoin#31451) and no blank-wallet branch
(bitcoin#28976); see the TODOs in MigrateLegacyToDescriptor.

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

Pull request description:

ACKs for top commit:
  achow101:
    ACK 6517394
  furszy:
    light ACK 6517394

Tree-SHA512: c10fe00dde512ca78cd6939a748b3875d0b40e9714997aedfd939a1dffdc7eaa2fd1779f3972a34b1c1d9a97d8f1ee1e082c970de15ac0e2ef5d9bbf3dc1d89a

Co-authored-by: merge-script <seb.kung@gmail.com>
@PastaPastaPasta @claude
test: prune past Dash's first fastprune block file in wallet_migration
f7ba6da 
unsynced_wallet_on_pruned_node_fails was taken from Bitcoin 28.x, which
mines 450 blocks and prunes to height 250. Dash's early regtest blocks are
small (~200 B), so the first 64 KiB -fastprune block file holds heights
0..~320. Manual pruning only removes whole files whose last block is at
or below min(250, tip - 288), so nothing was pruned, the wallet's next
block stayed available, and the test failed with "No exception raised"
(CI: "Prune (Manual): prune_height=250 removed 0 blk/rev pairs").

Mine 600 blocks and prune to height 400 so the first block file, which
holds the wallet's next block, is removed while the second one stays.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@PastaPastaPasta
PastaPastaPasta requested a review from knst as a code owner November 2, 2026 23:07
@knst
knst force-pushed the bp-wallet-descriptors branch from 7db2696 to 1574eb0 Compare November 3, 2026 08:46
knst pushed a commit that referenced this pull request Nov 4, 2026
@PastaPastaPasta
Merge dashpay#8040: feat: remove DEBUG_LOCKORDER and DEBUG_LOCKCONTEN…
041af1d 
…TION from slowest CI jobs: asan, tsan, multiprocess

a1f4cdd Merge bitcoin#30122: bench: enable wallet creation benchmarks on all platforms (Konstantin Akimov)
c90f040 fix: make nCachedBlockHeight atomic (Konstantin Akimov)
1d134cb ci: drop DEBUG_LOCKCONTENTION from the tsan job (Konstantin Akimov)
38cef44 ci: enable DEBUG_LOCKORDER and DEBUG_LOCKCONTENTION in the linux64 job (Konstantin Akimov)
8ba67c5 ci: drop DEBUG_LOCKCONTENTION from the multiprocess job (Konstantin Akimov)
cf3a404 ci: drop DEBUG_LOCKORDER from the multiprocess job (Konstantin Akimov)
e5f0e11 ci: drop DEBUG_LOCKORDER from the asan job (Konstantin Akimov)
c599fca ci: drop DEBUG_LOCKORDER from the tsan job (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  `DEBUG_LOCKORDER` is adding noticeable over-head and already slow job TSAN is loosing up to 45% CPU time purely on code related to lock-order feature; same for multiprocesses - it is built with -O0 and --enable-debug and overhead is huge.

  `DEBUG_LOCKCONTENTION` is giving less overhead, but it's bunch of logs + string formatting and with -O0 and with tsan it's visible [but smaller] overhead.

  Removing this options helped to discovered 2 new issues, one by asan, other by tsan that had been hidden by slow performance and extensive logging.

  Tsan issue:

      WARNING: ThreadSanitizer: data race
      Write of size 4 by thread T2 'd-scheduler':
        #0 CGovernanceManager::UpdatedBlockTip        src/governance/governance.cpp:1128   nCachedBlockHeight = pindex->nHeight;
        #1 CDSNotificationInterface::UpdatedBlockTip  src/dsnotificationinterface.cpp:70
        #4 CMainSignals::UpdatedBlockTip              src/validationinterface.cpp:199      (validation-interface callback, scheduler thread)

      Previous read of size 4 by thread T4 (an HTTP worker, holding 5 mutexes):
        #0 CGovernanceManager::CheckAndRemove                 src/governance/governance.cpp:424    m_superblocks.Clean(nCachedBlockHeight);
        #1 CDSNotificationInterface::NotifyMasternodeListChanged  src/dsnotificationinterface.cpp:94
        #4 CMainSignals::NotifyMasternodeListChanged           src/validationinterface.cpp:329     (called synchronously from an RPC)
        dashpay#23 ExecuteCommand                                     src/rpc/server.cpp:534
        dashpay#34 HTTPWorkQueueRun                                   src/httpserver.cpp:443

      Location is heap block of size 608 allocated by main thread:
        #1 std::make_unique<CGovernanceManager>  src/init.cpp:2159

  bench's leak:

      ==59354==ERROR: LeakSanitizer: detected memory leaks

      Indirect leak of 488417 byte(s) in 113 object(s) allocated from:
          #0 0x556ed06abde3 in malloc (/__w/dash/dash/build-ci/dashcore-linux64_asan/src/bench/bench_dash+0x16cfde3) (BuildId: b06bbed85096d73ca18d4ff5a611a620133f3ab7)
          #1 0x556ed2cce9c7 in sqlite3MemMalloc sqlite3.c

      Indirect leak of 312000 byte(s) in 3000 object(s) allocated from:
          #0 0x556ed06ed441 in operator new(unsigned long) (/__w/dash/dash/build-ci/dashcore-linux64_asan/src/bench/bench_dash+0x1711441) (BuildId: b06bbed85096d73ca18d4ff5a611a620133f3ab7)
          #1 0x556ed22273f7 in allocate /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/new_allocator.h:151:27
          #2 0x556ed22273f7 in allocate /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/allocator.h:196:32
          #3 0x556ed22273f7 in allocate /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/alloc_traits.h:478:20
          #4 0x556ed22273f7 in _M_get_node /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_tree.h:559:16
          #5 0x556ed22273f7 in _M_create_node<const std::piecewise_construct_t &, std::tuple<const CPubKey &>, std::tuple<> > /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_tree.h:609:23
          #6 0x556ed22273f7 in _Auto_node<const std::piecewise_construct_t &, std::tuple<const CPubKey &>, std::tuple<> > /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_tree.h:1633:18
          #7 0x556ed22273f7 in std::_Rb_tree_iterator<std::pair<CPubKey const, int>> std::_Rb_tree<CPubKey, std::pair<CPubKey const, int>, std::_Select1st<std::pair<CPubKey const, int>>, std::less<CPubKey>, std::allocator<std::pair<CPubKey const, int>>>::_M_emplace_hint_unique<std::piecewise_construct_t const&, std::tuple<CPubKey const&>, std::tuple<>>(std::_Rb_tree_const_iterator<std::pair<CPubKey const, int>>, std::piecewise_construct_t const&, std::tuple<CPubKey const&>&&, std::tuple<>&&) /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_tree.h:2458:13
          #8 0x556ed21e1458 in std::map<CPubKey, int, std::less<CPubKey>, std::allocator<std::pair<CPubKey const, int>>>::operator[](CPubKey const&) /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/stl_map.h:513:15
          #9 0x556ed21bd668 in wallet::DescriptorScriptPubKeyMan::TopUp(unsigned int) src/wallet/scriptpubkeyman.cpp:2393:13
          #10 0x556ed21c2ada in wallet::DescriptorScriptPubKeyMan::SetupDescriptorGeneration(CExtKey const&, std::__cxx11::basic_string<char, std::char_traits<char>, secure_allocator<char>> const&, std::__cxx11::basic_string<char, std::char_traits<char>, secure_allocator<char>> const&, wallet::PathDerivationType) src/wallet/scriptpubkeyman.cpp:2560:5
          #11 0x556ed22dcfdd in wallet::CWallet::SetupDescriptorScriptPubKeyMans(CExtKey const&, std::__cxx11::basic_string<char, std::char_traits<char>, secure_allocator<char>> const&, std::__cxx11::basic_string<char, std::char_traits<char>, secure_allocator<char>>) src/wallet/wallet.cpp:4455:26
          #12 0x556ed22895a2 in wallet::CWallet::SetupDescriptorScriptPubKeyMans(std::__cxx11::basic_string<char, std::char_traits<char>, secure_allocator<char>> const&, std::__cxx11::basic_string<char, std::char_traits<char>, secure_allocator<char>>) src/wallet/wallet.cpp:4497:9
          #13 0x556ed22726a0 in wallet::CreateWallet(wallet::WalletContext&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, std::optional<bool>, wallet::DatabaseOptions&, wallet::DatabaseStatus&, bilingual_str&, std::vector<bilingual_str, std::allocator<bilingual_str>>&) src/wallet/wallet.cpp:470:25
          dashpay#14 0x556ed08b479b in operator() src/bench/wallet_create.cpp:42:23
          dashpay#15 0x556ed08b479b in ankerl::nanobench::Bench& ankerl::nanobench::Bench::run<wallet::WalletCreate(ankerl::nanobench::Bench&, bool)::$_0>(wallet::WalletCreate(ankerl::nanobench::Bench&, bool)::$_0&&) src/./bench/nanobench.h:1221:13
          dashpay#16 0x556ed08b3d91 in wallet::WalletCreate(ankerl::nanobench::Bench&, bool) src/bench/wallet_create.cpp:41:11
          dashpay#17 0x556ed06fd086 in operator() /usr/lib/gcc/x86_64-linux-gnu/14/../../../../include/c++/14/bits/std_function.h:591:9
          dashpay#18 0x556ed06fd086 in benchmark::BenchRunner::RunAll(benchmark::Args const&) src/bench/bench.cpp:120:13
          dashpay#19 0x556ed075f6f8 in main src/bench/bench_bitcoin.cpp:137:9
          dashpay#20 0x7f8887a221c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: a4a7992a8e66555c8141ab2a08a8465ff6e0ea65)
          dashpay#21 0x7f8887a2228a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: a4a7992a8e66555c8141ab2a08a8465ff6e0ea65)
          dashpay#22 0x556ed060bfb4 in _start (/__w/dash/dash/build-ci/dashcore-linux64_asan/src/bench/bench_dash+0x162ffb4) (BuildId: b06bbed85096d73ca18d4ff5a611a620133f3ab7)

  ## What was done?
  Removed `DEBUG_LOCKORDER` from slowest CI jobs: asan, tsan, multiprocess, and it is added instead to `linux64` job, that has qt, that has previous releases, that have wallets, no sanitizers but have optimization flags.

  Extra fixes to make CI jobs succeed:
   - backport: bitcoin#30122 to fix asan issue
   - make nCachedBlockHeight atomic to fix tsan issue

  ## How Has This Been Tested?

  CI:
  |CI job| develop | PR|
  |---|---|---|
  |tsan unit|8m1|6m30 (-19%)|
  |tsan functional|54m16| 33m23 (-40%)|
  |tsan functional accumulated | 11830s | 7576s (-36%)|

  Tested locally on:
   - feature_llmq_chainlocks.py
   - feature_llmq_dkgerrors.py
   - feature_llmq_is_retroactive.py
   - feature_llmq_signing.py
   - rpc_verifyislock.py

    | CI job | current flags | without DEBUG_LOCKORDER | without both |
    |---|---|---|---|
    | linux64_tsan | 441 | 238 (-46%) | 238 |
    | linux64_asan | 332 | 229 (-31%) | 329* |
    | linux64_multiprocess (-O0) | 296 | 196 (-34%) | 189 (-36%) |
    | linux64* | 108 (+18%) | -- | 91 |
   - *ASAN no LOCKCONTENTION
  - *linux64 gained both DEBUG_LOCKORDER and LOCKCONTENTION

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(if you have permission to set one)_

Top commit has no ACKs.

Tree-SHA512: c7585983d911da722ae6b37199fd5c96135d8d8eaab10935c55d2acb38f0995712f738d1f28adf4f2e010471918e66dae1c7696512fa4dd398061a7c82d1d1fb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@knst knst Awaiting requested review from knst knst is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@PastaPastaPasta @knst