docs: ADR021 Restricted Block Size #1759

evan-forbes · 2023-05-12T23:31:16Z

Overview

Decided to document a synchronous discussion, and open (a hopefully quick) discussion while I implement two options to choose from (option 2 and 3)

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

rootulp

Thanks a ton for documenting the context around this decision! Do you think an additional section for "Goals" or "Desirable criteria" would be helpful?

My opinion is based on the desirable criteria:

Do we want the square size to be governance modifiable?

if yes we go with option 2 (i.e. GovMaxSquareSize)
if no we set MaxSquareSize=64 and at a later date (i.e. when pruning is implemented) we hard-fork to increase MaxSquareSize

liamsi · 2023-05-15T12:37:06Z

Do we want the square size to be governance modifiable?

In my opinion, the answer to this is:
Yes, we want it to be modifiable via governance but only to some reasonable upper cap. What is reasonable here depends on what we can increase the block size to without hitting tendermint's bottlenecks. The main reason we want to keep the block size lower than what is possible from that angle initially is that node hasn't pruning implemented properly.

evan-forbes · 2023-05-16T13:56:57Z

to quickly update this from the sync, we are moving forward with option 2.

option 3 is implemented and would work, however, it introduces more implicit complexity. Option 2 is more explicit and maximally flexible (we can have large squares and small blocks).

…ze (#1761) ## Overview The base implementation for all ADR021 #1759 options part of #1592 blocked by #1754 ## Checklist - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [ ] Linked issues closed with keywords

rootulp · 2023-05-17T14:46:35Z

docs/architecture/adr-021-restricted-block-size.md

+
+Currently, block sizes are controlled by three values, the smallest of which will determine what the protocol considers to be a valid block size.
+
+- `MaxBlockBytes` is a hard coded constant to the maximum total size of the protobuf encoded block, and therefore acts as a cap for `MaxBytes`. Currently set to 100 MB.


[question] where is MaxBlockBytes defined? I can't find it in

https://github.com/search?q=repo%3Acelestiaorg%2Fcelestia-app+MaxBlockBytes&type=code

https://github.com/search?q=repo%3Acelestiaorg%2Fcelestia-core+MaxBlockBytes&type=code

https://github.com/search?q=repo%3Acometbft%2Fcometbft+MaxBlockBytes&type=code

It's not obvious that MaxBytes and MaxBlockBytes are related and one is governance modifiable. Maybe we consider adopting a similar naming strategy as MaxSquareSize and GovMaxSquareSize because those are clearly related. In other words should we consider preserving MaxBlockBytes and renaming MaxBytes => GovMaxBlockBytes? My hunch is that the diff it will introduce with cometbft is not worth the extra clarity in naming.

yeah, the only reason that I didn't change them is because MaxBytes is a consensus param, which changing the name might break a few things in the sdk or explorers. We could rename the constant in core

and ahh whoops, the constant is MaxBlockSizeBytes, will update

MSevey · 2023-05-18T15:42:39Z

i wasn't very involved with these discussions so will defer to others.

rootulp

rootulp · 2023-05-19T21:39:57Z

docs/architecture/adr-021-restricted-block-size.md

+| Parameter        | Size   | Unit   | Description  | Value Control |
+|------------------|--------|--------|--------------|---------------|
+| `MaxBlockSizeBytes` | 100 | MiB | Maximum total size of the protobuf encoded block, a hard coded constant acting as a cap for `MaxBytes`. | Hard coded |
+| `MaxBytes`     |  21 | MiB | Determines the valid size of the entire protobuf encoded block. Is a governance modifiable parameter and is capped by `MaxBytes`. Used to regulate the amount of data gossiped in the consensus portion of the network (using the current block gossiping mechanism). | Modifiable |


[nit]

Suggested change

| `MaxBytes` | 21 | MiB | Determines the valid size of the entire protobuf encoded block. Is a governance modifiable parameter and is capped by `MaxBytes`. Used to regulate the amount of data gossiped in the consensus portion of the network (using the current block gossiping mechanism). | Modifiable |

| `MaxBytes` | 21 | MiB | Determines the maximum size of the entire protobuf encoded block. Is a governance modifiable parameter. Used to regulate the amount of data gossiped in the consensus portion of the network (using the current block gossiping mechanism). | Modifiable |

rootulp · 2023-05-19T21:45:14Z

docs/architecture/adr-021-restricted-block-size.md

+
+The second suggestion is to create a new governance parameter,
+`GovMaxSquareSize`, which governance could set. Now the protocol would use the
+lowest of 4 values to determine the size of a block at a given height. This has


Now the protocol would use the
lowest of 4 values to determine the size of a block at a given height

[no change needed] I interpret MaxBlockSizeBytes as a maximum value for MaxBytes. Similarly, I interpret MaxSquareSize as a maximum value for GovMaxSquareSize.

When evaluating if a square is valid, I expect that the proposed block only needs to be compared with MaxSquareSize and GovMaxSquareSize and doesn't need to be compared with the caps for those values because the param proposal that attempts to change MaxBytes or GovMaxSquareSize above their respective caps should fail.

tendermint will check first using both the bytes params no matter what, and then we check using the gov param in prepare/process proposal. So we actually end up using the lowest value of all 4 each height

rootulp · 2023-05-19T21:47:44Z

docs/architecture/adr-021-restricted-block-size.md

+| `MaxBlockSizeBytes` | 100 | MiB | Maximum total size of the protobuf encoded block, a hard coded constant acting as a cap for `MaxBytes`. | Hard coded |
+| `MaxBytes`     |  ~1.8 | MiB | Determines the valid size of the entire protobuf encoded block. Is a governance modifiable parameter and is capped by `MaxBytes`. Used to regulate the amount of data gossiped in the consensus portion of the network (using the current block gossiping mechanism). | Modifiable |
+| `MaxSquareSize` | ~7.5 | MiB | Determines the maximum size of the original data square. Used to regulate storage requirements for celestia-node, and the size of the data availability header. Default set to 128. | Hard coded |
+| `GovMaxSquareSize`     |  ~1.8 | MiB | Governance modifiable parameter that determines valid square sizes. Must be smaller than the `MaxSquareSize`. Default set to 64. | Modifiable |


merging for the sake of getting this through, but this does make sense to change

liamsi

💪🎉👏

codecov-commenter · 2023-05-24T15:32:08Z

Codecov Report

Merging #1759 (8020a4f) into main (2d70d7a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1759   +/-   ##
=======================================
  Coverage   21.84%   21.84%           
=======================================
  Files         116      116           
  Lines       13232    13232           
=======================================
  Hits         2890     2890           
  Misses      10050    10050           
  Partials      292      292

docs: ADR021 Restricted Block Size

4bc0fc5

evan-forbes added the ADR item is directly relevant to writing or modifying an ADR label May 12, 2023

evan-forbes added this to the Mainnet milestone May 12, 2023

evan-forbes requested a review from cmwaters May 12, 2023 23:31

evan-forbes self-assigned this May 12, 2023

evan-forbes requested review from liamsi, adlerjohn, MSevey and rootulp as code owners May 12, 2023 23:31

MSevey requested a review from a team May 12, 2023 23:31

docs: add link to implementation and test of option 1

8bb622f

evan-forbes mentioned this pull request May 13, 2023

chore!: set default consensus params and prep for restricted block size #1761

Merged

5 tasks

rootulp reviewed May 14, 2023

View reviewed changes

evan-forbes mentioned this pull request May 14, 2023

feat!: use consensus.MaxBytes to determine GovMaxSquareSize (option 3 from ADR021) #1765

Closed

5 tasks

chore: accepted

d4fa3f5

rootulp reviewed May 17, 2023

View reviewed changes

rootulp removed the request for review from MSevey May 18, 2023 16:05

evan-forbes added 2 commits May 18, 2023 12:47

Merge branch 'main' into evan/adr-021-restricted-block-size

a6fc49e

chore: flesh out after implementation

fd12378

MSevey requested a review from a team May 18, 2023 20:36

evan-forbes and others added 2 commits May 18, 2023 15:39

chore: docs

baef8b8

Merge branch 'main' into evan/adr-021-restricted-block-size

26d55fe

rootulp approved these changes May 19, 2023

View reviewed changes

liamsi approved these changes May 21, 2023

View reviewed changes

Merge branch 'main' into evan/adr-021-restricted-block-size

8020a4f

MSevey requested a review from a team May 24, 2023 15:19

evan-forbes merged commit a9a0d36 into main May 24, 2023

evan-forbes deleted the evan/adr-021-restricted-block-size branch May 24, 2023 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: ADR021 Restricted Block Size #1759

docs: ADR021 Restricted Block Size #1759

evan-forbes commented May 12, 2023 •

edited

Loading

rootulp left a comment

liamsi commented May 15, 2023

evan-forbes commented May 16, 2023

rootulp May 17, 2023

evan-forbes May 17, 2023

MSevey commented May 18, 2023

rootulp left a comment

rootulp May 19, 2023

rootulp May 19, 2023

evan-forbes May 24, 2023

rootulp May 19, 2023

evan-forbes May 24, 2023

liamsi left a comment

codecov-commenter commented May 24, 2023


		Currently, block sizes are controlled by three values, the smallest of which will determine what the protocol considers to be a valid block size.

		- `MaxBlockBytes` is a hard coded constant to the maximum total size of the protobuf encoded block, and therefore acts as a cap for `MaxBytes`. Currently set to 100 MB.

	\| `MaxBytes` \| 21 \| MiB \| Determines the valid size of the entire protobuf encoded block. Is a governance modifiable parameter and is capped by `MaxBytes`. Used to regulate the amount of data gossiped in the consensus portion of the network (using the current block gossiping mechanism). \| Modifiable \|
	\| `MaxBytes` \| 21 \| MiB \| Determines the maximum size of the entire protobuf encoded block. Is a governance modifiable parameter. Used to regulate the amount of data gossiped in the consensus portion of the network (using the current block gossiping mechanism). \| Modifiable \|

	\| `GovMaxSquareSize` \| ~1.8 \| MiB \| Governance modifiable parameter that determines valid square sizes. Must be smaller than the `MaxSquareSize`. Default set to 64. \| Modifiable \|
	\| `GovMaxSquareSize` \| ~1.8 \| MiB \| Governance modifiable parameter that determines the max valid square size. Must be smaller than the `MaxSquareSize`. Default set to 64. \| Modifiable \|

docs: ADR021 Restricted Block Size #1759

docs: ADR021 Restricted Block Size #1759

Conversation

evan-forbes commented May 12, 2023 • edited Loading

Overview

Checklist

rootulp left a comment

Choose a reason for hiding this comment

liamsi commented May 15, 2023

evan-forbes commented May 16, 2023

rootulp May 17, 2023

Choose a reason for hiding this comment

evan-forbes May 17, 2023

Choose a reason for hiding this comment

MSevey commented May 18, 2023

rootulp left a comment

Choose a reason for hiding this comment

rootulp May 19, 2023

Choose a reason for hiding this comment

rootulp May 19, 2023

Choose a reason for hiding this comment

evan-forbes May 24, 2023

Choose a reason for hiding this comment

rootulp May 19, 2023

Choose a reason for hiding this comment

evan-forbes May 24, 2023

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 24, 2023

Codecov Report

evan-forbes commented May 12, 2023 •

edited

Loading