-
Notifications
You must be signed in to change notification settings - Fork 278
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
feat: use square size params #1126
feat: use square size params #1126
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just have a few trivial comments.
A slightly adjacent question I have with these upper and lower square size bounds is how are they initially calculated and how do we know when it's time to change them?
pkg/da/data_availability_header.go
Outdated
func ExtendShares(squareSize uint64, shares [][]byte, minSquareSize, maxSquareSize uint64) (*rsmt2d.ExtendedDataSquare, error) { | ||
// Check that square size is with range | ||
if squareSize < appconsts.DefaultMinSquareSize || squareSize > appconsts.DefaultMaxSquareSize { | ||
if squareSize < minSquareSize || squareSize > maxSquareSize { | ||
return nil, fmt.Errorf( | ||
"invalid square size: min %d max %d provided %d", | ||
appconsts.DefaultMinSquareSize, | ||
appconsts.DefaultMaxSquareSize, | ||
minSquareSize, | ||
maxSquareSize, | ||
squareSize, | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought here, but I feel like verification of the square size and extending the shares should be two separate functions. In PrepareProposal
, estimateSquareSize
already guarantees that the square size is within the expected range.
Also the comparison of squareSize*squareSize == len(shares)
should probably be done in ComputeExtendedDataSquare
.
Co-authored-by: Rootul P <rootulp@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] why aren't unit tests running on CI for this PR? I checked it out locally and I'm observing a test failure
--- FAIL: TestBlobInclusionCheck (0.03s)
/Users/rootulp/git/rootulp/celestia/celestia-app/app/test/process_proposal_test.go:124:
Error Trace: /Users/rootulp/git/rootulp/celestia/celestia-app/app/test/process_proposal_test.go:124
Error: Not equal:
expected: 1
actual : 2
Test: TestBlobInclusionCheck
Messages: valid untouched data
FAIL
FAIL github.com/celestiaorg/celestia-app/app/test 0.534s
FAIL
# Conflicts: # app/estimate_square_size.go # app/prepare_proposal.go # x/blob/types/payforblob.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any blocking feedback but there is one unresolved blocking question on this PR: https://github.com/celestiaorg/celestia-app/pull/1126/files#r1049690184 so holding off on approval
if ctx.BlockHeader().Height < 1 { | ||
return appconsts.DefaultMinSquareSize | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Codecov Report
@@ Coverage Diff @@
## main #1126 +/- ##
==========================================
- Coverage 47.98% 47.90% -0.09%
==========================================
Files 71 71
Lines 3995 4025 +30
==========================================
+ Hits 1917 1928 +11
- Misses 1909 1927 +18
- Partials 169 170 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
I'm going to create an issue on the cosmos-sdk repository to get their opinion |
if ctx.BlockHeader().Height < 1 { | ||
return appconsts.DefaultMinSquareSize | ||
} |
There was a problem hiding this comment.
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.
@@ -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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
?
it looks like others have ran into this issue as well |
This might be a fix for this: cosmos/cosmos-sdk#14505 would love to get some more eyes on it.
EDIT: Didn't mean "this" as the PR but as the problems pointed out in the comments. Feel free to ping me |
Is there any update on this PR? Can we either use the hack where we use the default size for the first block or we cherry-pick the commit that facundo did. |
I think the decision was to not pursue this change, so I think we can close this issue and PR. IIRC, the reasoning was that while we want something like BlockSize (determined in MB), we don't want to expose square sizes as those are more complex internal params. We'd also still have a "HardMaxSquareSize", that we'd have to hardfork to change, so it seems a bit extra to have a blocksize param along with a max square size param along with a hard max square size |
In that case can we look to either increase the current max square size from 128 to 256 i.e. 32MB square size or reduce DefaultMaxBlockBytes to 4MB. What I'm trying to get at is that the ceiling in terms of size should be something we control (i.e. MaxBlockBytes) and not something we don't (i.e. MaxSquareSize) |
closing since we're simply using the existing block size limits in tendermint |
Removes dependence of
appconsts.DefaultMinSquareSize
andappconsts.DefaultMaxSquareSize
and instead fetches the relevant params from the blob keeper during process proposal