ADR square size independent message commitments

[nashqueue]

Resolvses #834

rendered

[rach-id]

Only some questions to understand more :D

[rach-id]

Nits

[rootulp]

Only non-blocking questions for my understanding. This is a really well written ADR, great work!

[MSevey]

I think this is very well written and the pictures are a great add :-)

Not sure I have enough context yet to really approve this yet, so will wait on @liamsi and/or @adlerjohn

[evan-forbes]

Nice! as commented below, regardless of the decision made for pursueing an implementation or not, I think we should merge this.

Left a few questions and comments. Thanks for writing this up so well, and including a lot of helpful diagrams. We should definitely continue thinking about this idea, such as even increasing the minimum square size to something quite large, which would help considerably. Since light clients don't have to sample rows that are completely tail padding, increasing the minimum square size could have very few downsides.

After very we get consistently very large blocks, we could increase the minimum size to something so large that most rollups will not take up an entire row. In which case they will only ever have a single commitment anyway.

I think we should also note in this somewhere that this doesn't actually have to be done by the celestia-app logic, the rollup can do this themselves if they only ever want a single commitment.

What are the benefits from having celestia-app perform this logic rather than having rollups choose to do this?

[evan-forbes]

imo, making a decision on the status of this should not be a blocker for merging. We should merge it regardless.

[nashqueue]

@musalbas

[evan-forbes]

I think we should also note in this somewhere that this doesn't actually have to be done by the celestia-app logic, the rollup can do this themselves if they only ever want a single commitment.
What are the benefits from having celestia-app perform this logic rather than having rollups choose to do this?

I think this is the only remaining thing from my end

[evan-forbes]

Its also important to note that while this ADR has great info how this new scheme would work, it does not included details on how this would actually be implemented.

Per #835 (comment), removing malleation and non-interactive defaults in favor of this proposal would essentially be a re-write, and dramatic simplification, of the app. We would be able to remove the malleation process (and therefore MsgWirePayForMessage) and use a simpler version of the non-interactive defaults.

Instead of following the current non-interactive defaults, where we start each message at the next aligned power of two, we would start the message at the next index that is divisible by the minimum block size. This would likely require less padding for most square arrangements.

fwiw, I'm continually liking this idea more and more. Increasing the minimum block size would dramatically reduce the number of subtree roots needed.

[rootulp]

Sorry for the pedantic proposal to rename block size to square size. Given we already use square size to describe the dimensions of the data square, I think this rename may make it slightly easier to discuss the proposal. Additionally, the block includes information outside the data square.

[nashqueue]

Instead of following the current non-interactive defaults, where we start each message at the next aligned power of two, we would start the message at the next index that is divisible by the minimum block size. This would likely require less padding for most square arrangements.

Should i make a diagram for that ?

[rootulp]

Should i make a diagram for that ?

Sorry for repeating but my comment isn't blocking. In other words, I think a section on non-interactive defaults would help but doesn't block this PR. If you share the software you used to generate the diagrams, I can try drafting one too

[evan-forbes]

Should i make a diagram for that ?

I think its fine. We should remember to update the status of this ADR after we complete the implementation, and potentially update the naming after the implementation as well.

[evan-forbes]

w/o further feedback, imo we should merge this on monday

[rootulp]

and potentially update the naming

Specifically the remaining renames for block size => square size are the PR title and Markdown heading

[rootulp]

w/o further feedback, imo we should merge this on monday

Agreed. I'm a day late but unless there are objections @nashqueue @evan-forbes , let's mergeee