From 2eb6fd9a81a3b3eaf606cccf4b512dfc4aa9d4c5 Mon Sep 17 00:00:00 2001 From: Sujong Lee Date: Mon, 17 Oct 2022 06:31:36 +0900 Subject: [PATCH 1/2] fix: allow up to one direct sign in multi sign tx (#708) * fix: TXT01 - allow one direct sign in multi sign * docs: add changelog --- CHANGELOG.md | 3 +- client/tx/tx.go | 55 ++++++++++++++++++++++++++++----- client/tx/tx_test.go | 26 ++++++++++++---- x/auth/client/testutil/suite.go | 3 +- 4 files changed, 71 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6018d4fd0c..dcf04b11c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,7 +77,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/wasm,distribution) [\#696](https://github.com/line/lbm-sdk/pull/696) x/wasm,distribution - add checking a file size before reading it * (x/foundation) [\#698](https://github.com/line/lbm-sdk/pull/698) update x/group relevant logic in x/foundation * (x/auth,bank,foundation,wasm) [\#691](https://github.com/line/lbm-sdk/pull/691) change AccAddressFromBech32 to MustAccAddressFromBech32 -* (x/wasm) [\#690](https://github.com/line/lbm-sdk/pull/690) fix to prevent accepting file name +* (x/wasm) [\#690](https://github.com/line/lbm-sdk/pull/690) fix to prevent accepting file name +* (cli) [\#708](https://github.com/line/lbm-sdk/pull/708) In CLI, allow 1 SIGN_MODE_DIRECT signer in transactions with multiple signers. ### Bug Fixes * (x/wasm) [\#453](https://github.com/line/lbm-sdk/pull/453) modify wasm grpc query api path diff --git a/client/tx/tx.go b/client/tx/tx.go index e9e94dfdad..a27ee09915 100644 --- a/client/tx/tx.go +++ b/client/tx/tx.go @@ -317,11 +317,41 @@ func SignWithPrivKey( return sigV2, nil } -func checkMultipleSigners(mode signing.SignMode, tx authsigning.Tx) error { - if mode == signing.SignMode_SIGN_MODE_DIRECT && - len(tx.GetSigners()) > 1 { - return sdkerrors.Wrap(sdkerrors.ErrNotSupported, "Signing in DIRECT mode is only supported for transactions with one signer only") +// countDirectSigners counts the number of DIRECT signers in a signature data. +func countDirectSigners(data signing.SignatureData) int { + switch data := data.(type) { + case *signing.SingleSignatureData: + if data.SignMode == signing.SignMode_SIGN_MODE_DIRECT { + return 1 + } + + return 0 + case *signing.MultiSignatureData: + directSigners := 0 + for _, d := range data.Signatures { + directSigners += countDirectSigners(d) + } + + return directSigners + default: + panic("unreachable case") + } +} + +// checkMultipleSigners checks that there can be maximum one DIRECT signer in a tx. +func checkMultipleSigners(tx authsigning.Tx) error { + directSigners := 0 + sigsV2, err := tx.GetSignaturesV2() + if err != nil { + return err + } + for _, sig := range sigsV2 { + directSigners += countDirectSigners(sig.Data) + if directSigners > 1 { + return sdkerrors.ErrNotSupported.Wrap("txs signed with CLI can have maximum 1 DIRECT signer") + } } + return nil } @@ -341,9 +371,6 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwriteSig boo // use the SignModeHandler's default mode if unspecified signMode = txf.txConfig.SignModeHandler().DefaultMode() } - if err := checkMultipleSigners(signMode, txBuilder.GetTx()); err != nil { - return err - } key, err := txf.keybase.Key(name) if err != nil { @@ -373,6 +400,7 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwriteSig boo Data: &sigData, Sequence: txf.Sequence(), } + var prevSignatures []signing.SignatureV2 if !overwriteSig { prevSignatures, err = txBuilder.GetTx().GetSignaturesV2() @@ -380,7 +408,18 @@ func Sign(txf Factory, name string, txBuilder client.TxBuilder, overwriteSig boo return err } } - if err := txBuilder.SetSignatures(sig); err != nil { + // Overwrite or append signer infos. + var sigs []signing.SignatureV2 + if overwriteSig { + sigs = []signing.SignatureV2{sig} + } else { + sigs = append(prevSignatures, sig) + } + if err := txBuilder.SetSignatures(sigs...); err != nil { + return err + } + + if err := checkMultipleSigners(txBuilder.GetTx()); err != nil { return err } diff --git a/client/tx/tx_test.go b/client/tx/tx_test.go index edee55de2f..911d8e391e 100644 --- a/client/tx/tx_test.go +++ b/client/tx/tx_test.go @@ -306,25 +306,39 @@ func TestSign(t *testing.T) { }, /**** test double sign Direct mode - signing transaction with more than 2 signers should fail in DIRECT mode ****/ + signing transaction with 2 or more DIRECT signers should fail in DIRECT mode ****/ { - "direct: should fail to append a signature with different mode", + "direct: should append a DIRECT signature with existing AMINO", + // txb already has 1 AMINO signature txfDirect, txb, from1, false, - []cryptotypes.PubKey{}, + []cryptotypes.PubKey{pubKey2, pubKey1}, nil, }, { - "direct: should fail to sign multi-signers tx", + "direct: should add single DIRECT sig in multi-signers tx", txfDirect, txb2, from1, false, + []cryptotypes.PubKey{pubKey1}, + nil, + }, + { + "direct: should fail to append 2nd DIRECT sig in multi-signers tx", + txfDirect, txb2, from2, false, []cryptotypes.PubKey{}, nil, }, { - "direct: should fail to overwrite multi-signers tx", - txfDirect, txb2, from1, true, + "amino: should append 2nd AMINO sig in multi-signers tx with 1 DIRECT sig", + // txb2 already has 1 DIRECT signature + txfAmino, txb2, from2, false, []cryptotypes.PubKey{}, nil, }, + { + "direct: should overwrite multi-signers tx with DIRECT sig", + txfDirect, txb2, from1, true, + []cryptotypes.PubKey{pubKey1}, + nil, + }, } var prevSigs []signingtypes.SignatureV2 for _, tc := range testCases { diff --git a/x/auth/client/testutil/suite.go b/x/auth/client/testutil/suite.go index 9bb89fcef8..4545ba9619 100644 --- a/x/auth/client/testutil/suite.go +++ b/x/auth/client/testutil/suite.go @@ -33,6 +33,7 @@ import ( authtypes "github.com/line/lbm-sdk/x/auth/types" bankcli "github.com/line/lbm-sdk/x/bank/client/testutil" banktypes "github.com/line/lbm-sdk/x/bank/types" + "github.com/line/lbm-sdk/x/genutil/client/cli" ) type IntegrationTestSuite struct { @@ -1247,7 +1248,7 @@ func (s *IntegrationTestSuite) TestTxWithoutPublicKey() { unsignedTxFile := testutil.WriteToNewTempFile(s.T(), string(txJSON)) // Sign the file with the unsignedTx. - signedTx, err := TxSignExec(val1.ClientCtx, val1.Address, unsignedTxFile.Name()) + signedTx, err := TxSignExec(val1.ClientCtx, val1.Address, unsignedTxFile.Name(), fmt.Sprintf("--%s=true", cli.FlagOverwrite)) s.Require().NoError(err) // Remove the signerInfo's `public_key` field manually from the signedTx. From 00ab960bd59775220bf84320e3146c882a63cce5 Mon Sep 17 00:00:00 2001 From: Youngtaek Yoon Date: Mon, 17 Oct 2022 09:47:24 +0000 Subject: [PATCH 2/2] fix: fix x/foundation EndBlocker (#712) * Fix EndBlocker * Update CHANGELOG.md --- CHANGELOG.md | 1 + x/foundation/keeper/abci_test.go | 72 +++++++++++++++++++++++++++++ x/foundation/keeper/keys.go | 79 ++++++++++++++++++-------------- x/foundation/keeper/proposal.go | 14 ++++-- 4 files changed, 127 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dcf04b11c2..e00026e5d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,6 +98,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/foundation) [\#687](https://github.com/line/lbm-sdk/pull/687) fix bugs on aborting x/foundation proposals * (global) [\#694](https://github.com/line/lbm-sdk/pull/694) replace deprecated functions since go 1.16 or 1.17 * (x/bankplus) [\#705](https://github.com/line/lbm-sdk/pull/705) add missing blockedAddr checking in bankplus +* (x/foundation) [\#712](https://github.com/line/lbm-sdk/pull/712) fix x/foundation EndBlocker ### Breaking Changes * (proto) [\#564](https://github.com/line/lbm-sdk/pull/564) change gRPC path to original cosmos path diff --git a/x/foundation/keeper/abci_test.go b/x/foundation/keeper/abci_test.go index c367aa332c..d328a472a0 100644 --- a/x/foundation/keeper/abci_test.go +++ b/x/foundation/keeper/abci_test.go @@ -26,3 +26,75 @@ func (s *KeeperTestSuite) TestBeginBlocker() { // s.balance + s.balance * 0.5 s.Require().Equal(sdk.NewDecFromInt(s.balance.Add(s.balance.Quo(sdk.NewInt(2)))), after[0].Amount) } + +func (s *KeeperTestSuite) TestEndBlocker() { + ctx, _ := s.ctx.CacheContext() + + // check preconditions + for name, tc := range map[string]struct { + id uint64 + status foundation.ProposalStatus + }{ + "active proposal": { + s.activeProposal, + foundation.PROPOSAL_STATUS_SUBMITTED, + }, + "voted proposal": { + s.votedProposal, + foundation.PROPOSAL_STATUS_SUBMITTED, + }, + "withdrawn proposal": { + s.withdrawnProposal, + foundation.PROPOSAL_STATUS_WITHDRAWN, + }, + "invalid proposal": { + s.invalidProposal, + foundation.PROPOSAL_STATUS_SUBMITTED, + }, + } { + proposal, err := s.keeper.GetProposal(ctx, tc.id) + s.Require().NoError(err, name) + s.Require().NotNil(proposal, name) + s.Require().Equal(tc.status, proposal.Status, name) + } + + // voting periods end + votingPeriod := s.keeper.GetFoundationInfo(ctx).GetDecisionPolicy().GetVotingPeriod() + ctx = ctx.WithBlockTime(ctx.BlockTime().Add(votingPeriod)) + keeper.EndBlocker(ctx, s.keeper) + + for name, tc := range map[string]struct { + id uint64 + status foundation.ProposalStatus + }{ + "active proposal": { + s.activeProposal, + foundation.PROPOSAL_STATUS_ACCEPTED, + }, + "voted proposal": { + s.votedProposal, + foundation.PROPOSAL_STATUS_ACCEPTED, + }, + "withdrawn proposal": { + s.withdrawnProposal, + foundation.PROPOSAL_STATUS_WITHDRAWN, + }, + "invalid proposal": { + s.invalidProposal, + foundation.PROPOSAL_STATUS_ACCEPTED, + }, + } { + proposal, err := s.keeper.GetProposal(ctx, tc.id) + s.Require().NoError(err, name) + s.Require().NotNil(proposal, name) + s.Require().Equal(tc.status, proposal.Status, name) + } + + // proposals expire + maxExecutionPeriod := foundation.DefaultConfig().MaxExecutionPeriod + ctx = ctx.WithBlockTime(ctx.BlockTime().Add(maxExecutionPeriod)) + keeper.EndBlocker(ctx, s.keeper) + + // all proposals must be pruned + s.Require().Empty(s.keeper.GetProposals(ctx)) +} diff --git a/x/foundation/keeper/keys.go b/x/foundation/keeper/keys.go index 32060cb4c9..c70add29b9 100644 --- a/x/foundation/keeper/keys.go +++ b/x/foundation/keeper/keys.go @@ -24,6 +24,9 @@ var ( poolKey = []byte{0x30} ) +// must be constant +var lenTime = len(sdk.FormatTimeBytes(time.Now())) + // Uint64FromBytes converts a byte array to uint64 func Uint64FromBytes(bz []byte) uint64 { return binary.BigEndian.Uint64(bz) @@ -38,30 +41,36 @@ func Uint64ToBytes(number uint64) []byte { // memberKey key for a specific member from the store func memberKey(address sdk.AccAddress) []byte { - key := make([]byte, len(memberKeyPrefix)+len(address)) - copy(key, memberKeyPrefix) - copy(key[len(memberKeyPrefix):], address) + prefix := memberKeyPrefix + key := make([]byte, len(prefix)+len(address)) + + copy(key, prefix) + copy(key[len(prefix):], address) + return key } // proposalKey key for a specific proposal from the store func proposalKey(id uint64) []byte { + prefix := proposalKeyPrefix idBz := Uint64ToBytes(id) + key := make([]byte, len(prefix)+len(idBz)) + + copy(key, prefix) + copy(key[len(prefix):], idBz) - key := make([]byte, len(proposalKeyPrefix)+len(idBz)) - copy(key, proposalKeyPrefix) - copy(key[len(proposalKeyPrefix):], idBz) return key } func voteKey(proposalID uint64, voter sdk.AccAddress) []byte { + prefix := voteKeyPrefix idBz := Uint64ToBytes(proposalID) - key := make([]byte, len(voteKeyPrefix)+len(idBz)+len(voter)) + key := make([]byte, len(prefix)+len(idBz)+len(voter)) begin := 0 - copy(key[begin:], voteKeyPrefix) + copy(key[begin:], prefix) - begin += len(voteKeyPrefix) + begin += len(prefix) copy(key[begin:], idBz) begin += len(idBz) @@ -70,39 +79,38 @@ func voteKey(proposalID uint64, voter sdk.AccAddress) []byte { return key } -func proposalByVPEndKey(id uint64, end time.Time) []byte { +func proposalByVPEndKey(vpEnd time.Time, id uint64) []byte { + prefix := proposalByVPEndKeyPrefix + vpEndBz := sdk.FormatTimeBytes(vpEnd) idBz := Uint64ToBytes(id) - endBz := sdk.FormatTimeBytes(end) - key := make([]byte, len(proposalByVPEndKeyPrefix)+1+len(idBz)+len(endBz)) + key := make([]byte, len(prefix)+lenTime+len(idBz)) begin := 0 - copy(key[begin:], proposalByVPEndKeyPrefix) + copy(key[begin:], prefix) - begin += len(proposalByVPEndKeyPrefix) - key[begin] = byte(len(idBz)) + begin += len(prefix) + copy(key[begin:], vpEndBz) - begin++ + begin += len(vpEndBz) copy(key[begin:], idBz) - begin += len(idBz) - copy(key[begin:], endBz) - return key } -// func splitProposalByVPEndKey(key []byte) (proposalID uint64, vpEnd time.Time) { -// begin := len(proposalByVPEndKeyPrefix) + 1 -// end := begin + int(key[begin-1]) // uint64 -// proposalID = Uint64FromBytes(key[begin:end]) +func splitProposalByVPEndKey(key []byte) (vpEnd time.Time, id uint64) { + prefix := proposalByVPEndKeyPrefix + begin := len(prefix) + end := begin + lenTime + vpEnd, err := sdk.ParseTimeBytes(key[begin:end]) + if err != nil { + panic(err) + } -// begin = end -// vpEnd, err := sdk.ParseTimeBytes(key[begin:]) -// if err != nil { -// panic(err) -// } + begin = end + id = Uint64FromBytes(key[begin:]) -// return -// } + return +} func grantKey(grantee sdk.AccAddress, url string) []byte { prefix := grantKeyPrefixByGrantee(grantee) @@ -115,12 +123,13 @@ func grantKey(grantee sdk.AccAddress, url string) []byte { } func grantKeyPrefixByGrantee(grantee sdk.AccAddress) []byte { - key := make([]byte, len(grantKeyPrefix)+1+len(grantee)) + prefix := grantKeyPrefix + key := make([]byte, len(prefix)+1+len(grantee)) begin := 0 - copy(key[begin:], grantKeyPrefix) + copy(key[begin:], prefix) - begin += len(grantKeyPrefix) + begin += len(prefix) key[begin] = byte(len(grantee)) begin++ @@ -130,7 +139,9 @@ func grantKeyPrefixByGrantee(grantee sdk.AccAddress) []byte { } func splitGrantKey(key []byte) (grantee sdk.AccAddress, url string) { - begin := len(grantKeyPrefix) + 1 + prefix := grantKeyPrefix + + begin := len(prefix) + 1 end := begin + int(key[begin-1]) grantee = key[begin:end] diff --git a/x/foundation/keeper/proposal.go b/x/foundation/keeper/proposal.go index e6c6241fa2..0b36331596 100644 --- a/x/foundation/keeper/proposal.go +++ b/x/foundation/keeper/proposal.go @@ -181,10 +181,14 @@ func (k Keeper) iterateProposalsByVPEnd(ctx sdk.Context, endTime time.Time, fn f defer iter.Close() for ; iter.Valid(); iter.Next() { - var proposal foundation.Proposal - k.cdc.MustUnmarshal(iter.Value(), &proposal) + _, id := splitProposalByVPEndKey(iter.Key()) + + proposal, err := k.GetProposal(ctx, id) + if err != nil { + panic(err) + } - if fn(proposal) { + if fn(*proposal) { break } } @@ -232,13 +236,13 @@ func (k Keeper) deleteProposal(ctx sdk.Context, proposalID uint64) { func (k Keeper) addProposalToVPEndQueue(ctx sdk.Context, proposal foundation.Proposal) { store := ctx.KVStore(k.storeKey) - key := proposalByVPEndKey(proposal.Id, proposal.VotingPeriodEnd) + key := proposalByVPEndKey(proposal.VotingPeriodEnd, proposal.Id) store.Set(key, []byte{}) } func (k Keeper) removeProposalFromVPEndQueue(ctx sdk.Context, proposal foundation.Proposal) { store := ctx.KVStore(k.storeKey) - key := proposalByVPEndKey(proposal.Id, proposal.VotingPeriodEnd) + key := proposalByVPEndKey(proposal.VotingPeriodEnd, proposal.Id) store.Delete(key) }