-
Notifications
You must be signed in to change notification settings - Fork 390
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
Fix length calculations of blob-type txs #5884
Conversation
@@ -201,7 +201,7 @@ public class SystemTransaction : Transaction { } | |||
/// <remarks>Created because of cyclic dependencies between Core and Rlp modules</remarks> | |||
public interface ITransactionSizeCalculator | |||
{ | |||
int GetLength(Transaction tx); | |||
int GetNetworkTxLength(Transaction tx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep it as GetLength
and then have a NetworkTransactionSizeCalculator
that would take TxDecoder
and call GetLength(tx, RlpBehaviors.InMempoolForm);
Then you can reuse this interface for other calculators if needed, rather than hardcoding that in TxDecoder
.
@@ -136,7 +136,7 @@ private void ClearPreHashInternal() | |||
/// </summary> | |||
public int GetLength(ITransactionSizeCalculator sizeCalculator) | |||
{ | |||
return _size ??= sizeCalculator.GetLength(this); | |||
return _size ??= sizeCalculator.GetNetworkTxLength(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now there is no differentiation what _size
is. Makes it confusing.
@@ -136,7 +136,7 @@ private void ClearPreHashInternal() | |||
/// </summary> | |||
public int GetLength(ITransactionSizeCalculator sizeCalculator) | |||
{ | |||
return _size ??= sizeCalculator.GetNetworkTxLength(this); | |||
return _size ??= sizeCalculator.GetLength(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_size
can be confusing as it will differ per sizecalculator
# Conflicts: # src/Nethermind/Nethermind.TxPool/TransactionExtensions.cs
* regression test * fix * fiz typo * adjust test * rename GetLength to GetNetworkTxLength * refactor (cherry picked from commit 88e4d31)
Changes
Without
RlpBehaviors.InMempoolForm
inGetLength
ofITransactionSizeCalculator
actual blobs were not included in calculation of tx size.While calculating content length we were not entering here:
https://github.com/NethermindEth/nethermind/blob/master/src/Nethermind/Nethermind.Serialization.Rlp/TxDecoder.cs#L724-L732
Which caused not adding size of actual blobs:
https://github.com/NethermindEth/nethermind/blob/master/src/Nethermind/Nethermind.Serialization.Rlp/TxDecoder.cs#L677-L684
And it causes, that we are sending huge messages on 4844 devnets which are rejected by peers (e.g.
PooledTransactionsMessage
with size 147x bigger than expected and 19x bigger than biggest possible 6-blob tx). Also ourNewPooledTransactionHashesMessage
ineth68
version has not correct sizes of blob txs.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Documentation
Requires documentation update
Requires explanation in Release Notes