Skip to content
This repository was archived by the owner on May 27, 2026. It is now read-only.

Bech32 (BIP173) address support - #115

Merged
prusnak merged 3 commits into
trezor:masterfrom
jhoenicke:bech32
Nov 3, 2017
Merged

Bech32 (BIP173) address support#115
prusnak merged 3 commits into
trezor:masterfrom
jhoenicke:bech32

Conversation

@jhoenicke

Copy link
Copy Markdown
Contributor

Added bech32 (code by Peter Wuille).
Adapted the unit tests to our library.

@mruddy

mruddy commented Oct 11, 2017

Copy link
Copy Markdown

I took sipa's and dropped over this version to see the diff:

--- a/segwit_addr.c
+++ b/segwit_addr.c
@@ -51,9 +51,13 @@ int bech32_encode(char *output, const char *hrp, const uint8_t *data, size_t dat
     uint32_t chk = 1;
     size_t i = 0;
     while (hrp[i] != 0) {
-        if (hrp[i] >= 'A' && hrp[i] <= 'Z') return 0;
-        if (!(hrp[i] >> 5)) return 0;
-        chk = bech32_polymod_step(chk) ^ (hrp[i] >> 5);
+        int ch = hrp[i];
+        if (ch < 33 || ch > 126) {
+            return 0;
+        }
+
+        if (ch >= 'A' && ch <= 'Z') return 0;
+        chk = bech32_polymod_step(chk) ^ (ch >> 5);
         ++i;
     }
     if (i + 7 + data_len > 90) return 0;

When given an invalid HRP, like 0x80 or 0x20, sipa's will return 0 (fail to encode) and this version encodes an invalid bech32 string ("�1wzqgcn" and " 1nwldj5", respectively).

Neither version accepts uppercase HRP values. They could as long as they follow the BIP guidance that "The lowercase form is used when determining a character's value for checksum purposes." and "Encoders MUST always output an all lowercase Bech32 string."
Thus, input HRP=A and data=empty would generate: "a12uel5l"
The various reference implementations handle this differently.
The python version is invalid according to the BIP, e.g.:

>>> import segwit_addr
>>> segwit_addr.bech32_encode('a', [])
'a12uel5l'
>>> segwit_addr.bech32_encode('A', [])
'A1g7sgd8' <-- BUG: should output 'a12uel5l'

This was helpful for testing: make clean && make test_check && ./test_check

The reference code has not been updated to include all of the latest test vectors either (https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki)

The C++ version that got merged into Bitcoin Core got more review (bitcoin/bitcoin#11167) and is implemented differently with better comments. You may want to start from that version and back out the C++ specific stuff like std::string, etc...

@jhoenicke

jhoenicke commented Oct 11, 2017

Copy link
Copy Markdown
Contributor Author

I imported the source code a while ago, just found the time to add it. But I can update the last patches from Sipa's repository. Hope that the changes to the tests apply without too much conflict...

I'm not sure I want to get the C++ code and backport it to C. It may introduce new problems with memory management, since C++ strings resize themselves automatically. I think it is better to stick to the reference implementation in sipa's repository unless there is a known problem with it.

EDIT: Okay, I see you claim there are problems, if we want to support hrp with uppercase letters. OTOH we will probably only call this function only with fixed hrp's for the coins we support.

@mruddy

mruddy commented Oct 11, 2017

Copy link
Copy Markdown

Right, there are some practicalities that make the inconsistencies with the BIP less of a concern. I think another one might be around empty HRP IIRC (bitcoin/bips#587 (comment)).

I wrote some java that tried to adhere closely to the BIP (https://github.com/sipa/bech32/pull/35/files) so that's why I'm aware of this.

You're probably right that the best strategy is to just use sipa's latest reference code and then update if the reference code updates.

@prusnak
prusnak merged commit f366fb8 into trezor:master Nov 3, 2017
@prusnak
prusnak deleted the bech32 branch November 3, 2017 17:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants