The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
The descriptor documentation (doc/descriptors.md) and BIP380 explicitly
require that hex-encoded public keys start with 02 or 03 (compressed) or
04 (uncompressed). However, the current parsing/inference code permit 06
and 07 (hybrid) encoding as well. Fix this.
So this was permitted in both the legacy wallet and descriptor wallets, but only for watchonly wallets. Since we wouldn’t generate one from a privkey. Right?
Trying to create a legacy wallet, importing a hybrid pubkey and migrating the wallet currently doesn’t return an error (??) but does give a helpful message:
2023-10-04T17:17:54Z [test_hybrid_key_3] Error: Unrecognized descriptor found in wallet test_hybrid_key_3. The database might be corrupted or the software version is not compatible with one of your wallet descriptors. Please try running the latest software version Details: Invalid descriptor: pkh(): Hybrid public keys are not allowed: iostream error
Trying to load a descriptor wallet containing a descriptor with a hybrid pubkey does return an error, although a slightly misleading one:
error code: -4 error message: Wallet loading failed. Unrecognized descriptor found. Loading wallet /home/darosior/.bitcoin/regtest/wallets/test_hybrid_key_2/wallet.dat
The wallet might had been created on a newer version. Please try running the latest software version.
And it still logs a helpful error message mentioning hybrid keys.
So this looks good to me overall. A node wouldn’t start with a wallet containing a descriptor with a hybrid pubkey and we do log the right error message so a user would be able to figure out why it’s failing. The migration not returning an error is a bit more concerning but also orthogonal to this PR. I’ll ACK with a fresh head tomorrow morning.
