Add Bech32.cpp LocateErrors case to check for minimum length. #28159

issue russeree openend this issue on July 26, 2023
  1. russeree commented at 7:39 am on July 26, 2023: contributor

    Please describe the feature you’d like to see added.

    Add a “Bech32 string too short” case to Bech32.cpp LocateErrors to reduce ambiguity and collisions with “Invalid separator position” case.

    Error representation when calling LocateErrors within src/test/bech32_tests.cpp is ambiguous and incorrect in some cases because of a missing bech32 string to short case. Not having a too short case causes a fall though into the conditions of "Invalid separator position" || "Missing separator"

    Describe the solution you’d like

    Simply add a check for a minimum Bech32 string size after or during the check for the maximum size which is already implemented.

    Cascading effects of this would be seen in DecodeDestination if https://github.com/bitcoin/bitcoin/blob/32c15237b656209b932c5d6d2e20736c0e3d5a34/src/key_io.cpp#L85-L86 was patched to not consider bech32 invalid then attempt to Base58Decode it becuase of the incorrect HRP for the current chain.

    Referenced in #26290

    PR #27260

    Describe any alternatives you’ve considered

    You could case this into other logic on a case by case basis in code. Example make a speical case inside DecodeDestination to display the correct error message to the user by if LocateErrors returns ‘Invalid "separator position" || "Missing separator" then you check to see if the string is to short.

    Please leave any additional context

    As an aside there is already a check for string length greater than 90 (MAX LEN)

    in src/test/bech32_tests.cpp both [“10a06t8”,“1qzzfhee”] produce {"Invalid separator position", {0}} The former is non conformant to BIP173 due not even having enough chars to represent a BASE32 string and the second genuinely has a wrong separator position.

  2. russeree added the label Feature on Jul 26, 2023
  3. fanquake commented at 8:16 am on July 26, 2023: member
    I don’t think there’s a need to open an issue if you’re going to open a PR 10 minutes later. Would suggest combining anything relevant from here into the PR description, and save splitting up the discussion.
  4. fanquake closed this on Jul 26, 2023

  5. russeree commented at 8:18 am on July 26, 2023: contributor
    I apologize for the double posting, I have seen others do it in the past. Lesson learned.
  6. bitcoin locked this on Jul 25, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-02 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me