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.

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

doc: improve documentation of BlockParams.MaxBytes #1405

Merged
merged 10 commits into from
Sep 28, 2023
Merged

Conversation

cason
Copy link
Contributor

@cason cason commented Sep 27, 2023

Additions to the description of the BlockParams.MaxBytes consensus parameter on the ABCI documentation.

Rendered: https://github.com/cometbft/cometbft/blob/cason/doc-maxbytes-param/spec/abci/abci%2B%2B_app_requirements.md

PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments

@cason cason changed the title spec: improve documentation of BlockParams.MaxBytes doc: improve documentation of BlockParams.MaxBytes Sep 27, 2023
Co-authored-by: Sergio Mena <sergio@informal.systems>
Co-authored-by: Sergio Mena <sergio@informal.systems>
@cason cason marked this pull request as ready for review September 27, 2023 15:29
@cason cason requested review from a team as code owners September 27, 2023 15:29
@cason cason added backport-to-v0.37.x Tell Mergify to backport the PR to v0.37.x backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x documentation Improvements or additions to documentation labels Sep 27, 2023
@cason cason self-assigned this Sep 27, 2023
Copy link
Contributor

@sergio-mena sergio-mena left a comment

Choose a reason for hiding this comment

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

🙏

Co-authored-by: glnro <8335464+glnro@users.noreply.github.com>
@@ -615,6 +622,13 @@ If the Application sets value -1, consensus will:

Must have `MaxBytes == -1` OR `0 < MaxBytes <= 100 MB`.
Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly this -1 was introduced by @sergio-mena to allow for arbitrary block size or is there some other reason? I know that in 0.34.x it is not allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is probably only on main. 34 and 37 not for sure, v0.38 I'll have to double check when backporting.

Copy link
Contributor

@sergio-mena sergio-mena Sep 27, 2023

Choose a reason for hiding this comment

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

IIRC, this is on main, v0.38.x, and v0.37x (as it tackles a corner case on PrepareProposal), but I might be mistaken

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In v0.37.x it is not implemented at consensus level, from this extract:

https://github.com/cometbft/cometbft/blob/v0.37.x/consensus/state.go#L1937

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In v0.38.x it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

So my memories weren't accurate

@cason cason added this pull request to the merge queue Sep 28, 2023
Merged via the queue into main with commit 80648c4 Sep 28, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-v0.37.x Tell Mergify to backport the PR to v0.37.x backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants