types: ConsensusParams: add feedback from @ebuchman and @melekes

types/genesis_test.go

@@ -60,31 +60,3 @@ func TestGenesis(t *testing.T) {
 	genDoc, err = GenesisDocFromJSON(genDocBytes)
 	assert.Error(t, err, "expected error for genDoc json with block size of 0")
 }
-
-func newConsensusParams(blockSize, partSize int) ConsensusParams {
-	return ConsensusParams{
-		BlockSizeParams:   BlockSizeParams{MaxBytes: blockSize},
-		BlockGossipParams: BlockGossipParams{BlockPartSizeBytes: partSize},
-	}
-
-}
-
-func TestConsensusParams(t *testing.T) {
-
-	testCases := []struct {
-		params ConsensusParams
-		valid  bool
-	}{
-		{newConsensusParams(1, 1), true},
-		{newConsensusParams(1, 0), false},
-		{newConsensusParams(0, 1), false},
-		{newConsensusParams(0, 0), false},
-	}
-	for _, testCase := range testCases {
-		if testCase.valid {
-			assert.NoError(t, testCase.params.Validate(), "expected no error for valid params")
-		} else {
-			assert.Error(t, testCase.params.Validate(), "expected error for non valid params")
-		}
-	}
-}

types/params.go

@@ -5,7 +5,7 @@ import (
 )
 
 const (
-	_100MiB = 104857600
+	maxBlockSizeBytes = 104857600 // 100MB
 )
 
 // ConsensusParams contains consensus critical parameters
@@ -18,7 +18,7 @@ type ConsensusParams struct {
 
 // BlockSizeParams contain limits on the block size.
 type BlockSizeParams struct {
-	MaxBytes int `json:"max_bytes"` // NOTE: must not be 0
+	MaxBytes int `json:"max_bytes"` // NOTE: must not be 0 nor greater than 100MB
 	MaxTxs   int `json:"max_txs"`
 	MaxGas   int `json:"max_gas"`
 }
@@ -69,10 +69,6 @@ func DefaultBlockGossipParams() BlockGossipParams {
 
 // Validate validates the ConsensusParams to ensure all values
 // are within their allowed limits, and returns an error if they are not.
-// Expecting:
-// * BlockSizeParams.MaxBytes > 0
-// * BlockGossipParams.BlockPartSizeBytes > 0
-// * BlockSizeParams.MaxBytes <= 100MiB
 func (params *ConsensusParams) Validate() error {
 	// ensure some values are greater than 0
 	if params.BlockSizeParams.MaxBytes <= 0 {
@@ -83,9 +79,9 @@ func (params *ConsensusParams) Validate() error {
 	}
 
 	// ensure blocks aren't too big
-	if params.BlockSizeParams.MaxBytes > _100MiB {
+	if params.BlockSizeParams.MaxBytes > maxBlockSizeBytes {
 		return errors.Errorf("BlockSizeParams.MaxBytes is too big. %d > %d",
-			params.BlockSizeParams.MaxBytes, _100MiB)
+			params.BlockSizeParams.MaxBytes, maxBlockSizeBytes)
 	}
 	return nil
 }

types/params_test.go

@@ -1,66 +1,40 @@
-package types_test
+package types
 
 import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-
-	"github.com/tendermint/tendermint/types"
 )
 
-func TestConsensusParamsValidation(t *testing.T) {
+func newConsensusParams(blockSize, partSize int) ConsensusParams {
-	tests := [...]struct {
+	return ConsensusParams{
-		params  *types.ConsensusParams
+		BlockSizeParams:   BlockSizeParams{MaxBytes: blockSize},
-		wantErr string
+		BlockGossipParams: BlockGossipParams{BlockPartSizeBytes: partSize},
-	}{
-		{&types.ConsensusParams{}, "BlockSizeParams.MaxBytes must be greater than 0"},
-		{
-			&types.ConsensusParams{BlockSizeParams: types.BlockSizeParams{MaxBytes: 10}},
-			"BlockGossipParams.BlockPartSizeBytes must be greater than 0",
-		},
-		{
-			&types.ConsensusParams{
-				BlockSizeParams:   types.BlockSizeParams{MaxBytes: 10},
-				BlockGossipParams: types.BlockGossipParams{BlockPartSizeBytes: -1},
-			}, "BlockGossipParams.BlockPartSizeBytes must be greater than 0",
-		},
-		{
-			&types.ConsensusParams{
-				BlockSizeParams:   types.BlockSizeParams{MaxBytes: 10},
-				BlockGossipParams: types.BlockGossipParams{BlockPartSizeBytes: 400},
-			}, ""},
-		{
-			&types.ConsensusParams{
-				BlockSizeParams:   types.BlockSizeParams{MaxBytes: 1024 * 1024 * 47},
-				BlockGossipParams: types.BlockGossipParams{BlockPartSizeBytes: 400},
-			}, "",
-		},
-		{
-			&types.ConsensusParams{
-				BlockSizeParams:   types.BlockSizeParams{MaxBytes: 1024 * 1024 * 100},
-				BlockGossipParams: types.BlockGossipParams{BlockPartSizeBytes: 400},
-			}, "",
-		},
-		{
-			&types.ConsensusParams{
-				BlockSizeParams:   types.BlockSizeParams{MaxBytes: 1024 * 1024 * 101},
-				BlockGossipParams: types.BlockGossipParams{BlockPartSizeBytes: 400},
-			}, "BlockSizeParams.MaxBytes is too big",
-		},
-		{
-			&types.ConsensusParams{
-				BlockSizeParams:   types.BlockSizeParams{MaxBytes: 1024 * 1024 * 1024},
-				BlockGossipParams: types.BlockGossipParams{BlockPartSizeBytes: 400},
-			}, "BlockSizeParams.MaxBytes is too big",
-		},
 	}
+}
 
-	for i, tt := range tests {
+func TestConsensusParamsValidation(t *testing.T) {
-		err := tt.params.Validate()
+	testCases := []struct {
-		if tt.wantErr != "" {
+		params ConsensusParams
-			assert.Contains(t, err.Error(), tt.wantErr, "#%d: params: %#v", i, tt.params)
+		valid  bool
+	}{
+		{newConsensusParams(1, 1), true},
+		{newConsensusParams(1, 0), false},
+		{newConsensusParams(0, 1), false},
+		{newConsensusParams(0, 0), false},
+		{newConsensusParams(0, 10), false},
+		{newConsensusParams(10, -1), false},
+		{newConsensusParams(47*1024*1024, 400), true},
+		{newConsensusParams(10, 400), true},
+		{newConsensusParams(100*1024*1024, 400), true},
+		{newConsensusParams(101*1024*1024, 400), false},
+		{newConsensusParams(1024*1024*1024, 400), false},
+	}
+	for _, testCase := range testCases {
+		if testCase.valid {
+			assert.NoError(t, testCase.params.Validate(), "expected no error for valid params")
 		} else {
-			assert.Nil(t, err, "#%d: want nil error; Params: %#v", i, tt.params)
+			assert.Error(t, testCase.params.Validate(), "expected error for non valid params")
 		}
 	}
 }