fix amino overhead computation for Tx #2792

liamsi · 2018-11-09T15:02:35Z

adds a method to compute overhead per Tx which also counts the fieldnum / typ3
fixes #2789

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

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

codecov-io · 2018-11-09T15:49:29Z

Codecov Report

Merging #2792 into develop will decrease coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #2792      +/-   ##
===========================================
- Coverage     62.3%   62.28%   -0.02%     
===========================================
  Files          212      212              
  Lines        17210    17141      -69     
===========================================
- Hits         10722    10677      -45     
+ Misses        5588     5566      -22     
+ Partials       900      898       -2

Impacted Files	Coverage Δ
mempool/mempool.go	`77.81% <100%> (ø)`	⬆️
state/execution.go	`75.11% <100%> (-0.57%)`	⬇️
node/node.go	`64.18% <100%> (-0.72%)`	⬇️
state/tx_filter.go	`100% <100%> (ø)`	⬆️
libs/autofile/autofile.go	`76.31% <0%> (-4.51%)`	⬇️
evidence/reactor.go	`63% <0%> (-2%)`	⬇️
rpc/client/httpclient.go	`68.78% <0%> (-0.98%)`	⬇️
consensus/wal.go	`62.79% <0%> (-0.85%)`	⬇️
evidence/wire.go	`100% <0%> (ø)`	⬆️
libs/autofile/group.go	`65.88% <0%> (+0.17%)`	⬆️
... and 2 more

mempool/mempool.go

ValarDragon

Thanks for fixing this!

melekes

🏖

mempool/mempool.go

melekes · 2018-11-10T09:36:16Z

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).


melekes · 2018-11-10T09:38:34Z

types/tx.go

@@ -118,3 +120,9 @@ type TxResult struct {
 	Tx     Tx                     `json:"tx"`
 	Result abci.ResponseDeliverTx `json:"result"`
 }
+
+func ComputeAminoOverhead(tx Tx, fieldNum int) int64 {


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

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

ValarDragon · 2018-11-10T16:54:56Z

types/tx.go

+// 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 {


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.

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]; }

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

Good to know though, thank you!

ebuchman · 2018-11-10T23:44:55Z

mempool/mempool.go

@@ -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.


Is the []Tx relevant here? The mempool only ever deals in Tx, not []Tx

Oh I see, we need to encode the size of txs as they will appear in the block, not as they appear individually in the mempool.

ebuchman

Thanks for the fix. Nasty bug. We should add some tests in the state package that check that blocks made from txs that pass these filters are valid. That might have caught this sooner.

ebuchman · 2018-11-11T14:27:52Z

types/tx.go

+
+// 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


What does (= length-delimited = 2) mean?

The wire-type is what we call Typ3_ByteLength in amino, or, length-delimited in proto3, or, equal to 2 in both cases. Maybe that comment is too verbose?

Ah I see. Yeh I would have been more explicit, like (2, in this case, for a length-delimited type) but that's fine

mempool/mempool.go

ebuchman · 2018-11-11T14:39:34Z

Oops I need to fix the test

ebuchman · 2018-11-11T15:09:58Z

Opened follow-up issue to write better test: #2806

* 'master' of https://github.com/tendermint/tendermint: (330 commits) Release/v0.26.1 (tendermint#2803) fix amino overhead computation for Tx (tendermint#2792) p2p: re-check after sleeps (tendermint#2664) check the result of `ps.peer.Send` before calling `ps.setHasVote` (tendermint#2787) p2p: AddressBook requires addresses to have IDs; Do not close conn immediately after sending pex addrs in seed mode (tendermint#2797) test AutoFile#Size (happy path) [autofile/group] do not panic when checking size openFile creates a file if not exist => ErrNotExist is not possible use our logger in autofile/group Add tests for ValidateBasic methods (tendermint#2754) [docs] improve organization of ABCI docs & fix links (tendermint#2749) p2p: peer-id -> peer_id (tendermint#2771) mempool: print postCheck error (tendermint#2762) Fix crypto/merkle ProofOperators.Verify to check bounds on keypath pa… (tendermint#2756) Mempool WAL is still created by default in home directory, leads to permission errors (tendermint#2758) mempool: ErrPreCheck and more log info (tendermint#2724) Release/v0.26.0 (tendermint#2726) [ADR] [DRAFT] pubsub 2.0 (tendermint#2532) validate reactor messages (tendermint#2711) TMHASH is 32 bytes. Closes tendermint#1990 (tendermint#2732) ...

fix amino overhead computation for Tx:

8b62789

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

liamsi requested review from ebuchman, melekes and xla as code owners November 9, 2018 15:02

liamsi changed the title ~~fix amino overhead computation for Tx~~ WIP: fix amino overhead computation for Tx Nov 9, 2018

liamsi added 2 commits November 9, 2018 16:18

fix TestReapMaxBytesMaxGas according to amino overhead

f815629

fix TestMempoolFilters according to amino overhead

2ab4a40

liamsi changed the title ~~WIP: fix amino overhead computation for Tx~~ fix amino overhead computation for Tx Nov 9, 2018

ValarDragon reviewed Nov 9, 2018

View reviewed changes

mempool/mempool.go Show resolved Hide resolved

ValarDragon approved these changes Nov 9, 2018

View reviewed changes

melekes reviewed Nov 10, 2018

View reviewed changes

address review comments:

c746d93

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

liamsi force-pushed the 2789-amino_overhead branch from 877d219 to c746d93 Compare November 10, 2018 15:48

ValarDragon reviewed Nov 10, 2018

View reviewed changes

ebuchman reviewed Nov 10, 2018

View reviewed changes

ebuchman approved these changes Nov 11, 2018

View reviewed changes

fix and use sm.TxPreCheck

1f5f32f

ebuchman added 2 commits November 11, 2018 09:59

fix test

6ea93fe

remove print statement

cabc2fc

ebuchman mentioned this pull request Nov 11, 2018

test: validate full blocks and blocks made from mempool.Reap #2806

Closed

ebuchman merged commit 3ff820b into develop Nov 11, 2018

ebuchman deleted the 2789-amino_overhead branch November 11, 2018 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix amino overhead computation for Tx #2792

fix amino overhead computation for Tx #2792

liamsi commented Nov 9, 2018

codecov-io commented Nov 9, 2018 •

edited

ValarDragon left a comment

melekes left a comment

melekes Nov 10, 2018

melekes Nov 10, 2018

ValarDragon Nov 10, 2018

liamsi Nov 10, 2018

ValarDragon Nov 10, 2018

ebuchman Nov 10, 2018

ebuchman Nov 11, 2018

ebuchman left a comment

ebuchman Nov 11, 2018

liamsi Nov 11, 2018

ebuchman Nov 12, 2018

ebuchman commented Nov 11, 2018

ebuchman commented Nov 11, 2018

fix amino overhead computation for Tx #2792

fix amino overhead computation for Tx #2792

Conversation

liamsi commented Nov 9, 2018

codecov-io commented Nov 9, 2018 • edited

Codecov Report

ValarDragon left a comment

Choose a reason for hiding this comment

melekes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebuchman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebuchman commented Nov 11, 2018

ebuchman commented Nov 11, 2018

codecov-io commented Nov 9, 2018 •

edited