diff --git a/CHANGELOG.md b/CHANGELOG.md index 479f5913a..bb5d1e639 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -109,6 +109,8 @@ IMPROVEMENTS - [baseapp] Allow any alphanumeric character in route - [tools] Remove `rm -rf vendor/` from `make get_vendor_deps` - [x/auth] Recover ErrorOutOfGas panic in order to set sdk.Result attributes correctly + - [x/auth] \#2376 No longer runs any signature in a multi-msg, if any account/sequence number is wrong. + - [x/auth] \#2376 No longer charge gas for subtracting fees - [x/bank] Unit tests are now table-driven - [tests] Add tests to example apps in docs - [tests] Fixes ansible scripts to work with AWS too diff --git a/PENDING.md b/PENDING.md index b60b7e2c9..b74257127 100644 --- a/PENDING.md +++ b/PENDING.md @@ -104,6 +104,7 @@ FEATURES * [simulation] [\#1924](https://github.com/cosmos/cosmos-sdk/issues/1924) allow operations to specify future operations * [simulation] [\#1924](https://github.com/cosmos/cosmos-sdk/issues/1924) Add benchmarking capabilities, with makefile commands "test_sim_gaia_benchmark, test_sim_gaia_profile" * [simulation] [\#2349](https://github.com/cosmos/cosmos-sdk/issues/2349) Add time-based future scheduled operations to simulator + * [x/auth] \#2376 Remove FeePayer() from StdTx * [x/stake] [\#1672](https://github.com/cosmos/cosmos-sdk/issues/1672) Implement basis for the validator commission model. diff --git a/docs/sdk/core/app3.md b/docs/sdk/core/app3.md index b84313adc..595a3694f 100644 --- a/docs/sdk/core/app3.md +++ b/docs/sdk/core/app3.md @@ -133,7 +133,7 @@ Now that we have a native model for accounts, it's time to introduce the native ```go // StdTx is a standard way to wrap a Msg with Fee and Signatures. -// NOTE: the first signature is the FeePayer (Signatures must not be nil). +// NOTE: the first signature is the fee payer (Signatures must not be nil). type StdTx struct { Msgs []sdk.Msg `json:"msg"` Fee StdFee `json:"fee"` @@ -259,8 +259,7 @@ the same message could be executed over and over again. The PubKey is required for signature verification, but it is only required in the StdSignature once. From that point on, it will be stored in the account. -The fee is paid by the first address returned by `msg.GetSigners()` for the first `Msg`, -as provided by the `FeePayer(tx Tx) sdk.AccAddress` function. +The fee is paid by the first address returned by `msg.GetSigners()` for the first `Msg`. ## CoinKeeper diff --git a/docs/sdk/sdk-by-examples/simple-governance/module-handler.md b/docs/sdk/sdk-by-examples/simple-governance/module-handler.md index fc142e0fa..a879c3174 100644 --- a/docs/sdk/sdk-by-examples/simple-governance/module-handler.md +++ b/docs/sdk/sdk-by-examples/simple-governance/module-handler.md @@ -17,7 +17,7 @@ func NewHandler(k Keeper) sdk.Handler { case VoteMsg: return handleVoteMsg(ctx, k, msg) default: - errMsg := "Unrecognized gov Msg type: " + reflect.TypeOf(msg).Name() + errMsg := "Unrecognized gov Msg type: " + msg.Name() return sdk.ErrUnknownRequest(errMsg).Result() } } diff --git a/examples/democoin/x/cool/handler.go b/examples/democoin/x/cool/handler.go index 33aa9bef4..a4c6ce7be 100644 --- a/examples/democoin/x/cool/handler.go +++ b/examples/democoin/x/cool/handler.go @@ -2,7 +2,6 @@ package cool import ( "fmt" - "reflect" sdk "github.com/cosmos/cosmos-sdk/types" ) @@ -26,7 +25,7 @@ func NewHandler(k Keeper) sdk.Handler { case MsgQuiz: return handleMsgQuiz(ctx, k, msg) default: - errMsg := fmt.Sprintf("Unrecognized cool Msg type: %v", reflect.TypeOf(msg).Name()) + errMsg := fmt.Sprintf("Unrecognized cool Msg type: %v", msg.Name()) return sdk.ErrUnknownRequest(errMsg).Result() } } diff --git a/examples/democoin/x/pow/handler.go b/examples/democoin/x/pow/handler.go index e4fa39d1d..246246e96 100644 --- a/examples/democoin/x/pow/handler.go +++ b/examples/democoin/x/pow/handler.go @@ -1,8 +1,6 @@ package pow import ( - "reflect" - sdk "github.com/cosmos/cosmos-sdk/types" ) @@ -12,7 +10,7 @@ func (pk Keeper) Handler(ctx sdk.Context, msg sdk.Msg) sdk.Result { case MsgMine: return handleMsgMine(ctx, pk, msg) default: - errMsg := "Unrecognized pow Msg type: " + reflect.TypeOf(msg).Name() + errMsg := "Unrecognized pow Msg type: " + msg.Name() return sdk.ErrUnknownRequest(errMsg).Result() } } diff --git a/x/auth/account.go b/x/auth/account.go index ceadd3951..4a55a48ea 100644 --- a/x/auth/account.go +++ b/x/auth/account.go @@ -8,8 +8,12 @@ import ( "github.com/tendermint/tendermint/crypto" ) -// Account is a standard account using a sequence number for replay protection -// and a pubkey for authentication. +// Account is an interface used to store coins at a given address within state. +// It presumes a notion of sequence numbers for replay protection, +// a notion of account numbers for replay protection for previously pruned accounts, +// and a pubkey for authentication purposes. +// +// Many complex conditions can be used in the concrete struct which implements Account. type Account interface { GetAddress() sdk.AccAddress SetAddress(sdk.AccAddress) error // errors if already set. @@ -35,9 +39,11 @@ type AccountDecoder func(accountBytes []byte) (Account, error) var _ Account = (*BaseAccount)(nil) -// BaseAccount - base account structure. -// Extend this by embedding this in your AppAccount. -// See the examples/basecoin/types/account.go for an example. +// BaseAccount - a base account structure. +// This can be extended by embedding within in your AppAccount. +// There are examples of this in: examples/basecoin/types/account.go. +// However one doesn't have to use BaseAccount as long as your struct +// implements Account. type BaseAccount struct { Address sdk.AccAddress `json:"address"` Coins sdk.Coins `json:"coins"` @@ -118,7 +124,7 @@ func (acc *BaseAccount) SetSequence(seq int64) error { //---------------------------------------- // Wire -// Most users shouldn't use this, but this comes handy for tests. +// Most users shouldn't use this, but this comes in handy for tests. func RegisterBaseAccount(cdc *codec.Codec) { cdc.RegisterInterface((*Account)(nil), nil) cdc.RegisterConcrete(&BaseAccount{}, "cosmos-sdk/BaseAccount", nil) diff --git a/x/auth/ante.go b/x/auth/ante.go index f423c9465..68f6f6540 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -12,19 +12,18 @@ import ( ) const ( - deductFeesCost sdk.Gas = 10 - memoCostPerByte sdk.Gas = 1 - ed25519VerifyCost = 59 - secp256k1VerifyCost = 100 - maxMemoCharacters = 100 - feeDeductionGasFactor = 0.001 + memoCostPerByte sdk.Gas = 1 + ed25519VerifyCost = 59 + secp256k1VerifyCost = 100 + maxMemoCharacters = 100 + // how much gas = 1 atom + gasPerUnitCost = 1000 ) // NewAnteHandler returns an AnteHandler that checks // and increments sequence numbers, checks signatures & account numbers, // and deducts fees from the first signer. func NewAnteHandler(am AccountMapper, fck FeeCollectionKeeper) sdk.AnteHandler { - return func( ctx sdk.Context, tx sdk.Tx, simulate bool, ) (newCtx sdk.Context, res sdk.Result, abort bool) { @@ -35,13 +34,17 @@ func NewAnteHandler(am AccountMapper, fck FeeCollectionKeeper) sdk.AnteHandler { return ctx, sdk.ErrInternal("tx must be StdTx").Result(), true } - // set the gas meter - if simulate { - newCtx = ctx.WithGasMeter(sdk.NewInfiniteGasMeter()) - } else { - newCtx = ctx.WithGasMeter(sdk.NewGasMeter(stdTx.Fee.Gas)) + // Ensure that the provided fees meet a minimum threshold for the validator, if this is a CheckTx. + // This is only for local mempool purposes, and thus is only ran on check tx. + if ctx.IsCheckTx() && !simulate { + res := ensureSufficientMempoolFees(ctx, stdTx) + if !res.IsOK() { + return newCtx, res, true + } } + newCtx = setGasMeter(simulate, ctx, stdTx) + // AnteHandlers must have their own defer/recover in order // for the BaseApp to know how much gas was used! // This is because the GasMeter is created in the AnteHandler, @@ -65,64 +68,49 @@ func NewAnteHandler(am AccountMapper, fck FeeCollectionKeeper) sdk.AnteHandler { if err != nil { return newCtx, err.Result(), true } - - sigs := stdTx.GetSignatures() // When simulating, this would just be a 0-length slice. - signerAddrs := stdTx.GetSigners() - msgs := tx.GetMsgs() - // charge gas for the memo newCtx.GasMeter().ConsumeGas(memoCostPerByte*sdk.Gas(len(stdTx.GetMemo())), "memo") - // Get the sign bytes (requires all account & sequence numbers and the fee) - sequences := make([]int64, len(sigs)) - accNums := make([]int64, len(sigs)) - for i := 0; i < len(sigs); i++ { - sequences[i] = sigs[i].Sequence - accNums[i] = sigs[i].AccountNumber + // stdSigs contains the sequence number, account number, and signatures + stdSigs := stdTx.GetSignatures() // When simulating, this would just be a 0-length slice. + signerAddrs := stdTx.GetSigners() + + // create the list of all sign bytes + signBytesList := getSignBytesList(newCtx.ChainID(), stdTx, stdSigs) + signerAccs, res := getSignerAccs(newCtx, am, signerAddrs) + if !res.IsOK() { + return newCtx, res, true + } + res = validateAccNumAndSequence(signerAccs, stdSigs) + if !res.IsOK() { + return newCtx, res, true } - fee := stdTx.Fee - // Check sig and nonce and collect signer accounts. - var signerAccs = make([]Account, len(signerAddrs)) - for i := 0; i < len(sigs); i++ { - signerAddr, sig := signerAddrs[i], sigs[i] + // first sig pays the fees + if !stdTx.Fee.Amount.IsZero() { + // signerAccs[0] is the fee payer + signerAccs[0], res = deductFees(signerAccs[0], stdTx.Fee) + if !res.IsOK() { + return newCtx, res, true + } + fck.addCollectedFees(newCtx, stdTx.Fee.Amount) + } + for i := 0; i < len(stdSigs); i++ { // check signature, return account with incremented nonce - signBytes := StdSignBytes(newCtx.ChainID(), accNums[i], sequences[i], fee, msgs, stdTx.GetMemo()) - signerAcc, res := processSig(newCtx, am, signerAddr, sig, signBytes, simulate) + signerAccs[i], res = processSig(newCtx, signerAccs[i], stdSigs[i], signBytesList[i], simulate) if !res.IsOK() { return newCtx, res, true } - requiredFees := adjustFeesByGas(ctx.MinimumFees(), fee.Gas) - // fees must be greater than the minimum set by the validator adjusted by gas - if ctx.IsCheckTx() && !simulate && !ctx.MinimumFees().IsZero() && fee.Amount.IsLT(requiredFees) { - // validators reject any tx from the mempool with less than the minimum fee per gas * gas factor - return newCtx, sdk.ErrInsufficientFee(fmt.Sprintf( - "insufficient fee, got: %q required: %q", fee.Amount, requiredFees)).Result(), true - } - - // first sig pays the fees - // Can this function be moved outside of the loop? - if i == 0 && !fee.Amount.IsZero() { - newCtx.GasMeter().ConsumeGas(deductFeesCost, "deductFees") - signerAcc, res = deductFees(signerAcc, fee) - if !res.IsOK() { - return newCtx, res, true - } - fck.addCollectedFees(newCtx, fee.Amount) - } - // Save the account. - am.SetAccount(newCtx, signerAcc) - signerAccs[i] = signerAcc + am.SetAccount(newCtx, signerAccs[i]) } // cache the signer accounts in the context newCtx = WithSigners(newCtx, signerAccs) // TODO: tx tags (?) - return newCtx, sdk.Result{GasWanted: stdTx.Fee.Gas}, false // continue... } } @@ -150,42 +138,45 @@ func validateBasic(tx StdTx) (err sdk.Error) { return nil } +func getSignerAccs(ctx sdk.Context, am AccountMapper, addrs []sdk.AccAddress) (accs []Account, res sdk.Result) { + accs = make([]Account, len(addrs)) + for i := 0; i < len(accs); i++ { + accs[i] = am.GetAccount(ctx, addrs[i]) + if accs[i] == nil { + return nil, sdk.ErrUnknownAddress(addrs[i].String()).Result() + } + } + return +} + +func validateAccNumAndSequence(accs []Account, sigs []StdSignature) sdk.Result { + for i := 0; i < len(accs); i++ { + accnum := accs[i].GetAccountNumber() + seq := accs[i].GetSequence() + // Check account number. + if accnum != sigs[i].AccountNumber { + return sdk.ErrInvalidSequence( + fmt.Sprintf("Invalid account number. Got %d, expected %d", sigs[i].AccountNumber, accnum)).Result() + } + + // Check sequence number. + if seq != sigs[i].Sequence { + return sdk.ErrInvalidSequence( + fmt.Sprintf("Invalid sequence. Got %d, expected %d", sigs[i].Sequence, seq)).Result() + } + } + return sdk.Result{} +} + // verify the signature and increment the sequence. // if the account doesn't have a pubkey, set it. -func processSig( - ctx sdk.Context, am AccountMapper, - addr sdk.AccAddress, sig StdSignature, signBytes []byte, simulate bool) ( - acc Account, res sdk.Result) { - // Get the account. - acc = am.GetAccount(ctx, addr) - if acc == nil { - return nil, sdk.ErrUnknownAddress(addr.String()).Result() - } - - accnum := acc.GetAccountNumber() - seq := acc.GetSequence() - - // Check account number. - if accnum != sig.AccountNumber { - return nil, sdk.ErrInvalidSequence( - fmt.Sprintf("Invalid account number. Got %d, expected %d", sig.AccountNumber, accnum)).Result() - } - - // Check sequence number. - if seq != sig.Sequence { - return nil, sdk.ErrInvalidSequence( - fmt.Sprintf("Invalid sequence. Got %d, expected %d", sig.Sequence, seq)).Result() - } - err := acc.SetSequence(seq + 1) - if err != nil { - // Handle w/ #870 - panic(err) - } +func processSig(ctx sdk.Context, + acc Account, sig StdSignature, signBytes []byte, simulate bool) (updatedAcc Account, res sdk.Result) { pubKey, res := processPubKey(acc, sig, simulate) if !res.IsOK() { return nil, res } - err = acc.SetPubKey(pubKey) + err := acc.SetPubKey(pubKey) if err != nil { return nil, sdk.ErrInternal("setting PubKey on signer's account").Result() } @@ -195,7 +186,14 @@ func processSig( return nil, sdk.ErrUnauthorized("signature verification failed").Result() } - return + // increment the sequence number + err = acc.SetSequence(acc.GetSequence() + 1) + if err != nil { + // Handle w/ #870 + panic(err) + } + + return acc, res } var dummySecp256k1Pubkey secp256k1.PubKeySecp256k1 @@ -244,8 +242,9 @@ func consumeSignatureVerificationGas(meter sdk.GasMeter, pubkey crypto.PubKey) { } func adjustFeesByGas(fees sdk.Coins, gas int64) sdk.Coins { - gasCost := int64(float64(gas) * feeDeductionGasFactor) + gasCost := gas / gasPerUnitCost gasFees := make(sdk.Coins, len(fees)) + // TODO: Make this not price all coins in the same way for i := 0; i < len(fees); i++ { gasFees[i] = sdk.NewInt64Coin(fees[i].Denom, gasCost) } @@ -272,5 +271,37 @@ func deductFees(acc Account, fee StdFee) (Account, sdk.Result) { return acc, sdk.Result{} } +func ensureSufficientMempoolFees(ctx sdk.Context, stdTx StdTx) sdk.Result { + // currently we use a very primitive gas pricing model with a constant gasPrice. + // adjustFeesByGas handles calculating the amount of fees required based on the provided gas. + // TODO: Make the gasPrice not a constant, and account for tx size. + requiredFees := adjustFeesByGas(ctx.MinimumFees(), stdTx.Fee.Gas) + + if !ctx.MinimumFees().IsZero() && stdTx.Fee.Amount.IsLT(requiredFees) { + // validators reject any tx from the mempool with less than the minimum fee per gas * gas factor + return sdk.ErrInsufficientFee(fmt.Sprintf( + "insufficient fee, got: %q required: %q", stdTx.Fee.Amount, requiredFees)).Result() + } + return sdk.Result{} +} + +func setGasMeter(simulate bool, ctx sdk.Context, stdTx StdTx) sdk.Context { + // set the gas meter + if simulate { + return ctx.WithGasMeter(sdk.NewInfiniteGasMeter()) + } + return ctx.WithGasMeter(sdk.NewGasMeter(stdTx.Fee.Gas)) +} + +func getSignBytesList(chainID string, stdTx StdTx, stdSigs []StdSignature) (signatureBytesList [][]byte) { + signatureBytesList = make([][]byte, len(stdSigs)) + for i := 0; i < len(stdSigs); i++ { + signatureBytesList[i] = StdSignBytes(chainID, + stdSigs[i].AccountNumber, stdSigs[i].Sequence, + stdTx.Fee, stdTx.Msgs, stdTx.Memo) + } + return +} + // BurnFeeHandler burns all fees (decreasing total supply) func BurnFeeHandler(_ sdk.Context, _ sdk.Tx, _ sdk.Coins) {} diff --git a/x/auth/stdtx.go b/x/auth/stdtx.go index 0c0a133ed..ca6eb7b42 100644 --- a/x/auth/stdtx.go +++ b/x/auth/stdtx.go @@ -11,7 +11,7 @@ import ( var _ sdk.Tx = (*StdTx)(nil) // StdTx is a standard way to wrap a Msg with Fee and Signatures. -// NOTE: the first signature is the FeePayer (Signatures must not be nil). +// NOTE: the first signature is the fee payer (Signatures must not be nil). type StdTx struct { Msgs []sdk.Msg `json:"msg"` Fee StdFee `json:"fee"` @@ -63,13 +63,6 @@ func (tx StdTx) GetMemo() string { return tx.Memo } // .Empty(). func (tx StdTx) GetSignatures() []StdSignature { return tx.Signatures } -// FeePayer returns the address responsible for paying the fees -// for the transactions. It's the first address returned by msg.GetSigners(). -// If GetSigners() is empty, this panics. -func FeePayer(tx sdk.Tx) sdk.AccAddress { - return tx.GetMsgs()[0].GetSigners()[0] -} - //__________________________________________________________ // StdFee includes the amount of coins paid in fees and the maximum diff --git a/x/auth/stdtx_test.go b/x/auth/stdtx_test.go index 8d9172a92..26bd79245 100644 --- a/x/auth/stdtx_test.go +++ b/x/auth/stdtx_test.go @@ -23,7 +23,7 @@ func TestStdTx(t *testing.T) { require.Equal(t, msgs, tx.GetMsgs()) require.Equal(t, sigs, tx.GetSignatures()) - feePayer := FeePayer(tx) + feePayer := tx.GetSigners()[0] require.Equal(t, addr, feePayer) } diff --git a/x/bank/handler.go b/x/bank/handler.go index ec56d05b4..cd60ac835 100644 --- a/x/bank/handler.go +++ b/x/bank/handler.go @@ -1,8 +1,6 @@ package bank import ( - "reflect" - sdk "github.com/cosmos/cosmos-sdk/types" ) @@ -15,7 +13,7 @@ func NewHandler(k Keeper) sdk.Handler { case MsgIssue: return handleMsgIssue(ctx, k, msg) default: - errMsg := "Unrecognized bank Msg type: " + reflect.TypeOf(msg).Name() + errMsg := "Unrecognized bank Msg type: %s" + msg.Name() return sdk.ErrUnknownRequest(errMsg).Result() } } diff --git a/x/ibc/handler.go b/x/ibc/handler.go index 1f334166b..4a9eebaeb 100644 --- a/x/ibc/handler.go +++ b/x/ibc/handler.go @@ -1,8 +1,6 @@ package ibc import ( - "reflect" - sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/bank" ) @@ -15,7 +13,7 @@ func NewHandler(ibcm Mapper, ck bank.Keeper) sdk.Handler { case IBCReceiveMsg: return handleIBCReceiveMsg(ctx, ibcm, ck, msg) default: - errMsg := "Unrecognized IBC Msg type: " + reflect.TypeOf(msg).Name() + errMsg := "Unrecognized IBC Msg type: " + msg.Name() return sdk.ErrUnknownRequest(errMsg).Result() } }