Skip to content

Conversation

Pttn
Copy link
Contributor

@Pttn Pttn commented Apr 16, 2023

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
Copy link
Contributor

DrahtBot commented Apr 16, 2023

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 achow101 added this to the 25.0 milestone Apr 16, 2023
@achow101
Copy link
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.

Fixes bitcoin#27472

Signed-off-by: Pttn <28868425+Pttn@users.noreply.github.com>
@Pttn Pttn force-pushed the handle-unknown-address-type branch from dfec384 to 0d6383f Compare April 16, 2023 21:54
@Pttn
Copy link
Contributor Author

Pttn commented Apr 16, 2023

Makes sense, commit updated.

@Pttn Pttn changed the title Properly handle "unknown" Address Type bugfix: Properly handle "unknown" Address Type Apr 16, 2023
@maflcko
Copy link
Member

maflcko commented Apr 17, 2023

Looks like this may have been introduced in f5649db, so not a regression in 25.x, but 24.x?

@fanquake fanquake requested a review from josibake April 17, 2023 08:47
@achow101
Copy link
Member

ACK 0d6383f

@maflcko
Copy link
Member

maflcko commented Apr 17, 2023

lgtm ACK 0d6383f

In a follow-up, might be good to at least add a regression test, or even extend the fuzz tests, see #27472 (comment)

Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 0d6383f

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Apr 17, 2023
Fixes bitcoin#27472

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

Github-Pull: bitcoin#27473
Rebased-From: 0d6383f
@achow101 achow101 merged commit 4ad20a2 into bitcoin:master Apr 17, 2023
Copy link
Member

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ACK 0d6383f.

@fanquake
Copy link
Member

Backported to 24.x in #27474.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 17, 2023
0d6383f Don't return OutputType::UNKNOWN in ParseOutputType (Pttn)

Pull request description:

  Fixes bitcoin#27472 by also handling at the relevant places the case where ParseOutputType returns `OutputType::UNKNOWN`, and not just when it returns `std::nullopt`.

ACKs for top commit:
  achow101:
    ACK 0d6383f
  MarcoFalke:
    lgtm ACK 0d6383f
  furszy:
    ACK bitcoin@0d6383f

Tree-SHA512: 776793027b926283d3162e69fb9c8883c814b19bcce4574ccdf8e3140a1ec4ebc4aa8ccd1abae7ef3571f942d2e6c35305fd1244259540d90605106e01afc34c
fanquake added a commit to fanquake/bitcoin that referenced this pull request Apr 18, 2023
dc711fb doc: update 24.1 release notes (fanquake)
fc8c1a8 doc: fix/improve warning helps in {create,load,unload,restore}wallet (Jon Atack)
3a26b19 bugfix: rest: avoid segfault for invalid URI (pablomartin4btc)
c40b1da depends: fix compiling bdb with clang-16 on aarch64 (fanquake)
0bac52d Don't return OutputType::UNKNOWN in ParseOutputType (Pttn)

Pull request description:

  Backports:
  * bitcoin#27279 (only f73782a)
  * bitcoin#27462
  * bitcoin#27468
  * bitcoin#27473

ACKs for top commit:
  stickies-v:
    ACK dc711fb
  hebasto:
    re-ACK dc711fb
  jonatack:
    ACK dc711fb

Tree-SHA512: 72c673be82689e3c3a1c2564a1fdd6afe0b357b7aa8bec9524fe6999804fbccf310da0b074e647af14b753e5e695024e268fe4f69aa58747f541f7f429ebede6
@bitcoin bitcoin locked and limited conversation to collaborators Apr 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mishandled "unknown" Address Type
7 participants