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.

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

Track blocks we requested, always broadcast otherwise #2349

Merged
merged 1 commit into from
Jan 12, 2019

Conversation

ignopeverell
Copy link
Contributor

@ignopeverell ignopeverell commented Jan 12, 2019

This is an attempt at improving upon #2324. It fixes the following issue:

  • Whether we're syncing or not should not impact our ability to relay blocks. Otherwise we open ourselves to an attack where someone could fake a high difficulty to stop all our blocks (including mined ones). This is mitigated with a ban for cheats, but the ban can take a few min.
  • We do not want to relay blocks we requested on sync. This would completely flood the network unnecessarily.

This PR adds the tracking of block requests we originate in the TrackingAdapter (which was already tracking what we received). We can then check upon receiving a block if it was in the list we requested and propagate this information to decide whether to relay or not. This is a little longer than I'd like but most of the changes are just due to adding a boolean to block_received

@garyyu
Copy link
Contributor

garyyu commented Jan 12, 2019

Generally this is nice! 👍

Just a bit of question to avoid us forgot sth: I see the gossip block request is send_compact_block_request so received block is compact block; and the syncing block request is Type::GetBlock so received block is normal block.

Can we use this to differentiate in the block_accepted? and then maybe the implementation is much simple than this PR?

@antiochp
Copy link
Member

antiochp commented Jan 12, 2019

Just a bit of question to avoid us forgot sth: I see the gossip block request is send_compact_block_request so received block is compact block; and the syncing block request is Type::GetBlock so received block is normal block.
Can we use this to differentiate in the block_accepted? and then maybe the implementation is much simple than this PR?

I don't think we can reliably. We have no control over whether peers send us blocks vs. compact blocks and we should avoid making local decisions based on what other nodes send us.

Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

I think makes a lot of sense.
We broadcast things we receive via broadcast. And we suppress anything we explicitly ask for.
None of this is sync vs. non-sync or mining vs. non-mining related (which are orthogonal to this).

👍

@garyyu
Copy link
Contributor

garyyu commented Jan 12, 2019

I don't think we can reliably. We have no control over whether peers send us blocks vs. compact blocks and we should avoid making local decisions based on what other nodes send us.

Agree, since only the remote peers can decide what to send.

@ignopeverell ignopeverell merged commit f9a20ae into mimblewimble:master Jan 12, 2019
@antiochp
Copy link
Member

antiochp commented Oct 7, 2019

So it turns out this is actually subtly broken.
Related #3087.

There are two different scenarios in which we can "ask for a block" -

  1. During sync (we ask for a full block based on headers)
  2. Compact block cannot by "hydrated" (we ask for a full block based on compact block)

In case (1) we want to track blocks we requested and suppress both the hooks and the broadcast.
But in case (2) we do not want to suppress the broadcast.

Turns out nodes have not been broadcasting block headers to their peers if they first attempted to process a compact block then fallback to requesting and processing the full block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants