feat: ban bad block-sync peers #5871
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghpbot-tari-project
added
P-acks_required
hansieodendaal
force-pushed
the
ho_block_sync
branch
from
2dee0e3
to
7121b61
Compare
SWvheerden
reviewed
Oct 26, 2023
SWvheerden
reviewed
Oct 27, 2023
| ) { | ||
| if blocks_with_anchor.is_empty() || blocks_with_anchor.len() < 2 { | ||
| panic!("blocks must have at least 2 elements"); | ||
| } | ||
| let set_best_block = set_best_block.unwrap_or(false); | ||
| let mut blocks: Vec<_> = blocks_with_anchor.to_vec(); | ||
| blocks.reverse(); | ||
| for i in 0..blocks.len() - 1 { |
There was a problem hiding this comment.
Suggested change
| for i in 0..blocks.len() - 1 { | |
| for i in (0..blocks.len() - 1).rev() { |
There was a problem hiding this comment.
nit: if just do this, you don't have to swap around the entire array, and the code above is more direct what's going on. Because it seems strange that you delete a block, and then you set the next block as the best block
| ) { | ||
| if blocks_with_anchor.is_empty() || blocks_with_anchor.len() < 2 { | ||
| panic!("blocks must have at least 2 elements"); | ||
| } | ||
| let set_best_block = set_best_block.unwrap_or(false); | ||
| let mut blocks: Vec<_> = blocks_with_anchor.to_vec(); | ||
| blocks.reverse(); |
There was a problem hiding this comment.
Suggested change
| blocks.reverse(); |
SWvheerden
reviewed
Oct 27, 2023
- Added a check to ban a misbahaving peer after block sync when not supplying any or all of the blocks corresponding to the accumulated difficulty they claimed they had. - Added a check in the RPC block-sync server method to not try and supply blocks past its own end of chain best block. - Added integration level unit tests for block sync.
hansieodendaal
force-pushed
the
ho_block_sync
branch
from
adb942e
to
40838b6
Compare
hansieodendaal
force-pushed
the
ho_block_sync
branch
from
40838b6
to
1218d43
Compare
SWvheerden
approved these changes
Oct 27, 2023
ghpbot-tari-project
removed
the
P-reviews_required
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Motivation and Context
The new unit tests that were added highlighted some issues where sync peers are not banned for their bad behaviour.
How Has This Been Tested?
Added new integration-level unit tests.
What process can a PR reviewer use to test or verify this change?
Breaking Changes