Skip to content

Commit

Permalink
fix: non consistent keyring (#10844)
Browse files Browse the repository at this point in the history
## Description

Normally keyring module creates two record for each public key created in keyringdb.
The first one with an address as a key witch contains only name of the second key, wich actually contains a public key
![Screenshot_20211227_173827](https://user-images.githubusercontent.com/62722506/147482783-ffd495e6-7f16-4497-b1a3-50e9db90e1e8.png)

But a couple of times we have faced an issue, when the first record exists, and the second for some reason does not. 
![Screenshot_20211227_173846](https://user-images.githubusercontent.com/62722506/147482893-ffe159fd-9eeb-493f-a9db-75c7ccedbf80.png)

In such case you are unable to import public key due to error
```shell
$ go run ./cmd/terrad/ keys --keyring-backend kwallet add swelf --pubkey '{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"Ap1W7ww/FaZVpAd487QUVXh7Nmxk4FlREr5IGPuzEnJu"}'
Error: public key already exists in keybase
```
in the same time terrad cli do not see any keys in the keyring
```shell
$ go run ./cmd/terrad/ keys --keyring-backend kwallet list
[]
```
The error occurs when the record with address still exists in the keyring db.

I would like to resolve the error.
I see at least three different ways to do it.
1) Informing the user about situation and recreate public key
```shell
$ go run ./cmd/terrad/ keys --keyring-backend kwallet add swelf --pubkey '{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"Ap1W7ww/FaZVpAd487QUVXh7Nmxk4FlREr5IGPuzEnJu"}'
**address "7cc4633deb18c0531b382a50275ad94e05f84580" exists but pubkey itself does not
recreating pubkey record**

- name: swelf
  type: offline
  address: terra10nzxx00trrq9xxec9fgzwkkefczls3vqkpkjl4
  pubkey: '{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"Ap1W7ww/FaZVpAd487QUVXh7Nmxk4FlREr5IGPuzEnJu"}'
  mnemonic: ""
```
with notifying user about an issue
2) Asking the user to confirm procedure of restoring public key
3) Just informing user about an issue and do nothing.

I prefer the first way, i do not see a reason when user do not want to fix an issue.

---

### 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...

- [x] 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
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/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/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [x] 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)
  • Loading branch information
swelf19 authored Feb 15, 2022
1 parent e7066c4 commit 5099c15
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 8 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Bug Fixes

* [\#10844](https://github.com/cosmos/cosmos-sdk/pull/10844) Automatic recovering non-consistent keyring storage during public key import
* (store) [\#11117](https://github.com/cosmos/cosmos-sdk/pull/11117) Fix data race in store trace component
* (cli) [\#11065](https://github.com/cosmos/cosmos-sdk/pull/11065) Ensure the `tendermint-validator-set` query command respects the `-o` output flag.
* (grpc) [\#10985](https://github.com/cosmos/cosmos-sdk/pull/10992) The `/cosmos/tx/v1beta1/txs/{hash}` endpoint returns a 404 when a tx does not exist.
Expand Down
27 changes: 19 additions & 8 deletions crypto/keyring/keyring.go
Original file line number Diff line number Diff line change
Expand Up @@ -810,18 +810,29 @@ func (ks keystore) writeRecord(k *Record) error {

// existsInDb returns (true, nil) if either addr or name exist is in keystore DB.
// On the other hand, it returns (false, error) if Get method returns error different from keyring.ErrKeyNotFound
// In case of inconsistent keyring, it recovers it automatically.
func (ks keystore) existsInDb(addr sdk.Address, name string) (bool, error) {

if _, err := ks.db.Get(addrHexKeyAsString(addr)); err == nil {
return true, nil // address lookup succeeds - info exists
} else if err != keyring.ErrKeyNotFound {
return false, err // received unexpected error - returns error
_, errAddr := ks.db.Get(addrHexKeyAsString(addr))
if errAddr != nil && !errors.Is(errAddr, keyring.ErrKeyNotFound) {
return false, errAddr
}

if _, err := ks.db.Get(name); err == nil {
_, errInfo := ks.db.Get(infoKey(name))
if errInfo == nil {
return true, nil // uid lookup succeeds - info exists
} else if err != keyring.ErrKeyNotFound {
return false, err // received unexpected error - returns
} else if !errors.Is(errInfo, keyring.ErrKeyNotFound) {
return false, errInfo // received unexpected error - returns
}

// looking for an issue, record with meta (getByAddress) exists, but record with public key itself does not
if errAddr == nil && errors.Is(errInfo, keyring.ErrKeyNotFound) {
fmt.Fprintf(os.Stderr, "address \"%s\" exists but pubkey itself does not\n", hex.EncodeToString(addr.Bytes()))
fmt.Fprintln(os.Stderr, "recreating pubkey record")
err := ks.db.Remove(addrHexKeyAsString(addr))
if err != nil {
return true, err
}
return false, nil
}

// both lookups failed, info does not exist
Expand Down
37 changes: 37 additions & 0 deletions crypto/keyring/keyring_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1064,6 +1064,43 @@ func TestAltKeyring_SaveOfflineKey(t *testing.T) {
require.Len(t, list, 1)
}

func TestNonConsistentKeyring_SavePubKey(t *testing.T) {
cdc := getCodec()
kr, err := New(t.Name(), BackendTest, t.TempDir(), nil, cdc)
require.NoError(t, err)

list, err := kr.List()
require.NoError(t, err)
require.Empty(t, list)

key := someKey
priv := ed25519.GenPrivKey()
pub := priv.PubKey()

k, err := kr.SaveOfflineKey(key, pub)
require.Nil(t, err)

// broken keyring state test
unsafeKr, ok := kr.(keystore)
require.True(t, ok)
// we lost public key for some reason, but still have an address record
unsafeKr.db.Remove(infoKey(key))
list, err = kr.List()
require.NoError(t, err)
require.Equal(t, 0, len(list))

k, err = kr.SaveOfflineKey(key, pub)
require.Nil(t, err)
pubKey, err := k.GetPubKey()
require.NoError(t, err)
require.Equal(t, pub, pubKey)
require.Equal(t, key, k.Name)

list, err = kr.List()
require.NoError(t, err)
require.Equal(t, 1, len(list))
}

func TestAltKeyring_SaveMultisig(t *testing.T) {
cdc := getCodec()
kr, err := New(t.Name(), BackendTest, t.TempDir(), nil, cdc)
Expand Down

0 comments on commit 5099c15

Please sign in to comment.