diff --git a/polkadot/node/network/approval-distribution/src/lib.rs b/polkadot/node/network/approval-distribution/src/lib.rs index f3c8e11b79..51b0feda6e 100644 --- a/polkadot/node/network/approval-distribution/src/lib.rs +++ b/polkadot/node/network/approval-distribution/src/lib.rs @@ -95,11 +95,35 @@ struct Knowledge { known_messages: HashSet, } +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, + known_by: HashMap, /// 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 } }; diff --git a/polkadot/node/network/approval-distribution/src/tests.rs b/polkadot/node/network/approval-distribution/src/tests.rs index 385834fa78..e420ca1ea8 100644 --- a/polkadot/node/network/approval-distribution/src/tests.rs +++ b/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(); diff --git a/polkadot/roadmap/implementers-guide/src/node/approval/approval-distribution.md b/polkadot/roadmap/implementers-guide/src/node/approval/approval-distribution.md index e382631afa..38fafd791d 100644 --- a/polkadot/roadmap/implementers-guide/src/node/approval/approval-distribution.md +++ b/polkadot/roadmap/implementers-guide/src/node/approval/approval-distribution.md @@ -76,10 +76,17 @@ struct Knowledge { known_messages: HashSet, } +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, + known_by: HashMap, // 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.