approval-distribution: split peer knowledge into sent and received (#2809)

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

* guide updates

* fixes

* revert doc changes

polkadot/node/network/approval-distribution/src/lib.rs

@@ -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.
@@ -212,7 +236,6 @@ impl State {
 							"Cleaning up stale pending messages",
 						);
 					}
-
 					live
 				});
 			}
@@ -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;
 					}
 				}
@@ -546,7 +573,7 @@ impl State {
 						?fingerprint,
 						"Known assignment",
 					);
-					peer_knowledge.known_messages.insert(fingerprint.clone());
+					peer_knowledge.received.insert(fingerprint.clone());
 				}
 				return;
 			}
@@ -584,7 +611,7 @@ 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 => {
@@ -592,7 +619,7 @@ impl State {
 					// 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;
 				}
@@ -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());
 			}
 		}
 
@@ -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",
-						);
+				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;
+							modify_reputation(ctx, peer_id, COST_DUPLICATE_MESSAGE).await;
+						}
+						peer_knowledge.received.insert(fingerprint);
 						return;
 					}
 				}
@@ -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,
@@ -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;
 			}
@@ -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 => {
@@ -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,
@@ -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());
 			}
 		}
 
@@ -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
 					}
 				};

polkadot/node/network/approval-distribution/src/tests.rs

@@ -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();
@@ -655,6 +736,7 @@ fn update_peer_view() {
 			.known_by
 			.get(peer)
 			.unwrap()
+			.sent
 			.known_messages
 			.len(),
 		1,
@@ -701,6 +783,7 @@ fn update_peer_view() {
 			.known_by
 			.get(peer)
 			.unwrap()
+			.sent
 			.known_messages
 			.len(),
 		1,
@@ -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();

polkadot/roadmap/implementers-guide/src/node/approval/approval-distribution.md

@@ -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.
@@ -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.
@@ -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.