Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bech32: add new EncodeM and DecodeGeneric functions for bech32 #202

Merged
merged 3 commits into from Sep 22, 2021

Conversation

Roasbeef
Copy link
Member

@Roasbeef Roasbeef commented Jul 17, 2021

In this commit, we add two new package level functions: EncodeM, and
DecodeGeneric. The new encode method is intended to allow callers to
specify that they want to use the new bech32m checksum. This should be
used when encoding segwit addresses with version 1 and beyond. The new
DecodeGeneric function allows a caller to decode a bech32 and bech32m
string with a single function. A new return value is added which is the
version of the returned bech32 string, which allows callers to perform
additional segwit addr validation (v1+ should use bech32m etc).

We opted to add new functions rather than modifying the existing
functions to not cause a breaking API change, as most uses in the wild
can just use the existing functions, and only taproot related logic/code
needs to worry about the new methods.

A series of tests have been added to ensure that DecodeGeneric
extracts the proper bech version, and we've also adopted the bech32m
tests from BIP 350.

Fixes part of btcsuite/btcd#1735.

Fixes #195

@onyb
Copy link
Contributor

onyb commented Jul 17, 2021

Mentioning here so we don't forget - we should probably update the relevant Bitcoin wiki article: https://en.bitcoin.it/wiki/Bech32_adoption#Libraries

bech32/bech32.go Outdated Show resolved Hide resolved
bech32/bech32.go Outdated Show resolved Hide resolved
@Roasbeef
Copy link
Member Author

Fixed typos, fixed linter error.

Copy link
Contributor

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Great to have this implemented in a generic way. So we'd even be able to decode segwit v0 addresses that use bech32m (even if probably no wallet will ever create those).

LGTM 💯

// Calculate the expected checksum, given the hrp and payload data.
// Calculate the expected checksum, given the hrp and payload
// data. We'll actually compute _both_ possibly valid checksum
// to further aide in debugging.
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, should be very useful to the user.

{"an83characterlonghumanreadablepartthatcontainsthenumber1andtheexcludedcharactersbio1tt5tgs", Version0},
{"abcdef1qpzry9x8gf2tvdw0s3jn54khce6mua7lmqqqxw", Version0},
{"11qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqc8247j", Version0},
{"split1checkupstagehandshakeupstreamerranterredcaperred2y9e3w", Version0},
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we throw a valid bech32 segwit v1 address in here just for completeness' sake? E.g. bc1puxkz8vpy900c7z4q4302lkc3jjr2s42mayfqzzgr5yqdk5mgma3s0kntlh

Copy link

Choose a reason for hiding this comment

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

Yeah, wouldn't hurt. I guess we'd recommend implementing all test vectors from the BIP. The last one in the "valid" section of the v0-v16 test vectors, bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqzk5jj0, also encodes a P2TR locking script.

@Roasbeef
Copy link
Member Author

So we'd even be able to decode segwit v0 addresses that use bech32m (even if probably no wallet will ever create those).

Yeah exactly, the idea is that say in lnd, we can detect that case and reject the send w/ a detailed error message.

@gitteri
Copy link

gitteri commented Sep 16, 2021

Looks like someone sent btc to the test vector mentioned above... most block explorers are unable to parse bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqzk5jj0. Can we get this in soon?
https://chain.so/address/BTC/bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqzk5jj0

In this commit, we add an additional field to the ErrInvalidChecksum,
the bech32m version of a checksum. When decoding, we don't now what
version they actually _intended_ to use, so we'll opt to include both
checksums to aide in debugging and error reporting.
In this commit, we add two new package level functions: `EncodeM`, and
`DecodeGeneric`. The new encode method is intended to allow callers to
specify that they want to use the new bech32m checksum. This should be
used when encoding segwit addresses with version 1 and beyond. The new
`DecodeGeneric` function allows a caller to decode a bech32 and bech32m
string with a single function. A new return value is added which is the
version of the returned bech32 string, which allows callers to perform
additional segwit addr validation (v1+ should use bech32m etc).

We opted to add new functions rather than modifying the existing
functions to not cause a breaking API change, as most uses in the wild
can just use the existing functions, and only taproot related logic/code
needs to worry about the new methods.

A series of tests have been added to ensure that `DecodeGeneric`
extracts the proper bech version, and we've also adopted the bech32m
tests from BIP 350.
@Roasbeef
Copy link
Member Author

I've added all the additional test vectors. Note that this PR doesn't implement new versions of btcuil.Address for the new script versions, so some of the vectors don't apply as directly. I'll add them in the follow up PR that creates new btcutil.Address implementations for taproot addresses.

@Roasbeef Roasbeef merged commit 43b172e into btcsuite:master Sep 22, 2021
btcd-dev automation moved this from Pending reviews to Done Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
btcd-dev
  
Done
Development

Successfully merging this pull request may close these issues.

Add support for the bech32m encoding defined in BIP-350
6 participants