From 5335697f8ffb604f0ce03c1e8b025a243e1f2a18 Mon Sep 17 00:00:00 2001 From: Marcin Sobczak Date: Fri, 28 Apr 2023 12:44:39 +0200 Subject: [PATCH] few fixes --- .../Validators/TxValidatorTests.cs | 13 +-- src/Nethermind/Nethermind.Consensus/Signer.cs | 2 +- .../Validators/TxValidator.cs | 17 ++-- src/Nethermind/Nethermind.Hive/HiveRunner.cs | 2 +- .../Nethermind.Network/NetworkNodeDecoder.cs | 1 - .../ChainLevelDecoder.cs | 1 + .../Eip2930/AccessListDecoder.cs | 50 ++++++++--- .../Nethermind.Serialization.Rlp/Rlp.cs | 79 +++++++++++++--- .../Nethermind.Serialization.Rlp/RlpStream.cs | 90 +++++++++++++++---- .../Nethermind.Serialization.Rlp/TxDecoder.cs | 70 +++++++-------- .../Nethermind.TxPool.Test/TxPoolTests.cs | 20 +++-- .../Collections/SortedPool.cs | 13 +++ .../Comparison/CompareReplacedTxByFee.cs | 10 ++- src/Nethermind/Nethermind.TxPool/TxPool.cs | 14 ++- .../Nethermind.Wallet/WalletExtensions.cs | 2 +- 15 files changed, 281 insertions(+), 103 deletions(-) diff --git a/src/Nethermind/Nethermind.Blockchain.Test/Validators/TxValidatorTests.cs b/src/Nethermind/Nethermind.Blockchain.Test/Validators/TxValidatorTests.cs index d2f77fbde79..a6d33b79341 100644 --- a/src/Nethermind/Nethermind.Blockchain.Test/Validators/TxValidatorTests.cs +++ b/src/Nethermind/Nethermind.Blockchain.Test/Validators/TxValidatorTests.cs @@ -89,11 +89,12 @@ public void Bad_chain_id_is_not_valid() } [Test, Timeout(Timeout.MaxTestTime)] - public void No_chain_id_tx_is_valid() + public void No_chain_id_legacy_tx_is_valid() { byte[] sigData = new byte[65]; sigData[31] = 1; // correct r sigData[63] = 1; // correct s + sigData[64] = 27; // correct v Signature signature = new(sigData); Transaction tx = Build.A.Transaction.WithSignature(signature).TestObject; @@ -146,7 +147,7 @@ public bool Before_eip_2930_has_to_be_legacy_tx(TxType txType, bool eip2930) byte[] sigData = new byte[65]; sigData[31] = 1; // correct r sigData[63] = 1; // correct s - sigData[64] = CalculateV(); + sigData[64] = 27; // correct v Signature signature = new(sigData); Transaction tx = Build.A.Transaction .WithType(txType > TxType.AccessList ? TxType.Legacy : txType) @@ -231,7 +232,7 @@ public bool MaxFeePerGas_is_required_to_be_greater_than_MaxPriorityFeePerGas(TxT byte[] sigData = new byte[65]; sigData[31] = 1; // correct r sigData[63] = 1; // correct s - sigData[64] = CalculateV(); + sigData[64] = 27; // correct v Signature signature = new(sigData); Transaction tx = Build.A.Transaction .WithType(txType > TxType.AccessList ? TxType.Legacy : txType) @@ -286,7 +287,7 @@ public bool MaxFeePerDataGas_should_be_set_for_blob_tx_only(TxType txType, bool byte[] sigData = new byte[65]; sigData[31] = 1; // correct r sigData[63] = 1; // correct s - sigData[64] = 1 + TestBlockchainIds.ChainId * 2 + 35; + sigData[64] = 27; // correct v Signature signature = new(sigData); TransactionBuilder txBuilder = Build.A.Transaction .WithType(txType) @@ -315,7 +316,7 @@ public bool Blobs_count_should_be_within_constraints(int blobsCount) byte[] sigData = new byte[65]; sigData[31] = 1; // correct r sigData[63] = 1; // correct s - sigData[64] = 1 + TestBlockchainIds.ChainId * 2 + 35; + sigData[64] = 27; // correct v Signature signature = new(sigData); Transaction tx = Build.A.Transaction .WithType(TxType.Blob) @@ -338,7 +339,7 @@ public bool BlobVersionedHash_should_be_correct(byte[] hash) byte[] sigData = new byte[65]; sigData[31] = 1; // correct r sigData[63] = 1; // correct s - sigData[64] = 1 + TestBlockchainIds.ChainId * 2 + 35; + sigData[64] = 27; // correct v Signature signature = new(sigData); Transaction tx = Build.A.Transaction .WithType(TxType.Blob) diff --git a/src/Nethermind/Nethermind.Consensus/Signer.cs b/src/Nethermind/Nethermind.Consensus/Signer.cs index af1717413e6..67f6feaeae0 100644 --- a/src/Nethermind/Nethermind.Consensus/Signer.cs +++ b/src/Nethermind/Nethermind.Consensus/Signer.cs @@ -47,7 +47,7 @@ public ValueTask Sign(Transaction tx) { Keccak hash = Keccak.Compute(Rlp.Encode(tx, true, true, _chainId).Bytes); tx.Signature = Sign(hash); - tx.Signature.V = tx.Signature.V + 8 + 2 * _chainId; + tx.Signature.V = tx.Type == TxType.Legacy ? tx.Signature.V + 8 + 2 * _chainId : (ulong)(tx.Signature.RecoveryId + 27); return default; } diff --git a/src/Nethermind/Nethermind.Consensus/Validators/TxValidator.cs b/src/Nethermind/Nethermind.Consensus/Validators/TxValidator.cs index 4a714122cc5..985a1b85f96 100644 --- a/src/Nethermind/Nethermind.Consensus/Validators/TxValidator.cs +++ b/src/Nethermind/Nethermind.Consensus/Validators/TxValidator.cs @@ -36,7 +36,7 @@ public bool IsWellFormed(Transaction transaction, IReleaseSpec releaseSpec) transaction.GasLimit >= IntrinsicGasCalculator.Calculate(transaction, releaseSpec) && /* if it is a call or a transfer then we require the 'To' field to have a value while for an init it will be empty */ - ValidateSignature(transaction.Signature, releaseSpec) && + ValidateSignature(transaction, releaseSpec) && ValidateChainId(transaction) && Validate1559GasFields(transaction, releaseSpec) && Validate3860Rules(transaction, releaseSpec) && @@ -71,8 +71,10 @@ private bool ValidateChainId(Transaction transaction) => _ => transaction.ChainId == _chainIdValue }; - private bool ValidateSignature(Signature? signature, IReleaseSpec spec) + private bool ValidateSignature(Transaction tx, IReleaseSpec spec) { + Signature? signature = tx.Signature; + if (signature is null) { return false; @@ -91,12 +93,17 @@ private bool ValidateSignature(Signature? signature, IReleaseSpec spec) return false; } - if (spec.IsEip155Enabled) + if (signature.V is 27 or 28) + { + return true; + } + + if (tx.Type == TxType.Legacy && spec.IsEip155Enabled && (signature.V == _chainIdValue * 2 + 35ul || signature.V == _chainIdValue * 2 + 36ul)) { - return (signature.ChainId ?? _chainIdValue) == _chainIdValue; + return true; } - return !spec.ValidateChainId || signature.V is 27 or 28; + return !spec.ValidateChainId; } private static bool Validate4844Fields(Transaction transaction) diff --git a/src/Nethermind/Nethermind.Hive/HiveRunner.cs b/src/Nethermind/Nethermind.Hive/HiveRunner.cs index f696e039e90..eeb6c4b6c73 100644 --- a/src/Nethermind/Nethermind.Hive/HiveRunner.cs +++ b/src/Nethermind/Nethermind.Hive/HiveRunner.cs @@ -168,7 +168,7 @@ private async Task InitializeChain(string chainFile) while (rlpStream.PeekNumberOfItemsRemaining() > 0) { rlpStream.PeekNextItem(); - Block block = Rlp.Decode(rlpStream); + Block block = Rlp.Decode(rlpStream, RlpBehaviors.AllowExtraBytes); if (_logger.IsInfo) _logger.Info($"HIVE Reading a chain.rlp block {block.ToString(Block.Format.Short)}"); blocks.Add(block); diff --git a/src/Nethermind/Nethermind.Network/NetworkNodeDecoder.cs b/src/Nethermind/Nethermind.Network/NetworkNodeDecoder.cs index f72faa4a91a..e714aaee37e 100644 --- a/src/Nethermind/Nethermind.Network/NetworkNodeDecoder.cs +++ b/src/Nethermind/Nethermind.Network/NetworkNodeDecoder.cs @@ -76,7 +76,6 @@ public int GetLength(NetworkNode item, RlpBehaviors rlpBehaviors) private int GetContentLength(NetworkNode item, RlpBehaviors rlpBehaviors) { return Rlp.LengthOf(item.NodeId.Bytes) - + Rlp.LengthOf(item.NodeId.Bytes) + Rlp.LengthOf(item.Host) + Rlp.LengthOf(item.Port) + 1 diff --git a/src/Nethermind/Nethermind.Serialization.Rlp/ChainLevelDecoder.cs b/src/Nethermind/Nethermind.Serialization.Rlp/ChainLevelDecoder.cs index e99a44238dc..85e2af8b498 100644 --- a/src/Nethermind/Nethermind.Serialization.Rlp/ChainLevelDecoder.cs +++ b/src/Nethermind/Nethermind.Serialization.Rlp/ChainLevelDecoder.cs @@ -20,6 +20,7 @@ public class ChainLevelDecoder : IRlpStreamDecoder, IRlpValueDec if (rlpStream.IsNextItemNull()) { + rlpStream.ReadByte(); return null; } diff --git a/src/Nethermind/Nethermind.Serialization.Rlp/Eip2930/AccessListDecoder.cs b/src/Nethermind/Nethermind.Serialization.Rlp/Eip2930/AccessListDecoder.cs index 9de3da37c16..db6bd5048d9 100644 --- a/src/Nethermind/Nethermind.Serialization.Rlp/Eip2930/AccessListDecoder.cs +++ b/src/Nethermind/Nethermind.Serialization.Rlp/Eip2930/AccessListDecoder.cs @@ -26,14 +26,14 @@ public class AccessListDecoder : IRlpStreamDecoder, IRlpValueDecode return null; } - int length = rlpStream.PeekNextRlpLength(); + int length = rlpStream.ReadSequenceLength(); int check = rlpStream.Position + length; - rlpStream.SkipLength(); AccessListBuilder accessListBuilder = new(); while (rlpStream.Position < check) { - rlpStream.SkipLength(); + int accessListItemLength = rlpStream.ReadSequenceLength(); + int accessListItemCheck = rlpStream.Position + accessListItemLength; Address address = rlpStream.DecodeAddress(); if (address is null) { @@ -44,14 +44,27 @@ public class AccessListDecoder : IRlpStreamDecoder, IRlpValueDecode if (rlpStream.Position < check) { - int storageCheck = rlpStream.Position + rlpStream.PeekNextRlpLength(); - rlpStream.SkipLength(); - while (rlpStream.Position < storageCheck) + int storagesLength = rlpStream.ReadSequenceLength(); + int storagesCheck = rlpStream.Position + storagesLength; + while (rlpStream.Position < storagesCheck) { + int storageItemCheck = rlpStream.Position + 33; UInt256 index = rlpStream.DecodeUInt256(); accessListBuilder.AddStorage(index); + if ((rlpBehaviors & RlpBehaviors.AllowExtraBytes) != RlpBehaviors.AllowExtraBytes) + { + rlpStream.Check(storageItemCheck); + } + } + if ((rlpBehaviors & RlpBehaviors.AllowExtraBytes) != RlpBehaviors.AllowExtraBytes) + { + rlpStream.Check(storagesCheck); } } + if ((rlpBehaviors & RlpBehaviors.AllowExtraBytes) != RlpBehaviors.AllowExtraBytes) + { + rlpStream.Check(accessListItemCheck); + } } if ((rlpBehaviors & RlpBehaviors.AllowExtraBytes) != RlpBehaviors.AllowExtraBytes) @@ -78,14 +91,14 @@ public class AccessListDecoder : IRlpStreamDecoder, IRlpValueDecode return null; } - int length = decoderContext.PeekNextRlpLength(); + int length = decoderContext.ReadSequenceLength(); int check = decoderContext.Position + length; - decoderContext.SkipLength(); AccessListBuilder accessListBuilder = new(); while (decoderContext.Position < check) { - decoderContext.SkipLength(); + int accessListItemLength = decoderContext.ReadSequenceLength(); + int accessListItemCheck = decoderContext.Position + accessListItemLength; Address address = decoderContext.DecodeAddress(); if (address is null) { @@ -96,14 +109,27 @@ public class AccessListDecoder : IRlpStreamDecoder, IRlpValueDecode if (decoderContext.Position < check) { - int storageCheck = decoderContext.Position + decoderContext.PeekNextRlpLength(); - decoderContext.SkipLength(); - while (decoderContext.Position < storageCheck) + int storagesLength = decoderContext.ReadSequenceLength(); + int storagesCheck = decoderContext.Position + storagesLength; + while (decoderContext.Position < storagesCheck) { + int storageItemCheck = decoderContext.Position + 33; UInt256 index = decoderContext.DecodeUInt256(); accessListBuilder.AddStorage(index); + if ((rlpBehaviors & RlpBehaviors.AllowExtraBytes) != RlpBehaviors.AllowExtraBytes) + { + decoderContext.Check(storageItemCheck); + } + } + if ((rlpBehaviors & RlpBehaviors.AllowExtraBytes) != RlpBehaviors.AllowExtraBytes) + { + decoderContext.Check(storagesCheck); } } + if ((rlpBehaviors & RlpBehaviors.AllowExtraBytes) != RlpBehaviors.AllowExtraBytes) + { + decoderContext.Check(accessListItemCheck); + } } if ((rlpBehaviors & RlpBehaviors.AllowExtraBytes) != RlpBehaviors.AllowExtraBytes) diff --git a/src/Nethermind/Nethermind.Serialization.Rlp/Rlp.cs b/src/Nethermind/Nethermind.Serialization.Rlp/Rlp.cs index 985ce10096e..ef227332153 100644 --- a/src/Nethermind/Nethermind.Serialization.Rlp/Rlp.cs +++ b/src/Nethermind/Nethermind.Serialization.Rlp/Rlp.cs @@ -158,13 +158,23 @@ public static T[] DecodeArray(RlpStream rlpStream, IRlpStreamDecoder? rlpD public static T Decode(RlpStream rlpStream, RlpBehaviors rlpBehaviors = RlpBehaviors.None) { IRlpStreamDecoder? rlpDecoder = GetStreamDecoder(); - return rlpDecoder is not null ? rlpDecoder.Decode(rlpStream, rlpBehaviors) : throw new RlpException($"{nameof(Rlp)} does not support decoding {typeof(T).Name}"); + bool shouldCheckStream = rlpStream.Position == 0 && (rlpBehaviors & RlpBehaviors.AllowExtraBytes) != RlpBehaviors.AllowExtraBytes; + int length = rlpStream.Length; + T? result = rlpDecoder is not null ? rlpDecoder.Decode(rlpStream, rlpBehaviors) : throw new RlpException($"{nameof(Rlp)} does not support decoding {typeof(T).Name}"); + if (shouldCheckStream) + rlpStream.Check(length); + return result; } public static T Decode(ref ValueDecoderContext decoderContext, RlpBehaviors rlpBehaviors = RlpBehaviors.None) { IRlpValueDecoder? rlpDecoder = GetValueDecoder(); - return rlpDecoder is not null ? rlpDecoder.Decode(ref decoderContext, rlpBehaviors) : throw new RlpException($"{nameof(Rlp)} does not support decoding {typeof(T).Name}"); + bool shouldCheckStream = decoderContext.Position == 0 && (rlpBehaviors & RlpBehaviors.AllowExtraBytes) != RlpBehaviors.AllowExtraBytes; + int length = decoderContext.Length; + T? result = rlpDecoder is not null ? rlpDecoder.Decode(ref decoderContext, rlpBehaviors) : throw new RlpException($"{nameof(Rlp)} does not support decoding {typeof(T).Name}"); + if (shouldCheckStream) + decoderContext.Check(length); + return result; } public static Rlp Encode(T item, RlpBehaviors behaviors = RlpBehaviors.None) @@ -876,7 +886,7 @@ public void DecodeAddressStructRef(out AddressStructRef address) } } - public UInt256 DecodeUInt256() + public UInt256 DecodeUInt256(bool allowLeadingZeroBytes = true) { Span byteSpan = DecodeByteArraySpan(); if (byteSpan.Length > 32) @@ -884,12 +894,21 @@ public UInt256 DecodeUInt256() throw new ArgumentException(); } + if (!allowLeadingZeroBytes && byteSpan.Length > 1 && byteSpan[0] == 0) + { + throw new RlpException($"Non-canonical UInt256 (leading zero bytes) at position {Position}"); + } + return new UInt256(byteSpan, true); } - public BigInteger DecodeUBigInt() + public BigInteger DecodeUBigInt(bool allowLeadingZeroBytes = true) { ReadOnlySpan bytes = DecodeByteArraySpan(); + if (!allowLeadingZeroBytes && bytes.Length > 1 && bytes[0] == 0) + { + throw new RlpException($"Non-canonical UBigInt (leading zero bytes) at position {Position}"); + } return bytes.ToUnsignedBigInteger(); } @@ -963,9 +982,15 @@ public bool IsNextItemNull() return Data[Position] == 192; } - public int DecodeInt() + public int DecodeInt(bool allowLeadingZeroBytes = true) { int prefix = ReadByte(); + + if (!allowLeadingZeroBytes && prefix == 0) + { + throw new RlpException($"Non-canonical integer (leading zero bytes) at position {Position}"); + } + if (prefix < 128) { return prefix; @@ -988,7 +1013,11 @@ public int DecodeInt() result = result << 8; if (i <= length) { - result = result | Data[Position + length - i]; + result |= Data[Position + length - i]; + if (!allowLeadingZeroBytes && result == 0) + { + throw new RlpException($"Non-canonical integer (leading zero bytes) at position {Position}"); + } } } @@ -997,14 +1026,18 @@ public int DecodeInt() return result; } - public byte[] DecodeByteArray() + public byte[] DecodeByteArray(bool allowLeadingZeroBytes = true) { - return DecodeByteArraySpan().ToArray(); + return DecodeByteArraySpan(allowLeadingZeroBytes).ToArray(); } - public Span DecodeByteArraySpan() + public Span DecodeByteArraySpan(bool allowLeadingZeroBytes = true) { int prefix = ReadByte(); + if (!allowLeadingZeroBytes && prefix == 0) + { + throw new RlpException($"Non-canonical ulong (leading zero bytes) at position {Position}"); + } if (prefix < 128) { @@ -1127,9 +1160,15 @@ public string DecodeString() return Encoding.UTF8.GetString(bytes); } - public long DecodeLong() + public long DecodeLong(bool allowLeadingZeroBytes = true) { int prefix = ReadByte(); + + if (!allowLeadingZeroBytes && prefix == 0) + { + throw new RlpException($"Non-canonical long (leading zero bytes) at position {Position}"); + } + if (prefix < 128) { return prefix; @@ -1152,7 +1191,11 @@ public long DecodeLong() result = result << 8; if (i <= length) { - result = result | PeekByte(length - i); + result |= PeekByte(length - i); + if (!allowLeadingZeroBytes && result == 0) + { + throw new RlpException($"Non-canonical long (leading zero bytes) at position {Position}"); + } } } @@ -1161,9 +1204,15 @@ public long DecodeLong() return result; } - public ulong DecodeULong() + public ulong DecodeULong(bool allowLeadingZeroBytes = true) { int prefix = ReadByte(); + + if (!allowLeadingZeroBytes && prefix == 0) + { + throw new RlpException($"Non-canonical ulong (leading zero bytes) at position {Position}"); + } + if (prefix < 128) { return (ulong)prefix; @@ -1186,7 +1235,11 @@ public ulong DecodeULong() result = result << 8; if (i <= length) { - result = result | PeekByte(length - i); + result |= PeekByte(length - i); + if (!allowLeadingZeroBytes && result == 0) + { + throw new RlpException($"Non-canonical ulong (leading zero bytes) at position {Position}"); + } } } diff --git a/src/Nethermind/Nethermind.Serialization.Rlp/RlpStream.cs b/src/Nethermind/Nethermind.Serialization.Rlp/RlpStream.cs index 78c43cd42a2..493c81520b3 100644 --- a/src/Nethermind/Nethermind.Serialization.Rlp/RlpStream.cs +++ b/src/Nethermind/Nethermind.Serialization.Rlp/RlpStream.cs @@ -619,7 +619,7 @@ public int PeekNextRlpLength() int length = DeserializeLength(lengthOfLength); if (length < 56) { - throw new RlpException("Expected length greater or equal 56 and was {length}"); + throw new RlpException($"Expected length greater or equal 56 and was {length}"); } result = (lengthOfLength + 1, length); @@ -847,9 +847,15 @@ public void Check(int nextCheck) return new Address(buffer); } - public UInt256 DecodeUInt256() + public UInt256 DecodeUInt256(bool allowLeadingZeroBytes = true) { byte byteValue = PeekByte(); + + if (!allowLeadingZeroBytes && byteValue == 0) + { + throw new RlpException($"Non-canonical UInt256 (leading zero bytes) at position {Position}"); + } + if (byteValue < 128) { SkipBytes(1); @@ -857,15 +863,21 @@ public UInt256 DecodeUInt256() } ReadOnlySpan byteSpan = DecodeByteArraySpan(); + if (byteSpan.Length > 32) { throw new ArgumentException(); } + if (!allowLeadingZeroBytes && byteSpan.Length > 1 && byteSpan[0] == 0) + { + throw new RlpException($"Non-canonical UInt256 (leading zero bytes) at position {Position}"); + } + return new UInt256(byteSpan, true); } - public UInt256? DecodeNullableUInt256() + public UInt256? DecodeNullableUInt256(bool allowLeadingZeroBytes = true) { if (PeekByte() == 0) { @@ -873,12 +885,16 @@ public UInt256 DecodeUInt256() return null; } - return DecodeUInt256(); + return DecodeUInt256(allowLeadingZeroBytes); } - public BigInteger DecodeUBigInt() + public BigInteger DecodeUBigInt(bool allowLeadingZeroBytes = true) { ReadOnlySpan bytes = DecodeByteArraySpan(); + if (!allowLeadingZeroBytes && bytes.Length > 1 && bytes[0] == 0) + { + throw new RlpException($"Non-canonical UBigInt (leading zero bytes) at position {Position}"); + } return bytes.ToUnsignedBigInteger(); } @@ -1023,9 +1039,15 @@ public byte DecodeByte() : bytes[1]; } - public int DecodeInt() + public int DecodeInt(bool allowLeadingZeroBytes = true) { int prefix = ReadByte(); + + if (!allowLeadingZeroBytes && prefix == 0) + { + throw new RlpException($"Non-canonical integer (leading zero bytes) at position {Position}"); + } + if (prefix < 128) { return prefix; @@ -1048,7 +1070,11 @@ public int DecodeInt() result = result << 8; if (i <= length) { - result = result | PeekByte(length - i); + result |= PeekByte(length - i); + if (!allowLeadingZeroBytes && result == 0) + { + throw new RlpException($"Non-canonical integer (leading zero bytes) at position {Position}"); + } } } @@ -1057,15 +1083,25 @@ public int DecodeInt() return result; } - public uint DecodeUInt() + public uint DecodeUInt(bool allowLeadingZeroBytes = true) { ReadOnlySpan bytes = DecodeByteArraySpan(); + if (!allowLeadingZeroBytes && bytes.Length > 1 && bytes[0] == 0) + { + throw new RlpException($"Non-canonical UInt (leading zero bytes) at position {Position}"); + } return bytes.Length == 0 ? 0 : bytes.ReadEthUInt32(); } - public long DecodeLong() + public long DecodeLong(bool allowLeadingZeroBytes = true) { int prefix = ReadByte(); + + if (!allowLeadingZeroBytes && prefix == 0) + { + throw new RlpException($"Non-canonical long (leading zero bytes) at position {Position}"); + } + if (prefix < 128) { return prefix; @@ -1088,7 +1124,11 @@ public long DecodeLong() result = result << 8; if (i <= length) { - result = result | PeekByte(length - i); + result |= PeekByte(length - i); + if (!allowLeadingZeroBytes && result == 0) + { + throw new RlpException($"Non-canonical long (leading zero bytes) at position {Position}"); + } } } @@ -1097,9 +1137,15 @@ public long DecodeLong() return result; } - public ulong DecodeULong() + public ulong DecodeULong(bool allowLeadingZeroBytes = true) { int prefix = ReadByte(); + + if (!allowLeadingZeroBytes && prefix == 0) + { + throw new RlpException($"Non-canonical ulong (leading zero bytes) at position {Position}"); + } + if (prefix < 128) { return (ulong)prefix; @@ -1122,7 +1168,11 @@ public ulong DecodeULong() result = result << 8; if (i <= length) { - result = result | PeekByte(length - i); + result |= PeekByte(length - i); + if (!allowLeadingZeroBytes && result == 0) + { + throw new RlpException($"Non-canonical ulong (leading zero bytes) at position {Position}"); + } } } @@ -1131,22 +1181,30 @@ public ulong DecodeULong() return result; } - public ulong DecodeUlong() + public ulong DecodeUlong(bool allowLeadingZeroBytes = true) { ReadOnlySpan bytes = DecodeByteArraySpan(); + if (!allowLeadingZeroBytes && bytes.Length > 1 && bytes[0] == 0) + { + throw new RlpException($"Non-canonical ulong (leading zero bytes) at position {Position}"); + } return bytes.Length == 0 ? 0L : bytes.ReadEthUInt64(); } - public byte[] DecodeByteArray() + public byte[] DecodeByteArray(bool allowLeadingZeroBytes = true) { - return DecodeByteArraySpan().ToArray(); + return DecodeByteArraySpan(allowLeadingZeroBytes).ToArray(); } - public ReadOnlySpan DecodeByteArraySpan() + public ReadOnlySpan DecodeByteArraySpan(bool allowLeadingZeroBytes = true) { int prefix = ReadByte(); if (prefix == 0) { + if (!allowLeadingZeroBytes) + { + throw new RlpException($"Non-canonical ulong (leading zero bytes) at position {Position}"); + } return new byte[] { 0 }; } diff --git a/src/Nethermind/Nethermind.Serialization.Rlp/TxDecoder.cs b/src/Nethermind/Nethermind.Serialization.Rlp/TxDecoder.cs index fb1388f681b..5246825810b 100644 --- a/src/Nethermind/Nethermind.Serialization.Rlp/TxDecoder.cs +++ b/src/Nethermind/Nethermind.Serialization.Rlp/TxDecoder.cs @@ -60,9 +60,8 @@ public class TxDecoder : } } - int transactionLength = rlpStream.PeekNextRlpLength(); + int transactionLength = rlpStream.ReadSequenceLength(); int lastCheck = rlpStream.Position + transactionLength; - rlpStream.SkipLength(); switch (transaction.Type) { @@ -103,70 +102,70 @@ public class TxDecoder : private void DecodeLegacyPayloadWithoutSig(T transaction, RlpStream rlpStream) { - transaction.Nonce = rlpStream.DecodeUInt256(); - transaction.GasPrice = rlpStream.DecodeUInt256(); - transaction.GasLimit = rlpStream.DecodeLong(); + transaction.Nonce = rlpStream.DecodeUInt256(allowLeadingZeroBytes: false); + transaction.GasPrice = rlpStream.DecodeUInt256(allowLeadingZeroBytes: false); + transaction.GasLimit = rlpStream.DecodeLong(allowLeadingZeroBytes: false); transaction.To = rlpStream.DecodeAddress(); - transaction.Value = rlpStream.DecodeUInt256(); + transaction.Value = rlpStream.DecodeUInt256(allowLeadingZeroBytes: false); transaction.Data = rlpStream.DecodeByteArray(); } private void DecodeAccessListPayloadWithoutSig(T transaction, RlpStream rlpStream, RlpBehaviors rlpBehaviors) { - transaction.ChainId = rlpStream.DecodeULong(); - transaction.Nonce = rlpStream.DecodeUInt256(); - transaction.GasPrice = rlpStream.DecodeUInt256(); - transaction.GasLimit = rlpStream.DecodeLong(); + transaction.ChainId = rlpStream.DecodeULong(allowLeadingZeroBytes: false); + transaction.Nonce = rlpStream.DecodeUInt256(allowLeadingZeroBytes: false); + transaction.GasPrice = rlpStream.DecodeUInt256(allowLeadingZeroBytes: false); + transaction.GasLimit = rlpStream.DecodeLong(allowLeadingZeroBytes: false); transaction.To = rlpStream.DecodeAddress(); - transaction.Value = rlpStream.DecodeUInt256(); + transaction.Value = rlpStream.DecodeUInt256(allowLeadingZeroBytes: false); transaction.Data = rlpStream.DecodeByteArray(); transaction.AccessList = _accessListDecoder.Decode(rlpStream, rlpBehaviors); } private void DecodeEip1559PayloadWithoutSig(T transaction, RlpStream rlpStream, RlpBehaviors rlpBehaviors) { - transaction.ChainId = rlpStream.DecodeULong(); - transaction.Nonce = rlpStream.DecodeUInt256(); - transaction.GasPrice = rlpStream.DecodeUInt256(); // gas premium - transaction.DecodedMaxFeePerGas = rlpStream.DecodeUInt256(); + transaction.ChainId = rlpStream.DecodeULong(allowLeadingZeroBytes: false); + transaction.Nonce = rlpStream.DecodeUInt256(allowLeadingZeroBytes: false); + transaction.GasPrice = rlpStream.DecodeUInt256(allowLeadingZeroBytes: false); // gas premium + transaction.DecodedMaxFeePerGas = rlpStream.DecodeUInt256(allowLeadingZeroBytes: false); transaction.GasLimit = rlpStream.DecodeLong(); transaction.To = rlpStream.DecodeAddress(); - transaction.Value = rlpStream.DecodeUInt256(); + transaction.Value = rlpStream.DecodeUInt256(allowLeadingZeroBytes: false); transaction.Data = rlpStream.DecodeByteArray(); transaction.AccessList = _accessListDecoder.Decode(rlpStream, rlpBehaviors); } private void DecodeLegacyPayloadWithoutSig(T transaction, ref Rlp.ValueDecoderContext decoderContext) { - transaction.Nonce = decoderContext.DecodeUInt256(); - transaction.GasPrice = decoderContext.DecodeUInt256(); - transaction.GasLimit = decoderContext.DecodeLong(); + transaction.Nonce = decoderContext.DecodeUInt256(allowLeadingZeroBytes: false); + transaction.GasPrice = decoderContext.DecodeUInt256(allowLeadingZeroBytes: false); + transaction.GasLimit = decoderContext.DecodeLong(allowLeadingZeroBytes: false); transaction.To = decoderContext.DecodeAddress(); - transaction.Value = decoderContext.DecodeUInt256(); + transaction.Value = decoderContext.DecodeUInt256(allowLeadingZeroBytes: false); transaction.Data = decoderContext.DecodeByteArray(); } private void DecodeAccessListPayloadWithoutSig(T transaction, ref Rlp.ValueDecoderContext decoderContext, RlpBehaviors rlpBehaviors) { - transaction.ChainId = decoderContext.DecodeULong(); - transaction.Nonce = decoderContext.DecodeUInt256(); - transaction.GasPrice = decoderContext.DecodeUInt256(); - transaction.GasLimit = decoderContext.DecodeLong(); + transaction.ChainId = decoderContext.DecodeULong(allowLeadingZeroBytes: false); + transaction.Nonce = decoderContext.DecodeUInt256(allowLeadingZeroBytes: false); + transaction.GasPrice = decoderContext.DecodeUInt256(allowLeadingZeroBytes: false); + transaction.GasLimit = decoderContext.DecodeLong(allowLeadingZeroBytes: false); transaction.To = decoderContext.DecodeAddress(); - transaction.Value = decoderContext.DecodeUInt256(); + transaction.Value = decoderContext.DecodeUInt256(allowLeadingZeroBytes: false); transaction.Data = decoderContext.DecodeByteArray(); transaction.AccessList = _accessListDecoder.Decode(ref decoderContext, rlpBehaviors); } private void DecodeEip1559PayloadWithoutSig(T transaction, ref Rlp.ValueDecoderContext decoderContext, RlpBehaviors rlpBehaviors) { - transaction.ChainId = decoderContext.DecodeULong(); - transaction.Nonce = decoderContext.DecodeUInt256(); - transaction.GasPrice = decoderContext.DecodeUInt256(); // gas premium - transaction.DecodedMaxFeePerGas = decoderContext.DecodeUInt256(); - transaction.GasLimit = decoderContext.DecodeLong(); + transaction.ChainId = decoderContext.DecodeULong(allowLeadingZeroBytes: false); + transaction.Nonce = decoderContext.DecodeUInt256(allowLeadingZeroBytes: false); + transaction.GasPrice = decoderContext.DecodeUInt256(allowLeadingZeroBytes: false); // gas premium + transaction.DecodedMaxFeePerGas = decoderContext.DecodeUInt256(allowLeadingZeroBytes: false); + transaction.GasLimit = decoderContext.DecodeLong(allowLeadingZeroBytes: false); transaction.To = decoderContext.DecodeAddress(); - transaction.Value = decoderContext.DecodeUInt256(); + transaction.Value = decoderContext.DecodeUInt256(allowLeadingZeroBytes: false); transaction.Data = decoderContext.DecodeByteArray(); transaction.AccessList = _accessListDecoder.Decode(ref decoderContext, rlpBehaviors); } @@ -241,9 +240,8 @@ private void EncodeEip1559PayloadWithoutPayload(T item, RlpStream stream, RlpBeh } } - int transactionLength = decoderContext.PeekNextRlpLength(); + int transactionLength = decoderContext.ReadSequenceLength(); int lastCheck = decoderContext.Position + transactionLength; - decoderContext.SkipLength(); switch (transaction.Type) { @@ -286,7 +284,7 @@ private static void DecodeSignature( RlpBehaviors rlpBehaviors, T transaction) { - ReadOnlySpan vBytes = rlpStream.DecodeByteArraySpan(); + ReadOnlySpan vBytes = rlpStream.DecodeByteArraySpan(allowLeadingZeroBytes: false); ReadOnlySpan rBytes = rlpStream.DecodeByteArraySpan(); ReadOnlySpan sBytes = rlpStream.DecodeByteArraySpan(); ApplySignature(transaction, vBytes, rBytes, sBytes, rlpBehaviors); @@ -297,7 +295,7 @@ private static void DecodeSignature( RlpBehaviors rlpBehaviors, T transaction) { - ReadOnlySpan vBytes = decoderContext.DecodeByteArraySpan(); + ReadOnlySpan vBytes = decoderContext.DecodeByteArraySpan(allowLeadingZeroBytes: false); ReadOnlySpan rBytes = decoderContext.DecodeByteArraySpan(); ReadOnlySpan sBytes = decoderContext.DecodeByteArraySpan(); ApplySignature(transaction, vBytes, rBytes, sBytes, rlpBehaviors); @@ -342,7 +340,7 @@ private static void ApplySignature( if (isSignatureOk) { ulong v = vBytes.ReadEthUInt64(); - if (v < Signature.VOffset) + if (transaction.Type != TxType.Legacy && v < Signature.VOffset) { v += Signature.VOffset; } diff --git a/src/Nethermind/Nethermind.TxPool.Test/TxPoolTests.cs b/src/Nethermind/Nethermind.TxPool.Test/TxPoolTests.cs index 8504adda5ad..84a2b2c82c0 100644 --- a/src/Nethermind/Nethermind.TxPool.Test/TxPoolTests.cs +++ b/src/Nethermind/Nethermind.TxPool.Test/TxPoolTests.cs @@ -512,7 +512,7 @@ public void should_add_underpaid_txs_to_full_TxPool_only_if_local(bool isLocal) AcceptTxResult result = _txPool.SubmitTx(tx, txHandlingOptions); _txPool.GetPendingTransactions().Length.Should().Be(30); _txPool.GetOwnPendingTransactions().Length.Should().Be(isLocal ? 1 : 0); - result.ToString().Should().Contain(isLocal ? nameof(AcceptTxResult.Accepted) : nameof(AcceptTxResult.FeeTooLow)); + result.ToString().Should().Contain(isLocal ? nameof(AcceptTxResult.FeeTooLowToCompete) : nameof(AcceptTxResult.FeeTooLow)); } [TestCase(0)] @@ -1374,10 +1374,12 @@ public void should_increase_nonce_when_transaction_not_included_in_txPool_but_br .SignedAndResolved(_ethereumEcdsa, TestItem.PrivateKeyA).TestObject; EnsureSenderBalance(TestItem.AddressA, UInt256.MaxValue); - _txPool.SubmitTx(firstTx, TxHandlingOptions.PersistentBroadcast).Should().Be(AcceptTxResult.Accepted); - peer.Received().SendNewTransaction(firstTx); - _txPool.SubmitTx(secondTx, TxHandlingOptions.PersistentBroadcast).Should().Be(AcceptTxResult.Accepted); - peer.Received().SendNewTransaction(secondTx); + _txPool.SubmitTx(firstTx, TxHandlingOptions.None).Should().Be(AcceptTxResult.Accepted); + _txPool.SubmitTx(secondTx, TxHandlingOptions.None).Should().Be(AcceptTxResult.Accepted); + _txPool.GetPendingTransactions().Should().Contain(firstTx); + _txPool.GetPendingTransactions().Should().Contain(secondTx); + _txPool.GetOwnPendingTransactions().Should().NotContain(firstTx); + _txPool.GetOwnPendingTransactions().Should().NotContain(secondTx); // Send cheap transaction => Not included in txPool Transaction cheapTx = Build.A.Transaction @@ -1386,7 +1388,9 @@ public void should_increase_nonce_when_transaction_not_included_in_txPool_but_br .WithMaxFeePerGas(1) .WithMaxPriorityFeePerGas(1) .SignedAndResolved(_ethereumEcdsa, TestItem.PrivateKeyA).TestObject; - _txPool.SubmitTx(cheapTx, TxHandlingOptions.PersistentBroadcast).Should().Be(AcceptTxResult.Accepted); + _txPool.SubmitTx(cheapTx, TxHandlingOptions.PersistentBroadcast).Should().Be(AcceptTxResult.FeeTooLowToCompete); + _txPool.GetPendingTransactions().Should().NotContain(cheapTx); + _txPool.GetOwnPendingTransactions().Should().Contain(cheapTx); peer.Received().SendNewTransaction(cheapTx); // Send transaction with increased nonce => NonceGap should not appear as previous transaction is broadcasted, should be accepted @@ -1396,7 +1400,9 @@ public void should_increase_nonce_when_transaction_not_included_in_txPool_but_br .WithMaxFeePerGas(1) .WithMaxPriorityFeePerGas(1) .SignedAndResolved(_ethereumEcdsa, TestItem.PrivateKeyA).TestObject; - _txPool.SubmitTx(fourthTx, TxHandlingOptions.PersistentBroadcast).Should().Be(AcceptTxResult.Accepted); + _txPool.SubmitTx(fourthTx, TxHandlingOptions.PersistentBroadcast).Should().Be(AcceptTxResult.FeeTooLowToCompete); + _txPool.GetPendingTransactions().Should().NotContain(fourthTx); + _txPool.GetOwnPendingTransactions().Should().Contain(fourthTx); peer.Received().SendNewTransaction(fourthTx); } diff --git a/src/Nethermind/Nethermind.TxPool/Collections/SortedPool.cs b/src/Nethermind/Nethermind.TxPool/Collections/SortedPool.cs index a09d5ba8f73..69026fbbe6b 100644 --- a/src/Nethermind/Nethermind.TxPool/Collections/SortedPool.cs +++ b/src/Nethermind/Nethermind.TxPool/Collections/SortedPool.cs @@ -407,6 +407,19 @@ public bool TryGetBucket(TGroupKey groupKey, out TValue[] items) return false; } + [MethodImpl(MethodImplOptions.Synchronized)] + public bool TryGetBucketsWorstValue(TGroupKey groupKey, out TValue? item) + { + if (_buckets.TryGetValue(groupKey, out EnhancedSortedSet? bucket)) + { + item = bucket.Max; + return true; + } + + item = default; + return false; + } + [MethodImpl(MethodImplOptions.Synchronized)] public void UpdatePool(Func, IEnumerable<(TValue Tx, Action? Change)>> changingElements) { diff --git a/src/Nethermind/Nethermind.TxPool/Comparison/CompareReplacedTxByFee.cs b/src/Nethermind/Nethermind.TxPool/Comparison/CompareReplacedTxByFee.cs index c4805b2b1ce..1982ac4574e 100644 --- a/src/Nethermind/Nethermind.TxPool/Comparison/CompareReplacedTxByFee.cs +++ b/src/Nethermind/Nethermind.TxPool/Comparison/CompareReplacedTxByFee.cs @@ -32,7 +32,10 @@ public int Compare(Transaction? x, Transaction? y) if (!x.Supports1559 && !y.Supports1559) { y.GasPrice.Divide(PartOfFeeRequiredToIncrease, out UInt256 bumpGasPrice); - return (y.GasPrice + bumpGasPrice).CompareTo(x.GasPrice); + int gasPriceResult = (y.GasPrice + bumpGasPrice).CompareTo(x.GasPrice); + // return -1 (replacement accepted) if fee bump is exactly by PartOfFeeRequiredToIncrease + // never return 0 - it's allowed or not + return gasPriceResult != 0 ? gasPriceResult : bumpGasPrice > 0 ? -1 : 1; } /* MaxFeePerGas for legacy will be GasPrice and MaxPriorityFeePerGas will be GasPrice too @@ -41,7 +44,10 @@ so we can compare legacy txs without any problems */ if (y.MaxFeePerGas + bumpMaxFeePerGas > x.MaxFeePerGas) return 1; y.MaxPriorityFeePerGas.Divide(PartOfFeeRequiredToIncrease, out UInt256 bumpMaxPriorityFeePerGas); - return (y.MaxPriorityFeePerGas + bumpMaxPriorityFeePerGas).CompareTo(x.MaxPriorityFeePerGas); + int result = (y.MaxPriorityFeePerGas + bumpMaxPriorityFeePerGas).CompareTo(x.MaxPriorityFeePerGas); + // return -1 (replacement accepted) if fee bump is exactly by PartOfFeeRequiredToIncrease + // never return 0 - it's allowed or not + return result != 0 ? result : (bumpMaxFeePerGas > 0 && bumpMaxPriorityFeePerGas > 0) ? -1 : 1; } } diff --git a/src/Nethermind/Nethermind.TxPool/TxPool.cs b/src/Nethermind/Nethermind.TxPool/TxPool.cs index ebe6fa28218..4a2649eccb9 100644 --- a/src/Nethermind/Nethermind.TxPool/TxPool.cs +++ b/src/Nethermind/Nethermind.TxPool/TxPool.cs @@ -346,10 +346,15 @@ private AcceptTxResult AddCore(Transaction tx, TxFilteringState state, bool isPe lock (_locker) { bool eip1559Enabled = _specProvider.GetCurrentHeadSpec().IsEip1559Enabled; + UInt256 effectiveGasPrice = tx.CalculateEffectiveGasPrice(eip1559Enabled, _headInfo.CurrentBaseFee); + + _transactions.TryGetBucketsWorstValue(tx.SenderAddress!, out Transaction? worstTx); + tx.GasBottleneck = (worstTx is null || effectiveGasPrice <= worstTx.GasBottleneck) + ? effectiveGasPrice + : worstTx.GasBottleneck; - tx.GasBottleneck = tx.CalculateEffectiveGasPrice(eip1559Enabled, _headInfo.CurrentBaseFee); bool inserted = _transactions.TryInsert(tx.Hash!, tx, out Transaction? removed); - if (inserted) + if (inserted && tx.Hash != removed?.Hash) { _transactions.UpdateGroup(tx.SenderAddress!, state.SenderAccount, UpdateBucketWithAddedTransaction); Metrics.PendingTransactionsAdded++; @@ -367,6 +372,11 @@ private AcceptTxResult AddCore(Transaction tx, TxFilteringState state, bool isPe } else { + if (isPersistentBroadcast && inserted) + { + // it means it was added and immediately evicted - we are adding only to persistent broadcast + _broadcaster.Broadcast(tx, isPersistentBroadcast); + } Metrics.PendingTransactionsPassedFiltersButCannotCompeteOnFees++; return AcceptTxResult.FeeTooLowToCompete; } diff --git a/src/Nethermind/Nethermind.Wallet/WalletExtensions.cs b/src/Nethermind/Nethermind.Wallet/WalletExtensions.cs index 297ce79b8b6..6974c681d66 100644 --- a/src/Nethermind/Nethermind.Wallet/WalletExtensions.cs +++ b/src/Nethermind/Nethermind.Wallet/WalletExtensions.cs @@ -41,7 +41,7 @@ public static void Sign(this IWallet @this, Transaction tx, ulong chainId) throw new CryptographicException($"Failed to sign tx {tx.Hash} using the {tx.SenderAddress} address."); } - tx.Signature.V = tx.Signature.V + 8 + 2 * chainId; + tx.Signature.V = tx.Type == TxType.Legacy ? tx.Signature.V + 8 + 2 * chainId : (ulong)(tx.Signature.RecoveryId + 27); } } }