Remove ConsensusParams.TxSize and ConsensusParams.BlockGossip by melekes · Pull Request #2364 · tendermint/tendermint

melekes · 2018-09-10T12:19:44Z

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

melekes · 2018-09-10T12:20:37Z

node/node.go


 	// Make MempoolReactor
-	maxBytes := state.ConsensusParams.TxSize.MaxBytes
+	maxBytes := state.ConsensusParams.BlockSize.MaxBytes


TODO: MaxBytes - headerSize and so on

melekes · 2018-09-10T12:20:57Z

state/execution.go


 	// Update mempool.
-	maxBytes := state.ConsensusParams.TxSize.MaxBytes
+	maxBytes := state.ConsensusParams.BlockSize.MaxBytes


TODO: MaxBytes - headerSize and so on

melekes · 2018-09-10T12:22:11Z

consensus/replay_test.go

 			func(stateDB dbm.DB, cs *ConsensusState, ctx context.Context) {
 				// XXX: is there a better way to change BlockPartSizeBytes?
-				cs.state.ConsensusParams.BlockPartSizeBytes = 512
+				// cs.state.ConsensusParams.BlockPartSizeBytes = 512


FIXME: make BlockPartSizeBytes a variable?

codecov-io · 2018-09-10T12:26:01Z

Codecov Report

Merging #2364 into develop will decrease coverage by 0.01%.
The diff coverage is 78.57%.

@@             Coverage Diff             @@
##           develop    #2364      +/-   ##
===========================================
- Coverage    60.96%   60.95%   -0.02%     
===========================================
  Files          197      197              
  Lines        16301    16259      -42     
===========================================
- Hits          9938     9910      -28     
+ Misses        5499     5489      -10     
+ Partials       864      860       -4

Impacted Files	Coverage Δ
consensus/wal.go	`61.24% <ø> (ø)`	⬆️
blockchain/reactor.go	`39.1% <0%> (-0.34%)`	⬇️
consensus/state.go	`76.4% <100%> (-0.86%)`	⬇️
state/state.go	`88.5% <100%> (ø)`	⬆️
state/execution.go	`75.62% <80%> (+0.36%)`	⬆️
node/node.go	`65.29% <80%> (+0.26%)`	⬆️
libs/db/remotedb/remotedb.go	`36.84% <0%> (-4.69%)`	⬇️
consensus/reactor.go	`72.61% <0%> (-0.94%)`	⬇️
... and 2 more

Refs #2347

ValarDragon · 2018-09-11T06:27:02Z

abci/types/types.proto


 // BlockSize contains limits on the block size.
 message BlockSize {
+  // Note: must be greater than 0


Couldn't it be -1?

Unlimited? Do you think it's a good idea?

Probably best if we're consistent, so these ints all have the same domain

ValarDragon · 2018-09-11T06:30:15Z

blockchain/reactor_test.go

 		firstBlock := makeBlock(blockHeight, state)
 		secondBlock := makeBlock(blockHeight+1, state)
-		firstParts := firstBlock.MakePartSet(state.ConsensusParams.BlockGossip.BlockPartSizeBytes)
+		firstParts := firstBlock.MakePartSet(types.BlockPartSizeBytes)


I still don't really understand why we don't want this as a consensus parameter. What optimizations are we getting by optimizing around 1MB blocks, it feels like we could still leave this as parameterized to me. (Or perhaps parameterize as "number of block parts")

NOTE: BlockPartSizeBytes really depends on the network bandwidth and network topology. It should be possible to make it adaptive to the current env by analysing the network, but it's #post-launch imho.

Plus we wasn't checking the size when accepting block part (no consensus per se 😄 ).

The goal is to reduce surface area for applications - its unlikely the applications should tweak this parameter directly, would be better if we had a heuristic for it based on the block size.

We'll still want to tweak this parameter in experiments, so we'll need a way for it to be configurable, but it will be a different domain than the ConsensusParams that gets updated by applications.

ebuchman

Looks good. A few minor nits. We also need to update the docs

ebuchman · 2018-09-11T15:02:50Z

blockchain/reactor_test.go

 		firstBlock := makeBlock(blockHeight, state)
 		secondBlock := makeBlock(blockHeight+1, state)
-		firstParts := firstBlock.MakePartSet(state.ConsensusParams.BlockGossip.BlockPartSizeBytes)
+		firstParts := firstBlock.MakePartSet(types.BlockPartSizeBytes)


The goal is to reduce surface area for applications - its unlikely the applications should tweak this parameter directly, would be better if we had a heuristic for it based on the block size.

We'll still want to tweak this parameter in experiments, so we'll need a way for it to be configurable, but it will be a different domain than the ConsensusParams that gets updated by applications.

ebuchman · 2018-09-11T15:07:40Z

consensus/wal.go

 const (
-	// must be greater than params.BlockGossip.BlockPartSizeBytes + a few bytes
+	// must be greater than types.BlockPartSizeBytes + a few bytes
 	maxMsgSizeBytes = 1024 * 1024 // 1MB


New issue (#2381): We should make this a function of BlockPartSizeBytes. Probably should do something similar in other reactors (ie. max is a function of size of largest message).

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

state/execution.go

 	// Update mempool.
-	maxBytes := state.ConsensusParams.TxSize.MaxBytes
-	filter := func(tx types.Tx) bool { return len(tx) <= maxBytes }
+	maxDataBytes := types.MaxDataBytesUnknownEvidence(


Maybe the State can expose a FilterTx method that does all this and returns a filter func can be passed into the NewMempool and mempool.Update?

ebuchman · 2018-09-11T15:12:17Z

types/evidence.go

 	cdc.RegisterConcrete(MockBadEvidence{}, "tendermint/MockBadEvidence", nil)
 }

+// MaxEvidenceBytesPerBlock returns the maximum evidence size per block.


Worth including the magic number (divide by 10) in the documentation

ebuchman · 2018-09-11T15:15:46Z

types/params.go

-// TxSize contain limits on the tx size.
-type TxSize struct {
-	MaxBytes int   `json:"max_bytes"`
+	MaxBytes int   `json:"max_txs_bytes"`


json here should be max_bytes as well

ebuchman · 2018-09-12T19:26:47Z

types/params.go

 			params.BlockSize.MaxBytes, MaxBlockSizeBytes)
 	}
+
+	if params.BlockSize.MaxGas < -1 {


Maybe these should all be the same for consistency? ie. < -1

ebuchman · 2018-09-12T19:27:39Z

types/params.go

-		if params2.TxSize.MaxGas > 0 {
-			res.TxSize.MaxGas = params2.TxSize.MaxGas
-		}
+		res.BlockSize.MaxBytes = int(params2.BlockSize.MaxBytes)


can we unify these and remove the cast?

ebuchman · 2018-09-12T19:28:56Z

types/params_test.go

 )

-func newConsensusParams(txsBytes, partSize int) ConsensusParams {
+func newConsensusParams(txsBytes, evidenceAge int) ConsensusParams {


txBytes should be called maxBytes or blockBytes

we should also pass in maxGas or blockGas

ebuchman · 2018-09-12T19:29:27Z

types/params_test.go

 }

-func makeParams(txsBytes, blockGas, txBytes, txGas, partSize int) ConsensusParams {
+func makeParams(txsBytes, blockGas, evidenceAge int) ConsensusParams {


deduplicate with newConsensusParams?

ebuchman · 2018-09-12T19:31:53Z

abci/types/types.proto


 // BlockSize contains limits on the block size.
 message BlockSize {
+  // Note: must be greater than 0


Probably best if we're consistent, so these ints all have the same domain

ebuchman · 2018-09-12T19:44:54Z

Follow up in #2382

remove ConsensusParams.TxSize and ConsensusParams.BlockGossip

674d564

Refs #2347

melekes requested review from ebuchman and xla as code owners September 10, 2018 12:19

melekes commented Sep 10, 2018

View reviewed changes

melekes changed the title ~~remove ConsensusParams.TxSize and ConsensusParams.BlockGossip~~ [WIP] remove ConsensusParams.TxSize and ConsensusParams.BlockGossip Sep 10, 2018

melekes added 2 commits September 11, 2018 09:47

block part size is now fixed

f94cf66

Refs #2347

use max data size, not max bytes for tx limit

af1a405

Refs #2347

melekes changed the title ~~[WIP] remove ConsensusParams.TxSize and ConsensusParams.BlockGossip~~ Remove ConsensusParams.TxSize and ConsensusParams.BlockGossip Sep 11, 2018

melekes requested a review from ValarDragon September 11, 2018 06:21

ValarDragon reviewed Sep 11, 2018

View reviewed changes

ebuchman approved these changes Sep 12, 2018

View reviewed changes

ebuchman mentioned this pull request Sep 12, 2018

Follow up to removing some consensus params #2382

Closed

5 tasks

ebuchman merged commit 0e1cd88 into develop Sep 12, 2018

ebuchman deleted the 2347-max-bytes branch September 12, 2018 19:44

This was referenced Sep 12, 2018

RFC: TxSize.MaxBytes consensus parameter #2347

Closed

MaxBytes follow up #2308

Closed

Conversation

melekes commented Sep 10, 2018

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!

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

Uh oh!

Codecov Report

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!

ValarDragon Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ebuchman Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebuchman commented Sep 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Sep 10, 2018 •

edited

Loading

ValarDragon Sep 11, 2018 •

edited

Loading

ebuchman Sep 11, 2018 •

edited

Loading