From 43e059856023585f0b17b674aaa9b2972e08bc1a Mon Sep 17 00:00:00 2001 From: likhita-809 <78951027+likhita-809@users.noreply.github.com> Date: Fri, 9 Jul 2021 20:08:25 +0530 Subject: [PATCH 1/4] types: Inconsistent limit on InfiniteGasMeter and add GasLeft func to GasMeter (#9651) --- CHANGELOG.md | 1 + store/types/gas.go | 14 +++++++++++++- store/types/gas_test.go | 15 +++++++++++++-- x/auth/ante/setup_test.go | 6 ++++-- 4 files changed, 31 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ccbc6d66f0..da47d74fbf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,6 +75,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes +* [\#9651](https://github.com/cosmos/cosmos-sdk/pull/9651) Change inconsistent limit of `0` to `MaxUint64` on InfiniteGasMeter and add GasRemaining func to GasMeter. * [\#9639](https://github.com/cosmos/cosmos-sdk/pull/9639) Check store keys length before accessing them by making sure that `key` is of length `m+1` (for `key[n:m]`) * (types) [\#9627](https://github.com/cosmos/cosmos-sdk/pull/9627) Fix nil pointer panic on `NewBigIntFromInt` * (x/genutil) [\#9574](https://github.com/cosmos/cosmos-sdk/pull/9575) Actually use the `gentx` client tx flags (like `--keyring-dir`) diff --git a/store/types/gas.go b/store/types/gas.go index bf44e717d0..dc7e44b3c2 100644 --- a/store/types/gas.go +++ b/store/types/gas.go @@ -41,6 +41,7 @@ type ErrorGasOverflow struct { type GasMeter interface { GasConsumed() Gas GasConsumedToLimit() Gas + GasRemaining() Gas Limit() Gas ConsumeGas(amount Gas, descriptor string) RefundGas(amount Gas, descriptor string) @@ -66,6 +67,13 @@ func (g *basicGasMeter) GasConsumed() Gas { return g.consumed } +func (g *basicGasMeter) GasRemaining() Gas { + if g.IsPastLimit() { + return 0 + } + return g.limit - g.consumed +} + func (g *basicGasMeter) Limit() Gas { return g.limit } @@ -145,8 +153,12 @@ func (g *infiniteGasMeter) GasConsumedToLimit() Gas { return g.consumed } +func (g *infiniteGasMeter) GasRemaining() Gas { + return math.MaxUint64 +} + func (g *infiniteGasMeter) Limit() Gas { - return 0 + return math.MaxUint64 } func (g *infiniteGasMeter) ConsumeGas(amount Gas, descriptor string) { diff --git a/store/types/gas_test.go b/store/types/gas_test.go index 8a02df4cfa..f4b5a6abe5 100644 --- a/store/types/gas_test.go +++ b/store/types/gas_test.go @@ -10,13 +10,16 @@ import ( func TestInfiniteGasMeter(t *testing.T) { t.Parallel() meter := NewInfiniteGasMeter() - require.Equal(t, uint64(0), meter.Limit()) + require.Equal(t, uint64(math.MaxUint64), meter.Limit()) + require.Equal(t, uint64(math.MaxUint64), meter.GasRemaining()) require.Equal(t, uint64(0), meter.GasConsumed()) require.Equal(t, uint64(0), meter.GasConsumedToLimit()) meter.ConsumeGas(10, "consume 10") + require.Equal(t, uint64(math.MaxUint64), meter.GasRemaining()) require.Equal(t, uint64(10), meter.GasConsumed()) require.Equal(t, uint64(10), meter.GasConsumedToLimit()) meter.RefundGas(1, "refund 1") + require.Equal(t, uint64(math.MaxUint64), meter.GasRemaining()) require.Equal(t, uint64(9), meter.GasConsumed()) require.False(t, meter.IsPastLimit()) require.False(t, meter.IsOutOfGas()) @@ -48,6 +51,7 @@ func TestGasMeter(t *testing.T) { used += usage require.NotPanics(t, func() { meter.ConsumeGas(usage, "") }, "Not exceeded limit but panicked. tc #%d, usage #%d", tcnum, unum) require.Equal(t, used, meter.GasConsumed(), "Gas consumption not match. tc #%d, usage #%d", tcnum, unum) + require.Equal(t, tc.limit-used, meter.GasRemaining(), "Gas left not match. tc #%d, usage #%d", tcnum, unum) require.Equal(t, used, meter.GasConsumedToLimit(), "Gas consumption (to limit) not match. tc #%d, usage #%d", tcnum, unum) require.False(t, meter.IsPastLimit(), "Not exceeded limit but got IsPastLimit() true") if unum < len(tc.usage)-1 { @@ -60,13 +64,20 @@ func TestGasMeter(t *testing.T) { require.Panics(t, func() { meter.ConsumeGas(1, "") }, "Exceeded but not panicked. tc #%d", tcnum) require.Equal(t, meter.GasConsumedToLimit(), meter.Limit(), "Gas consumption (to limit) not match limit") require.Equal(t, meter.GasConsumed(), meter.Limit()+1, "Gas consumption not match limit+1") + require.Equal(t, uint64(0), meter.GasRemaining()) require.NotPanics(t, func() { meter.RefundGas(1, "refund 1") }) - require.Equal(t, meter.GasConsumed(), meter.Limit(), "Gas consumption not match limit+1") + require.Equal(t, meter.GasConsumed(), meter.Limit(), "Gas consumption not match with limit") + require.Equal(t, uint64(0), meter.GasRemaining()) require.Panics(t, func() { meter.RefundGas(meter.GasConsumed()+1, "refund greater than consumed") }) + require.NotPanics(t, func() { meter.RefundGas(meter.GasConsumed(), "refund consumed gas") }) + require.Equal(t, meter.Limit(), meter.GasRemaining()) + meter2 := NewGasMeter(math.MaxUint64) + require.Equal(t, uint64(math.MaxUint64), meter2.GasRemaining()) meter2.ConsumeGas(Gas(math.MaxUint64/2), "consume half max uint64") + require.Equal(t, Gas(math.MaxUint64-(math.MaxUint64/2)), meter2.GasRemaining()) require.Panics(t, func() { meter2.ConsumeGas(Gas(math.MaxUint64/2)+2, "panic") }) } } diff --git a/x/auth/ante/setup_test.go b/x/auth/ante/setup_test.go index 4942665cac..86c633d899 100644 --- a/x/auth/ante/setup_test.go +++ b/x/auth/ante/setup_test.go @@ -1,6 +1,8 @@ package ante_test import ( + "math" + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" "github.com/cosmos/cosmos-sdk/testutil/testdata" sdk "github.com/cosmos/cosmos-sdk/types" @@ -33,8 +35,8 @@ func (suite *AnteTestSuite) TestSetup() { // Set height to non-zero value for GasMeter to be set suite.ctx = suite.ctx.WithBlockHeight(1) - // Context GasMeter Limit not set - suite.Require().Equal(uint64(0), suite.ctx.GasMeter().Limit(), "GasMeter set with limit before setup") + // Context GasMeter Limit set to MaxUint64 + suite.Require().Equal(uint64(math.MaxUint64), suite.ctx.GasMeter().Limit(), "GasMeter set with limit other than MaxUint64 before setup") newCtx, err := antehandler(suite.ctx, tx, false) suite.Require().Nil(err, "SetUpContextDecorator returned error") From c87d81e10b9049a35713ca2a7186a0f5e6891851 Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Fri, 9 Jul 2021 18:32:37 -0500 Subject: [PATCH 2/4] Switch `on` to `true` in cosmovisor readme --- cosmovisor/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cosmovisor/README.md b/cosmovisor/README.md index c150790c02..c32c398345 100644 --- a/cosmovisor/README.md +++ b/cosmovisor/README.md @@ -70,7 +70,7 @@ The `DAEMON` specific code and operations (e.g. tendermint config, the applicati Generally, the system requires that the system administrator place all relevant binaries on the disk before the upgrade happens. However, for people who don't need such control and want an easier setup (maybe they are syncing a non-validating fullnode and want to do little maintenance), there is another option. -If you set `DAEMON_ALLOW_DOWNLOAD_BINARIES=on`, and no local binary can be found when an upgrade is triggered, `cosmovisor` will attempt to download and install the binary itself. The plan stored in the upgrade module has an info field for arbitrary JSON. This info is expected to be outputed on the halt log message. There are two valid formats to specify a download in such a message: +If you set `DAEMON_ALLOW_DOWNLOAD_BINARIES=true`, and no local binary can be found when an upgrade is triggered, `cosmovisor` will attempt to download and install the binary itself. The plan stored in the upgrade module has an info field for arbitrary JSON. This info is expected to be outputed on the halt log message. There are two valid formats to specify a download in such a message: 1. Store an os/architecture -> binary URI map in the upgrade plan info field as JSON under the `"binaries"` key. For example: @@ -90,7 +90,7 @@ https://example.com/testnet-1001-info.json?checksum=sha256:deaaa99fda9407c4dbe1d This file contained in the link will be retrieved by [go-getter](https://github.com/hashicorp/go-getter) and the `"binaries"` field will be parsed as above. -If there is no local binary, `DAEMON_ALLOW_DOWNLOAD_BINARIES=on`, and we can access a canonical url for the new binary, then the `cosmovisor` will download it with [go-getter](https://github.com/hashicorp/go-getter) and unpack it into the `upgrades/` folder to be run as if we installed it manually. +If there is no local binary, `DAEMON_ALLOW_DOWNLOAD_BINARIES=true`, and we can access a canonical url for the new binary, then the `cosmovisor` will download it with [go-getter](https://github.com/hashicorp/go-getter) and unpack it into the `upgrades/` folder to be run as if we installed it manually. Note that for this mechanism to provide strong security guarantees, all URLs should include a SHA 256/512 checksum. This ensures that no false binary is run, even if someone hacks the server or hijacks the DNS. `go-getter` will always ensure the downloaded file matches the checksum if it is provided. `go-getter` will also handle unpacking archives into directories (in this case the download link should point to a `zip` file of all data in the `bin` directory). From a67ec1ceaf397e914466c039dee552f3f38e36b9 Mon Sep 17 00:00:00 2001 From: John Kemp Date: Mon, 12 Jul 2021 04:51:36 -0400 Subject: [PATCH 3/4] perf: Change localnet CIDR from /16 (65k addresses) to /25 (128 addresses) to reduce docker network conflicts (#9667) Changed the localnet network from /16 to /25 reducing IP address space to 128 which should be sufficient for localnet ## Description Closes: #9646 --- ### 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 - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] 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) - [ ] 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` - [ ] 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 - [x] manually tested (if applicable) --- docker-compose.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker-compose.yml b/docker-compose.yml index d335b9a06c..cd89e459fd 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -71,4 +71,4 @@ networks: ipam: driver: default config: - - subnet: 192.168.10.0/16 + - subnet: 192.168.10.0/25 From d4d25f5e18e77e3040df49f0f37006fb50ffd501 Mon Sep 17 00:00:00 2001 From: Tyler <48813565+technicallyty@users.noreply.github.com> Date: Mon, 12 Jul 2021 09:54:07 -0700 Subject: [PATCH 4/4] fix: remove stores from renamed/deleted store upgrades (#9409) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description - stores that were renamed are now properly deleted - deleted/renamed and renamed stores are no longer added to `CommitInfo` - deleted/renamed stores are now properly removed from rootmulti store's memory ref: #7991 closes: N/A --- Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why. - [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [x] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] Updated relevant documentation (`docs/`) or specification (`x//spec/`) - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md` - [x] Re-reviewed `Files changed` in the Github PR explorer - [x] Review `Codecov Report` in the comment section below once CI passes --- store/rootmulti/store.go | 35 +++++++++++++++++++++++------ store/rootmulti/store_test.go | 4 ++-- x/upgrade/types/storeloader_test.go | 7 +++++- 3 files changed, 36 insertions(+), 10 deletions(-) diff --git a/store/rootmulti/store.go b/store/rootmulti/store.go index da095d6807..daf3c22ace 100644 --- a/store/rootmulti/store.go +++ b/store/rootmulti/store.go @@ -54,6 +54,7 @@ type Store struct { lazyLoading bool pruneHeights []int64 initialVersion int64 + removalMap map[types.StoreKey]bool traceWriter io.Writer traceContext types.TraceContext @@ -81,6 +82,7 @@ func NewStore(db dbm.DB) *Store { keysByName: make(map[string]types.StoreKey), pruneHeights: make([]int64, 0), listeners: make(map[types.StoreKey][]types.WriteListener), + removalMap: make(map[types.StoreKey]bool), } } @@ -210,9 +212,10 @@ func (rs *Store) loadVersion(ver int64, upgrades *types.StoreUpgrades) error { if err := deleteKVStore(store.(types.KVStore)); err != nil { return errors.Wrapf(err, "failed to delete store %s", key.Name()) } + rs.removalMap[key] = true } else if oldName := upgrades.RenamedFrom(key.Name()); oldName != "" { // handle renames specially - // make an unregistered key to satify loadCommitStore params + // make an unregistered key to satisfy loadCommitStore params oldKey := types.NewKVStoreKey(oldName) oldParams := storeParams oldParams.key = oldKey @@ -227,6 +230,11 @@ func (rs *Store) loadVersion(ver int64, upgrades *types.StoreUpgrades) error { if err := moveKVStoreData(oldStore.(types.KVStore), store.(types.KVStore)); err != nil { return errors.Wrapf(err, "failed to move store %s -> %s", oldName, key.Name()) } + + // add the old key so its deletion is committed + newStores[oldKey] = oldStore + // this will ensure it's not perpetually stored in commitInfo + rs.removalMap[oldKey] = true } } @@ -361,8 +369,19 @@ func (rs *Store) Commit() types.CommitID { previousHeight = rs.lastCommitInfo.GetVersion() version = previousHeight + 1 } + rs.lastCommitInfo = commitStores(version, rs.stores, rs.removalMap) - rs.lastCommitInfo = commitStores(version, rs.stores) + // remove remnants of removed stores + for sk := range rs.removalMap { + if _, ok := rs.stores[sk]; ok { + delete(rs.stores, sk) + delete(rs.storesParams, sk) + delete(rs.keysByName, sk.Name()) + } + } + + // reset the removalMap + rs.removalMap = make(map[types.StoreKey]bool) // Determine if pruneHeight height needs to be added to the list of heights to // be pruned, where pruneHeight = (commitHeight - 1) - KeepRecent. @@ -944,7 +963,7 @@ func getLatestVersion(db dbm.DB) int64 { } // Commits each store and returns a new commitInfo. -func commitStores(version int64, storeMap map[types.StoreKey]types.CommitKVStore) *types.CommitInfo { +func commitStores(version int64, storeMap map[types.StoreKey]types.CommitKVStore, removalMap map[types.StoreKey]bool) *types.CommitInfo { storeInfos := make([]types.StoreInfo, 0, len(storeMap)) for key, store := range storeMap { @@ -954,10 +973,12 @@ func commitStores(version int64, storeMap map[types.StoreKey]types.CommitKVStore continue } - si := types.StoreInfo{} - si.Name = key.Name() - si.CommitId = commitID - storeInfos = append(storeInfos, si) + if !removalMap[key] { + si := types.StoreInfo{} + si.Name = key.Name() + si.CommitId = commitID + storeInfos = append(storeInfos, si) + } } return &types.CommitInfo{ diff --git a/store/rootmulti/store_test.go b/store/rootmulti/store_test.go index cf7653b076..05817d6a84 100644 --- a/store/rootmulti/store_test.go +++ b/store/rootmulti/store_test.go @@ -300,8 +300,8 @@ func TestMultistoreLoadWithUpgrade(t *testing.T) { ci, err = getCommitInfo(db, 2) require.NoError(t, err) require.Equal(t, int64(2), ci.Version) - require.Equal(t, 4, len(ci.StoreInfos), ci.StoreInfos) - checkContains(t, ci.StoreInfos, []string{"store1", "restore2", "store3", "store4"}) + require.Equal(t, 3, len(ci.StoreInfos), ci.StoreInfos) + checkContains(t, ci.StoreInfos, []string{"store1", "restore2", "store4"}) } func TestParsePath(t *testing.T) { diff --git a/x/upgrade/types/storeloader_test.go b/x/upgrade/types/storeloader_test.go index ec2bfa824d..680e4d3568 100644 --- a/x/upgrade/types/storeloader_test.go +++ b/x/upgrade/types/storeloader_test.go @@ -90,6 +90,7 @@ func TestSetLoader(t *testing.T) { loadStoreKey string }{ "don't set loader": { + setLoader: nil, origStoreKey: "foo", loadStoreKey: "foo", }, @@ -145,9 +146,13 @@ func TestSetLoader(t *testing.T) { res := app.Commit() require.NotNil(t, res.Data) + // checking the case of the store being renamed + if tc.setLoader != nil { + checkStore(t, db, upgradeHeight, tc.origStoreKey, k, nil) + } + // check db is properly updated checkStore(t, db, upgradeHeight, tc.loadStoreKey, k, v) - checkStore(t, db, upgradeHeight, tc.loadStoreKey, []byte("foo"), nil) }) } }