Ignore Envelopes with Unkown Validators (#546)

* update functions for validating transactions/envelopes in pallet stellar-relay

* #546 (comment),
#546 (comment),
#546 (comment)

* update commit # of `substrate-stellar-sdk`

* use `new_empty()` instead

* fix https://github.com/pendulum-chain/spacewalk/actions/runs/10261095162/job/28388240959#step:13:1188

* fix https://github.com/pendulum-chain/spacewalk/actions/runs/10261095162/job/28388240959#step:13:38

* fix https://github.com/pendulum-chain/spacewalk/actions/runs/10261095162/job/28388240959#step:13:437

explicitly define `sp_std`

* fix https://github.com/pendulum-chain/spacewalk/actions/runs/10263219351/job/28396526378?pr=546#step:13:556

* run the jobs sequentially

* fix https://github.com/pendulum-chain/spacewalk/actions/runs/10266350453/job/28410838494?pr=546#step:13:689

* cargo fmt

* adding back frankfurt in the list

* #546 (comment),
#546 (comment)

* update the tests to receive a slot first, before proceeding

* fix fmt: https://github.com/pendulum-chain/spacewalk/actions/runs/10696022263/job/29664333392?pr=546

clients/stellar-relay-lib/src/connection/connector/connector.rs

@@ -234,9 +234,7 @@ impl Connector {
 		local_overlay_version: u32,
 		remote_overlay_version: u32,
 	) -> StellarMessage {
-		return self
-			.flow_controller
-			.start(local_overlay_version, remote_overlay_version);
+		return self.flow_controller.start(local_overlay_version, remote_overlay_version);
 	}
 
 	pub fn handshake_completed(&mut self) {

clients/stellar-relay-lib/src/connection/connector/message_handler.rs

@@ -41,7 +41,7 @@ impl Connector {
 					return Err(Error::from(e));
 				},
 				other => error!(
-					"process_raw_message(): Received ErrorMsg during authentication: {:?}",
+					"process_raw_message(): Received a different message during authentication: {:?}",
 					other
 				),
 			},

clients/vault/src/oracle/testing_utils.rs

@@ -25,7 +25,7 @@ pub fn specific_stellar_relay_config(
 
 fn stellar_relay_config_choices(is_mainnet: bool) -> (Vec<&'static str>, &'static str) {
 	let node_points = if is_mainnet {
-		vec!["frankfurt", "iowa", "singapore"]
+		vec!["iowa", "singapore", "frankfurt"]
 	} else {
 		vec!["sdftest1", "sdftest2", "sdftest3"]
 	};

clients/vault/tests/helper/mod.rs

@@ -149,5 +149,8 @@ where
 			.expect("failed to start agent");
 	let oracle_agent = Arc::new(oracle_agent);
 
+	// continue ONLY if the oracle agent has received the first slot
+	while oracle_agent.last_slot_index().await == 0 {}
+
 	execute(client, vault_wallet, user_wallet, oracle_agent, vault_id, vault_provider).await
 }

pallets/stellar-relay/Cargo.toml

@@ -17,6 +17,7 @@ base64 = { version = '0.13.0', default-features = false, features = ['alloc'] }
 codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false, features = [
     "derive",
 ] }
+log = { version = "0.4.14", default-features = false }
 serde = {version = "1.0.130", default-features = false, features = ["derive"]}
 scale-info = { version = "2.1.1", default-features = false, features = ["derive"] }
 frame-support = { default-features = false, git = "https://github.com/paritytech/polkadot-sdk.git", branch = "release-polkadot-v1.1.0" }

pallets/stellar-relay/src/lib.rs

@@ -31,7 +31,7 @@ use primitives::{derive_shortened_request_id, get_text_memo_from_tx_env, TextMem
 #[frame_support::pallet]
 pub mod pallet {
 	use codec::FullCodec;
-	use frame_support::{pallet_prelude::*, sp_runtime, transactional};
+	use frame_support::{pallet_prelude::*, transactional};
 	use frame_system::pallet_prelude::*;
 	use primitives::stellar::{
 		compound_types::UnlimitedVarArray,
@@ -44,13 +44,14 @@ pub mod pallet {
 	use sp_std::{collections::btree_map::BTreeMap, fmt::Debug, vec, vec::Vec};
 
 	use default_weights::WeightInfo;
+	use primitives::stellar::types::ScpStatementPledges;
 
 	use crate::{
 		traits::FieldLength,
 		types::{OrganizationOf, ValidatorOf},
 		validation::{
-			check_for_valid_quorum_set, find_externalized_envelope, get_externalized_info,
-			validate_envelopes, validators_and_orgs,
+			check_for_valid_quorum_set, get_externalized_info, validate_envelopes,
+			validators_and_orgs,
 		},
 	};
 
@@ -108,7 +109,6 @@ pub mod pallet {
 		DuplicateOrganizationId,
 		DuplicateValidatorPublicKey,
 		EmptyEnvelopeSet,
-		EnvelopeSignedByUnknownValidator,
 		EnvelopeSlotIndexMismatch,
 		ExternalizedNHMismatch,
 		ExternalizedValueMismatch,
@@ -560,7 +560,12 @@ pub mod pallet {
 
 			let (validators, organizations) = validators_and_orgs()?;
 
-			let externalized_envelope = find_externalized_envelope(envelopes)?;
+			let externalized_envelope = envelopes
+				.get_element(|envelope| match envelope.statement.pledges {
+					ScpStatementPledges::ScpStExternalize(_) => true,
+					_ => false,
+				})
+				.ok_or(Error::MissingExternalizedMessage)?;
 
 			// We store the externalized value in a variable so that we can check if it's the same
 			// for all envelopes. We don't distinguish between externalized and confirmed values as
@@ -574,7 +579,8 @@ pub mod pallet {
 				.get_tx_set_hash()
 				.map_err(|_| Error::<T>::FailedToComputeNonGenericTxSetContentHash)?;
 
-			validate_envelopes(
+			// returns ONLY the valid envelopes
+			let validated_envelopes = validate_envelopes(
 				envelopes,
 				&validators,
 				&network,
@@ -586,7 +592,7 @@ pub mod pallet {
 			)?;
 
 			// ---- Check that externalized messages build valid quorum set ----
-			check_for_valid_quorum_set(envelopes, validators, organizations.len())
+			check_for_valid_quorum_set(validated_envelopes, validators, organizations.len())
 		}
 
 		pub(crate) fn get_tx_set_hash(scp_value: &Value) -> Result<Hash, Error<T>> {

pallets/stellar-relay/src/tests.rs

@@ -195,7 +195,7 @@ fn validate_stellar_transaction_fails_for_wrong_signature() {
 }
 
 #[test]
-fn validate_stellar_transaction_fails_for_unknown_validator() {
+fn validate_stellar_transaction_ignores_unknown_validator() {
 	run_test(|organizations, mut validators, mut validator_secret_keys| {
 		let public_network = true;
 
@@ -212,9 +212,8 @@ fn validate_stellar_transaction_fails_for_unknown_validator() {
 			false,
 		);
 
-		let result =
-			SpacewalkRelay::validate_stellar_transaction(&tx_envelope, &scp_envelopes, &tx_set);
-		assert!(matches!(result, Err(Error::<Test>::EnvelopeSignedByUnknownValidator)));
+		assert!(SpacewalkRelay::validate_stellar_transaction(&tx_envelope, &scp_envelopes, &tx_set)
+			.is_ok())
 	});
 }
 

pallets/stellar-relay/src/validation.rs

@@ -35,16 +35,18 @@ fn validator_count_per_org<T: Config>(
 
 /// Builds a map used to identify the targeted organizations
 fn targeted_organization_map<T: Config>(
-	envelopes: &UnlimitedVarArray<ScpEnvelope>,
+	envelopes: UnlimitedVarArray<&ScpEnvelope>,
 	validators: &ValidatorsList<T>,
 ) -> BTreeMap<T::OrganizationId, u32> {
 	// Find the validators that are targeted by the SCP messages
 	let targeted_validators = validators
 		.iter()
 		.filter(|validator| {
-			envelopes.get_vec().iter().any(|envelope| {
-				envelope.statement.node_id.to_encoding() == validator.public_key.to_vec()
-			})
+			envelopes
+				.get_element(|envelope| {
+					envelope.statement.node_id.to_encoding() == validator.public_key.to_vec()
+				})
+				.is_some()
 		})
 		.collect::<Vec<&ValidatorOf<T>>>();
 
@@ -75,22 +77,29 @@ pub fn get_externalized_info<T: Config>(envelope: &ScpEnvelope) -> Result<(&Valu
 }
 
 /// Returns the node id of the envelope if it is part of the set of validators
-fn get_node_id<T: Config>(
+fn is_node_id_exist<T: Config>(
 	envelope: &ScpEnvelope,
 	validators: &BoundedVec<ValidatorOf<T>, T::ValidatorLimit>,
-) -> Result<NodeId, Error<T>> {
+) -> Option<NodeId> {
 	let node_id = envelope.statement.node_id.clone();
 	let node_id_found = validators
 		.iter()
 		.any(|validator| validator.public_key.to_vec() == node_id.to_encoding());
 
-	ensure!(node_id_found, Error::<T>::EnvelopeSignedByUnknownValidator);
+	if !node_id_found {
+		log::warn!(
+			"Envelope with slot index {}: Node id {:?} is not part of validators list",
+			envelope.statement.slot_index,
+			envelope.statement.node_id
+		);
+		return None
+	}
 
-	Ok(node_id)
+	Some(node_id)
 }
 
 pub fn check_for_valid_quorum_set<T: Config>(
-	envelopes: &UnlimitedVarArray<ScpEnvelope>,
+	envelopes: UnlimitedVarArray<&ScpEnvelope>,
 	validators: BoundedVec<ValidatorOf<T>, T::ValidatorLimit>,
 	orgs_length: usize,
 ) -> Result<(), Error<T>> {
@@ -136,7 +145,8 @@ pub fn check_for_valid_quorum_set<T: Config>(
 	Ok(())
 }
 
-/// Checks that all envelopes have the same values
+/// Checks that all envelopes have the same values. IGNORES envelopes with DIFFERENT node id.
+/// Returns ONLY the valid envelopes.
 ///
 /// # Arguments
 ///
@@ -147,18 +157,25 @@ pub fn check_for_valid_quorum_set<T: Config>(
 /// * `externalized_n_h` - A value that must be equal amongst all envelopes
 /// * `expected_tx_set_hash` - A value that must be equal amongst all envelopes
 /// * `slot_index` - used to check if all envelopes are using the same slot
-pub fn validate_envelopes<T: Config>(
-	envelopes: &UnlimitedVarArray<ScpEnvelope>,
+pub fn validate_envelopes<'a, T: Config>(
+	envelopes: &'a UnlimitedVarArray<ScpEnvelope>,
 	validators: &BoundedVec<ValidatorOf<T>, T::ValidatorLimit>,
 	network: &Network,
 	externalized_value: &Value,
 	externalized_n_h: u32,
 	expected_tx_set_hash: Hash,
 	slot_index: u64,
-) -> Result<(), Error<T>> {
+) -> Result<UnlimitedVarArray<&'a ScpEnvelope>, Error<T>> {
+	// let's create a new placeholder for valid envelopes
+	let mut validated_envelopes = sp_std::vec![];
 	let mut externalized_n_h = externalized_n_h;
-	for envelope in envelopes.get_vec() {
-		let node_id = get_node_id(envelope, validators)?;
+
+	let envelopes = envelopes.get_vec();
+	for envelope in envelopes {
+		let Some(node_id) = is_node_id_exist::<T>(envelope, validators) else {
+			// ignore this envelope; continue to the next ones
+			continue
+		};
 
 		// Check if all envelopes are using the same slot index
 		ensure!(slot_index == envelope.statement.slot_index, Error::<T>::EnvelopeSlotIndexMismatch);
@@ -185,22 +202,12 @@ pub fn validate_envelopes<T: Config>(
 		else if n_h < u32::MAX {
 			ensure!(externalized_n_h == n_h, Error::<T>::ExternalizedNHMismatch);
 		}
-	}
 
-	Ok(())
-}
+		// if all checks passed, insert.
+		validated_envelopes.push(envelope);
+	}
 
-pub fn find_externalized_envelope<T: Config>(
-	envelopes: &UnlimitedVarArray<ScpEnvelope>,
-) -> Result<&ScpEnvelope, Error<T>> {
-	envelopes
-		.get_vec()
-		.iter()
-		.find(|envelope| match envelope.statement.pledges {
-			ScpStatementPledges::ScpStExternalize(_) => true,
-			_ => false,
-		})
-		.ok_or(Error::<T>::MissingExternalizedMessage)
+	Ok(UnlimitedVarArray::new(validated_envelopes).unwrap_or(UnlimitedVarArray::new_empty()))
 }
 
 pub fn validators_and_orgs<T: Config>(