descriptors: disallow hybrid public keys #28587
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee 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. ConflictsReviewers, 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. |
sipa
changed the title
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.
|
Concept ACK. |
sipa
force-pushed
the
202310_no_hybrid_descriptors
branch
from
7da67f9
to
c1e6c54
Compare
|
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:
Trying to load a descriptor wallet containing a descriptor with a hybrid pubkey does return an error, although a slightly misleading one:
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. |
|
ACK c1e6c54 |
|
ACK c1e6c54 |
c1e6c54 descriptors: disallow hybrid public keys (Pieter Wuille) Pull request description: Fixes bitcoin#28511 The descriptor documentation (`doc/descriptors.md`) and [BIP380](https://github.com/bitcoin/bips/blob/master/bip-0380.mediawiki) 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. ACKs for top commit: darosior: ACK c1e6c54 achow101: ACK c1e6c54 Tree-SHA512: 23b674fb420619b2536d12da10008bb87cf7bc0333ec59e618c0d02c3574b468cc71248475ece37f76658d743ef51e68566948e903bca79fda5f7d75416fea4d
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. Github-Pull: bitcoin#28587 Rebased-From: c1e6c54
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. Github-Pull: bitcoin#28587 Rebased-From: c1e6c54
|
Release note to be added in #29023. |
Fixes #28511
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.