Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[ETCM-415] Fix random subset of peers where block is broadcasted #814

Merged
merged 1 commit into from
Nov 26, 2020

Conversation

ntallar
Copy link

@ntallar ntallar commented Nov 25, 2020

Description

Fixes random subset of peers not working properly

Given a set, Random.shuffle doesn't work properly (kudos to @dmitry-worker for finding this!):

  • Random.shuffle((1 to 10).toSet).take(5) == (1 to 10).toSet.take(5), the shuffle does nothing (this seems to happen with more than 4 elements as the HashSet implementation is used)
  • Random.shuffle((1 to 10).toSet) == Random.shuffle((1 to 5).toSet), adding more elements doesn't alter the result of the shuffle

This might have lead into bad propagation of blocks, increasing the chances of having forks on our testnet

Proposed Solution

  • Converted Set to Seq before shuffling
  • Removed broadcast-new-block-hashes config as it's heavily used:
    1. Our current sync implementation relies on this message to have an up-to-date known top, and also triggering the fetching of new blocks
    2. Without sending this message (as when this flag was off) we are not following the Ethereum protocol

…oved unnecesary broadcast-new-block-hashes config
@ntallar ntallar added the bug Something isn't working label Nov 25, 2020
Copy link
Contributor

@mirkoAlic mirkoAlic left a comment

Choose a reason for hiding this comment

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

🚀

@mirkoAlic mirkoAlic merged commit 1d7076a into develop Nov 26, 2020
@mirkoAlic mirkoAlic deleted the etcm-415/fix-random-subset-block-broadcast branch November 26, 2020 12:50
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants