Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simulate transactions by default to set gas automatically #2047

Merged
merged 10 commits into from
Aug 28, 2018
2 changes: 2 additions & 0 deletions PENDING.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ FEATURES
* Gaia CLI (`gaiacli`)
* [cli] Cmds to query staking pool and params
* [gov][cli] #2062 added `--proposal` flag to `submit-proposal` that allows a JSON file containing a proposal to be passed in
* [cli] \#2047 Setting the --gas flag value to 0 triggers a simulation of the tx before the actual execution. The gas estimate obtained via the simulation will be used as gas limit in the actual execution.
* [cli] \#2047 The --gas-adjustment flag can be used to adjust the estimate obtained via the simulation triggered by --gas=0.

* Gaia

Expand Down
37 changes: 25 additions & 12 deletions baseapp/baseapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -493,15 +493,15 @@ func validateBasicTxMsgs(msgs []sdk.Msg) sdk.Error {
return nil
}

// retrieve the context for the ante handler and store the tx bytes; store
// the signing validators if the tx runs within the deliverTx() state.
func (app *BaseApp) getContextForAnte(mode runTxMode, txBytes []byte) (ctx sdk.Context) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets add some comment here about what the intended result of this is under each mode.

I agree with the code change, however this code is hard to grok, especially now that we've removed the reference to check/simulate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorporated, thanks

// Get the context
if mode == runTxModeCheck || mode == runTxModeSimulate {
ctx = app.checkState.ctx.WithTxBytes(txBytes)
} else {
ctx = app.deliverState.ctx.WithTxBytes(txBytes)
ctx = ctx.WithSigningValidators(app.signedValidators)
ctx = getState(app, mode).ctx.WithTxBytes(txBytes)
if mode != runTxModeDeliver {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit) Writing this with if mode == runTxModeDelvier is a bit more obvious (that we're doing something special for delivertx)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorporated, thanks

return
}

ctx = ctx.WithSigningValidators(app.signedValidators)
return
}

Expand Down Expand Up @@ -567,6 +567,13 @@ func getState(app *BaseApp, mode runTxMode) *state {
return app.deliverState
}

func (app *BaseApp) initializeContext(ctx sdk.Context, mode runTxMode) sdk.Context {
if mode != runTxModeSimulate {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit) Writing this with an if mode == runTxModeSimulate is a bit more obvious (that we're doing something special for simulation).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorporated, thanks.

return ctx
}
return ctx.WithMultiStore(getState(app, runTxModeSimulate).CacheMultiStore())
}

// runTx processes a transaction. The transactions is proccessed via an
// anteHandler. txBytes may be nil in some cases, eg. in tests. Also, in the
// future we may support "internal" transactions.
Expand All @@ -575,7 +582,9 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk
// determined by the GasMeter. We need access to the context to get the gas
// meter so we initialize upfront.
var gasWanted int64
var msCache sdk.CacheMultiStore
ctx := app.getContextForAnte(mode, txBytes)
ctx = app.initializeContext(ctx, mode)

defer func() {
if r := recover(); r != nil {
Expand All @@ -594,9 +603,7 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk
}()

var msgs = tx.GetMsgs()

err := validateBasicTxMsgs(msgs)
if err != nil {
if err := validateBasicTxMsgs(msgs); err != nil {
return err.Result()
}

Expand All @@ -613,9 +620,15 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk
gasWanted = result.GasWanted
}

if mode == runTxModeSimulate {
result = app.runMsgs(ctx, msgs, mode)
result.GasWanted = gasWanted
return
}

// Keep the state in a transient CacheWrap in case processing the messages
// fails.
msCache := getState(app, mode).CacheMultiStore()
msCache = getState(app, mode).CacheMultiStore()
if msCache.TracingEnabled() {
msCache = msCache.WithTracingContext(sdk.TraceContext(
map[string]interface{}{"txHash": cmn.HexBytes(tmhash.Sum(txBytes)).String()},
Expand All @@ -626,8 +639,8 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk
result = app.runMsgs(ctx, msgs, mode)
result.GasWanted = gasWanted

// only update state if all messages pass and we're not in a simulation
if result.IsOK() && mode != runTxModeSimulate {
// only update state if all messages pass
if result.IsOK() {
msCache.Write()
}

Expand Down
4 changes: 4 additions & 0 deletions client/context/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ type CLIContext struct {
Client rpcclient.Client
Logger io.Writer
Height int64
Gas int64
GasAdjustment float64
NodeURI string
FromAddressName string
AccountStore string
Expand All @@ -48,6 +50,8 @@ func NewCLIContext() CLIContext {
AccountStore: ctxAccStoreName,
FromAddressName: viper.GetString(client.FlagFrom),
Height: viper.GetInt64(client.FlagHeight),
Gas: viper.GetInt64(client.FlagGas),
GasAdjustment: viper.GetFloat64(client.FlagGasAdjustment),
TrustNode: viper.GetBool(client.FlagTrustNode),
UseLedger: viper.GetBool(client.FlagUseLedger),
Async: viper.GetBool(client.FlagAsync),
Expand Down
7 changes: 3 additions & 4 deletions client/context/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (

"github.com/pkg/errors"

"github.com/tendermint/tendermint/libs/common"
cmn "github.com/tendermint/tendermint/libs/common"
rpcclient "github.com/tendermint/tendermint/rpc/client"
ctypes "github.com/tendermint/tendermint/rpc/core/types"
Expand All @@ -27,8 +26,8 @@ func (ctx CLIContext) GetNode() (rpcclient.Client, error) {
}

// Query performs a query for information about the connected node.
func (ctx CLIContext) Query(path string) (res []byte, err error) {
return ctx.query(path, nil)
func (ctx CLIContext) Query(path string, data cmn.HexBytes) (res []byte, err error) {
return ctx.query(path, data)
}

// Query information about the connected node with a data payload
Expand Down Expand Up @@ -284,7 +283,7 @@ func (ctx CLIContext) ensureBroadcastTx(txBytes []byte) error {

// query performs a query from a Tendermint node with the provided store name
// and path.
func (ctx CLIContext) query(path string, key common.HexBytes) (res []byte, err error) {
func (ctx CLIContext) query(path string, key cmn.HexBytes) (res []byte, err error) {
node, err := ctx.GetNode()
if err != nil {
return res, err
Expand Down
6 changes: 5 additions & 1 deletion client/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,14 @@ import "github.com/spf13/cobra"

// nolint
const (
DefaultGasAdjustment = 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this was 1.2 (below)?


FlagUseLedger = "ledger"
FlagChainID = "chain-id"
FlagNode = "node"
FlagHeight = "height"
FlagGas = "gas"
FlagGasAdjustment = "gas-adjustment"
FlagTrustNode = "trust-node"
FlagFrom = "from"
FlagName = "name"
Expand Down Expand Up @@ -49,7 +52,8 @@ func PostCommands(cmds ...*cobra.Command) []*cobra.Command {
c.Flags().String(FlagChainID, "", "Chain ID of tendermint node")
c.Flags().String(FlagNode, "tcp://localhost:26657", "<host>:<port> to tendermint rpc interface for this chain")
c.Flags().Bool(FlagUseLedger, false, "Use a connected Ledger device")
c.Flags().Int64(FlagGas, 200000, "gas limit to set per-transaction")
c.Flags().Int64(FlagGas, 0, "gas limit to set per-transaction; set to 0 to calculate required gas automatically")
c.Flags().Float64(FlagGasAdjustment, DefaultGasAdjustment, "adjustment factor to be multiplied against the estimate returned by the tx simulation; defaults to an internal value")
c.Flags().Bool(FlagAsync, false, "broadcast transactions asynchronously")
c.Flags().Bool(FlagJson, false, "return output in json format")
c.Flags().Bool(FlagPrintResponse, true, "return tx response (only works with async = false)")
Expand Down
43 changes: 28 additions & 15 deletions client/lcd/lcd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,14 @@ func TestCoinSend(t *testing.T) {

require.Equal(t, "steak", mycoins.Denom)
require.Equal(t, int64(1), mycoins.Amount.Int64())

// test failure with too little gas
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets test success with sufficient, manually specified gas as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACKd

res, body, _ = doSendWithGas(t, port, seed, name, password, addr, 100)
require.Equal(t, http.StatusInternalServerError, res.StatusCode, body)

// test success with just enough gas
res, body, _ = doSendWithGas(t, port, seed, name, password, addr, 3000)
require.Equal(t, http.StatusOK, res.StatusCode, body)
}

func TestIBCTransfer(t *testing.T) {
Expand Down Expand Up @@ -712,7 +720,7 @@ func getAccount(t *testing.T, port string, addr sdk.AccAddress) auth.Account {
return acc
}

func doSend(t *testing.T, port, seed, name, password string, addr sdk.AccAddress) (receiveAddr sdk.AccAddress, resultTx ctypes.ResultBroadcastTxCommit) {
func doSendWithGas(t *testing.T, port, seed, name, password string, addr sdk.AccAddress, gas int64) (res *http.Response, body string, receiveAddr sdk.AccAddress) {

// create receive address
kb := client.MockKeyBase()
Expand All @@ -730,19 +738,31 @@ func doSend(t *testing.T, port, seed, name, password string, addr sdk.AccAddress
panic(err)
}

gasStr := ""
if gas > 0 {
gasStr = fmt.Sprintf(`
"gas":"%v",
`, gas)
}
jsonStr := []byte(fmt.Sprintf(`{
%v
"name":"%s",
"password":"%s",
"account_number":"%d",
"sequence":"%d",
"gas": "10000",
"amount":[%s],
"chain_id":"%s"
}`, name, password, accnum, sequence, coinbz, chainID))
res, body := Request(t, port, "POST", fmt.Sprintf("/accounts/%s/send", receiveAddr), jsonStr)
}`, gasStr, name, password, accnum, sequence, coinbz, chainID))

res, body = Request(t, port, "POST", fmt.Sprintf("/accounts/%s/send", receiveAddr), jsonStr)
return
}

func doSend(t *testing.T, port, seed, name, password string, addr sdk.AccAddress) (receiveAddr sdk.AccAddress, resultTx ctypes.ResultBroadcastTxCommit) {
res, body, receiveAddr := doSendWithGas(t, port, seed, name, password, addr, 0)
require.Equal(t, http.StatusOK, res.StatusCode, body)

err = cdc.UnmarshalJSON([]byte(body), &resultTx)
err := cdc.UnmarshalJSON([]byte(body), &resultTx)
require.Nil(t, err)

return receiveAddr, resultTx
Expand All @@ -768,7 +788,6 @@ func doIBCTransfer(t *testing.T, port, seed, name, password string, addr sdk.Acc
"password": "%s",
"account_number":"%d",
"sequence": "%d",
"gas": "100000",
"src_chain_id": "%s",
"amount":[
{
Expand Down Expand Up @@ -887,7 +906,6 @@ func doDelegate(t *testing.T, port, seed, name, password string, delegatorAddr,
"password": "%s",
"account_number": "%d",
"sequence": "%d",
"gas": "10000",
"chain_id": "%s",
"delegations": [
{
Expand Down Expand Up @@ -925,7 +943,6 @@ func doBeginUnbonding(t *testing.T, port, seed, name, password string,
"password": "%s",
"account_number": "%d",
"sequence": "%d",
"gas": "20000",
"chain_id": "%s",
"delegations": [],
"begin_unbondings": [
Expand Down Expand Up @@ -964,7 +981,6 @@ func doBeginRedelegation(t *testing.T, port, seed, name, password string,
"password": "%s",
"account_number": "%d",
"sequence": "%d",
"gas": "10000",
"chain_id": "%s",
"delegations": [],
"begin_unbondings": [],
Expand Down Expand Up @@ -1116,8 +1132,7 @@ func doSubmitProposal(t *testing.T, port, seed, name, password string, proposerA
"password": "%s",
"chain_id": "%s",
"account_number":"%d",
"sequence":"%d",
"gas":"100000"
"sequence":"%d"
}
}`, proposerAddr, name, password, chainID, accnum, sequence))
res, body := Request(t, port, "POST", "/gov/proposals", jsonStr)
Expand Down Expand Up @@ -1147,8 +1162,7 @@ func doDeposit(t *testing.T, port, seed, name, password string, proposerAddr sdk
"password": "%s",
"chain_id": "%s",
"account_number":"%d",
"sequence": "%d",
"gas":"100000"
"sequence": "%d"
}
}`, proposerAddr, name, password, chainID, accnum, sequence))
res, body := Request(t, port, "POST", fmt.Sprintf("/gov/proposals/%d/deposits", proposalID), jsonStr)
Expand Down Expand Up @@ -1178,8 +1192,7 @@ func doVote(t *testing.T, port, seed, name, password string, proposerAddr sdk.Ac
"password": "%s",
"chain_id": "%s",
"account_number": "%d",
"sequence": "%d",
"gas":"100000"
"sequence": "%d"
}
}`, proposerAddr, name, password, chainID, accnum, sequence))
res, body := Request(t, port, "POST", fmt.Sprintf("/gov/proposals/%d/votes", proposalID), jsonStr)
Expand Down
2 changes: 1 addition & 1 deletion client/lcd/version.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ func CLIVersionRequestHandler(w http.ResponseWriter, r *http.Request) {
// connected node version REST handler endpoint
func NodeVersionRequestHandler(cliCtx context.CLIContext) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
version, err := cliCtx.Query("/app/version")
version, err := cliCtx.Query("/app/version", nil)
if err != nil {
w.WriteHeader(http.StatusInternalServerError)
w.Write([]byte(fmt.Sprintf("Could't query version. Error: %s", err.Error())))
Expand Down
12 changes: 12 additions & 0 deletions client/utils/rest.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package utils

import (
"net/http"
)

// WriteErrorResponse prepares and writes a HTTP error
// given a status code and an error message.
func WriteErrorResponse(w *http.ResponseWriter, status int, msg string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets do stuff like this in separate PR's in the future. Theres probably other places it can be used, but its harder to reason about as its not the focus of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

(*w).WriteHeader(status)
(*w).Write([]byte(msg))
}
Loading