The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31378.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

No conflicts as of last run.

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 IsMine for 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.

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 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?

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?

Then let's do it afterwards.

Is this now pending #31495 instead of the closed ##30328?

we are enforcing this rules within the descriptors parsing logic.

That sounds dumb. Why are we?

@luke-jr can you rephrase your comment in a friendlier way please

we are enforcing this rules within the descriptors parsing logic.

That sounds dumb. Why are we?

I assume this was simply because there was no need to represent them in the descriptors' language. They've been non-standard for at least the past 10 years now.

However, there's no documentation about it. This restriction has always been present; it was introduced in the first commit that added descriptors in #13697.

We could relax the parsing restriction if a real use case arises. Until then, the descriptor's inference and parsing must allow for round-trips.

That being said, this PR might not end up getting merge. #31495 contains a general round-trip check that does not touch the legacy wallet IsMine code (haven't reviewed it in detail yet). And the fix for the descriptors issue is in #31404.

Could turn into draft while the CI is failing?

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.