Fix: Correct Select Median Implementation (#3934)

* fix: correct select median implementation

* test: more select median tests

state-chain/pallets/cf-witnesser/src/lib.rs

@@ -305,9 +305,7 @@ pub mod pallet {
 				.ok_or(Error::<T>::UnauthorisedWitness)? as usize;
 
 			// Register the vote
-			// `extract()` modifies the call, so we need to calculate the call hash *after* this.
-			let extra_data = call.extract();
-			let call_hash = CallHash(call.blake2_256());
+			let (extra_data, call_hash) = Self::split_calldata(&mut call);
 			let num_votes = Votes::<T>::try_mutate::<_, _, _, Error<T>, _>(
 				&epoch_index,
 				&call_hash,
@@ -409,6 +407,7 @@ pub mod pallet {
 impl<T: Config> Pallet<T> {
 	fn split_calldata(call: &mut <T as Config>::RuntimeCall) -> (Option<Vec<u8>>, CallHash) {
 		let extra_data = call.extract();
+		// `extract()` modifies the call, so we need to calculate the call hash *after* this.
 		(extra_data, CallHash(call.blake2_256()))
 	}
 

state-chain/runtime/src/chainflip/decompose_recompose.rs

@@ -1,26 +1,44 @@
 use crate::{BitcoinInstance, EthereumInstance, PolkadotInstance, Runtime, RuntimeCall};
+use cf_chains::{btc::BitcoinFeeInfo, dot::PolkadotBalance};
+use cf_primitives::EthAmount;
 use codec::{Decode, Encode};
 use pallet_cf_witnesser::WitnessDataExtraction;
 use sp_std::{mem, prelude::*};
 
-fn select_median<T: Encode + Decode>(data: &mut [Vec<u8>]) -> Option<T> {
+fn select_median<T: Ord + Copy>(mut data: Vec<T>) -> Option<T> {
 	if data.is_empty() {
 		return None
 	}
 
 	let len = data.len();
-	let median_index = if len % 2 == 0 { (len - 1) / 2 } else { len / 2 };
-	// Encoding is order-preserving so we can sort the raw encoded bytes and then decode
-	// just the result.
-	let (_, median_bytes, _) = data.select_nth_unstable(median_index);
-
-	match Decode::decode(&mut &median_bytes[..]) {
-		Ok(median) => Some(median),
-		Err(e) => {
-			log::error!("Failed to decode median priority fee: {:?}", e);
-			None
-		},
+	let median_index = (len - 1) / 2;
+	let (_, median_value, _) = data.select_nth_unstable(median_index);
+
+	Some(*median_value)
+}
+
+fn decode_many<T: Encode + Decode>(data: &mut [Vec<u8>]) -> Vec<T> {
+	data.iter_mut()
+		.map(|entry| T::decode(&mut entry.as_slice()))
+		.filter_map(Result::ok)
+		.collect()
+}
+
+fn select_median_btc_info(data: Vec<BitcoinFeeInfo>) -> Option<BitcoinFeeInfo> {
+	if data.is_empty() {
+		return None
 	}
+
+	Some(BitcoinFeeInfo {
+		fee_per_input_utxo: select_median(data.iter().map(|x| x.fee_per_input_utxo).collect())
+			.expect("non-empty list"),
+		fee_per_output_utxo: select_median(data.iter().map(|x| x.fee_per_output_utxo).collect())
+			.expect("non-empty list"),
+		min_fee_required_per_tx: select_median(
+			data.iter().map(|x| x.min_fee_required_per_tx).collect(),
+		)
+		.expect("non-empty list"),
+	})
 }
 
 impl WitnessDataExtraction for RuntimeCall {
@@ -64,27 +82,32 @@ impl WitnessDataExtraction for RuntimeCall {
 				EthereumInstance,
 			>::update_chain_state {
 				new_chain_state,
-			}) =>
-				if let Some(median) = select_median(data) {
+			}) => {
+				let fee_votes = decode_many::<EthAmount>(data);
+				if let Some(median) = select_median(fee_votes) {
 					new_chain_state.tracked_data.priority_fee = median;
-				},
+				}
+			},
 			RuntimeCall::BitcoinChainTracking(pallet_cf_chain_tracking::Call::<
 				Runtime,
 				BitcoinInstance,
 			>::update_chain_state {
 				new_chain_state,
 			}) => {
-				if let Some(median) = select_median(data) {
+				let fee_infos = decode_many::<BitcoinFeeInfo>(data);
+
+				if let Some(median) = select_median_btc_info(fee_infos) {
 					new_chain_state.tracked_data.btc_fee_info = median;
-				};
+				}
 			},
 			RuntimeCall::PolkadotChainTracking(pallet_cf_chain_tracking::Call::<
 				Runtime,
 				PolkadotInstance,
 			>::update_chain_state {
 				new_chain_state,
 			}) => {
-				if let Some(median) = select_median(data) {
+				let tip_votes = decode_many::<PolkadotBalance>(data);
+				if let Some(median) = select_median(tip_votes) {
 					new_chain_state.tracked_data.median_tip = median;
 				};
 			},
@@ -240,4 +263,60 @@ mod tests {
 			);
 		})
 	}
+
+	// Selecting median from integers spanning multiple bytes wasn't
+	// working correctly previously, so this serves as a regression test:
+	#[test]
+	fn select_median_multi_bytes_ints() {
+		let values = vec![1_u16, 8, 32, 256, 768];
+		assert_eq!(select_median::<u16>(values).unwrap(), 32);
+	}
+
+	#[test]
+	fn select_median_out_of_order() {
+		let values = vec![4, 1, 8, 7, 100];
+		assert_eq!(select_median::<u16>(values).unwrap(), 7);
+	}
+
+	#[test]
+	fn select_median_empty() {
+		assert_eq!(select_median::<u16>(vec![]), None);
+	}
+
+	// For BTC, we witness multiple values, and median should be
+	// selected for each value independently:
+	#[test]
+	fn select_median_btc_info_test() {
+		let votes = vec![
+			BitcoinFeeInfo {
+				fee_per_input_utxo: 10,
+				fee_per_output_utxo: 55,
+				min_fee_required_per_tx: 100,
+			},
+			BitcoinFeeInfo {
+				fee_per_input_utxo: 45,
+				fee_per_output_utxo: 100,
+				min_fee_required_per_tx: 10,
+			},
+			BitcoinFeeInfo {
+				fee_per_input_utxo: 100,
+				fee_per_output_utxo: 10,
+				min_fee_required_per_tx: 50,
+			},
+		];
+
+		assert_eq!(
+			select_median_btc_info(votes),
+			Some(BitcoinFeeInfo {
+				fee_per_input_utxo: 45,
+				fee_per_output_utxo: 55,
+				min_fee_required_per_tx: 50
+			})
+		);
+	}
+
+	#[test]
+	fn select_median_btc_info_empty() {
+		assert_eq!(select_median_btc_info(vec![]), None);
+	}
 }