[WIP] malus-collator: implement initial version of malicious collator submitting same collation to all backing groups #6924

sw10pa · 2024-12-17T12:17:01Z

Issues

Description

Modified the undying collator to include a malus mode, in which it submits the same collation to all assigned backing groups.

TODO

Implement the initial version of the malicious collator that submits the same collation to all backing groups;
Avoid the core index check in the collation generation subsystem: https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/node/collation-generation/src/lib.rs#L552-L553;
Provide incremental CoreSelector in upward messages for both malus and normal collators;
Resolve the mismatch between the descriptor and the commitments core index: [WIP] collation-generation: resolve mismatch between descriptor and commitments core index #7104
Add core assignment in same-collation-to-all-assigned-cores.toml using the assign-core.js script;
Update the same-collation-to-all-assigned-cores.zndsl Zombienet test;
Add PRdoc.

…or submitting same collation to all backing groups

paritytech-workflow-stopper · 2024-12-17T12:54:24Z

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/12372830027
Failed job name: test-linux-stable

…o Collator implementation

…hecks

…extend test scenario

alindima

Looking good overall!

alindima · 2025-01-13T09:04:03Z

polkadot/parachain/test-parachains/undying/collator/src/cli.rs

+	/// If true, the collator will behave maliciously by submitting
+	/// the same collations to all assigned backing groups.
+	#[arg(long, default_value_t = false)]
+	pub malus: bool,


we could have this malus flag map to an enum of variants, describing what malicious behaviour it's triggering

alindima · 2025-01-13T09:05:43Z

polkadot/parachain/test-parachains/undying/collator/src/main.rs

@@ -19,7 +19,7 @@
 use polkadot_cli::{Error, Result};
 use polkadot_node_primitives::CollationGenerationConfig;
 use polkadot_node_subsystem::messages::{CollationGenerationMessage, CollatorProtocolMessage};
-use polkadot_primitives::Id as ParaId;
+use polkadot_primitives::{self, Id as ParaId};


is the self used?

alindima · 2025-01-13T09:11:51Z

polkadot/parachain/test-parachains/undying/collator/src/lib.rs

+					let scheduled_cores: Vec<CoreIndex> = claim_queue
+						.iter()
+						.filter_map(move |(core_index, paras)| {
+							Some((*core_index, *paras.get(claim_queue_offset.0 as usize)?))
+						})


Have a look at ClaimQueueSnapshot to reuse some of the code there with iter_claims_at_depth

alindima · 2025-01-13T09:13:02Z

polkadot/parachain/test-parachains/undying/collator/src/lib.rs

+					let claim_queue = match client.runtime_api().claim_queue(relay_parent) {
+						Ok(claim_queue) =>
+							if claim_queue.is_empty() {
+								log::info!(target: LOG_TARGET, "Claim queue is empty.");


We can remove this case because it'll be caught by scheduled_cores.is_empty()

alindima · 2025-01-13T09:14:59Z

polkadot/parachain/test-parachains/undying/collator/src/lib.rs

+						})
+						.collect();
+
+					if scheduled_cores.is_empty() {


this log is super spammy until the para is scheduled. It's also not an error.
We should wait for a new slot before retrying (the 6 seconds). Same for all the other continues here.

I would also log something if scheduled_cores only has one core assigned, as in that case, this malus collator will not do anything malicious

alindima · 2025-01-13T09:18:20Z

polkadot/parachain/test-parachains/undying/collator/src/lib.rs

+
+						overseer_handle
+							.send_msg(
+								CollatorProtocolMessage::DistributeCollation {


add a comment about why we're not using SubmitCollation

alindima · 2025-01-13T09:20:28Z

polkadot/parachain/test-parachains/undying/collator/src/lib.rs

+							commitments: commitments.clone(),
+						};
+
+						let candidate_receipt = ccr.to_plain();


Avoid building a CommittedCandidateReceiptV2 if you only need the plain receipt

alindima · 2025-01-13T09:33:09Z

polkadot/zombienet_tests/functional/0020-same-collation-to-all-assigned-cores.zndsl

+charlie: js-script ./assign-core.js with "2,2000,57600" return is 0 within 600 seconds
+
+# Ensure parachains made progress.
+alice: parachain 2000 block height is at least 10 within 300 seconds


we need to add some better assertions, including some log checks that indeed validators got invalid candidates.
Also, if you want this test being run in the CI, it needs to be added to the gitlab config in polkadot.yml

alindima · 2025-01-13T09:35:37Z

polkadot/zombienet_tests/functional/0020-same-collation-to-all-assigned-cores.zndsl

+# Register three cores assigned to this parachain.
+alice: js-script ./assign-core.js with "0,2000,57600" return is 0 within 600 seconds
+bob: js-script ./assign-core.js with "1,2000,57600" return is 0 within 600 seconds
+charlie: js-script ./assign-core.js with "2,2000,57600" return is 0 within 600 seconds


core 2 is assigned by default already when force registering the parachain

sandreim · 2025-01-13T10:41:01Z

polkadot/zombienet_tests/functional/0020-same-collation-to-all-assigned-cores.toml

@@ -0,0 +1,54 @@
+[settings]


DSL is deprecated, we have a goal to port tests to Zombienet SDK.

sandreim · 2025-01-13T10:42:33Z

polkadot/parachain/test-parachains/undying/src/lib.rs

@@ -86,6 +90,8 @@ pub struct GraveyardState {
 	pub zombies: u64,
 	// Grave seal.
 	pub seal: [u8; 32],
+	// Increasing sequence number for core selector.
+	pub core_selector_number: u8,


I was expecting we use the block number (LSB), is there anything preventing us to do so ?

Since we use upward messages to place UMP signals and specify the core index, we needed this core_selector_number for the core selector. Do you mean doing something else?

Andrei means that we typically use the block number for the core selector in real parachains.
The undying collator's state does not store the block number, so this core_selector_number is actually the block number

sandreim · 2025-01-13T10:48:27Z

polkadot/parachain/test-parachains/undying/collator/src/lib.rs

+					}
+
+					// Wait before repeating the process.
+					sleep(Duration::from_secs(6 as u64));


Why do we need to wait 6s ? If relay chain skips a slot, we build another candidate, which is different than what the honest collator is doing. We should be building on each relay parent we import.

Do you mean no waiting at all, or should the waiting time somehow depend on the imports?

The current code is very roughly mimicking what the slot based collator does (every slot, pick the best relay chain block)
What Andrei is suggesting is to simulate what the lookahead collator does and subscribe to relay chain block import notifications instead. This would indeed be closer to what the honest collator is doing. I think for a test collator either is fine

alindima · 2025-01-13T11:15:50Z

polkadot/parachain/test-parachains/undying/src/lib.rs

@@ -119,6 +125,7 @@ pub fn execute_transaction(mut block_data: BlockData) -> GraveyardState {
 		// Chain hash the seals and burn CPU.
 		block_data.state.seal = hash_state(&block_data.state);
 	}
+	block_data.state.core_selector_number += 1;


we need to use wrapping_add.

some info about overflows in rust https://doc.rust-lang.org/reference/behavior-not-considered-unsafe.html#integer-overflow

feat(undying-collator): implement initial version of malicious collat…

fc24fab

…or submitting same collation to all backing groups

sw10pa added the T10-tests This PR/Issue is related to tests. label Dec 17, 2024

sw10pa requested review from alindima and sandreim December 17, 2024 12:17

sw10pa self-assigned this Dec 17, 2024

sw10pa and others added 10 commits December 18, 2024 17:28

chore(undying-collator): remove tokio dependency

8c7372c

feat(malus-collator): add initial zombienet test

d517be7

Merge branch 'master' into sw10pa/malus-collator

46fca3f

fix(malus-collator): set correct upward messages

1ecb271

Merge branch 'master' into sw10pa/malus-collator

0c0d625

refactor(malus-collator): move malicious behavior logic from script t…

e430a58

…o Collator implementation

fix(malus-collator): skip core index check when submitting collations

b6d8483

test(malus-collator): add core assignment and update Zombienet test c…

2b1650b

…hecks

Merge branch 'master' into sw10pa/malus-collator

c1f4bba

docs(prdoc): add prdoc for PR #6924

7fc1986

sw10pa mentioned this pull request Jan 9, 2025

Core Index Mismatch in Commitments and Descriptor #7107

Open

fix(malus-collator-zombienet-test): increase max_candidate_depth and …

9e40fe1

…extend test scenario

alindima reviewed Jan 13, 2025

View reviewed changes

sandreim reviewed Jan 13, 2025

View reviewed changes

alindima reviewed Jan 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] malus-collator: implement initial version of malicious collator submitting same collation to all backing groups #6924

[WIP] malus-collator: implement initial version of malicious collator submitting same collation to all backing groups #6924

sw10pa commented Dec 17, 2024 •

edited

Loading

paritytech-workflow-stopper bot commented Dec 17, 2024

alindima left a comment

alindima Jan 13, 2025

alindima Jan 13, 2025

alindima Jan 13, 2025

alindima Jan 13, 2025

alindima Jan 13, 2025

alindima Jan 13, 2025 •

edited

Loading

alindima Jan 13, 2025

alindima Jan 13, 2025

alindima Jan 13, 2025

sandreim Jan 13, 2025

sandreim Jan 13, 2025

sw10pa Jan 13, 2025

alindima Jan 13, 2025

sandreim Jan 13, 2025

sw10pa Jan 13, 2025

alindima Jan 13, 2025

alindima Jan 13, 2025

[WIP] malus-collator: implement initial version of malicious collator submitting same collation to all backing groups #6924

Are you sure you want to change the base?

[WIP] malus-collator: implement initial version of malicious collator submitting same collation to all backing groups #6924

Conversation

sw10pa commented Dec 17, 2024 • edited Loading

Issues

Description

TODO

paritytech-workflow-stopper bot commented Dec 17, 2024

alindima left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alindima Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sw10pa commented Dec 17, 2024 •

edited

Loading

alindima Jan 13, 2025 •

edited

Loading