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

decode multisig acc #882

Closed
hleb-albau opened this issue Sep 29, 2021 · 8 comments · Fixed by #884
Closed

decode multisig acc #882

hleb-albau opened this issue Sep 29, 2021 · 8 comments · Fixed by #884

Comments

@hleb-albau
Copy link
Contributor

function decodeBech32Pubkey can't decode multisig keys of next format bostrompub1ytql0csgqgfzd666axrjzqm4p586gn8vrm6958zd4c9lje8xeahzcf35mwsjw3gaxn7xnhe9eufzd666axrjzqcc9h5ms6x96eg23e4hxghnvwn7xwtuhgeupvg0lgppn7d6dkyaxc66dhat

seems multisig branch is missed

if (arrayContentStartsWith(data, pubkeyAminoPrefixSecp256k1)) {

@webmaster128
Copy link
Member

Your are right.

However, encoding a publich key as bech32 is a very bad idea. bech32 is only valid up to a length of 90 characters (for good reason). So the string you posted there is not valid as it is 156 characters long. I know there was a time when Cosmos SDK APIs encoded pubkeys as bech32. That was a bad idea, which was reverted as far as I know.

So the real question here is: why do you have bech32 encoded pubkeys and how to get rid of this encoding entirely?

@hleb-albau
Copy link
Contributor Author

hleb-albau commented Sep 29, 2021

Seems right now cosmos sdk works using bech32
I just generated osmo wallet osmopub1ytql0csgqgfzd666axrjzq5q3yaz9mz4322t7v98rvzx8c3uq2ag0u0zn5gcrgh93acgp9x0zqfzd666axrjzqa0cawzgvhal7sctp9w7jl6mnddl0z4w3v7x9et3lxudgw44s3wwc6mq25n using the latest osmo binaries with cosmos sdk 42.9

Also, multisig pubkey contains encoded pubkeys from all participants + threshold, thus it can't be like regular acc pubkey in terms of length

Also, there is code for construction multisig pubkey in the same file below

if (isMultisigThresholdPubkey(pubkey)) {

Default bench32 decoder by default has no limit, so it can decode correctly such addresses.

encoding.Bech32.decode("bostrompub1ytql0csgqgfzd666axrjzqm4p586gn8vrm6958zd4c9lje8xeahzcf35mwsjw3gaxn7xnhe9eufzd666axrjzqcc9h5ms6x96eg23e4hxghnvwn7xwtuhgeupvg0lgppn7d6dkyaxc66dhat")

So, is it all make sense? If so, I can create PR

@webmaster128
Copy link
Member

webmaster128 commented Sep 29, 2021

I just generated osmo wallet osmopub1ytql0csgqgfzd666axrjzq5q3yaz9mz4322t7v98rvzx8c3uq2ag0u0zn5gcrgh93acgp9x0zqfzd666axrjzqa0cawzgvhal7sctp9w7jl6mnddl0z4w3v7x9et3lxudgw44s3wwc6mq25n using the latest osmo binaries with cosmos sdk 42.9

Could you report this as a bug in Cosmos SDK please? You can refer to this spec:

"A Bech32 string is at most 90 characters long and consists of" https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki

Also, multisig pubkey contains encoded pubkeys from all participants + threshold, thus it can't be like regular acc pubkey in terms of length

Yes. This is why it should not be encoded as bech32.

Default bench32 decoder by default has no limit, so it can decode correctly such addresses.

It can, I know. This doesn't mean this should ever be done in practice.

Also, there is code for construction multisig pubkey in the same file below

Jupp. It's fair to encode this to raw binary, but when used in bech32 the trouble begins.

So, is it all make sense? If so, I can create PR

Mixed feelings. We are fixing other people's bugs by this.

@hleb-albau
Copy link
Contributor Author

Uh, seems it is already in v0.43 cosmos/cosmos-sdk#7477

@webmaster128
Copy link
Member

Haha, our test data is already full of this for the other direction: https://github.com/cosmos/cosmjs/blob/v0.26.0/packages/amino/src/testutils.spec.ts#L35-L76

I give up. Feel free to create a PR using that test data. See how it is used here in the other direction: https://github.com/cosmos/cosmjs/blob/v0.26.0/packages/amino/src/encoding.spec.ts#L149-L161

@webmaster128
Copy link
Member

Please note #883

hleb-albau added a commit to hleb-albau/cosmjs that referenced this issue Sep 29, 2021
hleb-albau added a commit to hleb-albau/cosmjs that referenced this issue Sep 29, 2021
hleb-albau added a commit to hleb-albau/cosmjs that referenced this issue Sep 29, 2021
hleb-albau added a commit to hleb-albau/cosmjs that referenced this issue Sep 30, 2021
hleb-albau added a commit to hleb-albau/cosmjs that referenced this issue Sep 30, 2021
webmaster128 added a commit that referenced this issue Sep 30, 2021
@webmaster128
Copy link
Member

Done on #884 with some minor additions in #886. Thanks!

@webmaster128
Copy link
Member

Released as part of 0.26.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants