diff --git a/.changelog/unreleased/breaking-changes/980-max-size-more-control.md b/.changelog/unreleased/breaking-changes/980-max-size-more-control.md new file mode 100644 index 00000000000..e4354e3cb05 --- /dev/null +++ b/.changelog/unreleased/breaking-changes/980-max-size-more-control.md @@ -0,0 +1,9 @@ +- `[mempool]` Application can now set `ConsensusParams.Block.MaxBytes` to -1 + to have visibility on all transactions in the + mempool at `PrepareProposal` time. + This means that the total size of transactions sent via `RequestPrepareProposal` + might exceed `RequestPrepareProposal.max_tx_bytes`. + If that is the case, the application MUST make sure that the total size of transactions + returned in `ResponsePrepareProposal.txs` does not exceed `RequestPrepareProposal.max_tx_bytes`, + otherwise CometBFT will panic. + ([\#980](https://github.com/cometbft/cometbft/issues/980)) \ No newline at end of file diff --git a/.changelog/unreleased/improvements/980-max-size-more-control.md b/.changelog/unreleased/improvements/980-max-size-more-control.md new file mode 100644 index 00000000000..e319779984c --- /dev/null +++ b/.changelog/unreleased/improvements/980-max-size-more-control.md @@ -0,0 +1,5 @@ +- `[mempool]` Application can now set `ConsensusParams.Block.MaxBytes` to -1 + to gain more control on the max size of transactions in a block. + It also allows the application to have visibility on all transactions in the + mempool at `PrepareProposal` time. + ([\#980](https://github.com/cometbft/cometbft/pull/980)) \ No newline at end of file diff --git a/consensus/state.go b/consensus/state.go index cae31f5be02..c79d5538fcd 100644 --- a/consensus/state.go +++ b/consensus/state.go @@ -1946,9 +1946,13 @@ func (cs *State) addProposalBlockPart(msg *BlockPartMessage, peerID p2p.ID) (add cs.metrics.DuplicateBlockPart.Add(1) } - if cs.ProposalBlockParts.ByteSize() > cs.state.ConsensusParams.Block.MaxBytes { + maxBytes := cs.state.ConsensusParams.Block.MaxBytes + if maxBytes == -1 { + maxBytes = int64(types.MaxBlockSizeBytes) + } + if cs.ProposalBlockParts.ByteSize() > maxBytes { return added, fmt.Errorf("total size of proposal block parts exceeds maximum block bytes (%d > %d)", - cs.ProposalBlockParts.ByteSize(), cs.state.ConsensusParams.Block.MaxBytes, + cs.ProposalBlockParts.ByteSize(), maxBytes, ) } if added && cs.ProposalBlockParts.IsComplete() { diff --git a/node/node_test.go b/node/node_test.go index 031ca6e8fe2..01d5f0f8fcc 100644 --- a/node/node_test.go +++ b/node/node_test.go @@ -264,10 +264,12 @@ func TestCreateProposalBlock(t *testing.T) { stateStore := sm.NewStore(stateDB, sm.StoreOptions{ DiscardABCIResponses: false, }) - maxBytes := 16384 - var partSize uint32 = 256 - maxEvidenceBytes := int64(maxBytes / 2) - state.ConsensusParams.Block.MaxBytes = int64(maxBytes) + var ( + partSize uint32 = 256 + maxBytes int64 = 16384 + ) + maxEvidenceBytes := maxBytes / 2 + state.ConsensusParams.Block.MaxBytes = maxBytes state.ConsensusParams.Evidence.MaxBytes = maxEvidenceBytes proposerAddr, _ := state.Validators.GetByIndex(0) @@ -305,7 +307,7 @@ func TestCreateProposalBlock(t *testing.T) { // fill the mempool with more txs // than can fit in a block txLength := 100 - for i := 0; i <= maxBytes/txLength; i++ { + for i := 0; i <= int(maxBytes)/txLength; i++ { tx := cmtrand.Bytes(txLength) err := mempool.CheckTx(tx, nil, mempl.TxInfo{}) assert.NoError(t, err) @@ -333,7 +335,7 @@ func TestCreateProposalBlock(t *testing.T) { // check that the part set does not exceed the maximum block size partSet, err := block.MakePartSet(partSize) require.NoError(t, err) - assert.Less(t, partSet.ByteSize(), int64(maxBytes)) + assert.Less(t, partSet.ByteSize(), maxBytes) partSetFromHeader := types.NewPartSetFromHeader(partSet.Header()) for partSetFromHeader.Count() < partSetFromHeader.Total() { diff --git a/spec/abci/abci++_app_requirements.md b/spec/abci/abci++_app_requirements.md index 6d545549313..d791c309d2e 100644 --- a/spec/abci/abci++_app_requirements.md +++ b/spec/abci/abci++_app_requirements.md @@ -61,7 +61,7 @@ Process *p*'s prepared proposal can differ in two different rounds where *p* is Full execution of blocks at `PrepareProposal` time stands on CometBFT's critical path. Thus, Requirement 1 ensures the Application or operator will set a value for `TimeoutPropose` such that the time it takes to fully execute blocks in `PrepareProposal` does not interfere with CometBFT's propose timer. -Note that the violation of Requirement 1 may lead to further rounds, but will not +Note that the violation of Requirement 1 may lead to further rounds, but will not compromise liveness because even though `TimeoutPropose` is used as the initial value for proposal timeouts, CometBFT will be dynamically adjust these timeouts such that they will eventually be enough for completing `PrepareProposal`. @@ -69,13 +69,14 @@ such that they will eventually be enough for completing `PrepareProposal`. * Requirement 2 [`PrepareProposal`, tx-size]: When *p*'s Application calls `ResponsePrepareProposal`, the total size in bytes of the transactions returned does not exceed `RequestPrepareProposal.max_tx_bytes`. -Busy blockchains might seek to maximize the amount of transactions included in each block. Under those conditions, -CometBFT might choose to increase the transactions passed to the Application via `RequestPrepareProposal.txs` -beyond the `RequestPrepareProposal.max_tx_bytes` limit. The idea is that, if the Application drops some of -those transactions, it can still return a transaction list whose byte size is as close to -`RequestPrepareProposal.max_tx_bytes` as possible. Thus, Requirement 2 ensures that the size in bytes of the -transaction list returned by the application will never cause the resulting block to go beyond its byte size -limit. +Busy blockchains might seek to gain full visibility into transactions in CometBFT's mempool, +rather than having visibility only on *a* subset of those transactions that fit in a block. +The application can do so by setting `ConsensusParams.Block.MaxBytes` to -1. +This instructs CometBFT (a) to enforce the maximum possible value for `MaxBytes` (100 MB) at CometBFT level, +and (b) to provide *all* transactions in the mempool when calling `RequestPrepareProposal`. +Under these settings, the aggregated size of all transactions may exceed `RequestPrepareProposal.max_tx_bytes`. +Hence, Requirement 2 ensures that the size in bytes of the transaction list returned by the application will never +cause the resulting block to go beyond its byte size limit. * Requirement 3 [`PrepareProposal`, `ProcessProposal`, coherence]: For any two correct processes *p* and *q*, if *q*'s CometBFT calls `RequestProcessProposal` on *up*, @@ -600,7 +601,19 @@ This is enforced by the consensus algorithm. This implies a maximum transaction size that is this `MaxBytes`, less the expected size of the header, the validator set, and any included evidence in the block. -Must have `0 < MaxBytes < 100 MB`. +If the Application wants full control over the size of blocks, +it can do so by enforcing a byte limit set up at the Application level. +This Application-internal limit is used by `PrepareProposal` to bound the total size +of transactions it returns, and by `ProcessProposal` to reject any received block +whose total transaction size is bigger than the enforced limit. +In such case, the Application MAY set `MaxBytes` to -1. + +If the Application sets value -1, consensus will: + +- consider that the actual value to enforce is 100 MB +- will provide *all* transactions in the mempool in calls to `PrepareProposal` + +Must have `MaxBytes == -1` OR `0 < MaxBytes <= 100 MB`. ##### BlockParams.MaxGas diff --git a/spec/abci/abci++_methods.md b/spec/abci/abci++_methods.md index a50f04653c0..5271602b64f 100644 --- a/spec/abci/abci++_methods.md +++ b/spec/abci/abci++_methods.md @@ -44,7 +44,7 @@ title: Methods | version | string | The application software semantic version | 2 | | app_version | uint64 | The application protocol version | 3 | | last_block_height | int64 | Latest height for which the app persisted its state | 4 | - | last_block_app_hash | bytes | Latest AppHash returned by `Commit` | 5 | + | last_block_app_hash | bytes | Latest AppHash returned by `FinalizeBlock` | 5 | * **Usage**: * Return information about the application state. @@ -355,12 +355,16 @@ title: Methods traceability, it is its responsibility's to support it. For instance, the Application could attach to a transformed transaction a list with the hashes of the transactions it derives from. - * CometBFT MAY include a list of transactions in `RequestPrepareProposal.txs` whose total - size in bytes exceeds `RequestPrepareProposal.max_tx_bytes`. + * The Application MAY configure CometBFT to include a list of transactions in `RequestPrepareProposal.txs` + whose total size in bytes exceeds `RequestPrepareProposal.max_tx_bytes`. + If the Application sets `ConsensusParams.Block.MaxBytes` to -1, CometBFT + will include _all_ transactions currently in the mempool in `RequestPrepareProposal.txs`, + which may not fit in `RequestPrepareProposal.max_tx_bytes`. Therefore, if the size of `RequestPrepareProposal.txs` is greater than `RequestPrepareProposal.max_tx_bytes`, the Application MUST remove transactions to ensure that the `RequestPrepareProposal.max_tx_bytes` limit is respected by those transactions - returned in `ResponsePrepareProposal.txs` . + returned in `ResponsePrepareProposal.txs`. + This is specified in [Requirement 2](./abci%2B%2B_app_requirements.md). * As a result of executing the prepared proposal, the Application may produce block events or transaction events. The Application must keep those events until a block is decided and then pass them on to CometBFT via `ResponseFinalizeBlock`. diff --git a/spec/consensus/creating-proposal.md b/spec/consensus/creating-proposal.md index cb43c8ebb41..feeb8e59666 100644 --- a/spec/consensus/creating-proposal.md +++ b/spec/consensus/creating-proposal.md @@ -4,40 +4,58 @@ order: 2 # Creating a proposal A block consists of a header, transactions, votes (the commit), -and a list of evidence of malfeasance (ie. signing conflicting votes). +and a list of evidence of malfeasance (eg. signing conflicting votes). -We include no more than 1/10th of the maximum block size -(`ConsensusParams.Block.MaxBytes`) of evidence with each block. +Outstanding evidence items get priority over outstanding transactions in the mempool. +All in all, the block MUST NOT exceed `ConsensusParams.Block.MaxBytes`, +or 100MB if `ConsensusParams.Block.MaxBytes == -1`. ## Reaping transactions from the mempool When we reap transactions from the mempool, we calculate maximum data size by subtracting maximum header size (`MaxHeaderBytes`), the maximum -amino overhead for a block (`MaxAminoOverheadForBlock`), the size of +protobuf overhead for a block (`MaxOverheadForBlock`), the size of the last commit (if present) and evidence (if present). While reaping -we account for amino overhead for each transaction. +we account for protobuf overhead for each transaction. ```go -func MaxDataBytes(maxBytes int64, valsCount, evidenceCount int) int64 { - return maxBytes - +func MaxDataBytes(maxBytes, evidenceBytes int64, valsCount int) int64 { + return maxBytes - MaxOverheadForBlock - MaxHeaderBytes - - int64(valsCount)*MaxVoteBytes - - int64(evidenceCount)*MaxEvidenceBytes + MaxCommitBytes(valsCount) - + evidenceBytes } ``` +If `ConsensusParams.Block.MaxBytes == -1`, we reap *all* outstanding transactions from the mempool + +## Preparing the proposal + +Once the transactions have been reaped from the mempool according to the rules described above, +CometBFT calls `PrepareProposal` to the application with the transaction list that has just been reaped. +As part of this call the application can remove, add, or reorder transactions in the transaction list. + +The `RequestPrepareProposal` contains two important fields: + +* `MaxTxBytes`, which contains the value returned by `MaxDataBytes` described above. + The application MUST NOT return a list of transactions whose size exceeds this number. +* `Txs`, which contains the list of reaped transactions. + +For more details on `PrepareProposal`, please see the +[relevant part of the spec](../abci/abci%2B%2B_methods.md#prepareproposal) + ## Validating transactions in the mempool -Before we accept a transaction in the mempool, we check if it's size is no more +Before we accept a transaction in the mempool, we check if its size is no more than {MaxDataSize}. {MaxDataSize} is calculated using the same formula as -above, except we subtract the max number of evidence, {MaxNum} by the maximum size of evidence +above, except we assume there is no evidence. ```go -func MaxDataBytesUnknownEvidence(maxBytes int64, valsCount int) int64 { - return maxBytes - - MaxOverheadForBlock - - MaxHeaderBytes - - (maxNumEvidence * MaxEvidenceBytes) +func MaxDataBytesNoEvidence(maxBytes int64, valsCount int) int64 { + return maxBytes - + MaxOverheadForBlock - + MaxHeaderBytes - + MaxCommitBytes(valsCount) } ``` diff --git a/spec/core/state.md b/spec/core/state.md index 1dbb020001c..c19d0096ca1 100644 --- a/spec/core/state.md +++ b/spec/core/state.md @@ -104,6 +104,12 @@ The total size of a block is limited in bytes by the `ConsensusParams.Block.MaxB Proposed blocks must be less than this size, and will be considered invalid otherwise. +The Application may set `ConsensusParams.Block.MaxBytes` to -1. +In that case, the actual block limit is set to 100 MB, +and CometBFT will provide all transactions in the mempool as part of `PrepareProposal`. +The application has to be careful to return a list of transactions in `ResponsePrepareProposal` +whose size is less than or equal to `RequestPrepareProposal.MaxTxBytes`. + Blocks should additionally be limited by the amount of "gas" consumed by the transactions in the block, though this is not yet implemented. diff --git a/state/execution.go b/state/execution.go index b9f7ed24ed9..f9c2f58a069 100644 --- a/state/execution.go +++ b/state/execution.go @@ -94,7 +94,7 @@ func (blockExec *BlockExecutor) SetEventBus(eventBus types.BlockEventPublisher) // CreateProposalBlock calls state.MakeBlock with evidence from the evpool // and txs from the mempool. The max bytes must be big enough to fit the commit. -// Up to 1/10th of the block space is allocated for maximum sized evidence. +// The block space is first allocated to outstanding evidence. // The rest is given to txs, up to the max gas. // // Contract: application will not return more bytes than are sent over the wire. @@ -107,14 +107,23 @@ func (blockExec *BlockExecutor) CreateProposalBlock( ) (*types.Block, error) { maxBytes := state.ConsensusParams.Block.MaxBytes + emptyMaxBytes := maxBytes == -1 + if emptyMaxBytes { + maxBytes = int64(types.MaxBlockSizeBytes) + } + maxGas := state.ConsensusParams.Block.MaxGas evidence, evSize := blockExec.evpool.PendingEvidence(state.ConsensusParams.Evidence.MaxBytes) // Fetch a limited amount of valid txs maxDataBytes := types.MaxDataBytes(maxBytes, evSize, state.Validators.Size()) + maxReapBytes := maxDataBytes + if emptyMaxBytes { + maxReapBytes = -1 + } - txs := blockExec.mempool.ReapMaxBytesMaxGas(maxDataBytes, maxGas) + txs := blockExec.mempool.ReapMaxBytesMaxGas(maxReapBytes, maxGas) commit := lastExtCommit.ToCommit() block := state.MakeBlock(height, txs, commit, evidence, proposerAddr) rpp, err := blockExec.proxyApp.PrepareProposal( diff --git a/state/tx_filter.go b/state/tx_filter.go index 8be843ee905..337a7a5e7fd 100644 --- a/state/tx_filter.go +++ b/state/tx_filter.go @@ -8,8 +8,12 @@ import ( // TxPreCheck returns a function to filter transactions before processing. // The function limits the size of a transaction to the block's maximum data size. func TxPreCheck(state State) mempl.PreCheckFunc { + maxBytes := state.ConsensusParams.Block.MaxBytes + if maxBytes == -1 { + maxBytes = int64(types.MaxBlockSizeBytes) + } maxDataBytes := types.MaxDataBytesNoEvidence( - state.ConsensusParams.Block.MaxBytes, + maxBytes, state.Validators.Size(), ) return mempl.PreCheckMaxBytes(maxDataBytes) diff --git a/types/block.go b/types/block.go index f26fe666438..82d0fa4d989 100644 --- a/types/block.go +++ b/types/block.go @@ -294,8 +294,7 @@ func MaxDataBytes(maxBytes, evidenceBytes int64, valsCount int) int64 { } // MaxDataBytesNoEvidence returns the maximum size of block's data when -// evidence count is unknown. MaxEvidencePerBlock will be used for the size -// of evidence. +// evidence count is unknown (will be assumed to be 0). // // XXX: Panics on negative result. func MaxDataBytesNoEvidence(maxBytes int64, valsCount int) int64 { @@ -306,7 +305,7 @@ func MaxDataBytesNoEvidence(maxBytes int64, valsCount int) int64 { if maxDataBytes < 0 { panic(fmt.Sprintf( - "Negative MaxDataBytesUnknownEvidence. Block.MaxBytes=%d is too small to accommodate header&lastCommit&evidence=%d", + "Negative MaxDataBytesNoEvidence. Block.MaxBytes=%d is too small to accommodate header&lastCommit&evidence=%d", maxBytes, -(maxDataBytes - maxBytes), )) diff --git a/types/params.go b/types/params.go index d12503ae0e0..a72429fb9a2 100644 --- a/types/params.go +++ b/types/params.go @@ -143,8 +143,12 @@ func IsValidPubkeyType(params ValidatorParams, pubkeyType string) bool { // Validate validates the ConsensusParams to ensure all values are within their // allowed limits, and returns an error if they are not. func (params ConsensusParams) ValidateBasic() error { - if params.Block.MaxBytes <= 0 { - return fmt.Errorf("block.MaxBytes must be greater than 0. Got %d", + if params.Block.MaxBytes == 0 { + return fmt.Errorf("block.MaxBytes cannot be 0") + } + if params.Block.MaxBytes < -1 { + return fmt.Errorf("block.MaxBytes must be -1 or greater than 0. Got %d", + params.Block.MaxBytes) } if params.Block.MaxBytes > MaxBlockSizeBytes { @@ -167,7 +171,11 @@ func (params ConsensusParams) ValidateBasic() error { params.Evidence.MaxAgeDuration) } - if params.Evidence.MaxBytes > params.Block.MaxBytes { + maxBytes := params.Block.MaxBytes + if maxBytes == -1 { + maxBytes = int64(MaxBlockSizeBytes) + } + if params.Evidence.MaxBytes > maxBytes { return fmt.Errorf("evidence.MaxBytesEvidence is greater than upper bound, %d > %d", params.Evidence.MaxBytes, params.Block.MaxBytes) } diff --git a/types/params_test.go b/types/params_test.go index 4311945d558..ac4305f4835 100644 --- a/types/params_test.go +++ b/types/params_test.go @@ -39,6 +39,8 @@ func TestConsensusParamsValidation(t *testing.T) { 11: {makeParams(1, 0, 2, 0, []string{}, 0), false}, // test invalid pubkey type provided 12: {makeParams(1, 0, 2, 0, []string{"potatoes make good pubkeys"}, 0), false}, + 13: {makeParams(-1, 0, 2, 0, valEd25519, 0), true}, + 14: {makeParams(-2, 0, 2, 0, valEd25519, 0), false}, } for i, tc := range testCases { if tc.valid {