Skip to content

Commit

Permalink
Make MsgSubmitProposal a nondeterministic message (#377)
Browse files Browse the repository at this point in the history
* Make `MsgSubmitProposal` a nondeterministic message

* Fix integration tests

* Merge master into wojtek/proposal-nondeterministic-gas

* Merge remote-tracking branch 'origin/master' into wojtek/proposal-nondeterministic-gas

# Conflicts:
#	x/deterministicgas/config_test.go

* Estimate gas for proposal

* Merge master into wojtek/proposal-nondeterministic-gas

* Revert gas estimation for proposal

* Merge master into wojtek/proposal-nondeterministic-gas
  • Loading branch information
wojtek-coreum authored Feb 9, 2023
1 parent fe2ce0c commit 8fba997
Show file tree
Hide file tree
Showing 7 changed files with 33 additions and 17 deletions.
9 changes: 5 additions & 4 deletions integration-tests/chain.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,10 @@ func (c ChainContext) GasLimitByMultiSendMsgs(msgs ...sdk.Msg) uint64 {

// BalancesOptions is the input type for the ComputeNeededBalanceFromOptions.
type BalancesOptions struct {
Messages []sdk.Msg
GasPrice sdk.Dec
Amount sdk.Int
Messages []sdk.Msg
NondeterministicMessagesGas uint64
GasPrice sdk.Dec
Amount sdk.Int
}

// ComputeNeededBalanceFromOptions computes the required balance based on the input options.
Expand All @@ -148,7 +149,7 @@ func (c ChainContext) ComputeNeededBalanceFromOptions(options BalancesOptions) s
totalAmount = totalAmount.Add(amt)
}

return totalAmount.Add(options.Amount)
return totalAmount.Add(options.GasPrice.Mul(sdk.NewDec(int64(options.NondeterministicMessagesGas))).Ceil().RoundInt()).Add(options.Amount)
}

// ChainConfig defines the config arguments required for the test chain initialisation.
Expand Down
9 changes: 5 additions & 4 deletions integration-tests/gov.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import (
"github.com/CoreumFoundation/coreum/testutil/event"
)

const submitProposalGas = 200_000

// Governance keep the test chain predefined account for the governance operations.
type Governance struct {
chainCtx ChainContext
Expand Down Expand Up @@ -53,8 +55,8 @@ func (g Governance) ComputeProposerBalance(ctx context.Context) (sdk.Coin, error

minDeposit := govParams.DepositParams.MinDeposit[0]
proposerInitialBalance := g.chainCtx.ComputeNeededBalanceFromOptions(BalancesOptions{
Messages: []sdk.Msg{&govtypes.MsgSubmitProposal{}},
Amount: minDeposit.Amount,
NondeterministicMessagesGas: submitProposalGas, // to cover MsgSubmitProposal
Amount: minDeposit.Amount,
})

return g.chainCtx.NewCoin(proposerInitialBalance), nil
Expand Down Expand Up @@ -125,8 +127,7 @@ func (g Governance) ProposeAndVote(ctx context.Context, proposer sdk.AccAddress,

// Propose creates a new proposal.
func (g Governance) Propose(ctx context.Context, msg *govtypes.MsgSubmitProposal) (uint64, error) {
txf := g.chainCtx.TxFactory().
WithGas(g.chainCtx.GasLimitByMsgs(&govtypes.MsgSubmitProposal{}))
txf := g.chainCtx.TxFactory().WithGas(submitProposalGas)
result, err := client.BroadcastTx(
ctx,
g.chainCtx.ClientContext.WithFromAddress(msg.GetProposer()),
Expand Down
5 changes: 3 additions & 2 deletions integration-tests/modules/bank_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,9 @@ func TestBankMultiSendBatchOutputs(t *testing.T) {
}

requireT.NoError(chain.Faucet.FundAccountsWithOptions(ctx, issuer, integrationtests.BalancesOptions{
Messages: append([]sdk.Msg{issueMsg}, multiSendMsgs...),
Amount: chain.NetworkConfig.AssetFTConfig.IssueFee.Add(sdk.NewInt(10_000_000)), // add more coins to cover extra bytes because of the message size
Messages: append([]sdk.Msg{issueMsg}, multiSendMsgs...),
NondeterministicMessagesGas: 10_000_000, // to cover extra bytes because of the message size
Amount: chain.NetworkConfig.AssetFTConfig.IssueFee,
}))

// issue fungible tokens
Expand Down
1 change: 0 additions & 1 deletion integration-tests/modules/staking_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,6 @@ func setUnbondingTimeViaGovernance(ctx context.Context, t *testing.T, chain inte
err = chain.Faucet.FundAccounts(ctx, integrationtests.NewFundedAccount(proposer, proposerBalance))
requireT.NoError(err)

// TODO(dhil) refactor other tests to use that func for the standard propose + vote action.
// Create proposition to change max the unbonding time value.
err = chain.Governance.ProposeAndVote(ctx, proposer,
paramproposal.NewParameterChangeProposal(
Expand Down
5 changes: 5 additions & 0 deletions pkg/client/tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,11 @@ func BroadcastTx(ctx context.Context, clientCtx Context, txf Factory, msgs ...sd
// CalculateGas simulates the execution of a transaction and returns the
// simulation response obtained by the query and the adjusted gas amount.
func CalculateGas(ctx context.Context, clientCtx Context, txf Factory, msgs ...sdk.Msg) (*sdktx.SimulateResponse, uint64, error) {
txf, err := prepareFactory(ctx, clientCtx, txf)
if err != nil {
return nil, 0, err
}

txBytes, err := tx.BuildSimTx(txf, msgs...)
if err != nil {
return nil, 0, err
Expand Down
14 changes: 10 additions & 4 deletions x/deterministicgas/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ type Config struct {
}

// DefaultConfig returns default config for deterministic gas.
//
//nolint:funlen
func DefaultConfig() Config {
cfg := Config{
FixedGas: 50000,
Expand Down Expand Up @@ -84,10 +86,9 @@ func DefaultConfig() Config {
MsgType(&feegranttypes.MsgRevokeAllowance{}): constantGasFunc(2500),

// gov
MsgType(&govtypes.MsgSubmitProposal{}): constantGasFunc(65000),
MsgType(&govtypes.MsgVote{}): constantGasFunc(7000),
MsgType(&govtypes.MsgVoteWeighted{}): constantGasFunc(9000),
MsgType(&govtypes.MsgDeposit{}): constantGasFunc(52000),
MsgType(&govtypes.MsgVote{}): constantGasFunc(7000),
MsgType(&govtypes.MsgVoteWeighted{}): constantGasFunc(9000),
MsgType(&govtypes.MsgDeposit{}): constantGasFunc(52000),

// nft
MsgType(&nfttypes.MsgSend{}): constantGasFunc(16000),
Expand All @@ -113,6 +114,11 @@ func DefaultConfig() Config {
registerUndeterministicGasFuncs(
&cfg,
[]sdk.Msg{
// gov
// MsgSubmitProposal is defined as undeterministic because it runs a proposal handler function
// specific for each proposal and those functions consume unknown amount of gas.
&govtypes.MsgSubmitProposal{},

// crisis
// MsgVerifyInvariant is defined as undeterministic since fee
// charged by this tx type is defined as param inside module.
Expand Down
7 changes: 5 additions & 2 deletions x/deterministicgas/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ func TestDeterministicGas_DeterministicMessages(t *testing.T) {

// WASM messages will be added here
undetermMsgTypes := []string{
// gov
"/cosmos.gov.v1beta1.MsgSubmitProposal",

// crisis
"/cosmos.crisis.v1beta1.MsgVerifyInvariant",

Expand Down Expand Up @@ -146,8 +149,8 @@ func TestDeterministicGas_DeterministicMessages(t *testing.T) {
// To make sure we do not increase/decrease deterministic types accidentally
// we assert length to be equal to exact number, so each change requires
// explicit adjustment of tests.
assert.Equal(t, 9, len(undetermMsgs))
assert.Equal(t, 40, len(determMsgs))
assert.Equal(t, 10, len(undetermMsgs))
assert.Equal(t, 39, len(determMsgs))

for _, sdkMsg := range determMsgs {
sdkMsg := sdkMsg
Expand Down

0 comments on commit 8fba997

Please sign in to comment.