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

What happens to transactions that contain a blobs that are too large? #2156

Closed
rootulp opened this issue Jul 24, 2023 · 4 comments · Fixed by #2202
Closed

What happens to transactions that contain a blobs that are too large? #2156

rootulp opened this issue Jul 24, 2023 · 4 comments · Fixed by #2202
Assignees
Labels
proposal item is not yet actionable and is suggesting a change that must first be agreed upon

Comments

@rootulp
Copy link
Collaborator

rootulp commented Jul 24, 2023

Context

#2108

Problem

Consensus nodes may not reject transactions that contain a blob that is too large to fit in a block. As a result, the transaction may sit in the mempool until it eventually expires (depends on mepool config).

Proposal

Explore if the mempool will reject PFB transactions that contain a blob that will not fit in the maximum data square. If it doesn't, consider:

Option A

Add a stateless check to ValidateBasic on a transaction that verifies that the blob size is <= the maximum blob size. Compute the maximum blob size based on the upper bound values for max square size and max block bytes.

Option B

Decrease max_tx_bytes in the mempool config to the size of a tx that contains the largest blob that can fit in the largest data square.

Option C

Add a stateful check via blob ante handler that verifies that the blob size is <= the maximum blob size. Compute the maximum blob size based on the governance parameters for max square size.

Option D

Introduce a new versioned constant that acts as a max supported blob size.

  • Pros: Since this is a versioned constant, a user can know prior to submitting their blob if it is valid. They don’t need to query the current chain state to determine if their constructed blob is valid.
  • Cons: One additional parameter that needs to be re-evaluated when other square parameters are modified.
@rootulp rootulp added T:investigate proposal item is not yet actionable and is suggesting a change that must first be agreed upon labels Jul 24, 2023
@rootulp
Copy link
Collaborator Author

rootulp commented Jul 30, 2023

Since there isn't a versioned constant for the maximum supported blob size, it must be computed based on a number of other parameters (see #2108). Since these parameters require access to the state, option A must be a stateful check.

@rootulp
Copy link
Collaborator Author

rootulp commented Jul 30, 2023

I started working on validating that blobs are < max blob size in https://github.com/rootulp/celestia-app/tree/rp/improve-too-big-blob

@evan-forbes
Copy link
Member

Option A makes a lot of sense, since this will get hit before even submitting a blob. Do we also have an option C (no mutually exclusive to A!), where we add an ante handler that checks the the size of the blob using the stateful parameters? This will get hit upon submitting a blobtx and when we are gossiping blobs

@rootulp
Copy link
Collaborator Author

rootulp commented Jul 31, 2023

Good idea! Added Option C above and it seems like we can do Option A and Option C.

@rootulp rootulp changed the title What happens to transactions that contain a blob that will not fit in the maximum data square? What happens to transactions that contain a blobs that are too large? Jul 31, 2023
rootulp added a commit that referenced this issue Aug 1, 2023
@rootulp rootulp added this to the Mainnet milestone Aug 5, 2023
rootulp added a commit that referenced this issue Aug 14, 2023
mergify bot pushed a commit that referenced this issue Aug 14, 2023
Closes #2156 by
implementing **Option C**

(cherry picked from commit b6f3968)

# Conflicts:
#	app/ante.go
#	app/test/check_tx_test.go
rootulp added a commit that referenced this issue Aug 14, 2023
rootulp added a commit that referenced this issue Aug 14, 2023
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this issue Aug 1, 2024
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this issue Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal item is not yet actionable and is suggesting a change that must first be agreed upon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants