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

Use real byte size limits #3409

Merged
merged 1 commit into from
Oct 22, 2021
Merged

Use real byte size limits #3409

merged 1 commit into from
Oct 22, 2021

Conversation

coot
Copy link
Contributor

@coot coot commented Oct 5, 2021

Fixes #1727.

@coot coot requested a review from nfrisby as a code owner October 5, 2021 11:29
@coot coot requested a review from karknu October 5, 2021 11:29
@coot coot added consensus issues related to ouroboros-consensus networking labels Oct 5, 2021
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

This looks very reasonable to me. Let's discuss it at the Planning Meeting (briefly, I hope) on the 12th.

I wrote one comment that possibly suggests a non-trivial simplification.

Also, this PR's current logic boils down to

some mini protocol states will reject a message of more than 64 kilobytes and others will reject a message of more than 2,500,000 bytes.

Is that right? That seems fine for Cardano mainnet. But is it a fit for other (potential) users of this library? If not, then we ultimately need to make those numbers depend on eg the block type blk.

@coot
Copy link
Contributor Author

coot commented Oct 8, 2021

Is that right? That seems fine for Cardano mainnet. But is it a fit for other (potential) users of this library? If not, then we ultimately need to make those numbers depend on eg the block type blk.

Block size is a parameter of the network and yes these limits should ultimately be configurable rather than hard coded. But block size is independent of block type (though tx size, probably does depend on tx type). So ultimately, these limits should be provided by a cardano-node configuration file (or hard coded there).

I think this makes it clear why we should keep the interface proposed in this PR. Note also timeouts are different, since they are much less conservative than size limits, but in long run we will also need to make them configurable.

Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

Marcin and I reached consensus on a Consensus Planning Meeting call. Approving.

Copy link
Contributor

@karknu karknu left a comment

Choose a reason for hiding this comment

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

LGTM

@coot
Copy link
Contributor Author

coot commented Oct 21, 2021

bors merge

iohk-bors bot added a commit that referenced this pull request Oct 21, 2021
3409: Use real byte size limits r=coot a=coot

Fixes #1727.


Co-authored-by: Marcin Szamotulski <profunctor@pm.me>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 21, 2021

Build failed:

@coot
Copy link
Contributor Author

coot commented Oct 22, 2021

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 22, 2021

@iohk-bors iohk-bors bot merged commit 1f4973f into master Oct 22, 2021
@iohk-bors iohk-bors bot deleted the coot/size-limits branch October 22, 2021 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add per state size limits in Ouroboros-Network
3 participants