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.
No conflicts as of last run.
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).
cdaa3a58dc16d6a27248dc4cdec2dd1909eee7fe “wallet: bugfix, stop treating multisig consensus-invalid/unspendable scripts as ours” modifies legacy IsMine(), ostensibly to mix issues in migration. However, I don’t think doing that makes sense that this point as legacy IsMine is going to be removed from migration with #30328, and any changes to it may result in incompatibilities with legacy wallets and migrated wallets.
I think that fixes to migration should be limited to the migration only code. Perhaps it would be easier to cherry pick the tests and migration specific fixes into #30328?
cdaa3a5 “wallet: bugfix, stop treating multisig consensus-invalid/unspendable scripts as ours” modifies legacy
IsMine(), ostensibly to mix issues in migration. However, I don’t think doing that makes sense that this point as legacyIsMineis going to be removed from migration with #30328, and any changes to it may result in incompatibilities with legacy wallets and migrated wallets.
I’m not going too strong over this point but.. it’s hard for me to see the incompatibilities when the changes block two consensus-invalid scripts and a +10 year-old non-standard script that is already blocked in other parts of the legacy wallet. Arguably, this could also be labeled as a legacy wallet bugfix.
I think that fixes to migration should be limited to the migration only code. Perhaps it would be easier to cherry pick the tests and migration specific fixes into #30328?
I was aiming to keep #30328 purely as a refactoring without mixing in bug fixes. Reviewing the PR’s matching functionality has been challenging in its current state, so I think that it would be good if we don’t expand it more than strictly necessary.
Note: If, after this comment, the general consensus is still to leave IsMine untouched, maybe postponing this PR until after #30328 would be the path that satisfies everyone?