fix amino overhead computation for Tx by liamsi · Pull Request #2792 · tendermint/tendermint

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

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

 	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

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

…) (tendermint#2795) Closes tendermint#2771 --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request tendermint#2792 done by [Mergify](https://mergify.com). Co-authored-by: Khanh Hoa <49144992+hoanguyenkh@users.noreply.github.com>

…rmint#2804) (tendermint#2813) Followup to tendermint#2792 which closed tendermint#2771. tendermint#2792 does not handle the case where Start is never called. If Start is not called, Stop returns an error, thus with tendermint#2792's implementation the only way to ensure that PingPongLatencyTimer is cleaned up is to call Start and Stop, even when not using any of the features provided by Start (i.e. events). This PR moves initialization of PingPongLatencyTimer into OnStart so that it is only initialized if it is going to be used. This PR also moves cleanup of PingPongLatencyTimer into readRoutine's defer statement to align it with other cleanup (i.e. closing the connection). #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request tendermint#2804 done by [Mergify](https://mergify.com). Co-authored-by: Ethan Reesor <firelizzard@gmail.com>

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

Conversation

liamsi commented Nov 9, 2018

Uh oh!

codecov-io commented Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

ValarDragon left a comment

Choose a reason for hiding this comment

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebuchman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ebuchman commented Nov 11, 2018

Uh oh!

ebuchman commented Nov 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Nov 9, 2018 •

edited

Loading