Assert that the HRP is lowercase in Bech32::Encode #16792

1. 
   meshcollider commented at 12:51 AM on September 3, 2019: contributor

   From BIP-173:

   The lowercase form is used when determining a character's value for checksum purposes. Encoders MUST always output an all lowercase Bech32 string. If an uppercase version of the encoding result is desired, (e.g.- for presentation purposes, or QR code use), then an uppercasing procedure can be performed external to the encoding process.

   Currently if HRP contains uppercase characters, the checksum will be generated over these uppercase characters resulting in mixed-case output that will always be invalid even if the case is changed manually after encoding. This shouldn't happen because both prefix's bc and tb are lowercase currently, but we assert this condition anyway.

   This is consistent also with the C reference implementation

2. fanquake added the label Wallet on Sep 3, 2019
3. 
   fanquake commented at 2:37 AM on September 3, 2019: member

   Could we call LowerCase here inside ExpandHRP?

   https://github.com/bitcoin/bitcoin/blob/33f9750b1b86a705d092b0e1314ed15287c45239/src/bech32.cpp#L109

4. fanquake requested review from sipa on Sep 3, 2019
5. 
   practicalswift commented at 12:40 PM on September 3, 2019: contributor

   Concept ACK on checking preconditions, but assert(…) is too drastic/risky.

   Alternatives:

   • Change return type to bool and return false if the precondition does not hold
   • Could apply LowerCase as suggested by @fanquake
6. 
   MarcoFalke commented at 12:47 PM on September 3, 2019: member

   assert(…) is too drastic/risky.

   What is the risk?

7. 
   practicalswift commented at 1:35 PM on September 3, 2019: contributor

   @MarcoFalke I was simply thinking about the case where a future call site pass user- or attacker controlled input in the form of an HRP to Encode(…).

   Perhaps that is farfetched but returning success in some form (say using true or returning a non-empty string) instead of assert:ing would also be consistent with bech32_encode(…) in the C implementation.

8. 
   promag commented at 1:59 PM on September 3, 2019: member

   ACK adding the assertion, currently it's not user input.

9. 
   laanwj commented at 2:34 PM on September 3, 2019: member

   I'm fine with an assertion. Please do add a warning to the documentation of the function in the header file that an assertion error will happen on invalid hrp, so that developers using the function are warned.

10. 
    meshcollider commented at 12:24 AM on September 4, 2019: contributor

    Please do add a warning to the documentation of the function in the header file

    Will do

    This is Sipa's Original Vision (SV)

    meshcollider: What's the "correct" behaviour, error or lowercase()? sipa: assert I think

11. Assert that the HRP is lowercase in Bech32::Encode 2457aea83c
12. 
    laanwj commented at 11:31 AM on September 5, 2019: member

    ACK 2457aea83c1f9fba708e2335bb197950bf0b6244

13. laanwj referenced this in commit 5667b0d758 on Sep 5, 2019
14. laanwj merged this on Sep 5, 2019
15. laanwj closed this on Sep 5, 2019

    ---

16. meshcollider deleted the branch on Sep 5, 2019
17. PastaPastaPasta referenced this in commit 595be44314 on Jun 27, 2021
18. PastaPastaPasta referenced this in commit 910309e895 on Jun 28, 2021
19. PastaPastaPasta referenced this in commit ca3a0cd1c0 on Jun 29, 2021
20. PastaPastaPasta referenced this in commit fe7bcdae59 on Jul 1, 2021
21. PastaPastaPasta referenced this in commit 984826c782 on Jul 1, 2021
22. PastaPastaPasta referenced this in commit a9785dd873 on Jul 12, 2021
23. PastaPastaPasta referenced this in commit e8f63fc657 on Jul 13, 2021
24. DrahtBot locked this on Dec 16, 2021

Review Requested
sipa