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

Set max tx size cap in ProcessProposal #4087

Open
2 tasks
ninabarbakadze opened this issue Dec 6, 2024 · 1 comment
Open
2 tasks

Set max tx size cap in ProcessProposal #4087

ninabarbakadze opened this issue Dec 6, 2024 · 1 comment
Labels
priority:high optional label to track the relative priority of planned items WS: V4

Comments

@ninabarbakadze
Copy link
Member

Description

In v4, we should enforce max tx size cap in ProcessProposal.

Acceptance Criteria

  • The check is enforced and thoroughly tested in integration tests with expected outputs and logs
  • Reconsider the current max tx size ante handler
@ninabarbakadze ninabarbakadze added WS: V4 priority:high optional label to track the relative priority of planned items labels Dec 6, 2024
@ninabarbakadze ninabarbakadze mentioned this issue Dec 6, 2024
Merged
1 task
@rootulp
Copy link
Collaborator

rootulp commented Dec 10, 2024 •
edited
Loading

Reconsider the current max tx size ante handler

To expand on this, the MaxTxSizeDecorator ante handler doesn't work as intended because it does not receive the full blob tx (including blobs). It only receives the inner sdk tx. Therefore, it can't reject blob txs where the blobs + sdk tx exceed 2 MiB.

We worked around this limitation in #4084 but we should deprecate and remove the MaxTxSizeDecorator.

Update: if we keep MaxTxSizeDecorator then we should add a unit test for blob txs of various sizes (less than 2 MiB and larger than 2 MiB)

ninabarbakadze added a commit that referenced this issue Dec 11, 2024
@ninabarbakadze @rootulp
fix: reject BlobTxs larger than 2 MiB (#4084)
3751aac 
## Overview

Directly checks transaction sizes even before they're decoded and
removes them if they exceed configured threshold.

We should add MaxTxSize constraint in ProcessProposal directly and
consider removing the AnteHandler in v4.
Issue tracking this work:
#4087

## Testing

- Check tx test asserts with logs that the expected error gets hit
- Getting logs from prepare proposal was more challenging so i'm
inserting a screenshot of application logs when the tests are run.
<img width="887" alt="Screenshot 2024-12-06 at 12 27 03"
src="https://github.com/user-attachments/assets/bb701834-5a3d-4eef-85f2-07074ae18a27">

<img width="837" alt="Screenshot 2024-12-06 at 12 27 20"
src="https://github.com/user-attachments/assets/651d9b87-3d65-43f4-a1a0-3874e03db455">


## Proposal for improving robustness of our test suites

- [ ] Open an issue to assert all logs in our integration tests.

---------

Co-authored-by: Rootul P <[email protected]>
mergify bot pushed a commit that referenced this issue Dec 11, 2024
@ninabarbakadze @mergify
fix: reject BlobTxs larger than 2 MiB (#4084)
b5ddfd4 
## Overview

Directly checks transaction sizes even before they're decoded and
removes them if they exceed configured threshold.

We should add MaxTxSize constraint in ProcessProposal directly and
consider removing the AnteHandler in v4.
Issue tracking this work:
#4087

## Testing

- Check tx test asserts with logs that the expected error gets hit
- Getting logs from prepare proposal was more challenging so i'm
inserting a screenshot of application logs when the tests are run.
<img width="887" alt="Screenshot 2024-12-06 at 12 27 03"
src="https://github.com/user-attachments/assets/bb701834-5a3d-4eef-85f2-07074ae18a27">

<img width="837" alt="Screenshot 2024-12-06 at 12 27 20"
src="https://github.com/user-attachments/assets/651d9b87-3d65-43f4-a1a0-3874e03db455">

## Proposal for improving robustness of our test suites

- [ ] Open an issue to assert all logs in our integration tests.

---------

Co-authored-by: Rootul P <[email protected]>
(cherry picked from commit 3751aac)

# Conflicts:
#	.github/CODEOWNERS
#	.github/auto_request_review.yml
#	.github/mergify.yml
#	.github/workflows/lint.yml
#	.github/workflows/pr-review-requester.yml
#	.github/workflows/test.yml
#	Makefile
#	app/app.go
#	app/benchmarks/README.md
#	app/benchmarks/results.md
#	app/check_tx.go
#	app/square_size.go
#	app/test/big_blob_test.go
#	app/test/check_tx_test.go
#	app/test/consistent_apphash_test.go
#	app/test/prepare_proposal_test.go
#	app/test/process_proposal_test.go
#	app/test/upgrade_test.go
#	cmd/celestia-appd/cmd/start.go
#	docs/architecture/adr-011-optimistic-blob-size-independent-inclusion-proofs-and-pfb-fraud-proofs.md
#	docs/architecture/adr-015-namespace-id-size.md
#	docs/architecture/adr-021-restricted-block-size.md
#	docs/maintainers/prebuilt-binaries.md
#	docs/maintainers/release-guide.md
#	docs/release-notes/release-notes.md
#	go.mod
#	go.sum
#	pkg/appconsts/chain_ids.go
#	pkg/appconsts/overrides.go
#	pkg/appconsts/v1/app_consts.go
#	pkg/appconsts/v2/app_consts.go
#	pkg/appconsts/v3/app_consts.go
#	pkg/appconsts/versioned_consts.go
#	pkg/appconsts/versioned_consts_test.go
#	scripts/single-node.sh
#	specs/src/cat_pool.md
#	specs/src/data_square_layout.md
#	specs/src/parameters_v1.md
#	specs/src/parameters_v2.md
#	specs/src/parameters_v3.md
#	test/e2e/benchmark/benchmark.go
#	test/e2e/benchmark/throughput.go
#	test/e2e/major_upgrade_v2.go
#	test/e2e/major_upgrade_v3.go
#	test/e2e/minor_version_compatibility.go
#	test/e2e/readme.md
#	test/e2e/simple.go
#	test/e2e/testnet/node.go
#	test/e2e/testnet/setup.go
#	test/e2e/testnet/testnet.go
#	test/e2e/testnet/txsimNode.go
#	test/e2e/testnet/util.go
#	test/interchain/go.mod
#	test/interchain/go.sum
#	test/util/blobfactory/payforblob_factory.go
#	test/util/blobfactory/test_util.go
#	test/util/testnode/comet_node_test.go
#	test/util/testnode/config.go
#	x/blob/ante/blob_share_decorator.go
#	x/blob/ante/max_total_blob_size_ante.go
#	x/tokenfilter/README.md
@mergify mergify bot mentioned this issue Dec 11, 2024
Merged
1 task
cmwaters pushed a commit that referenced this issue Dec 12, 2024
@mergify @ninabarbakadze
fix: reject BlobTxs larger than 2 MiB (backport #4084) (#4109)
c456d75 
## Overview

Directly checks transaction sizes even before they're decoded and
removes them if they exceed configured threshold.

We should add MaxTxSize constraint in ProcessProposal directly and
consider removing the AnteHandler in v4.
Issue tracking this work:
#4087

## Testing

- Check tx test asserts with logs that the expected error gets hit
- Getting logs from prepare proposal was more challenging so i'm
inserting a screenshot of application logs when the tests are run.
<img width="887" alt="Screenshot 2024-12-06 at 12 27 03"
src="https://github.com/user-attachments/assets/bb701834-5a3d-4eef-85f2-07074ae18a27">

<img width="837" alt="Screenshot 2024-12-06 at 12 27 20"
src="https://github.com/user-attachments/assets/651d9b87-3d65-43f4-a1a0-3874e03db455">


## Proposal for improving robustness of our test suites

- [ ] Open an issue to assert all logs in our integration tests.






<hr>This is an automatic backport of pull request #4084 done by
[Mergify](https://mergify.com).

Co-authored-by: Nina Barbakadze <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
priority:high optional label to track the relative priority of planned items WS: V4
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@rootulp @ninabarbakadze