Ensure legacy wallet migration skips the never standard bare multisig with +3 keys and consensus-invalid multisig scripts. Treating them as valid causes migration to crash because we are enforcing this rules within the descriptors parsing logic.
Testing Notes: This can be verified by cherry-picking and running the test commit on master. It will crash there but pass on this branch.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31378.
See the guideline for information on the review process. A summary of reviews will appear here.
IsMine for those?
Ruling out consensus-invalid scripts makes sense, but discarding non-standard scripts is a much bigger leap. Also it would be a wack-a-mole: does the migration crash on any non-standard script? How much of that do you want to cover?
Ok, I might have made the PR/commit description too broad when the actual changes are very specific. Migration does not crash on all non-standard scripts, it only crashes on scripts that fail to be parsed by the descriptors’ logic. The specific non-standard crash this PR addresses is a bare multisig with +3 keys. And I don’t think we want to cover all non-standard rules; we just need to handle the ones also applied by the current descriptors parsing logic.
Ultimately, we’re preserving/freezing the legacy wallet’s existing behavior ahead of its removal. We shouldn’t allow spendability or migration (even though it’s not possible anyway) of scripts that were not functional during the lifetime of the legacy wallet anyway.
Is it not possible to make the migration not crash on valid but not standard scripts instead of changing
IsMinefor those?
It’s possible, but it would mean duplicating more code. Still, at least for this case, I don’t think it matters much. #30328 inlines the IsMine() logic into the migration code, so the entire legacy wallet support can be removed by the next release.
In other words, duplicating code now won’t change the outcome; if #30328 proceeds, the code will end up been consolidated into the same migration function anyway.
I’ll update the PR description to reflect the actual scope of this fix. it’s not as broad as I initially described.
203+ return IsMineResult::INVALID;
204+ }
205+
206+ // Never treat bare multisig outputs as ours (they can still be made watchonly-though)
207+ if (sigversion == IsMineSigVersion::TOP) {
208+ if (keys.size() > 3) return IsMineResult::INVALID; // These are standard wise non-spendable
IsStandard.
Ensure legacy wallet migration skips the never standard bare multisig with +3 keys
and consensus-invalid multisig scripts. Treating them as valid causes migration to
crash because we are enforcing this rules within the descriptors parsing logic.
319+ # Now migrate it and verify we don't crash due to a non-allowed descriptor migration
320+ wallet.migratewallet()
321+ wallet.unloadwallet()
322+
323+ ##############################################################
324+ # Import a consensus-wise invalid p2sh multisig with 20 keys #
In d641931: A p2sh multisig with 20 keys is invalid? I thought 20 keys are the limit, but still valid.
Yes, they are. The limit is not based on the number of keys, it is on the size of the redeem script. Which can go up to 520 bytes max (which limits the number of keys inside a p2sh to 15).