bugfix: Properly handle "unknown" Address Type #27473

pull Pttn wants to merge 1 commits into bitcoin:master from Pttn:handle-unknown-address-type changing 1 files +0 −2

Pttn commented at 9:30 PM on April 16, 2023: contributor

Fixes #27472 by also handling at the relevant places the case where ParseOutputType returns OutputType::UNKNOWN, and not just when it returns std::nullopt.
DrahtBot commented at 9:30 PM on April 16, 2023: contributor



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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK achow101, MarcoFalke, furszy

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
achow101 added this to the milestone 25.0 on Apr 16, 2023
achow101 commented at 9:39 PM on April 16, 2023: member

Given that we shouldn't ever accept unknown as an output type, I think it would make sense to just make ParseOutputType to never return OutputType::UNKNOWN instead of having every caller handle it specially.

Type	Reviewers
ACK	achow101, MarcoFalke, furszy

Don't return OutputType::UNKNOWN in ParseOutputType

Fixes https://github.com/bitcoin/bitcoin/issues/27472

Signed-off-by: Pttn <28868425+Pttn@users.noreply.github.com>

0d6383fda0

Pttn force-pushed on Apr 16, 2023
Pttn commented at 9:56 PM on April 16, 2023: contributor

Makes sense, commit updated.
Pttn renamed this:
~~Properly handle "unknown" Address Type~~
bugfix: Properly handle "unknown" Address Type
on Apr 16, 2023
maflcko commented at 8:22 AM on April 17, 2023: member

Looks like this may have been introduced in f5649db9d5e984ba7f376ccfd5b0a627f5c42402, so not a regression in 25.x, but 24.x?
fanquake requested review from josibake on Apr 17, 2023
achow101 commented at 1:47 PM on April 17, 2023: member

ACK 0d6383fda04a99726654945a737bbb1369e0e44a
maflcko added the label Wallet on Apr 17, 2023
maflcko added the label Needs backport (24.x) on Apr 17, 2023
maflcko commented at 2:04 PM on April 17, 2023: member

lgtm ACK 0d6383fda04a99726654945a737bbb1369e0e44a

In a follow-up, might be good to at least add a regression test, or even extend the fuzz tests, see #27472 (comment)
furszy approved
furszy commented at 2:10 PM on April 17, 2023: member

ACK https://github.com/bitcoin/bitcoin/commit/0d6383fda04a99726654945a737bbb1369e0e44a
fanquake referenced this in commit 0bac52d5cf on Apr 17, 2023
achow101 merged this on Apr 17, 2023
achow101 closed this on Apr 17, 2023
pablomartin4btc approved
pablomartin4btc commented at 2:18 PM on April 17, 2023: member

Reproduced the issue manually from #27472. Agree with above to add more test coverage if possible.

ACK https://github.com/bitcoin/bitcoin/commit/0d6383fda04a99726654945a737bbb1369e0e44a.
fanquake removed the label Needs backport (24.x) on Apr 17, 2023
fanquake commented at 2:20 PM on April 17, 2023: member

Backported to 24.x in #27474.
sidhujag referenced this in commit dafb6f11ce on Apr 17, 2023
fanquake referenced this in commit 15a24781d0 on Apr 18, 2023
bitcoin locked this on Apr 16, 2024