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
Merged

Simulate transactions by default to set gas automatically #2047

merged 10 commits into from
Aug 28, 2018

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Aug 15, 2018

Relevant issue: #1246

  • Change --gas=0 semantic in order to enable gas auto estimate.
  • REST clients have been modified to simulate the execution of the tx first to then populate the context with the estimated gas amount returned by the simulation.
  • The simulation returns both an unadjusted gas estimate and an adjusted one. The adjustment is required to ensure that the ensuing execution doesn't fail due to state changes that might have occurred. Gas adjustment can be controlled via the CLI's --gas-adjustment flag.

Closes: #1246

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Updated all relevant documentation (docs/)
  • Updated all relevant code comments
  • Wrote tests
  • Added entries in PENDING.md that include links to the relevant issue or PR that most accurately describes the change.
  • Updated cmd/gaia and examples/

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@fedekunze fedekunze added the wip label Aug 15, 2018
@alessio alessio changed the title WIP: Simulate transactions by default Simulate transactions by default to set gas automatically Aug 16, 2018
// execution failures due to state changes that might
// occur between the tx simulation and the actual run.
const (
SimulateGasLimit = 200000
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be avoid by introducing a dry-run ante handler that, othen than simulating sig verification, would use an infinite gas meter as well.

@alessio
Copy link
Contributor Author

alessio commented Aug 21, 2018

Logged all calls to ConsumeGas, when the tx is executed ante handler is called twice - possibly due to checkTx:

alessio@bizet:~/.../github.com/cosmos/cosmos-sdk$ i && go test -v ./client/lcd/ 
go install -tags "netgo ledger" -ldflags "-X github.com/cosmos/cosmos-sdk/version.GitCommit=7a92f913" ./cmd/gaia/cmd/gaiad
go install -tags "netgo ledger" -ldflags "-X github.com/cosmos/cosmos-sdk/version.GitCommit=7a92f913" ./cmd/gaia/cmd/gaiacli
=== RUN   TestCoinSend
LADDR tcp://0.0.0.0:36885
E[08-21|05:52:03.173] Couldn't connect to any seeds                module=p2p 
REQUEST GET http://localhost:39161/accounts/cosmosaccaddr137n2k4addpc0dddju4mntuu09ucww09k8vp0n4
REQUEST GET http://localhost:39161/accounts/cosmosaccaddr1q5x4j9xk3wylpvuywvqxdzf0zduwh8dlngh5f8
REQUEST GET http://localhost:39161/accounts/cosmosaccaddr1q5x4j9xk3wylpvuywvqxdzf0zduwh8dlngh5f8
REQUEST POST http://localhost:39161/accounts/cosmosaccaddr150n2lj9cyrhmqpnq7zs0y5g2zwm7mtz7j2ks2q/send
[START EXECUTION]
[START SIMULATION]
[ConsumeGas] [memo] amount: 0 - consumed: 0
[ConsumeGas] [ReadFlat] amount: 10 - consumed: 10
[ConsumeGas] [ReadPerByte] amount: 42 - consumed: 52
[ConsumeGas] [ante verify] amount: 100 - consumed: 152
[ConsumeGas] [WriteFlat] amount: 10 - consumed: 162
[ConsumeGas] [WritePerByte] amount: 840 - consumed: 1002
[ConsumeGas] [subtractCoins] amount: 10 - consumed: 1012
[ConsumeGas] [getCoins] amount: 10 - consumed: 1022
[ConsumeGas] [ReadFlat] amount: 10 - consumed: 1032
[ConsumeGas] [ReadPerByte] amount: 84 - consumed: 1116
[ConsumeGas] [setCoins] amount: 100 - consumed: 1216
[ConsumeGas] [ReadFlat] amount: 10 - consumed: 1226
[ConsumeGas] [ReadPerByte] amount: 84 - consumed: 1310
[ConsumeGas] [WriteFlat] amount: 10 - consumed: 1320
[ConsumeGas] [WritePerByte] amount: 830 - consumed: 2150
[ConsumeGas] [addCoins] amount: 10 - consumed: 2160
[ConsumeGas] [getCoins] amount: 10 - consumed: 2170
[ConsumeGas] [ReadFlat] amount: 10 - consumed: 2180
[ConsumeGas] [ReadPerByte] amount: 0 - consumed: 2180
[ConsumeGas] [setCoins] amount: 100 - consumed: 2280
[ConsumeGas] [ReadFlat] amount: 10 - consumed: 2290
[ConsumeGas] [ReadPerByte] amount: 0 - consumed: 2290
[ConsumeGas] [ReadFlat] amount: 10 - consumed: 2300
[ConsumeGas] [ReadPerByte] amount: 2 - consumed: 2302
[ConsumeGas] [WriteFlat] amount: 10 - consumed: 2312
[ConsumeGas] [WritePerByte] amount: 20 - consumed: 2332
[ConsumeGas] [WriteFlat] amount: 10 - consumed: 2342
[ConsumeGas] [WritePerByte] amount: 400 - consumed: 2742
gas: estimated=2742
[START EXECUTION]
[ConsumeGas] [memo] amount: 0 - consumed: 0
[ConsumeGas] [ReadFlat] amount: 10 - consumed: 10
[ConsumeGas] [ReadPerByte] amount: 42 - consumed: 52
[ConsumeGas] [ante verify] amount: 100 - consumed: 152
[ConsumeGas] [WriteFlat] amount: 10 - consumed: 162
[ConsumeGas] [WritePerByte] amount: 840 - consumed: 1002
[START EXECUTION]
[ConsumeGas] [memo] amount: 0 - consumed: 0
[ConsumeGas] [ReadFlat] amount: 10 - consumed: 10
[ConsumeGas] [ReadPerByte] amount: 42 - consumed: 52
[ConsumeGas] [ante verify] amount: 100 - consumed: 152
[ConsumeGas] [WriteFlat] amount: 10 - consumed: 162
[ConsumeGas] [WritePerByte] amount: 840 - consumed: 1002
[ConsumeGas] [subtractCoins] amount: 10 - consumed: 1012
[ConsumeGas] [getCoins] amount: 10 - consumed: 1022
[ConsumeGas] [ReadFlat] amount: 10 - consumed: 1032
[ConsumeGas] [ReadPerByte] amount: 84 - consumed: 1116
[ConsumeGas] [setCoins] amount: 100 - consumed: 1216
[ConsumeGas] [ReadFlat] amount: 10 - consumed: 1226
[ConsumeGas] [ReadPerByte] amount: 84 - consumed: 1310
[ConsumeGas] [WriteFlat] amount: 10 - consumed: 1320
[ConsumeGas] [WritePerByte] amount: 830 - consumed: 2150
[ConsumeGas] [addCoins] amount: 10 - consumed: 2160
[ConsumeGas] [getCoins] amount: 10 - consumed: 2170
[ConsumeGas] [ReadFlat] amount: 10 - consumed: 2180
[ConsumeGas] [ReadPerByte] amount: 0 - consumed: 2180
[ConsumeGas] [setCoins] amount: 100 - consumed: 2280
[ConsumeGas] [ReadFlat] amount: 10 - consumed: 2290
[ConsumeGas] [ReadPerByte] amount: 0 - consumed: 2290
[ConsumeGas] [ReadFlat] amount: 10 - consumed: 2300
[ConsumeGas] [ReadPerByte] amount: 2 - consumed: 2302
[ConsumeGas] [WriteFlat] amount: 10 - consumed: 2312
[ConsumeGas] [WritePerByte] amount: 20 - consumed: 2332
[ConsumeGas] [WriteFlat] amount: 10 - consumed: 2342
[ConsumeGas] [WritePerByte] amount: 400 - consumed: 2742
REQUEST GET http://localhost:39161/accounts/cosmosaccaddr1q5x4j9xk3wylpvuywvqxdzf0zduwh8dlngh5f8
REQUEST GET http://localhost:39161/accounts/cosmosaccaddr150n2lj9cyrhmqpnq7zs0y5g2zwm7mtz7j2ks2q
E[08-21|05:52:04.286] RPC HTTP server stopped                      module=rpc-server err="accept tcp [::]:36885: use of closed network connection"
E[08-21|05:52:04.287] RPC HTTP server stopped                      err="accept tcp [::]:39161: use of closed network connection"
--- PASS: TestCoinSend (1.41s)
PASS
ok  	github.com/cosmos/cosmos-sdk/client/lcd	1.478s

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Thanks; left a few comments. Can we also list the parts we're intentionally saving for a later PR (looks like skipping the signature check isn't in this yet?)

client/flags.go Outdated
@@ -4,11 +4,14 @@ import "github.com/spf13/cobra"

// nolint
const (
DefaultGasLimit = 200000
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this at all? If the user doesn't want to simulate the transaction I think we should force them to manually specify gas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I will change it accordingly.
I was thinking to drop the --gas-auto flag in favor of having --gas value set to 0 by default, which would enable the auto gas feature.

// execution failures due to state changes that might
// occur between the tx simulation and the actual run.
const (
SimulateGasLimit = 200000
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think a limit makes sense here either, can we use an InfiniteGasMeter on 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.

Addressed, please review

// occur between the tx simulation and the actual run.
const (
SimulateGasLimit = 200000
GasEstimateAdjustment = 1.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be a CLI flag (1.2 is probably a fine default value)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about --gas-adjustment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good

@@ -575,6 +617,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
if mode == runTxModeSimulate {
return app.simulateTx(txBytes, tx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I'm not sure this is the optimal way to structure this function set, now there seems to be some duplicate code in simulateTx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressing this now, expect a commit soon

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Setting the --gas flag value to 0 triggers a simulation of the tx before the actual execution

Could we change it to an independent flag (e.g --with-simulation) so that the user can get information from the --help command as well ?

Setting --gas=0 looks more like an easter egg to me... (specially if we lack the necessary documentation for the simulation)

@fedekunze
Copy link
Collaborator

fedekunze commented Aug 23, 2018

@alessio left a review. If you still want to keep --gas=0 logic, please add docs for it in the client section of the SDK docs :)

@alessio
Copy link
Contributor Author

alessio commented Aug 23, 2018 via email

@fedekunze
Copy link
Collaborator

@alessio Ok, makes sense to me then. Please add the instructions on the documentation and --gas flag as well. Then you can dismiss my review and I'll approve your PR

@alessio alessio requested a review from zramsay as a code owner August 23, 2018 15:10
@alessio
Copy link
Contributor Author

alessio commented Aug 23, 2018 via email

"github.com/stretchr/testify/assert"
)

func Test_parseQueryResponse(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename to TestParseQueryResponse

@alessio
Copy link
Contributor Author

alessio commented Aug 23, 2018 via email

@@ -277,10 +286,29 @@ func getTestingHomeDirs() (string, string) {
return gaiadHome, gaiacliHome
}

func initialiseFixtures(t *testing.T) (chainID, servAddr, port string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo here initialize

@@ -97,6 +97,11 @@ gaiacli send \
The `--amount` flag accepts the format `--amount=<value|coin_name>`.
:::

::: tip Note
You may want to cap the maximum gas that can be consumed by the transaction via the `--gas` flag.
If set to 0, the gas limit will be automatically estimated.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we need to add the --gas-adjustement flag 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.

Done

@alessio
Copy link
Contributor Author

alessio commented Aug 23, 2018 via email

return
}

w.Write(output)
}
}

func writeErr(w *http.ResponseWriter, status int, msg string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool ! could we move this to another pkg and export it ?

if m.Gas == 0 {
txCtx, err = utils.EnrichTxContextWithGas(txCtx, cliCtx, m.LocalAccountName, m.Password, []sdk.Msg{msg})
if err != nil {
w.WriteHeader(http.StatusUnauthorized)
Copy link
Collaborator

Choose a reason for hiding this comment

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

use your writeErr function

if m.Gas == 0 {
txCtx, err = utils.EnrichTxContextWithGas(txCtx, cliCtx, m.LocalAccountName, m.Password, []sdk.Msg{msg})
if err != nil {
w.WriteHeader(http.StatusUnauthorized)
Copy link
Collaborator

Choose a reason for hiding this comment

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

use writeErr

if m.Gas == 0 {
txCtx, err = utils.EnrichTxContextWithGas(txCtx, cliCtx, m.LocalAccountName, m.Password, []sdk.Msg{msg})
if err != nil {
w.WriteHeader(http.StatusUnauthorized)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

utACK. Left a few comments but LGTM ! Thanks @alessio !

@alessio
Copy link
Contributor Author

alessio commented Aug 23, 2018 via email

Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

Lets move the write header stuff / error stuff into a seperate PR. Its easier to review code if the refactors are seperated from actual code changes. Also some of the cases with gas = 0 checks seem unclear to me.

@@ -495,13 +495,11 @@ func validateBasicTxMsgs(msgs []sdk.Msg) sdk.Error {

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

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

func (app *BaseApp) applyTxMode(ctx sdk.Context, mode runTxMode) 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.

Why is this called applyTxMode if it only does stuff for simulate? It seems confusing imo to generalize this when there is only one case. We're also not really applying the tx mode imo, its just initializing the context.

Perhaps rename this to initializeTxMode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about initializeContext()?

client/flags.go Outdated
@@ -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, "gas adjustment to be applied on the estimate returned by the tx simulation")
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps indicate that this is a multiplicative factor being multiplied against the estimate (vs additive).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK'd

@@ -263,6 +263,10 @@ 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

"sequence":"%d",
"amount":[%s],
"chain_id":"%s",
"gas":"%v"
Copy link
Contributor

Choose a reason for hiding this comment

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

We can eliminate some of the code duplication here by just making the line "gas":"%v" a fmt string arg, and just set that at the top, and pass that in. (so if gas > 0, that argument would be set to "gas":"\n", otherwise it would be set to "".

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

::: tip Note
You may want to cap the maximum gas that can be consumed by the transaction via the `--gas` flag.
If set to 0, the gas limit will be automatically estimated.
Gas estimate might be inaccurate as state changes could occur in between the end of the simulation and the actual execution of a transaction, thus an adjustment is applied on top of the original estimate in order to ensure the transaction is broadcasted successfully. The adjustment can be controlled via the `--gas-adjustment` flag, whose default value is 1.2.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we write up into an issue to make this configurable per tx type in a config somewhere? The reason for considering it would be that a bank send tx probably is going to be most common tx, and also shouldn't have an increased cost due to other state changes, so it shouldn't have the same adjustment as staking txs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Please fork it into a new issue 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is definitely something to consider because the user pays for the allocated gas, not the consumed gas.

Copy link
Contributor

Choose a reason for hiding this comment

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

x/auth/ante.go Outdated
@@ -35,7 +35,11 @@ func NewAnteHandler(am AccountMapper, fck FeeCollectionKeeper) sdk.AnteHandler {
}

// set the gas meter
newCtx = ctx.WithGasMeter(sdk.NewGasMeter(stdTx.Fee.Gas))
if stdTx.Fee.Gas == 0 {
newCtx = ctx.WithGasMeter(sdk.NewInfiniteGasMeter())
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this here? Using an infinite gas meter here seems odd to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When running in simulation mode, we want no gas capping whatsoever - CC'ing @cwgoes

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to cap gas in a simulation, we just need to measure it - is there a reason you think we would need to cap it @ValarDragon?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like switching on gas equals 0. This feels like it could cause more bugs / increase the attack surface. I think we should find another way of communicating that were in simulation mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could it not be set in the context somewhere upstream?

Copy link
Contributor Author

@alessio alessio Aug 24, 2018

Choose a reason for hiding this comment

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

What if we use a default gas limit (some sort of sane default, e.g. 200000 - which is the current default value) and switch back to use a capped GasMeter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re:

simulations shouldn't be verifying signatures

That too is tricky. Simulations go through the query interface, which requires messages to be signed.

Copy link
Contributor

Choose a reason for hiding this comment

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

That still doesn't really alleviate my concern. (That does limit the situation though)

I do think switching on 0 gas is less safe, as we now have to make sure that a tx on the network with 0 gas is stopped before getting to this line. Its prefferrable to just have a separate method to flag simulations, and have the normal gas logic prevent a 0 gas tx imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you reckon the following could mitigate the risk?

diff --git a/client/utils/utils.go b/client/utils/utils.go
index fb5d6198..49258b86 100644
--- a/client/utils/utils.go
+++ b/client/utils/utils.go
@@ -1,6 +1,7 @@
 package utils
 
 import (
+       "errors"
        "fmt"
        "os"
 
@@ -108,6 +109,10 @@ func CalculateGas(queryFunc func(string, common.HexBytes) ([]byte, error), cdc *
        if err != nil {
                return
        }
+       if estimate <= 0 {
+               err = errors.New("simulation returned gas=0")
+               return
+       }
        adjusted = adjustGasEstimate(estimate, adjustment)
        fmt.Fprintf(os.Stderr, "gas: [estimated = %v] [adjusted = %v]\n", estimate, adjusted)
        return

Copy link
Contributor

@ValarDragon ValarDragon Aug 24, 2018

Choose a reason for hiding this comment

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

I think I'm not conveying my concern well. This doesn't really alleviate it.

Right now the current code is saying "If tx.fee.Gas = 0, I get infinite gas". I think this is a bad design for it, even if we add checks in other places. As now were opening more holes, where are a complex series of conditionals could lead to this gas=0, get infinite gas, or a change may happen later that forgets that this could be the case. Instead we should signal directly that we want simulation mode, so this is no longer a fear. (As a tx itself cannot signal simulation mode, so theres no safety risk there)

I agree that this isn't a simple change, but I think its necessary for this to go through. I also think requiring that simulation not take in the private key / a signature is necessary, even if it requires another AnteHandler constructor. You don't want to increase the amount of things a private key has to sign / increase the amount of exposure a private key has if you don't have to.

@@ -360,7 +360,7 @@ func TestAnteHandlerMemoGas(t *testing.T) {
var tx sdk.Tx
msg := newTestMsg(addr1)
privs, accnums, seqs := []crypto.PrivKey{priv1}, []int64{0}, []int64{0}
fee := NewStdFee(0, sdk.NewInt64Coin("atom", 0))
fee := NewStdFee(1, sdk.NewInt64Coin("atom", 0))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't 0 be used for the fee here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0 gas triggers the simulation

@@ -32,15 +33,13 @@ func QueryAccountRequestHandlerFn(

addr, err := sdk.AccAddressFromBech32(bech32addr)
if err != nil {
w.WriteHeader(http.StatusBadRequest)
w.Write([]byte(err.Error()))
utils.WriteErrorResponse(&w, http.StatusInternalServerError, err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets move this refactor into a seperate 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.

requested by @fedekunze - I'd keep it if you don't mind as IMHO it does more good than bad

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yet I'll refrain in future to tackle technical debt while working on a specific issue 👍

@@ -82,21 +76,28 @@ func signAndBuild(w http.ResponseWriter, cliCtx context.CLIContext, baseReq base
Gas: baseReq.Gas,
}

if baseReq.Gas == 0 {
txCtx, err = utils.EnrichTxContextWithGas(txCtx, cliCtx, baseReq.Name, baseReq.Password, []sdk.Msg{msg})
Copy link
Contributor

Choose a reason for hiding this comment

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

EnrichTxContextWithGas feels like an odd name for whats happening here. I think it should be renamed to use the word Calculate. Also this should be refactored to not depend on the password imo. (We should not increase the surface area that gets password access) We can charge gas purely on signature type. (As simulation shouldn't spend the signature verification time anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about the following?

// BuildAndSignTxWithZeroGas builds transactions with GasWanted set to 0.
func BuildAndSignTxWithZeroGas(txCtx authctx.TxContext, name, passphrase string, msgs []sdk.Msg) ([]byte, error) {
	return txCtx.WithGas(0).BuildAndSign(name, passphrase, msgs)
}

// EnrichTxContextWithGas simulates the execution of a transaction to
// then populate the relevant TxContext.Gas field with the estimate
// obtained by the query.
func EnrichTxContextWithGas(txCtx authctx.TxContext, cliCtx context.CLIContext, txBytes []byte) (authctx.TxContext, error) {
	// run a simulation (via /app/simulate query) to
	// estimate gas and update TxContext accordingly
	rawRes, err := cliCtx.Query("/app/simulate", txBytes)
	if err != nil {
		return txCtx, err
	}
	estimate, err := parseQueryResponse(cliCtx.Codec, rawRes)
	if err != nil {
		return txCtx, err
	}
	adjusted := adjustGasEstimate(estimate, cliCtx.GasAdjustment)
	fmt.Fprintf(os.Stderr, "gas: [estimated = %v] [adjusted = %v]\n", estimate, adjusted)
	return txCtx.WithGas(adjusted), nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree with needing a signature here. We don't want simulations to depend on the private key. (Extra computation time, inconvenience, etc. Especially if your signing from a ledger)

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

A few questions/comments.

@@ -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.

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

client/flags.go Outdated
@@ -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)?

// broadcast to a Tendermint node
return cliCtx.EnsureBroadcastTx(txBytes)
}

func enrichCtxWithGasIfGasAuto(txCtx authctx.TxContext, cliCtx context.CLIContext, passphrase string, msgs []sdk.Msg) (authctx.TxContext, error) {
if cliCtx.Gas == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be slightly clearer to put this conditional outside of the function, and only call the function if cliCtx.Gas == 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as linter doesn't complain yes, that was my intent

::: tip Note
You may want to cap the maximum gas that can be consumed by the transaction via the `--gas` flag.
If set to 0, the gas limit will be automatically estimated.
Gas estimate might be inaccurate as state changes could occur in between the end of the simulation and the actual execution of a transaction, thus an adjustment is applied on top of the original estimate in order to ensure the transaction is broadcasted successfully. The adjustment can be controlled via the `--gas-adjustment` flag, whose default value is 1.2.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is definitely something to consider because the user pays for the allocated gas, not the consumed gas.

@@ -114,3 +116,15 @@ func parseInt64OrReturnBadRequest(s string, w http.ResponseWriter) (n int64, ok
}
return n, true
}

func enrichContextWithGas(txCtx authctx.TxContext, cliCtx context.CLIContext, name, password string, msg sdk.Msg) (authctx.TxContext, int, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we deduplicate this function (put it in a common file)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dedup'd, though left it there for now if that is OK for you

return
}

w.Write(output)
}
}

func enrichContextWithGas(txCtx authctx.TxContext, cliCtx context.CLIContext, name, password string, msg sdk.Msg) (authctx.TxContext, int, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we deduplicate this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored, it now uses utils.EnrichCtxWithGas()

return
}

w.Write(output)
}
}

func enrichContextWithGas(txCtx authctx.TxContext, cliCtx context.CLIContext, name, password string, msg sdk.Msg) (authctx.TxContext, int, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we deduplicate this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

return
}

w.Write(output)
}
}

func enrichContextWithGas(txCtx authcliCtx.TxContext, cliCtx context.CLIContext, name, password string, msg sdk.Msg) (authcliCtx.TxContext, int, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we deduplicate this function? This really shouldn't need to be in the module-specific REST files anyways I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

@alexanderbez
Copy link
Contributor

Huh -- looks like test_cover failed due to an already binded address. I thought this was fixed?

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

utACK, thanks @alessio!

Try merging develop, might fix CI.

@ValarDragon
Copy link
Contributor

Just realized one of my comments was hidden, so I'll repost down here:

I disagree with needing a signature / passphrase in the simulation. We don't want simulations to depend on the private key. (Extra computation time for signing / verifying, inconvenience, etc. Especially if your signing from a ledger)

@cwgoes
Copy link
Contributor

cwgoes commented Aug 24, 2018

I disagree with needing a signature / passphrase in the simulation. We don't want simulations to depend on the private key. (Extra computation time for signing / verifying, inconvenience, etc. Especially if your signing from a ledger)

I think we're agreed on this, but it was a point for a future PR (@alessio ?)

@alessio
Copy link
Contributor Author

alessio commented Aug 24, 2018 via email

@ValarDragon
Copy link
Contributor

I think that is something that should absolutely be prelaunch. I'm opposed to having us have private keys create signatures they don't have to. (There are many classes of sidechannel attacks, and they all only get more feasible the more signature / msg pairs you have)

I'm fine with merging this without that then, though I really would prefer that being done here. I do think the Gas=0 concern should be addressed before merge.

@ValarDragon
Copy link
Contributor

If we're going to merge this without removing the privkey dependency in simulation, lets make the default not to simulate, and only switch the default once we fix this. I really dislike the idea of having every client signing an extra message, just because thats unnecessary side channel leakage for something that could have tons of money behind it. (I fear we may forget to fix this prelaunch / decide to punt to postlaunch for quicker launch)

@alessio
Copy link
Contributor Author

alessio commented Aug 25, 2018

It's easy to skip sig verification or replace gas=0 with something else. It's far from easy to estimate gas consumption, with a certain degree of accuracy, needed to verify the signature when you don't have a public key/signature attached to the transaction - unless we change the tx struct radically. Simulation will not return reliable gas estimate. In the end, quite a big share of ante handler's job is to handle accounts validation and sigs verification.

@ValarDragon
Copy link
Contributor

ValarDragon commented Aug 25, 2018

I'm suggesting just have the public key. The AnteHandler determines verification gas cost from just the pubkey. https://github.com/cosmos/cosmos-sdk/blob/develop/x/auth/ante.go#L202

The pubkey is derived from the account in the message. Account validation is independent of knowledge of private key as well.

I think having an alternate Simulation AnteHandler would be preferrable though than conditionals on simulation mode, but conditions on simulation mode are safe as long as simulation mode can never be set from information inside of the tx.

@alessio
Copy link
Contributor Author

alessio commented Aug 25, 2018 via email

@alessio
Copy link
Contributor Author

alessio commented Aug 25, 2018

I'm suggesting just have the public key. The AnteHandler determines verification gas cost from just the pubkey. https://github.com/cosmos/cosmos-sdk/blob/develop/x/auth/ante.go#L202

I am aware of that. Yet no signature, no public key is attached to the tx.

@ValarDragon
Copy link
Contributor

In those cases the pubkey is supplied in the tx. I agree with inclusion of pubkey when necessary, I'm only opposed to calculation of a signature on a message you don't want broadcasted, and privkey dependency.

@ValarDragon
Copy link
Contributor

This can be saved for later pr, I just don't think simulation should be default until this is done though.

@alessio
Copy link
Contributor Author

alessio commented Aug 25, 2018

In those cases the pubkey is supplied in the tx.
No signature, no pubkey really.

I agree with inclusion of pubkey when necessary, I'm only opposed to calculation of a signature on a message you don't want broadcasted, and privkey dependency.

Don't get me wrong, I believe you've got quite a point, yet txs need to be enriched with more information re: keys the msgs will be signed with to go through a simulation which could lead to potentially helpful gas estimates. And REST endpoints too are in scope here, therefore passing information via CLIContext is not a viable option.

@ValarDragon
Copy link
Contributor

ValarDragon commented Aug 25, 2018

In those cases the pubkey is supplied in the tx.

No signature, no pubkey really.

This cannot be the case. Unlike Ethereum, we don't do pubkey recovery, so in order to ever have the pubkey, it must have been explicitly provided.

Don't get me wrong, I believe you've got quite a point, yet txs need to be enriched with more information re: keys the msgs will be signed with to go through a simulation which could lead to potentially helpful gas estimates. And REST endpoints too are in scope here, therefore passing information via CLIContext is not a viable option.

I don't really understand what your suggesting here. The simulation fundamentally requires no dependency on the private key, or the signature. The gas cost relating to the signature is determined from the pubkey type. (Signatures are just []byte now, they no longer have types)

I agree CLIContext doesn't make sense here. As @alexanderbez suggested here, we should be able to use the normal context to carry information about "are we in simulation mode". (Note this refers to sdk.Context, not the CLIContext. The number of contexts is confusing)

For purposes of this PR, I think making simulation non-default suffices.

Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM

I'll write up making simulation independent of the private key in another issue.

I think refactoring the API to put simulation mode in context would be more ideal. We can definitely handle that in a separate issue / postlaunch though.

@alexanderbez
Copy link
Contributor

Good dialogue here folks! Yes, indeed, I was referring to the application's context 👍

@cwgoes cwgoes merged commit 73f90e8 into cosmos:develop Aug 28, 2018
@cwgoes
Copy link
Contributor

cwgoes commented Aug 28, 2018

Thanks @alessio - if we could get that future PR with no-sign simulation in before 0.25, that would be 💯.

@alessio alessio deleted the alessio/1246-simulate-txs branch August 28, 2018 14:27
@ValarDragon
Copy link
Contributor

ValarDragon commented Aug 28, 2018

I don't understand the push for this to be prelaunch, let alone next release. (Note this contradicts what I said earlier, but upon further thought I think it can be postlaunch) Its a nice to have (and a really cool thing at that), but something we can roll out in a non-breaking manner. The more features we keep trying to add prelaunch that aren't necessary, the longer it will take to launch.

@cwgoes
Copy link
Contributor

cwgoes commented Aug 28, 2018

I think it will be very hard for users to estimate gas otherwise, and since we charge for gas allocated, not gas used, estimating gas is pretty essential.

If there's another way to address that concern, open to it!

chillyvee pushed a commit to chillyvee/cosmos-sdk that referenced this pull request Mar 1, 2024
* Add v9 changelog

* Update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants