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.

Sign up for GitHub

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

Jump to bottom

Update signbatch multisig to work online #7801

Merged
merged 14 commits into from
Dec 3, 2020
Merged

Update signbatch multisig to work online #7801

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c6e7665
add test for signbatch multisig
sahith-narahari Nov 3, 2020
f2b9abd
Merge branch 'master' into sahith/add-batch-test
sahith-narahari Nov 3, 2020
df711ac
update test
sahith-narahari Nov 3, 2020
c12c85a
Merge remote-tracking branch 'origin/sahith/add-batch-test' into sahi…
sahith-narahari Nov 3, 2020
9821c77
Merge branch 'master' into sahith/add-batch-test
Nov 5, 2020
54ecb38
Merge branch 'master' of github.com:cosmos/cosmos-sdk into sahith/add…
sahith-narahari Nov 23, 2020
a3f8641
fix sign batch multisig
sahith-narahari Nov 23, 2020
9879824
Merge branch 'master' of github.com:cosmos/cosmos-sdk into sahith/add…
sahith-narahari Nov 24, 2020
21226ec
update offline usage
sahith-narahari Nov 24, 2020
f10cf86
Merge branch 'master' into sahith/add-batch-test
sahith-narahari Nov 27, 2020
e8d7643
Merge branch 'master' into sahith/add-batch-test
sahith-narahari Nov 27, 2020
30f1193
address comments
sahith-narahari Nov 30, 2020
0ae873b
Merge branch 'master' into sahith/add-batch-test
Dec 2, 2020
ea2ab08
Merge branch 'master' into sahith/add-batch-test
sahith-narahari Dec 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 72 additions & 0 deletions x/auth/client/cli/cli_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,6 +735,78 @@ func (s *IntegrationTestSuite) TestCLIMultisign() {
s.Require().NoError(s.network.WaitForNextBlock())
}

func (s *IntegrationTestSuite) TestSignBatchMultisig() {
val := s.network.Validators[0]

// Fetch 2 accounts and a multisig.
account1, err := val.ClientCtx.Keyring.Key("newAccount1")
s.Require().NoError(err)
account2, err := val.ClientCtx.Keyring.Key("newAccount2")
s.Require().NoError(err)
multisigInfo, err := val.ClientCtx.Keyring.Key("multi")

// Send coins from validator to multisig.
sendTokens := sdk.NewInt64Coin(s.cfg.BondDenom, 10)
_, err = bankcli.MsgSendExec(
val.ClientCtx,
val.Address,
multisigInfo.GetAddress(),
sdk.NewCoins(sendTokens),
fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation),
fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock),
fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()),
fmt.Sprintf("--gas=%d", flags.DefaultGasLimit),
)
s.Require().NoError(err)
s.Require().NoError(s.network.WaitForNextBlock())

resp, err := bankcli.QueryBalancesExec(val.ClientCtx, multisigInfo.GetAddress())
s.Require().NoError(err)

var balRes banktypes.QueryAllBalancesResponse
err = val.ClientCtx.JSONMarshaler.UnmarshalJSON(resp.Bytes(), &balRes)
s.Require().NoError(err)
sahith-narahari marked this conversation as resolved.
Show resolved Hide resolved

generatedStd, err := bankcli.MsgSendExec(
val.ClientCtx,
multisigInfo.GetAddress(),
val.Address,
sdk.NewCoins(
sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(1)),
),
fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation),
fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock),
fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()),
fmt.Sprintf("--%s=true", flags.FlagGenerateOnly),
)
s.Require().NoError(err)

// Write the output to disk
filename, cleanup1 := testutil.WriteToNewTempFile(s.T(), strings.Repeat(generatedStd.String(), 1))
defer cleanup1()

val.ClientCtx.HomeDir = strings.Replace(val.ClientCtx.HomeDir, "simd", "simcli", 1)

// sign-batch file
res, err := authtest.TxSignBatchExec(val.ClientCtx, account1.GetAddress(), filename.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, val.ClientCtx.ChainID), "--multisig", multisigInfo.GetAddress().String())
s.Require().NoError(err)
s.Require().Equal(1, len(strings.Split(strings.Trim(res.String(), "\n"), "\n")))
// write sigs to file
file1, cleanup2 := testutil.WriteToNewTempFile(s.T(), res.String())
defer cleanup2()

// sign-batch file with account2
res, err = authtest.TxSignBatchExec(val.ClientCtx, account2.GetAddress(), filename.Name(), fmt.Sprintf("--%s=%s", flags.FlagChainID, val.ClientCtx.ChainID), "--multisig", multisigInfo.GetAddress().String())
s.Require().NoError(err)
s.Require().Equal(1, len(strings.Split(strings.Trim(res.String(), "\n"), "\n")))

// write sigs to file2
file2, cleanup3 := testutil.WriteToNewTempFile(s.T(), res.String())
defer cleanup3()
res, err = authtest.TxMultiSignExec(val.ClientCtx, multisigInfo.GetName(), filename.Name(), file1.Name(), file2.Name())
s.Require().NoError(err)
}

func (s *IntegrationTestSuite) TestGetAccountCmd() {
val := s.network.Validators[0]
_, _, addr1 := testdata.KeyTestPubAddr()
Expand Down
5 changes: 4 additions & 1 deletion x/auth/client/cli/tx_sign.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,10 @@ func makeSignBatchCmd() func(cmd *cobra.Command, args []string) error {
return err
}
} else {
err = authclient.SignTxWithSignerAddress(txFactory, clientCtx, multisigAddr, clientCtx.GetFromName(), txBuilder, true)
if txFactory.SignMode() == signing.SignMode_SIGN_MODE_UNSPECIFIED {
txFactory = txFactory.WithSignMode(signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit confusing for me. Why don't we use DIRECT as default sign mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't we need multisig to use legacy mode for signing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that's incorrect, the multisig.LegacyAminoPubKey can generate nested signatures that are SIGN_MODE_DIRECT.

Here's how I tested it: in the following test file

  • cosmos-sdk/crypto/keys/multisig/multisig_test.go

    Line 273 in 90e9370

    signatures[i] = &signing.SingleSignatureData{Signature: sig}
  • this creates sigs that are SIGN_MODE_UNSPECIFIED, which I believe defaults to SIGN_MODE_DIRECT
  • however, in any case, if I explicitly put SignMode: signing.SignMode_SIGN_MODE_DIRECT, all tests still pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot use sign mode direct with multisig as we'll need to know all the signers info before, this may not be a problem with tests though

Copy link
Member

Choose a reason for hiding this comment

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

We should revisit the sign modes discussion from #6078. I'm partly responsible for this, but I did also try to present alternatives, none of them got enough buy-in so we are where we are with SIGN_MODE_DIRECT and it's less than ideal for multisigs

Copy link
Member

Choose a reason for hiding this comment

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

See specifically the discussion starting from here: #6078 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

@zmanian could you advise on this please?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we need a new SIGN_MODE?

Copy link
Member

@zmanian zmanian Dec 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

IMO yeah SIGN_MODE_DIRECT isn't great for multisig users but I don't think it is a show stopper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest we merge this PR as is, to fix the existing multisig implementation of signbatch.

I'll create a separate issue to continue this discussion on a new SIGN_MODE for multisigs , which will be a breaking change and can't be incorporated into stargate at this point.

}
err = authclient.SignTxWithSignerAddress(txFactory, clientCtx, multisigAddr, clientCtx.GetFromName(), txBuilder, clientCtx.Offline)
}

if err != nil {
Expand Down