From 5099c15f73cd14aa060328fcd16588f46a469890 Mon Sep 17 00:00:00 2001 From: swelf19 <62722506+swelf19@users.noreply.github.com> Date: Tue, 15 Feb 2022 13:16:41 +0300 Subject: [PATCH] fix: non consistent keyring (#10844) ## 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) --- CHANGELOG.md | 1 + crypto/keyring/keyring.go | 27 +++++++++++++++++-------- crypto/keyring/keyring_test.go | 37 ++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b9815b66928..3d70f1886f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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. diff --git a/crypto/keyring/keyring.go b/crypto/keyring/keyring.go index 4011b2a2847..db4eb8fbb9c 100644 --- a/crypto/keyring/keyring.go +++ b/crypto/keyring/keyring.go @@ -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 diff --git a/crypto/keyring/keyring_test.go b/crypto/keyring/keyring_test.go index 3afd35676fb..ad3e109b74d 100644 --- a/crypto/keyring/keyring_test.go +++ b/crypto/keyring/keyring_test.go @@ -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)