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) (#19249)

Co-authored-by: Brann Bronzebeard <90186866+ZiHengLee@users.noreply.github.com>
Co-authored-by: Facundo <facundomedica@gmail.com>
  • Loading branch information
@mergify @ZiHengLee @facundomedica
3 people authored Jan 26, 2024
1 parent 0abf94a commit c4a2fe2
Show file tree
Hide file tree
Showing 3 changed files with 255 additions and 10 deletions.
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ Ref: https://keepachangelog.com/en/1.0.0/

* (abci): [#19200](https://github.com/cosmos/cosmos-sdk/pull/19200) Ensure that sdk side ve math matches cometbft
* [#19106](https://github.com/cosmos/cosmos-sdk/pull/19106) Allow empty public keys when setting signatures. Public keys aren't needed for every transaction.
* (baseapp) [#19198](https://github.com/cosmos/cosmos-sdk/pull/19198) Remove usage of pointers in logs in all OE goroutines.
* (baseapp) [#19177](https://github.com/cosmos/cosmos-sdk/pull/19177) Fix baseapp DefaultProposalHandler same-sender non-sequential sequence

## [v0.50.3](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.3) - 2023-01-15

Expand All @@ -63,7 +65,6 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Bug Fixes

* (baseapp) [#19198](https://github.com/cosmos/cosmos-sdk/pull/19198) Remove usage of pointers in logs in all OE goroutines.
* (baseapp) [#19058](https://github.com/cosmos/cosmos-sdk/pull/19058) Fix baseapp posthandler branch would fail if the `runMsgs` had returned an error.
* (baseapp) [#18609](https://github.com/cosmos/cosmos-sdk/issues/18609) Fixed accounting in the block gas meter after module's beginBlock and before DeliverTx, ensuring transaction processing always starts with the expected zeroed out block gas meter.
* (baseapp) [#18895](https://github.com/cosmos/cosmos-sdk/pull/18895) Fix de-duplicating vote extensions during validation in ValidateVoteExtensions.
Expand Down
56 changes: 50 additions & 6 deletions baseapp/abci_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,17 +154,19 @@ type (
// DefaultProposalHandler defines the default ABCI PrepareProposal and
// ProcessProposal handlers.
DefaultProposalHandler struct {
mempool mempool.Mempool
txVerifier ProposalTxVerifier
txSelector TxSelector
mempool mempool.Mempool
txVerifier ProposalTxVerifier
txSelector TxSelector
signerExtAdapter mempool.SignerExtractionAdapter
}
)

func NewDefaultProposalHandler(mp mempool.Mempool, txVerifier ProposalTxVerifier) *DefaultProposalHandler {
return &DefaultProposalHandler{
mempool: mp,
txVerifier: txVerifier,
txSelector: NewDefaultTxSelector(),
mempool: mp,
txVerifier: txVerifier,
txSelector: NewDefaultTxSelector(),
signerExtAdapter: mempool.NewDefaultSignerExtractionAdapter(),
}
}

Expand Down Expand Up @@ -224,8 +226,40 @@ 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()
signerData, err := h.signerExtAdapter.GetSigners(memTx)
if err != nil {
return nil, 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 _, signer := range signerData {
seq, ok := selectedTxsSignersSeqs[signer.Signer.String()]
if !ok {
txSignersSeqs[signer.Signer.String()] = signer.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 != signer.Sequence {
shouldAdd = false
break
}
txSignersSeqs[signer.Signer.String()] = signer.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 @@ -242,6 +276,16 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan
if stop {
break
}

txsLen := len(h.txSelector.SelectedTxs(ctx))
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
206 changes: 203 additions & 3 deletions baseapp/abci_utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,27 +2,33 @@ package baseapp_test

import (
"bytes"
"strings"
"testing"

abci "github.com/cometbft/cometbft/abci/types"
"github.com/cometbft/cometbft/crypto/secp256k1"
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"
dbm "github.com/cosmos/cosmos-db"
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"

"cosmossdk.io/log"

"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"
codectestutil "github.com/cosmos/cosmos-sdk/codec/testutil"
"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
"github.com/cosmos/cosmos-sdk/testutil/testdata"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/mempool"
signingtypes "github.com/cosmos/cosmos-sdk/types/tx/signing"
authtx "github.com/cosmos/cosmos-sdk/x/auth/tx"
)

Expand All @@ -33,11 +39,11 @@ const (
type testValidator struct {
consAddr sdk.ConsAddress
tmPk cmtprotocrypto.PublicKey
privKey secp256k1.PrivKey
privKey cmtsecp256k1.PrivKey
}

func newTestValidator() testValidator {
privkey := secp256k1.GenPrivKey()
privkey := cmtsecp256k1.GenPrivKey()
pubkey := privkey.PubKey()
tmPk := cmtprotocrypto.PublicKey{
Sum: &cmtprotocrypto.PublicKey_Secp256K1{
Expand Down Expand Up @@ -403,6 +409,162 @@ func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_NoOpMempoolTxSelection()
}
}

func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_PriorityNonceMempoolTxSelection() {
cdc := codectestutil.CodecOptions{}.NewCodec()
baseapptestutil.RegisterInterfaces(cdc.InterfaceRegistry())
txConfig := authtx.NewTxConfig(cdc, authtx.DefaultSignModes)
ctrl := gomock.NewController(s.T())
app := mock.NewMockProposalTxVerifier(ctrl)
mp1 := mempool.NewPriorityMempool(
mempool.PriorityNonceMempoolConfig[int64]{
TxPriority: mempool.NewDefaultTxPriority(),
MaxTx: 0,
SignerExtractor: mempool.NewDefaultSignerExtractionAdapter(),
},
)
mp2 := mempool.NewPriorityMempool(
mempool.PriorityNonceMempoolConfig[int64]{
TxPriority: mempool.NewDefaultTxPriority(),
MaxTx: 0,
SignerExtractor: mempool.NewDefaultSignerExtractionAdapter(),
},
)
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, err := tc.handler(tc.ctx, tc.req)
s.Require().NoError(err)
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 {
Expand All @@ -411,3 +573,41 @@ func marshalDelimitedFn(msg proto.Message) ([]byte, error) {

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)
}

0 comments on commit c4a2fe2

Please sign in to comment.