Merge branch 'main' into hieu/comet_output

Dockerfile

@@ -39,8 +39,11 @@ COPY x/auth/go.mod x/auth/go.sum ./x/auth/
 COPY x/authz/go.mod x/authz/go.sum ./x/authz/
 COPY x/bank/go.mod x/bank/go.sum ./x/bank/
 COPY x/mint/go.mod x/mint/go.sum ./x/mint/
+COPY x/tx/go.mod x/tx/go.sum ./x/tx/
 COPY x/consensus/go.mod x/consensus/go.sum ./x/consensus/
 COPY depinject/go.mod depinject/go.sum ./depinject/
+COPY core/testing/go.mod core/testing/go.sum ./core/testing/
+COPY log/go.mod log/go.sum ./log/
 RUN go mod download
 
 # Add source files

client/keys/output_test.go

@@ -45,6 +45,30 @@ func TestBech32KeysOutput(t *testing.T) {
 	require.Equal(t, "{Name:multisig Type:multi Address:cosmos1nf8lf6n4wa43rzmdzwe6hkrnw5guekhqt595cw PubKey:{\"@type\":\"/cosmos.crypto.multisig.LegacyAminoPubKey\",\"threshold\":1,\"public_keys\":[{\"@type\":\"/cosmos.crypto.secp256k1.PubKey\",\"key\":\"AurroA7jvfPd1AadmmOvWM2rJSwipXfRf8yD6pLbA2DJ\"}]} Mnemonic:}", fmt.Sprintf("%+v", out))
 }
 
+// TestBech32KeysOutputNestedMsig tests that the output of a nested multisig key is correct
+func TestBech32KeysOutputNestedMsig(t *testing.T) {
+	sk := secp256k1.PrivKey{Key: []byte{154, 49, 3, 117, 55, 232, 249, 20, 205, 216, 102, 7, 136, 72, 177, 2, 131, 202, 234, 81, 31, 208, 46, 244, 179, 192, 167, 163, 142, 117, 246, 13}}
+	tmpKey := sk.PubKey()
+	nestedMultiSig := kmultisig.NewLegacyAminoPubKey(1, []types.PubKey{tmpKey})
+	multisigPk := kmultisig.NewLegacyAminoPubKey(2, []types.PubKey{tmpKey, nestedMultiSig})
+	k, err := keyring.NewMultiRecord("multisig", multisigPk)
+	require.NotNil(t, k)
+	require.NoError(t, err)
+
+	pubKey, err := k.GetPubKey()
+	require.NoError(t, err)
+
+	accAddr := sdk.AccAddress(pubKey.Address())
+	expectedOutput, err := NewKeyOutput(k.Name, k.GetType(), accAddr, multisigPk, addresscodec.NewBech32Codec("cosmos"))
+	require.NoError(t, err)
+
+	out, err := MkAccKeyOutput(k, addresscodec.NewBech32Codec("cosmos"))
+	require.NoError(t, err)
+
+	require.Equal(t, expectedOutput, out)
+	require.Equal(t, "{Name:multisig Type:multi Address:cosmos1nffp6v2j7wva4y4975exlrv8x5vh39axxt3swz PubKey:{\"@type\":\"/cosmos.crypto.multisig.LegacyAminoPubKey\",\"threshold\":2,\"public_keys\":[{\"@type\":\"/cosmos.crypto.secp256k1.PubKey\",\"key\":\"AurroA7jvfPd1AadmmOvWM2rJSwipXfRf8yD6pLbA2DJ\"},{\"@type\":\"/cosmos.crypto.multisig.LegacyAminoPubKey\",\"threshold\":1,\"public_keys\":[{\"@type\":\"/cosmos.crypto.secp256k1.PubKey\",\"key\":\"AurroA7jvfPd1AadmmOvWM2rJSwipXfRf8yD6pLbA2DJ\"}]}]} Mnemonic:}", fmt.Sprintf("%+v", out))
+}
+
 func TestProtoMarshalJSON(t *testing.T) {
 	require := require.New(t)
 	pubkeys := generatePubKeys(3)

client/rpc/tx.go

@@ -204,7 +204,7 @@ func parseHashFromInput(in []byte) ([]byte, error) {
 	// That outputs a sdk.TxResponse as either the json or yaml. As json, we can't unmarshal it back into that struct,
 	// though, because the height field ends up quoted which confuses json.Unmarshal (because it's for an int64 field).
 
-	// Try to find the txhash from json ouptut.
+	// Try to find the txhash from json output.
 	resultTx := make(map[string]json.RawMessage)
 	if err := json.Unmarshal(in, &resultTx); err == nil && len(resultTx["txhash"]) > 0 {
 		// input was JSON, return the hash

contrib/images/simd-env/Dockerfile

@@ -25,9 +25,9 @@ COPY x/consensus/go.mod x/consensus/go.sum /work/x/consensus/
 COPY x/accounts/go.mod x/accounts/go.sum /work/x/accounts/
 COPY runtime/v2/go.mod runtime/v2/go.sum /work/runtime/v2/
 COPY server/v2/appmanager/go.mod server/v2/appmanager/go.sum /work/server/v2/appmanager/
-COPY server/v2/core/go.mod server/v2/core/go.sum /work/server/v2/core/
 COPY server/v2/stf/go.mod server/v2/stf/go.sum /work/server/v2/stf/
 COPY server/v2/cometbft/go.mod server/v2/cometbft/go.sum /work/server/v2/cometbft/
+COPY core/testing/go.mod core/testing/go.sum /work/core/testing/
 
 RUN go mod download
 

crypto/keys/multisig/multisig.go

@@ -169,6 +169,11 @@ func packPubKeys(pubKeys []cryptotypes.PubKey) ([]*types.Any, error) {
 			return nil, err
 		}
 		anyPubKeys[i] = any
+
+		// sets the compat.aminoBz value
+		if err := anyPubKeys[i].UnmarshalAmino(pubKeys[i].Bytes()); err != nil {
+			return nil, err
+		}
 	}
 	return anyPubKeys, nil
 }

docs/architecture/README.md

@@ -90,6 +90,7 @@ When writing ADRs, follow the same best practices for writing RFCs. When writing
 * [ADR 063: Core Module API](./adr-063-core-module-api.md)
 * [ADR 067: Simulator v2](./adr-067-simulator-v2.md)
 * [ADR 069: `x/gov` modularity, multiple choice and optimisic proposals](./adr-069-gov-improvements.md)
+* [ADR 074: Messages with implicit signers](./adr-074-implicit-msg-signers.md)
 
 ### Draft
 

docs/architecture/adr-074-implicit-msg-signers.md

@@ -0,0 +1,131 @@
+# ADR 074: Messages with Implicit Signers
+
+## Changelog
+
+* 2024-06-10: Initial draft
+
+## Status
+
+PROPOSED Not Implemented
+
+## Abstract
+
+This ADR introduces a new `MsgV2` standard where the signer of the message is implied by the
+credentials of the party sending it, and unlike the current design not part of the message body.
+This can be used for both simple inter-module message passing and simpler messages in transactions.
+
+## Context
+
+Historically operations in the SDK have been modelled with the `sdk.Msg` interface and
+the account signing the message has to be explicitly extracted from the body of `Msg`s.
+Originally this was via a `GetSigners` method on the `sdk.Msg` interface which returned
+instances of `sdk.AccAddress` which itself relied on a global variable for decoding
+the addresses from bech32 strings. This was a messy situation. In addition, the implementation
+for `GetSigners` was different for each `Msg` type and clients would need to do a custom
+implementation for each `Msg` type. These were improved somewhat with the introduction of
+the `cosmos.msg.v1.signer` protobuf option which allowed for a more standardised way of
+defining who the signer of a message was and its implementation in the `x/tx` module which
+extracts signers dynamically and allowed removing the dependency on the global bech32
+configuration.
+
+Still this design introduces a fair amount of complexity. For instance, inter-module message
+passing ([ADR 033](./adr-033-protobuf-inter-module-comm.md)) has been in discussion for years
+without much progress and one of the main blockers is figuring out how to properly authenticate
+messages in a performant and consistent way. With embedded message signers there will always need
+to be a step of extracting the signer and then checking with the module sending is actually
+authorized to perform the operation. With dynamic signer extraction, although the system is
+more consistent, more performance overhead is introduced. In any case why should an inter-module
+message passing system need to do so much conversion, parsing, etc. just to check if a message
+is authenticated? In addition, we have the complexity where modules can actually have many valid
+addresses. How are we to accommodate this? Should there be a lookup into `x/auth` to check if an
+address belongs to a module or not? All of these thorny questions are delaying the delivery of
+inter-module message passing because we do not want an implementation that is overly complex.
+There are many use cases for inter-module message passing which are still relevant, the most
+immediate of which is a more robust denom management system in `x/bank` `v2` which is being explored
+in [ADR 071](https://github.com/cosmos/cosmos-sdk/pull/20316).
+
+## Alternatives
+
+Alternatives that have been considered are extending the current `x/tx` signer extraction system
+to inter-module message passing as defined in [ADR 033](./adr-033-protobuf-inter-module-comm.md).
+
+## Decision
+
+We have decided to introduce a new `MsgV2` standard whereby the signer of the message is implied
+by the credentials of the party sending it. These messages will be distinct from the existing messages
+and define new semantics with the understanding that signers are implicit.
+
+In the case of messages passed internally by a module or `x/account` instance, the signer of a message
+will simply be the main root address of the module or account sending the message. An interface for
+safely passing such messages to the message router will need to be defined.
+
+In the case of messages passed externally in transactions, `MsgV2` instances will need to be wrapped
+in a `MsgV2` envelope:
+```protobuf
+message MsgV2 {
+  string signer = 1;
+  google.protobuf.Any msg = 2;  
+}
+```
+
+Because the `cosmos.msg.v1.signer` annotation is required currently, `MsgV2` types should set the message option
+`cosmos.msg.v2.is_msg` to `true` instead.
+
+Here is an example comparing a v1 an v2 message:
+```protobuf
+// v1
+message MsgSendV1 {
+  option (cosmos.msg.v1.signer) = "from_address";
+  string from_address = 1 ;
+  string to_address = 2;
+  repeated Coin amount = 3;
+}
+
+// v2
+message MsgSendV2 {
+  option (cosmos.msg.v2.is_msg) = true;
+  // from address is implied by the signer
+  string to_address = 1;
+  repeated Coin amount = 2;
+}
+```
+
+Modules defining handlers for `MsgV2` instances will need to extract the sender from the `context.Context` that is
+passed in. An interface in `core` which will be present on the `appmodule.Environment` will be defined for this purpose:
+```go
+type GetSenderService interface {
+  GetSender(ctx context.Context) []byte
+}
+```
+
+Sender addresses that are returned by the service will be simple `[]byte` slices and any bech32 conversion will be
+done by the framework.
+
+## Consequences
+
+### Backwards Compatibility
+
+This design does not depreciate the existing method of embedded signers in `Msg`s and is totally compatible with it.
+
+### Positive
+
+* Allows for a simple inter-module communication design which can be used soon for the `bank` `v2` redesign.
+* Allows for simpler client implementations for messages in the future.
+
+### Negative
+
+* There will be two message designs and developers will need to pick between them.
+
+### Neutral
+
+## Further Discussions
+
+Two possible directions that have been proposed are:
+1. allowing for the omission of the `cosmos.msg.v2.is_msg` option and assuming any `Msg`s registered that do not include `cosmos.msg.v1.signer` are `MsgV2` instances. The pitfall is that this could be incorrect if `Msg` v1 behavior is actually decided but the user forgot the `cosmos.msg.v1.signer` option.
+2. allow `Msg` v1 instances to be wrapped in a `MsgV2` envelope as well to simplify things client-side. In this scenario we would need to either a) check that the signer in the envelope and the signer in the message are the same or b) allow the signer in the message to be empty and then set it inside the state machine before it reaches the module. While this may be easier for some clients, it may introduce unexpected behavior with Ledger signing via Amino JSON or SIGN_MODE_TEXTUAL. 
+
+Both of these are seem as quality of life improvements for some users, but not strictly necessary and could have some pitfalls so further discussion is needed.
+
+## References
+
+* [ADR 033](./adr-033-protobuf-inter-module-comm.md)

x/auth/README.md

@@ -447,6 +447,22 @@ simd tx multi-sign transaction.json k1k2k3 k1sig.json k2sig.json k3sig.json
 
 Where `k1k2k3` is the multisig account address, `k1sig.json` is the signature of the first signer, `k2sig.json` is the signature of the second signer, and `k3sig.json` is the signature of the third signer.
 
+##### Nested multisig transactions
+
+To allow transactions to be signed by nested multisigs, meaning that a participant of a multisig account can be another multisig account, the `--skip-signature-verification` flag must be used.
+
+```bash
+# First aggregate signatures of the multisig participant
+simd tx multi-sign transaction.json ms1 ms1p1sig.json ms1p2sig.json --signature-only --skip-signature-verification > ms1sig.json
+
+# Then use the aggregated signatures and the other signatures to sign the final transaction
+simd tx multi-sign transaction.json k1ms1 k1sig.json ms1sig.json --skip-signature-verification
+```
+
+Where `ms1` is the nested multisig account address, `ms1p1sig.json` is the signature of the first participant of the nested multisig account, `ms1p2sig.json` is the signature of the second participant of the nested multisig account, and `ms1sig.json` is the aggregated signature of the nested multisig account.
+
+`k1ms1` is a multisig account comprised of an individual signer and another nested multisig account (`ms1`). `k1sig.json` is the signature of the first signer of the individual member.
+
 More information about the `multi-sign` command can be found running `simd tx multi-sign --help`.
 
 #### `multisign-batch`

x/auth/ante/expected_keepers.go

@@ -30,4 +30,5 @@ type FeegrantKeeper interface {
 
 type ConsensusKeeper interface {
 	Params(context.Context, *consensustypes.QueryParamsRequest) (*consensustypes.QueryParamsResponse, error)
+	GetCometInfo(context.Context, *consensustypes.QueryGetCometInfoRequest) (*consensustypes.QueryGetCometInfoResponse, error)
 }

x/auth/client/cli/tx_multisign.go

@@ -47,6 +47,10 @@ If the --offline flag is on, the client will not reach out to an external node.
 Account number or sequence number lookups are not performed so you must
 set these parameters manually.
 
+If the --skip-signature-verification flag is on, the command will not verify the
+signatures in the provided signature files. This is useful when the multisig
+account is a signer in a nested multisig scenario.
+
 The current multisig implementation defaults to amino-json sign mode.
 The SIGN_MODE_DIRECT sign mode is not supported.'
 `,
@@ -57,6 +61,7 @@ The SIGN_MODE_DIRECT sign mode is not supported.'
 		Args: cobra.MinimumNArgs(3),
 	}
 
+	cmd.Flags().Bool(flagSkipSignatureVerification, false, "Skip signature verification")
 	cmd.Flags().Bool(flagSigOnly, false, "Print only the generated signature, then exit")
 	cmd.Flags().String(flags.FlagOutputDocument, "", "The document is written to the given file instead of STDOUT")
 	flags.AddTxFlagsToCmd(cmd)
@@ -109,6 +114,10 @@ func makeMultiSignCmd() func(cmd *cobra.Command, args []string) (err error) {
 			return err
 		}
 
+		// avoid signature verification if the sender of the tx is different than
+		// the multisig key (useful for nested multisigs).
+		skipSigVerify, _ := cmd.Flags().GetBool(flagSkipSignatureVerification)
+
 		multisigPub := pubKey.(*kmultisig.LegacyAminoPubKey)
 		multisigSig := multisig.NewMultisig(len(multisigPub.PubKeys))
 		if !clientCtx.Offline {
@@ -153,11 +162,13 @@ func makeMultiSignCmd() func(cmd *cobra.Command, args []string) (err error) {
 				}
 				txData := adaptableTx.GetSigningTxData()
 
-				err = signing.VerifySignature(cmd.Context(), sig.PubKey, txSignerData, sig.Data,
-					txCfg.SignModeHandler(), txData)
-				if err != nil {
-					addr, _ := sdk.AccAddressFromHexUnsafe(sig.PubKey.Address().String())
-					return fmt.Errorf("couldn't verify signature for address %s", addr)
+				if !skipSigVerify {
+					err = signing.VerifySignature(cmd.Context(), sig.PubKey, txSignerData, sig.Data,
+						txCfg.SignModeHandler(), txData)
+					if err != nil {
+						addr, _ := sdk.AccAddressFromHexUnsafe(sig.PubKey.Address().String())
+						return fmt.Errorf("couldn't verify signature for address %s %w", addr, err)
+					}
 				}
 
 				if err := multisig.AddSignatureV2(multisigSig, sig, multisigPub.GetPubKeys()); err != nil {