Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: make mebibyte a constant #4225

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
5 changes: 1 addition & 4 deletions app/benchmarks/benchmark_msg_send_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -307,17 +307,14 @@ func generateMsgSendTransactions(b *testing.B, count int) (*app.App, [][]byte) {
return testApp, rawTxs
}

// mebibyte the number of bytes in a mebibyte
const mebibyte = 1048576

// calculateBlockSizeInMb returns the block size in mb given a set
// of raw transactions.
func calculateBlockSizeInMb(txs [][]byte) float64 {
numberOfBytes := 0
for _, tx := range txs {
numberOfBytes += len(tx)
}
mb := float64(numberOfBytes) / mebibyte
mb := float64(numberOfBytes) / Mebibyte
return mb
}

Expand Down
10 changes: 5 additions & 5 deletions app/default_overrides.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ import (
coretypes "github.com/tendermint/tendermint/types"
)

const Mebibyte = 1048576
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we want this to be a constant, then we might as well use a global constant in the constant package

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a good idea. Where is the constant package? you mean the one in core

Copy link
Collaborator

@rootulp rootulp Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have https://github.com/celestiaorg/celestia-app/tree/main/pkg/appconsts.

My uber nit is that we don't want Mebibyte part of the public API of any of our packages so seems fine to define it once per package as a private constant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got you 👍 My initial motivation is just to stop writing that magic number everywhere and give it a name

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My uber nit is that we don't want Mebibyte part of the public API of any of our packages so seems fine to define it once per package as a private constant.

I'm fine with whichever you prefer :D


// bankModule defines a custom wrapper around the x/bank module's AppModuleBasic
// implementation to provide custom default genesis state.
type bankModule struct {
Expand Down Expand Up @@ -274,9 +276,8 @@ func DefaultConsensusConfig() *tmcfg.Config {
cfg.TxIndex.Indexer = "null"
cfg.Storage.DiscardABCIResponses = true

const mebibyte = 1048576
cfg.P2P.SendRate = 10 * mebibyte
cfg.P2P.RecvRate = 10 * mebibyte
cfg.P2P.SendRate = 10 * Mebibyte
cfg.P2P.RecvRate = 10 * Mebibyte

return cfg
}
Expand All @@ -295,7 +296,6 @@ func DefaultAppConfig() *serverconfig.Config {
cfg.StateSync.SnapshotKeepRecent = 2
cfg.MinGasPrices = fmt.Sprintf("%v%s", appconsts.DefaultMinGasPrice, BondDenom)

const mebibyte = 1048576
cfg.GRPC.MaxRecvMsgSize = 20 * mebibyte
cfg.GRPC.MaxRecvMsgSize = 20 * Mebibyte
return cfg
}
8 changes: 3 additions & 5 deletions app/default_overrides_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,7 @@ func TestDefaultAppConfig(t *testing.T) {
assert.Equal(t, uint32(2), cfg.StateSync.SnapshotKeepRecent)
assert.Equal(t, "0.002utia", cfg.MinGasPrices)

mebibyte := 1048576
assert.Equal(t, 20*mebibyte, cfg.GRPC.MaxRecvMsgSize)
assert.Equal(t, 20*Mebibyte, cfg.GRPC.MaxRecvMsgSize)
}

func TestDefaultConsensusConfig(t *testing.T) {
Expand All @@ -93,9 +92,8 @@ func TestDefaultConsensusConfig(t *testing.T) {
assert.Equal(t, want, *got.Mempool)
})
t.Run("p2p overrides", func(t *testing.T) {
const mebibyte = 1048576
assert.Equal(t, int64(10*mebibyte), got.P2P.SendRate)
assert.Equal(t, int64(10*mebibyte), got.P2P.RecvRate)
assert.Equal(t, int64(10*Mebibyte), got.P2P.SendRate)
assert.Equal(t, int64(10*Mebibyte), got.P2P.RecvRate)
})
}

Expand Down
4 changes: 2 additions & 2 deletions app/test/big_blob_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ func (s *BigBlobSuite) SetupSuite() {
s.accounts = testfactory.GenerateAccounts(1)

tmConfig := testnode.DefaultTendermintConfig()
tmConfig.Mempool.MaxTxBytes = 10 * mebibyte
tmConfig.Mempool.MaxTxBytes = 10 * app.Mebibyte

cParams := testnode.DefaultConsensusParams()
cParams.Block.MaxBytes = 10 * mebibyte
cParams.Block.MaxBytes = 10 * app.Mebibyte

cfg := testnode.DefaultConfig().
WithFundedAccounts(s.accounts...).
Expand Down
2 changes: 1 addition & 1 deletion app/test/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ func (s *IntegrationTestSuite) TestMaxBlockSize() {
for i, tx := range txs {
// The default CometBFT mempool MaxTxBytes is 1 MiB so the generators in
// this test must create transactions that are smaller than that.
require.LessOrEqual(t, len(tx), 1*mebibyte)
require.LessOrEqual(t, len(tx), 1*app.Mebibyte)

res, err := s.cctx.Context.BroadcastTxSync(tx)
require.NoError(t, err)
Expand Down
1 change: 0 additions & 1 deletion app/test/unit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,4 @@ package app_test

const (
kibibyte = 1024
mebibyte = 1024 * kibibyte
)
4 changes: 2 additions & 2 deletions pkg/da/data_availability_header.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,8 @@ func EmptySquareShares() []share.Share {

// SquareSize is a copy of the function defined in the square package to avoid
// a circular dependency. TODO deduplicate
func SquareSize(len int) int {
return RoundUpPowerOfTwo(int(math.Ceil(math.Sqrt(float64(len)))))
func SquareSize(length int) int {
return RoundUpPowerOfTwo(int(math.Ceil(math.Sqrt(float64(length)))))
}

// RoundUpPowerOfTwo returns the next power of two greater than or equal to input.
Expand Down
4 changes: 2 additions & 2 deletions test/txsim/blob.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,8 @@ type Range struct {
Max int
}

func NewRange(min, max int) Range {
return Range{Min: min, Max: max}
func NewRange(minimum, maximum int) Range {
return Range{Min: minimum, Max: maximum}
}

// Rand returns a random number between min (inclusive) and max (exclusive).
Expand Down
2 changes: 1 addition & 1 deletion test/util/testnode/comet_node_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func customTendermintConfig() *tmconfig.Config {
// Override the MaxBodyBytes to allow the testnode to accept very large
// transactions and respond to queries with large responses (200 MiB was
// chosen only as an arbitrary large number).
tmCfg.RPC.MaxBodyBytes = 200 * mebibyte
tmCfg.RPC.MaxBodyBytes = 200 * Mebibyte

tmCfg.RPC.TimeoutBroadcastTxCommit = time.Minute
return tmCfg
Expand Down
2 changes: 1 addition & 1 deletion test/util/testnode/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (

const (
kibibyte = 1024 // bytes
mebibyte = 1_048_576 // bytes
Mebibyte = 1_048_576 // bytes
DefaultValidatorAccountName = "validator"
DefaultInitialBalance = genesis.DefaultInitialBalance
)
Expand Down
10 changes: 5 additions & 5 deletions x/blob/ante/blob_share_decorator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ import (
)

const (
mebibyte = 1_048_576 // 1 MiB
squareSize = 64
Mebibyte = 1_048_576 // bytes
)

func TestBlobShareDecorator(t *testing.T) {
Expand All @@ -42,7 +42,7 @@ func TestBlobShareDecorator(t *testing.T) {
{
name: "want no error if appVersion v1 and 8 MiB blob",
blobsPerPFB: 1,
blobSize: 8 * mebibyte,
blobSize: 8 * Mebibyte,
appVersion: v1.Version,
},
{
Expand All @@ -54,13 +54,13 @@ func TestBlobShareDecorator(t *testing.T) {
{
name: "PFB with 1 blob that is 1 MiB",
blobsPerPFB: 1,
blobSize: 1 * mebibyte,
blobSize: 1 * Mebibyte,
appVersion: v2.Version,
},
{
name: "PFB with 1 blob that is 2 MiB",
blobsPerPFB: 1,
blobSize: 2 * mebibyte,
blobSize: 2 * Mebibyte,
appVersion: v2.Version,
// This test case should return an error because a square size of 64
// has exactly 2 MiB of total capacity so the total blob capacity
Expand All @@ -76,7 +76,7 @@ func TestBlobShareDecorator(t *testing.T) {
{
name: "PFB with 2 blobs that are 1 MiB each",
blobsPerPFB: 2,
blobSize: 1 * mebibyte,
blobSize: 1 * Mebibyte,
appVersion: v2.Version,
// This test case should return an error for the same reason a
// single blob that is 2 MiB returns an error.
Expand Down
6 changes: 3 additions & 3 deletions x/blob/ante/max_total_blob_size_ante.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,11 @@ func (d MaxTotalBlobSizeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
return next(ctx, tx, simulate)
}

max := d.maxTotalBlobSize(ctx)
maximum := d.maxTotalBlobSize(ctx)
for _, m := range tx.GetMsgs() {
if pfb, ok := m.(*blobtypes.MsgPayForBlobs); ok {
if total := getTotal(pfb.BlobSizes); total > max {
return ctx, errors.Wrapf(blobtypes.ErrTotalBlobSizeTooLarge, "total blob size %d exceeds max %d", total, max)
if total := getTotal(pfb.BlobSizes); total > maximum {
return ctx, errors.Wrapf(blobtypes.ErrTotalBlobSizeTooLarge, "total blob size %d exceeds max %d", total, maximum)
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions x/blob/ante/max_total_blob_size_ante_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,14 @@ func TestMaxTotalBlobSizeDecorator(t *testing.T) {
{
name: "PFB with 1 blob that is 1 MiB",
pfb: &blob.MsgPayForBlobs{
BlobSizes: []uint32{mebibyte},
BlobSizes: []uint32{Mebibyte},
},
appVersion: v1.Version,
},
{
name: "PFB with 1 blob that is 2 MiB",
pfb: &blob.MsgPayForBlobs{
BlobSizes: []uint32{2 * mebibyte},
BlobSizes: []uint32{2 * Mebibyte},
},
appVersion: v1.Version,
// This test case should return an error because a square size of 64
Expand All @@ -68,7 +68,7 @@ func TestMaxTotalBlobSizeDecorator(t *testing.T) {
{
name: "PFB with 2 blobs that are 1 MiB each",
pfb: &blob.MsgPayForBlobs{
BlobSizes: []uint32{mebibyte, mebibyte},
BlobSizes: []uint32{Mebibyte, Mebibyte},
},
appVersion: v1.Version,
// This test case should return an error for the same reason a
Expand Down
10 changes: 5 additions & 5 deletions x/mint/types/minter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,14 +156,14 @@ func TestCalculateBlockProvisionError(t *testing.T) {
}

func randomBlockInterval() time.Duration {
min := (14 * time.Second).Nanoseconds()
max := (16 * time.Second).Nanoseconds()
return time.Duration(randInRange(min, max))
minimum := (14 * time.Second).Nanoseconds()
maximum := (16 * time.Second).Nanoseconds()
return time.Duration(randInRange(minimum, maximum))
Comment on lines -159 to +161
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why were these also changed?

}

// randInRange returns a random number in the range (min, max) inclusive.
func randInRange(min int64, max int64) int64 {
return rand.Int63n(max-min) + min
func randInRange(minimum int64, maximum int64) int64 {
return rand.Int63n(maximum-minimum) + minimum
Comment on lines -165 to +166
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

}

func BenchmarkCalculateBlockProvision(b *testing.B) {
Expand Down
Loading