feat: loop through all txs before returning the tx set to consensus #1113
Conversation
Yeah good idea. I'll add a test for that |
| // smaller low priority tx with different sender | ||
| smallTx := []byte(fmt.Sprintf("sender-2-1=%X=1", smallPrefix)) | ||
| require.NoError(t, txmp.CheckTx(bigTx, nil, mempool.TxInfo{SenderID: 1})) | ||
| require.NoError(t, txmp.CheckTx(smallTx, nil, mempool.TxInfo{SenderID: 1})) |
There was a problem hiding this comment.
I don't think it impacts the results of the test but is this line supposed to have a different SenderID?
| require.NoError(t, txmp.CheckTx(smallTx, nil, mempool.TxInfo{SenderID: 1})) | |
| require.NoError(t, txmp.CheckTx(smallTx, nil, mempool.TxInfo{SenderID: 2})) |
There was a problem hiding this comment.
no the sender can be the same. It's okay for the txs to be sent by the same peer. You might be confusing it with sender in CheckTx
There was a problem hiding this comment.
Sorry I don't follow. Is this line referencing the sender in CheckTx?
| // smaller low priority tx with different sender | ||
| smallTx := []byte(fmt.Sprintf("sender-2-1=%X=1", smallPrefix)) | ||
| require.NoError(t, txmp.CheckTx(bigTx, nil, mempool.TxInfo{SenderID: 1})) | ||
| require.NoError(t, txmp.CheckTx(smallTx, nil, mempool.TxInfo{SenderID: 1})) |
There was a problem hiding this comment.
| require.NoError(t, txmp.CheckTx(smallTx, nil, mempool.TxInfo{SenderID: 1})) | |
| require.NoError(t, txmp.CheckTx(smallTx, nil, mempool.TxInfo{SenderID: 2})) |
…1113) ## Description This modifies the behavior of the `ReapMaxBytesMaxGas` method called by consensus. Prior we would return the tx set the moment the first tx was greater than the limit. Now instead of breaking we continue through all txs. This means that a large tx doesn't block smaller less priority transactions from getting through. (cherry picked from commit 793ece9)
|
Apparently at some point the behavior of the CosmosSDK changed again and it now allows transactions transactions with ascending sequence numbers into the mempool. This behavior change is going to result in annoying behavior where the block proposer will now start proposing sequences out of order under congestion. |
|
I don't think this PR is a good idea. |
thanks this is definitely something I'll look into.
fwiw since all PFB transactions included in a block must be valid, proposers filter invalid txs out. Which results in a perhaps equally unwanted behavior, where if the txs' are out of order, the higher sequence will be dropped that block and the lower sequence will be included. However the lower sequenced tx will be included. |
Description
This modifies the behavior of the
ReapMaxBytesMaxGasmethod called by consensus. Prior we would return the tx set the moment the first tx was greater than the limit. Now instead of breaking we continue through all txs. This means that a large tx doesn't block smaller less priority transactions from getting through.PR checklist
.changelog(we useunclog to manage our changelog)
docs/orspec/) and code comments