From c7c8bbddcb08f3f657d9cf5959ebb20d56173e44 Mon Sep 17 00:00:00 2001 From: Alexander Bezobchuk Date: Thu, 23 May 2019 12:44:18 -0400 Subject: [PATCH 1/8] Merge PR #4400: Release v0.34.5 - RC1 * Cherry Pick PR #4345: Upgrade ledger-cosmos-go * Cherry Pick PR #4336: Fix AppendTags usage error * Update modules * Cherry Pick PR #4265: CacheKVStore keep sorted items * Cherry Pick #4227: Adding support for Ledger Cosmos App v1.5 * Cherry Pick #4395: Improve sig verification error message * Cherry Pick PR #4140: Fix Failed Simulation Seeds --- CHANGELOG.md | 23 +++++- client/keys/add.go | 5 +- client/keys/show.go | 4 +- cmd/gaia/app/sim_test.go | 32 ++++---- crypto/keys/keybase.go | 7 +- crypto/keys/keybase_test.go | 4 +- crypto/keys/lazy_keybase.go | 4 +- crypto/keys/types.go | 2 +- crypto/ledger_mock.go | 33 +++++++- crypto/ledger_secp256k1.go | 104 ++++++++++++++++++------ crypto/ledger_test.go | 127 +++++++++++++++++++++++++++--- go.mod | 10 +-- go.sum | 18 +++-- store/cachekv/memiterator.go | 45 ++++++++--- store/cachekv/store.go | 73 +++++++++++------ store/cachekv/store_bench_test.go | 46 +++++++++++ store/cachekv/store_test.go | 4 +- x/auth/ante.go | 2 +- x/mint/client/module_client.go | 6 +- x/simulation/params.go | 2 +- x/simulation/rand_util.go | 5 ++ x/staking/handler.go | 6 +- 22 files changed, 439 insertions(+), 123 deletions(-) create mode 100644 store/cachekv/store_bench_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index d5a87988534e..ad0800688532 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,26 @@ # Changelog +## 0.34.5 + +### Bug Fixes + +#### SDK + +* [\#4273](https://github.com/cosmos/cosmos-sdk/issues/4273) Fix usage of `AppendTags` in x/staking/handler.go + +### Improvements + +### SDK + +* [\#2286](https://github.com/cosmos/cosmos-sdk/issues/2286) Improve performance of `CacheKVStore` iterator. +* [\#3655](https://github.com/cosmos/cosmos-sdk/issues/3655) Improve signature verification failure error message. + +#### Gaia CLI + +* [\#4227](https://github.com/cosmos/cosmos-sdk/issues/4227) Support for Ledger App v1.5. +* [#4345](https://github.com/cosmos/cosmos-sdk/pull/4345) Update `ledger-cosmos-go` +to v0.10.3. + ## 0.34.4 ### Bug Fixes @@ -7,7 +28,7 @@ #### SDK * [#4234](https://github.com/cosmos/cosmos-sdk/pull/4234) Allow `tx send --generate-only` to -actually work offline. +actually work offline. #### Gaia diff --git a/client/keys/add.go b/client/keys/add.go index f341af125b9a..2aa439405620 100644 --- a/client/keys/add.go +++ b/client/keys/add.go @@ -171,9 +171,10 @@ func runAddCmd(_ *cobra.Command, args []string) error { account := uint32(viper.GetInt(flagAccount)) index := uint32(viper.GetInt(flagIndex)) - // If we're using ledger, only thing we need is the path. So generate key and we're done. + // If we're using ledger, only thing we need is the path and the bech32 prefix. if viper.GetBool(client.FlagUseLedger) { - info, err := kb.CreateLedger(name, keys.Secp256k1, account, index) + bech32PrefixAccAddr := sdk.GetConfig().GetBech32AccountAddrPrefix() + info, err := kb.CreateLedger(name, keys.Secp256k1, bech32PrefixAccAddr, account, index) if err != nil { return err } diff --git a/client/keys/show.go b/client/keys/show.go index eaa800e11fbe..8f87482e09d8 100644 --- a/client/keys/show.go +++ b/client/keys/show.go @@ -24,7 +24,7 @@ const ( FlagPublicKey = "pubkey" // FlagBechPrefix defines a desired Bech32 prefix encoding for a key. FlagBechPrefix = "bech" - // FlagBechPrefix defines a desired Bech32 prefix encoding for a key. + // FlagDevice indicates that the information should be shown in the device FlagDevice = "device" flagMultiSigThreshold = "multisig-threshold" @@ -47,7 +47,7 @@ consisting of all the keys provided by name and multisig threshold.`, cmd.Flags().String(FlagBechPrefix, sdk.PrefixAccount, "The Bech32 prefix encoding for a key (acc|val|cons)") cmd.Flags().BoolP(FlagAddress, "a", false, "Output the address only (overrides --output)") cmd.Flags().BoolP(FlagPublicKey, "p", false, "Output the public key only (overrides --output)") - cmd.Flags().BoolP(FlagDevice, "d", false, "Output the address in the device") + cmd.Flags().BoolP(FlagDevice, "d", false, "Output the address in a ledger device") cmd.Flags().Uint(flagMultiSigThreshold, 1, "K out of N required signatures") cmd.Flags().BoolP(flagShowMultiSig, "m", false, "Output multisig pubkey constituents, threshold, and weights") cmd.Flags().Bool(client.FlagIndentResponse, false, "Add indent to JSON response") diff --git a/cmd/gaia/app/sim_test.go b/cmd/gaia/app/sim_test.go index 80a268ee28d3..28e94e4d7f45 100644 --- a/cmd/gaia/app/sim_test.go +++ b/cmd/gaia/app/sim_test.go @@ -119,7 +119,7 @@ func appStateRandomizedFn(r *rand.Rand, accs []simulation.Account, genesisTimest if int64(i) > numInitiallyBonded && r.Intn(100) < 50 { var ( vacc auth.VestingAccount - endTime int + endTime int64 ) startTime := genesisTimestamp.Unix() @@ -127,15 +127,19 @@ func appStateRandomizedFn(r *rand.Rand, accs []simulation.Account, genesisTimest // Allow for some vesting accounts to vest very quickly while others very // slowly. if r.Intn(100) < 50 { - endTime = randIntBetween(r, int(startTime), int(startTime+(60*60*24*30))) + endTime = int64(simulation.RandIntBetween(r, int(startTime), int(startTime+(60*60*24*30)))) } else { - endTime = randIntBetween(r, int(startTime), int(startTime+(60*60*12))) + endTime = int64(simulation.RandIntBetween(r, int(startTime), int(startTime+(60*60*12)))) + } + + if startTime == endTime { + endTime += 1 } if r.Intn(100) < 50 { - vacc = auth.NewContinuousVestingAccount(&bacc, startTime, int64(endTime)) + vacc = auth.NewContinuousVestingAccount(&bacc, startTime, endTime) } else { - vacc = auth.NewDelayedVestingAccount(&bacc, int64(endTime)) + vacc = auth.NewDelayedVestingAccount(&bacc, endTime) } gacc = NewGenesisAccountI(vacc) @@ -148,11 +152,11 @@ func appStateRandomizedFn(r *rand.Rand, accs []simulation.Account, genesisTimest authGenesis := auth.GenesisState{ Params: auth.Params{ - MaxMemoCharacters: uint64(randIntBetween(r, 100, 200)), + MaxMemoCharacters: uint64(simulation.RandIntBetween(r, 100, 200)), TxSigLimit: uint64(r.Intn(7) + 1), - TxSizeCostPerByte: uint64(randIntBetween(r, 5, 15)), - SigVerifyCostED25519: uint64(randIntBetween(r, 500, 1000)), - SigVerifyCostSecp256k1: uint64(randIntBetween(r, 500, 1000)), + TxSizeCostPerByte: uint64(simulation.RandIntBetween(r, 5, 15)), + SigVerifyCostED25519: uint64(simulation.RandIntBetween(r, 500, 1000)), + SigVerifyCostSecp256k1: uint64(simulation.RandIntBetween(r, 500, 1000)), }, } fmt.Printf("Selected randomly generated auth parameters:\n\t%+v\n", authGenesis) @@ -182,7 +186,7 @@ func appStateRandomizedFn(r *rand.Rand, accs []simulation.Account, genesisTimest stakingGenesis := staking.GenesisState{ Pool: staking.InitialPool(), Params: staking.Params{ - UnbondingTime: time.Duration(randIntBetween(r, 60, 60*60*24*3*2)) * time.Second, + UnbondingTime: time.Duration(simulation.RandIntBetween(r, 60, 60*60*24*3*2)) * time.Second, MaxValidators: uint16(r.Intn(250) + 1), BondDenom: sdk.DefaultBondDenom, }, @@ -192,9 +196,9 @@ func appStateRandomizedFn(r *rand.Rand, accs []simulation.Account, genesisTimest slashingGenesis := slashing.GenesisState{ Params: slashing.Params{ MaxEvidenceAge: stakingGenesis.Params.UnbondingTime, - SignedBlocksWindow: int64(randIntBetween(r, 10, 1000)), + SignedBlocksWindow: int64(simulation.RandIntBetween(r, 10, 1000)), MinSignedPerWindow: sdk.NewDecWithPrec(int64(r.Intn(10)), 1), - DowntimeJailDuration: time.Duration(randIntBetween(r, 60, 60*60*24)) * time.Second, + DowntimeJailDuration: time.Duration(simulation.RandIntBetween(r, 60, 60*60*24)) * time.Second, SlashFractionDoubleSign: sdk.NewDec(1).Quo(sdk.NewDec(int64(r.Intn(50) + 1))), SlashFractionDowntime: sdk.NewDec(1).Quo(sdk.NewDec(int64(r.Intn(200) + 1))), }, @@ -269,10 +273,6 @@ func appStateFn(r *rand.Rand, accs []simulation.Account, genesisTimestamp time.T return appStateRandomizedFn(r, accs, genesisTimestamp) } -func randIntBetween(r *rand.Rand, min, max int) int { - return r.Intn(max-min) + min -} - func testAndRunTxs(app *GaiaApp) []simulation.WeightedOperation { return []simulation.WeightedOperation{ {5, authsim.SimulateDeductFee(app.accountKeeper, app.feeCollectionKeeper)}, diff --git a/crypto/keys/keybase.go b/crypto/keys/keybase.go index e4ff5651c640..a72b43238ae7 100644 --- a/crypto/keys/keybase.go +++ b/crypto/keys/keybase.go @@ -137,18 +137,19 @@ func (kb dbKeybase) Derive(name, mnemonic, bip39Passphrase, encryptPasswd string // CreateLedger creates a new locally-stored reference to a Ledger keypair // It returns the created key info and an error if the Ledger could not be queried -func (kb dbKeybase) CreateLedger(name string, algo SigningAlgo, account uint32, index uint32) (Info, error) { +func (kb dbKeybase) CreateLedger(name string, algo SigningAlgo, hrp string, account, index uint32) (Info, error) { if algo != Secp256k1 { return nil, ErrUnsupportedSigningAlgo } hdPath := hd.NewFundraiserParams(account, index) - priv, err := crypto.NewPrivKeyLedgerSecp256k1(*hdPath) + priv, _, err := crypto.NewPrivKeyLedgerSecp256k1(*hdPath, hrp) if err != nil { return nil, err } pub := priv.PubKey() + // Note: Once Cosmos App v1.3.1 is compulsory, it could be possible to check that pubkey and addr match return kb.writeLedgerKey(name, pub, *hdPath), nil } @@ -246,7 +247,7 @@ func (kb dbKeybase) Sign(name, passphrase string, msg []byte) (sig []byte, pub t case ledgerInfo: linfo := info.(ledgerInfo) - priv, err = crypto.NewPrivKeyLedgerSecp256k1(linfo.Path) + priv, err = crypto.NewPrivKeyLedgerSecp256k1Unsafe(linfo.Path) if err != nil { return } diff --git a/crypto/keys/keybase_test.go b/crypto/keys/keybase_test.go index 09829e1dd833..abdd93a879f9 100644 --- a/crypto/keys/keybase_test.go +++ b/crypto/keys/keybase_test.go @@ -37,7 +37,7 @@ func TestCreateAccountInvalidMnemonic(t *testing.T) { func TestCreateLedgerUnsupportedAlgo(t *testing.T) { kb := NewInMemory() - _, err := kb.CreateLedger("some_account", Ed25519, 0, 1) + _, err := kb.CreateLedger("some_account", Ed25519, "cosmos", 0, 1) assert.Error(t, err) assert.Equal(t, "unsupported signing algo: only secp256k1 is supported", err.Error()) } @@ -49,7 +49,7 @@ func TestCreateLedger(t *testing.T) { // test_cover does not compile some dependencies so ledger is disabled // test_unit may add a ledger mock // both cases are acceptable - ledger, err := kb.CreateLedger("some_account", Secp256k1, 3, 1) + ledger, err := kb.CreateLedger("some_account", Secp256k1, "cosmos", 3, 1) if err != nil { assert.Error(t, err) diff --git a/crypto/keys/lazy_keybase.go b/crypto/keys/lazy_keybase.go index 6f0b68523481..6922bd152f47 100644 --- a/crypto/keys/lazy_keybase.go +++ b/crypto/keys/lazy_keybase.go @@ -106,14 +106,14 @@ func (lkb lazyKeybase) Derive(name, mnemonic, bip39Passwd, encryptPasswd string, return newDbKeybase(db).Derive(name, mnemonic, bip39Passwd, encryptPasswd, params) } -func (lkb lazyKeybase) CreateLedger(name string, algo SigningAlgo, account uint32, index uint32) (info Info, err error) { +func (lkb lazyKeybase) CreateLedger(name string, algo SigningAlgo, hrp string, account, index uint32) (info Info, err error) { db, err := sdk.NewLevelDB(lkb.name, lkb.dir) if err != nil { return nil, err } defer db.Close() - return newDbKeybase(db).CreateLedger(name, algo, account, index) + return newDbKeybase(db).CreateLedger(name, algo, hrp, account, index) } func (lkb lazyKeybase) CreateOffline(name string, pubkey crypto.PubKey) (info Info, err error) { diff --git a/crypto/keys/types.go b/crypto/keys/types.go index 1459e1ae027f..5389f93686ad 100644 --- a/crypto/keys/types.go +++ b/crypto/keys/types.go @@ -35,7 +35,7 @@ type Keybase interface { Derive(name, mnemonic, bip39Passwd, encryptPasswd string, params hd.BIP44Params) (Info, error) // CreateLedger creates, stores, and returns a new Ledger key reference - CreateLedger(name string, algo SigningAlgo, account uint32, index uint32) (info Info, err error) + CreateLedger(name string, algo SigningAlgo, hrp string, account, index uint32) (info Info, err error) // CreateOffline creates, stores, and returns a new offline key reference CreateOffline(name string, pubkey crypto.PubKey) (info Info, err error) diff --git a/crypto/ledger_mock.go b/crypto/ledger_mock.go index 75e6941da12a..159d9c2d3796 100644 --- a/crypto/ledger_mock.go +++ b/crypto/ledger_mock.go @@ -4,14 +4,17 @@ package crypto import ( "fmt" - "github.com/btcsuite/btcd/btcec" + "github.com/pkg/errors" + "github.com/cosmos/cosmos-sdk/crypto/keys/hd" "github.com/cosmos/cosmos-sdk/tests" - bip39 "github.com/cosmos/go-bip39" - "github.com/pkg/errors" + "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/go-bip39" + secp256k1 "github.com/tendermint/btcd/btcec" "github.com/tendermint/tendermint/crypto" + tmsecp256k1 "github.com/tendermint/tendermint/crypto/secp256k1" ) // If ledger support (build tag) has been enabled, which implies a CGO dependency, @@ -30,6 +33,8 @@ func (mock LedgerSECP256K1Mock) Close() error { return nil } +// GetPublicKeySECP256K1 mocks a ledger device +// as per the original API, it returns an uncompressed key func (mock LedgerSECP256K1Mock) GetPublicKeySECP256K1(derivationPath []uint32) ([]byte, error) { if derivationPath[0] != 44 { return nil, errors.New("Invalid derivation path") @@ -55,6 +60,28 @@ func (mock LedgerSECP256K1Mock) GetPublicKeySECP256K1(derivationPath []uint32) ( return pubkeyObject.SerializeUncompressed(), nil } +// GetAddressPubKeySECP256K1 mocks a ledger device +// as per the original API, it returns a compressed key and a bech32 address +func (mock LedgerSECP256K1Mock) GetAddressPubKeySECP256K1(derivationPath []uint32, hrp string) ([]byte, string, error) { + pk, err := mock.GetPublicKeySECP256K1(derivationPath) + if err != nil { + return nil, "", err + } + + // re-serialize in the 33-byte compressed format + cmp, err := btcec.ParsePubKey(pk[:], btcec.S256()) + if err != nil { + return nil, "", fmt.Errorf("error parsing public key: %v", err) + } + + var compressedPublicKey tmsecp256k1.PubKeySecp256k1 + copy(compressedPublicKey[:], cmp.SerializeCompressed()) + + // Generate the bech32 addr using existing tmcrypto/etc. + addr := types.AccAddress(compressedPublicKey.Address()).String() + return pk, addr, err +} + func (mock LedgerSECP256K1Mock) SignSECP256K1(derivationPath []uint32, message []byte) ([]byte, error) { path := hd.NewParams(derivationPath[0], derivationPath[1], derivationPath[2], derivationPath[3] != 0, derivationPath[4]) seed, err := bip39.NewSeedWithErrorChecking(tests.TestMnemonic, "") diff --git a/crypto/ledger_secp256k1.go b/crypto/ledger_secp256k1.go index fae0c1569b96..b0c573f113dd 100644 --- a/crypto/ledger_secp256k1.go +++ b/crypto/ledger_secp256k1.go @@ -5,11 +5,11 @@ import ( "os" "github.com/btcsuite/btcd/btcec" + "github.com/pkg/errors" + "github.com/cosmos/cosmos-sdk/crypto/keys/hd" "github.com/cosmos/cosmos-sdk/types" - "github.com/pkg/errors" - tmbtcec "github.com/tendermint/btcd/btcec" tmcrypto "github.com/tendermint/tendermint/crypto" tmsecp256k1 "github.com/tendermint/tendermint/crypto/secp256k1" @@ -27,13 +27,15 @@ type ( // dependencies when Ledger support is potentially not enabled. discoverLedgerFn func() (LedgerSECP256K1, error) - // LedgerSECP256K1 reflects an interface a Ledger API must implement for - // the SECP256K1 scheme. + // LedgerSECP256K1 reflects an interface a Ledger API must implement for SECP256K1 LedgerSECP256K1 interface { Close() error + // Returns an uncompressed pubkey GetPublicKeySECP256K1([]uint32) ([]byte, error) + // Returns a compressed pubkey and bech32 address (requires user confirmation) + GetAddressPubKeySECP256K1([]uint32, string) ([]byte, string, error) + // Signs a message (requires user confirmation) SignSECP256K1([]uint32, []byte) ([]byte, error) - ShowAddressSECP256K1([]uint32, string) error } // PrivKeyLedgerSecp256k1 implements PrivKey, calling the ledger nano we @@ -47,16 +49,19 @@ type ( } ) -// NewPrivKeyLedgerSecp256k1 will generate a new key and store the public key -// for later use. -func NewPrivKeyLedgerSecp256k1(path hd.BIP44Params) (tmcrypto.PrivKey, error) { +// NewPrivKeyLedgerSecp256k1Unsafe will generate a new key and store the public key for later use. +// +// This function is marked as unsafe as it will retrieve a pubkey without user verification. +// It can only be used to verify a pubkey but never to create new accounts/keys. In that case, +// please refer to NewPrivKeyLedgerSecp256k1 +func NewPrivKeyLedgerSecp256k1Unsafe(path hd.BIP44Params) (tmcrypto.PrivKey, error) { device, err := getLedgerDevice() if err != nil { return nil, err } defer warnIfErrors(device.Close) - pubKey, err := getPubKey(device, path) + pubKey, err := getPubKeyUnsafe(device, path) if err != nil { return nil, err } @@ -64,24 +69,21 @@ func NewPrivKeyLedgerSecp256k1(path hd.BIP44Params) (tmcrypto.PrivKey, error) { return PrivKeyLedgerSecp256k1{pubKey, path}, nil } -// LedgerShowAddress triggers a ledger device to show the corresponding address. -func LedgerShowAddress(path hd.BIP44Params, expectedPubKey tmcrypto.PubKey) error { +// NewPrivKeyLedgerSecp256k1 will generate a new key and store the public key for later use. +// The request will require user confirmation and will show account and index in the device +func NewPrivKeyLedgerSecp256k1(path hd.BIP44Params, hrp string) (tmcrypto.PrivKey, string, error) { device, err := getLedgerDevice() if err != nil { - return err + return nil, "", err } defer warnIfErrors(device.Close) - pubKey, err := getPubKey(device, path) + pubKey, addr, err := getPubKeyAddrSafe(device, path, hrp) if err != nil { - return err - } - - if pubKey != expectedPubKey { - return fmt.Errorf("pubkey does not match, Check this is the same device") + return nil, "", err } - return device.ShowAddressSECP256K1(path.DerivationPath(), types.Bech32PrefixAccAddr) + return PrivKeyLedgerSecp256k1{pubKey, path}, addr, nil } // PubKey returns the cached public key. @@ -100,6 +102,35 @@ func (pkl PrivKeyLedgerSecp256k1) Sign(message []byte) ([]byte, error) { return sign(device, pkl, message) } +// LedgerShowAddress triggers a ledger device to show the corresponding address. +func LedgerShowAddress(path hd.BIP44Params, expectedPubKey tmcrypto.PubKey) error { + device, err := getLedgerDevice() + if err != nil { + return err + } + defer warnIfErrors(device.Close) + + pubKey, err := getPubKeyUnsafe(device, path) + if err != nil { + return err + } + + if pubKey != expectedPubKey { + return fmt.Errorf("the key's pubkey does not match with the one retrieved from Ledger. Check that the HD path and device are the correct ones") + } + + pubKey2, _, err := getPubKeyAddrSafe(device, path, types.Bech32PrefixAccAddr) + if err != nil { + return err + } + + if pubKey2 != expectedPubKey { + return fmt.Errorf("the key's pubkey does not match with the one retrieved from Ledger. Check that the HD path and device are the correct ones") + } + + return nil +} + // ValidateKey allows us to verify the sanity of a public key after loading it // from disk. func (pkl PrivKeyLedgerSecp256k1) ValidateKey() error { @@ -161,7 +192,7 @@ func getLedgerDevice() (LedgerSECP256K1, error) { } func validateKey(device LedgerSECP256K1, pkl PrivKeyLedgerSecp256k1) error { - pub, err := getPubKey(device, pkl.Path) + pub, err := getPubKeyUnsafe(device, pkl.Path) if err != nil { return err } @@ -193,10 +224,15 @@ func sign(device LedgerSECP256K1, pkl PrivKeyLedgerSecp256k1, msg []byte) ([]byt return convertDERtoBER(sig) } -// getPubKey reads the pubkey the ledger itself +// getPubKeyUnsafe reads the pubkey from a ledger device +// +// This function is marked as unsafe as it will retrieve a pubkey without user verification +// It can only be used to verify a pubkey but never to create new accounts/keys. In that case, +// please refer to getPubKeyAddrSafe +// // since this involves IO, it may return an error, which is not exposed // in the PubKey interface, so this function allows better error handling -func getPubKey(device LedgerSECP256K1, path hd.BIP44Params) (tmcrypto.PubKey, error) { +func getPubKeyUnsafe(device LedgerSECP256K1, path hd.BIP44Params) (tmcrypto.PubKey, error) { publicKey, err := device.GetPublicKeySECP256K1(path.DerivationPath()) if err != nil { return nil, fmt.Errorf("please open Cosmos app on the Ledger device - error: %v", err) @@ -213,3 +249,27 @@ func getPubKey(device LedgerSECP256K1, path hd.BIP44Params) (tmcrypto.PubKey, er return compressedPublicKey, nil } + +// getPubKeyAddr reads the pubkey and the address from a ledger device. +// This function is marked as Safe as it will require user confirmation and +// account and index will be shown in the device. +// +// Since this involves IO, it may return an error, which is not exposed +// in the PubKey interface, so this function allows better error handling. +func getPubKeyAddrSafe(device LedgerSECP256K1, path hd.BIP44Params, hrp string) (tmcrypto.PubKey, string, error) { + publicKey, addr, err := device.GetAddressPubKeySECP256K1(path.DerivationPath(), hrp) + if err != nil { + return nil, "", fmt.Errorf("address %s rejected", addr) + } + + // re-serialize in the 33-byte compressed format + cmp, err := btcec.ParsePubKey(publicKey[:], btcec.S256()) + if err != nil { + return nil, "", fmt.Errorf("error parsing public key: %v", err) + } + + var compressedPublicKey tmsecp256k1.PubKeySecp256k1 + copy(compressedPublicKey[:], cmp.SerializeCompressed()) + + return compressedPublicKey, addr, nil +} diff --git a/crypto/ledger_test.go b/crypto/ledger_test.go index ea2bc9681c3c..ab92663b3ad6 100644 --- a/crypto/ledger_test.go +++ b/crypto/ledger_test.go @@ -4,41 +4,44 @@ import ( "fmt" "testing" - "github.com/cosmos/cosmos-sdk/tests" - + "github.com/stretchr/testify/require" tmcrypto "github.com/tendermint/tendermint/crypto" "github.com/tendermint/tendermint/crypto/encoding/amino" "github.com/cosmos/cosmos-sdk/crypto/keys/hd" + "github.com/cosmos/cosmos-sdk/tests" sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/stretchr/testify/require" ) func TestLedgerErrorHandling(t *testing.T) { // first, try to generate a key, must return an error // (no panic) path := *hd.NewParams(44, 555, 0, false, 0) - _, err := NewPrivKeyLedgerSecp256k1(path) + _, err := NewPrivKeyLedgerSecp256k1Unsafe(path) require.Error(t, err) } -func TestPublicKey(t *testing.T) { +func TestPublicKeyUnsafe(t *testing.T) { path := *hd.NewFundraiserParams(0, 0) - priv, err := NewPrivKeyLedgerSecp256k1(path) + priv, err := NewPrivKeyLedgerSecp256k1Unsafe(path) require.Nil(t, err, "%s", err) require.NotNil(t, priv) + require.Equal(t, "eb5ae98721034fef9cd7c4c63588d3b03feb5281b9d232cba34d6f3d71aee59211ffbfe1fe87", + fmt.Sprintf("%x", priv.PubKey().Bytes()), + "Is your device using test mnemonic: %s ?", tests.TestMnemonic) + pubKeyAddr, err := sdk.Bech32ifyAccPub(priv.PubKey()) require.NoError(t, err) require.Equal(t, "cosmospub1addwnpepqd87l8xhcnrrtzxnkql7k55ph8fr9jarf4hn6udwukfprlalu8lgw0urza0", pubKeyAddr, "Is your device using test mnemonic: %s ?", tests.TestMnemonic) - require.Equal(t, "5075624b6579536563703235366b317b303334464546394344374334433633353838443342303"+ - "3464542353238314239443233324342413334443646334437314145453539323131464642464531464538377d", - fmt.Sprintf("%x", priv.PubKey())) + addr := sdk.AccAddress(priv.PubKey().Address()).String() + require.Equal(t, "cosmos1w34k53py5v5xyluazqpq65agyajavep2rflq6h", + addr, "Is your device using test mnemonic: %s ?", tests.TestMnemonic) } -func TestPublicKeyHDPath(t *testing.T) { +func TestPublicKeyUnsafeHDPath(t *testing.T) { expectedAnswers := []string{ "cosmospub1addwnpepqd87l8xhcnrrtzxnkql7k55ph8fr9jarf4hn6udwukfprlalu8lgw0urza0", "cosmospub1addwnpepqfsdqjr68h7wjg5wacksmqaypasnra232fkgu5sxdlnlu8j22ztxvlqvd65", @@ -61,7 +64,7 @@ func TestPublicKeyHDPath(t *testing.T) { path := *hd.NewFundraiserParams(0, i) fmt.Printf("Checking keys at %v\n", path) - priv, err := NewPrivKeyLedgerSecp256k1(path) + priv, err := NewPrivKeyLedgerSecp256k1Unsafe(path) require.Nil(t, err, "%s", err) require.NotNil(t, priv) @@ -93,6 +96,104 @@ func TestPublicKeyHDPath(t *testing.T) { } } +func TestPublicKeySafe(t *testing.T) { + path := *hd.NewFundraiserParams(0, 0) + priv, addr, err := NewPrivKeyLedgerSecp256k1(path, "cosmos") + + require.Nil(t, err, "%s", err) + require.NotNil(t, priv) + + require.Equal(t, "eb5ae98721034fef9cd7c4c63588d3b03feb5281b9d232cba34d6f3d71aee59211ffbfe1fe87", + fmt.Sprintf("%x", priv.PubKey().Bytes()), + "Is your device using test mnemonic: %s ?", tests.TestMnemonic) + + pubKeyAddr, err := sdk.Bech32ifyAccPub(priv.PubKey()) + require.NoError(t, err) + require.Equal(t, "cosmospub1addwnpepqd87l8xhcnrrtzxnkql7k55ph8fr9jarf4hn6udwukfprlalu8lgw0urza0", + pubKeyAddr, "Is your device using test mnemonic: %s ?", tests.TestMnemonic) + + require.Equal(t, "cosmos1w34k53py5v5xyluazqpq65agyajavep2rflq6h", + addr, "Is your device using test mnemonic: %s ?", tests.TestMnemonic) + + addr2 := sdk.AccAddress(priv.PubKey().Address()).String() + require.Equal(t, addr, addr2) +} + +func TestPublicKeyHDPath(t *testing.T) { + expectedPubKeys := []string{ + "cosmospub1addwnpepqd87l8xhcnrrtzxnkql7k55ph8fr9jarf4hn6udwukfprlalu8lgw0urza0", + "cosmospub1addwnpepqfsdqjr68h7wjg5wacksmqaypasnra232fkgu5sxdlnlu8j22ztxvlqvd65", + "cosmospub1addwnpepqw3xwqun6q43vtgw6p4qspq7srvxhcmvq4jrx5j5ma6xy3r7k6dtxmrkh3d", + "cosmospub1addwnpepqvez9lrp09g8w7gkv42y4yr5p6826cu28ydrhrujv862yf4njmqyyjr4pjs", + "cosmospub1addwnpepq06hw3enfrtmq8n67teytcmtnrgcr0yntmyt25kdukfjkerdc7lqg32rcz7", + "cosmospub1addwnpepqg3trf2gd0s2940nckrxherwqhgmm6xd5h4pcnrh4x7y35h6yafmcpk5qns", + "cosmospub1addwnpepqdm6rjpx6wsref8wjn7ym6ntejet430j4szpngfgc20caz83lu545vuv8hp", + "cosmospub1addwnpepqvdhtjzy2wf44dm03jxsketxc07vzqwvt3vawqqtljgsr9s7jvydjmt66ew", + "cosmospub1addwnpepqwystfpyxwcava7v3t7ndps5xzu6s553wxcxzmmnxevlzvwrlqpzz695nw9", + "cosmospub1addwnpepqw970u6gjqkccg9u3rfj99857wupj2z9fqfzy2w7e5dd7xn7kzzgkgqch0r", + } + + expectedAddrs := []string{ + "cosmos1w34k53py5v5xyluazqpq65agyajavep2rflq6h", + "cosmos19ewxwemt6uahejvwf44u7dh6tq859tkyvarh2q", + "cosmos1a07dzdjgjsntxpp75zg7cgatgq0udh3pcdcxm3", + "cosmos1qvw52lmn9gpvem8welghrkc52m3zczyhlqjsl7", + "cosmos17m78ka80fqkkw2c4ww0v4xm5nsu2drgrlm8mn2", + "cosmos1ferh9ll9c452d2p8k2v7heq084guygkn43up9e", + "cosmos10vf3sxmjg96rqq36axcphzfsl74dsntuehjlw5", + "cosmos1cq83av8cmnar79h0rg7duh9gnr7wkh228a7fxg", + "cosmos1dszhfrt226jy5rsre7e48vw9tgwe90uerfyefa", + "cosmos1734d7qsylzrdt05muhqqtpd90j8mp4y6rzch8l", + } + + const numIters = 10 + + privKeys := make([]tmcrypto.PrivKey, numIters) + + // Check with device + for i := uint32(0); i < 10; i++ { + path := *hd.NewFundraiserParams(0, i) + fmt.Printf("Checking keys at %v\n", path) + + priv, addr, err := NewPrivKeyLedgerSecp256k1(path, "cosmos") + require.Nil(t, err, "%s", err) + require.NotNil(t, addr) + require.NotNil(t, priv) + + addr2 := sdk.AccAddress(priv.PubKey().Address()).String() + require.Equal(t, addr2, addr) + require.Equal(t, + expectedAddrs[i], addr, + "Is your device using test mnemonic: %s ?", tests.TestMnemonic) + + // Check other methods + require.NoError(t, priv.(PrivKeyLedgerSecp256k1).ValidateKey()) + tmp := priv.(PrivKeyLedgerSecp256k1) + (&tmp).AssertIsPrivKeyInner() + + pubKeyAddr, err := sdk.Bech32ifyAccPub(priv.PubKey()) + require.NoError(t, err) + require.Equal(t, + expectedPubKeys[i], pubKeyAddr, + "Is your device using test mnemonic: %s ?", tests.TestMnemonic) + + // Store and restore + serializedPk := priv.Bytes() + require.NotNil(t, serializedPk) + require.True(t, len(serializedPk) >= 50) + + privKeys[i] = priv + } + + // Now check equality + for i := 0; i < 10; i++ { + for j := 0; j < 10; j++ { + require.Equal(t, i == j, privKeys[i].Equals(privKeys[j])) + require.Equal(t, i == j, privKeys[j].Equals(privKeys[i])) + } + } +} + func getFakeTx(accountNumber uint32) []byte { tmp := fmt.Sprintf( `{"account_number":"%d","chain_id":"1234","fee":{"amount":[{"amount":"150","denom":"atom"}],"gas":"5000"},"memo":"memo","msgs":[[""]],"sequence":"6"}`, @@ -108,7 +209,7 @@ func TestSignaturesHD(t *testing.T) { path := *hd.NewFundraiserParams(account, account/5) fmt.Printf("Checking signature at %v --- PLEASE REVIEW AND ACCEPT IN THE DEVICE\n", path) - priv, err := NewPrivKeyLedgerSecp256k1(path) + priv, err := NewPrivKeyLedgerSecp256k1Unsafe(path) require.Nil(t, err, "%s", err) pub := priv.PubKey() @@ -123,7 +224,7 @@ func TestSignaturesHD(t *testing.T) { func TestRealLedgerSecp256k1(t *testing.T) { msg := getFakeTx(50) path := *hd.NewFundraiserParams(0, 0) - priv, err := NewPrivKeyLedgerSecp256k1(path) + priv, err := NewPrivKeyLedgerSecp256k1Unsafe(path) require.Nil(t, err, "%s", err) pub := priv.PubKey() diff --git a/go.mod b/go.mod index a845f33837cc..69715d3dcb54 100644 --- a/go.mod +++ b/go.mod @@ -7,8 +7,7 @@ require ( github.com/bgentry/speakeasy v0.1.0 github.com/btcsuite/btcd v0.0.0-20190115013929-ed77733ec07d github.com/cosmos/go-bip39 v0.0.0-20180618194314-52158e4697b8 - github.com/cosmos/ledger-cosmos-go v0.9.11 - github.com/cosmos/ledger-go v0.9.1 // indirect + github.com/cosmos/ledger-cosmos-go v0.10.3 github.com/fortytw2/leaktest v1.3.0 // indirect github.com/go-logfmt/logfmt v0.4.0 // indirect github.com/gogo/protobuf v1.1.1 @@ -27,7 +26,7 @@ require ( github.com/otiai10/curr v0.0.0-20150429015615-9b4961190c95 // indirect github.com/otiai10/mint v1.2.3 // indirect github.com/pelletier/go-toml v1.2.0 - github.com/pkg/errors v0.8.0 + github.com/pkg/errors v0.8.1 github.com/prometheus/client_golang v0.9.2 // indirect github.com/prometheus/client_model v0.0.0-20190129233127-fd36f4220a90 // indirect github.com/prometheus/common v0.2.0 // indirect @@ -41,13 +40,12 @@ require ( github.com/spf13/jwalterweatherman v1.1.0 // indirect github.com/spf13/pflag v1.0.3 github.com/spf13/viper v1.0.3 - github.com/stretchr/testify v1.2.2 + github.com/stretchr/testify v1.3.0 github.com/syndtr/goleveldb v0.0.0-20180708030551-c4c61651e9e3 // indirect github.com/tendermint/btcd v0.1.1 github.com/tendermint/go-amino v0.14.1 github.com/tendermint/iavl v0.12.1 - github.com/tendermint/tendermint v0.31.4 - github.com/zondax/hid v0.9.0 // indirect + github.com/tendermint/tendermint v0.31.5 golang.org/x/crypto v0.0.0-20180904163835-0709b304e793 google.golang.org/grpc v1.19.0 // indirect gopkg.in/yaml.v2 v2.2.2 // indirect diff --git a/go.sum b/go.sum index f7fe4d27863c..f2b1641c1226 100644 --- a/go.sum +++ b/go.sum @@ -27,11 +27,12 @@ github.com/btcsuite/winsvc v1.0.0/go.mod h1:jsenWakMcC0zFBFurPLEAyrnc/teJEM1O46f github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw= github.com/cosmos/go-bip39 v0.0.0-20180618194314-52158e4697b8 h1:Iwin12wRQtyZhH6FV3ykFcdGNlYEzoeR0jN8Vn+JWsI= github.com/cosmos/go-bip39 v0.0.0-20180618194314-52158e4697b8/go.mod h1:tSxLoYXyBmiFeKpvmq4dzayMdCjCnu8uqmCysIGBT2Y= -github.com/cosmos/ledger-cosmos-go v0.9.11 h1:bkcIKqaM6evicjkSP+Le8HDLXt9P+MqGRnGiMUC20m4= -github.com/cosmos/ledger-cosmos-go v0.9.11/go.mod h1:RWldjvUf4Hfi46ti/8etBH3eQ2rOqqz2hstdzROQSHo= -github.com/cosmos/ledger-go v0.9.1 h1:bRIamtlWShVk1THw52NdCPHxtBxKnauglSB23mH1/w8= -github.com/cosmos/ledger-go v0.9.1/go.mod h1:oZJ2hHAZROdlHiwTg4t7kP+GKIIkBT+o6c9QWFanOyI= +github.com/cosmos/ledger-cosmos-go v0.10.3 h1:Qhi5yTR5Pg1CaTpd00pxlGwNl4sFRdtK1J96OTjeFFc= +github.com/cosmos/ledger-cosmos-go v0.10.3/go.mod h1:J8//BsAGTo3OC/vDLjMRFLW6q0WAaXvHnVc7ZmE8iUY= +github.com/cosmos/ledger-go v0.9.2 h1:Nnao/dLwaVTk1Q5U9THldpUMMXU94BOTWPddSmVB6pI= +github.com/cosmos/ledger-go v0.9.2/go.mod h1:oZJ2hHAZROdlHiwTg4t7kP+GKIIkBT+o6c9QWFanOyI= github.com/davecgh/go-spew v0.0.0-20171005155431-ecdeabc65495/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/fortytw2/leaktest v1.3.0 h1:u8491cBMTQ8ft8aeV+adlcytMZylmA5nnwwkRZjI8vw= @@ -99,6 +100,8 @@ github.com/pelletier/go-toml v1.2.0 h1:T5zMGML61Wp+FlcbWjRDT7yAxhJNAiPPLOFECq181 github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/94hg7ilaic= github.com/pkg/errors v0.8.0 h1:WdK/asTD0HN+q6hsWO3/vpuAkAr+tw6aNJNDFFf0+qw= github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= +github.com/pkg/errors v0.8.1 h1:iURUrRGxPUNPdy5/HRSm+Yj6okJ6UtLINN0Q9M4+h3I= +github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/prometheus/client_golang v0.9.1/go.mod h1:7SWBe2y4D6OKWSNQJUaRYU/AaXPKyh/dDVn+NZz0KFw= @@ -133,9 +136,12 @@ github.com/spf13/pflag v1.0.3 h1:zPAT6CGy6wXeQ7NtTnaTerfKOsV6V6F8agHXFiazDkg= github.com/spf13/pflag v1.0.3/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnInEg4= github.com/spf13/viper v1.0.3 h1:z5LPUc2iz8VLT5Cw1UyrESG6FUUnOGecYGY08BLKSuc= github.com/spf13/viper v1.0.3/go.mod h1:A8kyI5cUJhb8N+3pkfONlcEcZbueH6nhAm0Fq7SrnBM= +github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.2.2 h1:bSDNvY7ZPG5RlJ8otE/7V6gMiyenm9RtJ7IUVIAoJ1w= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= +github.com/stretchr/testify v1.3.0 h1:TivCn/peBQ7UY8ooIcPgZFpTNSz0Q2U6UrFlUfqbe0Q= +github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/syndtr/goleveldb v0.0.0-20180708030551-c4c61651e9e3 h1:sAlSBRDl4psFR3ysKXRSE8ss6Mt90+ma1zRTroTNBJA= github.com/syndtr/goleveldb v0.0.0-20180708030551-c4c61651e9e3/go.mod h1:Z4AUp2Km+PwemOoO/VB5AOx9XSsIItzFjoJlOSiYmn0= github.com/tendermint/btcd v0.1.1 h1:0VcxPfflS2zZ3RiOAHkBiFUcPvbtRj5O7zHmcJWHV7s= @@ -146,8 +152,8 @@ github.com/tendermint/go-amino v0.14.1 h1:o2WudxNfdLNBwMyl2dqOJxiro5rfrEaU0Ugs6o github.com/tendermint/go-amino v0.14.1/go.mod h1:i/UKE5Uocn+argJJBb12qTZsCDBcAYMbR92AaJVmKso= github.com/tendermint/iavl v0.12.1 h1:JDfyhM/Hhrumu1CL1Nxrypm8sNTPYqmeHo1IZLiJoXM= github.com/tendermint/iavl v0.12.1/go.mod h1:EoKMMv++tDOL5qKKVnoIqtVPshRrEPeJ0WsgDOLAauM= -github.com/tendermint/tendermint v0.31.4 h1:F/vZ/fMHZJriAkDLjf9ZrReJQsuELiTmJLqigmbE5NU= -github.com/tendermint/tendermint v0.31.4/go.mod h1:ymcPyWblXCplCPQjbOYbrF1fWnpslATMVqiGgWbZrlc= +github.com/tendermint/tendermint v0.31.5 h1:vTet8tCq3B9/J9Yo11dNZ8pOB7NtSy++bVSfkP4KzR4= +github.com/tendermint/tendermint v0.31.5/go.mod h1:ymcPyWblXCplCPQjbOYbrF1fWnpslATMVqiGgWbZrlc= github.com/zondax/hid v0.9.0 h1:eiT3P6vNxAEVxXMw66eZUAAnU2zD33JBkfG/EnfAKl8= github.com/zondax/hid v0.9.0/go.mod h1:l5wttcP0jwtdLjqjMMWFVEE7d1zO0jvSPA9OPZxWpEM= golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE= diff --git a/store/cachekv/memiterator.go b/store/cachekv/memiterator.go index 972280c42a0a..9e5a6ac33164 100644 --- a/store/cachekv/memiterator.go +++ b/store/cachekv/memiterator.go @@ -2,6 +2,7 @@ package cachekv import ( "bytes" + "container/list" cmn "github.com/tendermint/tendermint/libs/common" dbm "github.com/tendermint/tendermint/libs/db" @@ -12,20 +13,30 @@ import ( // Implements Iterator. type memIterator struct { start, end []byte - items []cmn.KVPair + items []*cmn.KVPair + ascending bool } -func newMemIterator(start, end []byte, items []cmn.KVPair) *memIterator { - itemsInDomain := make([]cmn.KVPair, 0) - for _, item := range items { - if dbm.IsKeyInDomain(item.Key, start, end) { - itemsInDomain = append(itemsInDomain, item) +func newMemIterator(start, end []byte, items *list.List, ascending bool) *memIterator { + itemsInDomain := make([]*cmn.KVPair, 0) + var entered bool + for e := items.Front(); e != nil; e = e.Next() { + item := e.Value.(*cmn.KVPair) + if !dbm.IsKeyInDomain(item.Key, start, end) { + if entered { + break + } + continue } + itemsInDomain = append(itemsInDomain, item) + entered = true } + return &memIterator{ - start: start, - end: end, - items: itemsInDomain, + start: start, + end: end, + items: itemsInDomain, + ascending: ascending, } } @@ -45,17 +56,27 @@ func (mi *memIterator) assertValid() { func (mi *memIterator) Next() { mi.assertValid() - mi.items = mi.items[1:] + if mi.ascending { + mi.items = mi.items[1:] + } else { + mi.items = mi.items[:len(mi.items)-1] + } } func (mi *memIterator) Key() []byte { mi.assertValid() - return mi.items[0].Key + if mi.ascending { + return mi.items[0].Key + } + return mi.items[len(mi.items)-1].Key } func (mi *memIterator) Value() []byte { mi.assertValid() - return mi.items[0].Value + if mi.ascending { + return mi.items[0].Value + } + return mi.items[len(mi.items)-1].Value } func (mi *memIterator) Close() { diff --git a/store/cachekv/store.go b/store/cachekv/store.go index 673a101efa61..259a888af655 100644 --- a/store/cachekv/store.go +++ b/store/cachekv/store.go @@ -2,6 +2,7 @@ package cachekv import ( "bytes" + "container/list" "io" "sort" "sync" @@ -24,9 +25,11 @@ type cValue struct { // Store wraps an in-memory cache around an underlying types.KVStore. type Store struct { - mtx sync.Mutex - cache map[string]cValue - parent types.KVStore + mtx sync.Mutex + cache map[string]*cValue + unsortedCache map[string]struct{} + sortedCache *list.List // always ascending sorted + parent types.KVStore } var _ types.CacheKVStore = (*Store)(nil) @@ -34,8 +37,10 @@ var _ types.CacheKVStore = (*Store)(nil) // nolint func NewStore(parent types.KVStore) *Store { return &Store{ - cache: make(map[string]cValue), - parent: parent, + cache: make(map[string]*cValue), + unsortedCache: make(map[string]struct{}), + sortedCache: list.New(), + parent: parent, } } @@ -116,7 +121,9 @@ func (store *Store) Write() { } // Clear the cache - store.cache = make(map[string]cValue) + store.cache = make(map[string]*cValue) + store.unsortedCache = make(map[string]struct{}) + store.sortedCache = list.New() } //---------------------------------------- @@ -146,6 +153,9 @@ func (store *Store) ReverseIterator(start, end []byte) types.Iterator { } func (store *Store) iterator(start, end []byte, ascending bool) types.Iterator { + store.mtx.Lock() + defer store.mtx.Unlock() + var parent, cache types.Iterator if ascending { @@ -154,33 +164,49 @@ func (store *Store) iterator(start, end []byte, ascending bool) types.Iterator { parent = store.parent.ReverseIterator(start, end) } - items := store.dirtyItems(start, end, ascending) - cache = newMemIterator(start, end, items) + store.dirtyItems(start, end) + cache = newMemIterator(start, end, store.sortedCache, ascending) return newCacheMergeIterator(parent, cache, ascending) } // Constructs a slice of dirty items, to use w/ memIterator. -func (store *Store) dirtyItems(start, end []byte, ascending bool) []cmn.KVPair { - items := make([]cmn.KVPair, 0) +func (store *Store) dirtyItems(start, end []byte) { + unsorted := make([]*cmn.KVPair, 0) - for key, cacheValue := range store.cache { - if !cacheValue.dirty { - continue - } + for key := range store.unsortedCache { + cacheValue := store.cache[key] if dbm.IsKeyInDomain([]byte(key), start, end) { - items = append(items, cmn.KVPair{Key: []byte(key), Value: cacheValue.value}) + unsorted = append(unsorted, &cmn.KVPair{Key: []byte(key), Value: cacheValue.value}) + delete(store.unsortedCache, key) } } - sort.Slice(items, func(i, j int) bool { - if ascending { - return bytes.Compare(items[i].Key, items[j].Key) < 0 - } - return bytes.Compare(items[i].Key, items[j].Key) > 0 + sort.Slice(unsorted, func(i, j int) bool { + return bytes.Compare(unsorted[i].Key, unsorted[j].Key) < 0 }) - return items + for e := store.sortedCache.Front(); e != nil && len(unsorted) != 0; { + uitem := unsorted[0] + sitem := e.Value.(*cmn.KVPair) + comp := bytes.Compare(uitem.Key, sitem.Key) + switch comp { + case -1: + unsorted = unsorted[1:] + store.sortedCache.InsertBefore(uitem, e) + case 1: + e = e.Next() + case 0: + unsorted = unsorted[1:] + e.Value = uitem + e = e.Next() + } + } + + for _, kvp := range unsorted { + store.sortedCache.PushBack(kvp) + } + } //---------------------------------------- @@ -188,9 +214,12 @@ func (store *Store) dirtyItems(start, end []byte, ascending bool) []cmn.KVPair { // Only entrypoint to mutate store.cache. func (store *Store) setCacheValue(key, value []byte, deleted bool, dirty bool) { - store.cache[string(key)] = cValue{ + store.cache[string(key)] = &cValue{ value: value, deleted: deleted, dirty: dirty, } + if dirty { + store.unsortedCache[string(key)] = struct{}{} + } } diff --git a/store/cachekv/store_bench_test.go b/store/cachekv/store_bench_test.go new file mode 100644 index 000000000000..d45017f8eb20 --- /dev/null +++ b/store/cachekv/store_bench_test.go @@ -0,0 +1,46 @@ +package cachekv_test + +import ( + "crypto/rand" + "sort" + "testing" + + dbm "github.com/tendermint/tendermint/libs/db" + + "github.com/cosmos/cosmos-sdk/store/cachekv" + "github.com/cosmos/cosmos-sdk/store/dbadapter" +) + +func benchmarkCacheKVStoreIterator(numKVs int, b *testing.B) { + mem := dbadapter.Store{DB: dbm.NewMemDB()} + cstore := cachekv.NewStore(mem) + keys := make([]string, numKVs, numKVs) + + for i := 0; i < numKVs; i++ { + key := make([]byte, 32) + value := make([]byte, 32) + + _, _ = rand.Read(key) + _, _ = rand.Read(value) + + keys[i] = string(key) + cstore.Set(key, value) + } + + sort.Strings(keys) + + for n := 0; n < b.N; n++ { + iter := cstore.Iterator([]byte(keys[0]), []byte(keys[numKVs-1])) + + for _ = iter.Key(); iter.Valid(); iter.Next() { + } + + iter.Close() + } +} + +func BenchmarkCacheKVStoreIterator500(b *testing.B) { benchmarkCacheKVStoreIterator(500, b) } +func BenchmarkCacheKVStoreIterator1000(b *testing.B) { benchmarkCacheKVStoreIterator(1000, b) } +func BenchmarkCacheKVStoreIterator10000(b *testing.B) { benchmarkCacheKVStoreIterator(10000, b) } +func BenchmarkCacheKVStoreIterator50000(b *testing.B) { benchmarkCacheKVStoreIterator(50000, b) } +func BenchmarkCacheKVStoreIterator100000(b *testing.B) { benchmarkCacheKVStoreIterator(100000, b) } diff --git a/store/cachekv/store_test.go b/store/cachekv/store_test.go index 30c8d148ac1d..7c0a4d1d0b8a 100644 --- a/store/cachekv/store_test.go +++ b/store/cachekv/store_test.go @@ -293,8 +293,8 @@ func TestCacheKVMergeIteratorRandom(t *testing.T) { st := newCacheKVStore() truth := dbm.NewMemDB() - start, end := 25, 75 - max := 100 + start, end := 25, 975 + max := 1000 setRange(st, truth, start, end) // do an op, test the iterator diff --git a/x/auth/ante.go b/x/auth/ante.go index 376f36018e03..9aae4489bf1f 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -193,7 +193,7 @@ func processSig( } if !simulate && !pubKey.VerifyBytes(signBytes, sig.Signature) { - return nil, sdk.ErrUnauthorized("signature verification failed").Result() + return nil, sdk.ErrUnauthorized("signature verification failed; verify correct account sequence and chain-id").Result() } if err := acc.SetSequence(acc.GetSequence() + 1); err != nil { diff --git a/x/mint/client/module_client.go b/x/mint/client/module_client.go index 857bb7992665..0b023f792567 100644 --- a/x/mint/client/module_client.go +++ b/x/mint/client/module_client.go @@ -1,12 +1,12 @@ package clientpackage import ( + "github.com/spf13/cobra" + "github.com/tendermint/go-amino" + "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/x/mint" "github.com/cosmos/cosmos-sdk/x/mint/client/cli" - - "github.com/spf13/cobra" - "github.com/tendermint/go-amino" ) // ModuleClient exports all CLI client functionality from the minting module. diff --git a/x/simulation/params.go b/x/simulation/params.go index 8499e6c1189b..6202ec6fef37 100644 --- a/x/simulation/params.go +++ b/x/simulation/params.go @@ -60,7 +60,7 @@ func DefaultParams() Params { func RandomParams(r *rand.Rand) Params { return Params{ PastEvidenceFraction: r.Float64(), - NumKeys: r.Intn(250), + NumKeys: RandIntBetween(r, 2, 250), EvidenceFraction: r.Float64(), InitialLivenessWeightings: []int{r.Intn(80), r.Intn(10), r.Intn(10)}, LivenessTransitionMatrix: defaultLivenessTransitionMatrix, diff --git a/x/simulation/rand_util.go b/x/simulation/rand_util.go index a9b32ca48dd4..9677f2a612e6 100644 --- a/x/simulation/rand_util.go +++ b/x/simulation/rand_util.go @@ -72,6 +72,11 @@ func RandTimestamp(r *rand.Rand) time.Time { return time.Unix(unixTime, 0) } +// RandIntBetween returns a random int between two numbers inclusively. +func RandIntBetween(r *rand.Rand, min, max int) int { + return r.Intn(max-min) + min +} + // Derive a new rand deterministically from a rand. // Unlike rand.New(rand.NewSource(seed)), the result is "more random" // depending on the source and state of r. diff --git a/x/staking/handler.go b/x/staking/handler.go index 7da83cec2e5b..aa39618c50c9 100644 --- a/x/staking/handler.go +++ b/x/staking/handler.go @@ -59,8 +59,8 @@ func EndBlocker(ctx sdk.Context, k keeper.Keeper) ([]abci.ValidatorUpdate, sdk.T continue } - resTags.AppendTags(sdk.NewTags( - tags.Action, ActionCompleteUnbonding, + resTags = resTags.AppendTags(sdk.NewTags( + tags.Action, tags.ActionCompleteUnbonding, tags.Delegator, dvPair.DelegatorAddress.String(), tags.SrcValidator, dvPair.ValidatorAddress.String(), )) @@ -75,7 +75,7 @@ func EndBlocker(ctx sdk.Context, k keeper.Keeper) ([]abci.ValidatorUpdate, sdk.T continue } - resTags.AppendTags(sdk.NewTags( + resTags = resTags.AppendTags(sdk.NewTags( tags.Action, tags.ActionCompleteRedelegation, tags.Delegator, dvvTriplet.DelegatorAddress.String(), tags.SrcValidator, dvvTriplet.ValidatorSrcAddress.String(), From e1a33e6635703a2a8c37419043e5368489510a6e Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 24 May 2019 09:56:41 -0400 Subject: [PATCH 2/8] Cherry Pick #4384: allow splitting withdrawal txs --- CHANGELOG.md | 1 + x/distribution/client/cli/tx.go | 49 +++++++++++++++-- x/distribution/client/cli/tx_test.go | 79 ++++++++++++++++++++++++++++ 3 files changed, 125 insertions(+), 4 deletions(-) create mode 100644 x/distribution/client/cli/tx_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index ad0800688532..27eea2c0b80e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ * [\#2286](https://github.com/cosmos/cosmos-sdk/issues/2286) Improve performance of `CacheKVStore` iterator. * [\#3655](https://github.com/cosmos/cosmos-sdk/issues/3655) Improve signature verification failure error message. +* [\#4384](https://github.com/cosmos/cosmos-sdk/issues/4384) Allow splitting withdrawal transaction in several chunks. #### Gaia CLI diff --git a/x/distribution/client/cli/tx.go b/x/distribution/client/cli/tx.go index 8bf143ce953f..b8e59cbec482 100644 --- a/x/distribution/client/cli/tx.go +++ b/x/distribution/client/cli/tx.go @@ -23,6 +23,11 @@ var ( flagOnlyFromValidator = "only-from-validator" flagIsValidator = "is-validator" flagComission = "commission" + flagMaxMessagesPerTx = "max-msgs" +) + +const ( + MaxMessagesPerTxDefault = 5 ) // GetTxCmd returns the transaction commands for this module @@ -40,6 +45,39 @@ func GetTxCmd(storeKey string, cdc *amino.Codec) *cobra.Command { return distTxCmd } +type generateOrBroadcastFunc func(context.CLIContext, authtxb.TxBuilder, []sdk.Msg, bool) error + +func splitAndApply( + generateOrBroadcast generateOrBroadcastFunc, + cliCtx context.CLIContext, + txBldr authtxb.TxBuilder, + msgs []sdk.Msg, + chunkSize int, + offline bool, +) error { + + if chunkSize == 0 { + return generateOrBroadcast(cliCtx, txBldr, msgs, offline) + } + + // split messages into slices of length chunkSize + totalMessages := len(msgs) + for i := 0; i < len(msgs); i += chunkSize { + + sliceEnd := i + chunkSize + if sliceEnd > totalMessages { + sliceEnd = totalMessages + } + + msgChunk := msgs[i:sliceEnd] + if err := generateOrBroadcast(cliCtx, txBldr, msgChunk, offline); err != nil { + return err + } + } + + return nil +} + // command to withdraw rewards func GetCmdWithdrawRewards(cdc *codec.Codec) *cobra.Command { cmd := &cobra.Command{ @@ -77,7 +115,7 @@ $ gaiacli tx distr withdraw-rewards cosmosvaloper1gghjut3ccd8ay0zduzj64hwre2fxs9 // command to withdraw all rewards func GetCmdWithdrawAllRewards(cdc *codec.Codec, queryRoute string) *cobra.Command { - return &cobra.Command{ + cmd := &cobra.Command{ Use: "withdraw-all-rewards", Short: "withdraw all delegations rewards for a delegator", Long: strings.TrimSpace(`Withdraw all rewards for a single delegator: @@ -98,14 +136,18 @@ $ gaiacli tx distr withdraw-all-rewards --from mykey return err } - return utils.GenerateOrBroadcastMsgs(cliCtx, txBldr, msgs, false) + chunkSize := viper.GetInt(flagMaxMessagesPerTx) + return splitAndApply(utils.GenerateOrBroadcastMsgs, cliCtx, txBldr, msgs, chunkSize, false) }, } + + cmd.Flags().Int(flagMaxMessagesPerTx, MaxMessagesPerTxDefault, "Limit the number of messages per tx (0 for unlimited)") + return cmd } // command to replace a delegator's withdrawal address func GetCmdSetWithdrawAddr(cdc *codec.Codec) *cobra.Command { - cmd := &cobra.Command{ + return &cobra.Command{ Use: "set-withdraw-addr [withdraw-addr]", Short: "change the default withdraw address for rewards associated with an address", Long: strings.TrimSpace(`Set the withdraw address for rewards associated with a delegator address: @@ -130,5 +172,4 @@ $ gaiacli tx set-withdraw-addr cosmos1gghjut3ccd8ay0zduzj64hwre2fxs9ld75ru9p --f return utils.GenerateOrBroadcastMsgs(cliCtx, txBldr, []sdk.Msg{msg}, false) }, } - return cmd } diff --git a/x/distribution/client/cli/tx_test.go b/x/distribution/client/cli/tx_test.go new file mode 100644 index 000000000000..3477329e03c8 --- /dev/null +++ b/x/distribution/client/cli/tx_test.go @@ -0,0 +1,79 @@ +package cli + +import ( + "testing" + + "github.com/cosmos/cosmos-sdk/client/context" + "github.com/cosmos/cosmos-sdk/client/utils" + "github.com/cosmos/cosmos-sdk/codec" + sdk "github.com/cosmos/cosmos-sdk/types" + authtxb "github.com/cosmos/cosmos-sdk/x/auth/client/txbuilder" + "github.com/stretchr/testify/assert" + "github.com/tendermint/tendermint/crypto/secp256k1" +) + +func createFakeTxBuilder() authtxb.TxBuilder { + cdc := codec.New() + return authtxb.NewTxBuilder( + utils.GetTxEncoder(cdc), + 123, + 9876, + 0, + 1.2, + false, + "test_chain", + "hello", + sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(1))), + sdk.DecCoins{sdk.NewDecCoinFromDec(sdk.DefaultBondDenom, sdk.NewDecWithPrec(10000, sdk.Precision))}, + ) +} + +func Test_splitAndCall_NoMessages(t *testing.T) { + ctx := context.CLIContext{} + txBldr := createFakeTxBuilder() + + err := splitAndApply(nil, ctx, txBldr, nil, 10, false) + assert.NoError(t, err, "") +} + +func Test_splitAndCall_Splitting(t *testing.T) { + ctx := context.CLIContext{} + txBldr := createFakeTxBuilder() + + addr := sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) + + // Add five messages + msgs := []sdk.Msg{ + sdk.NewTestMsg(addr), + sdk.NewTestMsg(addr), + sdk.NewTestMsg(addr), + sdk.NewTestMsg(addr), + sdk.NewTestMsg(addr), + } + + // Keep track of number of calls + const chunkSize = 2 + + callCount := 0 + err := splitAndApply( + func(ctx context.CLIContext, txBldr authtxb.TxBuilder, msgs []sdk.Msg, offline bool) error { + callCount += 1 + + assert.NotNil(t, ctx) + assert.NotNil(t, txBldr) + assert.NotNil(t, msgs) + + if callCount < 3 { + assert.Equal(t, len(msgs), 2) + } else { + assert.Equal(t, len(msgs), 1) + } + + return nil + }, + ctx, txBldr, msgs, chunkSize, false, + ) + + assert.NoError(t, err, "") + assert.Equal(t, 3, callCount) +} From 74bcef1ee2ede1b324a2e39aa4630cc9c882f3bf Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Wed, 29 May 2019 16:44:19 -0400 Subject: [PATCH 3/8] Patch unbonding logic to wait full period after height --- x/staking/keeper/delegation.go | 82 +++++++++++++++++++++++----------- 1 file changed, 57 insertions(+), 25 deletions(-) diff --git a/x/staking/keeper/delegation.go b/x/staking/keeper/delegation.go index f13c2b40fda2..9626d1f1ba97 100644 --- a/x/staking/keeper/delegation.go +++ b/x/staking/keeper/delegation.go @@ -2,12 +2,15 @@ package keeper import ( "bytes" + "fmt" "time" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/staking/types" ) +const undelegatePatchHeight = 50 + // return a specific delegation func (k Keeper) GetDelegation(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) ( @@ -544,16 +547,18 @@ func (k Keeper) unbond(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValA return amount, nil } -// get info for begin functions: completionTime and CreationHeight -func (k Keeper) getBeginInfo(ctx sdk.Context, valSrcAddr sdk.ValAddress) ( - completionTime time.Time, height int64, completeNow bool) { +// getBeginInfo returns the completion time and height of a redelegation, along +// with a boolean signaling if the redelegation is complete based on the source +// validator. +func (k Keeper) getRedelegationBeginInfo( + ctx sdk.Context, valSrcAddr sdk.ValAddress, +) (completionTime time.Time, height int64, completeNow bool) { validator, found := k.GetValidator(ctx, valSrcAddr) + // TODO: When would the validator not be found? switch { - // TODO: when would the validator not be found? case !found || validator.Status == sdk.Bonded: - // the longest wait - just unbonding period from now completionTime = ctx.BlockHeader().Time.Add(k.UnbondingTime(ctx)) height = ctx.BlockHeight() @@ -563,48 +568,75 @@ func (k Keeper) getBeginInfo(ctx sdk.Context, valSrcAddr sdk.ValAddress) ( return completionTime, height, true case validator.Status == sdk.Unbonding: - completionTime = validator.UnbondingCompletionTime - height = validator.UnbondingHeight - return completionTime, height, false + return validator.UnbondingCompletionTime, validator.UnbondingHeight, false default: - panic("unknown validator status") + panic(fmt.Sprintf("unknown validator status: %s", validator.Status)) + } +} + +func (k Keeper) getUndelegateBeginInfo(ctx sdk.Context, valAddr sdk.ValAddress) (completionTime time.Time, height int64) { + validator, found := k.GetValidator(ctx, valAddr) + + // TODO: When would the validator not be found? + switch { + case !found || validator.Status == sdk.Bonded: + completionTime = ctx.BlockHeader().Time.Add(k.UnbondingTime(ctx)) + height = ctx.BlockHeight() + return completionTime, height + + case validator.Status == sdk.Unbonded: + return completionTime, height + + case validator.Status == sdk.Unbonding: + return validator.UnbondingCompletionTime, validator.UnbondingHeight + + default: + panic(fmt.Sprintf("unknown validator status: %s", validator.Status)) } } // begin unbonding part or all of a delegation -func (k Keeper) Undelegate(ctx sdk.Context, delAddr sdk.AccAddress, - valAddr sdk.ValAddress, sharesAmount sdk.Dec) (completionTime time.Time, sdkErr sdk.Error) { +func (k Keeper) Undelegate( + ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress, sharesAmount sdk.Dec, +) (completionTime time.Time, sdkErr sdk.Error) { // create the unbonding delegation - completionTime, height, completeNow := k.getBeginInfo(ctx, valAddr) + completionTime, height := k.getUndelegateBeginInfo(ctx, valAddr) returnAmount, err := k.unbond(ctx, delAddr, valAddr, sharesAmount) if err != nil { return completionTime, err } - balance := sdk.NewCoin(k.BondDenom(ctx), returnAmount) - // no need to create the ubd object just complete now - if completeNow { - // track undelegation only when remaining or truncated shares are non-zero - if !balance.IsZero() { - if _, err := k.bankKeeper.UndelegateCoins(ctx, delAddr, sdk.Coins{balance}); err != nil { - return completionTime, err + if ctx.BlockHeight() < undelegatePatchHeight { + // Transactions prior undelegatePatchHeight must obey the previous logic + // where a undelegation can mature immediately if the validator is + // unbonded. + _, _, completeNow := k.getRedelegationBeginInfo(ctx, valAddr) + + // no need to create the ubd object just complete now + if completeNow { + balance := sdk.NewCoin(k.BondDenom(ctx), returnAmount) + + // track undelegation only when remaining or truncated shares are non-zero + if !balance.IsZero() { + if _, err := k.bankKeeper.UndelegateCoins(ctx, delAddr, sdk.Coins{balance}); err != nil { + return completionTime, err + } } - } - return completionTime, nil + return completionTime, nil + } } if k.HasMaxUnbondingDelegationEntries(ctx, delAddr, valAddr) { return time.Time{}, types.ErrMaxUnbondingDelegationEntries(k.Codespace()) } - ubd := k.SetUnbondingDelegationEntry(ctx, delAddr, - valAddr, height, completionTime, returnAmount) - + ubd := k.SetUnbondingDelegationEntry(ctx, delAddr, valAddr, height, completionTime, returnAmount) k.InsertUBDQueue(ctx, ubd, completionTime) + return completionTime, nil } @@ -684,7 +716,7 @@ func (k Keeper) BeginRedelegation(ctx sdk.Context, delAddr sdk.AccAddress, } // create the unbonding delegation - completionTime, height, completeNow := k.getBeginInfo(ctx, valSrcAddr) + completionTime, height, completeNow := k.getRedelegationBeginInfo(ctx, valSrcAddr) if completeNow { // no need to create the redelegation object return completionTime, nil From 032a40d9f005d35b2be5c1c4dfbd67cd7c50d8d5 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Wed, 29 May 2019 20:36:17 -0400 Subject: [PATCH 4/8] Update to make fork more clear --- x/staking/keeper/delegation.go | 69 ++++++++++++++++------------------ 1 file changed, 32 insertions(+), 37 deletions(-) diff --git a/x/staking/keeper/delegation.go b/x/staking/keeper/delegation.go index 9626d1f1ba97..fef46c36cb2c 100644 --- a/x/staking/keeper/delegation.go +++ b/x/staking/keeper/delegation.go @@ -550,7 +550,7 @@ func (k Keeper) unbond(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValA // getBeginInfo returns the completion time and height of a redelegation, along // with a boolean signaling if the redelegation is complete based on the source // validator. -func (k Keeper) getRedelegationBeginInfo( +func (k Keeper) getBeginInfo( ctx sdk.Context, valSrcAddr sdk.ValAddress, ) (completionTime time.Time, height int64, completeNow bool) { @@ -575,45 +575,25 @@ func (k Keeper) getRedelegationBeginInfo( } } -func (k Keeper) getUndelegateBeginInfo(ctx sdk.Context, valAddr sdk.ValAddress) (completionTime time.Time, height int64) { - validator, found := k.GetValidator(ctx, valAddr) - - // TODO: When would the validator not be found? - switch { - case !found || validator.Status == sdk.Bonded: - completionTime = ctx.BlockHeader().Time.Add(k.UnbondingTime(ctx)) - height = ctx.BlockHeight() - return completionTime, height - - case validator.Status == sdk.Unbonded: - return completionTime, height - - case validator.Status == sdk.Unbonding: - return validator.UnbondingCompletionTime, validator.UnbondingHeight - - default: - panic(fmt.Sprintf("unknown validator status: %s", validator.Status)) - } -} - -// begin unbonding part or all of a delegation +// Undelegate unbonds an amount of delegator shares from a given validator. It +// will verify that the unbonding entries between the delegator and validator +// are not exceeded and unbond the staked tokens (based on shares) by creating +// an unbonding object and inserting it into the unbonding queue which will be +// processed during the staking EndBlocker. func (k Keeper) Undelegate( ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress, sharesAmount sdk.Dec, -) (completionTime time.Time, sdkErr sdk.Error) { - - // create the unbonding delegation - completionTime, height := k.getUndelegateBeginInfo(ctx, valAddr) - - returnAmount, err := k.unbond(ctx, delAddr, valAddr, sharesAmount) - if err != nil { - return completionTime, err - } +) (time.Time, sdk.Error) { + // Undelegating must obey the unbonding period regardless of the validator's + // status for safety. Block heights prior to undelegatePatchHeight allowed + // delegates to unbond from an unbonded validator immediately. if ctx.BlockHeight() < undelegatePatchHeight { - // Transactions prior undelegatePatchHeight must obey the previous logic - // where a undelegation can mature immediately if the validator is - // unbonded. - _, _, completeNow := k.getRedelegationBeginInfo(ctx, valAddr) + completionTime, height, completeNow := k.getBeginInfo(ctx, valAddr) + + returnAmount, err := k.unbond(ctx, delAddr, valAddr, sharesAmount) + if err != nil { + return completionTime, err + } // no need to create the ubd object just complete now if completeNow { @@ -628,13 +608,28 @@ func (k Keeper) Undelegate( return completionTime, nil } + + if k.HasMaxUnbondingDelegationEntries(ctx, delAddr, valAddr) { + return time.Time{}, types.ErrMaxUnbondingDelegationEntries(k.Codespace()) + } + + ubd := k.SetUnbondingDelegationEntry(ctx, delAddr, valAddr, height, completionTime, returnAmount) + k.InsertUBDQueue(ctx, ubd, completionTime) + + return completionTime, nil + } + + returnAmount, err := k.unbond(ctx, delAddr, valAddr, sharesAmount) + if err != nil { + return time.Time{}, err } if k.HasMaxUnbondingDelegationEntries(ctx, delAddr, valAddr) { return time.Time{}, types.ErrMaxUnbondingDelegationEntries(k.Codespace()) } - ubd := k.SetUnbondingDelegationEntry(ctx, delAddr, valAddr, height, completionTime, returnAmount) + completionTime := ctx.BlockHeader().Time.Add(k.UnbondingTime(ctx)) + ubd := k.SetUnbondingDelegationEntry(ctx, delAddr, valAddr, ctx.BlockHeight(), completionTime, returnAmount) k.InsertUBDQueue(ctx, ubd, completionTime) return completionTime, nil From 40f1c2ced796d4902b0fd15e76d33043f8118a19 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Wed, 29 May 2019 20:37:22 -0400 Subject: [PATCH 5/8] Fix build --- x/staking/keeper/delegation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/staking/keeper/delegation.go b/x/staking/keeper/delegation.go index fef46c36cb2c..342c4a9560ca 100644 --- a/x/staking/keeper/delegation.go +++ b/x/staking/keeper/delegation.go @@ -711,7 +711,7 @@ func (k Keeper) BeginRedelegation(ctx sdk.Context, delAddr sdk.AccAddress, } // create the unbonding delegation - completionTime, height, completeNow := k.getRedelegationBeginInfo(ctx, valSrcAddr) + completionTime, height, completeNow := k.getBeginInfo(ctx, valSrcAddr) if completeNow { // no need to create the redelegation object return completionTime, nil From d5b495f5a92d1c82197e067a761b37bb49cbb86a Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Thu, 30 May 2019 09:38:57 -0400 Subject: [PATCH 6/8] Update staking tests --- x/staking/handler_test.go | 69 ++++++++++++++++++++++++++--- x/staking/keeper/delegation.go | 6 ++- x/staking/keeper/delegation_test.go | 20 +++------ 3 files changed, 74 insertions(+), 21 deletions(-) diff --git a/x/staking/handler_test.go b/x/staking/handler_test.go index 19865aeba19b..61b69d3ee5f2 100644 --- a/x/staking/handler_test.go +++ b/x/staking/handler_test.go @@ -796,16 +796,73 @@ func TestUnbondingFromUnbondingValidator(t *testing.T) { got = handleMsgUndelegate(ctx, msgUndelegateDelegator, keeper) require.True(t, got.IsOK(), "expected no error") - // move the Block time forward by one second - ctx = ctx.WithBlockTime(ctx.BlockHeader().Time.Add(time.Second * 1)) + ctx = ctx.WithBlockTime(ctx.BlockHeader().Time.Add(keeper.UnbondingTime(ctx))) + EndBlocker(ctx, keeper) + + // validate the unbonding object does not exist as it was never created + _, found := keeper.GetUnbondingDelegation(ctx, delegatorAddr, validatorAddr) + require.False(t, found, "unbonding object should not exist") +} + +func TestUnbondingFork(t *testing.T) { + ctx, _, keeper := keep.CreateTestInput(t, false, 1000) + validatorAddr, delegatorAddr := sdk.ValAddress(keep.Addrs[0]), keep.Addrs[1] + + // create the validator + msgCreateValidator := NewTestMsgCreateValidator(validatorAddr, keep.PKs[0], sdk.NewInt(10)) + got := handleMsgCreateValidator(ctx, msgCreateValidator, keeper) + require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator") + + // bond a delegator + msgDelegate := NewTestMsgDelegate(delegatorAddr, validatorAddr, sdk.NewInt(10)) + got = handleMsgDelegate(ctx, msgDelegate, keeper) + require.True(t, got.IsOK(), "expected ok, got %v", got) + + // unbond the delegator from the validator + unbondAmt := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(10)) + msgUndelegateDelegator := NewMsgUndelegate(delegatorAddr, validatorAddr, unbondAmt) + got = handleMsgUndelegate(ctx, msgUndelegateDelegator, keeper) + require.True(t, got.IsOK(), "expected no error") - // Run the EndBlocker EndBlocker(ctx, keeper) - // Check to make sure that the unbonding delegation is no longer in state - // (meaning it was deleted in the above EndBlocker) + // validate the unbonding object does not exist as it was never created _, found := keeper.GetUnbondingDelegation(ctx, delegatorAddr, validatorAddr) - require.False(t, found, "should be removed from state") + require.False(t, found, "unbonding object should not exist") + + // reproduce same sequence except past fork height + ctx, _, keeper = keep.CreateTestInput(t, false, 1000) + validatorAddr, delegatorAddr = sdk.ValAddress(keep.Addrs[0]), keep.Addrs[1] + ctx = ctx.WithBlockHeight(keep.UndelegatePatchHeight) + + // create the validator + msgCreateValidator = NewTestMsgCreateValidator(validatorAddr, keep.PKs[0], sdk.NewInt(10)) + got = handleMsgCreateValidator(ctx, msgCreateValidator, keeper) + require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator") + + // bond a delegator + msgDelegate = NewTestMsgDelegate(delegatorAddr, validatorAddr, sdk.NewInt(10)) + got = handleMsgDelegate(ctx, msgDelegate, keeper) + require.True(t, got.IsOK(), "expected ok, got %v", got) + + // unbond the delegator from the validator + msgUndelegateDelegator = NewMsgUndelegate(delegatorAddr, validatorAddr, unbondAmt) + got = handleMsgUndelegate(ctx, msgUndelegateDelegator, keeper) + require.True(t, got.IsOK(), "expected no error") + + EndBlocker(ctx, keeper) + + // validate the unbonding object exists prior to the unbonding period expiring + _, found = keeper.GetUnbondingDelegation(ctx, delegatorAddr, validatorAddr) + require.True(t, found, "unbonding object should exist") + + // move to past unbonding period + ctx = ctx.WithBlockTime(ctx.BlockHeader().Time.Add(keeper.UnbondingTime(ctx))) + EndBlocker(ctx, keeper) + + // validate the unbonding object no longer exists as the unbonding period has expired + _, found = keeper.GetUnbondingDelegation(ctx, delegatorAddr, validatorAddr) + require.False(t, found, "unbonding object should not exist") } func TestRedelegationPeriod(t *testing.T) { diff --git a/x/staking/keeper/delegation.go b/x/staking/keeper/delegation.go index 342c4a9560ca..f505555ae715 100644 --- a/x/staking/keeper/delegation.go +++ b/x/staking/keeper/delegation.go @@ -9,7 +9,9 @@ import ( "github.com/cosmos/cosmos-sdk/x/staking/types" ) -const undelegatePatchHeight = 50 +// UndelegatePatchHeight reflects the height at which to switch to the +// undelegating patch. +const UndelegatePatchHeight = 50 // return a specific delegation func (k Keeper) GetDelegation(ctx sdk.Context, @@ -587,7 +589,7 @@ func (k Keeper) Undelegate( // Undelegating must obey the unbonding period regardless of the validator's // status for safety. Block heights prior to undelegatePatchHeight allowed // delegates to unbond from an unbonded validator immediately. - if ctx.BlockHeight() < undelegatePatchHeight { + if ctx.BlockHeight() < UndelegatePatchHeight { completionTime, height, completeNow := k.getBeginInfo(ctx, valAddr) returnAmount, err := k.unbond(ctx, delAddr, valAddr, sharesAmount) diff --git a/x/staking/keeper/delegation_test.go b/x/staking/keeper/delegation_test.go index bc9d8e72bfae..700350ccc583 100644 --- a/x/staking/keeper/delegation_test.go +++ b/x/staking/keeper/delegation_test.go @@ -351,12 +351,10 @@ func TestUndelegateFromUnbondingValidator(t *testing.T) { require.True(t, blockTime.Add(params.UnbondingTime).Equal(validator.UnbondingCompletionTime)) //change the context - header = ctx.BlockHeader() - blockHeight2 := int64(20) - header.Height = blockHeight2 - blockTime2 := time.Unix(444, 0) - header.Time = blockTime2 - ctx = ctx.WithBlockHeader(header) + blockHeight2 := int64(2000000) + blockTime2 := time.Unix(444, 0).UTC() + ctx = ctx.WithBlockHeight(blockHeight2) + ctx = ctx.WithBlockTime(blockTime2) // unbond some of the other delegation's shares _, err = keeper.Undelegate(ctx, addrDels[0], addrVals[0], sdk.NewDec(6)) @@ -367,8 +365,8 @@ func TestUndelegateFromUnbondingValidator(t *testing.T) { require.True(t, found) require.Len(t, ubd.Entries, 1) require.True(t, ubd.Entries[0].Balance.Equal(sdk.NewInt(6))) - assert.Equal(t, blockHeight, ubd.Entries[0].CreationHeight) - assert.True(t, blockTime.Add(params.UnbondingTime).Equal(ubd.Entries[0].CompletionTime)) + assert.Equal(t, blockHeight2, ubd.Entries[0].CreationHeight) + assert.True(t, blockTime2.Add(params.UnbondingTime).Equal(ubd.Entries[0].CompletionTime)) } func TestUndelegateFromUnbondedValidator(t *testing.T) { @@ -377,7 +375,7 @@ func TestUndelegateFromUnbondedValidator(t *testing.T) { startTokens := sdk.TokensFromTendermintPower(20) pool.NotBondedTokens = startTokens - //create a validator with a self-delegation + // create a validator with a self-delegation validator := types.NewValidator(addrVals[0], PKs[0], types.Description{}) valTokens := sdk.TokensFromTendermintPower(10) @@ -432,10 +430,6 @@ func TestUndelegateFromUnbondedValidator(t *testing.T) { _, err = keeper.Undelegate(ctx, addrDels[0], addrVals[0], unbondTokens.ToDec()) require.NoError(t, err) - // no ubd should have been found, coins should have been returned direcly to account - ubd, found := keeper.GetUnbondingDelegation(ctx, addrDels[0], addrVals[0]) - require.False(t, found, "%v", ubd) - // unbond rest of the other delegation's shares remainingTokens := delTokens.Sub(unbondTokens) _, err = keeper.Undelegate(ctx, addrDels[0], addrVals[0], remainingTokens.ToDec()) From 91e494c0c5b02a28672b2679ba22110f1383cea6 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Thu, 30 May 2019 10:34:23 -0400 Subject: [PATCH 7/8] Update fork height to 482100 --- x/staking/keeper/delegation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/staking/keeper/delegation.go b/x/staking/keeper/delegation.go index f505555ae715..9559398923db 100644 --- a/x/staking/keeper/delegation.go +++ b/x/staking/keeper/delegation.go @@ -11,7 +11,7 @@ import ( // UndelegatePatchHeight reflects the height at which to switch to the // undelegating patch. -const UndelegatePatchHeight = 50 +const UndelegatePatchHeight = 482100 // return a specific delegation func (k Keeper) GetDelegation(ctx sdk.Context, From 5941b74df9e69e698cd5a024dce15f90928d8d67 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Thu, 30 May 2019 13:41:54 -0400 Subject: [PATCH 8/8] Add CHANGELOG entry --- CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 27eea2c0b80e..9d7328ca8f50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,14 @@ # Changelog +## 0.34.6 + +### Bug Fixes + +#### SDK + +* Unbonding from a validator is now only considered "complete" after the full +unbonding period has elapsed regardless of the validator's status. + ## 0.34.5 ### Bug Fixes