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.

Sign up for GitHub

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

Jump to bottom

feat: reject tx if total blob size too large #2202

Merged
merged 51 commits into from
Aug 14, 2023
Merged

feat: reject tx if total blob size too large #2202

merged 51 commits into from
Aug 14, 2023

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Aug 1, 2023 •
edited
Loading

Closes #2156 by implementing Option C

@rootulp rootulp added the enhancement New feature or request label Aug 1, 2023
@rootulp rootulp added this to the Mainnet milestone Aug 1, 2023
@rootulp rootulp self-assigned this Aug 1, 2023
@rootulp rootulp marked this pull request as ready for review August 1, 2023 19:12
evan-forbes
evan-forbes reviewed Aug 2, 2023
View reviewed changes
test/util/blobfactory/payforblob_factory.go Outdated Show resolved Hide resolved
x/blob/types/payforblob.go Outdated Show resolved Hide resolved
app/test/check_tx_test.go Outdated Show resolved Hide resolved
@MSevey MSevey requested a review from a team August 2, 2023 18:51
rootulp
rootulp commented Aug 2, 2023
View reviewed changes
app/test/check_tx_test.go Outdated Show resolved Hide resolved
@rootulp rootulp requested a review from evan-forbes August 2, 2023 19:15
@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2023 •
edited
Loading

Codecov Report

Merging #2202 (4828a6e) into main (3e6d22c) will increase coverage by 0.15%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##             main    #2202      +/-   ##
==========================================
+ Coverage   21.55%   21.70%   +0.15%     
==========================================
  Files         128      129       +1     
  Lines       14346    14390      +44     
==========================================
+ Hits         3092     3124      +32     
- Misses      10949    10960      +11     
- Partials      305      306       +1
Files Changed Coverage Δ
app/ante/ante.go 0.00% <0.00%> (ø)
x/blob/ante/ante.go 85.00% <ø> (ø)
x/blob/ante/max_total_blob_size_ante.go 74.41% <74.41%> (ø)

rootulp
rootulp commented Aug 3, 2023
View reviewed changes
x/blob/types/payforblob.go Outdated Show resolved Hide resolved
@rootulp rootulp marked this pull request as draft August 3, 2023 15:33
rootulp
rootulp commented Aug 3, 2023
View reviewed changes
x/blob/types/payforblob.go Outdated Show resolved Hide resolved
@rootulp rootulp marked this pull request as ready for review August 10, 2023 18:55
@rootulp rootulp added the backport:v1.x PR will be backported automatically to the v1.x branch upon merging label Aug 10, 2023
@rootulp
Copy link
Collaborator Author

rootulp commented Aug 10, 2023

I have low confidence that this will backport to the v1.x branch cleanly but at least mergify can open a PR that I can fix.

cmwaters
cmwaters approved these changes Aug 11, 2023
View reviewed changes
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

Nice work 👍

@evan-forbes evan-forbes mentioned this pull request Aug 11, 2023
Closed
7 tasks
@rootulp rootulp requested a review from rach-id August 14, 2023 14:54
evan-forbes
evan-forbes approved these changes Aug 14, 2023
View reviewed changes
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

nice!

x/blob/ante/max_total_blob_size_ante.go Show resolved Hide resolved
@rootulp rootulp mentioned this pull request Aug 14, 2023
Open
@rootulp rootulp merged commit b6f3968 into celestiaorg:main Aug 14, 2023
@rootulp rootulp deleted the rp/blob-too-large-option-a branch August 14, 2023 19:00
mergify bot pushed a commit that referenced this pull request Aug 14, 2023
@rootulp @mergify
feat: reject tx if total blob size too large (#2202)
dbdcedd 
Closes #2156 by
implementing **Option C**

(cherry picked from commit b6f3968)

# Conflicts:
#	app/ante.go
#	app/test/check_tx_test.go
@mergify mergify bot mentioned this pull request Aug 14, 2023
Merged
rootulp added a commit that referenced this pull request Aug 14, 2023
@rootulp
Revert "feat: reject tx if total blob size too large (#2202)"
7edcf3d 
This reverts commit dbdcedd.
rootulp added a commit that referenced this pull request Aug 14, 2023
@rootulp
feat: reject tx if total blob size too large (#2202)
15f0b89 
Closes #2156 by
implementing **Option C**
rootulp added a commit that referenced this pull request Aug 15, 2023
@rootulp
feat: reject tx if total blob size too large (backport #2202) (#2268)
906ef24
@rootulp rootulp mentioned this pull request Sep 7, 2023
Closed
@rootulp rootulp mentioned this pull request Feb 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evan-forbes evan-forbes evan-forbes approved these changes

@cmwaters cmwaters cmwaters approved these changes

@rach-id rach-id Awaiting requested review from rach-id

Assignees

@rootulp rootulp

Labels
backport:v1.x PR will be backported automatically to the v1.x branch upon merging enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

What happens to transactions that contain a blobs that are too large?
4 participants
@rootulp @codecov-commenter @cmwaters @evan-forbes