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.

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

fix: skip same-sender non-sequential sequence and then add others txs new solution #19177

Merged
merged 4 commits into from
Jan 25, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i

* (x/staking) [#19226](https://github.com/cosmos/cosmos-sdk/pull/19226) Ensure `GetLastValidators` in `x/staking` does not return an error when `MaxValidators` exceeds total number of bonded validators.
* (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
* (baseapp) [#18727](https://github.com/cosmos/cosmos-sdk/pull/18727) Ensure that `BaseApp.Init` firstly returns any errors from a nil commit multistore instead of panicking on nil dereferencing and before sealing the app.
* (client) [#18622](https://github.com/cosmos/cosmos-sdk/pull/18622) Fixed a potential under/overflow from `uint64->int64` when computing gas fees as a LegacyDec.
* (client/keys) [#18562](https://github.com/cosmos/cosmos-sdk/pull/18562) `keys delete` won't terminate when a key is not found.
Expand Down
56 changes: 50 additions & 6 deletions baseapp/abci_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,17 +157,19 @@
// 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 @@ -227,8 +229,40 @@
}

iterator := h.mempool.Select(ctx, req.Txs)
selectedTxsSignersSeqs := make(map[string]uint64)
var selectedTxsNums int
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -245,6 +279,16 @@
if stop {
break
}

txsLen := len(h.txSelector.SelectedTxs(ctx))
ZiHengLee marked this conversation as resolved.
Show resolved Hide resolved
for sender, seq := range txSignersSeqs {
if txsLen != selectedTxsNums {
selectedTxsSignersSeqs[sender] = seq
} else if _, ok := selectedTxsSignersSeqs[sender]; !ok {
selectedTxsSignersSeqs[sender] = seq - 1
}
}
Comment on lines +284 to +290

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism
selectedTxsNums = txsLen
ZiHengLee marked this conversation as resolved.
Show resolved Hide resolved
}

iterator = iterator.Next()
Expand Down
205 changes: 202 additions & 3 deletions baseapp/abci_utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,19 @@ 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"
Expand All @@ -21,10 +23,13 @@ import (
"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"
)

const (
Expand All @@ -34,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 @@ -415,6 +420,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": {
Copy link
Member

Choose a reason for hiding this comment

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

let's add a multi signer test please

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 @@ -423,3 +584,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)
}
Loading