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.

Sign up for GitHub

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

remove ReportCollator message #6628

Merged
merged 6 commits into from
Nov 25, 2024
Merged

remove ReportCollator message #6628

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8c14fcd
remove ReportCollator message
jpserrat Nov 24, 2024
48d3df0
remove unused import
jpserrat Nov 25, 2024
04eb373
add prdoc for pr 6628
jpserrat Nov 25, 2024
91d9773
Merge branch 'master' of github.com:paritytech/polkadot-sdk into jpse…
jpserrat Nov 25, 2024
23539f7
Fix prdoc
tdimitrov Nov 25, 2024
9c34572
".git/.scripts/commands/fmt/fmt.sh"
Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion polkadot/node/network/collator-protocol/src/collator_side/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -899,7 +899,7 @@ async fn process_msg<Context>(
);
}
},
msg @ (ReportCollator(..) | Invalid(..) | Seconded(..)) => {
msg @ (Invalid(..) | Seconded(..)) => {
gum::warn!(
target: LOG_TARGET,
"{:?} message is not expected on the collator side of the protocol",
Expand Down
3 changes: 0 additions & 3 deletions polkadot/node/network/collator-protocol/src/validator_side/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1462,9 +1462,6 @@ async fn process_msg<Context>(
"DistributeCollation message is not expected on the validator side of the protocol",
);
},
ReportCollator(id) => {
report_collator(&mut state.reputation, ctx.sender(), &state.peer_data, id).await;
},
NetworkBridgeUpdate(event) => {
if let Err(e) = handle_network_msg(ctx, state, keystore, event).await {
gum::warn!(
Expand Down
60 changes: 0 additions & 60 deletions polkadot/node/network/collator-protocol/src/validator_side/tests/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,66 +638,6 @@ fn act_on_advertisement_v2() {
});
}

// Test that other subsystems may modify collators' reputations.
#[test]
fn collator_reporting_works() {
let test_state = TestState::default();

test_harness(ReputationAggregator::new(|_| true), |test_harness| async move {
let TestHarness { mut virtual_overseer, .. } = test_harness;

overseer_send(
&mut virtual_overseer,
CollatorProtocolMessage::NetworkBridgeUpdate(NetworkBridgeEvent::OurViewChange(
our_view![test_state.relay_parent],
)),
)
.await;

respond_to_runtime_api_queries(&mut virtual_overseer, &test_state, test_state.relay_parent)
.await;

let peer_b = PeerId::random();
let peer_c = PeerId::random();

connect_and_declare_collator(
&mut virtual_overseer,
peer_b,
test_state.collators[0].clone(),
test_state.chain_ids[0],
CollationVersion::V1,
)
.await;

connect_and_declare_collator(
&mut virtual_overseer,
peer_c,
test_state.collators[1].clone(),
test_state.chain_ids[0],
CollationVersion::V1,
)
.await;

overseer_send(
&mut virtual_overseer,
CollatorProtocolMessage::ReportCollator(test_state.collators[0].public()),
)
.await;

assert_matches!(
overseer_recv(&mut virtual_overseer).await,
AllMessages::NetworkBridgeTx(
NetworkBridgeTxMessage::ReportPeer(ReportPeerMessage::Single(peer, rep)),
) => {
assert_eq!(peer, peer_b);
assert_eq!(rep.value, COST_REPORT_BAD.cost_or_benefit());
}
);

virtual_overseer
});
}

// Test that we verify the signatures on `Declare` and `AdvertiseCollation` messages.
#[test]
fn collator_authentication_verification_works() {
Expand Down
5 changes: 1 addition & 4 deletions polkadot/node/subsystem-types/src/messages.rs
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please remove CollatorId if it is not used anymore? I see a CI job complaining about it.

I can't comment on the exact line - it's 51, the error is shown on the PR (unused import).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ use polkadot_primitives::{
CommittedCandidateReceiptV2 as CommittedCandidateReceipt, CoreState,
},
ApprovalVotingParams, AuthorityDiscoveryId, BlockNumber, CandidateCommitments, CandidateHash,
CandidateIndex, CollatorId, CoreIndex, DisputeState, ExecutorParams, GroupIndex,
CandidateIndex, CoreIndex, DisputeState, ExecutorParams, GroupIndex,
GroupRotationInfo, Hash, HeadData, Header as BlockHeader, Id as ParaId, InboundDownwardMessage,
InboundHrmpMessage, MultiDisputeStatementSet, NodeFeatures, OccupiedCoreAssumption,
PersistedValidationData, PvfCheckStatement, PvfExecKind as RuntimePvfExecKind, SessionIndex,
Expand Down Expand Up @@ -250,9 +250,6 @@ pub enum CollatorProtocolMessage {
/// The core index where the candidate should be backed.
core_index: CoreIndex,
},
/// Report a collator as having provided an invalid collation. This should lead to disconnect
/// and blacklist of the collator.
ReportCollator(CollatorId),
/// Get a network bridge update.
#[from]
NetworkBridgeUpdate(NetworkBridgeEvent<net_protocol::CollatorProtocolMessage>),
Expand Down
6 changes: 0 additions & 6 deletions polkadot/roadmap/implementers-guide/src/node/collators/collator-protocol.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,12 +151,6 @@ time per relay parent. This reduces the bandwidth requirements and as we can sec
the others are probably not required anyway. If the request times out, we need to note the collator as being unreliable
and reduce its priority relative to other collators.

As a validator, once the collation has been fetched some other subsystem will inspect and do deeper validation of the
collation. The subsystem will report to this subsystem with a [`CollatorProtocolMessage`][CPM]`::ReportCollator`. In
that case, if we are connected directly to the collator, we apply a cost to the `PeerId` associated with the collator
and potentially disconnect or blacklist it. If the collation is seconded, we notify the collator and apply a benefit to
the `PeerId` associated with the collator.

### Interaction with [Candidate Backing][CB]

As collators advertise the availability, a validator will simply second the first valid parablock candidate per relay
Expand Down
7 changes: 1 addition & 6 deletions polkadot/roadmap/implementers-guide/src/node/subsystems-and-jobs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,6 @@ digraph {

cand_sel -> coll_prot [arrowhead = "diamond", label = "FetchCollation"]
cand_sel -> cand_back [arrowhead = "onormal", label = "Second"]
cand_sel -> coll_prot [arrowhead = "onormal", label = "ReportCollator"]

cand_val -> runt_api [arrowhead = "diamond", label = "Request::PersistedValidationData"]
cand_val -> runt_api [arrowhead = "diamond", label = "Request::ValidationCode"]
Expand Down Expand Up @@ -231,7 +230,7 @@ sequenceDiagram

VS ->> CandidateSelection: Collation

Note over CandidateSelection: Lots of other machinery in play here,<br/>but there are only three outcomes from the<br/>perspective of the `CollatorProtocol`:
Note over CandidateSelection: Lots of other machinery in play here,<br/>but there are only two outcomes from the<br/>perspective of the `CollatorProtocol`:

alt happy path
CandidateSelection -->> VS: FetchCollation
Expand All @@ -242,10 +241,6 @@ sequenceDiagram
NB ->> VS: Collation
Deactivate VS

else collation invalid or unexpected
CandidateSelection ->> VS: ReportCollator
VS ->> NB: ReportPeer

else CandidateSelection already selected a different candidate
Note over CandidateSelection: silently drop
end
Expand Down
3 changes: 0 additions & 3 deletions polkadot/roadmap/implementers-guide/src/types/overseer-protocol.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,9 +436,6 @@ enum CollatorProtocolMessage {
DistributeCollation(CandidateReceipt, PoV, Option<oneshot::Sender<CollationSecondedSignal>>),
/// Fetch a collation under the given relay-parent for the given ParaId.
FetchCollation(Hash, ParaId, ResponseChannel<(CandidateReceipt, PoV)>),
/// Report a collator as having provided an invalid collation. This should lead to disconnect
/// and blacklist of the collator.
ReportCollator(CollatorId),
/// Note a collator as having provided a good collation.
NoteGoodCollation(CollatorId, SignedFullStatement),
/// Notify a collator that its collation was seconded.
Expand Down
12 changes: 12 additions & 0 deletions prdoc/pr_6628.prdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
title: "Remove ReportCollator message"

doc:
- audience: Node Dev
description: |
Remove unused message ReportCollator and test related to this message on the collator protocol validator side.

crates:
- name: polkadot-node-subsystem-types
bump: patch
- name: polkadot-collator-protocol
bump: major
Loading