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

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--021abf342d371248e50ceaed478a90ca-->

   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. fanquake cross-referenced this on Apr 17, 2023 from issue [24.x] Additional backports for 24.1 by fanquake
19. achow101 merged this on Apr 17, 2023
20. achow101 closed this on Apr 17, 2023

    ---

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

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

    Backported to 24.x in #27474.

25. sidhujag referenced this in commit dafb6f11ce on Apr 17, 2023
26. fanquake referenced this in commit 15a24781d0 on Apr 18, 2023
27. bitcoin locked this on Apr 16, 2024

Contributors

PttnDrahtBotachow101maflckofurszypablomartin4btcfanquake

Review Requested
josibake

Milestone
25.0