Skip to content

fuzz: Backport the spkm_migration target (bitcoin/bitcoin#32624) - #445

Open
chrisguida wants to merge 2 commits into
bitcoinknots:29.x-knotsfrom
chrisguida:fuzz_spkm_migration-29
Open

chrisguida wants to merge 2 commits into
bitcoinknots:29.x-knotsfrom
chrisguida:fuzz_spkm_migration-29

Conversation

@chrisguida

Copy link
Copy Markdown

Backport of bitcoin#32624 (the spkm_migration fuzz target for LegacyDataSPKM::MigrateToDescriptor), rooted at branch-29.

Why now: the same upstream commit drops SelectParams(ChainType::MAIN) from initialize_spkm in the scriptpubkeyman target. That call frees the regtest CChainParams that the target's TestingSetup chainman still references, so any wallet code that reads consensus params through the chain interface reads freed memory. #429 adds the first such read, and its fuzz CI job fails with a heap-use-after-free there (reported by @kwsantiago). Upstream removed the line as redundant without noticing the dangling reference.

Commits, in upstream order:

  1. wallet: Remove wallet version and several legacy related functions bitcoin/bitcoin#32977, commit ba01585 only (1 of 6): MigrateToDescriptor decides whether to migrate the internal chain from the HD chain's own version instead of the wallet's feature version. The new target's accounting assumes this. The other five commits of wallet: Remove wallet version and several legacy related functions bitcoin/bitcoin#32977 remove the wallet-version machinery, which 29.x still needs for legacy wallets, so they are left out. For real wallets the two criteria agree: new chains are created split only when the wallet supports HD_SPLIT, and the wallet upgrade path bumps the chain version together with the wallet version.
  2. fuzz: wallet: add target for MigrateToDescriptor bitcoin/bitcoin#32624, the whole PR: adds the spkm_migration target and drops the SelectParams line. Rebased around 29.x's TooDeepDerivPath helper; otherwise unchanged.

Left out: bitcoin#28333 (commit "Migrate hd seed once") and bitcoin#35437 (identical seeds with different IDs), which later adjusted this target. They are not needed for the qa-assets corpora to pass on 29.x, and they change migration behaviour, so they are a separate decision.

Verification, ASan/UBSan fuzz build (clang 19, BUILD_FOR_FUZZING=ON, the fuzz CI job's sanitizers minus libFuzzer) on the 29.x-knots tip with these two commits: scriptpubkeyman clean over its 6784-input qa-assets corpus, spkm_migration clean over its 1328. With bitcoin#32624 alone, spkm_migration fails its accounting assertion on 14 of those inputs; that is the reason for commit 1. The use-after-free reproduces at #429's head a049372 and is gone with the SelectParams line removed.

@kwsantiago

Copy link
Copy Markdown

Wallets upgraded via -upgradewallet in 0.17-0.20 keep VERSION_HD_BASE despite supporting HD split, so with this PR their migration drops the ranged internal combo() descriptor (tested 0.14.3 → 0.20.1 → migrate). Existing keys and funds are kept; upstream behaves the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants