Skip to content

Commit a46cf12

Browse files
mergify[bot]facundomedica
authored and
MSalopek
committed
test(baseapp): Refactor tx selector tests + better comments (backport cosmos#19284) (cosmos#19288)
Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Co-authored-by: Facundo <facundomedica@gmail.com>
1 parent 84cbbda commit a46cf12

2 files changed

Lines changed: 104 additions & 119 deletions

File tree

‎baseapp/abci_utils.go‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,8 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan
105105
panic(fmt.Errorf("failed to get signatures: %w", err))
106106
}
107107

108-
// if the signers aren't in selectedTxsSignersSeqs then we haven't seen them before
109-
// so we add them and return true so this tx gets selected.
108+
// If the signers aren't in selectedTxsSignersSeqs then we haven't seen them before
109+
// so we add them and continue given that we don't need to check the sequence.
110110
shouldAdd := true
111111
txSignersSeqs := make(map[string]uint64)
112112
for _, sig := range sigs {
@@ -117,10 +117,9 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan
117117
continue
118118
}
119119

120-
// if we have seen this signer before we check if the sequence we just got is
121-
// seq+1 and if it is we update the sequence and return true so this tx gets
122-
// selected. If it isn't seq+1 we return false so this tx doesn't get
123-
// selected (it could be the same sequence or seq+2 which are invalid).
120+
// If we have seen this signer before in this block, we must make
121+
// sure that the current sequence is seq+1; otherwise is invalid
122+
// and we skip it.
124123
if seq+1 != sig.Sequence {
125124
shouldAdd = false
126125
break
@@ -150,9 +149,16 @@ func (h *DefaultProposalHandler) PrepareProposalHandler() sdk.PrepareProposalHan
150149

151150
txsLen := len(h.txSelector.SelectedTxs())
152151
for sender, seq := range txSignersSeqs {
152+
// If txsLen != selectedTxsNums is true, it means that we've
153+
// added a new tx to the selected txs, so we need to update
154+
// the sequence of the sender.
153155
if txsLen != selectedTxsNums {
154156
selectedTxsSignersSeqs[sender] = seq
155157
} else if _, ok := selectedTxsSignersSeqs[sender]; !ok {
158+
// The transaction hasn't been added but it passed the
159+
// verification, so we know that the sequence is correct.
160+
// So we set this sender's sequence to seq-1, in order
161+
// to avoid unnecessary calls to PrepareProposalVerifyTx.
156162
selectedTxsSignersSeqs[sender] = seq - 1
157163
}
158164
}

‎baseapp/abci_utils_test.go‎

Lines changed: 92 additions & 113 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package baseapp_test
22

33
import (
44
"bytes"
5-
"strings"
65
"testing"
76

87
abci "github.com/cometbft/cometbft/abci/types"
@@ -94,141 +93,129 @@ func (s *ABCIUtilsTestSuite) TestDefaultProposalHandler_PriorityNonceMempoolTxSe
9493
cdc := codec.NewProtoCodec(codectypes.NewInterfaceRegistry())
9594
baseapptestutil.RegisterInterfaces(cdc.InterfaceRegistry())
9695
txConfig := authtx.NewTxConfig(cdc, authtx.DefaultSignModes)
97-
ctrl := gomock.NewController(s.T())
98-
app := mock.NewMockProposalTxVerifier(ctrl)
99-
mp1 := mempool.NewPriorityMempool()
100-
mp2 := mempool.NewPriorityMempool()
101-
ph1 := baseapp.NewDefaultProposalHandler(mp1, app)
102-
handler1 := ph1.PrepareProposalHandler()
103-
ph2 := baseapp.NewDefaultProposalHandler(mp2, app)
104-
handler2 := ph2.PrepareProposalHandler()
10596
var (
10697
secret1 = []byte("secret1")
10798
secret2 = []byte("secret2")
10899
secret3 = []byte("secret3")
109100
secret4 = []byte("secret4")
110101
secret5 = []byte("secret5")
111102
secret6 = []byte("secret6")
112-
ctx1 = s.ctx.WithPriority(10)
113-
ctx2 = s.ctx.WithPriority(8)
114103
)
115104

116-
tx1 := buildMsg(s.T(), txConfig, []byte(`1`), [][]byte{secret1}, []uint64{1})
117-
tx2 := buildMsg(s.T(), txConfig, []byte(`12345678910`), [][]byte{secret1}, []uint64{2})
118-
tx3 := buildMsg(s.T(), txConfig, []byte(`12`), [][]byte{secret1}, []uint64{3})
119-
tx4 := buildMsg(s.T(), txConfig, []byte(`12`), [][]byte{secret2}, []uint64{1})
120-
err := mp1.Insert(ctx1, tx1)
121-
s.Require().NoError(err)
122-
err = mp1.Insert(ctx1, tx2)
123-
s.Require().NoError(err)
124-
err = mp1.Insert(ctx1, tx3)
125-
s.Require().NoError(err)
126-
err = mp1.Insert(ctx2, tx4)
127-
s.Require().NoError(err)
128-
txBz1, err := txConfig.TxEncoder()(tx1)
129-
s.Require().NoError(err)
130-
txBz2, err := txConfig.TxEncoder()(tx2)
131-
s.Require().NoError(err)
132-
txBz3, err := txConfig.TxEncoder()(tx3)
133-
s.Require().NoError(err)
134-
txBz4, err := txConfig.TxEncoder()(tx4)
135-
s.Require().NoError(err)
136-
app.EXPECT().PrepareProposalVerifyTx(tx1).Return(txBz1, nil).AnyTimes()
137-
app.EXPECT().PrepareProposalVerifyTx(tx2).Return(txBz2, nil).AnyTimes()
138-
app.EXPECT().PrepareProposalVerifyTx(tx3).Return(txBz3, nil).AnyTimes()
139-
app.EXPECT().PrepareProposalVerifyTx(tx4).Return(txBz4, nil).AnyTimes()
140-
txDataSize1 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz1}))
141-
txDataSize2 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz2}))
142-
txDataSize3 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz3}))
143-
txDataSize4 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz4}))
144-
s.Require().Equal(txDataSize1, 111)
145-
s.Require().Equal(txDataSize2, 121)
146-
s.Require().Equal(txDataSize3, 112)
147-
s.Require().Equal(txDataSize4, 112)
148-
149-
tx5 := buildMsg(s.T(), txConfig, []byte(`1`), [][]byte{secret1, secret2}, []uint64{1, 1})
150-
tx6 := buildMsg(s.T(), txConfig, []byte(`12345678910`), [][]byte{secret1, secret3}, []uint64{2, 1})
151-
tx7 := buildMsg(s.T(), txConfig, []byte(`12`), [][]byte{secret1, secret4}, []uint64{3, 1})
152-
tx8 := buildMsg(s.T(), txConfig, []byte(`12`), [][]byte{secret3, secret5}, []uint64{2, 1})
153-
tx9 := buildMsg(s.T(), txConfig, []byte(`12`), [][]byte{secret2, secret6}, []uint64{2, 1})
105+
type testTx struct {
106+
tx sdk.Tx
107+
priority int64
108+
bz []byte
109+
size int
110+
}
154111

155-
err = mp2.Insert(ctx1, tx5)
156-
s.Require().NoError(err)
157-
err = mp2.Insert(ctx1, tx6)
158-
s.Require().NoError(err)
159-
err = mp2.Insert(ctx2, tx7)
160-
s.Require().NoError(err)
161-
err = mp2.Insert(ctx2, tx8)
162-
s.Require().NoError(err)
163-
err = mp2.Insert(ctx2, tx9)
164-
s.Require().NoError(err)
165-
txBz5, err := txConfig.TxEncoder()(tx5)
166-
s.Require().NoError(err)
167-
txBz6, err := txConfig.TxEncoder()(tx6)
168-
s.Require().NoError(err)
169-
txBz7, err := txConfig.TxEncoder()(tx7)
170-
s.Require().NoError(err)
171-
txBz8, err := txConfig.TxEncoder()(tx8)
172-
s.Require().NoError(err)
173-
txBz9, err := txConfig.TxEncoder()(tx9)
174-
s.Require().NoError(err)
175-
app.EXPECT().PrepareProposalVerifyTx(tx5).Return(txBz5, nil).AnyTimes()
176-
app.EXPECT().PrepareProposalVerifyTx(tx6).Return(txBz6, nil).AnyTimes()
177-
app.EXPECT().PrepareProposalVerifyTx(tx7).Return(txBz7, nil).AnyTimes()
178-
app.EXPECT().PrepareProposalVerifyTx(tx8).Return(txBz8, nil).AnyTimes()
179-
app.EXPECT().PrepareProposalVerifyTx(tx9).Return(txBz9, nil).AnyTimes()
180-
txDataSize5 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz5}))
181-
txDataSize6 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz6}))
182-
txDataSize7 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz7}))
183-
txDataSize8 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz8}))
184-
txDataSize9 := int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{txBz9}))
185-
s.Require().Equal(txDataSize5, 195)
186-
s.Require().Equal(txDataSize6, 205)
187-
s.Require().Equal(txDataSize7, 196)
188-
s.Require().Equal(txDataSize8, 196)
189-
s.Require().Equal(txDataSize9, 196)
112+
testTxs := []testTx{
113+
// test 1
114+
{tx: buildMsg(s.T(), txConfig, []byte(`0`), [][]byte{secret1}, []uint64{1}), priority: 10},
115+
{tx: buildMsg(s.T(), txConfig, []byte(`12345678910`), [][]byte{secret1}, []uint64{2}), priority: 10},
116+
{tx: buildMsg(s.T(), txConfig, []byte(`22`), [][]byte{secret1}, []uint64{3}), priority: 10},
117+
{tx: buildMsg(s.T(), txConfig, []byte(`32`), [][]byte{secret2}, []uint64{1}), priority: 8},
118+
// test 2
119+
{tx: buildMsg(s.T(), txConfig, []byte(`4`), [][]byte{secret1, secret2}, []uint64{3, 3}), priority: 10},
120+
{tx: buildMsg(s.T(), txConfig, []byte(`52345678910`), [][]byte{secret1, secret3}, []uint64{4, 3}), priority: 10},
121+
{tx: buildMsg(s.T(), txConfig, []byte(`62`), [][]byte{secret1, secret4}, []uint64{5, 3}), priority: 8},
122+
{tx: buildMsg(s.T(), txConfig, []byte(`72`), [][]byte{secret3, secret5}, []uint64{4, 3}), priority: 8},
123+
{tx: buildMsg(s.T(), txConfig, []byte(`82`), [][]byte{secret2, secret6}, []uint64{4, 3}), priority: 8},
124+
// test 3
125+
{tx: buildMsg(s.T(), txConfig, []byte(`9`), [][]byte{secret3, secret4}, []uint64{3, 3}), priority: 10},
126+
{tx: buildMsg(s.T(), txConfig, []byte(`1052345678910`), [][]byte{secret1, secret2}, []uint64{4, 4}), priority: 8},
127+
{tx: buildMsg(s.T(), txConfig, []byte(`11`), [][]byte{secret1, secret2}, []uint64{5, 5}), priority: 8},
128+
// test 4
129+
{tx: buildMsg(s.T(), txConfig, []byte(`1252345678910`), [][]byte{secret1}, []uint64{3}), priority: 10},
130+
{tx: buildMsg(s.T(), txConfig, []byte(`13`), [][]byte{secret1}, []uint64{5}), priority: 10},
131+
{tx: buildMsg(s.T(), txConfig, []byte(`14`), [][]byte{secret1}, []uint64{6}), priority: 8},
132+
}
190133

191-
mapTxs := map[string]string{
192-
string(txBz1): "1",
193-
string(txBz2): "2",
194-
string(txBz3): "3",
195-
string(txBz4): "4",
196-
string(txBz5): "5",
197-
string(txBz6): "6",
198-
string(txBz7): "7",
199-
string(txBz8): "8",
200-
string(txBz9): "9",
134+
for i := range testTxs {
135+
bz, err := txConfig.TxEncoder()(testTxs[i].tx)
136+
s.Require().NoError(err)
137+
testTxs[i].bz = bz
138+
testTxs[i].size = int(cmttypes.ComputeProtoSizeForTxs([]cmttypes.Tx{bz}))
201139
}
140+
141+
s.Require().Equal(testTxs[0].size, 111)
142+
s.Require().Equal(testTxs[1].size, 121)
143+
s.Require().Equal(testTxs[2].size, 112)
144+
s.Require().Equal(testTxs[3].size, 112)
145+
s.Require().Equal(testTxs[4].size, 195)
146+
s.Require().Equal(testTxs[5].size, 205)
147+
s.Require().Equal(testTxs[6].size, 196)
148+
s.Require().Equal(testTxs[7].size, 196)
149+
s.Require().Equal(testTxs[8].size, 196)
150+
202151
testCases := map[string]struct {
203152
ctx sdk.Context
153+
txInputs []testTx
204154
req abci.RequestPrepareProposal
205155
handler sdk.PrepareProposalHandler
206-
expectedTxs [][]byte
156+
expectedTxs []int
207157
}{
208158
"skip same-sender non-sequential sequence and then add others txs": {
209-
ctx: s.ctx,
159+
ctx: s.ctx,
160+
txInputs: []testTx{testTxs[0], testTxs[1], testTxs[2], testTxs[3]},
210161
req: abci.RequestPrepareProposal{
211-
Txs: [][]byte{txBz1, txBz2, txBz3, txBz4},
212162
MaxTxBytes: 111 + 112,
213163
},
214-
handler: handler1,
215-
expectedTxs: [][]byte{txBz1, txBz4},
164+
expectedTxs: []int{0, 3},
216165
},
217166
"skip multi-signers msg non-sequential sequence": {
218-
ctx: s.ctx,
167+
ctx: s.ctx,
168+
txInputs: []testTx{testTxs[4], testTxs[5], testTxs[6], testTxs[7], testTxs[8]},
169+
req: abci.RequestPrepareProposal{
170+
MaxTxBytes: 195 + 196,
171+
},
172+
expectedTxs: []int{4, 8},
173+
},
174+
"only the first tx is added": {
175+
// Because tx 10 is valid, tx 11 can't be valid as they have higher sequence numbers.
176+
ctx: s.ctx,
177+
txInputs: []testTx{testTxs[9], testTxs[10], testTxs[11]},
219178
req: abci.RequestPrepareProposal{
220-
Txs: [][]byte{txBz5, txBz6, txBz7, txBz8, txBz9},
221179
MaxTxBytes: 195 + 196,
222180
},
223-
handler: handler2,
224-
expectedTxs: [][]byte{txBz5, txBz9},
181+
expectedTxs: []int{9},
182+
},
183+
"no txs added": {
184+
// Becasuse the first tx was deemed valid but too big, the next expected valid sequence is tx[0].seq (3), so
185+
// the rest of the txs fail because they have a seq of 4.
186+
ctx: s.ctx,
187+
txInputs: []testTx{testTxs[12], testTxs[13], testTxs[14]},
188+
req: abci.RequestPrepareProposal{
189+
MaxTxBytes: 112,
190+
},
191+
expectedTxs: []int{},
225192
},
226193
}
227194

228195
for name, tc := range testCases {
229196
s.Run(name, func() {
230-
resp := tc.handler(tc.ctx, tc.req)
231-
s.Require().EqualValues(toHumanReadable(mapTxs, resp.Txs), toHumanReadable(mapTxs, tc.expectedTxs))
197+
ctrl := gomock.NewController(s.T())
198+
app := mock.NewMockProposalTxVerifier(ctrl)
199+
mp := mempool.NewPriorityMempool()
200+
ph := baseapp.NewDefaultProposalHandler(mp, app).PrepareProposalHandler()
201+
202+
for _, v := range tc.txInputs {
203+
app.EXPECT().PrepareProposalVerifyTx(v.tx).Return(v.bz, nil).AnyTimes()
204+
s.NoError(mp.Insert(s.ctx.WithPriority(v.priority), v.tx))
205+
tc.req.Txs = append(tc.req.Txs, v.bz)
206+
}
207+
208+
resp := ph(tc.ctx, tc.req)
209+
respTxIndexes := []int{}
210+
for _, tx := range resp.Txs {
211+
for i, v := range testTxs {
212+
if bytes.Equal(tx, v.bz) {
213+
respTxIndexes = append(respTxIndexes, i)
214+
}
215+
}
216+
}
217+
218+
s.Require().EqualValues(tc.expectedTxs, respTxIndexes)
232219
})
233220
}
234221
}
@@ -264,14 +251,6 @@ func buildMsg(t *testing.T, txConfig client.TxConfig, value []byte, secrets [][]
264251
return builder.GetTx()
265252
}
266253

267-
func toHumanReadable(mapTxs map[string]string, txs [][]byte) string {
268-
strs := []string{}
269-
for _, v := range txs {
270-
strs = append(strs, mapTxs[string(v)])
271-
}
272-
return strings.Join(strs, ",")
273-
}
274-
275254
func setTxSignatureWithSecret(t *testing.T, builder client.TxBuilder, signatures ...signingtypes.SignatureV2) {
276255
t.Helper()
277256
err := builder.SetSignatures(

0 commit comments

Comments
 (0)
⚡