Fix "Backward sync stuck in a loop" #6749 #6756

pinges · 2024-03-19T08:13:29Z

PR description

This PR is fixing issue #6749, trying to make minimal changes to the existing implementation.

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

macfarla · 2024-03-19T08:36:31Z

...c/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContextTest.java

+    final CompletableFuture<Void> secondFuture =
+        context.syncBackwardsUntil(getBlockByNumber(REMOTE_HEIGHT - 1).getHash());
+    respondUntilFutureIsDone(secondFuture);
+    secondFuture.get();


is there any useful assertion to make at this point? or just that it didn't error out

it's still in dev, will tidy up this test now I know it works for the impl

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

garyschulte · 2024-03-19T20:33:21Z

gradle.properties

@@ -14,3 +14,4 @@ org.gradle.jvmargs=-Xmx4g \
 --add-opens jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED
 # Could be moved to sonar properties after https://sonarsource.atlassian.net/browse/SONARGRADL-134
 systemProp.sonar.gradle.skipCompile=true
+version=24.3.1-SNAPSHOT


we removed version from gradle.properties intentionally. Is this an errant add?

I had to add it to be able to use the Jenkins build from source. Sally found that problem in the scripts and I think it should get fixed soon. Once that is done I can get rid of it.

this doesn't look removed... ?

garyschulte

Good 'light touch' approach. good to merge once those two minor issues are resolved

garyschulte · 2024-03-19T20:36:03Z

...src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncAlgorithm.java

+                    // context.getEthContext().getEthPeers().peerCount() >
+                    // (context.getEthContext().getEthPeers().getMaxPeers() / 2)


it probably is wise to at least assert we have peers before we drop hashes, but making a qualitative judgement about the peers, when all we have to go by is a hash, is not straightforward.

IMO dropping the hash is straightforward and not catastrophic, since we will get a subsequent fcU. As long as we do not give up either:

too easily

in the absence of peers

I think a simple peer count check with a task retry backoff will suffice 👍

We do know that we have at least one peer, otherwise we would not get the max retries exception.

Even if we do remove a hash from the list that is on the chain, the BWS still works.

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

matkt · 2024-03-21T09:36:17Z

...c/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContextTest.java

@@ -83,7 +83,7 @@ public class BackwardSyncContextTest {
  public static final int REMOTE_HEIGHT = 50;
  public static final int UNCLE_HEIGHT = 25 - 3;

-  public static final int NUM_OF_RETRIES = 100;
+  public static final int NUM_OF_RETRIES = 1;


only one retry ? in case of bad peers it will not be a problem? not sure how backwardsync is working so just asking

This is test code, prod code has 2 retries (which is actually just one retry: two "tries")

…s from source with Jenkins Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

garyschulte

LGTM, non-blocking feedback

garyschulte · 2024-03-22T12:55:33Z

gradle.properties

@@ -14,3 +14,4 @@ org.gradle.jvmargs=-Xmx4g \
 --add-opens jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED
 # Could be moved to sonar properties after https://sonarsource.atlassian.net/browse/SONARGRADL-134
 systemProp.sonar.gradle.skipCompile=true
+version=24.3.1-SNAPSHOT


this doesn't look removed... ?

garyschulte · 2024-03-22T12:56:13Z

...c/test/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncContextTest.java

@@ -438,4 +441,53 @@ public void shouldFailAfterMaxNumberOfRetries() {
      }
    }
  }
+
+  @SuppressWarnings("BannedMethod")


what is the banned method? a comment might help here and on the other suppresswarnings statement

Had to add the version back in so I can use the Jankins build from source. Will fix it :-)

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

* minimal change to fix BWS Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net> Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Co-authored-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: Justin Florentine <justin+github@florentine.us>

* minimal change to fix BWS Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net> Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Co-authored-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: amsmota <antonio.mota@citi.com>

* minimal change to fix BWS Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net> Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Co-authored-by: Simon Dudley <simon.dudley@consensys.net>

pinges added 2 commits March 19, 2024 18:05

minimal change to fix BWS

69fc176

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

Merge branch 'main' of github.com:hyperledger/besu into BWS

8d512f9

pinges requested review from garyschulte and matkt March 19, 2024 08:14

macfarla reviewed Mar 19, 2024

View reviewed changes

set version

eeefadc

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

garyschulte reviewed Mar 19, 2024

View reviewed changes

garyschulte requested changes Mar 19, 2024

View reviewed changes

siladu added 3 commits March 20, 2024 16:36

Improve unit test and fix edge case for single entry queue

614a3a4

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

Remove cruft

058221b

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

Add extra test case and tweak code accordingly

05f9581

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

siladu mentioned this pull request Mar 21, 2024

Refactor BackwardsSyncAlgorithm.pickNextStep for readability #6775

Merged

matkt reviewed Mar 21, 2024

View reviewed changes

pinges added 3 commits March 22, 2024 15:56

remove version from gradle properties. Was only needed to create node…

670e628

…s from source with Jenkins Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

Merge branch 'main' of github.com:hyperledger/besu into BWS

5824392

need this to be able to create a VM from source

29a0b1e

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

pinges marked this pull request as ready for review March 22, 2024 12:30

pinges requested a review from garyschulte March 22, 2024 12:30

garyschulte approved these changes Mar 22, 2024

View reviewed changes

remove the version again

e13ca9e

Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>

pinges merged commit 42f4748 into hyperledger:main Mar 22, 2024
42 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix "Backward sync stuck in a loop" #6749 #6756

Fix "Backward sync stuck in a loop" #6749 #6756

pinges commented Mar 19, 2024

macfarla Mar 19, 2024

siladu Mar 19, 2024

siladu Mar 20, 2024

garyschulte Mar 19, 2024

pinges Mar 20, 2024

pinges Mar 22, 2024

garyschulte Mar 22, 2024

garyschulte left a comment

garyschulte Mar 19, 2024

pinges Mar 20, 2024

pinges Mar 22, 2024

matkt Mar 21, 2024

siladu Mar 21, 2024

garyschulte left a comment

garyschulte Mar 22, 2024

garyschulte Mar 22, 2024

pinges Mar 22, 2024

		// context.getEthContext().getEthPeers().peerCount() >
		// (context.getEthContext().getEthPeers().getMaxPeers() / 2)

Fix "Backward sync stuck in a loop" #6749 #6756

Fix "Backward sync stuck in a loop" #6749 #6756

Conversation

pinges commented Mar 19, 2024

PR description

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

garyschulte left a 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

garyschulte left a 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