Skip to content

Commit

Permalink
revert: deduplicate max effective square size (celestiaorg#3338)
Browse files Browse the repository at this point in the history
Closes celestiaorg#3336 by
reverting celestiaorg#3294
Opens celestiaorg#2267

## Testing

I observe a consensus failure if I reboot celestia-app on `main`. I
don't observe a consensus failure if I reboot celestia-app on this
branch after the revert.
  • Loading branch information
rootulp authored Apr 18, 2024
1 parent 105d628 commit a8e3085
Show file tree
Hide file tree
Showing 6 changed files with 30 additions and 23 deletions.
3 changes: 1 addition & 2 deletions app/prepare_proposal.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"time"

"github.com/celestiaorg/celestia-app/v2/app/ante"
"github.com/celestiaorg/celestia-app/v2/app/squaresize"
"github.com/celestiaorg/celestia-app/v2/pkg/appconsts"
"github.com/celestiaorg/celestia-app/v2/pkg/da"
"github.com/celestiaorg/go-square/shares"
Expand Down Expand Up @@ -49,7 +48,7 @@ func (app *App) PrepareProposal(req abci.RequestPrepareProposal) abci.ResponsePr
// Build the square from the set of valid and prioritised transactions.
// The txs returned are the ones used in the square and block.
dataSquare, txs, err := square.Build(txs,
squaresize.MaxEffective(sdkCtx, app.BlobKeeper),
app.MaxEffectiveSquareSize(sdkCtx),
appconsts.SubtreeRootThreshold(app.GetBaseApp().AppVersion()),
)
if err != nil {
Expand Down
3 changes: 1 addition & 2 deletions app/process_proposal.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"time"

"github.com/celestiaorg/celestia-app/v2/app/ante"
"github.com/celestiaorg/celestia-app/v2/app/squaresize"
"github.com/celestiaorg/celestia-app/v2/pkg/appconsts"
"github.com/celestiaorg/celestia-app/v2/pkg/da"
blobtypes "github.com/celestiaorg/celestia-app/v2/x/blob/types"
Expand Down Expand Up @@ -122,7 +121,7 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) (resp abci.Resp
// Construct the data square from the block's transactions
dataSquare, err := square.Construct(
req.BlockData.Txs,
squaresize.MaxEffective(sdkCtx, app.BlobKeeper),
app.MaxEffectiveSquareSize(sdkCtx),
subtreeRootThreshold,
)
if err != nil {
Expand Down
16 changes: 6 additions & 10 deletions app/squaresize/max_effective.go → app/square_size.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
package squaresize
package app

import (
"github.com/celestiaorg/celestia-app/v2/pkg/appconsts"
sdk "github.com/cosmos/cosmos-sdk/types"
)

// MaxEffective returns the max effective square size.
func MaxEffective(ctx sdk.Context, blobKeeper BlobKeeper) int {
// MaxEffectiveSquareSize returns the max effective square size.
func (app *App) MaxEffectiveSquareSize(ctx sdk.Context) int {
// TODO: fix hack that forces the max square size for the first height to
// 64. This is due to our fork of the sdk not initializing state before
// BeginBlock of the first block. This is remedied in versions of the sdk
Expand All @@ -17,11 +17,7 @@ func MaxEffective(ctx sdk.Context, blobKeeper BlobKeeper) int {
return int(appconsts.DefaultGovMaxSquareSize)
}

govParam := int(blobKeeper.GovMaxSquareSize(ctx))
upperBound := appconsts.SquareSizeUpperBound(ctx.ConsensusParams().Version.AppVersion)
return min(govParam, upperBound)
}

type BlobKeeper interface {
GovMaxSquareSize(ctx sdk.Context) uint64
govMax := int(app.BlobKeeper.GovMaxSquareSize(ctx))
hardMax := appconsts.SquareSizeUpperBound(app.AppVersion())
return min(govMax, hardMax)
}
3 changes: 1 addition & 2 deletions test/util/malicious/out_of_order_prepare.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package malicious
import (
"github.com/celestiaorg/celestia-app/v2/app"
"github.com/celestiaorg/celestia-app/v2/app/ante"
"github.com/celestiaorg/celestia-app/v2/app/squaresize"
"github.com/celestiaorg/celestia-app/v2/pkg/da"
"github.com/celestiaorg/go-square/shares"
abci "github.com/tendermint/tendermint/abci/types"
Expand Down Expand Up @@ -47,7 +46,7 @@ func (a *App) OutOfOrderPrepareProposal(req abci.RequestPrepareProposal) abci.Re

// build the square from the set of valid and prioritised transactions.
// The txs returned are the ones used in the square and block
dataSquare, txs, err := Build(txs, a.GetBaseApp().AppVersion(), squaresize.MaxEffective(sdkCtx, a.BlobKeeper), OutOfOrderExport)
dataSquare, txs, err := Build(txs, a.GetBaseApp().AppVersion(), a.MaxEffectiveSquareSize(sdkCtx), OutOfOrderExport)
if err != nil {
panic(err)
}
Expand Down
22 changes: 20 additions & 2 deletions x/blob/ante/blob_share_decorator.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package ante

import (
"github.com/celestiaorg/celestia-app/v2/app/squaresize"
"github.com/celestiaorg/celestia-app/v2/pkg/appconsts"
blobtypes "github.com/celestiaorg/celestia-app/v2/x/blob/types"
"github.com/celestiaorg/go-square/shares"

Expand Down Expand Up @@ -42,14 +42,32 @@ func (d BlobShareDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool

// getMaxBlobShares returns the max the number of shares available for blob data.
func (d BlobShareDecorator) getMaxBlobShares(ctx sdk.Context) int {
squareSize := squaresize.MaxEffective(ctx, d.k)
squareSize := d.getMaxSquareSize(ctx)
totalShares := squareSize * squareSize
// The PFB tx share must occupy at least one share so the number of blob shares
// is at most one less than totalShares.
blobShares := totalShares - 1
return blobShares
}

// getMaxSquareSize returns the maximum square size based on the current values
// for the governance parameter and the versioned constant.
func (d BlobShareDecorator) getMaxSquareSize(ctx sdk.Context) int {
// TODO: fix hack that forces the max square size for the first height to
// 64. This is due to our fork of the sdk not initializing state before
// BeginBlock of the first block. This is remedied in versions of the sdk
// and comet that have full support of PreparePropsoal, although
// celestia-app does not currently use those. see this PR for more details
// https://github.com/cosmos/cosmos-sdk/pull/14505
if ctx.BlockHeader().Height <= 1 {
return int(appconsts.DefaultGovMaxSquareSize)
}

upperBound := appconsts.SquareSizeUpperBound(ctx.BlockHeader().Version.App)
govParam := d.k.GovMaxSquareSize(ctx)
return min(upperBound, int(govParam))
}

// getSharesNeeded returns the total number of shares needed to represent all of
// the blobs described by blobSizes.
func getSharesNeeded(blobSizes []uint32) (sum int) {
Expand Down
6 changes: 1 addition & 5 deletions x/blob/keeper/params.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,7 @@ func (k Keeper) GasPerBlobByte(ctx sdk.Context) (res uint32) {
return res
}

// GovMaxSquareSize returns the GovMaxSquareSize param.
//
// Note: it is unlikely that you want to use this value directly because
// governance can modify it to be anything. Most use-cases will want to use
// squaresize.MaxEffective instead of GovMaxSquareSize.
// GovMaxSquareSize returns the GovMaxSquareSize param
func (k Keeper) GovMaxSquareSize(ctx sdk.Context) (res uint64) {
k.paramStore.Get(ctx, types.KeyGovMaxSquareSize, &res)
return res
Expand Down

0 comments on commit a8e3085

Please sign in to comment.