support multisig bech32 address decoding #6237

assafmo · 2020-05-17T21:37:45Z

Description

Multisig addresses are 218 characters long (for 6 character length prefix like cosmos), but the bech32 specs limit the address length to 90 characters by default, and the library you're using to decode bech32 doesn't allow to pass the limit as an arg.

This only affects reading bech32 address, so for example on a chain with a multisig address, if you do gaiad export --for-zero-height > x.json and then gaiad validate-genesis x.json you'll get ERROR: error validating genesis file x.json: failed to unmarshal auth genesis state: decoding bech32 failed: invalid bech32 string length 218.

To fix this I forked the bech32 library and allowed for passing the character limit as an input, then passed 250 as the limit which is what needed for decoding multisig addresses and then some. I used 250 instead of 218 to support longer address prefixes for other chains. (In theory 1023 chars is still okay, longer than that the checksum at the end cannot guarantee error detection)

This PR also fixes this bug: #6054

Refs:

https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki#Bech32

auto-comment · 2020-05-17T21:37:50Z

codecov · 2020-05-17T21:51:31Z

Codecov Report

Merging #6237 into master will decrease coverage by 1.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6237      +/-   ##
==========================================
- Coverage   61.42%   60.26%   -1.16%     
==========================================
  Files         509      390     -119     
  Lines       31457    25331    -6126     
==========================================
- Hits        19321    15266    -4055     
+ Misses      10637     8897    -1740     
+ Partials     1499     1168     -331

alessio

ACKd

alexanderbez · 2020-05-18T00:13:40Z

types/bech32/bech32.go

@@ -18,7 +18,7 @@ func ConvertAndEncode(hrp string, data []byte) (string, error) {

 // DecodeAndConvert decodes a bech32 encoded string and converts to base64 encoded bytes.
 func DecodeAndConvert(bech string) (string, []byte, error) {
-	hrp, data, err := bech32.Decode(bech)
+	hrp, data, err := bech32.Decode(bech, 250)


Pretty sure we have 90 for a reason. But it won't work with multisigs. I'm not familiar with any issues that could arise if we change it.

/cc @zmanian

Yeah, 90 is from the bech32 specs and it was hard coded in the bech32 library.

In practice cosmos-sdk uses longer addresses, mainly for multisig public keys, and the 90 limit creates bugs (e.g. in Enigma people are using multisigs and thus we cannot export the state and validate it if we want to hard fork)

zmanian · 2020-05-18T07:38:42Z

What's annoying about the 90 character limit is that the bech32 spec suggests warning on > 90 characters. The kernel chosen should support up to 1024 characters reasonably.

Generally what to do about to support encodings of larger values seems to be an open problem.

See the Checksum design section from https://en.bitcoin.it/wiki/BIP_0173

A lot of people were angry at my for requiring forks of bech32 across the ecosystem in one my original designs.

alexanderbez · 2020-05-18T13:41:03Z

Can we accept this PR @zmanian? We'll be able to finally support JSON encoding for accounts again using Bech32. I just don't know the repercussions of choosing a larger value.

zmanian · 2020-05-18T17:42:19Z

I don't think the right place for the forked > 90 character bech32 implementation should live in the engima github org.

It should live in the Cosmos org or maybe a new github org that's going to be focused on maintaining multiple implementations of the modified bech32.

This is the line that always caused me some anxiety.

Even though the chosen code performs reasonably well up to 1023 characters, other designs are preferable for lengths above 89 characters (excluding the separator).

The advantages of using the same kernel with a larger character limit is that it's a small fork of widely available software.

The downside is that an alternative kernel would detect errors better in longer strings.

How do we maximally retain the good properties of bech32 while loosening the restriction?

assafmo · 2020-05-19T22:37:53Z

That's really that only change I made to the base library: scrtlabs/btcutil@7ac7d43 (+ made the tests to reflect the changed API)
I don't know if there's anything to maintain here.

zmanian · 2020-05-19T23:16:39Z

I think we will probably need to have implementations in other languages with this API so we should probably have fork.

Should we also enforce that limit must be no greater than 1023?

romeo4934 · 2020-06-03T15:11:53Z

any update on this PR?

alexanderbez · 2020-06-03T15:15:56Z

Again, I don't have enough context to really give approval here. Why 250? Why not 1024?

assafmo · 2020-06-03T16:14:36Z

250 because it's enough for multisig with any prefix. 1024 is also good.

…

On Wed, Jun 3, 2020, 18:16 Alexander Bezobchuk ***@***.***> wrote: Again, I don't have enough context to really give approval here. Why 250? Why not 1024? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#6237 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFNWHRJGMWYEGMMUNESRPCDRUZSL5ANCNFSM4NDR73MA> .

alexanderbez · 2020-06-03T16:32:14Z

My point is the value should not be arbitrary and have reasoning behind it. And not only that, but also be flexible for future use without having to do this again. So it would make sense to select the largest value possible, no?

romeo4934 · 2020-06-03T17:38:34Z

I agree. FYI, it seems this command in the v38 is broken as well when I query a multisig

iovnscli query account star1j4dx6dwgz8nfpmtecmzvqlv5p4tjahvd5q9qte
ERROR: decoding bech32 failed: invalid bech32 string length 216

assafmo · 2020-06-03T20:13:02Z

My point is the value should not be arbitrary and have reasoning behind it. And not only that, but also be flexible for future use without having to do this again. So it would make sense to select the largest value possible, no?

I agree. But I think the checksum guarantee is up to 1023 bytes: https://zips.z.cash/zip-0173

romeo4934 · 2020-06-05T06:55:33Z

Do you think you will solve this issue?
Otherwise, we will do like Enigma or Kava and add our own patch.

…s the checksum)

assafmo · 2020-06-28T13:19:28Z

@alexanderbez @zmanian Now the number isn't arbitrary. I changed it to 1023 chars, which is the maximum length that supports the checksum property of bech32. ☺️

alexanderbez · 2020-06-29T13:08:19Z

Should we port this over to the cosmos repo? What are your plans around maintaining this library?

assafmo · 2020-06-29T13:18:05Z

Should we port this over to the cosmos repo?

I'd prefer it, yes.

What are your plans around maintaining this library?

No plans to maintain this. We have a module in our mainnet right now that makes heavy use of multisigs, so this is already live in our mainnet, but I don't expect this module to stay with us for more than a year.

Ideally for us this PR will be merged, so that we could again point our code to this repo for cosmos-sdk.

alessio · 2020-06-29T18:59:52Z

How about importing github.com/enigmampc/btcutil/bech32 into the sdk? Either as top level package or crypto/ subdirectory

alexanderbez

ACK

onyb · 2020-07-14T09:47:23Z

This issue will most likely be addressed once btcsuite/btcutil#177 gets merged. If you have an example of a multisig bech32 address used in Cosmos, I am happy to include it in our tests.

assafmo · 2020-07-14T11:01:42Z

If you have an example of a multisig bech32 address used in Cosmos, I am happy to include it in our tests.

Currently, this is not possible on any cosmos-based chain, except I think for the Secret Network and Kava, and that is only because we applied this patch ourselves.

Anyway, here's an example: The address cosmos1337nkls6z6z2ygngzrawmp25l8qvywkw2xchwq with pubkey cosmospub1ytql0csgqvfzd666axrjzqmn5q2ucztcyxw8hvlzen94ay05tegaerkug5pn3xn8wqdymt598ufzd666axrjzqsxllmwacap3f6xyc4x30jl8ecrcs2tze3zzgxkmthcsqxnqxhwwgfzd666axrjzqs2rlu3wz5gnslgpprszjr8r65n0d6y39q657th77eyvengtk3z0y6h2pnk

If you try to broadcast a tx with that pubkey, it will fail.

$ gaiacli keys add 1                                        
{
  "name": "1",
  "type": "local",
  "address": "cosmos1w2h5x0kmxeluhvv8mvrmdujrs9aldwmsxewmmz",
  "pubkey": "cosmospub1addwnpepqg9pl7ghp2yfc05qs3cpfpn3a2fhkazgjsd209ml0vjxve59mg38jr2yftf",
  "mnemonic": "bind there illegal digital badge taxi guess oyster toy void lunch potato vivid oak canvas pill discover describe sentence disorder hint brisk husband winter"
}
$ gaiacli keys add 2
{
  "name": "2",
  "type": "local",
  "address": "cosmos1zhldgcq2lfgwzflewq0vr0nqpm5kz7xtge2fmj",
  "pubkey": "cosmospub1addwnpepqde6q9wvp9uzr8rmk03vej67j869u5wu3mwy2qecnfnhqxjd46zn78aj2yn",
  "mnemonic": "demise inner skin twist text elephant mind below reflect fan ecology bone black verify sweet song caught jump possible fetch marble language pony tongue"
}
$ gaiacli keys add 3
{
  "name": "3",
  "type": "local",
  "address": "cosmos18ds9jlaehg09vrkk9zmjr8qacqzmj73jhaxvj9",
  "pubkey": "cosmospub1addwnpepqgr0lahwuwsc5arzv2nghe0nuupug993vc3pyrtd4mugqrfsrth8yw7mcxh",
  "mnemonic": "outside nasty siege rice pave gloom mistake hood leaf true spider traffic bubble breeze swim announce feel release garbage exercise suspect kiwi artist beef"
}
$ gaiacli keys add --multisig=1,2,3 --multisig-threshold=3 m
Key "m" saved to disk.
$ gaiacli keys list                                         
[
  {
    "name": "1",
    "type": "local",
    "address": "cosmos1w2h5x0kmxeluhvv8mvrmdujrs9aldwmsxewmmz",
    "pubkey": "cosmospub1addwnpepqg9pl7ghp2yfc05qs3cpfpn3a2fhkazgjsd209ml0vjxve59mg38jr2yftf"
  },
  {
    "name": "2",
    "type": "local",
    "address": "cosmos1zhldgcq2lfgwzflewq0vr0nqpm5kz7xtge2fmj",
    "pubkey": "cosmospub1addwnpepqde6q9wvp9uzr8rmk03vej67j869u5wu3mwy2qecnfnhqxjd46zn78aj2yn"
  },
  {
    "name": "3",
    "type": "local",
    "address": "cosmos18ds9jlaehg09vrkk9zmjr8qacqzmj73jhaxvj9",
    "pubkey": "cosmospub1addwnpepqgr0lahwuwsc5arzv2nghe0nuupug993vc3pyrtd4mugqrfsrth8yw7mcxh"
  },
  {
    "name": "m",
    "type": "multi",
    "address": "cosmos1337nkls6z6z2ygngzrawmp25l8qvywkw2xchwq",
    "pubkey": "cosmospub1ytql0csgqvfzd666axrjzqmn5q2ucztcyxw8hvlzen94ay05tegaerkug5pn3xn8wqdymt598ufzd666axrjzqsxllmwacap3f6xyc4x30jl8ecrcs2tze3zzgxkmthcsqxnqxhwwgfzd666axrjzqs2rlu3wz5gnslgpprszjr8r65n0d6y39q657th77eyvengtk3z0y6h2pnk"
  }
]

romeo4934 · 2020-07-23T15:41:21Z

Is it going to be released for launchpad as well? @alexanderbez

alexanderbez · 2020-07-23T15:52:54Z

Is it going to be released for launchpad as well? @alexanderbez

Yes, it has to be actually. This is why I'm doing it primarily.

support multisig bech32 address parsing

f91a2c6

assafmo requested review from aaronc, alessio, alexanderbez, clevinson, fedekunze, hschoenburg, jgimeno and nylira as code owners May 17, 2020 21:37

assafmo mentioned this pull request May 17, 2020

Support decoding with length limit as a param btcsuite/btcutil#168

Closed

alessio approved these changes May 17, 2020

View reviewed changes

Increase bech32 length limit to 250

cf00372

alexanderbez suggested changes May 18, 2020

View reviewed changes

Merge branch 'master' into master

3221bdd

rhuairahrighairidh mentioned this pull request May 25, 2020

v0.8 Migration Scripts Kava-Labs/kava#518

Merged

Increase max bech32 address size to 1023 (the max length that support…

d0ce172

…s the checksum)

Merge branch 'master' into master

1fd1773

assafmo requested review from alexanderbez and removed request for hschoenburg June 29, 2020 20:37

alexanderbez approved these changes Jun 30, 2020

View reviewed changes

Merge branch 'master' into master

e71db65

alexanderbez closed this Jul 23, 2020

alexanderbez reopened this Jul 23, 2020

assafmo added 4 commits July 23, 2020 19:27

Update go.mod

257181f

Update go.mod to point to the updated enigmampc/btcutil

758d19e

Merge branch 'master' of github.com:enigmampc/cosmos-sdk

152b899

Merge branch 'master' into master

67871b5

alexanderbez added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 23, 2020

mergify bot merged commit ee04dd7 into cosmos:master Jul 23, 2020

webmaster128 mentioned this pull request Aug 3, 2020

Ensure Bech32.encode throws for output length > 90 cosmos/cosmjs#335

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support multisig bech32 address decoding #6237

support multisig bech32 address decoding #6237

assafmo commented May 17, 2020 •

edited

Loading

auto-comment bot commented May 17, 2020

codecov bot commented May 17, 2020 •

edited

Loading

alessio left a comment

alexanderbez May 18, 2020

assafmo May 18, 2020

zmanian commented May 18, 2020

alexanderbez commented May 18, 2020 •

edited

Loading

zmanian commented May 18, 2020

assafmo commented May 19, 2020

zmanian commented May 19, 2020 •

edited

Loading

romeo4934 commented Jun 3, 2020

alexanderbez commented Jun 3, 2020

assafmo commented Jun 3, 2020 via email

alexanderbez commented Jun 3, 2020

romeo4934 commented Jun 3, 2020 •

edited

Loading

assafmo commented Jun 3, 2020

romeo4934 commented Jun 5, 2020 •

edited

Loading

assafmo commented Jun 28, 2020

alexanderbez commented Jun 29, 2020

assafmo commented Jun 29, 2020 •

edited

Loading

alessio commented Jun 29, 2020 •

edited

Loading

alexanderbez left a comment

onyb commented Jul 14, 2020

assafmo commented Jul 14, 2020 •

edited

Loading

romeo4934 commented Jul 23, 2020

alexanderbez commented Jul 23, 2020

support multisig bech32 address decoding #6237

support multisig bech32 address decoding #6237

Conversation

assafmo commented May 17, 2020 • edited Loading

Description

auto-comment bot commented May 17, 2020

codecov bot commented May 17, 2020 • edited Loading

Codecov Report

alessio left a comment

Choose a reason for hiding this comment

alexanderbez May 18, 2020

Choose a reason for hiding this comment

assafmo May 18, 2020

Choose a reason for hiding this comment

zmanian commented May 18, 2020

alexanderbez commented May 18, 2020 • edited Loading

zmanian commented May 18, 2020

assafmo commented May 19, 2020

zmanian commented May 19, 2020 • edited Loading

romeo4934 commented Jun 3, 2020

alexanderbez commented Jun 3, 2020

assafmo commented Jun 3, 2020 via email

alexanderbez commented Jun 3, 2020

romeo4934 commented Jun 3, 2020 • edited Loading

assafmo commented Jun 3, 2020

romeo4934 commented Jun 5, 2020 • edited Loading

assafmo commented Jun 28, 2020

alexanderbez commented Jun 29, 2020

assafmo commented Jun 29, 2020 • edited Loading

alessio commented Jun 29, 2020 • edited Loading

alexanderbez left a comment

Choose a reason for hiding this comment

onyb commented Jul 14, 2020

assafmo commented Jul 14, 2020 • edited Loading

romeo4934 commented Jul 23, 2020

alexanderbez commented Jul 23, 2020

assafmo commented May 17, 2020 •

edited

Loading

codecov bot commented May 17, 2020 •

edited

Loading

alexanderbez commented May 18, 2020 •

edited

Loading

zmanian commented May 19, 2020 •

edited

Loading

romeo4934 commented Jun 3, 2020 •

edited

Loading

romeo4934 commented Jun 5, 2020 •

edited

Loading

assafmo commented Jun 29, 2020 •

edited

Loading

alessio commented Jun 29, 2020 •

edited

Loading

assafmo commented Jul 14, 2020 •

edited

Loading