Skip to content

Commit

Permalink
fix: skip same-sender non-sequential sequence and then add others txs…
Browse files Browse the repository at this point in the history
… new solution (backport #19177) (#19250)

Co-authored-by: Brann Bronzebeard <[email protected]>
Co-authored-by: Facundo <[email protected]>
Co-authored-by: Facundo Medica <[email protected]>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
  • Loading branch information
5 people authored Jan 29, 2024
1 parent e62c19c commit 2a10f13
Show file tree
Hide file tree
Showing 5 changed files with 498 additions and 1 deletion.
6 changes: 5 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ Ref: https://keepachangelog.com/en/1.0.0/

## [Unreleased]

### Bug Fixes

* (baseapp) [#19177](https://github.com/cosmos/cosmos-sdk/pull/19177) Fix baseapp DefaultProposalHandler same-sender non-sequential sequence

## [v0.47.8](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.8) - 2024-01-22

### Improvements
Expand Down Expand Up @@ -440,4 +444,4 @@ extension interfaces. `module.Manager.Modules` is now of type `map[string]interf

## Previous Versions

[CHANGELOG of previous versions](https://github.com/cosmos/cosmos-sdk/blob/main/CHANGELOG.md#v0460---2022-07-26).
[CHANGELOG of previous versions](https://github.com/cosmos/cosmos-sdk/blob/main/CHANGELOG.md#v0460---2022-07-26).
46 changes: 46 additions & 0 deletions baseapp/abci_utils.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
package baseapp

import (
"fmt"

"github.com/cockroachdb/errors"
abci "github.com/cometbft/cometbft/abci/types"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/mempool"
"github.com/cosmos/cosmos-sdk/x/auth/signing"
)

type (
Expand Down Expand Up @@ -93,8 +96,41 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan
}

iterator := h.mempool.Select(ctx, req.Txs)
selectedTxsSignersSeqs := make(map[string]uint64)
var selectedTxsNums int
for iterator != nil {
memTx := iterator.Tx()
sigs, err := memTx.(signing.SigVerifiableTx).GetSignaturesV2()
if err != nil {
panic(fmt.Errorf("failed to get signatures: %w", err))
}

// if the signers aren't in selectedTxsSignersSeqs then we haven't seen them before
// so we add them and return true so this tx gets selected.
shouldAdd := true
txSignersSeqs := make(map[string]uint64)
for _, sig := range sigs {
signer := sdk.AccAddress(sig.PubKey.Address()).String()
seq, ok := selectedTxsSignersSeqs[signer]
if !ok {
txSignersSeqs[signer] = sig.Sequence
continue
}

// if we have seen this signer before we check if the sequence we just got is
// seq+1 and if it is we update the sequence and return true so this tx gets
// selected. If it isn't seq+1 we return false so this tx doesn't get
// selected (it could be the same sequence or seq+2 which are invalid).
if seq+1 != sig.Sequence {
shouldAdd = false
break
}
txSignersSeqs[signer] = sig.Sequence
}
if !shouldAdd {
iterator = iterator.Next()
continue
}

// NOTE: Since transaction verification was already executed in CheckTx,
// which calls mempool.Insert, in theory everything in the pool should be
Expand All @@ -111,6 +147,16 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan
if stop {
break
}

txsLen := len(h.txSelector.SelectedTxs())
for sender, seq := range txSignersSeqs {
if txsLen != selectedTxsNums {
selectedTxsSignersSeqs[sender] = seq
} else if _, ok := selectedTxsSignersSeqs[sender]; !ok {
selectedTxsSignersSeqs[sender] = seq - 1
}
}
selectedTxsNums = txsLen
}

iterator = iterator.Next()
Expand Down
281 changes: 281 additions & 0 deletions baseapp/abci_utils_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,281 @@
package baseapp_test

import (
"bytes"
"strings"
"testing"

abci "github.com/cometbft/cometbft/abci/types"
cmtsecp256k1 "github.com/cometbft/cometbft/crypto/secp256k1"
cmtprotocrypto "github.com/cometbft/cometbft/proto/tendermint/crypto"
cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
cmttypes "github.com/cometbft/cometbft/types"
protoio "github.com/cosmos/gogoproto/io"
"github.com/cosmos/gogoproto/proto"
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"

"github.com/cosmos/cosmos-sdk/codec"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
authtx "github.com/cosmos/cosmos-sdk/x/auth/tx"

"github.com/cosmos/cosmos-sdk/baseapp"
baseapptestutil "github.com/cosmos/cosmos-sdk/baseapp/testutil"
"github.com/cosmos/cosmos-sdk/baseapp/testutil/mock"
"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/mempool"
signingtypes "github.com/cosmos/cosmos-sdk/types/tx/signing"
)

const (
chainID = "chain-id"
)

type testValidator struct {
consAddr sdk.ConsAddress
tmPk cmtprotocrypto.PublicKey
privKey cmtsecp256k1.PrivKey
}

func newTestValidator() testValidator {
privkey := cmtsecp256k1.GenPrivKey()
pubkey := privkey.PubKey()
tmPk := cmtprotocrypto.PublicKey{
Sum: &cmtprotocrypto.PublicKey_Secp256K1{
Secp256K1: pubkey.Bytes(),
},
}

return testValidator{
consAddr: sdk.ConsAddress(pubkey.Address()),
tmPk: tmPk,
privKey: privkey,
}
}

func (t testValidator) toValidator(power int64) abci.Validator {
return abci.Validator{
Address: t.consAddr.Bytes(),
Power: power,
}
}

type ABCIUtilsTestSuite struct {
suite.Suite

vals [3]testValidator
ctx sdk.Context
}

func NewABCIUtilsTestSuite(t *testing.T) *ABCIUtilsTestSuite {
t.Helper()
// create 3 validators
s := &ABCIUtilsTestSuite{
vals: [3]testValidator{
newTestValidator(),
newTestValidator(),
newTestValidator(),
},
}

// create context
s.ctx = sdk.Context{}.WithConsensusParams(&cmtproto.ConsensusParams{})
return s
}

func TestABCIUtilsTestSuite(t *testing.T) {
suite.Run(t, NewABCIUtilsTestSuite(t))
}

func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_PriorityNonceMempoolTxSelection() {
cdc := codec.NewProtoCodec(codectypes.NewInterfaceRegistry())
baseapptestutil.RegisterInterfaces(cdc.InterfaceRegistry())
txConfig := authtx.NewTxConfig(cdc, authtx.DefaultSignModes)
ctrl := gomock.NewController(s.T())
app := mock.NewMockProposalTxVerifier(ctrl)
mp1 := mempool.NewPriorityMempool()
mp2 := mempool.NewPriorityMempool()
ph1 := baseapp.NewDefaultProposalHandler(mp1, app)
handler1 := ph1.PrepareProposalHandler()
ph2 := baseapp.NewDefaultProposalHandler(mp2, app)
handler2 := ph2.PrepareProposalHandler()
var (
secret1 = []byte("secret1")
secret2 = []byte("secret2")
secret3 = []byte("secret3")
secret4 = []byte("secret4")
secret5 = []byte("secret5")
secret6 = []byte("secret6")
ctx1 = s.ctx.WithPriority(10)
ctx2 = s.ctx.WithPriority(8)
)

tx1 := buildMsg(s.T(), txConfig, []byte(`1`), [][]byte{secret1}, []uint64{1})
tx2 := buildMsg(s.T(), txConfig, []byte(`12345678910`), [][]byte{secret1}, []uint64{2})
tx3 := buildMsg(s.T(), txConfig, []byte(`12`), [][]byte{secret1}, []uint64{3})
tx4 := buildMsg(s.T(), txConfig, []byte(`12`), [][]byte{secret2}, []uint64{1})
err := mp1.Insert(ctx1, tx1)
s.Require().NoError(err)
err = mp1.Insert(ctx1, tx2)
s.Require().NoError(err)
err = mp1.Insert(ctx1, tx3)
s.Require().NoError(err)
err = mp1.Insert(ctx2, tx4)
s.Require().NoError(err)
txBz1, err := txConfig.TxEncoder()(tx1)
s.Require().NoError(err)
txBz2, err := txConfig.TxEncoder()(tx2)
s.Require().NoError(err)
txBz3, err := txConfig.TxEncoder()(tx3)
s.Require().NoError(err)
txBz4, err := txConfig.TxEncoder()(tx4)
s.Require().NoError(err)
app.EXPECT().PrepareProposalVerifyTx(tx1).Return(txBz1, nil).AnyTimes()
app.EXPECT().PrepareProposalVerifyTx(tx2).Return(txBz2, nil).AnyTimes()
app.EXPECT().PrepareProposalVerifyTx(tx3).Return(txBz3, nil).AnyTimes()
app.EXPECT().PrepareProposalVerifyTx(tx4).Return(txBz4, nil).AnyTimes()
txDataSize1 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz1}))
txDataSize2 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz2}))
txDataSize3 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz3}))
txDataSize4 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz4}))
s.Require().Equal(txDataSize1, 111)
s.Require().Equal(txDataSize2, 121)
s.Require().Equal(txDataSize3, 112)
s.Require().Equal(txDataSize4, 112)

tx5 := buildMsg(s.T(), txConfig, []byte(`1`), [][]byte{secret1, secret2}, []uint64{1, 1})
tx6 := buildMsg(s.T(), txConfig, []byte(`12345678910`), [][]byte{secret1, secret3}, []uint64{2, 1})
tx7 := buildMsg(s.T(), txConfig, []byte(`12`), [][]byte{secret1, secret4}, []uint64{3, 1})
tx8 := buildMsg(s.T(), txConfig, []byte(`12`), [][]byte{secret3, secret5}, []uint64{2, 1})
tx9 := buildMsg(s.T(), txConfig, []byte(`12`), [][]byte{secret2, secret6}, []uint64{2, 1})

err = mp2.Insert(ctx1, tx5)
s.Require().NoError(err)
err = mp2.Insert(ctx1, tx6)
s.Require().NoError(err)
err = mp2.Insert(ctx2, tx7)
s.Require().NoError(err)
err = mp2.Insert(ctx2, tx8)
s.Require().NoError(err)
err = mp2.Insert(ctx2, tx9)
s.Require().NoError(err)
txBz5, err := txConfig.TxEncoder()(tx5)
s.Require().NoError(err)
txBz6, err := txConfig.TxEncoder()(tx6)
s.Require().NoError(err)
txBz7, err := txConfig.TxEncoder()(tx7)
s.Require().NoError(err)
txBz8, err := txConfig.TxEncoder()(tx8)
s.Require().NoError(err)
txBz9, err := txConfig.TxEncoder()(tx9)
s.Require().NoError(err)
app.EXPECT().PrepareProposalVerifyTx(tx5).Return(txBz5, nil).AnyTimes()
app.EXPECT().PrepareProposalVerifyTx(tx6).Return(txBz6, nil).AnyTimes()
app.EXPECT().PrepareProposalVerifyTx(tx7).Return(txBz7, nil).AnyTimes()
app.EXPECT().PrepareProposalVerifyTx(tx8).Return(txBz8, nil).AnyTimes()
app.EXPECT().PrepareProposalVerifyTx(tx9).Return(txBz9, nil).AnyTimes()
txDataSize5 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz5}))
txDataSize6 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz6}))
txDataSize7 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz7}))
txDataSize8 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz8}))
txDataSize9 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz9}))
s.Require().Equal(txDataSize5, 195)
s.Require().Equal(txDataSize6, 205)
s.Require().Equal(txDataSize7, 196)
s.Require().Equal(txDataSize8, 196)
s.Require().Equal(txDataSize9, 196)

mapTxs := map[string]string{
string(txBz1): "1",
string(txBz2): "2",
string(txBz3): "3",
string(txBz4): "4",
string(txBz5): "5",
string(txBz6): "6",
string(txBz7): "7",
string(txBz8): "8",
string(txBz9): "9",
}
testCases := map[string]struct {
ctx sdk.Context
req abci.RequestPrepareProposal
handler sdk.PrepareProposalHandler
expectedTxs [][]byte
}{
"skip same-sender non-sequential sequence and then add others txs": {
ctx: s.ctx,
req: abci.RequestPrepareProposal{
Txs: [][]byte{txBz1, txBz2, txBz3, txBz4},
MaxTxBytes: 111 + 112,
},
handler: handler1,
expectedTxs: [][]byte{txBz1, txBz4},
},
"skip multi-signers msg non-sequential sequence": {
ctx: s.ctx,
req: abci.RequestPrepareProposal{
Txs: [][]byte{txBz5, txBz6, txBz7, txBz8, txBz9},
MaxTxBytes: 195 + 196,
},
handler: handler2,
expectedTxs: [][]byte{txBz5, txBz9},
},
}

for name, tc := range testCases {
s.Run(name, func() {
resp := tc.handler(tc.ctx, tc.req)
s.Require().EqualValues(toHumanReadable(mapTxs, resp.Txs), toHumanReadable(mapTxs, tc.expectedTxs))
})
}
}

func marshalDelimitedFn(msg proto.Message) ([]byte, error) {
var buf bytes.Buffer
if err := protoio.NewDelimitedWriter(&buf).WriteMsg(msg); err != nil {
return nil, err
}

return buf.Bytes(), nil
}

func buildMsg(t *testing.T, txConfig client.TxConfig, value []byte, secrets [][]byte, nonces []uint64) sdk.Tx {
t.Helper()
builder := txConfig.NewTxBuilder()
_ = builder.SetMsgs(
&baseapptestutil.MsgKeyValue{Value: value},
)
require.Equal(t, len(secrets), len(nonces))
signatures := make([]signingtypes.SignatureV2, 0)
for index, secret := range secrets {
nonce := nonces[index]
privKey := secp256k1.GenPrivKeyFromSecret(secret)
pubKey := privKey.PubKey()
signatures = append(signatures, signingtypes.SignatureV2{
PubKey: pubKey,
Sequence: nonce,
Data: &signingtypes.SingleSignatureData{},
})
}
setTxSignatureWithSecret(t, builder, signatures...)
return builder.GetTx()
}

func toHumanReadable(mapTxs map[string]string, txs [][]byte) string {
strs := []string{}
for _, v := range txs {
strs = append(strs, mapTxs[string(v)])
}
return strings.Join(strs, ",")
}

func setTxSignatureWithSecret(t *testing.T, builder client.TxBuilder, signatures ...signingtypes.SignatureV2) {
t.Helper()
err := builder.SetSignatures(
signatures...,
)
require.NoError(t, err)
}
Loading

0 comments on commit 2a10f13

Please sign in to comment.