fix amino overhead computation for Tx (#2792)

* fix amino overhead computation for Tx:

- also count the fieldnum / typ3
- add method to compute overhead per Tx
- slightly clarify comment on MaxAminoOverheadForBlock
- add tests

* fix TestReapMaxBytesMaxGas according to amino overhead

* fix TestMempoolFilters according to amino overhead

* address review comments:

 - add a note about fieldNum = 1
 - add forgotten godoc comment

* fix and use sm.TxPreCheck

* fix test

* remove print statement

mempool/mempool.go

@@ -11,7 +11,6 @@ import (
 
 	"github.com/pkg/errors"
 
-	amino "github.com/tendermint/go-amino"
 	abci "github.com/tendermint/tendermint/abci/types"
 	cfg "github.com/tendermint/tendermint/config"
 	auto "github.com/tendermint/tendermint/libs/autofile"
@@ -88,8 +87,12 @@ func IsPreCheckError(err error) bool {
 func PreCheckAminoMaxBytes(maxBytes int64) PreCheckFunc {
 	return func(tx types.Tx) error {
 		// We have to account for the amino overhead in the tx size as well
-		aminoOverhead := amino.UvarintSize(uint64(len(tx)))
-		txSize := int64(len(tx) + aminoOverhead)
+		// NOTE: fieldNum = 1 as types.Block.Data contains Txs []Tx as first field.
+		// If this field order ever changes this needs to updated here accordingly.
+		// NOTE: if some []Tx are encoded without a parenting struct, the
+		// fieldNum is also equal to 1.
+		aminoOverhead := types.ComputeAminoOverhead(tx, 1)
+		txSize := int64(len(tx)) + aminoOverhead
 		if txSize > maxBytes {
 			return fmt.Errorf("Tx size (including amino overhead) is too big: %d, max: %d",
 				txSize, maxBytes)
@@ -482,7 +485,7 @@ func (mem *Mempool) ReapMaxBytesMaxGas(maxBytes, maxGas int64) types.Txs {
 	for e := mem.txs.Front(); e != nil; e = e.Next() {
 		memTx := e.Value.(*mempoolTx)
 		// Check total size requirement
-		aminoOverhead := int64(amino.UvarintSize(uint64(len(memTx.tx))))
+		aminoOverhead := types.ComputeAminoOverhead(memTx.tx, 1)
 		if maxBytes > -1 && totalBytes+int64(len(memTx.tx))+aminoOverhead > maxBytes {
 			return txs
 		}

mempool/mempool_test.go

@@ -107,11 +107,11 @@ func TestReapMaxBytesMaxGas(t *testing.T) {
 		{20, 0, -1, 0},
 		{20, 0, 10, 0},
 		{20, 10, 10, 0},
-		{20, 21, 10, 1},
-		{20, 210, -1, 10},
-		{20, 210, 5, 5},
-		{20, 210, 10, 10},
-		{20, 210, 15, 10},
+		{20, 22, 10, 1},
+		{20, 220, -1, 10},
+		{20, 220, 5, 5},
+		{20, 220, 10, 10},
+		{20, 220, 15, 10},
 		{20, 20000, -1, 20},
 		{20, 20000, 5, 5},
 		{20, 20000, 30, 20},
@@ -145,15 +145,15 @@ func TestMempoolFilters(t *testing.T) {
 		{10, nopPreFilter, nopPostFilter, 10},
 		{10, PreCheckAminoMaxBytes(10), nopPostFilter, 0},
 		{10, PreCheckAminoMaxBytes(20), nopPostFilter, 0},
-		{10, PreCheckAminoMaxBytes(21), nopPostFilter, 10},
+		{10, PreCheckAminoMaxBytes(22), nopPostFilter, 10},
 		{10, nopPreFilter, PostCheckMaxGas(-1), 10},
 		{10, nopPreFilter, PostCheckMaxGas(0), 0},
 		{10, nopPreFilter, PostCheckMaxGas(1), 10},
 		{10, nopPreFilter, PostCheckMaxGas(3000), 10},
 		{10, PreCheckAminoMaxBytes(10), PostCheckMaxGas(20), 0},
 		{10, PreCheckAminoMaxBytes(30), PostCheckMaxGas(20), 10},
-		{10, PreCheckAminoMaxBytes(21), PostCheckMaxGas(1), 10},
-		{10, PreCheckAminoMaxBytes(21), PostCheckMaxGas(0), 0},
+		{10, PreCheckAminoMaxBytes(22), PostCheckMaxGas(1), 10},
+		{10, PreCheckAminoMaxBytes(22), PostCheckMaxGas(0), 0},
 	}
 	for tcIndex, tt := range tests {
 		mempool.Update(1, emptyTxArr, tt.preFilter, tt.postFilter)

node/node.go

@@ -265,17 +265,8 @@ func NewNode(config *cfg.Config,
 		proxyApp.Mempool(),
 		state.LastBlockHeight,
 		mempl.WithMetrics(memplMetrics),
-		mempl.WithPreCheck(
-			mempl.PreCheckAminoMaxBytes(
-				types.MaxDataBytesUnknownEvidence(
-					state.ConsensusParams.BlockSize.MaxBytes,
-					state.Validators.Size(),
-				),
-			),
-		),
-		mempl.WithPostCheck(
-			mempl.PostCheckMaxGas(state.ConsensusParams.BlockSize.MaxGas),
-		),
+		mempl.WithPreCheck(sm.TxPreCheck(state)),
+		mempl.WithPostCheck(sm.TxPostCheck(state)),
 	)
 	mempoolLogger := logger.With("module", "mempool")
 	mempool.SetLogger(mempoolLogger)

state/execution.go

@@ -8,7 +8,6 @@ import (
 	dbm "github.com/tendermint/tendermint/libs/db"
 	"github.com/tendermint/tendermint/libs/fail"
 	"github.com/tendermint/tendermint/libs/log"
-	"github.com/tendermint/tendermint/mempool"
 	"github.com/tendermint/tendermint/proxy"
 	"github.com/tendermint/tendermint/types"
 )
@@ -180,13 +179,8 @@ func (blockExec *BlockExecutor) Commit(
 	err = blockExec.mempool.Update(
 		block.Height,
 		block.Txs,
-		mempool.PreCheckAminoMaxBytes(
-			types.MaxDataBytesUnknownEvidence(
-				state.ConsensusParams.BlockSize.MaxBytes,
-				state.Validators.Size(),
-			),
-		),
-		mempool.PostCheckMaxGas(state.ConsensusParams.BlockSize.MaxGas),
+		TxPreCheck(state),
+		TxPostCheck(state),
 	)
 
 	return res.Data, err

state/tx_filter.go

@@ -1,15 +1,22 @@
 package state
 
 import (
+	mempl "github.com/tendermint/tendermint/mempool"
 	"github.com/tendermint/tendermint/types"
 )
 
-// TxFilter returns a function to filter transactions. The function limits the
-// size of a transaction to the maximum block's data size.
-func TxFilter(state State) func(tx types.Tx) bool {
+// 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 {
 	maxDataBytes := types.MaxDataBytesUnknownEvidence(
 		state.ConsensusParams.BlockSize.MaxBytes,
 		state.Validators.Size(),
 	)
-	return func(tx types.Tx) bool { return int64(len(tx)) <= maxDataBytes }
+	return mempl.PreCheckAminoMaxBytes(maxDataBytes)
+}
+
+// TxPostCheck returns a function to filter transactions after processing.
+// The function limits the gas wanted by a transaction to the block's maximum total gas.
+func TxPostCheck(state State) mempl.PostCheckFunc {
+	return mempl.PostCheckMaxGas(state.ConsensusParams.BlockSize.MaxGas)
 }

state/tx_filter_test.go

@@ -18,12 +18,18 @@ func TestTxFilter(t *testing.T) {
 	genDoc := randomGenesisDoc()
 	genDoc.ConsensusParams.BlockSize.MaxBytes = 3000
 
+	// Max size of Txs is much smaller than size of block,
+	// since we need to account for commits and evidence.
 	testCases := []struct {
-		tx        types.Tx
-		isTxValid bool
+		tx    types.Tx
+		isErr bool
 	}{
-		{types.Tx(cmn.RandBytes(250)), true},
-		{types.Tx(cmn.RandBytes(3001)), false},
+		{types.Tx(cmn.RandBytes(250)), false},
+		{types.Tx(cmn.RandBytes(1809)), false},
+		{types.Tx(cmn.RandBytes(1810)), false},
+		{types.Tx(cmn.RandBytes(1811)), true},
+		{types.Tx(cmn.RandBytes(1812)), true},
+		{types.Tx(cmn.RandBytes(3000)), true},
 	}
 
 	for i, tc := range testCases {
@@ -31,8 +37,12 @@ func TestTxFilter(t *testing.T) {
 		state, err := LoadStateFromDBOrGenesisDoc(stateDB, genDoc)
 		require.NoError(t, err)
 
-		f := TxFilter(state)
-		assert.Equal(t, tc.isTxValid, f(tc.tx), "#%v", i)
+		f := TxPreCheck(state)
+		if tc.isErr {
+			assert.NotNil(t, f(tc.tx), "#%v", i)
+		} else {
+			assert.Nil(t, f(tc.tx), "#%v", i)
+		}
 	}
 }
 

types/block.go

@@ -21,6 +21,8 @@ const (
 
 	// MaxAminoOverheadForBlock - maximum amino overhead to encode a block (up to
 	// MaxBlockSizeBytes in size) not including it's parts except Data.
+	// This means it also excludes the overhead for individual transactions.
+	// To compute individual transactions' overhead use types.ComputeAminoOverhead(tx types.Tx, fieldNum int).
 	//
 	// Uvarint length of MaxBlockSizeBytes: 4 bytes
 	// 2 fields (2 embedded):               2 bytes

types/block_test.go

@@ -250,7 +250,7 @@ func TestMaxHeaderBytes(t *testing.T) {
 	timestamp := time.Date(math.MaxInt64, 0, 0, 0, 0, 0, math.MaxInt64, time.UTC)
 
 	h := Header{
-		Version:            version.Consensus{math.MaxInt64, math.MaxInt64},
+		Version:            version.Consensus{Block: math.MaxInt64, App: math.MaxInt64},
 		ChainID:            maxChainID,
 		Height:             math.MaxInt64,
 		Time:               timestamp,

types/tx.go

@@ -5,6 +5,8 @@ import (
 	"errors"
 	"fmt"
 
+	"github.com/tendermint/go-amino"
+
 	abci "github.com/tendermint/tendermint/abci/types"
 	"github.com/tendermint/tendermint/crypto/merkle"
 	"github.com/tendermint/tendermint/crypto/tmhash"
@@ -118,3 +120,18 @@ type TxResult struct {
 	Tx     Tx                     `json:"tx"`
 	Result abci.ResponseDeliverTx `json:"result"`
 }
+
+// ComputeAminoOverhead calculates the overhead for amino encoding a transaction.
+// The overhead consists of varint encoding the field number and the wire type
+// (= length-delimited = 2), and another varint encoding the length of the
+// transaction.
+// The field number can be the field number of the particular transaction, or
+// the field number of the parenting struct that contains the transactions []Tx
+// as a field (this field number is repeated for each contained Tx).
+// If some []Tx are encoded directly (without a parenting struct), the default
+// fieldNum is also 1 (see BinFieldNum in amino.MarshalBinaryBare).
+func ComputeAminoOverhead(tx Tx, fieldNum int) int64 {
+	fnum := uint64(fieldNum)
+	typ3AndFieldNum := (uint64(fnum) << 3) | uint64(amino.Typ3_ByteLength)
+	return int64(amino.UvarintSize(typ3AndFieldNum)) + int64(amino.UvarintSize(uint64(len(tx))))
+}

types/tx_test.go

@@ -96,6 +96,63 @@ func TestTxProofUnchangable(t *testing.T) {
 	}
 }
 
+func TestComputeTxsOverhead(t *testing.T) {
+	cases := []struct {
+		txs          Txs
+		wantOverhead int
+	}{
+		{Txs{[]byte{6, 6, 6, 6, 6, 6}}, 2},
+		// one 21 Mb transaction:
+		{Txs{make([]byte, 22020096, 22020096)}, 5},
+		// two 21Mb/2 sized transactions:
+		{Txs{make([]byte, 11010048, 11010048), make([]byte, 11010048, 11010048)}, 10},
+		{Txs{[]byte{1, 2, 3}, []byte{1, 2, 3}, []byte{4, 5, 6}}, 6},
+		{Txs{[]byte{100, 5, 64}, []byte{42, 116, 118}, []byte{6, 6, 6}, []byte{6, 6, 6}}, 8},
+	}
+
+	for _, tc := range cases {
+		totalBytes := int64(0)
+		totalOverhead := int64(0)
+		for _, tx := range tc.txs {
+			aminoOverhead := ComputeAminoOverhead(tx, 1)
+			totalOverhead += aminoOverhead
+			totalBytes += aminoOverhead + int64(len(tx))
+		}
+		bz, err := cdc.MarshalBinaryBare(tc.txs)
+		assert.EqualValues(t, tc.wantOverhead, totalOverhead)
+		assert.NoError(t, err)
+		assert.EqualValues(t, len(bz), totalBytes)
+	}
+}
+
+func TestComputeAminoOverhead(t *testing.T) {
+	cases := []struct {
+		tx       Tx
+		fieldNum int
+		want     int
+	}{
+		{[]byte{6, 6, 6}, 1, 2},
+		{[]byte{6, 6, 6}, 16, 3},
+		{[]byte{6, 6, 6}, 32, 3},
+		{[]byte{6, 6, 6}, 64, 3},
+		{[]byte{6, 6, 6}, 512, 3},
+		{[]byte{6, 6, 6}, 1024, 3},
+		{[]byte{6, 6, 6}, 2048, 4},
+		{make([]byte, 64), 1, 2},
+		{make([]byte, 65), 1, 2},
+		{make([]byte, 127), 1, 2},
+		{make([]byte, 128), 1, 3},
+		{make([]byte, 256), 1, 3},
+		{make([]byte, 512), 1, 3},
+		{make([]byte, 1024), 1, 3},
+		{make([]byte, 128), 16, 4},
+	}
+	for _, tc := range cases {
+		got := ComputeAminoOverhead(tc.tx, tc.fieldNum)
+		assert.EqualValues(t, tc.want, got)
+	}
+}
+
 func testTxProofUnchangable(t *testing.T) {
 	// make some proof
 	txs := makeTxs(randInt(2, 100), randInt(16, 128))