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

Jump to bottom

fix amino overhead computation for Tx #2792

Merged
merged 7 commits into from Nov 11, 2018
Merged

fix amino overhead computation for Tx #2792

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8b62789
fix amino overhead computation for Tx:
liamsi Nov 9, 2018
f815629
fix TestReapMaxBytesMaxGas according to amino overhead
liamsi Nov 9, 2018
2ab4a40
fix TestMempoolFilters according to amino overhead
liamsi Nov 9, 2018
c746d93
address review comments:
liamsi Nov 10, 2018
1f5f32f
fix and use sm.TxPreCheck
ebuchman Nov 11, 2018
6ea93fe
fix test
ebuchman Nov 11, 2018
cabc2fc
remove print statement
ebuchman Nov 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 3 additions & 4 deletions mempool/mempool.go
View file
Open in desktop
Expand Up @@ -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"
Expand Down Expand Up @@ -88,8 +87,8 @@ 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)
aminoOverhead := types.ComputeAminoOverhead(tx, 1)
ebuchman marked this conversation as resolved.
Show resolved Hide resolved
txSize := int64(len(tx)) + aminoOverhead
if txSize > maxBytes {
return fmt.Errorf("Tx size (including amino overhead) is too big: %d, max: %d",
txSize, maxBytes)
Expand Down Expand Up @@ -482,7 +481,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)
ebuchman marked this conversation as resolved.
Show resolved Hide resolved
if maxBytes > -1 && totalBytes+int64(len(memTx.tx))+aminoOverhead > maxBytes {
return txs
}
Expand Down
16 changes: 8 additions & 8 deletions mempool/mempool_test.go
View file
Open in desktop
Expand Up @@ -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},
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions types/block.go
View file
Open in desktop
Expand Up @@ -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).
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

//
// Uvarint length of MaxBlockSizeBytes: 4 bytes
// 2 fields (2 embedded): 2 bytes
Expand Down
2 changes: 1 addition & 1 deletion types/block_test.go
View file
Open in desktop
Expand Up @@ -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,
Expand Down
8 changes: 8 additions & 0 deletions types/tx.go
View file
Open in desktop
Expand Up @@ -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"
Expand Down Expand Up @@ -118,3 +120,9 @@ type TxResult struct {
Tx Tx `json:"tx"`
Result abci.ResponseDeliverTx `json:"result"`
}

func ComputeAminoOverhead(tx Tx, fieldNum int) int64 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought you're going to write a godoc comment?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Protobuf is so silly :/, the type is getting prepended to every single element of the slice. Why wouldn't they just make a native concept of slices with a length prefix at the beginning :L.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Protobuf can actually behave as you'd expect it. But only for primitive types. bytes doesn't seem to fall in this category (as it is length-delimited) ¯_(ツ)_/¯

Otherwise, you could do sth along the lines of:

message Data {
  repeated bytes Txs = 1 [packed = true];
}

Copy link
Contributor

Choose a reason for hiding this comment

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

:(, protobuf why you no treat bytes as primitives :(

Good to know though, thank you!

fnum := uint64(fieldNum)
typ3AndFieldNum := (uint64(fnum) << 3) | uint64(amino.Typ3_ByteLength)
return int64(amino.UvarintSize(typ3AndFieldNum)) + int64(amino.UvarintSize(uint64(len(tx))))
}
57 changes: 57 additions & 0 deletions types/tx_test.go
View file
Open in desktop
Expand Up @@ -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))
Expand Down