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.

Sign up for GitHub

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

Jump to bottom

feat: use square size params #1126

Closed
wants to merge 22 commits into from
Closed

feat: use square size params #1126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
296d26c
chore: remove unused constant
rahulghangas Dec 15, 2022
74253c3
feat: remove dependence on constant vals for min/max sqaure size
rahulghangas Dec 15, 2022
ef6ecb5
feat: remove dependence on constant vals for min/max sqaure size
rahulghangas Dec 15, 2022
deb9035
feat: pull params from blob module keeper
rahulghangas Dec 15, 2022
e656e36
test: refactor test to use new interface
rahulghangas Dec 15, 2022
e3d33af
Update pkg/da/data_availability_header.go
rahulghangas Dec 19, 2022
4e4e4ad
feat: add function for checking if square size is valid
rahulghangas Dec 19, 2022
35820d4
chore: remove validity check for square size while creating extended …
rahulghangas Dec 19, 2022
6d7f9f0
chore: remove extra parameters in call to ExtendShares
rahulghangas Dec 19, 2022
4406e4a
test: remove extra parameters in call to ExtendShares
rahulghangas Dec 19, 2022
20a72aa
feat: add square size check as a block validity rule
rahulghangas Dec 19, 2022
2888fbe
test: check rejection on incorrect block size
rahulghangas Dec 19, 2022
90b624b
fix: incorrect logic for square size validity check
rahulghangas Dec 19, 2022
bc82ef6
chore: typo
rahulghangas Dec 19, 2022
012666a
Merge branch 'main' into feat/use-squaresize-params
rahulghangas Dec 19, 2022
9a06093
chore: convert uint32 to uint64
rahulghangas Dec 19, 2022
eaabd6c
test: refactor test to use updated function signature
rahulghangas Dec 19, 2022
10b7e34
test: remove now invalid test
rahulghangas Dec 19, 2022
076b9ad
fix: return default values for min/max square size if not block has b…
rahulghangas Dec 20, 2022
286eaaa
test: test rejection of process proposal on square size less than min…
rahulghangas Dec 20, 2022
0fca546
Merge branch 'main' into feat/use-squaresize-params
rahulghangas Dec 20, 2022
0a296c1
chore: add documentation for edge case of fetching min/max square size
rahulghangas Dec 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions app/estimate_square_size.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import (
//
// NOTE: The estimation process does not have to be perfect. We can overestimate
// because the cost of padding is limited.
func estimateSquareSize(txs []parsedTx) (squareSize uint64, nonreserveStart int) {
func estimateSquareSize(txs []parsedTx, minSquareSize, maxSquareSize uint64) (squareSize uint64, nonreserveStart int) {
txSharesUsed := estimateCompactShares(appconsts.DefaultMaxSquareSize, txs)
blobSharesUsed := 0

Expand All @@ -35,11 +35,11 @@ func estimateSquareSize(txs []parsedTx) (squareSize uint64, nonreserveStart int)
totalSharesUsed *= 2
minSize := uint64(math.Sqrt(float64(totalSharesUsed)))
squareSize = shares.RoundUpPowerOfTwo(minSize)
if squareSize >= appconsts.DefaultMaxSquareSize {
squareSize = appconsts.DefaultMaxSquareSize
if squareSize >= maxSquareSize {
squareSize = maxSquareSize
}
if squareSize <= appconsts.DefaultMinSquareSize {
squareSize = appconsts.DefaultMinSquareSize
if squareSize <= minSquareSize {
squareSize = minSquareSize
}

return squareSize, txSharesUsed
Expand Down Expand Up @@ -88,3 +88,8 @@ func maxWrappedTxOverhead(squareSize uint64) int {
}
return len(wtx) - int(maxTxLen)
}

func isValidSquareSize(squareSize, minSquareSize, maxSquareSize int) bool {
// Check that square size is within range
return squareSize >= minSquareSize && squareSize <= maxSquareSize
}
2 changes: 1 addition & 1 deletion app/estimate_square_size_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func Test_estimateSquareSize(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ptxs := generateMixedParsedTxs(tt.normalTxs, tt.pfbCount, tt.pfbSize)
res, _ := estimateSquareSize(ptxs)
res, _ := estimateSquareSize(ptxs, appconsts.DefaultMinSquareSize, appconsts.DefaultMaxSquareSize)
assert.Equal(t, tt.expectedSquareSize, res)
})
}
Expand Down
5 changes: 4 additions & 1 deletion app/prepare_proposal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ func (app *App) PrepareProposal(req abci.RequestPrepareProposal) abci.ResponsePr

// estimate the square size. This estimation errs on the side of larger
// squares but can only return values within the min and max square size.
squareSize, nonreservedStart := estimateSquareSize(parsedTxs)
ctx := app.NewContext(true, core.Header{Height: app.LastBlockHeight()})
rahulghangas marked this conversation as resolved.
Show resolved Hide resolved
minSquareSize := app.BlobKeeper.MinSquareSize(ctx)
maxSquareSize := app.BlobKeeper.MaxSquareSize(ctx)
rahulghangas marked this conversation as resolved.
Show resolved Hide resolved
squareSize, nonreservedStart := estimateSquareSize(parsedTxs, uint64(minSquareSize), uint64(maxSquareSize))

// finalizeLayout wraps any blob transactions with their final share index.
// This requires sorting the blobs by namespace and potentially pruning
Expand Down
22 changes: 21 additions & 1 deletion app/process_proposal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package app

import (
"bytes"
"fmt"
"sort"

"github.com/celestiaorg/celestia-app/pkg/appconsts"
Expand All @@ -27,6 +28,10 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) abci.ResponsePr
// - the commitment in each PFB should match the commitment for the shares that contain that blob data
// - there should be no unpaid-for data

ctx := app.NewContext(true, tmproto.Header{Height: app.LastBlockHeight()})
minSquareSize := app.BlobKeeper.MinSquareSize(ctx)
maxSquareSize := app.BlobKeeper.MaxSquareSize(ctx)

data, err := coretypes.DataFromProto(req.BlockData)
if err != nil {
logInvalidPropBlockError(app.Logger(), req.Header, "failure to unmarshal block data:", err)
Expand All @@ -35,6 +40,21 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) abci.ResponsePr
}
}

squareSize := data.SquareSize

// validate proposed square size
if !isValidSquareSize(int(squareSize), int(minSquareSize), int(maxSquareSize)) {
rahulghangas marked this conversation as resolved.
Show resolved Hide resolved
logInvalidPropBlockError(app.Logger(), req.Header, "invalid square size:", fmt.Errorf(
"min %d max %d provided %d",
minSquareSize,
maxSquareSize,
squareSize,
))
return abci.ResponseProcessProposal{
Result: abci.ResponseProcessProposal_REJECT,
}
}

if !sort.IsSorted(coretypes.BlobsByNamespace(data.Blobs)) {
logInvalidPropBlock(app.Logger(), req.Header, "blobs are unsorted")
return abci.ResponseProcessProposal{
Expand All @@ -50,7 +70,7 @@ func (app *App) ProcessProposal(req abci.RequestProcessProposal) abci.ResponsePr
}
}

cacher := inclusion.NewSubtreeCacher(data.SquareSize)
cacher := inclusion.NewSubtreeCacher(squareSize)
eds, err := rsmt2d.ComputeExtendedDataSquare(shares.ToBytes(dataSquare), appconsts.DefaultCodec(), cacher.Constructor)
if err != nil {
logInvalidPropBlockError(app.Logger(), req.Header, "failure to erasure the data square", err)
Expand Down
16 changes: 16 additions & 0 deletions app/test/process_proposal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,22 @@ func TestBlobInclusionCheck(t *testing.T) {
},
expectedResult: abci.ResponseProcessProposal_REJECT,
},
{
name: "square size greater than max square size",
input: validData(),
mutator: func(d *core.Data) {
d.SquareSize = appconsts.DefaultMaxSquareSize * 2
},
expectedResult: abci.ResponseProcessProposal_REJECT,
},
rahulghangas marked this conversation as resolved.
Show resolved Hide resolved
{
name: "square size of zero",
input: validData(),
mutator: func(d *core.Data) {
d.SquareSize = 0
},
expectedResult: abci.ResponseProcessProposal_REJECT,
},
}

for _, tt := range tests {
Expand Down
27 changes: 8 additions & 19 deletions pkg/da/data_availability_header.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,6 @@ import (
"github.com/tendermint/tendermint/types"
)

const (
maxExtendedSquareWidth = appconsts.DefaultMaxSquareSize * 2
minExtendedSquareWidth = appconsts.DefaultMinSquareSize * 2
)

// DataAvailabilityHeader (DAHeader) contains the row and column roots of the
// erasure coded version of the data in Block.Data. The original Block.Data is
// split into shares and arranged in a square of width squareSize. Then, this
Expand Down Expand Up @@ -49,16 +44,8 @@ func NewDataAvailabilityHeader(eds *rsmt2d.ExtendedDataSquare) DataAvailabilityH
return dah
}

// ExtendShares generates an extended data square from given square size and shares
func ExtendShares(squareSize uint64, shares [][]byte) (*rsmt2d.ExtendedDataSquare, error) {
// Check that square size is with range
if squareSize < appconsts.DefaultMinSquareSize || squareSize > appconsts.DefaultMaxSquareSize {
return nil, fmt.Errorf(
"invalid square size: min %d max %d provided %d",
appconsts.DefaultMinSquareSize,
appconsts.DefaultMaxSquareSize,
squareSize,
)
}
// check that valid number of shares have been provided
if squareSize*squareSize != uint64(len(shares)) {
return nil, fmt.Errorf(
Expand Down Expand Up @@ -115,7 +102,7 @@ func (dah *DataAvailabilityHeader) ToProto() (*daproto.DataAvailabilityHeader, e
return dahp, nil
}

func DataAvailabilityHeaderFromProto(dahp *daproto.DataAvailabilityHeader) (dah *DataAvailabilityHeader, err error) {
func DataAvailabilityHeaderFromProto(dahp *daproto.DataAvailabilityHeader, minSquareSize, maxSquareSize int) (dah *DataAvailabilityHeader, err error) {
if dahp == nil {
return nil, errors.New("nil DataAvailabilityHeader")
}
Expand All @@ -124,11 +111,13 @@ func DataAvailabilityHeaderFromProto(dahp *daproto.DataAvailabilityHeader) (dah
dah.RowsRoots = dahp.RowRoots
dah.ColumnRoots = dahp.ColumnRoots

return dah, dah.ValidateBasic()
return dah, dah.ValidateBasic(minSquareSize, maxSquareSize)
}

// ValidateBasic runs stateless checks on the DataAvailabilityHeader.
func (dah *DataAvailabilityHeader) ValidateBasic() error {
func (dah *DataAvailabilityHeader) ValidateBasic(minSquareSize, maxSquareSize int) error {
minExtendedSquareWidth := minSquareSize * 2
maxExtendedSquareWidth := maxSquareSize * 2
if dah == nil {
return errors.New("nil data availability header is not valid")
}
Expand Down Expand Up @@ -174,9 +163,9 @@ var tailPaddingShare = append(

// MinDataAvailabilityHeader returns the minimum valid data availability header.
// It is equal to the data availability header for an empty block
func MinDataAvailabilityHeader() DataAvailabilityHeader {
func MinDataAvailabilityHeader(minSquareSize int) DataAvailabilityHeader {
shares := GenerateEmptyShares(appconsts.MinShareCount)
eds, err := ExtendShares(appconsts.DefaultMinSquareSize, shares)
eds, err := ExtendShares(uint64(minSquareSize), shares)
if err != nil {
panic(err)
}
Expand Down
22 changes: 8 additions & 14 deletions pkg/da/data_availability_header_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,13 @@ func TestNilDataAvailabilityHeaderHashDoesntCrash(t *testing.T) {
}

func TestMinDataAvailabilityHeader(t *testing.T) {
dah := MinDataAvailabilityHeader()
dah := MinDataAvailabilityHeader(appconsts.DefaultMinSquareSize)
expectedHash := []byte{
0x6f, 0x52, 0xda, 0xc1, 0x65, 0x45, 0xe4, 0x57, 0x25, 0xbe, 0x6e, 0xa3, 0x2a, 0xed, 0x55, 0x26,
0x6e, 0x45, 0x3, 0x48, 0x0, 0xee, 0xe1, 0xd8, 0x7c, 0x94, 0x28, 0xf4, 0x84, 0x4e, 0xa4, 0x7a,
}
require.Equal(t, expectedHash, dah.hash)
require.NoError(t, dah.ValidateBasic())
require.NoError(t, dah.ValidateBasic(appconsts.DefaultMinSquareSize, appconsts.DefaultMaxSquareSize))
// important note: also see the types.TestEmptyBlockDataAvailabilityHeader test
// which ensures that empty block data results in the minimum data availability
// header
Expand Down Expand Up @@ -85,12 +85,6 @@ func TestExtendShares(t *testing.T) {
}

tests := []test{
{
name: "too large square size",
expectedErr: true,
squareSize: appconsts.DefaultMaxSquareSize + 1,
shares: generateShares((appconsts.DefaultMaxSquareSize+1)*(appconsts.DefaultMaxSquareSize+1), 1),
},
{
name: "invalid number of shares",
expectedErr: true,
Expand Down Expand Up @@ -125,7 +119,7 @@ func TestDataAvailabilityHeaderProtoConversion(t *testing.T) {
tests := []test{
{
name: "min",
dah: MinDataAvailabilityHeader(),
dah: MinDataAvailabilityHeader(appconsts.DefaultMinSquareSize),
},
{
name: "max",
Expand All @@ -137,7 +131,7 @@ func TestDataAvailabilityHeaderProtoConversion(t *testing.T) {
tt := tt
pdah, err := tt.dah.ToProto()
require.NoError(t, err)
resDah, err := DataAvailabilityHeaderFromProto(pdah)
resDah, err := DataAvailabilityHeaderFromProto(pdah, appconsts.DefaultMinSquareSize, appconsts.DefaultMaxSquareSize)
require.NoError(t, err)
resDah.Hash() // calc the hash to make the comparisons fair
require.Equal(t, tt.dah, *resDah, tt.name)
Expand Down Expand Up @@ -170,16 +164,16 @@ func Test_DAHValidateBasic(t *testing.T) {
tooSmallDah.ColumnRoots = [][]byte{bytes.Repeat([]byte{2}, 32)}
tooSmallDah.RowsRoots = [][]byte{bytes.Repeat([]byte{2}, 32)}
// use a bad hash
badHashDah := MinDataAvailabilityHeader()
badHashDah := MinDataAvailabilityHeader(appconsts.DefaultMinSquareSize)
badHashDah.hash = []byte{1, 2, 3, 4}
// dah with not equal number of roots
mismatchDah := MinDataAvailabilityHeader()
mismatchDah := MinDataAvailabilityHeader(appconsts.DefaultMinSquareSize)
mismatchDah.ColumnRoots = append(mismatchDah.ColumnRoots, bytes.Repeat([]byte{2}, 32))

tests := []test{
{
name: "min",
dah: MinDataAvailabilityHeader(),
dah: MinDataAvailabilityHeader(appconsts.DefaultMinSquareSize),
},
{
name: "max",
Expand Down Expand Up @@ -213,7 +207,7 @@ func Test_DAHValidateBasic(t *testing.T) {

for _, tt := range tests {
tt := tt
err := tt.dah.ValidateBasic()
err := tt.dah.ValidateBasic(appconsts.DefaultMinSquareSize, appconsts.DefaultMaxSquareSize)
if tt.expectErr {
require.True(t, strings.Contains(err.Error(), tt.errStr), tt.name)
require.Error(t, err)
Expand Down
11 changes: 11 additions & 0 deletions x/blob/keeper/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper

import (
"github.com/celestiaorg/celestia-app/pkg/appconsts"
"github.com/celestiaorg/celestia-app/x/blob/types"
sdk "github.com/cosmos/cosmos-sdk/types"
)
Expand All @@ -21,12 +22,22 @@ func (k Keeper) SetParams(ctx sdk.Context, params types.Params) {

// MinSquareSize returns the MinSquareSize param
func (k Keeper) MinSquareSize(ctx sdk.Context) (res uint32) {
// use the default size for the first block so that we return a value on the
// first block in PrepareProposal
if ctx.BlockHeader().Height < 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure the first block is at height 1 not 0. Also we should be using something like InitialHeight as it's possible with Tendermint chains to start at any height.

Most importantly, I'm still not sure if this logic is necessary. At least I haven't seen this in other SDK modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure either. However, with this logic removed, prepare proposal (where the params are fetched for the first time) blocks and using the cli to query the params panics with an error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

chain logs

5:47PM INF Timed out dur=10000 height=1 module=consensus round=0 step=3
5:47PM INF Ensure peers module=pex numDialing=0 numInPeers=0 numOutPeers=0 numToDial=10
5:47PM INF No addresses to dial. Falling back to seeds module=pex
5:48PM INF Ensure peers module=pex numDialing=0 numInPeers=0 numOutPeers=0 numToDial=10
5:48PM INF No addresses to dial. Falling back to seeds module=pex

using celestia-appd query blob params results in

Error: rpc error: code = Unknown desc = UnmarshalJSON cannot decode empty bytes: panic

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Maybe you're right. I'd have thought the genesis params would be set at InitChain which comes before PrepareProposal but maybe I'm wrong.

What you could also do is add a log for when the params first get set in the Keeper and we can see if that comes before the first block gets proposed. Also wouldn't this panic you're seeing also happen in PrepareProposal?

return appconsts.DefaultMinSquareSize
}
Comment on lines +27 to +29
Copy link
Collaborator

Choose a reason for hiding this comment

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

[non blocking question] sorry I'm not familiar with defining genesis params. Is there a way to define the DefaultMinSquareSize param at chain genesis time so that we don't need this conditional?

If it isn't possible, why is a similar conditional not needed for the GasPerBlobByte param later in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We ran into this today. Apparently the genesis params aren't set until the first block is committed. We added this check to min/max square size only because those two are queried in prepare/process proposal.

p.s. surprisingly it wasn't panicking (at the first block) on being unable to unmarshal nil byte slices to relevant values, but rather just blocking forever

Copy link
Member

@evan-forbes evan-forbes Dec 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think it would be good to document this at least in the code, and then we might even want to document it elsewhere (but am unsure where)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible for someone to include a payForBlob in the first block of the chain? It seems unlikely but if yes, would GasPerBlobByte also need to be initialized?

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 point, I don't think so, as the params are used in many places, but I'm not sure of what is done to make that the case! perhaps we can do that process a bit earlier, so that it works for processproposal, which would almost certainly be a better solution

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently the genesis params aren't set until the first block is committed

This seems odd. You should be able to set params in genesis that can be used for the first block.

rahulghangas marked this conversation as resolved.
Show resolved Hide resolved
k.paramStore.Get(ctx, types.KeyMinSquareSize, &res)
return
}

// MaxSquareSize returns the MaxSquareSize param
func (k Keeper) MaxSquareSize(ctx sdk.Context) (res uint32) {
// use the default size for the first block so that we return a value on the
// first block in PrepareProposal
if ctx.BlockHeader().Height < 1 {
return appconsts.DefaultMaxSquareSize
}
k.paramStore.Get(ctx, types.KeyMaxSquareSize, &res)
return
}
Expand Down
1 change: 0 additions & 1 deletion x/blob/types/payforblob.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ const (
URLBlobTx = "/blob.BlobTx"
URLMsgPayForBlob = "/blob.MsgPayForBlob"
ShareSize = appconsts.ShareSize
SquareSize = appconsts.DefaultMaxSquareSize
NamespaceIDSize = appconsts.NamespaceSize
)

Expand Down