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

Jump to bottom

Refactor and fix retrying get block switching peer #4256

Merged
merged 11 commits into from
Aug 18, 2022
Merged

Refactor and fix retrying get block switching peer #4256

merged 11 commits into from
Aug 18, 2022

Conversation

fab-10
Copy link
Contributor

@fab-10 fab-10 commented Aug 12, 2022 •
edited
Loading

PR description

RetryingGetBlockFromPeersTask had a problem that prevented to complete
when all the peers fail, and that also had the
consequence to not removing the failed requested block from the internal
caches in BlockPropagationManager, that could cause a stall since that
block will not be tried to be retrieved again.

Made also an abstraction of a retrying peer task that tries a different peer on every execution, so it can used also for other tasks

Fixed Issue(s)

relates to #3955

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

@fab-10 fab-10 changed the title Fix retrying get block Refactor and fix retrying get block switching peer Aug 12, 2022
@fab-10 fab-10 added the mainnet label Aug 12, 2022
macfarla
macfarla reviewed Aug 14, 2022
View reviewed changes
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

a few comments. I like the task that tries different peers, I can see that being useful!

...n/java/org/hyperledger/besu/ethereum/eth/manager/task/AbstractRetryingSwitchingPeerTask.java Outdated Show resolved Hide resolved
...n/java/org/hyperledger/besu/ethereum/eth/manager/task/AbstractRetryingSwitchingPeerTask.java Show resolved Hide resolved
...hyperledger/besu/ethereum/eth/manager/ethtaskutils/RetryingSwitchingPeerMessageTaskTest.java Outdated Show resolved Hide resolved
...hyperledger/besu/ethereum/eth/manager/ethtaskutils/RetryingSwitchingPeerMessageTaskTest.java Outdated Show resolved Hide resolved
...hyperledger/besu/ethereum/eth/manager/ethtaskutils/RetryingSwitchingPeerMessageTaskTest.java Show resolved Hide resolved
...t/java/org/hyperledger/besu/ethereum/eth/manager/task/RetryingGetBlockFromPeersTaskTest.java Show resolved Hide resolved
@fab-10
Refactor retrying peer task switching peers at every try
63ce3e3 
RetryingGetBlockFromPeersTask had a problem that prevented to complete
when all the peers were tried without success, and that also had the
consequence to not removing the failed requested block for the internal
caches in BlockPropagationManager, that could cause a stall since that
block will to be tried to be retrieved again.

Signed-off-by: Fabio Di Fabio <[email protected]>
@fab-10
Update CHANGELOG
2d10c0f 
Signed-off-by: Fabio Di Fabio <[email protected]>
@fab-10
Fix logs and comments
bb76662 
Signed-off-by: Fabio Di Fabio <[email protected]>
@fab-10
When retrying switching peer, disconnect one less useful peer upon re…
fafacec 
…cycling peers

Signed-off-by: Fabio Di Fabio <[email protected]>

# Conflicts:
#	ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/task/AbstractRetryingSwitchingPeerTask.java
#	ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java
@fab-10 fab-10 mentioned this pull request Aug 17, 2022
Merged
2 tasks
garyschulte
garyschulte approved these changes Aug 18, 2022
View reviewed changes
Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

👍

@garyschulte garyschulte enabled auto-merge (squash) August 18, 2022 19:12
@garyschulte garyschulte merged commit 3a2aeab into hyperledger:main Aug 18, 2022
jflo pushed a commit to jflo/besu that referenced this pull request Aug 18, 2022
@fab-10 @jflo
Refactor and fix retrying get block switching peer (hyperledger#4256)
10e22b5 
* Refactor retrying peer task switching peers at every try

RetryingGetBlockFromPeersTask had a problem that prevented to complete
when all the peers were tried without success, and that also had the
consequence to not removing the failed requested block for the internal
caches in BlockPropagationManager, that could cause a stall since that
block will to be tried to be retrieved again.

Signed-off-by: Fabio Di Fabio <[email protected]>
freemanzMrojo pushed a commit to freemanzMrojo/besu that referenced this pull request Aug 19, 2022
@fab-10 @freemanzMrojo
Refactor and fix retrying get block switching peer (hyperledger#4256)
4a804fc 
* Refactor retrying peer task switching peers at every try

RetryingGetBlockFromPeersTask had a problem that prevented to complete
when all the peers were tried without success, and that also had the
consequence to not removing the failed requested block for the internal
caches in BlockPropagationManager, that could cause a stall since that
block will to be tried to be retrieved again.

Signed-off-by: Fabio Di Fabio <[email protected]>
freemanzMrojo added a commit to freemanzMrojo/besu that referenced this pull request Aug 19, 2022
@freemanzMrojo
Squashed commit of the following:
86f4659 
commit ce3b476
Author: Miguel Rojo <[email protected]>
Date:   Fri Aug 19 16:37:54 2022 +0100

    changelog

    Signed-off-by: Miguel Rojo <[email protected]>

commit 606c53a
Author: Miguel Rojo <[email protected]>
Date:   Fri Aug 19 16:12:41 2022 +0100

    small refactor

    Signed-off-by: Miguel Rojo <[email protected]>

commit 1441f72
Merge: b78403b 3a2aeab
Author: Miguel Angel Rojo <[email protected]>
Date:   Fri Aug 19 15:32:51 2022 +0100

    Merge branch 'hyperledger:main' into flexible-privacy-ec-encryptor

commit b78403b
Author: Miguel Rojo <[email protected]>
Date:   Fri Aug 19 15:23:17 2022 +0100

    flexible multitenancy working as well

    Signed-off-by: Miguel Rojo <[email protected]>

commit b68efc2
Author: Miguel Rojo <[email protected]>
Date:   Fri Aug 19 13:31:28 2022 +0100

    removed unused constructor

    Signed-off-by: Miguel Rojo <[email protected]>

commit c6f7c48
Author: Miguel Rojo <[email protected]>
Date:   Fri Aug 19 13:21:23 2022 +0100

    fixed removeparticipant smart contract method

    Signed-off-by: Miguel Rojo <[email protected]>

commit 09e51ef
Author: Miguel Rojo <[email protected]>
Date:   Fri Aug 19 11:22:17 2022 +0100

    added encryptor type to flexible method in the factory

    Signed-off-by: Miguel Rojo <[email protected]>

commit 4a804fc
Author: Fabio Di Fabio <[email protected]>
Date:   Thu Aug 18 21:45:07 2022 +0200

    Refactor and fix retrying get block switching peer (hyperledger#4256)

    * Refactor retrying peer task switching peers at every try

    RetryingGetBlockFromPeersTask had a problem that prevented to complete
    when all the peers were tried without success, and that also had the
    consequence to not removing the failed requested block for the internal
    caches in BlockPropagationManager, that could cause a stall since that
    block will to be tried to be retrieved again.

    Signed-off-by: Fabio Di Fabio <[email protected]>

commit 8d1de9e
Author: garyschulte <[email protected]>
Date:   Thu Aug 18 12:17:56 2022 -0700

    implement tentative mainnet TTD (hyperledger#4260)

    * implement tentative mainnet TTD
    * fix unit test breakage
    * add to change log

    Signed-off-by: garyschulte <[email protected]>

commit 95f95a1
Author: Justin Florentine <[email protected]>
Date:   Thu Aug 18 14:51:37 2022 -0400

    Panda prove ments (hyperledger#4267)

    * breaks pandas up, test coverage

    Signed-off-by: Justin Florentine <[email protected]>

commit 5bb9a30
Author: garyschulte <[email protected]>
Date:   Thu Aug 18 10:52:50 2022 -0700

    Bugfix/clique post merge fast sync (hyperledger#4276)

    * if merge enabled, wrap two clique rules in composed Attached rule to enable fast-sync to proceed normally for post-merge networks
    * move BlockPropagationManager warning to debug until hyperledger#4274

    Signed-off-by: garyschulte <[email protected]>

commit 95d0825
Author: Miguel Rojo <[email protected]>
Date:   Fri Aug 19 10:43:15 2022 +0100

    test fixed

    Signed-off-by: Miguel Rojo <[email protected]>

commit 6c2f17a
Author: Miguel Rojo <[email protected]>
Date:   Thu Aug 18 19:07:39 2022 +0100

    reverted some logs

    Signed-off-by: Miguel Rojo <[email protected]>

commit 57faef6
Author: Miguel Rojo <[email protected]>
Date:   Thu Aug 18 19:07:00 2022 +0100

    reverted some logs

    Signed-off-by: Miguel Rojo <[email protected]>

commit ecde451
Author: Miguel Rojo <[email protected]>
Date:   Thu Aug 18 18:49:03 2022 +0100

    reverted some logs

    Signed-off-by: Miguel Rojo <[email protected]>

commit d6625e7
Author: Miguel Rojo <[email protected]>
Date:   Thu Aug 18 18:43:43 2022 +0100

    reverted some logs

    Signed-off-by: Miguel Rojo <[email protected]>

commit d554b7a
Author: Miguel Rojo <[email protected]>
Date:   Thu Aug 18 18:42:09 2022 +0100

    reverted some logs

    Signed-off-by: Miguel Rojo <[email protected]>

commit e4663e7
Author: Miguel Rojo <[email protected]>
Date:   Thu Aug 18 18:38:20 2022 +0100

    fixed issue with decodelist and no elements

    Signed-off-by: Miguel Rojo <[email protected]>

commit 2188c66
Author: Miguel Rojo <[email protected]>
Date:   Thu Aug 18 18:36:59 2022 +0100

    fixed issue with decodelist and no elements

    Signed-off-by: Miguel Rojo <[email protected]>

commit 84881d7
Author: Danno Ferrin <[email protected]>
Date:   Wed Aug 17 22:40:26 2022 -0600

    Gradle repository maintenance (hyperledger#4273)

    Cleanup repository references and limit scope of non-mavenCentral
    repositories.

    Signed-off-by: Danno Ferrin <[email protected]>

commit 93cc7b7
Author: Stefan Pingel <[email protected]>
Date:   Thu Aug 18 11:49:09 2022 +1000

    make obvious when a breach of protocol is logged, add peer in some places (hyperledger#4268)

    Signed-off-by: Stefan <[email protected]>
    Co-authored-by: Sally MacFarlane <[email protected]>

commit d0283f4
Author: Justin Florentine <[email protected]>
Date:   Wed Aug 17 15:48:53 2022 -0400

    Block prop on first final (hyperledger#4265)

    * start filtering peers after 1 finalized instead of 2
    * stops counting finalized, and starts filtering on first finalized
    * DefaultSynchronizer now listens to Forkhoice messages so it can stop block propagation at finalization, as opposed to TTD (previous behavior)

    Signed-off-by: Justin Florentine <[email protected]>

commit 2e2ea1f
Author: Miguel Rojo <[email protected]>
Date:   Thu Aug 18 17:24:22 2022 +0100

    modified flexibleutil so it works with dynamic byte arrays, some logs to remove

    Signed-off-by: Miguel Rojo <[email protected]>

commit 0464ccc
Author: Miguel Rojo <[email protected]>
Date:   Thu Aug 18 11:42:16 2022 +0100

    smart contracts adapted, changed bytecodes

    Signed-off-by: Miguel Rojo <[email protected]>

commit 0cf4586
Author: Miguel Rojo <[email protected]>
Date:   Thu Aug 18 11:02:08 2022 +0100

    smart contracts adapted, changed bytecodes

    Signed-off-by: Miguel Rojo <[email protected]>

commit 56a0ea3
Author: Miguel Rojo <[email protected]>
Date:   Thu Aug 18 10:36:45 2022 +0100

    added some logs, to be reverted

    Signed-off-by: Miguel Rojo <[email protected]>

commit 952085d
Author: Miguel Rojo <[email protected]>
Date:   Wed Aug 17 17:52:09 2022 +0100

    solidity contracts changed so they use bytes instead of bytes32

    Signed-off-by: Miguel Rojo <[email protected]>

commit 2d956f0
Author: Miguel Rojo <[email protected]>
Date:   Wed Aug 17 17:45:26 2022 +0100

    solidity contracts changed so they use bytes instead of bytes32

    Signed-off-by: Miguel Rojo <[email protected]>

commit d91db9c
Author: garyschulte <[email protected]>
Date:   Tue Aug 16 11:35:07 2022 -0700

    getProof encoding fix for 4249 (hyperledger#4261)

    * getProof encoding fix for 4249

    Signed-off-by: garyschulte <[email protected]>

commit e154a47
Author: Miguel Rojo <[email protected]>
Date:   Wed Aug 17 17:38:27 2022 +0100

    solidity contracts changed so they use bytes instead of bytes32

    Signed-off-by: Miguel Rojo <[email protected]>

commit 46e2e41
Author: Miguel Rojo <[email protected]>
Date:   Wed Aug 17 13:34:55 2022 +0100

    modified flexibleutil to support ec keys

    Signed-off-by: Miguel Rojo <[email protected]>

commit 4159147
Author: Fabio Di Fabio <[email protected]>
Date:   Tue Aug 16 11:17:00 2022 +0200

    Fix off-by-one error in AbstractRetryingPeerTask (hyperledger#4254)

    Signed-off-by: Fabio Di Fabio <[email protected]>

commit 2813ce7
Author: Miguel Rojo <[email protected]>
Date:   Tue Aug 16 10:37:22 2022 +0100

    first commit with the flexible privacy tests adapted

    Signed-off-by: Miguel Rojo <[email protected]>

Signed-off-by: Miguel Rojo <[email protected]>
@fab-10 fab-10 deleted the fix-retrying-get-block branch November 29, 2022 17:51
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
@fab-10 @eum602
Refactor and fix retrying get block switching peer (hyperledger#4256)
fc34cb3 
* Refactor retrying peer task switching peers at every try

RetryingGetBlockFromPeersTask had a problem that prevented to complete
when all the peers were tried without success, and that also had the
consequence to not removing the failed requested block for the internal
caches in BlockPropagationManager, that could cause a stall since that
block will to be tried to be retrieved again.

Signed-off-by: Fabio Di Fabio <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@macfarla macfarla macfarla left review comments

@garyschulte garyschulte garyschulte approved these changes

@gezero gezero gezero approved these changes

Assignees
No one assigned
Labels
mainnet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fab-10 @gezero @garyschulte @macfarla @jflo