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

simd keys list #11939

Closed
4 tasks
yaruwangway opened this issue May 11, 2022 · 22 comments
Closed
4 tasks

simd keys list #11939

yaruwangway opened this issue May 11, 2022 · 22 comments
Assignees
Labels

Comments

@yaruwangway
Copy link
Contributor

yaruwangway commented May 11, 2022

Summary of Bug

when build simd from v0.45, when do simd keys list , i get
Error: Bytes left over in UnmarshalBinaryLengthPrefixed, should read 10 more bytes but have 163

it seems some of the keys cannot be parsed by simd 0.45, maybe some of them are added by other version of simd.

Version

Steps to Reproduce


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez
Copy link
Contributor

alexanderbez commented May 11, 2022

What keyring backend? Are these new keys or imported?

@glnro
Copy link
Contributor

glnro commented May 11, 2022

I can confirm running the following

$ ./simd version
0.45.0

$ ./simd init test --chain-id test --home ~/.simd1

$ ./simd keys add test-2 --keyring-backend test
- name: test-2
  type: local
  address: cosmos1h526wpl0z55tztts2qpju0tr8lmu23vnsa98ww
  pubkey: '{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"AjyCBWrIpJX/5uPkFSwdyqZInNmAgwq2p6RCrdyBvDXT"}'
  mnemonic: ""

$ ./simd keys list --keyring-backend test
Error: Bytes left over in UnmarshalBinaryLengthPrefixed, should read 10 more bytes but have 164

@yaruwangway
Copy link
Contributor Author

@alexanderbez , local key, keyring-backend test, this is an issue from testing tendermint key-migration by cosmovisor pre-upgrade.

i did not change my key or anything, it was working with simd v0.45 to show the key, it suddenly does not work , i wonder if it is because i did tendermint key migration.

does tendermint key migration also migration local account address key ?

@yaruwangway
Copy link
Contributor Author

@glnro can you try simd v0.46, to list the keys.

@glnro
Copy link
Contributor

glnro commented May 11, 2022

I had no problem doing the same with 0.46.0-beta2

@yaruwangway
Copy link
Contributor Author

@alexanderbez , local key, keyring-backend test, this is an issue from testing tendermint key-migration by cosmovisor pre-upgrade.

i did not change my key or anything, it was working with simd v0.45 to show the key, it suddenly does not work , i wonder if it is because i did tendermint key migration.

does tendermint key migration also migration local account address key ?

confirmed from tendermint team, this is not related to tendermint key migration !

@glnro
Copy link
Contributor

glnro commented May 11, 2022

Fwiw no problem running this on v7.0.2 of gaia, just simapp

@tac0turtle
Copy link
Member

@AmauryM I saw you are looking at the crypto package. Did you come across anything special?

@glnro
Copy link
Contributor

glnro commented May 12, 2022

Update, when working off of sdk v0.46.0-beta2 in our rho base branch, I encounter the following when trying to set up a local chain:

$ gaiad keys list --keyring-backend test
migrate err: "unable to unmarshal item.Data, err: Bytes left over in UnmarshalBinaryLengthPrefixed, should read 10 more bytes but have 166"Error: no registered implementations of type types.PubKey

@glnro
Copy link
Contributor

glnro commented May 12, 2022

@alexanderbez btw these are all new keys

@amaury1093
Copy link
Contributor

Hey @glnro and @yaruwangway, I can take a look at this issue.

I can think of 3 scenarios:

  1. Create a key using v0.45, and simd keys list using v0.45 errors (Repro: simd keys list #11939 (comment))
  2. Create a key using v0.46, and simd keys list using v0.46 errors (Repro: simd keys list #11939 (comment))
  3. Create a key using v0.45, and simd keys list using v0.46 errors (no repro yet)

Does this seem correct?

@yaruwangway
Copy link
Contributor Author

yaruwangway commented May 12, 2022

hi, a bit more info:
create a key by simd v0.45 (lets call it 045key), create a key by simd v0.46 (lets call it 046key),

  • simd v0.45 keys show 045key works, simd v0.46 keys show 045key works,
  • simd v0.45 keys show 046key fails, simd v0.46 keys show 046key works,
  • simd v0.45 keys list fail, simd v0.46 keys list works.

@yaruwangway
Copy link
Contributor Author

so it seems the decoding of keys by v0.45 is not compatible with other version keys.

@amaury1093
Copy link
Contributor

amaury1093 commented May 12, 2022

Hey, unfortunately I couldn't reproduce any of the errors above :(. Here's the full bash script I used: https://gist.github.com/amaurym/e6dbe3363b1b2fbd83ff0d415a52127b. I tested with:

  • v0.45.4
  • v0.46.0-beta2

@yaruwangway @glnro could you provide a similar bash script, where you could reproduce all errors you found? Ideally using simapp (not gaiad). Thanks!

@yaruwangway
Copy link
Contributor Author

yaruwangway commented May 12, 2022

@AmauryM , the script you used does not include $v046_NODE keys add and with $v045_NODE keys list ? use 0.45 list a key created by 0.46

@amaury1093
Copy link
Contributor

amaury1093 commented May 12, 2022

@yaruwangway In v0.46 we migrated keyriing from amino to protobuf. Doing what you say would require writing additional logic to migrate from proto back to amino, which I would like to avoid (amino is being deprecated).

What's your use case to list v046 keys using a v045 node?

@glnro
Copy link
Contributor

glnro commented May 12, 2022

@AmauryM I think one of the issues I'm running into for example is I've created keys on my local machine while testing with both binaries as we've had to go back and forth with versions during our work. Still trying to figure out how this is affecting our e2e test and I'm writing another test to confirm what you posted above.

@yaruwangway
Copy link
Contributor Author

list v046 keys using a v045 node:
Error: Bytes left over in UnmarshalBinaryLengthPrefixed, should read 10 more bytes but have 164

list v045 keys using a v046 node: works!

list v045 keys using a v045 node: works!
list v046 keys using a v046 node: works!

@yaruwangway
Copy link
Contributor Author

yaruwangway commented May 12, 2022

are the above cases expected, due to migrated keyring from amino to protobuf ?

@amaury1093
Copy link
Contributor

The case "list v046 keys using a v045 node" is not expected, and should fail. Users shouldn't be able to show newer keys with older software.

@yaruwangway
Copy link
Contributor Author

ok, thank you!

This was referenced May 12, 2022
mergify bot pushed a commit that referenced this issue May 15, 2022
## Description

ref: #11362 

I did **NOT** review the following folders, as they contain cryptography which I don't think I'm competent enough to give a useful review:
- [ ] `crypto/xsalsa20symmetric` (new in v046, ported from TM i think)
- [ ] `crypto/keys/secp256k1` (some new stuff in v046 too)

Also performed some manual tests as part of #11939:

  - [x] Create keys on v0.45, make sure they still work in v0.46 #11939 (comment)
  - [x] Create new keys in v0.46 #11939 (comment)
  - [x] `--multisig` flag works with an address that's not in the keyring (see [repro](#9553))



---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
@faddat
Copy link
Contributor

faddat commented Apr 5, 2023

We seem to be hitting exactly this:

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

No branches or pull requests

6 participants