MaxBytes #2184

melekes · 2018-08-08T12:28:28Z

Refs #2035

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

melekes · 2018-08-08T12:29:14Z

consensus/state.go

-		_, err = cdc.UnmarshalBinaryReader(cs.ProposalBlockParts.GetReader(), &cs.ProposalBlock, int64(cs.state.ConsensusParams.BlockSize.MaxBytes))
+		maxDataSize := cs.state.ConsensusParams.BlockSize.MaxTxsBytes
+		// header + evidence + last commit
+		const maxOtherPartsSize = 5000000 // 5 MB


this is ugly. and it should be lower

Have we ran through the numbers for how we got this size?
header will be 100 ed25519 sigs, plus a bunch of other stuff. We should have the exact size of last commit. Not sure how getting evidence size should work.

We should also calculate the exact amino overhead. (Perhaps we make a method in amino to reflect through all the structs and tell you how big it would be if you were to marshall it)

we should be able to be more precise, and we should do the opposite (ie. subtract from maxBlockSize to get maxDataSize)

melekes · 2018-08-08T12:33:09Z

mempool/mempool.go

@@ -368,7 +383,8 @@ func (mem *Mempool) notifyTxsAvailable() {

 // Reap returns a list of transactions currently in the mempool.
 // If maxTxs is -1, there is no cap on the number of returned transactions.
-func (mem *Mempool) Reap(maxTxs int) types.Txs {
+// If maxTxsBytes is -1, there is no cap on the size of returned transactions.
+func (mem *Mempool) Reap(maxTxs, maxTxsBytes int) types.Txs {


having maxTxs is convenient for testing and /unconfirmed_txs?limit=N endpoint, but it can be removed still. But then I am not sure what to do with /unconfirmed_txs?limit=N endpoint. Should we limit by total bytes?

I think we could just make a separate ReapTxs function, which should only be used by the endpoint and testing. I don't think that sort of code repitition will hurt us, as long as we don't use it in consensus.

melekes · 2018-08-08T12:33:35Z

state/state_test.go

@@ -328,7 +328,7 @@ func TestConsensusParamsChangesSaveLoad(t *testing.T) {
 	params[0] = state.ConsensusParams
 	for i := 1; i < N+1; i++ {
 		params[i] = *types.DefaultConsensusParams()
-		params[i].BlockSize.MaxBytes += i
+		params[i].BlockSize.MaxTxsBytes += i


not sure what's going on here

ValarDragon · 2018-08-08T15:21:59Z

abci/types/types.proto

@@ -206,7 +206,7 @@ message ConsensusParams {

 // BlockSize contains limits on the block size.
 message BlockSize {
-  int32 max_bytes = 1;
+  int32 max_txs_bytes = 1;
  int32 max_txs = 2;


I think we can remove max_txs. The only utility of maximum txs is for the RPC end point now.

ValarDragon · 2018-08-08T18:14:12Z

mempool/mempool.go

@@ -393,33 +393,34 @@ func (mem *Mempool) Reap(maxTxs, maxTxsBytes int) types.Txs {
 		time.Sleep(time.Millisecond * 10)
 	}

-	txs := mem.collectTxs(maxTxs, maxTxsBytes)
+	var cur int
+	txs := make([]types.Tx, 0, mem.txs.Len())


I think we will get a performance boost if we have a good estimate of avg size per tx, and set the initial capacity based off of that. (We could under-esitmate avg size per tx a bit) In the case of max=-1, we could just call reapTxs(-1) for simplicity.

i.e. Min(mem.txs.Len(), max / avgTxSize)

Indeed, but how big of a boost? It's better to make such optimizations after we know current performance profile. It may be that the time to calculate avg size will hurt us more (overweights gains).

Great point!

ValarDragon · 2018-08-08T18:15:52Z

mempool/mempool.go

 	}

-	txs := make([]types.Tx, 0, cmn.MinInt(mem.txs.Len(), maxTxs))
-	for e := mem.txs.Front(); e != nil && len(txs) < maxTxs; e = e.Next() {
+	txs := make([]types.Tx, 0, mem.txs.Len())


Can we still do the min between mem.txs.Len() and max? (and keep the maxTxs == -1 check)

ValarDragon · 2018-08-08T21:10:31Z

consensus/state.go

@@ -1328,6 +1336,13 @@ func (cs *ConsensusState) finalizeCommit(height int64) {

 	fail.Fail() // XXX

+	// update mempool filter because consensus params might have changed
+	cs.mempool.SetFilter(func(tx types.Tx) bool {


Is the idea behind using a filter here that we can more easily use any consensus param, rather than just the maxbytes? The only other thing I can imagine us doing prelaunch is enforcing a minfee. I think both of these would be more well suited inside of the mempool itself. This would also avoid us having to use the mutex on the consensus param everytime a new tx is added. I think this wall cause blocking issues, plus the defer is a performance hit.

more well suited inside of the mempool itself

but consensus params can be changed in any block (https://tendermint.com/docs/app-dev/abci-spec.html#endblock). you mean we should just copy them over if they change. That actually not a bad idea.

ValarDragon · 2018-08-08T22:33:31Z

Do you want to enforce max gas in this pr as well, or should we save that for a separate pr?

melekes · 2018-08-09T13:17:55Z

Do you want to enforce max gas in this pr as well, or should we save that for a separate pr?

separate PR

codecov-io · 2018-08-09T13:21:23Z

Codecov Report

Merging #2184 into develop will decrease coverage by 0.28%.
The diff coverage is 61.03%.

@@             Coverage Diff             @@
##           develop    #2184      +/-   ##
===========================================
- Coverage    61.14%   60.86%   -0.29%     
===========================================
  Files          196      196              
  Lines        16131    16201      +70     
===========================================
- Hits          9864     9860       -4     
- Misses        5411     5479      +68     
- Partials       856      862       +6

Impacted Files	Coverage Δ
rpc/core/mempool.go	`0% <0%> (ø)`	⬆️
rpc/core/pipe.go	`31.7% <0%> (ø)`	⬆️
consensus/state.go	`76.89% <100%> (-0.55%)`	⬇️
evidence/pool.go	`56.14% <100%> (ø)`	⬆️
state/services.go	`38.46% <38.46%> (ø)`	⬆️
node/node.go	`64.19% <50%> (-0.08%)`	⬇️
mempool/mempool.go	`69.35% <51.72%> (-2.27%)`	⬇️
state/execution.go	`75.25% <60%> (-0.26%)`	⬇️
evidence/store.go	`90.36% <75%> (-2.05%)`	⬇️
libs/autofile/autofile.go	`69.23% <0%> (-6.16%)`	⬇️
... and 4 more

ebuchman

Thanks for getting this started Anton. I did a cursory review. We probably should have started with an ADR here though. Would you mind writing one before we continue working on the code ?

ebuchman · 2018-08-10T21:57:08Z

abci/types/types.proto

-  int32 max_bytes = 1;
-  int32 max_txs = 2;
-  int64 max_gas = 3;
+  int32 max_txs_bytes = 1;


I think we should try to keep these as the actual block size - it includes commits and evidence and the header. We should have upper bounds for each of those that we can use to determine the max_tx_bytes

ebuchman · 2018-08-10T21:57:59Z

consensus/state.go

@@ -153,6 +153,13 @@ func NewConsensusState(
 	cs.setProposal = cs.defaultSetProposal

 	cs.updateToState(state)
+
+	// filter mempool txs based on consensus params
+	maxBytes := cs.state.ConsensusParams.TxSize.MaxBytes


i feel like it would be much better if this kind of thing were set higher up, rather than being a weird side effect of the consensus state. can this be done from the node.go?

I see we might need this because ConsensusParams might change. Maybe we can pass it in through Update instead ? better to concentrate the "changes due to a block commit" in one place

better to concentrate the "changes due to a block commit" in one place

agree

You mean

tendermint/state/execution.go

Line 148 in d580006

if err := blockExec.mempool.Update(block.Height, block.Txs); err != nil {

, right?

yes - if there's more parameters the mempool will depend on beyond the height, we should pass those in there too

ebuchman · 2018-08-10T22:00:48Z

consensus/state.go

-		_, err = cdc.UnmarshalBinaryReader(cs.ProposalBlockParts.GetReader(), &cs.ProposalBlock, int64(cs.state.ConsensusParams.BlockSize.MaxBytes))
+		maxDataSize := cs.state.ConsensusParams.BlockSize.MaxTxsBytes
+		// header + evidence + last commit
+		const maxOtherPartsSize = 5000000 // 5 MB


we should be able to be more precise, and we should do the opposite (ie. subtract from maxBlockSize to get maxDataSize)

melekes · 2018-08-13T09:23:49Z

We probably should have started with an ADR here though

agree. wanted to throw some code in for discussion, but it took longer than expected. will write an ADR

melekes · 2018-08-13T12:01:25Z

ADR: /~https://github.com/tendermint/tendermint/blob/b19bb3a51b76f088edab5a2cb44a87f40f4b065c/docs/architecture/adr-020-block-size.md

ValarDragon · 2018-08-13T18:25:12Z

docs/architecture/adr-020-block-size.md

+
+- MaxHeaderBytes - 512 bytes (~200 bytes hashes + 200 bytes - 50 UTF-8 encoded symbols of chain ID)
+- MaxEvidenceBytes - 4096 bytes (10 DuplicateVoteEvidence ~350 bytes each)
+- MaxLastCommitBytes - 163840 (160 KB) (1000 votes ~150 bytes each)


Not sure this one should be a parameter. Seems completely determined by number of validators and tendermint protocol right?

Correct, but we still want to limit the block size. We know that TM currently does not support more than 300 validators (even less), so 1000 should be fine.

Why not go the other way around?

i.e. Set a MaxBlockSize, and then derive tx size as follows:

MaxLastCommitBytes = function of number of validators currently enabled MaxEvidenceBytes = function of consensus param: "maxNumEvidences" MaxHeaderBytes = a consensus param

Then MaxTxSize = MaxBlockSize - amino overhead - MaxLastCommitBytes - MaxEvidenceBytes - MaxHeaderBytes. This makes MaxBlockSize the constant in consensus, not commit / tx size. (Max TxSize would then decrease automatically as num validators increases)

function of consensus param: "maxNumEvidences"

and MaxEvidenceBytes - max size of a single evidence

function of number of validators currently enabled

and MaxVoteBytes, right?

This makes MaxBlockSize the constant in consensus, not commit / tx size. (Max TxSize would then decrease automatically as num validators increases)

Right, so it's either a) we have a sum and block size is growing when number of validators increasing OR b) we have a fixed BlockSize.MaxBytes and number of txs is decreasing while number of validators increases

I prefer the latter, fixed block size and space for txs decreases as validators increases. Fixing the block size rather than tx data size feels more natural to me, and allows for better predictions of storage requirements, hence my preference for it.

MaxEvidenceBytes and MaxVoteBytes would be calculated values, not part of the config right?

ValarDragon · 2018-08-13T18:26:23Z

docs/architecture/adr-020-block-size.md

+
+Proposed default values are:
+
+- MaxHeaderBytes - 512 bytes (~200 bytes hashes + 200 bytes - 50 UTF-8 encoded symbols of chain ID)


This is probably my lack of familiarity, but why are there 200 bytes of hashes? I thought it would just be one merkelized hash at the root of the header.

nah, tendermint header will be identical to ABCI header

Your right, thanks for the clarification! (I hadn't read through the ABCI header definition before, this makes sense now)

ValarDragon · 2018-08-13T18:26:59Z

docs/architecture/adr-020-block-size.md

+Proposed default values are:
+
+- MaxHeaderBytes - 512 bytes (~200 bytes hashes + 200 bytes - 50 UTF-8 encoded symbols of chain ID)
+- MaxEvidenceBytes - 4096 bytes (10 DuplicateVoteEvidence ~350 bytes each)


Why don't we make this maximum "MaxEvidencesPerBlock", and then derive the assoc. bytes directly from that.

melekes · 2018-08-15T12:18:47Z

Updated ADR.

ValarDragon · 2018-08-15T17:26:57Z

docs/architecture/adr-020-block-size.md

+
+1) Get rid of MaxTxs.
+2) Rename MaxTxsBytes to MaxBytes.
+3) Add MaxHeaderBytes cs parameter (TODO: /~https://github.com/tendermint/tendermint/blob/8a1a79257e8e8b309cd35bb1fe40bf9b3330fd7d/types/block.go#L195)


I'm confused as to why this is a consensus parameter.

AFAICT, everything here can be upper bounded from type definition:

tendermint/types/block.go

Lines 197 to 222 in 8a1a792

type Header struct {

// basic block info

ChainID string `json:"chain_id"`

Height int64 `json:"height"`

Time time.Time `json:"time"`

NumTxs int64 `json:"num_txs"`

TotalTxs int64 `json:"total_txs"`

// prev block info

LastBlockID BlockID `json:"last_block_id"`

// hashes of block data

LastCommitHash cmn.HexBytes `json:"last_commit_hash"` // commit from validators from the last block

DataHash cmn.HexBytes `json:"data_hash"` // transactions

// hashes from the app output from the prev block

ValidatorsHash cmn.HexBytes `json:"validators_hash"` // validators for the current block

NextValidatorsHash cmn.HexBytes `json:"next_validators_hash"` // validators for the next block

ConsensusHash cmn.HexBytes `json:"consensus_hash"` // consensus params for current block

AppHash cmn.HexBytes `json:"app_hash"` // state after txs from the previous block

LastResultsHash cmn.HexBytes `json:"last_results_hash"` // root hash of all results from the txs from the previous block

// consensus info

EvidenceHash cmn.HexBytes `json:"evidence_hash"` // evidence included in the block

ProposerAddress Address `json:"proposer_address"` // original proposer of the block

}

All of the hashes are tmhashes, time.Time surely has a max size, we can bound chain-ID (256 bits b64 encoded? This is moreso dependent on what our plan for chain-ID is). We can use the max size for "int" in PartSetHeader (used within BlockID). So we should be able to derive the MaxHeaderBytes just from its definition. We can just amino encode it as well, so we don't have to guess at the amino overhead for the header.

Right. Thanks.

ValarDragon · 2018-08-16T16:14:37Z

ADR looks great to me! Thanks for writing it!

melekes · 2018-08-17T15:46:39Z

abci/types/types_test.go

@@ -21,7 +21,7 @@ func TestConsensusParams(t *testing.T) {

 	// get values with real fields
 	assert.NotNil(params.GetBlockSize())


not sure I understand the point of this test

melekes · 2018-08-17T15:47:49Z

consensus/reactor_test.go

@@ -194,9 +194,9 @@ func newMockEvidencePool(val []byte) *mockEvidencePool {
 	}
 }

-func (m *mockEvidencePool) PendingEvidence() []types.Evidence {
+func (m *mockEvidencePool) PendingEvidence(limit int) []types.Evidence {


should be uint really, but not sure if it's the right time

melekes · 2018-08-17T15:50:09Z

evidence/pool.go

@@ -58,8 +58,8 @@ func (evpool *EvidencePool) PriorityEvidence() []types.Evidence {
 }

 // PendingEvidence returns all uncommitted evidence.


update description

melekes · 2018-08-17T15:50:38Z

evidence/store.go

@@ -87,8 +88,9 @@ func (store *EvidenceStore) PriorityEvidence() (evidence []types.Evidence) {
 }

 // PendingEvidence returns all known uncommitted evidence.


update description

melekes · 2018-08-17T15:50:52Z

mempool/mempool.go

@@ -57,6 +57,8 @@ var (
 	ErrMempoolIsFull = errors.New("Mempool is full")
 )

+// type Filter func(types.Tx) bool


melekes · 2018-08-17T15:52:41Z

types/block.go

+	MaxHeaderBytes = 476
+
+	// AminoOverheadForBlock - amino overhead to encode the block (whole block + data field).
+	AminoOverheadForBlock = 5


do we need a script to get these values?

At the very least a test should fail if the Header changes and MaxHeaderBytes doesn't change with it.

melekes · 2018-08-17T15:53:54Z

TODO: limit chain ID (since now we have MaxHeaderBytes, calculated with max 50 UTF-8 symbols) to 50 symbols max

ebuchman

Just a few comments, not a thorough review

ebuchman · 2018-08-17T16:25:40Z

abci/types/types.proto

-  int32 max_txs = 2;
-  int64 max_gas = 3;
+  int64 max_gas = 2;
+  int32 max_num_evidences = 3;


do we need to expose this in here? maybe we can set some internal heuristic and let all just be bounded by max_bytes ?

What do you mean by this? Don't we want this to be a consensus parameter, and since it pertains to parts of a block size it belongs in this object?

If I understand correctly, your suggesting we have a function inside of the code that calculates max_num_evidence = f(max_bytes). This seems harder to configure, and as a user of the protocol I would want an easy way to alter this function. (At which point it would then have to go back into the config)

This seems harder to configure, and as a user of the protocol I would want an easy way to alter this function.

Do you really though? Seems like it should basically be as large as possible - if there's evidence, Tendermint should prioritize including it. Not clear that it's really something that needs to be exposed to users right now.

ebuchman · 2018-08-17T16:26:50Z

consensus/state.go

@@ -153,6 +153,13 @@ func NewConsensusState(
 	cs.setProposal = cs.defaultSetProposal

 	cs.updateToState(state)
+
+	// filter mempool txs based on consensus params
+	maxBytes := cs.state.ConsensusParams.TxSize.MaxBytes


yes - if there's more parameters the mempool will depend on beyond the height, we should pass those in there too

ebuchman · 2018-08-17T16:29:03Z

docs/architecture/adr-020-block-size.md

+
+```
+MaxLastCommitBytes = {number of validators currently enabled} * {MaxVoteBytes}
+MaxEvidenceBytes = {MaxNumEvidences} * {MaxEvidenceBytes}


i feel like we can set an internal MaxEvidenceBytes and check if evidence is available, take up to that amount, and then subtract precisely the amount of evidence we're including rather than potentially leaving a lot of room here with every block (since most blocks will have no evidence)

In order to do that, we would have to acquire the evidence before reaping the txs, or do 2 tx reaps. (The second after evidence is included) I'm not opposed to either of those possibilities.

Setting MaxEvidenceBytes vs MaxNumEvidences is independent of that however. I do think MaxNumEvidences is the better variable in the config.

rather than potentially leaving a lot of room here with every block

Yeah, did not think of that. Thank you! Let's do that.

In order to do that, we would have to acquire the evidence before reaping the txs, or do 2 tx reaps.

we can do it in one pass

tendermint/consensus/state.go

Lines 957 to 958 in 29a51a8

txs := cs.mempool.ReapMaxBytes(maxDataBytes(cs.state.Validators.Size(), &cs.state.ConsensusParams.BlockSize))

evidence := cs.evpool.PendingEvidence(cs.state.ConsensusParams.BlockSize.MaxNumEvidences)

ebuchman · 2018-08-17T16:30:25Z

docs/architecture/adr-020-block-size.md

+
+### Negative
+
+* Neccessity to configure additional variables


Will actually reduce the need to configure more variables potentially!

ebuchman · 2018-08-17T16:31:39Z

types/block.go

+	MaxHeaderBytes = 476
+
+	// AminoOverheadForBlock - amino overhead to encode the block (whole block + data field).
+	AminoOverheadForBlock = 5


At the very least a test should fail if the Header changes and MaxHeaderBytes doesn't change with it.

ebuchman · 2018-08-17T16:33:18Z

We should probably add something about gas to the ADR too so that we limit by both bytes and gas (which ever limit is reached first)

melekes · 2018-08-28T12:06:22Z

consensus/state.go

@@ -946,14 +947,25 @@ func (cs *ConsensusState) createProposalBlock() (block *types.Block, blockParts
 		return
 	}

+	maxBytes := cs.state.ConsensusParams.BlockSize.MaxBytes
+	// bound evidence to 1/10th of the block
+	evidence := cs.evpool.PendingEvidence(maxBytes / 10)


note: incoming block evidence is unbound (same as other parts)

We should state this ratio more explicitly

Spec for how we construct a block?

melekes · 2018-08-28T12:07:28Z

evidence/store.go

 // It is wrapped by PriorityEvidence and PendingEvidence for convenience.
-func (store *EvidenceStore) ListEvidence(prefixKey string) (evidence []types.Evidence) {
+// If maxBytes is -1, there's no cap on the size of returned evidence.
+func (store *EvidenceStore) ListEvidence(prefixKey string, maxBytes int) (evidence []types.Evidence) {


make private?

melekes · 2018-08-30T08:35:56Z

types/block_test.go

@@ -118,7 +120,7 @@ func TestBlockMakePartSetWithEvidence(t *testing.T) {

 	partSet := MakeBlock(h, txs, commit, evList).MakePartSet(1024)
 	assert.NotNil(t, partSet)
-	assert.Equal(t, 3, partSet.Total())
+	assert.Equal(t, 2, partSet.Total())


melekes · 2018-08-30T08:37:39Z

types/tx.go

@@ -11,6 +11,11 @@ import (
 	cmn "github.com/tendermint/tendermint/libs/common"
 )

+const (
+	// MaxAminoOverheadForTx - amino overhead to encode a transaction.


maximum amino overhead seems to range from 1 to 4 bytes

melekes · 2018-08-30T12:13:03Z

types/block.go

+	MaxHeaderBytes = 478
+
+	// MaxAminoOverheadForBlock - maximum amino overhead to encode a block (up to
+	// MaxBlockSizeBytes in size).


not including it's parts (only varint len + fields without data)

melekes · 2018-08-30T12:13:12Z

types/block.go

+
+	// MaxAminoOverheadForBlock - maximum amino overhead to encode a block (up to
+	// MaxBlockSizeBytes in size).
+	MaxAminoOverheadForBlock = 4


This seems way too optimistic imo. We have an extra byte of overhead for every field (field numbers). The Txs are in the block as well, so each one of those will increase the total overhead. We'd have to calculate a maximum number of txs to create the MaxAminoOverheadForBlock

I know. Should be 4 + 1 byte for each field probably ~ 8

We'd have to calculate a maximum number of txs to create the MaxAminoOverheadForBlock

no. see what I've done with MaxAminoOverheadForTx. In short: when we add txs, we count len(tx)+MaxAminoOverheadForTx

It's still seems a bit complex. If you or anybody else have ideas how to simplify this stuff, I'd be happy to hear them

melekes · 2018-08-30T12:13:57Z

types/tx.go

+const (
+	// MaxAminoOverheadForTx - maximum amino overhead to encode a transaction
+	// (ranges from 1 to 4 bytes).
+	MaxAminoOverheadForTx = 4


melekes · 2018-08-30T16:40:08Z

mempool/mempool.go

+	txs := make([]types.Tx, 0, mem.txs.Len())
+	for e := mem.txs.Front(); e != nil; e = e.Next() {
+		memTx := e.Value.(*mempoolTx)
+		if max > 0 && cur+len(memTx.tx)+types.MaxAminoOverheadForTx > max {


it does not have to be Max. We can create a amino#EstimateSize(o interface{}) function

See ADR 020: Limiting txs size inside a block docs/architecture/adr-020-block-size.md Refs #2035

ebuchman

I think we can merge this but there's a few things we should address before release - especially that it seems the numbers aren't quite right

ebuchman · 2018-08-31T18:18:21Z

consensus/state.go

@@ -946,14 +947,25 @@ func (cs *ConsensusState) createProposalBlock() (block *types.Block, blockParts
 		return
 	}

+	maxBytes := cs.state.ConsensusParams.BlockSize.MaxBytes
+	// bound evidence to 1/10th of the block
+	evidence := cs.evpool.PendingEvidence(maxBytes / 10)


We should state this ratio more explicitly

mempool/mempool.go

ebuchman · 2018-08-31T18:36:11Z

state/execution.go

@@ -145,7 +145,9 @@ func (blockExec *BlockExecutor) Commit(block *types.Block) ([]byte, error) {
 		"appHash", fmt.Sprintf("%X", res.Data))

 	// Update mempool.
-	if err := blockExec.mempool.Update(block.Height, block.Txs); err != nil {
+	maxBytes := state.ConsensusParams.TxSize.MaxBytes
+	filter := func(tx types.Tx) bool { return len(tx) <= maxBytes }


Maybe better to define this and export it.

Also do we want to allow a negative or zero MaxBytes to indicate unbounded?

It seems we're not checking TxSize.MaxBytes for txs in the block, just in the mempool. Technically that would make it not a consensus param, because a proposer running different mempool code could include txs larger than TxSize.MaxBytes

I wonder if we even need a TxSize.MaxBytes in the consensus params - it might be simpler to just use the computed maxDataBytes.

We'd still need Update to take a filter, but we'd be able to remove the TxSize configuration and just do it all through blocks.

I think making TxSize.MaxBytes a consensus parameter should be based on how we important we feel guarantees about the total number of transactions that can fit into a block is. (Thats the only advantage I see to making it a consensus parameter)

If we're unsure, I'd opt to include it if implementation of it wouldn't be hard. I like the idea of being able to make guarantees about the number of txs which can fit into a block, though I can't find a super convincing reason as to why.

though I can't find a super convincing reason as to why.

Right. We have a lot of surface area. When we don't have a really good reason to have something, it's probably worth removing it. Helps simplify things over all and reduce the possibility for errors.

Maybe better to define this and export it.

That was an initial implementation, but the problem is in

tendermint/state/services.go

Line 26 in fef6ab6

Update(height int64, txs types.Txs, filter func(types.Tx) bool) error

it's undesirable to import mempool package (just for mempl.Filter). Am I wrong?

We'd still need Update to take a filter, but we'd be able to remove the TxSize configuration and just do it all through blocks.

Should this be done along with enforcing MaxGas?

ebuchman · 2018-08-31T18:46:39Z

types/block_test.go

+		Height:             10,
+		Time:               time.Now().UTC(),
+		NumTxs:             100,
+		TotalTxs:           200,


Aren't these varints? What if we had TotalTxs: 20000 - then the test fails because the header is 479 bytes

ebuchman · 2018-08-31T18:48:36Z

types/evidence_test.go

+	const chainID = "mychain"
+	ev := &DuplicateVoteEvidence{
+		PubKey: secp256k1.GenPrivKey().PubKey(), // use secp because it's pubkey is longer
+		VoteA:  makeVote(val, chainID, 0, 10, 2, 1, blockID),


ebuchman · 2018-08-31T18:53:06Z

node/node.go

@@ -248,6 +248,8 @@ func NewNode(config *cfg.Config,
 		mempl.WithMetrics(memplMetrics),
 	)
 	mempool.SetLogger(mempoolLogger)
+	maxBytes := state.ConsensusParams.TxSize.MaxBytes
+	mempool.SetFilter(func(tx types.Tx) bool { return len(tx) <= maxBytes })


should we make this a mempool option and set it on the constructor instead?

I think it makes sense to add to the mempool config.

this mirrors ReapMaxBytes behavior See #2184 (comment)

by using math.MaxInt64 See #2184 (comment)

See #2184 (comment)

* ReapMaxTxs: return all txs if max is negative this mirrors ReapMaxBytes behavior See #2184 (comment) * increase MaxAminoOverheadForBlock tested with: ``` func TestMaxAminoOverheadForBlock(t *testing.T) { maxChainID := "" for i := 0; i < MaxChainIDLen; i++ { maxChainID += "𠜎" } h := Header{ ChainID: maxChainID, Height: 10, Time: time.Now().UTC(), NumTxs: 100, TotalTxs: 200, LastBlockID: makeBlockID(make([]byte, 20), 300, make([]byte, 20)), LastCommitHash: tmhash.Sum([]byte("last_commit_hash")), DataHash: tmhash.Sum([]byte("data_hash")), ValidatorsHash: tmhash.Sum([]byte("validators_hash")), NextValidatorsHash: tmhash.Sum([]byte("next_validators_hash")), ConsensusHash: tmhash.Sum([]byte("consensus_hash")), AppHash: tmhash.Sum([]byte("app_hash")), LastResultsHash: tmhash.Sum([]byte("last_results_hash")), EvidenceHash: tmhash.Sum([]byte("evidence_hash")), ProposerAddress: tmhash.Sum([]byte("proposer_address")), } b := Block{ Header: h, Data: Data{Txs: makeTxs(10000, 100)}, Evidence: EvidenceData{}, LastCommit: &Commit{}, } bz, err := cdc.MarshalBinary(b) require.NoError(t, err) assert.Equal(t, MaxHeaderBytes+MaxAminoOverheadForBlock-2, len(bz)-1000000-20000-1) } ``` * fix MaxYYY constants calculation by using math.MaxInt64 See #2184 (comment) * pass mempool filter as an option See #2184 (comment) * fixes after Dev's comments

melekes requested review from ebuchman and xla as code owners August 8, 2018 12:28

melekes commented Aug 8, 2018

View reviewed changes

ValarDragon reviewed Aug 8, 2018

View reviewed changes

ebuchman reviewed Aug 10, 2018

View reviewed changes

melekes force-pushed the 2035-max-bytes branch from 915787e to 0158d23 Compare August 13, 2018 11:59

melekes requested a review from zramsay as a code owner August 13, 2018 11:59

ValarDragon reviewed Aug 13, 2018

View reviewed changes

ValarDragon reviewed Aug 15, 2018

View reviewed changes

melekes force-pushed the 2035-max-bytes branch from 5e20841 to a62e6ff Compare August 17, 2018 09:56

melekes commented Aug 17, 2018

View reviewed changes

ebuchman reviewed Aug 17, 2018

View reviewed changes

xla added the C:abci Component: Application Blockchain Interface label Aug 27, 2018

melekes commented Aug 28, 2018

View reviewed changes

melekes force-pushed the 2035-max-bytes branch from 3544261 to 03d5462 Compare August 30, 2018 08:26

melekes commented Aug 30, 2018

View reviewed changes

melekes changed the title ~~[WIP] MaxBytes~~ MaxBytes Aug 31, 2018

melekes added 6 commits August 31, 2018 16:01

reap max bytes from the mempool & check transaction size

d73c5cb

See ADR 020: Limiting txs size inside a block docs/architecture/adr-020-block-size.md Refs #2035

limit chain ID to 50 symbols max

0f74856

update Gopkg.lock

ad3d429

be more precise in comments

e957f32

calculate amino overhead on the fly

e873fed

update comment for MaxBlockSizeBytes

02d1b03

melekes force-pushed the 2035-max-bytes branch from 2184078 to 02d1b03 Compare August 31, 2018 12:01

update arch doc

4147f85

ebuchman approved these changes Aug 31, 2018

View reviewed changes

ebuchman merged commit ffe91ae into develop Aug 31, 2018

ebuchman deleted the 2035-max-bytes branch August 31, 2018 19:16

This was referenced Aug 31, 2018

MaxBytes follow up #2308

Closed

Implement MaxGas #2310

Closed

Use ConsensusParams #998

Closed

melekes added a commit that referenced this pull request Sep 3, 2018

ReapMaxTxs: return all txs if max is negative

8d6bb4a

this mirrors ReapMaxBytes behavior See #2184 (comment)

melekes added a commit that referenced this pull request Sep 3, 2018

fix MaxYYY constants calculation

b15a655

by using math.MaxInt64 See #2184 (comment)

melekes added a commit that referenced this pull request Sep 3, 2018

pass mempool filter as an option

fef6ab6

See #2184 (comment)

melekes added a commit that referenced this pull request Sep 3, 2018

pass mempool filter as an option

4da659d

See #2184 (comment)

melekes mentioned this pull request Sep 6, 2018

RFC: TxSize.MaxBytes consensus parameter #2347

Closed

philipstanislaus mentioned this pull request Sep 7, 2018

Reset consensus param max block size to 1 MB once Tenermint released their fix thorchain/THORChain-v1#27

Open

ebuchman mentioned this pull request Sep 21, 2018

Cap evidence #1637

Closed


		Proposed default values are:

		- MaxHeaderBytes - 512 bytes (~200 bytes hashes + 200 bytes - 50 UTF-8 encoded symbols of chain ID)

	type Header struct {
	// basic block info
	ChainID string `json:"chain_id"`
	Height int64 `json:"height"`
	Time time.Time `json:"time"`
	NumTxs int64 `json:"num_txs"`
	TotalTxs int64 `json:"total_txs"`

	// prev block info
	LastBlockID BlockID `json:"last_block_id"`

	// hashes of block data
	LastCommitHash cmn.HexBytes `json:"last_commit_hash"` // commit from validators from the last block
	DataHash cmn.HexBytes `json:"data_hash"` // transactions

	// hashes from the app output from the prev block
	ValidatorsHash cmn.HexBytes `json:"validators_hash"` // validators for the current block
	NextValidatorsHash cmn.HexBytes `json:"next_validators_hash"` // validators for the next block
	ConsensusHash cmn.HexBytes `json:"consensus_hash"` // consensus params for current block
	AppHash cmn.HexBytes `json:"app_hash"` // state after txs from the previous block
	LastResultsHash cmn.HexBytes `json:"last_results_hash"` // root hash of all results from the txs from the previous block

	// consensus info
	EvidenceHash cmn.HexBytes `json:"evidence_hash"` // evidence included in the block
	ProposerAddress Address `json:"proposer_address"` // original proposer of the block
	}

		@@ -21,7 +21,7 @@ func TestConsensusParams(t *testing.T) {

		// get values with real fields
		assert.NotNil(params.GetBlockSize())

		@@ -58,8 +58,8 @@ func (evpool *EvidencePool) PriorityEvidence() []types.Evidence {
		}

		// PendingEvidence returns all uncommitted evidence.

		@@ -87,8 +88,9 @@ func (store *EvidenceStore) PriorityEvidence() (evidence []types.Evidence) {
		}

		// PendingEvidence returns all known uncommitted evidence.

	txs := cs.mempool.ReapMaxBytes(maxDataBytes(cs.state.Validators.Size(), &cs.state.ConsensusParams.BlockSize))
	evidence := cs.evpool.PendingEvidence(cs.state.ConsensusParams.BlockSize.MaxNumEvidences)

MaxBytes #2184

MaxBytes #2184

Conversation

melekes commented Aug 8, 2018

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

Choose a reason for hiding this comment

ValarDragon Aug 8, 2018 • edited Loading

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

ValarDragon commented Aug 8, 2018

melekes commented Aug 9, 2018

codecov-io commented Aug 9, 2018 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melekes commented Aug 13, 2018

melekes commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Aug 14, 2018 • edited Loading

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

melekes commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented Aug 16, 2018

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

melekes commented Aug 17, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebuchman Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebuchman commented Aug 17, 2018

melekes Aug 28, 2018 • edited Loading

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

ValarDragon Aug 8, 2018 •

edited

Loading

codecov-io commented Aug 9, 2018 •

edited

Loading

ValarDragon Aug 13, 2018 •

edited

Loading

ValarDragon Aug 14, 2018 •

edited

Loading

ValarDragon Aug 20, 2018 •

edited

Loading

ebuchman Aug 17, 2018 •

edited

Loading

melekes Aug 28, 2018 •

edited

Loading

melekes Aug 30, 2018 •

edited

Loading