From 37c3c65eda38385929dfa92da6b362adc5c1372a Mon Sep 17 00:00:00 2001 From: kogisin Date: Thu, 14 Apr 2022 13:59:08 +0900 Subject: [PATCH 1/4] fix: fix gas consumption issue for vote condition type (cherry picked from commit 1afc2b671041c81ef0f8bb9f7e218780dac051b0) --- x/claim/keeper/claim.go | 15 ++++++-- x/claim/keeper/claim_test.go | 61 +++++++++++++++++++++++++++++++ x/claim/types/expected_keepers.go | 3 +- 3 files changed, 74 insertions(+), 5 deletions(-) diff --git a/x/claim/keeper/claim.go b/x/claim/keeper/claim.go index 6a4574e64..030542584 100644 --- a/x/claim/keeper/claim.go +++ b/x/claim/keeper/claim.go @@ -66,11 +66,15 @@ func (k Keeper) ValidateCondition(ctx sdk.Context, recipient sdk.AccAddress, ct switch ct { case types.ConditionTypeDeposit: + // Expect this condition to be executed with multiple messages (MsgDeposit + MsgClaim) + // in a single transaction because deposit request gets deleted at the begin blocker if len(k.liquidityKeeper.GetDepositRequestsByDepositor(ctx, recipient)) != 0 { ok = true } case types.ConditionTypeSwap: + // Expect this condition to be executed with multiple messages (MsgLimitOrder or MsgMarketOrder + MsgClaim) + // in a single transaction because order request gets deleted at the begin blocker after order lifespan if len(k.liquidityKeeper.GetOrdersByOrderer(ctx, recipient)) != 0 { ok = true } @@ -84,10 +88,13 @@ func (k Keeper) ValidateCondition(ctx sdk.Context, recipient sdk.AccAddress, ct } case types.ConditionTypeVote: - k.govKeeper.IterateAllVotes(ctx, func(vote govtypes.Vote) (stop bool) { - if vote.Voter == recipient.String() { - ok = true - return true + k.govKeeper.IterateProposals(ctx, func(proposal govtypes.Proposal) (stop bool) { + if proposal.Status == govtypes.StatusVotingPeriod { + _, found := k.govKeeper.GetVote(ctx, proposal.ProposalId, recipient) + if found { + ok = true + return true + } } return false }) diff --git a/x/claim/keeper/claim_test.go b/x/claim/keeper/claim_test.go index b338c59bc..e3e454ffa 100644 --- a/x/claim/keeper/claim_test.go +++ b/x/claim/keeper/claim_test.go @@ -494,3 +494,64 @@ func (s *KeeperTestSuite) TestClaim_Partial_TerminatAirdrop() { feePool := s.app.DistrKeeper.GetFeePool(s.ctx) s.Require().False(feePool.CommunityPool.IsZero()) } + +func (s *KeeperTestSuite) TestSimulateGasUsage_VoteCondition() { + // Create an airdrop + sourceAddr := s.addr(0) + airdrop := s.createAirdrop( + 1, + sourceAddr, + utils.ParseCoins("100000000000denom1"), + []types.ConditionType{ + types.ConditionTypeDeposit, + types.ConditionTypeSwap, + types.ConditionTypeLiquidStake, + types.ConditionTypeVote, + }, + s.ctx.BlockTime(), + s.ctx.BlockTime().AddDate(0, 6, 0), + true, + ) + + // Submit governance proposals + s.createTextProposal(sourceAddr, "Text1", "Description") + s.createTextProposal(sourceAddr, "Text2", "Description") + + recipients := []sdk.AccAddress{} + numRecipients := 10000 + + // Claim records for all recipients + for i := 1; i <= numRecipients; i++ { + recipient := s.addr(i) + recipients = append(recipients, recipient) + + s.createClaimRecord( + airdrop.Id, + recipient, + utils.ParseCoins("1000000denom1"), + utils.ParseCoins("1000000denom1"), + []types.ConditionType{}, + ) + + _, found := s.keeper.GetClaimRecordByRecipient(s.ctx, airdrop.Id, recipient) + s.Require().True(found) + } + + for _, recipient := range recipients[:5000] { + s.vote(recipient, 1, govtypes.OptionYes) + } + + // Vote proposal and claim condition + for i, recipient := range recipients[5000:] { + gasConsumedBefore := s.ctx.GasMeter().GasConsumed() + + s.vote(recipient, 2, govtypes.OptionYes) + + _, err := s.keeper.Claim(s.ctx, types.NewMsgClaim(airdrop.Id, recipient, types.ConditionTypeVote)) + s.Require().NoError(err) + + gasConsumed := s.ctx.GasMeter().GasConsumed() + gasConsumed = gasConsumed - gasConsumedBefore + s.T().Logf("[%d] GasConsumed: %d\n", i+1, gasConsumed) + } +} diff --git a/x/claim/types/expected_keepers.go b/x/claim/types/expected_keepers.go index 24dfee60f..fcacd4e0e 100644 --- a/x/claim/types/expected_keepers.go +++ b/x/claim/types/expected_keepers.go @@ -26,7 +26,8 @@ type DistrKeeper interface { } type GovKeeper interface { - IterateAllVotes(ctx sdk.Context, cb func(vote govtypes.Vote) (stop bool)) + IterateProposals(ctx sdk.Context, cb func(proposal govtypes.Proposal) (stop bool)) + GetVote(ctx sdk.Context, proposalID uint64, voterAddr sdk.AccAddress) (vote govtypes.Vote, found bool) } // LiquidityKeeper defines the expected interface needed to check the condition. From 0a6b9ecc625eea44dfb0667980c1d5be0cfe6ea6 Mon Sep 17 00:00:00 2001 From: queencre Date: Thu, 14 Apr 2022 14:53:25 +0900 Subject: [PATCH 2/4] chore: add upgrade height and add upgrade gas consumption test --- app/upgrades/mainnet/v1.1.0/upgrade.go | 4 ++ x/claim/keeper/claim.go | 28 ++++++--- x/claim/keeper/claim_test.go | 82 ++++++++++++++++++++++++++ x/claim/types/expected_keepers.go | 2 + 4 files changed, 109 insertions(+), 7 deletions(-) create mode 100644 app/upgrades/mainnet/v1.1.0/upgrade.go diff --git a/app/upgrades/mainnet/v1.1.0/upgrade.go b/app/upgrades/mainnet/v1.1.0/upgrade.go new file mode 100644 index 000000000..17e36cd78 --- /dev/null +++ b/app/upgrades/mainnet/v1.1.0/upgrade.go @@ -0,0 +1,4 @@ +package v1_1_0 + +// UpgradeHeight defines the upgrade height that is used in claim module +const UpgradeHeight = int64(48000) diff --git a/x/claim/keeper/claim.go b/x/claim/keeper/claim.go index 030542584..a858f3c01 100644 --- a/x/claim/keeper/claim.go +++ b/x/claim/keeper/claim.go @@ -7,6 +7,7 @@ import ( sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" + v1_1_0 "github.com/crescent-network/crescent/app/upgrades/mainnet/v1.1.0" "github.com/crescent-network/crescent/x/claim/types" ) @@ -88,16 +89,29 @@ func (k Keeper) ValidateCondition(ctx sdk.Context, recipient sdk.AccAddress, ct } case types.ConditionTypeVote: - k.govKeeper.IterateProposals(ctx, func(proposal govtypes.Proposal) (stop bool) { - if proposal.Status == govtypes.StatusVotingPeriod { - _, found := k.govKeeper.GetVote(ctx, proposal.ProposalId, recipient) - if found { + // IterateAllVotes consumes more gas as a number of votes increase. + // To prevent from letting some airdrop recipients experience out of gas issue, + // an upgrade is inevitable after the UpgradeHeight. + if ctx.BlockHeight() < v1_1_0.UpgradeHeight { + k.govKeeper.IterateAllVotes(ctx, func(vote govtypes.Vote) (stop bool) { + if vote.Voter == recipient.String() { ok = true return true } - } - return false - }) + return false + }) + } else { + k.govKeeper.IterateProposals(ctx, func(proposal govtypes.Proposal) (stop bool) { + if proposal.Status == govtypes.StatusVotingPeriod { + _, found := k.govKeeper.GetVote(ctx, proposal.ProposalId, recipient) + if found { + ok = true + return true + } + } + return false + }) + } } if !ok { diff --git a/x/claim/keeper/claim_test.go b/x/claim/keeper/claim_test.go index e3e454ffa..10158255a 100644 --- a/x/claim/keeper/claim_test.go +++ b/x/claim/keeper/claim_test.go @@ -4,6 +4,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" + v1_1_0 "github.com/crescent-network/crescent/app/upgrades/mainnet/v1.1.0" utils "github.com/crescent-network/crescent/types" "github.com/crescent-network/crescent/x/claim" "github.com/crescent-network/crescent/x/claim/types" @@ -555,3 +556,84 @@ func (s *KeeperTestSuite) TestSimulateGasUsage_VoteCondition() { s.T().Logf("[%d] GasConsumed: %d\n", i+1, gasConsumed) } } + +func (s *KeeperTestSuite) TestGasConsumption_Upgrade_v1_0_0() { + // Create an airdrop + sourceAddr := s.addr(0) + airdrop := s.createAirdrop( + 1, + sourceAddr, + utils.ParseCoins("100000000000denom1"), + []types.ConditionType{ + types.ConditionTypeDeposit, + types.ConditionTypeSwap, + types.ConditionTypeLiquidStake, + types.ConditionTypeVote, + }, + s.ctx.BlockTime(), + s.ctx.BlockTime().AddDate(0, 6, 0), + true, + ) + + // Submit governance proposals + s.createTextProposal(sourceAddr, "Text1", "Description") + s.createTextProposal(sourceAddr, "Text2", "Description") + + recipients := []sdk.AccAddress{} + numRecipients := 100100 + + // Claim records for all recipients + for i := 1; i <= numRecipients; i++ { + recipient := s.addr(i) + recipients = append(recipients, recipient) + + s.createClaimRecord( + airdrop.Id, + recipient, + utils.ParseCoins("1000000denom1"), + utils.ParseCoins("1000000denom1"), + []types.ConditionType{}, + ) + + _, found := s.keeper.GetClaimRecordByRecipient(s.ctx, airdrop.Id, recipient) + s.Require().True(found) + } + + for _, recipient := range recipients[:100000] { + s.vote(recipient, 1, govtypes.OptionYes) + } + + // Expected gas threshold + expConsumedGasLimit := sdk.Gas(100_000) + + // Vote proposal and claim condition + for _, recipient := range recipients[100000:100050] { + gasConsumedBefore := s.ctx.GasMeter().GasConsumed() + + s.vote(recipient, 2, govtypes.OptionYes) + + _, err := s.keeper.Claim(s.ctx, types.NewMsgClaim(airdrop.Id, recipient, types.ConditionTypeVote)) + s.Require().NoError(err) + + gasConsumed := s.ctx.GasMeter().GasConsumed() + gasConsumed = gasConsumed - gasConsumedBefore + s.Require().GreaterOrEqual(gasConsumed, expConsumedGasLimit) + } + + // Set upgrade height + s.ctx = s.ctx.WithBlockHeight(v1_1_0.UpgradeHeight) + + // Vote proposal and claim condition + for _, recipient := range recipients[100050:100100] { + gasConsumedBefore := s.ctx.GasMeter().GasConsumed() + + s.vote(recipient, 2, govtypes.OptionYes) + + _, err := s.keeper.Claim(s.ctx, types.NewMsgClaim(airdrop.Id, recipient, types.ConditionTypeVote)) + s.Require().NoError(err) + + gasConsumed := s.ctx.GasMeter().GasConsumed() + gasConsumed = gasConsumed - gasConsumedBefore + s.Require().LessOrEqual(gasConsumed, expConsumedGasLimit) + } +} diff --git a/x/claim/types/expected_keepers.go b/x/claim/types/expected_keepers.go index fcacd4e0e..a1d8739e3 100644 --- a/x/claim/types/expected_keepers.go +++ b/x/claim/types/expected_keepers.go @@ -28,6 +28,8 @@ type DistrKeeper interface { type GovKeeper interface { IterateProposals(ctx sdk.Context, cb func(proposal govtypes.Proposal) (stop bool)) GetVote(ctx sdk.Context, proposalID uint64, voterAddr sdk.AccAddress) (vote govtypes.Vote, found bool) + // Note that this function is only used before the UpgradeHeight defined in app/upgrades/v1.1.0 + IterateAllVotes(ctx sdk.Context, cb func(vote govtypes.Vote) (stop bool)) } // LiquidityKeeper defines the expected interface needed to check the condition. From 7658ef635f6379750e736d50c8bb035ba61700ca Mon Sep 17 00:00:00 2001 From: queencre Date: Thu, 14 Apr 2022 15:02:55 +0900 Subject: [PATCH 3/4] docs: update CHANGELOD.md and README.md --- CHANGELOG.md | 11 ++++++++++- README.md | 7 +++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 54a3b30fc..1116b2b5a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,14 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [Unreleased] +## [v1.1.0] - 2022-04-14 + +### State Machine Breaking + +Running a full node will encounter wrong app hash issue if it doesn't upgrade to this version prior to `UpgradeHeight (48000)`. Instead of going through on-chain governance proposal by using `UpgradeProposal`, this upgrade mechanism is chosen as it is security hot fix that is better to be fixed as soon as it can and also it is directly related to governance proposal. + +* (x/claim) [\#23](https://github.com/crescent-network/crescent/pull/23) Fix gas consumption issue for `ConditionTypeVote`. `UpgradeHeight` is set as `48000`. + ## [v1.0.0] - 2022-04-12 ### Features @@ -54,4 +62,5 @@ Ref: https://keepachangelog.com/en/1.0.0/ * `x/bank` feat: Add dynamic blockedAddrs [Unreleased]: https://github.com/crescent-network/crescent/compare/v1.0.0...HEAD -[v1.0.0]: https://github.com/crescent-network/crescent/releases/tag/v1.0.0 \ No newline at end of file +[v1.0.0]: https://github.com/crescent-network/crescent/releases/tag/v1.0.0 +[v1.1.0]: https://github.com/crescent-network/crescent/releases/tag/v1.1.0 \ No newline at end of file diff --git a/README.md b/README.md index 3368ba317..5fed4448b 100644 --- a/README.md +++ b/README.md @@ -45,11 +45,11 @@ If you haven't already, install Go by following the [official docs](https://gola **Step 2. Get Crescent Core source code** -Use `git` to retrieve Crescent Core from the [official repo](https://github.com/crescent-network/crescent/) and checkout the `release/v1.0.x` branch. This branch contains the latest release, which will install the `crescentd` binary. +Use `git` to retrieve Crescent Core from the [official repo](https://github.com/crescent-network/crescent/) and checkout the `release/v1.1.x` branch. This branch contains the latest release, which will install the `crescentd` binary. ```bash git clone https://github.com/crescent-network/crescent.git -cd crescent && git checkout release/v1.0.x +cd crescent && git checkout release/v1.1.x make install ``` @@ -74,6 +74,9 @@ Crescent Core uses a customized Cosmos SDK. Please check the differences on [her The documentation is available in [docs](docs) directory. If you are a developer interested in technical specification, see inside each `x/{module}`'s `spec` directory. +* [Crescent Official Docs](https://docs.crescent.network/) +* [Swagger API Docs](https://app.swaggerhub.com/apis-docs/crescent/crescent) + ## Community * [Official Website](https://crescent.network/) From 646b25227d6ac93db66a9ff50f86c1bd9ddfd516 Mon Sep 17 00:00:00 2001 From: queencre Date: Thu, 14 Apr 2022 16:00:09 +0900 Subject: [PATCH 4/4] test: decrease a number of recipients --- x/claim/keeper/claim_test.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/x/claim/keeper/claim_test.go b/x/claim/keeper/claim_test.go index 10158255a..dd8542570 100644 --- a/x/claim/keeper/claim_test.go +++ b/x/claim/keeper/claim_test.go @@ -542,8 +542,14 @@ func (s *KeeperTestSuite) TestSimulateGasUsage_VoteCondition() { s.vote(recipient, 1, govtypes.OptionYes) } + // Set upgrade height + s.ctx = s.ctx.WithBlockHeight(v1_1_0.UpgradeHeight) + + // Expected gas threshold + expConsumedGasLimit := sdk.Gas(100_000) + // Vote proposal and claim condition - for i, recipient := range recipients[5000:] { + for _, recipient := range recipients[5000:] { gasConsumedBefore := s.ctx.GasMeter().GasConsumed() s.vote(recipient, 2, govtypes.OptionYes) @@ -553,7 +559,7 @@ func (s *KeeperTestSuite) TestSimulateGasUsage_VoteCondition() { gasConsumed := s.ctx.GasMeter().GasConsumed() gasConsumed = gasConsumed - gasConsumedBefore - s.T().Logf("[%d] GasConsumed: %d\n", i+1, gasConsumed) + s.Require().LessOrEqual(gasConsumed, expConsumedGasLimit) } } @@ -580,7 +586,7 @@ func (s *KeeperTestSuite) TestGasConsumption_Upgrade_v1_0_0() { s.createTextProposal(sourceAddr, "Text2", "Description") recipients := []sdk.AccAddress{} - numRecipients := 100100 + numRecipients := 10100 // Claim records for all recipients for i := 1; i <= numRecipients; i++ { @@ -599,7 +605,7 @@ func (s *KeeperTestSuite) TestGasConsumption_Upgrade_v1_0_0() { s.Require().True(found) } - for _, recipient := range recipients[:100000] { + for _, recipient := range recipients[:10000] { s.vote(recipient, 1, govtypes.OptionYes) } @@ -607,7 +613,7 @@ func (s *KeeperTestSuite) TestGasConsumption_Upgrade_v1_0_0() { expConsumedGasLimit := sdk.Gas(100_000) // Vote proposal and claim condition - for _, recipient := range recipients[100000:100050] { + for _, recipient := range recipients[10000:10050] { gasConsumedBefore := s.ctx.GasMeter().GasConsumed() s.vote(recipient, 2, govtypes.OptionYes) @@ -624,7 +630,7 @@ func (s *KeeperTestSuite) TestGasConsumption_Upgrade_v1_0_0() { s.ctx = s.ctx.WithBlockHeight(v1_1_0.UpgradeHeight) // Vote proposal and claim condition - for _, recipient := range recipients[100050:100100] { + for _, recipient := range recipients[10050:10100] { gasConsumedBefore := s.ctx.GasMeter().GasConsumed() s.vote(recipient, 2, govtypes.OptionYes)