diff --git a/CHANGELOG.md b/CHANGELOG.md index 16b5fe41e136..f33de414ce93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * [\#10045](https://github.com/cosmos/cosmos-sdk/pull/10045) Revert [#8549](https://github.com/cosmos/cosmos-sdk/pull/8549). Do not route grpc queries through Tendermint. * [\#10326](https://github.com/cosmos/cosmos-sdk/pull/10326) `x/authz` add query all grants by granter query. * [\#10024](https://github.com/cosmos/cosmos-sdk/pull/10024) `store/cachekv` performance improvement by reduced growth factor for iterator ranging by using binary searches to find dirty items when unsorted key count >= 1024 +* [\#10348](https://github.com/cosmos/cosmos-sdk/pull/10348) Add `fee.{payer,granter}` and `tip` fields to StdSignDoc for signing tipped transactions. ### API Breaking Changes @@ -98,8 +99,9 @@ Ref: https://keepachangelog.com/en/1.0.0/ * Replace `baseapp.SetAnteHandler` with `baseapp.SetTxHandler`. * Move Msg routers from BaseApp to middlewares. * Move Baseapp panic recovery into a middleware. - * Rename simulation helper methods `baseapp.{Check,Deliver}` to `baseapp.Sim{Check,Deliver}**. + * Rename simulation helper methods `baseapp.{Check,Deliver}` to `baseapp.Sim{Check,Deliver}**`. * (x/gov) [\#10373](https://github.com/cosmos/cosmos-sdk/pull/10373) Removed gov `keeper.{MustMarshal, MustUnmarshal}`. +* [\#10348](https://github.com/cosmos/cosmos-sdk/pull/10348) StdSignBytes takes a new argument of type `*tx.Tip` for signing over tips using LEGACY_AMINO_JSON. ### Client Breaking Changes diff --git a/docs/architecture/adr-040-storage-and-smt-state-commitments.md b/docs/architecture/adr-040-storage-and-smt-state-commitments.md index 4ee109b6c4df..c97cc15ed998 100644 --- a/docs/architecture/adr-040-storage-and-smt-state-commitments.md +++ b/docs/architecture/adr-040-storage-and-smt-state-commitments.md @@ -48,7 +48,7 @@ For data access we propose 2 additional KV buckets (implemented as namespaces fo 2. B2: `hash(key) → key`: a reverse index to get a key from an SMT path. Internally the SMT will store `(key, value)` as `prefix || hash(key) || hash(value)`. So, we can get an object value by composing `hash(key) → B2 → B1`. 3. We could use more buckets to optimize the app usage if needed. -Above, we propose to use a KV DB. However, for the state machine, we could use an RDBMS, which we discuss below. +We propose to use a KV database for both `SS` and `SC`. The store interface will allow to use the same physical DB backend for both `SS` and `SC` as well two separate DBs. The latter option allows for the separation of `SS` and `SC` into different hardware units, providing support for more complex setup scenarios and improving overall performance: one can use different backends (eg RocksDB and Badger) as well as independently tuning the underlying DB configuration. ### Requirements @@ -64,6 +64,7 @@ State Commitment requirements: + fast updates + tree path should be short ++ query historical commitment proofs using ICS-23 standard + pruning (garbage collection) ### SMT for State Commitment @@ -82,27 +83,27 @@ The full specification can be found at [Celestia](https://github.com/celestiaorg Below, with simple _snapshot_ we refer to a database snapshot mechanism, not to a _ABCI snapshot sync_. The latter will be referred as _snapshot sync_ (which will directly use DB snapshot as described below). -Database snapshot is a view of DB state at a certain time or transaction. It's not a full copy of a database (it would be too big), usually a snapshot mechanism is based on a _copy on write_ and it allows to efficiently deliver DB state at a certain stage. -Some DB engines support snapshotting. Hence, we propose to reuse that functionality for the state sync and versioning (described below). It will the supported DB engines to ones which efficiently implement snapshots. In a final section we will discuss evaluated DBs. +Database snapshot is a view of DB state at a certain time or transaction. It's not a full copy of a database (it would be too big). Usually a snapshot mechanism is based on a _copy on write_ and it allows DB state to be efficiently delivered at a certain stage. +Some DB engines support snapshotting. Hence, we propose to reuse that functionality for the state sync and versioning (described below). We limit the supported DB engines to ones which efficiently implement snapshots. In a final section we discuss the evaluated DBs. One of the Stargate core features is a _snapshot sync_ delivered in the `/snapshot` package. It provides a way to trustlessly sync a blockchain without repeating all transactions from the genesis. This feature is implemented in Cosmos SDK and requires storage support. Currently IAVL is the only supported backend. It works by streaming to a client a snapshot of a `SS` at a certain version together with a header chain. -A new `SS` snapshot will be created in every `EndBlocker` and identified by a block height. The `rootmulti.Store` keeps track of the available snapshots to offer `SS` at a certain version. The `rootmulti.Store` implements the `CommitMultiStore` interface, which encapsulates a `Committer` interface. `Committer` has a `Commit`, `SetPruning`, `GetPruning` functions which will be used for creating and removing snapshots. The `rootStore.Commit` function creates a new snapshot and increments the version on each call, and checks if it needs to remove old versions. We will need to update the SMT interface to implement the `Committer` interface. +A new database snapshot will be created in every `EndBlocker` and identified by a block height. The `root` store keeps track of the available snapshots to offer `SS` at a certain version. The `root` store implements the `RootStore` interface described below. In essence, `RootStore` encapsulates a `Committer` interface. `Committer` has a `Commit`, `SetPruning`, `GetPruning` functions which will be used for creating and removing snapshots. The `rootStore.Commit` function creates a new snapshot and increments the version on each call, and checks if it needs to remove old versions. We will need to update the SMT interface to implement the `Committer` interface. NOTE: `Commit` must be called exactly once per block. Otherwise we risk going out of sync for the version number and block height. NOTE: For the Cosmos SDK storage, we may consider splitting that interface into `Committer` and `PruningCommitter` - only the multiroot should implement `PruningCommitter` (cache and prefix store don't need pruning). -Number of historical versions for `abci.Query` and state sync snapshots is part of a node configuration, not a chain configuration (configuration implied by the blockchain consensus). A configuration should allow to specify number of past blocks and number of past blocks modulo some number (eg: 100 past blocks and one snapshot every 100 blocks for past 2000 blocks). Archival nodes can keep all past versions. +Number of historical versions for `abci.RequestQuery` and state sync snapshots is part of a node configuration, not a chain configuration (configuration implied by the blockchain consensus). A configuration should allow to specify number of past blocks and number of past blocks modulo some number (eg: 100 past blocks and one snapshot every 100 blocks for past 2000 blocks). Archival nodes can keep all past versions. -Pruning old snapshots is effectively done by a database. Whenever we update a record in `SC`, SMT won't update nodes - instead it creates new nodes on the update path, without removing the old one. Since we are snapshoting each block, we need to update that mechanism to immediately remove orphaned nodes from the storage. This is a safe operation - snapshots will keep track of the records which should be available for past versions. +Pruning old snapshots is effectively done by a database. Whenever we update a record in `SC`, SMT won't update nodes - instead it creates new nodes on the update path, without removing the old one. Since we are snapshotting each block, we need to change that mechanism to immediately remove orphaned nodes from the database. This is a safe operation - snapshots will keep track of the records and make it available when accessing past versions. -To manage the active snapshots we will either us a DB _max number of snapshots_ option (if available), or will remove snapshots in the `EndBlocker`. The latter option can be done efficiently by identifying snapshots with block height. +To manage the active snapshots we will either use a DB _max number of snapshots_ option (if available), or we will remove DB snapshots in the `EndBlocker`. The latter option can be done efficiently by identifying snapshots with block height and calling a store function to remove past versions. #### Accessing old state versions -One of the functional requirements is to access old state. This is done through `abci.Query` structure. The version is specified by a block height (so we query for an object by a key `K` at block height `H`). The number of old versions supported for `abci.Query` is configurable. Accessing an old state is done by using available snapshots. -`abci.Query` doesn't need old state of `SC`. So, for efficiency, we should keep `SC` and `SS` in different databases (however using the same DB engine). +One of the functional requirements is to access old state. This is done through `abci.RequestQuery` structure. The version is specified by a block height (so we query for an object by a key `K` at block height `H`). The number of old versions supported for `abci.RequestQuery` is configurable. Accessing an old state is done by using available snapshots. +`abci.RequestQuery` doesn't need old state of `SC` unless the `prove=true` parameter is set. The SMT merkle proof must be included in the `abci.ResponseQuery` only if both `SC` and `SS` have a snapshot for requested version. -Moreover, Cosmos SDK could provide a way to directly access the state. However, a state machine shouldn't do that - since the number of snapshots is configurable, it would lead to nondeterministic execution. +Moreover, Cosmos SDK could provide a way to directly access a historical state. However, a state machine shouldn't do that - since the number of snapshots is configurable, it would lead to nondeterministic execution. We positively [validated](https://github.com/cosmos/cosmos-sdk/discussions/8297) a versioning and snapshot mechanism for querying old state with regards to the database we evaluated. @@ -118,6 +119,93 @@ We need to be able to process transactions and roll-back state updates if a tran We identified use-cases, where modules will need to save an object commitment without storing an object itself. Sometimes clients are receiving complex objects, and they have no way to prove a correctness of that object without knowing the storage layout. For those use cases it would be easier to commit to the object without storing it directly. + +### Refactor MultiStore + +The Stargate `/store` implementation (store/v1) adds an additional layer in the SDK store construction - the `MultiStore` structure. The multistore exists to support the modularity of the Cosmos SDK - each module is using its own instance of IAVL, but in the current implementation, all instances share the same database. The latter indicates, however, that the implementation doesn't provide true modularity. Instead it causes problems related to race condition and atomic DB commits (see: [\#6370](https://github.com/cosmos/cosmos-sdk/issues/6370) and [discussion](https://github.com/cosmos/cosmos-sdk/discussions/8297#discussioncomment-757043)). + +We propose to reduce the multistore concept from the SDK, and to use a single instance of `SC` and `SS` in a `RootStore` object. To avoid confusion, we should rename the `MultiStore` interface to `RootStore`. The `RootStore` will have the following interface; the methods for configuring tracing and listeners are omitted for brevity. + +```go +// Used where read-only access to versions is needed. +type BasicRootStore interface { + Store + GetKVStore(StoreKey) KVStore + CacheRootStore() CacheRootStore +} + +// Used as the main app state, replacing CommitMultiStore. +type CommitRootStore interface { + BasicRootStore + Committer + Snapshotter + + GetVersion(uint64) (BasicRootStore, error) + SetInitialVersion(uint64) error + + ... // Trace and Listen methods +} + +// Replaces CacheMultiStore for branched state. +type CacheRootStore interface { + BasicRootStore + Write() + + ... // Trace and Listen methods +} + +// Example of constructor parameters for the concrete type. +type RootStoreConfig struct { + Upgrades *StoreUpgrades + InitialVersion uint64 + + ReservePrefix(StoreKey, StoreType) +} +``` + + + + +In contrast to `MultiStore`, `RootStore` doesn't allow to dynamically mount sub-stores or provide an arbitrary backing DB for individual sub-stores. + +NOTE: modules will be able to use a special commitment and their own DBs. For example: a module which will use ZK proofs for state can store and commit this proof in the `RootStore` (usually as a single record) and manage the specialized store privately or using the `SC` low level interface. + +#### Compatibility support + +To ease the transition to this new interface for users, we can create a shim which wraps a `CommitMultiStore` but provides a `CommitRootStore` interface, and expose functions to safely create and access the underlying `CommitMultiStore`. + +The new `RootStore` and supporting types can be implemented in a `store/v2` package to avoid breaking existing code. + +#### Merkle Proofs and IBC + +Currently, an IBC (v1.0) Merkle proof path consists of two elements (`["", ""]`), with each key corresponding to a separate proof. These are each verified according to individual [ICS-23 specs](https://github.com/cosmos/ibc-go/blob/f7051429e1cf833a6f65d51e6c3df1609290a549/modules/core/23-commitment/types/merkle.go#L17), and the result hash of each step is used as the committed value of the next step, until a root commitment hash is obtained. +The root hash of the proof for `""` is hashed with the `""` to validate against the App Hash. + +This is not compatible with the `RootStore`, which stores all records in a single Merkle tree structure, and won't produce separate proofs for the store- and record-key. Ideally, the store-key component of the proof could just be omitted, and updated to use a "no-op" spec, so only the record-key is used. However, because the IBC verification code hardcodes the `"ibc"` prefix and applies it to the SDK proof as a separate element of the proof path, this isn't possible without a breaking change. Breaking this behavior would severely impact the Cosmos ecosystem which already widely adopts the IBC module. Requesting an update of the IBC module across the chains is a time consuming effort and not easily feasible. + +As a workaround, the `RootStore` will have to use two separate SMTs (they could use the same underlying DB): one for IBC state and one for everything else. A simple Merkle map that reference these SMTs will act as a Merkle Tree to create a final App hash. The Merkle map is not stored in a DBs - it's constructed in the runtime. The IBC substore key must be `"ibc"`. + +The workaround can still guarantee atomic syncs: the [proposed DB backends](#evaluated-kv-databases) support atomic transactions and efficient rollbacks, which will be used in the commit phase. + +The presented workaround can be used until the IBC module is fully upgraded to supports single-element commitment proofs. + +### Optimization: compress module key prefixes + +We consider a compression of prefix keys by creating a mapping from module key to an integer, and serializing the integer using varint coding. Varint coding assures that different values don't have common byte prefix. For Merkle Proofs we can't use prefix compression - so it should only apply for the `SS` keys. Moreover, the prefix compression should be only applied for the module namespace. More precisely: ++ each module has it's own namespace; ++ when accessing a module namespace we create a KVStore with embedded prefix; ++ that prefix will be compressed only when accessing and managing `SS`. + +We need to assure that the codes won't change. We can fix the mapping in a static variable (provided by an app) or SS state under a special key. + +TODO: need to make decision about the key compression. + +## Optimization: SS key compression + +Some objects may be saved with key, which contains a Protobuf message type. Such keys are long. We could save a lot of space if we can map Protobuf message types in varints. + +TODO: finalize this or move to another ADR. + ## Consequences ### Backwards Compatibility @@ -131,11 +219,14 @@ We change the storage layout of the state machine, a storage hard fork and netwo + Decoupling state from state commitment introduce better engineering opportunities for further optimizations and better storage patterns. + Performance improvements. + Joining SMT based camp which has wider and proven adoption than IAVL. Example projects which decided on SMT: Ethereum2, Diem (Libra), Trillan, Tezos, Celestia. ++ Multistore removal fixes a longstanding issue with the current MultiStore design. ++ Simplifies merkle proofs - all modules, except IBC, have only one pass for merkle proof. ### Negative + Storage migration + LL SMT doesn't support pruning - we will need to add and test that functionality. ++ `SS` keys will have an overhead of a key prefix. This doesn't impact `SC` because all keys in `SC` have same size (they are hashed). ### Neutral @@ -170,3 +261,5 @@ We were discussing use case where modules can use a support database, which is n + Facebook Diem (Libra) SMT [design](https://developers.diem.com/papers/jellyfish-merkle-tree/2021-01-14.pdf) + [Trillian Revocation Transparency](https://github.com/google/trillian/blob/master/docs/papers/RevocationTransparency.pdf), [Trillian Verifiable Data Structures](https://github.com/google/trillian/blob/master/docs/papers/VerifiableDataStructures.pdf). + Design and implementation [discussion](https://github.com/cosmos/cosmos-sdk/discussions/8297). ++ [How to Upgrade IBC Chains and their Clients](https://github.com/cosmos/ibc-go/blob/main/docs/ibc/upgrades/quick-guide.md) ++ [ADR-40 Effect on IBC](https://github.com/cosmos/ibc-go/discussions/256) diff --git a/docs/core/upgrade.md b/docs/core/upgrade.md index c4abb1eadbe1..2a3844161b81 100644 --- a/docs/core/upgrade.md +++ b/docs/core/upgrade.md @@ -15,7 +15,7 @@ The Cosmos SDK uses two methods to perform upgrades. - Exporting the entire application state to a JSON file using the `export` CLI command, making changes, and then starting a new binary with the changed JSON file as the genesis file. See [Chain Upgrade Guide to v0.42](https://docs.cosmos.network/v0.42/migrations/chain-upgrade-guide-040.html). - Version v0.44 and later can perform upgrades in place to significantly decrease the upgrade time for chains with a larger state. Use the [Module Upgrade Guide](../building-modules/upgrade.md) to set up your application modules to take advantage of in-place upgrades. - + This document provides steps to use the In-Place Store Migrations upgrade method. ## Tracking Module Versions @@ -95,7 +95,7 @@ if err != nil { if upgradeInfo.Name == "my-plan" && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) { storeUpgrades := storetypes.StoreUpgrades{ // add store upgrades for new modules - // Example: + // Example: // Added: []string{"foo", "bar"}, // ... } @@ -119,7 +119,7 @@ You MUST manually set the consensus version in the version map passed to the `Up import foo "github.com/my/module/foo" app.UpgradeKeeper.SetUpgradeHandler("my-plan", func(ctx sdk.Context, plan upgradetypes.Plan, vm module.VersionMap) (module.VersionMap, error) { - + // Register the consensus version in the version map // to avoid the SDK from triggering the default // InitGenesis function. @@ -138,7 +138,7 @@ If you do not have a custom genesis function and want to skip the module's defau import foo "github.com/my/module/foo" app.UpgradeKeeper.SetUpgradeHandler("my-plan", func(ctx sdk.Context, plan upgradetypes.Plan, vm module.VersionMap) (module.VersionMap, error) { - + // Set foo's version to the latest ConsensusVersion in the VersionMap. // This will skip running InitGenesis on Foo vm["foo"] = foo.AppModule{}.ConsensusVersion() @@ -151,6 +151,6 @@ app.UpgradeKeeper.SetUpgradeHandler("my-plan", func(ctx sdk.Context, plan upgrad You can sync a full node to an existing blockchain which has been upgraded using Cosmovisor -In order to successfully sync, you must start with the initial binary that the blockchain started with at genesis. Cosmovisor will handle downloading and switching to the binaries associated with each sequential upgrade. +In order to successfully sync, you must start with the initial binary that the blockchain started with at genesis. If all Software Upgrade Plans contain binary instruction then you can run Cosmovisor with auto download option to automatically handle downloading and switching to the binaries associated with each sequential upgrade. Otherwise you need to manually provide all binaries to Cosmovisor. To learn more about Cosmovisor, see the [Cosmovisor Quick Start](../run-node/cosmovisor.md). diff --git a/types/module/module.go b/types/module/module.go index 685a6dcd601b..6e1012c0ac8a 100644 --- a/types/module/module.go +++ b/types/module/module.go @@ -415,32 +415,25 @@ func (m Manager) RunMigrations(ctx sdk.Context, cfg Configurator, fromVM Version fromVersion, exists := fromVM[moduleName] toVersion := module.ConsensusVersion() - // Only run migrations when the module exists in the fromVM. - // Run InitGenesis otherwise. + // We run migration if the module is specified in `fromVM`. + // Otherwise we run InitGenesis. // - // the module won't exist in the fromVM in two cases: + // The module won't exist in the fromVM in two cases: // 1. A new module is added. In this case we run InitGenesis with an // empty genesis state. - // 2. An existing chain is upgrading to v043 for the first time. In this case, - // all modules have yet to be added to x/upgrade's VersionMap store. + // 2. An existing chain is upgrading from version < 0.43 to v0.43+ for the first time. + // In this case, all modules have yet to be added to x/upgrade's VersionMap store. if exists { err := c.runModuleMigrations(ctx, moduleName, fromVersion, toVersion) if err != nil { return nil, err } } else { - cfgtor, ok := cfg.(configurator) - if !ok { - // Currently, the only implementator of Configurator (the interface) - // is configurator (the struct). - return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "expected %T, got %T", configurator{}, cfg) - } - - moduleValUpdates := module.InitGenesis(ctx, cfgtor.cdc, module.DefaultGenesis(cfgtor.cdc)) + moduleValUpdates := module.InitGenesis(ctx, c.cdc, module.DefaultGenesis(c.cdc)) // The module manager assumes only one module will update the - // validator set, and that it will not be by a new module. + // validator set, and it can't be a new module. if len(moduleValUpdates) > 0 { - return nil, sdkerrors.Wrapf(sdkerrors.ErrLogic, "validator InitGenesis updates already set by a previous module") + return nil, sdkerrors.Wrapf(sdkerrors.ErrLogic, "validator InitGenesis update is already set by another module") } } diff --git a/types/tx/tips.go b/types/tx/tips.go new file mode 100644 index 000000000000..ca8afb36d5ae --- /dev/null +++ b/types/tx/tips.go @@ -0,0 +1,11 @@ +package tx + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" +) + +// TipTx defines the interface to be implemented by Txs that handle Tips. +type TipTx interface { + sdk.FeeTx + GetTip() *Tip +} diff --git a/x/auth/migrations/legacytx/amino_signing.go b/x/auth/migrations/legacytx/amino_signing.go index 2f5b1d4a4218..4115efa70493 100644 --- a/x/auth/migrations/legacytx/amino_signing.go +++ b/x/auth/migrations/legacytx/amino_signing.go @@ -43,7 +43,7 @@ func (stdTxSignModeHandler) GetSignBytes(mode signingtypes.SignMode, data signin } return StdSignBytes( - data.ChainID, data.AccountNumber, data.Sequence, stdTx.GetTimeoutHeight(), StdFee{Amount: stdTx.GetFee(), Gas: stdTx.GetGas()}, tx.GetMsgs(), stdTx.GetMemo(), + data.ChainID, data.AccountNumber, data.Sequence, stdTx.GetTimeoutHeight(), StdFee{Amount: stdTx.GetFee(), Gas: stdTx.GetGas()}, tx.GetMsgs(), stdTx.GetMemo(), nil, ), nil } diff --git a/x/auth/migrations/legacytx/amino_signing_test.go b/x/auth/migrations/legacytx/amino_signing_test.go index 89410d6c8948..3342c2dba01b 100644 --- a/x/auth/migrations/legacytx/amino_signing_test.go +++ b/x/auth/migrations/legacytx/amino_signing_test.go @@ -55,7 +55,7 @@ func TestLegacyAminoJSONHandler_GetSignBytes(t *testing.T) { signBz, err := handler.GetSignBytes(signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, signingData, tx) require.NoError(t, err) - expectedSignBz := StdSignBytes(chainId, accNum, seqNum, timeoutHeight, fee, msgs, memo) + expectedSignBz := StdSignBytes(chainId, accNum, seqNum, timeoutHeight, fee, msgs, memo, nil) require.Equal(t, expectedSignBz, signBz) diff --git a/x/auth/migrations/legacytx/stdsign.go b/x/auth/migrations/legacytx/stdsign.go index d22e3786d9f0..9467ee8027c8 100644 --- a/x/auth/migrations/legacytx/stdsign.go +++ b/x/auth/migrations/legacytx/stdsign.go @@ -13,6 +13,7 @@ import ( "github.com/cosmos/cosmos-sdk/crypto/types/multisig" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + "github.com/cosmos/cosmos-sdk/types/tx" "github.com/cosmos/cosmos-sdk/types/tx/signing" ) @@ -47,10 +48,11 @@ type StdSignDoc struct { Memo string `json:"memo" yaml:"memo"` Fee json.RawMessage `json:"fee" yaml:"fee"` Msgs []json.RawMessage `json:"msgs" yaml:"msgs"` + Tip *StdTip `json:"tip,omitempty" yaml:"tip"` } // StdSignBytes returns the bytes to sign for a transaction. -func StdSignBytes(chainID string, accnum, sequence, timeout uint64, fee StdFee, msgs []sdk.Msg, memo string) []byte { +func StdSignBytes(chainID string, accnum, sequence, timeout uint64, fee StdFee, msgs []sdk.Msg, memo string, tip *tx.Tip) []byte { msgsBytes := make([]json.RawMessage, 0, len(msgs)) for _, msg := range msgs { legacyMsg, ok := msg.(LegacyMsg) @@ -61,6 +63,15 @@ func StdSignBytes(chainID string, accnum, sequence, timeout uint64, fee StdFee, msgsBytes = append(msgsBytes, json.RawMessage(legacyMsg.GetSignBytes())) } + var stdTip *StdTip + if tip != nil { + if tip.Tipper == "" { + panic(fmt.Errorf("tipper cannot be empty")) + } + + stdTip = &StdTip{Amount: tip.Amount, Tipper: tip.Tipper} + } + bz, err := legacy.Cdc.MarshalJSON(StdSignDoc{ AccountNumber: accnum, ChainID: chainID, @@ -69,6 +80,7 @@ func StdSignBytes(chainID string, accnum, sequence, timeout uint64, fee StdFee, Msgs: msgsBytes, Sequence: sequence, TimeoutHeight: timeout, + Tip: stdTip, }) if err != nil { panic(err) @@ -106,7 +118,6 @@ func (ss StdSignature) MarshalYAML() (interface{}, error) { pk = ss.PubKey.String() } - bz, err := yaml.Marshal(struct { PubKey string `json:"pub_key"` Signature string `json:"signature"` diff --git a/x/auth/migrations/legacytx/stdsignmsg.go b/x/auth/migrations/legacytx/stdsignmsg.go index 07ee29a06324..56e68b0f5b6f 100644 --- a/x/auth/migrations/legacytx/stdsignmsg.go +++ b/x/auth/migrations/legacytx/stdsignmsg.go @@ -21,7 +21,7 @@ type StdSignMsg struct { // get message bytes func (msg StdSignMsg) Bytes() []byte { - return StdSignBytes(msg.ChainID, msg.AccountNumber, msg.Sequence, msg.TimeoutHeight, msg.Fee, msg.Msgs, msg.Memo) + return StdSignBytes(msg.ChainID, msg.AccountNumber, msg.Sequence, msg.TimeoutHeight, msg.Fee, msg.Msgs, msg.Memo, nil) } func (msg StdSignMsg) UnpackInterfaces(unpacker types.AnyUnpacker) error { diff --git a/x/auth/migrations/legacytx/stdtx.go b/x/auth/migrations/legacytx/stdtx.go index 55ec38e603cc..68f3b330eae8 100644 --- a/x/auth/migrations/legacytx/stdtx.go +++ b/x/auth/migrations/legacytx/stdtx.go @@ -25,8 +25,10 @@ var ( // which must be above some miminum to be accepted into the mempool. // [Deprecated] type StdFee struct { - Amount sdk.Coins `json:"amount" yaml:"amount"` - Gas uint64 `json:"gas" yaml:"gas"` + Amount sdk.Coins `json:"amount" yaml:"amount"` + Gas uint64 `json:"gas" yaml:"gas"` + Payer string `json:"payer,omitempty" yaml:"payer"` + Granter string `json:"granter,omitempty" yaml:"granter"` } // Deprecated: NewStdFee returns a new instance of StdFee @@ -70,6 +72,12 @@ func (fee StdFee) GasPrices() sdk.DecCoins { return sdk.NewDecCoinsFromCoins(fee.Amount...).QuoDec(sdk.NewDec(int64(fee.Gas))) } +// StdTip is the tips used in a tipped transaction. +type StdTip struct { + Amount sdk.Coins `json:"amount" yaml:"amount"` + Tipper string `json:"tipper" yaml:"tipper"` +} + // StdTx is the legacy transaction format for wrapping a Msg with Fee and Signatures. // It only works with Amino, please prefer the new protobuf Tx in types/tx. // NOTE: the first signature is the fee payer (Signatures must not be nil). diff --git a/x/auth/migrations/legacytx/stdtx_test.go b/x/auth/migrations/legacytx/stdtx_test.go index ec70281f6828..650f6f4b155c 100644 --- a/x/auth/migrations/legacytx/stdtx_test.go +++ b/x/auth/migrations/legacytx/stdtx_test.go @@ -17,6 +17,7 @@ import ( "github.com/cosmos/cosmos-sdk/testutil/testdata" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + "github.com/cosmos/cosmos-sdk/types/tx" "github.com/cosmos/cosmos-sdk/types/tx/signing" ) @@ -41,7 +42,7 @@ func NewTestStdFee() StdFee { func NewTestTx(ctx sdk.Context, msgs []sdk.Msg, privs []cryptotypes.PrivKey, accNums []uint64, seqs []uint64, timeout uint64, fee StdFee) sdk.Tx { sigs := make([]StdSignature, len(privs)) for i, priv := range privs { - signBytes := StdSignBytes(ctx.ChainID(), accNums[i], seqs[i], timeout, fee, msgs, "") + signBytes := StdSignBytes(ctx.ChainID(), accNums[i], seqs[i], timeout, fee, msgs, "", nil) sig, err := priv.Sign(signBytes) if err != nil { @@ -80,24 +81,72 @@ func TestStdSignBytes(t *testing.T) { fee StdFee msgs []sdk.Msg memo string + tip *tx.Tip } defaultFee := NewTestStdFee() + defaultTip := &tx.Tip{Tipper: addr.String(), Amount: sdk.NewCoins(sdk.NewInt64Coin("tiptoken", 150))} tests := []struct { + name string args args want string }{ { - args{"1234", 3, 6, 10, defaultFee, []sdk.Msg{testdata.NewTestMsg(addr)}, "memo"}, - fmt.Sprintf("{\"account_number\":\"3\",\"chain_id\":\"1234\",\"fee\":{\"amount\":[{\"amount\":\"150\",\"denom\":\"atom\"}],\"gas\":\"100000\"},\"memo\":\"memo\",\"msgs\":[[\"%s\"]],\"sequence\":\"6\",\"timeout_height\":\"10\"}", addr), + "with timeout height", + args{"1234", 3, 6, 10, defaultFee, []sdk.Msg{testdata.NewTestMsg(addr)}, "memo", nil}, + fmt.Sprintf(`{"account_number":"3","chain_id":"1234","fee":{"amount":[{"amount":"150","denom":"atom"}],"gas":"100000"},"memo":"memo","msgs":[["%s"]],"sequence":"6","timeout_height":"10"}`, addr), }, { - args{"1234", 3, 6, 0, defaultFee, []sdk.Msg{testdata.NewTestMsg(addr)}, "memo"}, - fmt.Sprintf("{\"account_number\":\"3\",\"chain_id\":\"1234\",\"fee\":{\"amount\":[{\"amount\":\"150\",\"denom\":\"atom\"}],\"gas\":\"100000\"},\"memo\":\"memo\",\"msgs\":[[\"%s\"]],\"sequence\":\"6\"}", addr), + "no timeout height (omitempty)", + args{"1234", 3, 6, 0, defaultFee, []sdk.Msg{testdata.NewTestMsg(addr)}, "memo", nil}, + fmt.Sprintf(`{"account_number":"3","chain_id":"1234","fee":{"amount":[{"amount":"150","denom":"atom"}],"gas":"100000"},"memo":"memo","msgs":[["%s"]],"sequence":"6"}`, addr), + }, + { + "empty fee", + args{"1234", 3, 6, 0, StdFee{}, []sdk.Msg{testdata.NewTestMsg(addr)}, "memo", nil}, + fmt.Sprintf(`{"account_number":"3","chain_id":"1234","fee":{"amount":[],"gas":"0"},"memo":"memo","msgs":[["%s"]],"sequence":"6"}`, addr), + }, + { + "no fee payer and fee granter (both omitempty)", + args{"1234", 3, 6, 0, StdFee{Amount: defaultFee.Amount, Gas: defaultFee.Gas}, []sdk.Msg{testdata.NewTestMsg(addr)}, "memo", nil}, + fmt.Sprintf(`{"account_number":"3","chain_id":"1234","fee":{"amount":[{"amount":"150","denom":"atom"}],"gas":"100000"},"memo":"memo","msgs":[["%s"]],"sequence":"6"}`, addr), + }, + { + "with fee granter, no fee payer (omitempty)", + args{"1234", 3, 6, 0, StdFee{Amount: defaultFee.Amount, Gas: defaultFee.Gas, Granter: addr.String()}, []sdk.Msg{testdata.NewTestMsg(addr)}, "memo", nil}, + fmt.Sprintf(`{"account_number":"3","chain_id":"1234","fee":{"amount":[{"amount":"150","denom":"atom"}],"gas":"100000","granter":"%s"},"memo":"memo","msgs":[["%s"]],"sequence":"6"}`, addr, addr), + }, + { + "with fee payer, no fee granter (omitempty)", + args{"1234", 3, 6, 0, StdFee{Amount: defaultFee.Amount, Gas: defaultFee.Gas, Payer: addr.String()}, []sdk.Msg{testdata.NewTestMsg(addr)}, "memo", nil}, + fmt.Sprintf(`{"account_number":"3","chain_id":"1234","fee":{"amount":[{"amount":"150","denom":"atom"}],"gas":"100000","payer":"%s"},"memo":"memo","msgs":[["%s"]],"sequence":"6"}`, addr, addr), + }, + { + "with fee payer and fee granter", + args{"1234", 3, 6, 0, StdFee{Amount: defaultFee.Amount, Gas: defaultFee.Gas, Payer: addr.String(), Granter: addr.String()}, []sdk.Msg{testdata.NewTestMsg(addr)}, "memo", nil}, + fmt.Sprintf(`{"account_number":"3","chain_id":"1234","fee":{"amount":[{"amount":"150","denom":"atom"}],"gas":"100000","granter":"%s","payer":"%s"},"memo":"memo","msgs":[["%s"]],"sequence":"6"}`, addr, addr, addr), + }, + { + "no fee, with tip", + args{"1234", 3, 6, 0, StdFee{}, []sdk.Msg{testdata.NewTestMsg(addr)}, "memo", defaultTip}, + fmt.Sprintf(`{"account_number":"3","chain_id":"1234","fee":{"amount":[],"gas":"0"},"memo":"memo","msgs":[["%s"]],"sequence":"6","tip":{"amount":[{"amount":"150","denom":"tiptoken"}],"tipper":"%s"}}`, addr, addr), + }, + { + "with fee and with tip", + args{"1234", 3, 6, 0, StdFee{Amount: defaultFee.Amount, Gas: defaultFee.Gas, Payer: addr.String()}, []sdk.Msg{testdata.NewTestMsg(addr)}, "memo", defaultTip}, + fmt.Sprintf(`{"account_number":"3","chain_id":"1234","fee":{"amount":[{"amount":"150","denom":"atom"}],"gas":"100000","payer":"%s"},"memo":"memo","msgs":[["%s"]],"sequence":"6","tip":{"amount":[{"amount":"150","denom":"tiptoken"}],"tipper":"%s"}}`, addr, addr, addr), + }, + { + "with empty tip (but not nil), tipper cannot be empty", + args{"1234", 3, 6, 0, defaultFee, []sdk.Msg{testdata.NewTestMsg(addr)}, "memo", &tx.Tip{Tipper: addr.String()}}, + fmt.Sprintf(`{"account_number":"3","chain_id":"1234","fee":{"amount":[{"amount":"150","denom":"atom"}],"gas":"100000"},"memo":"memo","msgs":[["%s"]],"sequence":"6","tip":{"amount":[],"tipper":"%s"}}`, addr, addr), }, } for i, tc := range tests { - got := string(StdSignBytes(tc.args.chainID, tc.args.accnum, tc.args.sequence, tc.args.timeoutHeight, tc.args.fee, tc.args.msgs, tc.args.memo)) - require.Equal(t, tc.want, got, "Got unexpected result on test case i: %d", i) + tc := tc + t.Run(tc.name, func(t *testing.T) { + got := string(StdSignBytes(tc.args.chainID, tc.args.accnum, tc.args.sequence, tc.args.timeoutHeight, tc.args.fee, tc.args.msgs, tc.args.memo, tc.args.tip)) + require.Equal(t, tc.want, got, "Got unexpected result on test case i: %d", i) + }) } } diff --git a/x/auth/signing/verify_test.go b/x/auth/signing/verify_test.go index 99eeaf8a04e3..f67590491d61 100644 --- a/x/auth/signing/verify_test.go +++ b/x/auth/signing/verify_test.go @@ -55,7 +55,7 @@ func TestVerifySignature(t *testing.T) { Sequence: acc.GetSequence(), SignerIndex: 0, } - signBytes := legacytx.StdSignBytes(signerData.ChainID, signerData.AccountNumber, signerData.Sequence, 10, fee, msgs, memo) + signBytes := legacytx.StdSignBytes(signerData.ChainID, signerData.AccountNumber, signerData.Sequence, 10, fee, msgs, memo, nil) signature, err := priv.Sign(signBytes) require.NoError(t, err) @@ -73,7 +73,7 @@ func TestVerifySignature(t *testing.T) { multisigKey := kmultisig.NewLegacyAminoPubKey(2, pkSet) multisignature := multisig.NewMultisig(2) msgs = []sdk.Msg{testdata.NewTestMsg(addr, addr1)} - multiSignBytes := legacytx.StdSignBytes(signerData.ChainID, signerData.AccountNumber, signerData.Sequence, 10, fee, msgs, memo) + multiSignBytes := legacytx.StdSignBytes(signerData.ChainID, signerData.AccountNumber, signerData.Sequence, 10, fee, msgs, memo, nil) sig1, err := priv.Sign(multiSignBytes) require.NoError(t, err) diff --git a/x/auth/tx/builder.go b/x/auth/tx/builder.go index 52e35705f062..d3b83d5018bb 100644 --- a/x/auth/tx/builder.go +++ b/x/auth/tx/builder.go @@ -35,6 +35,7 @@ var ( _ client.TxBuilder = &wrapper{} _ middleware.HasExtensionOptionsTx = &wrapper{} _ ExtensionOptionsTxBuilder = &wrapper{} + _ tx.TipTx = &wrapper{} ) // ExtensionOptionsTxBuilder defines a TxBuilder that can also set extensions. @@ -156,6 +157,10 @@ func (w *wrapper) FeeGranter() sdk.AccAddress { return nil } +func (w *wrapper) GetTip() *tx.Tip { + return w.tx.AuthInfo.Tip +} + func (w *wrapper) GetMemo() string { return w.tx.Body.Memo } diff --git a/x/auth/tx/direct_aux.go b/x/auth/tx/direct_aux.go index c7324c18bb90..71a3fd5c0426 100644 --- a/x/auth/tx/direct_aux.go +++ b/x/auth/tx/direct_aux.go @@ -50,7 +50,7 @@ func (signModeDirectAuxHandler) GetSignBytes( AccountNumber: data.AccountNumber, Sequence: data.Sequence, Tip: protoTx.tx.AuthInfo.Tip, - PublicKey: protoTx.tx.AuthInfo.SignerInfos[data.SignerIndex].PublicKey, + PublicKey: signerInfo.PublicKey, } return signDocDirectAux.Marshal() diff --git a/x/auth/tx/legacy_amino_json.go b/x/auth/tx/legacy_amino_json.go index fea9d78a9bd3..01274f9f6ed9 100644 --- a/x/auth/tx/legacy_amino_json.go +++ b/x/auth/tx/legacy_amino_json.go @@ -46,9 +46,33 @@ func (s signModeLegacyAminoJSONHandler) GetSignBytes(mode signingtypes.SignMode, return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "%s does not support protobuf extension options", signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON) } + addr := data.Address + if addr == "" { + return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "got empty address in %s handler", signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON) + } + + tip := protoTx.GetTip() + isTipper := tip != nil && tip.Tipper == addr + + // We set a convention that if the tipper signs with LEGACY_AMINO_JSON, then + // they sign over empty fees and 0 gas. + if isTipper { + return legacytx.StdSignBytes( + data.ChainID, data.AccountNumber, data.Sequence, protoTx.GetTimeoutHeight(), + // The tipper signs over 0 fee and 0 gas, no feepayer, no feegranter by convention. + legacytx.StdFee{}, + tx.GetMsgs(), protoTx.GetMemo(), tip, + ), nil + } + return legacytx.StdSignBytes( data.ChainID, data.AccountNumber, data.Sequence, protoTx.GetTimeoutHeight(), - legacytx.StdFee{Amount: protoTx.GetFee(), Gas: protoTx.GetGas()}, - tx.GetMsgs(), protoTx.GetMemo(), + legacytx.StdFee{ + Amount: protoTx.GetFee(), + Gas: protoTx.GetGas(), + Payer: protoTx.tx.AuthInfo.Fee.Payer, + Granter: protoTx.tx.AuthInfo.Fee.Granter, + }, + tx.GetMsgs(), protoTx.GetMemo(), tip, ), nil } diff --git a/x/auth/tx/legacy_amino_json_test.go b/x/auth/tx/legacy_amino_json_test.go index 893d06f5ff2d..9734e7704eea 100644 --- a/x/auth/tx/legacy_amino_json_test.go +++ b/x/auth/tx/legacy_amino_json_test.go @@ -8,6 +8,7 @@ import ( cdctypes "github.com/cosmos/cosmos-sdk/codec/types" "github.com/cosmos/cosmos-sdk/testutil/testdata" sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/types/tx" signingtypes "github.com/cosmos/cosmos-sdk/types/tx/signing" "github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx" "github.com/cosmos/cosmos-sdk/x/auth/signing" @@ -33,17 +34,79 @@ func buildTx(t *testing.T, bldr *wrapper) { } func TestLegacyAminoJSONHandler_GetSignBytes(t *testing.T) { - bldr := newBuilder() - buildTx(t, bldr) - tx := bldr.GetTx() - var ( - chainId = "test-chain" - accNum uint64 = 7 - seqNum uint64 = 7 + chainId = "test-chain" + accNum uint64 = 7 + seqNum uint64 = 7 + tip *tx.Tip = &tx.Tip{Tipper: addr1.String(), Amount: coins} ) + testcases := []struct { + name string + signer string + malleate func(*wrapper) + expectedSignBz []byte + }{ + { + "signer which is also fee payer (no tips)", addr1.String(), + func(w *wrapper) {}, + legacytx.StdSignBytes(chainId, accNum, seqNum, timeout, legacytx.StdFee{Amount: coins, Gas: gas}, []sdk.Msg{msg}, memo, nil), + }, + { + "signer which is also fee payer (with tips)", addr2.String(), + func(w *wrapper) { w.SetTip(tip) }, + legacytx.StdSignBytes(chainId, accNum, seqNum, timeout, legacytx.StdFee{Amount: coins, Gas: gas}, []sdk.Msg{msg}, memo, tip), + }, + { + "explicit fee payer", addr1.String(), + func(w *wrapper) { w.SetFeePayer(addr2) }, + legacytx.StdSignBytes(chainId, accNum, seqNum, timeout, legacytx.StdFee{Amount: coins, Gas: gas, Payer: addr2.String()}, []sdk.Msg{msg}, memo, nil), + }, + { + "explicit fee granter", addr1.String(), + func(w *wrapper) { w.SetFeeGranter(addr2) }, + legacytx.StdSignBytes(chainId, accNum, seqNum, timeout, legacytx.StdFee{Amount: coins, Gas: gas, Granter: addr2.String()}, []sdk.Msg{msg}, memo, nil), + }, + { + "explicit fee payer and fee granter", addr1.String(), + func(w *wrapper) { + w.SetFeePayer(addr2) + w.SetFeeGranter(addr2) + }, + legacytx.StdSignBytes(chainId, accNum, seqNum, timeout, legacytx.StdFee{Amount: coins, Gas: gas, Payer: addr2.String(), Granter: addr2.String()}, []sdk.Msg{msg}, memo, nil), + }, + { + "signer which is also tipper", addr1.String(), + func(w *wrapper) { w.SetTip(tip) }, + legacytx.StdSignBytes(chainId, accNum, seqNum, timeout, legacytx.StdFee{}, []sdk.Msg{msg}, memo, tip), + }, + } + handler := signModeLegacyAminoJSONHandler{} + for _, tc := range testcases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + bldr := newBuilder() + buildTx(t, bldr) + tx := bldr.GetTx() + tc.malleate(bldr) + + signingData := signing.SignerData{ + Address: tc.signer, + ChainID: chainId, + AccountNumber: accNum, + Sequence: seqNum, + } + signBz, err := handler.GetSignBytes(signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, signingData, tx) + require.NoError(t, err) + + require.Equal(t, tc.expectedSignBz, signBz) + }) + } + + bldr := newBuilder() + buildTx(t, bldr) + tx := bldr.GetTx() signingData := signing.SignerData{ Address: addr1.String(), ChainID: chainId, @@ -51,18 +114,9 @@ func TestLegacyAminoJSONHandler_GetSignBytes(t *testing.T) { Sequence: seqNum, SignerIndex: 0, } - signBz, err := handler.GetSignBytes(signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, signingData, tx) - require.NoError(t, err) - - expectedSignBz := legacytx.StdSignBytes(chainId, accNum, seqNum, timeout, legacytx.StdFee{ - Amount: coins, - Gas: gas, - }, []sdk.Msg{msg}, memo) - - require.Equal(t, expectedSignBz, signBz) // expect error with wrong sign mode - _, err = handler.GetSignBytes(signingtypes.SignMode_SIGN_MODE_DIRECT, signingData, tx) + _, err := handler.GetSignBytes(signingtypes.SignMode_SIGN_MODE_DIRECT, signingData, tx) require.Error(t, err) // expect error with extension options @@ -72,7 +126,7 @@ func TestLegacyAminoJSONHandler_GetSignBytes(t *testing.T) { require.NoError(t, err) bldr.tx.Body.ExtensionOptions = []*cdctypes.Any{any} tx = bldr.GetTx() - signBz, err = handler.GetSignBytes(signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, signingData, tx) + _, err = handler.GetSignBytes(signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, signingData, tx) require.Error(t, err) // expect error with non-critical extension options @@ -80,7 +134,7 @@ func TestLegacyAminoJSONHandler_GetSignBytes(t *testing.T) { buildTx(t, bldr) bldr.tx.Body.NonCriticalExtensionOptions = []*cdctypes.Any{any} tx = bldr.GetTx() - signBz, err = handler.GetSignBytes(signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, signingData, tx) + _, err = handler.GetSignBytes(signingtypes.SignMode_SIGN_MODE_LEGACY_AMINO_JSON, signingData, tx) require.Error(t, err) }