docs: add CIP for block limits on pfbs and tx size by jcstein · Pull Request #220 · celestiaorg/CIPs

jcstein · 2024-10-16T14:30:04Z

Overview

Resolves #219

rootulp · 2024-10-16T14:54:34Z

cips/cip-tx-limits.md

+1. The number of PFBs per block is limited to 600 by setting `PFBTransactionCap` to 600. `PFBTransactionCap` is the maximum number of PFB messages a block can contain.
+
+1. The number of non-PFB transactions per block is limited to 200 by setting `NonPFBTransactionCap` to 200. `NonPFBTransactionCap` is the maximum number of SDK messages, aside from PFBs, that a block can contain.


[optional] I think it's worth calling out that these limits aren't actually enforced. They're limits used by the celestia-app implementation but a validator could hypothetically modify prepare proposal, run that binary and create blocks with more PFBs or non-PFBs.

[nit] the constants are poorly named b/c the thing being limited is the number of messages in a block not the number of transactions. Should we change the implementation and the specs here? cc: @rach-id

@rootulp yes sure. want me to open a PR or you do it?

Created celestiaorg/celestia-app#3984

8fbee0e updated to new variables, if these don't get included with v3.0.0-rc, I will revert to names NonPFBTransactionCap and PFBTransactionCap

cips/cip-tx-limits.md

rootulp · 2024-10-16T14:58:49Z

cips/cip-tx-limits.md

+
+The rationale for this proposal is twofold:
+
+1. To prevent congestion on the network by limiting the number of PFBs and non-PFB transactions per block. This was initially not considered consensus-breaking, but it has a meaningful effect on users and should be formalized in a CIP.


[optional] I think specifically to prevent block times from spiking b/c if a block includes > 600 PFB messages and 200 non-PFB messages, the time to process the block expands past the 6 second time window. cc: @rach-id can clarify

Suggested change

1. To prevent congestion on the network by limiting the number of PFBs and non-PFB transactions per block. This was initially not considered consensus-breaking, but it has a meaningful effect on users and should be formalized in a CIP.

1. To prevent long block times on the network by limiting the number of PFBs and non-PFB transactions per block. This was initially not considered consensus-breaking, but it has a meaningful effect on users and should be formalized in a CIP.

cips/cip-tx-limits.md

Co-authored-by: Rootul P <rootulp@gmail.com>

cips/cip-tx-limits.md

Co-authored-by: nina / ნინა <barbakadzeninaa@gmail.com>

cips/cip-tx-limits.md

cmwaters · 2024-10-17T13:14:05Z

cips/cip-tx-limits.md

+
+The rationale for this proposal is twofold:
+
+1. To prevent long block times on the network by limiting the number of PFBs and non-PFBs messages per block. This was initially not considered consensus-breaking, but it has a meaningful effect on users and should be formalized in a CIP.


We can do this at a later point but it might be helpful to remember later on how these numbers were chosen

we could point to this PR celestiaorg/celestia-app#390 as I am adding the results to it and will open it for review afterwards

The PR 309 referenced is from deciding on the prefix for addresses and unrelated to this? celestiaorg/celestia-app#390

@rach-id Can you please provide a condensed explanation of what you learned in the PR that was intended to be linked? to explain why these values were chosen?

my bad, this PR: celestiaorg/celestia-app#3904

That PR contains benchmarks that we run to measure the processing time different ABCI methods take with different transaction types.

Then, to limit that processing time to ~0.25sec, we added a soft limiter in prepare proposal so that the default block created by the block construction mechanism respects that processing time limit.

This soft limiter is a simple cap on the number of messages a block can contain, based on running the PRs benchmarks on 4 CPU 16GB RAM machines, which is the suggested validator configuration.

This way, the default blocks don't take much time to process. However, if a block not respecting those limits reaches consensus, it still can be included and won't be wasted.

cips/cip-tx-limits.md

cmwaters · 2024-10-17T13:17:04Z

cips/cip-tx-limits.md

+
+1. It's important to note that these limits are not strictly enforced. While they are defined by the `celestia-app` implementation, a validator could potentially modify the `PrepareProposal` logic, run a custom binary, and produce blocks that exceed the specified limits for PFB or non-PFBs transactions.
+
+1. The size of a transaction is limited to 2MiB by setting `MaxTxSize` to 2097152, which is 2MiB in bytes. From version v3 and above, in `CheckTx`, `PrepareProposal`, and `ProcessProposal`, each transaction's size is checked against the `appconsts.MaxTxSize` threshold. This ensures that transactions over the limit are rejected or excluded at all stages, from initial submission to execution.


This first line also feels verbose. We should also point out that this is a versioned parameter

#220 (comment)

cips/cip-tx-limits.md

rootulp · 2024-10-17T17:21:50Z

cips/cip-tx-limits.md

@@ -0,0 +1,44 @@
+| cip | TBD(27) |
+| - | - |
+| title | Block limits for number of PFBs/non-PFBs and transaction size |


We discussed this in all core devs but IMO there wasn't a clear decision made in the call. I think we could split this into two CIPs:

Block limits for PFBs / non-PFBs

Limit transaction size

I think @cmwaters explanation is that these both impact how a block is constructed, so was okay to leave it as 1 CIP.

I'm also happy to scrap this and have it split to 2 CIPs by authors of the features. but I worry this will likely just add more delay to these being drafted.

cips/cip-tx-limits.md

jcstein · 2024-10-17T20:04:42Z

closing this in favor of 2 separate CIPs for these features

Motivation: celestiaorg/CIPs#220 (comment) IMO we should either include this in v3.0.0-rc0 or close it b/c not worth it to break these constant names after that.

Motivation: celestiaorg/CIPs#220 (comment) IMO we should either include this in v3.0.0-rc0 or close it b/c not worth it to break these constant names after that. (cherry picked from commit f217064)

Motivation: celestiaorg/CIPs#220 (comment) IMO we should either include this in v3.0.0-rc0 or close it b/c not worth it to break these constant names after that.

docs: add CIP for block limits on pfbs and tx size

f7f4a3d

celestia-bot requested review from ebuchman and rootulp October 16, 2024 14:30

docs: clarify rationale and security considerations

282bc93

jcstein self-assigned this Oct 16, 2024

rootulp reviewed Oct 16, 2024

View reviewed changes

jcstein mentioned this pull request Oct 16, 2024

Celestia Core Devs Call 18 #214

Closed

Apply suggestions from code review

f89c0ba

Co-authored-by: Rootul P <rootulp@gmail.com>

ninabarbakadze reviewed Oct 16, 2024

View reviewed changes

cips/cip-tx-limits.md Outdated Show resolved Hide resolved

ninabarbakadze reviewed Oct 16, 2024

View reviewed changes

cips/cip-tx-limits.md Show resolved Hide resolved

jcstein and others added 5 commits October 16, 2024 11:39

Apply suggestions from code review

2d60926

Co-authored-by: nina / ნინა <barbakadzeninaa@gmail.com>

docs: suggestion from nina review

f401653

docs: add Nina as author

e81a4de

style: add to list

61de07e

docs: code review

c6d9a7c

rach-id reviewed Oct 16, 2024

View reviewed changes

cips/cip-tx-limits.md Outdated Show resolved Hide resolved

rootulp mentioned this pull request Oct 16, 2024

refactor: rename limits from tx to msg celestiaorg/celestia-app#3984

Merged

jcstein added 2 commits October 16, 2024 18:01

docs: rachid's suggestion

164e63c

docs: use new variables, suggestions from review

8fbee0e

cmwaters reviewed Oct 17, 2024

View reviewed changes

jcstein commented Oct 17, 2024

View reviewed changes

cips/cip-tx-limits.md Outdated Show resolved Hide resolved

jcstein commented Oct 17, 2024

View reviewed changes

cips/cip-tx-limits.md Outdated Show resolved Hide resolved

jcstein commented Oct 17, 2024

View reviewed changes

cips/cip-tx-limits.md Outdated Show resolved Hide resolved

Apply suggestions from code review

0bb0df6

jcstein commented Oct 17, 2024

View reviewed changes

cips/cip-tx-limits.md Outdated Show resolved Hide resolved

Apply suggestions from code review

cdb6f4c

jcstein requested review from cmwaters, ninabarbakadze, rach-id and rootulp October 17, 2024 16:29

jcstein commented Oct 17, 2024

View reviewed changes

cips/cip-tx-limits.md Outdated Show resolved Hide resolved

jcstein commented Oct 17, 2024

View reviewed changes

cips/cip-tx-limits.md Outdated Show resolved Hide resolved

Apply suggestions from code review

fbb27e3

rootulp reviewed Oct 17, 2024

View reviewed changes

jcstein commented Oct 17, 2024

View reviewed changes

cips/cip-tx-limits.md Outdated Show resolved Hide resolved

Apply suggestions from code review

d251f3a

jcstein closed this Oct 17, 2024

jcstein deleted the jcs/cip-pfbs-limits-tx-limit branch October 17, 2024 20:19

		1. The number of PFBs per block is limited to 600 by setting `PFBTransactionCap` to 600. `PFBTransactionCap` is the maximum number of PFB messages a block can contain.

		1. The number of non-PFB transactions per block is limited to 200 by setting `NonPFBTransactionCap` to 200. `NonPFBTransactionCap` is the maximum number of SDK messages, aside from PFBs, that a block can contain.


		The rationale for this proposal is twofold:

		1. To prevent congestion on the network by limiting the number of PFBs and non-PFB transactions per block. This was initially not considered consensus-breaking, but it has a meaningful effect on users and should be formalized in a CIP.

	1. To prevent congestion on the network by limiting the number of PFBs and non-PFB transactions per block. This was initially not considered consensus-breaking, but it has a meaningful effect on users and should be formalized in a CIP.
	1. To prevent long block times on the network by limiting the number of PFBs and non-PFB transactions per block. This was initially not considered consensus-breaking, but it has a meaningful effect on users and should be formalized in a CIP.


		1. It's important to note that these limits are not strictly enforced. While they are defined by the `celestia-app` implementation, a validator could potentially modify the `PrepareProposal` logic, run a custom binary, and produce blocks that exceed the specified limits for PFB or non-PFBs transactions.

		1. The size of a transaction is limited to 2MiB by setting `MaxTxSize` to 2097152, which is 2MiB in bytes. From version v3 and above, in `CheckTx`, `PrepareProposal`, and `ProcessProposal`, each transaction's size is checked against the `appconsts.MaxTxSize` threshold. This ensures that transactions over the limit are rejected or excluded at all stages, from initial submission to execution.

Conversation

jcstein commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcstein Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcstein Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jcstein commented Oct 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jcstein commented Oct 16, 2024 •

edited

Loading

jcstein Oct 17, 2024 •

edited

Loading

jcstein Oct 17, 2024 •

edited

Loading