fix: skip same-sender non-sequential sequence and then add others txs #19119
Conversation
WalkthroughThe overall change involves modifications to the transaction selection process in the Cosmos SDK, specifically addressing a bug in the Changes
Assessment against linked issues
The provided code changes address the key objectives specified in the linked issue #19097. The Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
baseapp/abci_utils.go
Outdated
| } | ||
|
|
||
| iterator := h.mempool.Select(ctx, req.Txs) | ||
| skipSenders := make(map[string]any) | ||
| var selectedTxsNums int | ||
| for iterator != nil { | ||
| memTx := iterator.Tx() | ||
|
|
||
| sender := iterator.Sender() | ||
| if _, ok := skipSenders[sender]; ok { | ||
| 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 | ||
| // valid. But some mempool implementations may insert invalid txs, so we |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [230-258]
The logic introduced in the PrepareProposalHandler method to skip transactions from senders with non-sequential transactions is consistent with the PR objectives. The use of the skipSenders map to track senders whose transactions should be skipped and the selectedTxsNums variable to count the number of selected transactions are appropriate for the intended functionality. However, there is a potential issue with the logic that determines when to skip a sender's transactions.
- if txsLen == selectedTxsNums {
+ if txsLen <= selectedTxsNums {
skipSenders[sender] = nil
}The condition should check if the length of the selected transactions has not increased (<=) after attempting to add a new transaction, indicating that the transaction was not added and the sender should be skipped.
baseapp/abci_utils_test.go
Outdated
| 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) | ||
| mp := mempool.NewPriorityMempool( | ||
| mempool.PriorityNonceMempoolConfig[int64]{ | ||
| TxPriority: mempool.NewDefaultTxPriority(), | ||
| MaxTx: 0, | ||
| SignerExtractor: mempool.NewDefaultSignerExtractionAdapter(), | ||
| }, | ||
| ) | ||
| ph := baseapp.NewDefaultProposalHandler(mp, app) | ||
| handler := ph.PrepareProposalHandler() | ||
| var ( | ||
| secret1 = []byte("secret1") | ||
| secret2 = []byte("secret2") | ||
| tx1 = buildMsg(s.T(), txConfig, []byte(`1`), secret1, 1) | ||
| ctx1 = s.ctx.WithPriority(10) | ||
| tx2 = buildMsg(s.T(), txConfig, []byte(`12345678910`), secret1, 2) | ||
| tx3 = buildMsg(s.T(), txConfig, []byte(`12`), secret1, 3) | ||
|
|
||
| ctx2 = s.ctx.WithPriority(8) | ||
| tx4 = buildMsg(s.T(), txConfig, []byte(`12`), secret2, 1) | ||
| ) | ||
| mp.Insert(ctx1, tx1) | ||
| mp.Insert(ctx1, tx2) | ||
| mp.Insert(ctx1, tx3) | ||
| mp.Insert(ctx2, tx4) | ||
|
|
||
| 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) | ||
|
|
||
| testCases := map[string]struct { | ||
| ctx sdk.Context | ||
| req *abci.RequestPrepareProposal | ||
| 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 + 1, | ||
| }, | ||
| expectedTxs: [][]byte{txBz1, txBz4}, | ||
| }, | ||
| } | ||
|
|
||
| for name, tc := range testCases { | ||
| s.Run(name, func() { | ||
| resp, err := handler(tc.ctx, tc.req) | ||
| s.Require().NoError(err) | ||
| s.Require().EqualValues(resp.Txs, tc.expectedTxs) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func buildMsg(t *testing.T, txConfig client.TxConfig, value, secret []byte, nonce uint64) sdk.Tx { | ||
| builder := txConfig.NewTxBuilder() | ||
| builder.SetMsgs( | ||
| &baseapptestutil.MsgKeyValue{Value: value}, | ||
| ) | ||
| setTxSignatureWithSecret(t, builder, nonce, secret) | ||
| return builder.GetTx() | ||
| } |
There was a problem hiding this comment.
The new test TestDefaultProposalHandler_PriorityNonceMempoolTxSelection is well-structured and covers the scenario where transactions from the same sender with non-sequential nonce values are correctly skipped. The test ensures that the proposal handler selects and prepares transactions based on priority and nonce as expected. The use of the buildMsg helper function simplifies the creation of test transactions.
However, there is a minor issue with the expectedTxs slice in the test case. The MaxTxBytes is set to 111 + 112 + 1, which seems to be intended to allow only two transactions (txBz1 and txBz4) based on their data sizes. But the + 1 is unnecessary and could be misleading since the data sizes already account for the full size of the transactions.
- MaxTxBytes: 111 + 112 + 1,
+ MaxTxBytes: 111 + 112,This change would make the intention clearer and avoid any confusion about the calculation of MaxTxBytes.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| 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) | |
| mp := mempool.NewPriorityMempool( | |
| mempool.PriorityNonceMempoolConfig[int64]{ | |
| TxPriority: mempool.NewDefaultTxPriority(), | |
| MaxTx: 0, | |
| SignerExtractor: mempool.NewDefaultSignerExtractionAdapter(), | |
| }, | |
| ) | |
| ph := baseapp.NewDefaultProposalHandler(mp, app) | |
| handler := ph.PrepareProposalHandler() | |
| var ( | |
| secret1 = []byte("secret1") | |
| secret2 = []byte("secret2") | |
| tx1 = buildMsg(s.T(), txConfig, []byte(`1`), secret1, 1) | |
| ctx1 = s.ctx.WithPriority(10) | |
| tx2 = buildMsg(s.T(), txConfig, []byte(`12345678910`), secret1, 2) | |
| tx3 = buildMsg(s.T(), txConfig, []byte(`12`), secret1, 3) | |
| ctx2 = s.ctx.WithPriority(8) | |
| tx4 = buildMsg(s.T(), txConfig, []byte(`12`), secret2, 1) | |
| ) | |
| mp.Insert(ctx1, tx1) | |
| mp.Insert(ctx1, tx2) | |
| mp.Insert(ctx1, tx3) | |
| mp.Insert(ctx2, tx4) | |
| 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) | |
| testCases := map[string]struct { | |
| ctx sdk.Context | |
| req *abci.RequestPrepareProposal | |
| 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 + 1, | |
| }, | |
| expectedTxs: [][]byte{txBz1, txBz4}, | |
| }, | |
| } | |
| for name, tc := range testCases { | |
| s.Run(name, func() { | |
| resp, err := handler(tc.ctx, tc.req) | |
| s.Require().NoError(err) | |
| s.Require().EqualValues(resp.Txs, tc.expectedTxs) | |
| }) | |
| } | |
| } | |
| func buildMsg(t *testing.T, txConfig client.TxConfig, value, secret []byte, nonce uint64) sdk.Tx { | |
| builder := txConfig.NewTxBuilder() | |
| builder.SetMsgs( | |
| &baseapptestutil.MsgKeyValue{Value: value}, | |
| ) | |
| setTxSignatureWithSecret(t, builder, nonce, secret) | |
| return builder.GetTx() | |
| } | |
| "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, | |
| }, | |
| expectedTxs: [][]byte{txBz1, txBz4}, | |
| }, |
ZiHengLee
force-pushed
the
lee/tx_default_selector_2
branch
2 times, most recently
from
bf1b67d to
9ae16f5
Compare
|
Hi @ZiHengLee, please update the PR description with a changelog or what exactly this PR does and why. This would be helpful to provide context to reviewers. |
ZiHengLee
force-pushed
the
lee/tx_default_selector_2
branch
from
9ae16f5 to
15a6483
Compare
done |
ZiHengLee
force-pushed
the
lee/tx_default_selector_2
branch
from
15a6483 to
a5e705c
Compare
|
@facundomedica I believe you have a similar PR in the works. Should we compare and contract solutions? |
okay，but i think this pr more bette,our project is running normally so far |
|
|
The solution I was thinking about is pretty much the same as yours, with the main difference being that I encapsulated it into the tx selector (https://github.com/cosmos/cosmos-sdk/compare/facu/fix-samesender-nonseq?expand=1) |
i consider this solution before，but it'll leading to poor performance. |
ZiHengLee
force-pushed
the
lee/tx_default_selector_2
branch
from
a5e705c to
0801d17
Compare
how about my new solution，merging our two ideas（solved multi signdata） and better performance and i fund a bug in your new solution，muilti signdata，only one signer seq is true，u just add tx |
|
Let's continue the discussion on the latest PR (#19177), and if you need to make changes do it on that same one. Do not open another PR, thank you! 🙏 |
Description
Closes: #19097
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...