gnolang / gno

Gno: An interpreted, stack-based Go virtual machine to build succinct and composable apps + Gno.land: a blockchain for timeless code and fair open-source
https://gno.land/
Other
842 stars 343 forks source link

gnokey add can't add pubkeys #241

Closed jaekwon closed 2 years ago

jaekwon commented 2 years ago

i think it might be failing due to a bech32 decoding error. it could be that the prefix is set to "g" and length assumed to be shorter than a "gpub" prefixed bech32 pubkey. i suppose we should check the prefix and then check the length accordingly, in some places.

piux2 commented 2 years ago

Here is the problem.

1) gno's amino marshaled pubkey byte length is 58 2) converted base256 pubkey byte length is 93 3) encoded bech32 pubkey byte length is 104

however, the github.com/btcsuite/btcutil v1.0.2 require the bech32 max length to be 90

When we add gno pubkey back to the local keybase, btcutil.bech32 tried to decode it and threw an error. "invalid bech32 string length 104"

Solution:

1) install the latest version of btcutil github.com/btcsuite/btcd/btcutil v1.1.1

2) call a new method bech32.DecodeNoLimit(bech), which does not limit the pubkey bech32 length to 90.

https://github.com/gnolang/gno/blob/1b7dad1732eb71a794e32ebbcd329e6ac3f1d850/pkgs/bech32/bech32.go#L25

https://github.com/btcsuite/btcd/blob/74e9690d0e402446c3c366d595123e7219f8cce7/btcutil/bech32/bech32.go#L264

I will make a pull request, if this solution is accepted.

piux2 commented 2 years ago

Trade-Off:

Since Bech32 is designed for max length 90 bytes/character with 6 checksum characters to detect and correct 4 errors with 0 faults. When we extend the length to 104, it will slightly decrease the success rate of error detection.

However, the chances of failure could be tiny. According to the BIP173 the current code selection performs well up to 1023 characters.

jaekwon commented 2 years ago

that sounds great, yes please.

moul commented 2 years ago

Fixed by #264