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.

Sign up for GitHub

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

Add new MinTxsInBlock consensus param #252

Merged
merged 3 commits into from
Jan 6, 2025
Merged

Add new MinTxsInBlock consensus param #252

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4a7d97a
Add new MinTxsInBlock consensus param
codchen Dec 27, 2024
e8d9f32
test
codchen Jan 2, 2025
bea5c8f
exclude nonzero gas tx
codchen Jan 6, 2025
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
539 changes: 288 additions & 251 deletions abci/types/types.pb.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion internal/consensus/mempool_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ func TestMempoolRmBadTx(t *testing.T) {

// check for the tx
for {
txs := assertMempool(t, cs.txNotifier).ReapMaxBytesMaxGas(int64(len(txBytes)), -1)
txs := assertMempool(t, cs.txNotifier).ReapMaxBytesMaxGas(int64(len(txBytes)), -1, 0)
if len(txs) == 0 {
emptyMempoolCh <- struct{}{}
return
Expand Down
6 changes: 3 additions & 3 deletions internal/consensus/replay_stubs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ func (emptyMempool) Size() int { return 0 }
func (emptyMempool) CheckTx(context.Context, types.Tx, func(*abci.ResponseCheckTx), mempool.TxInfo) error {
return nil
}
func (emptyMempool) RemoveTxByKey(txKey types.TxKey) error { return nil }
func (emptyMempool) ReapMaxBytesMaxGas(_, _ int64) types.Txs { return types.Txs{} }
func (emptyMempool) ReapMaxTxs(n int) types.Txs { return types.Txs{} }
func (emptyMempool) RemoveTxByKey(txKey types.TxKey) error { return nil }
func (emptyMempool) ReapMaxBytesMaxGas(_, _, _ int64) types.Txs { return types.Txs{} }
func (emptyMempool) ReapMaxTxs(n int) types.Txs { return types.Txs{} }
func (emptyMempool) Update(
_ context.Context,
_ int64,
Expand Down
4 changes: 2 additions & 2 deletions internal/mempool/mempool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@ func (txmp *TxMempool) Flush() {
// NOTE:
// - Transactions returned are not removed from the mempool transaction
// store or indexes.
func (txmp *TxMempool) ReapMaxBytesMaxGas(maxBytes, maxGas int64) types.Txs {
func (txmp *TxMempool) ReapMaxBytesMaxGas(maxBytes, maxGas, minTxsInBlock int64) types.Txs {
txmp.mtx.Lock()
defer txmp.mtx.Unlock()

Expand All @@ -453,7 +453,7 @@ func (txmp *TxMempool) ReapMaxBytesMaxGas(maxBytes, maxGas int64) types.Txs {
}
totalSize += size
gas := totalGas + wtx.gasWanted
if maxGas > -1 && gas > maxGas {
if len(txs) >= int(minTxsInBlock) && maxGas > -1 && gas > maxGas {
Copy link
Contributor

Choose a reason for hiding this comment

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

this might need to be also or size threshold is exceeded. I suspect the incremented values totalSize, totalGas should only be incremented if the transaction is appended?

Copy link
Contributor

Choose a reason for hiding this comment

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

here's kinda what i'm thinking:

txmp.priorityIndex.ForEachTx(func(wtx *WrappedTx) bool {
	size := types.ComputeProtoSizeForTxs([]types.Tx{wtx.tx})
	if len(txs) >= int(minTxsInBlock) {
		if thresholdExceeded(maxBytes, totalSize, size) {
			return false
		}
		if thresholdExceeded(maxGas, totalGas, wtx.gasWanted) {
			return false
		}
	}
	totalGas += wtx.gasWanted
	totalSize += size

	txs = append(txs, wtx.tx)
	return true
})

with helper:

func thresholdExceeded(threshold, totalSoFar, txValue int64) bool {
	return threshold > -1 && ((totalSoFar + txValue) > threshold)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we should exempt txs from exceeding size limit since a large size does mean we need to store more data on chain (i.e. it's not sizeWanted but always sizeUsed)

return false
}

Expand Down
6 changes: 3 additions & 3 deletions internal/mempool/mempool_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ func TestTxMempool_ReapMaxBytesMaxGas(t *testing.T) {
wg.Add(1)
go func() {
defer wg.Done()
reapedTxs := txmp.ReapMaxBytesMaxGas(-1, 50)
reapedTxs := txmp.ReapMaxBytesMaxGas(-1, 50, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

coudl we add unit tests for when this value isn't 0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added

Copy link
Contributor

Choose a reason for hiding this comment

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

it might be good to permutate these situations:

  • min txs reached / not reached
  • min size reached / not reached
  • min gas reached / not reached
  • with combinations of configurations (has gas/size/txs config)
    (chat gpt should be able to help make a scenario-based test for that)

ensurePrioritized(reapedTxs)
require.Equal(t, len(tTxs), txmp.Size())
require.Equal(t, int64(5690), txmp.SizeBytes())
Expand All @@ -395,7 +395,7 @@ func TestTxMempool_ReapMaxBytesMaxGas(t *testing.T) {
wg.Add(1)
go func() {
defer wg.Done()
reapedTxs := txmp.ReapMaxBytesMaxGas(1000, -1)
reapedTxs := txmp.ReapMaxBytesMaxGas(1000, -1, 0)
ensurePrioritized(reapedTxs)
require.Equal(t, len(tTxs), txmp.Size())
require.Equal(t, int64(5690), txmp.SizeBytes())
Expand All @@ -407,7 +407,7 @@ func TestTxMempool_ReapMaxBytesMaxGas(t *testing.T) {
wg.Add(1)
go func() {
defer wg.Done()
reapedTxs := txmp.ReapMaxBytesMaxGas(1500, 30)
reapedTxs := txmp.ReapMaxBytesMaxGas(1500, 30, 0)
ensurePrioritized(reapedTxs)
require.Equal(t, len(tTxs), txmp.Size())
require.Equal(t, int64(5690), txmp.SizeBytes())
Expand Down
2 changes: 1 addition & 1 deletion internal/mempool/mocks/mempool.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion internal/mempool/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ type Mempool interface {
//
// If both maxes are negative, there is no cap on the size of all returned
// transactions (~ all available transactions).
ReapMaxBytesMaxGas(maxBytes, maxGas int64) types.Txs
ReapMaxBytesMaxGas(maxBytes, maxGas, minTxsInBlock int64) types.Txs

// ReapMaxTxs reaps up to max transactions from the mempool. If max is
// negative, there is no cap on the size of all returned transactions
Expand Down
2 changes: 1 addition & 1 deletion internal/state/execution.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func (blockExec *BlockExecutor) CreateProposalBlock(
// Fetch a limited amount of valid txs
maxDataBytes := types.MaxDataBytes(maxBytes, evSize, state.Validators.Size())

txs := blockExec.mempool.ReapMaxBytesMaxGas(maxDataBytes, maxGas)
txs := blockExec.mempool.ReapMaxBytesMaxGas(maxDataBytes, maxGas, state.ConsensusParams.Block.MinTxsInBlock)
commit := lastExtCommit.ToCommit()
block := state.MakeBlock(height, txs, commit, evidence, proposerAddr)
rpp, err := blockExec.appClient.PrepareProposal(
Expand Down
2 changes: 2 additions & 0 deletions proto/tendermint/abci/types.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,8 @@ message BlockParams {
int64 max_bytes = 1;
// Note: must be greater or equal to -1
int64 max_gas = 2;
// Minimum txs to include in a block regardless of gas limit
int64 min_txs_in_block = 3;
}

message ConsensusParams {
Expand Down
138 changes: 89 additions & 49 deletions proto/tendermint/types/params.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions proto/tendermint/types/params.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ message BlockParams {
// Max gas per block.
// Note: must be greater or equal to -1
int64 max_gas = 2;
// Minimum txs to include in a block regardless of gas limit
int64 min_txs_in_block = 3;
}

// EvidenceParams determine how we handle evidence of malfeasance.
Expand Down
24 changes: 17 additions & 7 deletions types/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,9 @@ type HashedParams struct {
// BlockParams define limits on the block size and gas plus minimum time
// between blocks.
type BlockParams struct {
MaxBytes int64 `json:"max_bytes,string"`
MaxGas int64 `json:"max_gas,string"`
MaxBytes int64 `json:"max_bytes,string"`
MaxGas int64 `json:"max_gas,string"`
MinTxsInBlock int64 `json:"min_txs_in_block,string"`
}

// EvidenceParams determine how we handle evidence of malfeasance.
Expand Down Expand Up @@ -132,7 +133,8 @@ func DefaultBlockParams() BlockParams {
MaxBytes: 22020096, // 21MB
// Default, can be increased and tuned as needed
// match sei-devnet-3 and atlantic-2 current values
MaxGas: 100000000,
MaxGas: 100000000,
MinTxsInBlock: 0,
}
}

Expand Down Expand Up @@ -286,6 +288,11 @@ func (params ConsensusParams) ValidateConsensusParams() error {
params.Block.MaxGas)
}

if params.Block.MinTxsInBlock < 0 {
return fmt.Errorf("block.MinTxsInBlock must be positive. Got %d",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - "must be non-negative"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

params.Block.MinTxsInBlock)
}

if params.Evidence.MaxAgeNumBlocks <= 0 {
return fmt.Errorf("evidence.MaxAgeNumBlocks must be greater than 0. Got %d",
params.Evidence.MaxAgeNumBlocks)
Expand Down Expand Up @@ -423,6 +430,7 @@ func (params ConsensusParams) UpdateConsensusParams(params2 *tmproto.ConsensusPa
if params2.Block != nil {
res.Block.MaxBytes = params2.Block.MaxBytes
res.Block.MaxGas = params2.Block.MaxGas
res.Block.MinTxsInBlock = params2.Block.MinTxsInBlock
}
if params2.Evidence != nil {
res.Evidence.MaxAgeNumBlocks = params2.Evidence.MaxAgeNumBlocks
Expand Down Expand Up @@ -473,8 +481,9 @@ func (params ConsensusParams) UpdateConsensusParams(params2 *tmproto.ConsensusPa
func (params *ConsensusParams) ToProto() tmproto.ConsensusParams {
return tmproto.ConsensusParams{
Block: &tmproto.BlockParams{
MaxBytes: params.Block.MaxBytes,
MaxGas: params.Block.MaxGas,
MaxBytes: params.Block.MaxBytes,
MaxGas: params.Block.MaxGas,
MinTxsInBlock: params.Block.MinTxsInBlock,
},
Evidence: &tmproto.EvidenceParams{
MaxAgeNumBlocks: params.Evidence.MaxAgeNumBlocks,
Expand Down Expand Up @@ -509,8 +518,9 @@ func (params *ConsensusParams) ToProto() tmproto.ConsensusParams {
func ConsensusParamsFromProto(pbParams tmproto.ConsensusParams) ConsensusParams {
c := ConsensusParams{
Block: BlockParams{
MaxBytes: pbParams.Block.MaxBytes,
MaxGas: pbParams.Block.MaxGas,
MaxBytes: pbParams.Block.MaxBytes,
MaxGas: pbParams.Block.MaxGas,
MinTxsInBlock: pbParams.Block.MinTxsInBlock,
},
Evidence: EvidenceParams{
MaxAgeNumBlocks: pbParams.Evidence.MaxAgeNumBlocks,
Expand Down
Loading