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

Add filter for max transaction size #7925

Merged
merged 33 commits into from
Jan 7, 2025
Merged

Conversation

marcindsobczak
Copy link
Contributor

@marcindsobczak marcindsobczak commented Dec 17, 2024

Changes

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

@marcindsobczak marcindsobczak marked this pull request as ready for review December 17, 2024 15:58
@marcindsobczak marcindsobczak requested a review from rubo as a code owner December 17, 2024 15:58
Co-authored-by: Ruben Buniatyan <rubo@users.noreply.github.com>
Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

We could add similar logic in transaction gossip, before we even deserialise RLP. We could avoid deserialization and serialization later. Also NewPooledTransactionHashesMessage68 have size property that we could leverage and not request transactions over that size at all.
Do we want exception for local transactions? (probably not)

src/Nethermind/Nethermind.TxPool/Filters/SizeTxFilter.cs Outdated Show resolved Hide resolved
@marcindsobczak
Copy link
Contributor Author

Good idea with not requesting txs with size above max limit!

About skipping deserialization - we need to deserialise at least a part of tx to check tx type. But I think we can reject at the very beginning all txs with size exceeding maximally loaded blob tx (max number of blobs + max size of non-blob data).

About exception for local txs - I don't think so. If user would need to send such tx, there is always an option to increase config value

@LukaszRozmej
Copy link
Member

LukaszRozmej commented Dec 18, 2024

Good idea with not requesting txs with size above max limit!

About skipping deserialization - we need to deserialise at least a part of tx to check tx type. But I think we can reject at the very beginning all txs with size exceeding maximally loaded blob tx (max number of blobs + max size of non-blob data).

About exception for local txs - I don't think so. If user would need to send such tx, there is always an option to increase config value

We can get the type without deserializing anything else, it is just first byte.

@marcindsobczak marcindsobczak marked this pull request as draft December 20, 2024 19:59
@marcindsobczak
Copy link
Contributor Author

Good idea with not requesting txs with size above max limit!
About skipping deserialization - we need to deserialise at least a part of tx to check tx type. But I think we can reject at the very beginning all txs with size exceeding maximally loaded blob tx (max number of blobs + max size of non-blob data).
About exception for local txs - I don't think so. If user would need to send such tx, there is always an option to increase config value

We can get the type without deserializing anything else, it is just first byte.

There are some exceptions like legacy txs without tx type. For sure it's doable, but requires more insights, time, caution and extended testing, which I don't want to do in the scope of this PR. I added an issue for it: #7986

Here I added:

  • TxType comparisons as TxTypeExtension
  • limiting max blob tx size (excluding blobs)
  • max blob tx size as config value
  • not requesting too large txs in PooledTxsRequestor

@marcindsobczak marcindsobczak marked this pull request as ready for review December 31, 2024 11:19
@@ -21,6 +21,7 @@ public class TxPoolConfig : ITxPoolConfig
public int HashCacheSize { get; set; } = 512 * 1024;
public long? GasLimit { get; set; } = null;
public long? MaxTxSize { get; set; } = 128.KiB();
public long? MaxBlobTxSize { get; set; } = 1.MiB();
Copy link
Member

Choose a reason for hiding this comment

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

why blopb tx's can be 8x bigger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't have sense to me, but I wanted to do it similar to Geth implementation
https://github.com/ethereum/go-ethereum/blob/master/core/txpool/blobpool/blobpool.go#L63-L68

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And it is not a big deal as blob txs are saved to disc. Memory is not affected as in case of non-blob txs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can also go with standard 128 KiB and it will simplify this PR, but we will then reject some txs accepted by Geth

@marcindsobczak marcindsobczak merged commit 480f5f6 into master Jan 7, 2025
79 checks passed
@marcindsobczak marcindsobczak deleted the feature/tx_size_filter branch January 7, 2025 11:44
rjnrohit pushed a commit that referenced this pull request Jan 11, 2025
Co-authored-by: Ruben Buniatyan <rubo@users.noreply.github.com>
Co-authored-by: lukasz.rozmej <lukasz.rozmej@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants