Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Commit

Permalink
approval-distribution: split peer knowledge into sent and received (#…
Browse files Browse the repository at this point in the history 
…2809)

* approval-distribution: split peer knowledge into sent and received

* guide updates

* fixes

* revert doc changes
  • Loading branch information
@ordian
ordian authored Apr 3, 2021
1 parent 92a744d commit 8c7a6d9
Show file tree
Hide file tree
Showing 3 changed files with 162 additions and 36 deletions.
101 changes: 68 additions & 33 deletions node/network/approval-distribution/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,35 @@ struct Knowledge {
known_messages: HashSet<MessageFingerprint>,
}

impl Knowledge {
fn contains(&self, fingerprint: &MessageFingerprint) -> bool {
self.known_messages.contains(fingerprint)
}

fn insert(&mut self, fingerprint: MessageFingerprint) -> bool {
self.known_messages.insert(fingerprint)
}
}

#[derive(Debug, Clone, Default)]
struct PeerKnowledge {
/// The knowledge we've sent to the peer.
sent: Knowledge,
/// The knowledge we've received from the peer.
received: Knowledge,
}

impl PeerKnowledge {
fn contains(&self, fingerprint: &MessageFingerprint) -> bool {
self.sent.contains(fingerprint) || self.received.contains(fingerprint)
}
}

/// Information about blocks in our current view as well as whether peers know of them.
struct BlockEntry {
/// Peers who we know are aware of this block and thus, the candidates within it.
/// This maps to their knowledge of messages.
known_by: HashMap<PeerId, Knowledge>,
known_by: HashMap<PeerId, PeerKnowledge>,
/// The number of the block.
number: BlockNumber,
/// The parent hash of the block.
Expand Down Expand Up @@ -212,7 +236,6 @@ impl State {
"Cleaning up stale pending messages",
);
}

live
});
}
Expand Down Expand Up @@ -513,15 +536,19 @@ impl State {
if let Some(peer_id) = source.peer_id() {
// check if our knowledge of the peer already contains this assignment
match entry.known_by.entry(peer_id.clone()) {
hash_map::Entry::Occupied(knowledge) => {
if knowledge.get().known_messages.contains(&fingerprint) {
tracing::debug!(
target: LOG_TARGET,
?peer_id,
?fingerprint,
"Duplicate assignment",
);
modify_reputation(ctx, peer_id, COST_DUPLICATE_MESSAGE).await;
hash_map::Entry::Occupied(mut peer_knowledge) => {
let peer_knowledge = peer_knowledge.get_mut();
if peer_knowledge.contains(&fingerprint) {
if peer_knowledge.received.contains(&fingerprint) {
tracing::debug!(
target: LOG_TARGET,
?peer_id,
?fingerprint,
"Duplicate assignment",
);
modify_reputation(ctx, peer_id, COST_DUPLICATE_MESSAGE).await;
}
peer_knowledge.received.insert(fingerprint);
return;
}
}
Expand All @@ -546,7 +573,7 @@ impl State {
?fingerprint,
"Known assignment",
);
peer_knowledge.known_messages.insert(fingerprint.clone());
peer_knowledge.received.insert(fingerprint.clone());
}
return;
}
Expand Down Expand Up @@ -584,15 +611,15 @@ impl State {
modify_reputation(ctx, peer_id.clone(), BENEFIT_VALID_MESSAGE_FIRST).await;
entry.knowledge.known_messages.insert(fingerprint.clone());
if let Some(peer_knowledge) = entry.known_by.get_mut(&peer_id) {
peer_knowledge.known_messages.insert(fingerprint.clone());
peer_knowledge.received.insert(fingerprint.clone());
}
}
AssignmentCheckResult::AcceptedDuplicate => {
// "duplicate" assignments aren't necessarily equal.
// There is more than one way each validator can be assigned to each core.
// cf. https://github.com/paritytech/polkadot/pull/2160#discussion_r557628699
if let Some(peer_knowledge) = entry.known_by.get_mut(&peer_id) {
peer_knowledge.known_messages.insert(fingerprint);
peer_knowledge.received.insert(fingerprint);
}
return;
}
Expand Down Expand Up @@ -663,8 +690,8 @@ impl State {
// Add the fingerprint of the assignment to the knowledge of each peer.
for peer in peers.iter() {
// we already filtered peers above, so this should always be Some
if let Some(entry) = entry.known_by.get_mut(peer) {
entry.known_messages.insert(fingerprint.clone());
if let Some(peer_knowledge) = entry.known_by.get_mut(peer) {
peer_knowledge.sent.insert(fingerprint.clone());
}
}

Expand Down Expand Up @@ -735,16 +762,20 @@ impl State {

// check if our knowledge of the peer already contains this approval
match entry.known_by.entry(peer_id.clone()) {
hash_map::Entry::Occupied(knowledge) => {
if knowledge.get().known_messages.contains(&fingerprint) {
tracing::debug!(
target: LOG_TARGET,
?peer_id,
?fingerprint,
"Duplicate approval",
);

modify_reputation(ctx, peer_id, COST_DUPLICATE_MESSAGE).await;
hash_map::Entry::Occupied(mut knowledge) => {
let peer_knowledge = knowledge.get_mut();
if peer_knowledge.contains(&fingerprint) {
if peer_knowledge.received.contains(&fingerprint) {
tracing::debug!(
target: LOG_TARGET,
?peer_id,
?fingerprint,
"Duplicate approval",
);

modify_reputation(ctx, peer_id, COST_DUPLICATE_MESSAGE).await;
}
peer_knowledge.received.insert(fingerprint);
return;
}
}
Expand All @@ -760,7 +791,7 @@ impl State {
}

// if the approval is known to be valid, reward the peer
if entry.knowledge.known_messages.contains(&fingerprint) {
if entry.knowledge.contains(&fingerprint) {
tracing::trace!(
target: LOG_TARGET,
?peer_id,
Expand All @@ -769,7 +800,7 @@ impl State {
);
modify_reputation(ctx, peer_id.clone(), BENEFIT_VALID_MESSAGE).await;
if let Some(peer_knowledge) = entry.known_by.get_mut(&peer_id) {
peer_knowledge.known_messages.insert(fingerprint.clone());
peer_knowledge.received.insert(fingerprint.clone());
}
return;
}
Expand Down Expand Up @@ -805,9 +836,9 @@ impl State {
ApprovalCheckResult::Accepted => {
modify_reputation(ctx, peer_id.clone(), BENEFIT_VALID_MESSAGE_FIRST).await;

entry.knowledge.known_messages.insert(fingerprint.clone());
entry.knowledge.insert(fingerprint.clone());
if let Some(peer_knowledge) = entry.known_by.get_mut(&peer_id) {
peer_knowledge.known_messages.insert(fingerprint.clone());
peer_knowledge.received.insert(fingerprint.clone());
}
}
ApprovalCheckResult::Bad => {
Expand All @@ -821,7 +852,7 @@ impl State {
}
}
} else {
if !entry.knowledge.known_messages.insert(fingerprint.clone()) {
if !entry.knowledge.insert(fingerprint.clone()) {
// if we already imported an approval, there is no need to distribute it again
tracing::warn!(
target: LOG_TARGET,
Expand Down Expand Up @@ -898,7 +929,7 @@ impl State {
for peer in peers.iter() {
// we already filtered peers above, so this should always be Some
if let Some(entry) = entry.known_by.get_mut(peer) {
entry.known_messages.insert(fingerprint.clone());
entry.sent.insert(fingerprint.clone());
}
}

Expand Down Expand Up @@ -947,7 +978,11 @@ impl State {
hash_map::Entry::Occupied(_) => return None,
// step 4.
hash_map::Entry::Vacant(vacant) => {
vacant.insert(entry.knowledge.clone());
let knowledge = PeerKnowledge {
sent: entry.knowledge.clone(),
received: Default::default(),
};
vacant.insert(knowledge);
block
}
};
Expand Down
84 changes: 84 additions & 0 deletions node/network/approval-distribution/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,87 @@ fn spam_attack_results_in_negative_reputation_change() {
});
}


/// Imagine we send a message to peer A and peer B.
/// Upon receiving them, they both will try to send the message each other.
/// This test makes sure they will not punish each other for such duplicate messages.
///
/// See https://github.com/paritytech/polkadot/issues/2499.
#[test]
fn peer_sending_us_the_same_we_just_sent_them_is_ok() {
let parent_hash = Hash::repeat_byte(0xFF);
let peer_a = PeerId::random();
let hash = Hash::repeat_byte(0xAA);

let _ = test_harness(State::default(), |mut virtual_overseer| async move {
let overseer = &mut virtual_overseer;
let peer = &peer_a;
setup_peer_with_view(overseer, peer, view![]).await;

// new block `hash` with 1 candidates
let meta = BlockApprovalMeta {
hash,
parent_hash,
number: 1,
candidates: vec![Default::default(); 1],
slot: 1.into(),
};
let msg = ApprovalDistributionMessage::NewBlocks(vec![meta]);
overseer_send(overseer, msg).await;

// import an assignment related to `hash` locally
let validator_index = ValidatorIndex(0);
let candidate_index = 0u32;
let cert = fake_assignment_cert(hash, validator_index);
overseer_send(
overseer,
ApprovalDistributionMessage::DistributeAssignment(cert.clone(), candidate_index)
).await;

// update peer view to include the hash
overseer_send(
overseer,
ApprovalDistributionMessage::NetworkBridgeUpdateV1(
NetworkBridgeEvent::PeerViewChange(peer.clone(), view![hash])
)
).await;

// we should send them the assignment
assert_matches!(
overseer_recv(overseer).await,
AllMessages::NetworkBridge(NetworkBridgeMessage::SendValidationMessage(
peers,
protocol_v1::ValidationProtocol::ApprovalDistribution(
protocol_v1::ApprovalDistributionMessage::Assignments(assignments)
)
)) => {
assert_eq!(peers.len(), 1);
assert_eq!(assignments.len(), 1);
}
);

// but if someone else is sending it the same assignment
// the peer could send us it as well
let assignments = vec![(cert, candidate_index)];
let msg = protocol_v1::ApprovalDistributionMessage::Assignments(assignments);
send_message_from_peer(overseer, peer, msg.clone()).await;

assert!(overseer
.recv()
.timeout(TIMEOUT)
.await
.is_none(),
"we should not punish the peer",
);

// send the assignments again
send_message_from_peer(overseer, peer, msg).await;

// now we should
expect_reputation_change(overseer, peer, COST_DUPLICATE_MESSAGE).await;
});
}

#[test]
fn import_approval_happy_path() {
let peer_a = PeerId::random();
Expand Down Expand Up @@ -655,6 +736,7 @@ fn update_peer_view() {
.known_by
.get(peer)
.unwrap()
.sent
.known_messages
.len(),
1,
Expand Down Expand Up @@ -701,6 +783,7 @@ fn update_peer_view() {
.known_by
.get(peer)
.unwrap()
.sent
.known_messages
.len(),
1,
Expand Down Expand Up @@ -729,6 +812,7 @@ fn update_peer_view() {
);
}

/// E.g. if someone copies the keys...
#[test]
fn import_remotely_then_locally() {
let peer_a = PeerId::random();
Expand Down
13 changes: 10 additions & 3 deletions roadmap/implementers-guide/src/node/approval/approval-distribution.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,17 @@ struct Knowledge {
known_messages: HashSet<MessageFingerprint>,
}

struct PeerKnowledge {
/// The knowledge we've sent to the peer.
sent: Knowledge,
/// The knowledge we've received from the peer.
received: Knowledge,
}

/// Information about blocks in our current view as well as whether peers know of them.
struct BlockEntry {
// Peers who we know are aware of this block and thus, the candidates within it. This maps to their knowledge of messages.
known_by: HashMap<PeerId, Knowledge>,
known_by: HashMap<PeerId, PeerKnowledge>,
// The number of the block.
number: BlockNumber,
// The parent hash of the block.
Expand Down Expand Up @@ -188,7 +195,7 @@ The algorithm is the following:
* Load the BlockEntry using `assignment.block_hash`. If it does not exist, report the source if it is `MessageSource::Peer` and return.
* Compute a fingerprint for the `assignment` using `claimed_candidate_index`.
* If the source is `MessageSource::Peer(sender)`:
* check if `peer` appears under `known_by` and whether the fingerprint is in the `known_messages` of the peer. If the peer does not know the block, report for providing data out-of-view and proceed. If the peer does know the block and the knowledge contains the fingerprint, report for providing replicate data and return.
* check if `peer` appears under `known_by` and whether the fingerprint is in the knowledge of the peer. If the peer does not know the block, report for providing data out-of-view and proceed. If the peer does know the block and the `sent` knowledge contains the fingerprint, report for providing replicate data and return, otherwise, insert into the `received` knowledge and return.
* If the message fingerprint appears under the `BlockEntry`'s `Knowledge`, give the peer a small positive reputation boost,
add the fingerprint to the peer's knowledge only if it knows about the block and return.
Note that we must do this after checking for out-of-view and if the peers knows about the block to avoid being spammed.
Expand All @@ -215,7 +222,7 @@ Imports an approval signature referenced by block hash and candidate index:
* Compute a fingerprint for the approval.
* Compute a fingerprint for the corresponding assignment. If the `BlockEntry`'s knowledge does not contain that fingerprint, then report the source if it is `MessageSource::Peer` and return. All references to a fingerprint after this refer to the approval's, not the assignment's.
* If the source is `MessageSource::Peer(sender)`:
* check if `peer` appears under `known_by` and whether the fingerprint is in the `known_messages` of the peer. If the peer does not know the block, report for providing data out-of-view and proceed. If the peer does know the block and the knowledge contains the fingerprint, report for providing replicate data and return.
* check if `peer` appears under `known_by` and whether the fingerprint is in the knowledge of the peer. If the peer does not know the block, report for providing data out-of-view and proceed. If the peer does know the block and the `sent` knowledge contains the fingerprint, report for providing replicate data and return, otherwise, insert into the `received` knowledge and return.
* If the message fingerprint appears under the `BlockEntry`'s `Knowledge`, give the peer a small positive reputation boost,
add the fingerprint to the peer's knowledge only if it knows about the block and return.
Note that we must do this after checking for out-of-view to avoid being spammed. If we did this check earlier, a peer could provide data out-of-view repeatedly and be rewarded for it.
Expand Down

0 comments on commit 8c7a6d9

Please sign in to comment.