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

Refactor broadcasting for a needs of 4844 #5485

Merged
merged 10 commits into from
Apr 25, 2023
Merged

Refactor broadcasting for a needs of 4844 #5485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a3ea4fe
send hash of added local blob tx in eth68 and nothing in older versions
marcindsobczak Jan 11, 2023
2f9edf6
move tx size logic to extension
marcindsobczak Mar 22, 2023
744e9d7
adjust broadcaster
marcindsobczak Mar 22, 2023
3b23142
adjust handlers
marcindsobczak Mar 22, 2023
9dbf58a
remove redundant check
marcindsobczak Mar 22, 2023
1ddfd28
fix?
marcindsobczak Mar 22, 2023
8a35cf5
add test for MaxSizeOfTxForBroadcast
marcindsobczak Mar 23, 2023
d15e6d4
cosmetic
marcindsobczak Mar 23, 2023
00674e7
cosmetic
marcindsobczak Mar 23, 2023
761a51e
Merge remote-tracking branch 'origin/master' into feature/networking_…
marcindsobczak Mar 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/Nethermind/Nethermind.Network.Test/P2P/Subprotocols/Eth/V62/Eth62ProtocolHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
using Nethermind.Core.Crypto;
using Nethermind.Core.Test.Builders;
using Nethermind.Core.Timers;
using Nethermind.Crypto;
using Nethermind.Int256;
using Nethermind.Logging;
using Nethermind.Network.P2P;
using Nethermind.Network.P2P.EventArg;
Expand Down Expand Up @@ -519,7 +517,7 @@ public void should_send_single_transaction_even_if_exceed_MaxPacketSize(int data
}

Transaction tx = txs[0];
int sizeOfOneTx = tx.GetLength(new TxDecoder());
int sizeOfOneTx = tx.GetLength();
int numberOfTxsInOneMsg = Math.Max(TransactionsMessage.MaxPacketSize / sizeOfOneTx, 1);
int nonFullMsgTxsCount = txCount % numberOfTxsInOneMsg;
int messagesCount = txCount / numberOfTxsInOneMsg + (nonFullMsgTxsCount > 0 ? 1 : 0);
Expand Down
6 changes: 2 additions & 4 deletions src/Nethermind/Nethermind.Network.Test/P2P/Subprotocols/Eth/V65/Eth65ProtocolHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
using System.Collections.Generic;
using System.Net;
using FluentAssertions;
using Nethermind.Blockchain;
using Nethermind.Consensus;
using Nethermind.Core;
using Nethermind.Core.Crypto;
Expand All @@ -19,7 +18,6 @@
using Nethermind.Network.P2P.Subprotocols.Eth.V65;
using Nethermind.Network.P2P.Subprotocols.Eth.V65.Messages;
using Nethermind.Network.Test.Builders;
using Nethermind.Serialization.Rlp;
using Nethermind.Stats;
using Nethermind.Stats.Model;
using Nethermind.Synchronization;
Expand Down Expand Up @@ -132,7 +130,7 @@ public void should_send_more_than_MaxCount_hashes_in_more_than_one_NewPooledTran
public void should_send_requested_PooledTransactions_up_to_MaxPacketSize()
{
Transaction tx = Build.A.Transaction.WithData(new byte[1024]).SignedAndResolved().TestObject;
int sizeOfOneTx = tx.GetLength(new TxDecoder());
int sizeOfOneTx = tx.GetLength();
int numberOfTxsInOneMsg = TransactionsMessage.MaxPacketSize / sizeOfOneTx;
_transactionPool.TryGetPendingTransaction(Arg.Any<Keccak>(), out Arg.Any<Transaction>())
.Returns(x =>
Expand All @@ -156,7 +154,7 @@ public void should_send_requested_PooledTransactions_up_to_MaxPacketSize()
public void should_send_single_requested_PooledTransaction_even_if_exceed_MaxPacketSize(int dataSize)
{
Transaction tx = Build.A.Transaction.WithData(new byte[dataSize]).SignedAndResolved().TestObject;
int sizeOfOneTx = tx.GetLength(new TxDecoder());
int sizeOfOneTx = tx.GetLength();
int numberOfTxsInOneMsg = Math.Max(TransactionsMessage.MaxPacketSize / sizeOfOneTx, 1);
_transactionPool.TryGetPendingTransaction(Arg.Any<Keccak>(), out Arg.Any<Transaction>())
.Returns(x =>
Expand Down
1 change: 0 additions & 1 deletion src/Nethermind/Nethermind.Network.Test/P2P/Subprotocols/Eth/V68/Eth68ProtocolHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
using System.Net;
using DotNetty.Buffers;
using FluentAssertions;
using Nethermind.Blockchain;
using Nethermind.Consensus;
using Nethermind.Core;
using Nethermind.Core.Crypto;
Expand Down
18 changes: 14 additions & 4 deletions src/Nethermind/Nethermind.Network/P2P/ProtocolHandlers/SyncPeerProtocolHandlerBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
using Nethermind.Network.P2P.Subprotocols.Eth.V62;
using Nethermind.Network.P2P.Subprotocols.Eth.V62.Messages;
using Nethermind.Network.P2P.Subprotocols.Eth.V63.Messages;
using Nethermind.Network.Rlpx;
using Nethermind.Serialization.Rlp;
using Nethermind.Stats;
using Nethermind.Stats.Model;
Expand Down Expand Up @@ -183,7 +182,18 @@ public virtual Task<byte[][]> GetNodeData(IReadOnlyList<Keccak> hashes, Cancella

public void SendNewTransaction(Transaction tx)
{
SendMessage(new[] { tx });
if (tx.Hash != null && NotifiedTransactions.Set(tx.Hash))
Copy link
Contributor

Choose a reason for hiding this comment

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

QQ: we haven't had this if tx.Hash != null && NotifiedTransactions.Set(tx.Hash) before. Is it fixing some bug, but not related to 4844?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is recently added feature #5449
It was changed for SendNewTransactions:
https://github.com/NethermindEth/nethermind/pull/5449/files#diff-e2801b05e4ee2fda5592da789a82f0e4c733c8b20c8688020cbb0de71e3db5e5
and SendNewTransaction was skipped. I just added it here too.
It is a protection from sending the same tx few times to one peer - we are caching sent transactions. Not directly related to 4844

Copy link
Member

Choose a reason for hiding this comment

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

Would move this to ShouldNotifyTransaction method for this condition in both places.

{
SendNewTransactionCore(tx);
}
}

protected virtual void SendNewTransactionCore(Transaction tx)
{
if (tx.Type != TxType.Blob) //additional protection from sending full blob-type txs
{
SendMessage(new[] { tx });
}
}

public void SendNewTransactions(IEnumerable<Transaction> txs, bool sendFullTx = false)
Expand All @@ -209,7 +219,7 @@ protected virtual void SendNewTransactionsCore(IEnumerable<Transaction> txs, boo

foreach (Transaction tx in txs)
{
int txSize = tx.GetLength(_txDecoder);
int txSize = tx.GetLength();

if (txSize > packetSizeLeft && txsToSend.Count > 0)
{
Expand All @@ -218,7 +228,7 @@ protected virtual void SendNewTransactionsCore(IEnumerable<Transaction> txs, boo
packetSizeLeft = TransactionsMessage.MaxPacketSize;
}

if (tx.Hash is not null)
if (tx.Hash is not null && tx.Type != TxType.Blob) //additional protection from sending full blob-type txs
Copy link
Member

Choose a reason for hiding this comment

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

Isn't there tx.SupportsBlobs that would be better for future as an abstraction than just checking Type?

{
txsToSend.Add(tx);
packetSizeLeft -= txSize;
Expand Down
3 changes: 1 addition & 2 deletions src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V65/Eth65ProtocolHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using Nethermind.Consensus;
using Nethermind.Core;
using Nethermind.Core.Collections;
Expand Down Expand Up @@ -119,7 +118,7 @@ internal PooledTransactionsMessage FulfillPooledTransactionsRequest(
{
if (_txPool.TryGetPendingTransaction(msg.Hashes[i], out Transaction tx))
{
int txSize = tx.GetLength(_txDecoder);
int txSize = tx.GetLength();

if (txSize > packetSizeLeft && txsToSend.Count > 0)
{
Expand Down
14 changes: 13 additions & 1 deletion src/Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V68/Eth68ProtocolHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,18 @@ private void Handle(NewPooledTransactionHashesMessage68 message)
if (isTrace) Logger.Trace($"OUT {Counter:D5} {nameof(NewPooledTransactionHashesMessage68)} to {Node:c} in {stopwatch.Elapsed.TotalMilliseconds}ms");
}

protected override void SendNewTransactionCore(Transaction tx)
{
if (tx.CanBeBroadcast())
{
base.SendNewTransactionCore(tx);
}
else
{
SendMessage(new byte[] { (byte)tx.Type }, new int[] { tx.GetLength() }, new Keccak[] { tx.Hash });
}
}

protected override void SendNewTransactionsCore(IEnumerable<Transaction> txs, bool sendFullTx)
{
if (sendFullTx)
Expand All @@ -117,7 +129,7 @@ protected override void SendNewTransactionsCore(IEnumerable<Transaction> txs, bo
if (tx.Hash is not null)
{
types.Add((byte)tx.Type);
sizes.Add(tx.GetLength(_txDecoder));
sizes.Add(tx.GetLength());
hashes.Add(tx.Hash);
TxPool.Metrics.PendingTransactionsHashesSent++;
}
Expand Down
Loading