bugfix: Properly handle “unknown” Address Type #27473

1. 
   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.
2. 
   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.

3. achow101 added this to the milestone 25.0 on Apr 16, 2023
4. 
   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.
5. 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
6. Pttn force-pushed on Apr 16, 2023
7. 
   Pttn commented at 9:56 pm on April 16, 2023: contributor

   Makes sense, commit updated.
8. Pttn renamed this:
   Properly handle "unknown" Address Type
   bugfix: Properly handle "unknown" Address Type
   on Apr 16, 2023
9. 
   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?
10. fanquake requested review from josibake on Apr 17, 2023
11. 
    achow101 commented at 1:47 pm on April 17, 2023: member

    ACK 0d6383fda04a99726654945a737bbb1369e0e44a
12. maflcko added the label Wallet on Apr 17, 2023
13. maflcko added the label Needs backport (24.x) on Apr 17, 2023
14. 
    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)

15. furszy approved
16. 
    furszy commented at 2:10 pm on April 17, 2023: member

    ACK https://github.com/bitcoin/bitcoin/commit/0d6383fda04a99726654945a737bbb1369e0e44a
17. fanquake referenced this in commit 0bac52d5cf on Apr 17, 2023
18. achow101 merged this on Apr 17, 2023
19. achow101 closed this on Apr 17, 2023

    ---

20. pablomartin4btc approved
21. 
    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.

22. fanquake removed the label Needs backport (24.x) on Apr 17, 2023
23. 
    fanquake commented at 2:20 pm on April 17, 2023: member

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

Contributors
Pttn DrahtBot achow101 maflcko furszy pablomartin4btc fanquake

Review Requested
josibake

Labels
Wallet

Milestone
25.0