Reestimate deterministic gas (#646)

CoreumFoundation · Oct 11, 2023 · e13e057 · e13e057
1 parent f54d178
commit e13e057
Show file tree

Hide file tree

Showing 9 changed files with 305 additions and 111 deletions.
diff --git a/cmd/cored/cosmoscmd/gas_price_test.go b/cmd/cored/cosmoscmd/gas_price_test.go
@@ -44,10 +44,10 @@ func TestAutoGasPrices(t *testing.T) {
 			},
 		},
 		{
-			name:  "specific gas prices is provided",
-			flags: []string{fmt.Sprintf("--gas-prices=0.1%s", denom), "--gas=100000"},
+			name:  "specific gas prices are provided",
+			flags: []string{fmt.Sprintf("--gas-prices=0.1%s", denom), "--gas=115000"},
 			feeAssertion: func(t *testing.T, fee sdk.Coins) {
-				assert.True(t, fee.IsEqual(sdk.NewCoins(sdk.NewCoin(denom, sdkmath.NewInt(10000)))))
+				assert.True(t, fee.IsEqual(sdk.NewCoins(sdk.NewCoin(denom, sdkmath.NewInt(11500)))))
 			},
 		},
 		{

diff --git a/integration-tests/modules/assetft_test.go b/integration-tests/modules/assetft_test.go
@@ -2352,7 +2352,7 @@ func TestAssetFTWhitelistIssuerAccount(t *testing.T) {
 	requireT.ErrorIs(err, cosmoserrors.ErrUnauthorized)
 }
 
-// TestBareToken checks non of the features will work if the flags are not set.
+// TestBareToken checks none of the features will work if the flags are not set.
 func TestBareToken(t *testing.T) {
 	t.Parallel()
 

diff --git a/integration-tests/modules/authz_test.go b/integration-tests/modules/authz_test.go
@@ -19,6 +19,53 @@ import (
 	"github.com/CoreumFoundation/coreum/v3/testutil/integration"
 )
 
+// TestAuthzDirectTransferFails if grantee sends message directly, without using authz.
+func TestAuthzDirectTransferFails(t *testing.T) {
+	t.Parallel()
+
+	ctx, chain := integrationtests.NewCoreumTestingContext(t)
+
+	requireT := require.New(t)
+
+	granter := chain.GenAccount()
+	grantee := chain.GenAccount()
+	recipient := chain.GenAccount()
+
+	amountToSend := sdkmath.NewInt(1_000)
+
+	// init the messages provisionally to use in the authztypes.MsgExec
+	msgBankSend := &banktypes.MsgSend{
+		FromAddress: granter.String(),
+		ToAddress:   recipient.String(),
+		// send a half to have 2 messages in the Exec
+		Amount: sdk.NewCoins(chain.NewCoin(amountToSend)),
+	}
+
+	chain.FundAccountWithOptions(ctx, t, granter, integration.BalancesOptions{
+		Messages: []sdk.Msg{
+			// Grantee signs the transaction, but granter is the sender, so fees are taken from the granter's account.
+			// In ante handler, fees are deducted before verifying signature, so funding granter to cover the fee is important,
+			// to verify that transaction is rejected due to invalid signature.
+			msgBankSend,
+		},
+		Amount: amountToSend,
+	})
+
+	// this is done because account must exist to send transaction
+	chain.FundAccountWithOptions(ctx, t, grantee, integration.BalancesOptions{
+		Amount: sdk.NewIntFromUint64(1),
+	})
+
+	// try to send from grantee directly
+	_, err := client.BroadcastTx(
+		ctx,
+		chain.ClientContext.WithFromAddress(grantee),
+		chain.TxFactory().WithGas(chain.GasLimitByMsgs(msgBankSend)),
+		msgBankSend,
+	)
+	requireT.ErrorIs(err, cosmoserrors.ErrInvalidPubKey)
+}
+
 // TestAuthz tests the authz module Grant/Execute/Revoke messages execution and their deterministic gas.
 func TestAuthz(t *testing.T) {
 	t.Parallel()
@@ -51,6 +98,15 @@ func TestAuthz(t *testing.T) {
 		Amount: sdk.NewCoins(chain.NewCoin(sdkmath.NewInt(1_000))),
 	}
 	execMsg := authztypes.NewMsgExec(grantee, []sdk.Msg{msgBankSend, msgBankSend})
+
+	chain.FundAccountWithOptions(ctx, t, granter, integration.BalancesOptions{
+		Messages: []sdk.Msg{
+			&authztypes.MsgGrant{},
+			&authztypes.MsgRevoke{},
+		},
+		Amount: totalAmountToSend,
+	})
+
 	chain.FundAccountWithOptions(ctx, t, grantee, integration.BalancesOptions{
 		Messages: []sdk.Msg{
 			msgBankSend,
@@ -85,15 +141,6 @@ func TestAuthz(t *testing.T) {
 	requireT.NoError(err)
 	requireT.Equal(1, len(gransRes.Grants))
 
-	// try to send from grantee directly
-	_, err = client.BroadcastTx(
-		ctx,
-		chain.ClientContext.WithFromAddress(grantee),
-		chain.TxFactory().WithGas(chain.GasLimitByMsgs(msgBankSend)),
-		msgBankSend,
-	)
-	requireT.ErrorIs(err, cosmoserrors.ErrInvalidPubKey)
-
 	// try to send using the authz
 	txResult, err = client.BroadcastTx(
 		ctx,

diff --git a/integration-tests/modules/bank_test.go b/integration-tests/modules/bank_test.go
@@ -493,23 +493,21 @@ func TestBankMultiSend(t *testing.T) {
 
 	amount := sdkmath.NewInt(1000)
 
-	issueMsgs := []sdk.Msg{
-		&assetfttypes.MsgIssue{
-			Issuer:        sender.String(),
-			Symbol:        "TOK1",
-			Subunit:       "tok1",
-			Precision:     1,
-			Description:   "TOK1 Description",
-			InitialAmount: amount,
-		},
-		&assetfttypes.MsgIssue{
-			Issuer:        sender.String(),
-			Symbol:        "TOK2",
-			Subunit:       "tok2",
-			Precision:     1,
-			Description:   "TOK2 Description",
-			InitialAmount: amount,
-		},
+	issueMsg1 := &assetfttypes.MsgIssue{
+		Issuer:        sender.String(),
+		Symbol:        "TOK1",
+		Subunit:       "tok1",
+		Precision:     1,
+		Description:   "TOK1 Description",
+		InitialAmount: amount,
+	}
+	issueMsg2 := &assetfttypes.MsgIssue{
+		Issuer:        sender.String(),
+		Symbol:        "TOK2",
+		Subunit:       "tok2",
+		Precision:     1,
+		Description:   "TOK2 Description",
+		InitialAmount: amount,
 	}
 
 	chain.FundAccountWithOptions(ctx, t, sender, integration.BalancesOptions{
@@ -521,25 +519,36 @@ func TestBankMultiSend(t *testing.T) {
 				{Coins: make(sdk.Coins, 2)},
 				{Coins: make(sdk.Coins, 2)},
 			},
-		}}, issueMsgs...),
-		Amount: chain.QueryAssetFTParams(ctx, t).IssueFee.Amount.MulRaw(int64(len(issueMsgs))),
+		}}, issueMsg1, issueMsg2),
+		Amount: chain.QueryAssetFTParams(ctx, t).IssueFee.Amount.MulRaw(2),
 	})
 
 	// Issue fungible tokens
 	res, err := client.BroadcastTx(
 		ctx,
 		chain.ClientContext.WithFromAddress(sender),
-		chain.TxFactory().WithGas(chain.GasLimitByMsgs(issueMsgs...)),
-		issueMsgs...,
+		chain.TxFactory().WithGas(chain.GasLimitByMsgs(issueMsg1)),
+		issueMsg1,
 	)
 	require.NoError(t, err)
 
 	tokenIssuedEvts, err := event.FindTypedEvents[*assetfttypes.EventIssued](res.Events)
 	require.NoError(t, err)
-	require.Equal(t, len(issueMsgs), len(tokenIssuedEvts))
 
 	denom1 := tokenIssuedEvts[0].Denom
-	denom2 := tokenIssuedEvts[1].Denom
+
+	res, err = client.BroadcastTx(
+		ctx,
+		chain.ClientContext.WithFromAddress(sender),
+		chain.TxFactory().WithGas(chain.GasLimitByMsgs(issueMsg2)),
+		issueMsg2,
+	)
+	require.NoError(t, err)
+
+	tokenIssuedEvts, err = event.FindTypedEvents[*assetfttypes.EventIssued](res.Events)
+	require.NoError(t, err)
+
+	denom2 := tokenIssuedEvts[0].Denom
 
 	msg := &banktypes.MsgMultiSend{
 		Inputs: []banktypes.Input{
@@ -581,6 +590,8 @@ func TestBankMultiSend(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, bankMultiSendGas, uint64(res.GasUsed))
 
+	// =============================
+
 	bankClient := banktypes.NewQueryClient(chain.ClientContext)
 
 	qres, err := bankClient.AllBalances(ctx, &banktypes.QueryAllBalancesRequest{Address: sender.String()})

diff --git a/integration-tests/modules/gas_estimation_test.go b/integration-tests/modules/gas_estimation_test.go
@@ -0,0 +1,116 @@
+//go:build integrationtests && gasestimationtests
+
+package modules
+
+import (
+	"fmt"
+	"testing"
+
+	sdkmath "cosmossdk.io/math"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
+	"github.com/stretchr/testify/require"
+
+	integrationtests "github.com/CoreumFoundation/coreum/v3/integration-tests"
+	"github.com/CoreumFoundation/coreum/v3/pkg/client"
+	"github.com/CoreumFoundation/coreum/v3/testutil/integration"
+	assetfttypes "github.com/CoreumFoundation/coreum/v3/x/asset/ft/types"
+)
+
+// TestBankSendEstimation is used to estimate gas required by each additional token present in bank send message.
+// It executes transactions sending from 1 to 201 tokens in single message and reports gas estimated by each of them.
+// Then you may copy the results to a spreadsheet and calculate the gas required by each transfer.
+// Spreadsheet example might be found here: https://docs.google.com/spreadsheets/d/1qoKa8udTPYdS_-ofJ8xNbnZFDh-gqGb4n0_OgcTHUOA/edit?usp=sharing
+// Keep in mind that to estimate the gas you need to move bank send message to nondeterministic section inside deterministic gas config.
+func TestBankSendEstimation(t *testing.T) {
+	const (
+		nTokens = 101
+		step    = 20
+	)
+
+	requireT := require.New(t)
+
+	ctx, chain := integrationtests.NewCoreumTestingContext(t)
+
+	issuer := chain.GenAccount()
+	recipient1 := chain.GenAccount()
+	recipient2 := chain.GenAccount()
+
+	tokens := make([]string, 0, nTokens)
+	deterministicMsgs := make([]sdk.Msg, 0, 3*nTokens)
+	sendMsg := &banktypes.MsgSend{
+		FromAddress: issuer.String(),
+		ToAddress:   recipient1.String(),
+	}
+	initialAmount := sdkmath.NewInt(100_000_000_000)
+	for i := 0; i < nTokens; i++ {
+		subunit := fmt.Sprintf("tok%d", i)
+		denom := assetfttypes.BuildDenom(subunit, issuer)
+		deterministicMsgs = append(deterministicMsgs, &assetfttypes.MsgIssue{
+			Issuer:        issuer.String(),
+			Symbol:        fmt.Sprintf("TOK%d", i),
+			Subunit:       fmt.Sprintf("tok%d", i),
+			Precision:     1,
+			Description:   fmt.Sprintf("TOK%d", i),
+			InitialAmount: initialAmount,
+			Features: []assetfttypes.Feature{
+				assetfttypes.Feature_minting,
+				assetfttypes.Feature_burning,
+				assetfttypes.Feature_freezing,
+				assetfttypes.Feature_whitelisting,
+				assetfttypes.Feature_ibc,
+			},
+			BurnRate:           sdkmath.LegacyMustNewDecFromStr("0.01"),
+			SendCommissionRate: sdkmath.LegacyMustNewDecFromStr("0.01"),
+		}, &assetfttypes.MsgSetWhitelistedLimit{
+			Sender:  issuer.String(),
+			Account: recipient1.String(),
+			Coin:    sdk.NewInt64Coin(denom, 1_000_000_000_000),
+		}, &assetfttypes.MsgSetWhitelistedLimit{
+			Sender:  issuer.String(),
+			Account: recipient2.String(),
+			Coin:    sdk.NewInt64Coin(denom, 1_000_000_000_000),
+		})
+
+		tokens = append(tokens, assetfttypes.BuildDenom(subunit, issuer))
+		sendMsg.Amount = sendMsg.Amount.Add(sdk.NewCoin(denom, initialAmount))
+	}
+
+	chain.FundAccountWithOptions(ctx, t, issuer, integration.BalancesOptions{
+		Messages: deterministicMsgs,
+		Amount:   chain.QueryAssetFTParams(ctx, t).IssueFee.Amount.MulRaw(nTokens).AddRaw(1_000_000_000),
+	})
+
+	chain.FundAccountWithOptions(ctx, t, recipient1, integration.BalancesOptions{
+		Amount: sdk.NewIntFromUint64(1_000_000_000),
+	})
+
+	_, err := client.BroadcastTx(
+		ctx,
+		chain.ClientContext.WithFromAddress(issuer),
+		chain.TxFactory().WithGas(chain.GasLimitByMsgs(deterministicMsgs...)+5_000_000),
+		append(append([]sdk.Msg{}, deterministicMsgs...), sendMsg)...,
+	)
+	requireT.NoError(err)
+
+	for n := 1; n <= nTokens; n += step {
+		sendMsg := &banktypes.MsgSend{
+			FromAddress: recipient1.String(),
+			ToAddress:   recipient2.String(),
+		}
+
+		for i := 0; i < n; i++ {
+			sendMsg.Amount = sendMsg.Amount.Add(sdk.NewCoin(tokens[i], sdkmath.NewInt(1_0000_000)))
+		}
+
+		txRes, err := client.BroadcastTx(
+			ctx,
+			chain.ClientContext.WithFromAddress(recipient1),
+			chain.TxFactory().WithGas(50_000_000),
+			sendMsg,
+		)
+		requireT.NoError(err)
+
+		fmt.Printf("%d\t%d\n", n, txRes.GasUsed)
+	}
+}
diff --git a/testutil/integration/coreum.go b/testutil/integration/coreum.go
@@ -95,11 +95,11 @@ func (c CoreumChain) ComputeNeededBalanceFromOptions(options BalancesOptions) sd
 		gas := c.GasLimitByMsgs(msg)
 		// Ceil().RoundInt() is here to be compatible with the sdk's TxFactory
 		// https://github.com/cosmos/cosmos-sdk/blob/ff416ee63d32da5d520a8b2d16b00da762416146/client/tx/factory.go#L223
-		amt := options.GasPrice.Mul(sdk.NewDec(int64(gas))).Ceil().RoundInt()
+		amt := options.GasPrice.Mul(sdkmath.LegacyNewDec(int64(gas))).Ceil().RoundInt()
 		totalAmount = totalAmount.Add(amt)
 	}
 
-	return totalAmount.Add(options.GasPrice.Mul(sdk.NewDec(int64(options.NondeterministicMessagesGas))).Ceil().RoundInt()).Add(options.Amount)
+	return totalAmount.Add(options.GasPrice.Mul(sdkmath.LegacyNewDec(int64(options.NondeterministicMessagesGas))).Ceil().RoundInt()).Add(options.Amount)
 }
 
 // FundAccountWithOptions computes the needed balances and fund account with it.

diff --git a/x/deterministicgas/README.md b/x/deterministicgas/README.md
@@ -0,0 +1,14 @@
+# Gas estimation procedure
+
+After every upgrade of Cosmos SDK (or whenever we decide) we should verify that our deterministic gas estimations for messages are correct.
+
+To do it:
+1. Modify crust to deploy `explorer` and `monitoring` profiles when integration tests are started
+2. Run integration tests
+3. Go to our Grafana dashboard in `znet`
+4. Take values for deterministic gas factors reported there
+5. Recalculate the deterministic gas by multiplying it by the minimum value taken from the metric.
+
+If there is huge divergence between min and max values reported, reason should be identified.
+
+For Bank Send message we have integration test `TestBankSendEstimation` used to estimate the gas required by each additional coin present in the message.