Do not send messages twice in bitfield distribution #2005
Conversation
This removes a bug which resulted in sending bitfield messages multiple times by not checking if we already relayed them. Besides that it also adds an optimization to not relay a message to a peer that send us this message.
bkchr
added
A0-please_review
| @@ -443,6 +447,9 @@ where | |||
| } | |||
| one_per_validator.insert(validator.clone(), message.clone()); | |||
|
|
|||
| // If the peer has sent us a message, we don't need to send him the same. | |||
| job_data.message_sent_to_peer.entry(origin.clone()).or_default().insert(validator.clone()); | |||
There was a problem hiding this comment.
Isn't this already done above here? https://github.com/paritytech/polkadot/pull/2005/files#diff-6e99314d667011a73a36844509b05e6bfbf87e4cdcfa04d43d758d9ecf65fefbR424
There was a problem hiding this comment.
No, this is the set of received messages.
There was a problem hiding this comment.
Oh, good point. But why should we update the message_sent_to_peer if we've only received the message? It seems like we should only update the message_received_from_peer
There was a problem hiding this comment.
We should also check message_received_from_peer in the interested_peers computation in relay_peers, I guess.
There was a problem hiding this comment.
Oh, good point. But why should we update the message_sent_to_peer if we've only received the message? It seems like we should only update the message_received_from_peer
If I have received the message from you, what is the benefit of sending you the same message again? This would happen below in `relay_message'. While you will not decrease my reputation for this, as I did not yet send you this message, I just waste my bandwidth for a message that is already known by you.
There was a problem hiding this comment.
Who says they're the same message? We only know that they're from the same validator. The bitfield distribution section of the guide goes deeper into the rationale.
That is a good point, but we actually don't account for this in the code? As far as I have seen, we only check if we have send/received a message based on the validator is message is coming from.
You are saying that we actually should look into the message and check if they are different, is that correct?
There was a problem hiding this comment.
You are saying that we actually should look into the message and check if they are different, is that correct?
No, the change I suggest is just to check if we've received a message from the peer before sending a message. I think the thing this is trying to work around is where both peers send each other the message at the same time and then report each other.
There was a problem hiding this comment.
No, I don't try to work around this and this did not happen. We also just report a peer if it sends us the same message twice. We don't check if we send the message to this peer already for reporting the peer. Because as you said, this is very likely to end in a race condition.
However, I will do your requested changes.
There was a problem hiding this comment.
The other thing that I spoke about is that the code currently assumes that each validator only sends one bitfield per relay chain. Will this always be true or will we may change this in the future?
There was a problem hiding this comment.
I think this will be true for the foreseeable future
| // create a signed message by validator 0 | ||
| let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); | ||
| let signed_bitfield = | ||
| executor::block_on(Signed::<AvailabilityBitfield>::sign(&keystore, payload, &signing_context, 0, &validator)) |
There was a problem hiding this comment.
Would you mind wrapping this and other lines introduced by this PR that grew unwieldly?
This removes a bug which resulted in sending bitfield messages multiple
times by not checking if we already relayed them. Besides that it also
adds an optimization to not relay a message to a peer that send us
this message.