Improve eth/66 support #3616

fab-10 · 2022-03-21T13:44:21Z

PR description

Currently Besu has a limited support for sending NewPooledTransactionHashes messages, and other aspect related to reduce transactions synchronization traffic, described in the Ethereum Wire Protocol version 66.

Specifically:

Besu only uses NewPooledTransactionHashes for new local transactions, while it could be extended to any transaction added to the transaction pool
Besu does not limit the sending of the full transaction messages to a small fraction of the connected peers, and sends the new transaction hashes to all the remaining peers

This PR, extends eth/66 support and does some code refactoring, to remove some reduntant code and rename some classes to identify they are related to the NewPooledTransactionHashes message.

The main changes are:

Do not have a separate tracker for transaction hashes, since for them we can reuse PeerTransactionTracker, that tracks full transactions exchange history and sending queue with a peer. So PeerPendingTransactionTracker has been removed. --tx-pool-hashes-max-size is now deprecated and has no more effect and it will be removed in a future release.
When a new peer connects, if it support eth/6[56] then we send all the transaction hashes we have in the pool, otherwise we send the full transactions.
When new transactions are added to the pool, we send full transactions to peers without eth/6[56] support, or to a small fractions of all peers, and then we send only transaction hashes to the remaining peer that support eth/6[56]. Both transactions and transaction hashes are only sent if not already exchanged with that specific peer.

Fixed Issue(s)

Changelog

I thought about the changelog and included a changelog update if required.

Currently Besu handles hundreds of transaction message per seconds, so having a queue of 1M messages result in a lot of expired messages, make sense to reduce the capacity to drop incoming messages that could not be handled anyway. Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

Currently on mainnet there are many thousand of pending transactions exchanged between peers, but Besu has a short memory of what has been exchanged with a specific peer, with the result that the same transaction is often exchanged back and forth with the same peer, expecially when the peer is another Besu. Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

jflo · 2022-03-21T19:28:27Z

...eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionBroadcaster.java

+        (int) Math.ceil(Math.sqrt(ethContext.getEthPeers().getMaxPeers()));
+  }
+
+  public void handlePeerConnection(final EthPeer peer) {


non-blocking suggestion: since this isn't an interface method, and it is public, I think a more descriptive name might help. What about "relayTransactionsTo"? That would describe what is being done to the peer a bit more obviously.

make sense, rename to relayTransactionPoolTo

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java

jflo · 2022-03-21T19:42:20Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/Utils.java

+public class Utils {
+  private Utils() {}
+
+  static List<Hash> toHashList(final Collection<Transaction> transactions) {


Can we move this to Transaction instead of introducing a Util class? Seems like a reasonable location for it?

make sense, done

jflo · 2022-03-21T19:55:40Z

...um/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolTest.java

@@ -544,7 +554,7 @@ public void shouldDiscardRemoteTransactionThatAlreadyExistsBeforeValidation() {
    transactionPool.addRemoteTransactions(singletonList(transaction1));

    verify(pendingTransactions).containsTransaction(transaction1.getHash());
-    verify(pendingTransactions).tryEvictTransactionHash(transaction1.getHash());
+    //    verify(pendingTransactions).tryEvictTransactionHash(transaction1.getHash());


missed comment removal

good catch!

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

gezero · 2022-03-22T12:53:56Z

...eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionBroadcaster.java

+    this.newPooledTransactionHashesMessageSender = newPooledTransactionHashesMessageSender;
+    this.ethContext = ethContext;
+    this.numPeersToSendFullTransactions =
+        (int) Math.ceil(Math.sqrt(ethContext.getEthPeers().getMaxPeers()));


Would it maybe be better if this calculation got performed outside of constructor?

this value never changes during the execution, so I calculate it once in the constructor, are you suggesting to pass the result as parameter of the constructor?

I was just thinking that in case I would like to manually set up this value to some concrete number, I cannot because its calculation is hardcoded in the constructor. I don't think if I ever should or if it makes sense to do it.

the value is taken from https://eips.ethereum.org/EIPS/eip-2464 where it suggests to send full transactions only to the square root of peers. Not sure we want to ever change this value, but in that case we could make it configurable in future.

gezero · 2022-03-22T13:02:28Z

...eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionBroadcaster.java

+              peersWithTransactionHashesSupport.size());
+      // add peer from the other list to reach the required size
+      Collections.shuffle(peersWithTransactionHashesSupport);
+      IntStream.range(0, delta)


Maybe I don't understand properly what is going on here, I feel this would be more understandable if we would just use something like peersWithOnlyTransactionSupport.addAll(peersWithTransactionHashesSupport.sublist(..)); peersWithTransactionHashesSupport.removeRange(...);
This way it looks like a for cycle obscured in a functional way.

yes it could be hard to read, I like your suggestion but removeRange is protected, so I am rewriting this with a plain for cycle

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

sonarcloud · 2022-03-23T10:14:41Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

85.6% Coverage
0.0% Duplication

Currently Besu has a limited support for sending NewPooledTransactionHashes messages, and other aspect related to reduce transactions synchronization traffic, described in the Ethereum Wire Protocol version 66. Specifically: Besu only uses NewPooledTransactionHashes for new local transactions, while it could be extended to any transaction added to the transaction pool Besu does not limit the sending of the full transaction messages to a small fraction of the connected peers, and sends the new transaction hashes to all the remaining peers This PR, extends eth/66 support and does some code refactoring, to remove some reduntant code and rename some classes to identify they are related to the NewPooledTransactionHashes message. The main changes are: Do not have a separate tracker for transaction hashes, since for them we can reuse PeerTransactionTracker, that tracks full transactions exchange history and sending queue with a peer. So PeerPendingTransactionTracker has been removed. --tx-pool-hashes-max-size is now deprecated and has no more effect and it will be removed in a future release. When a new peer connects, if it support eth/6[56] then we send all the transaction hashes we have in the pool, otherwise we send the full transactions. When new transactions are added to the pool, we send full transactions to peers without eth/6[56] support, or to a small fractions of all peers, and then we send only transaction hashes to the remaining peer that support eth/6[56]. Both transactions and transaction hashes are only sent if not already exchanged with that specific peer. Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Justin Florentine <justin+github@florentine.us>

Currently Besu has a limited support for sending NewPooledTransactionHashes messages, and other aspect related to reduce transactions synchronization traffic, described in the Ethereum Wire Protocol version 66. Specifically: Besu only uses NewPooledTransactionHashes for new local transactions, while it could be extended to any transaction added to the transaction pool Besu does not limit the sending of the full transaction messages to a small fraction of the connected peers, and sends the new transaction hashes to all the remaining peers This PR, extends eth/66 support and does some code refactoring, to remove some reduntant code and rename some classes to identify they are related to the NewPooledTransactionHashes message. The main changes are: Do not have a separate tracker for transaction hashes, since for them we can reuse PeerTransactionTracker, that tracks full transactions exchange history and sending queue with a peer. So PeerPendingTransactionTracker has been removed. --tx-pool-hashes-max-size is now deprecated and has no more effect and it will be removed in a future release. When a new peer connects, if it support eth/6[56] then we send all the transaction hashes we have in the pool, otherwise we send the full transactions. When new transactions are added to the pool, we send full transactions to peers without eth/6[56] support, or to a small fractions of all peers, and then we send only transaction hashes to the remaining peer that support eth/6[56]. Both transactions and transaction hashes are only sent if not already exchanged with that specific peer. Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

fab-10 added 12 commits March 18, 2022 10:05

Improve trace log of transaction exchanges with other peers

ffda609

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

WIP

10d5a46

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

Merge branch 'fix-3524' into fix-eth66

2cceed7

WIP

8f17a13

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

Merge branch 'main' into fix-eth66

9ab1a6f

Remove commented code

91112dc

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

Deprecate --tx-pool-hashes-max-size since has no more any effect

91ac55e

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

Merge branch 'main' into fix-eth66

9e0ac99

Minor code optimizations

aefcc3c

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

Rename classes related to NewPooledTransactionHashes message

98fa5a3

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

fab-10 force-pushed the fix-eth66 branch 2 times, most recently from f9e1182 to e511d90 Compare March 21, 2022 16:35

Fix unit tests

3796693

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

fab-10 force-pushed the fix-eth66 branch from e511d90 to 3796693 Compare March 21, 2022 18:07

Various Sonar fixes

e28d43c

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

fab-10 marked this pull request as ready for review March 21, 2022 18:26

jflo approved these changes Mar 21, 2022

View reviewed changes

jflo requested a review from macfarla March 21, 2022 20:13

fab-10 added 2 commits March 22, 2022 09:19

Merge branch 'main' into fix-eth66

d2764b3

apply suggestions from code review

977d117

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

gezero approved these changes Mar 22, 2022

View reviewed changes

fab-10 added 6 commits March 22, 2022 15:53

Code improvements

3b462f2

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

Add unit tests for TransactionBroadcaster class

52f35d4

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

Merge branch 'main' into fix-eth66

532f530

Add trace log

c951cb9

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

Fix code smell

a074a22

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

Update CHANGELOG

39e7d01

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

fab-10 merged commit 6c179ba into hyperledger:main Mar 23, 2022

fab-10 deleted the fix-eth66 branch March 23, 2022 13:57

fab-10 added the doc-change-required Indicates an issue or PR that requires doc to be updated label Mar 24, 2022

bgravenorst mentioned this pull request Mar 25, 2022

Add a deprecation warning for --tx-pool-hashes-max-size hyperledger/besu-docs#998

Closed

bgravenorst removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Mar 25, 2022

fab-10 added the mainnet label Jun 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve eth/66 support #3616

Improve eth/66 support #3616

fab-10 commented Mar 21, 2022 •

edited

jflo Mar 21, 2022

fab-10 Mar 22, 2022

jflo Mar 21, 2022

fab-10 Mar 22, 2022

jflo Mar 21, 2022

fab-10 Mar 22, 2022

gezero Mar 22, 2022

fab-10 Mar 22, 2022

gezero Mar 22, 2022

fab-10 Mar 22, 2022

gezero Mar 22, 2022

fab-10 Mar 22, 2022

sonarcloud bot commented Mar 23, 2022

Improve eth/66 support #3616

Improve eth/66 support #3616

Conversation

fab-10 commented Mar 21, 2022 • edited

PR description

Fixed Issue(s)

Changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Mar 23, 2022

fab-10 commented Mar 21, 2022 •

edited