From b2f29a6a5c2a3cc06b483932cecfd526db1636e3 Mon Sep 17 00:00:00 2001 From: Maxim Shishmarev Date: Fri, 1 Sep 2023 15:33:35 +1000 Subject: [PATCH 1/2] fix: correct select median implementation --- state-chain/pallets/cf-witnesser/src/lib.rs | 5 +- .../src/chainflip/decompose_recompose.rs | 99 +++++++++++++++---- 2 files changed, 83 insertions(+), 21 deletions(-) diff --git a/state-chain/pallets/cf-witnesser/src/lib.rs b/state-chain/pallets/cf-witnesser/src/lib.rs index f737906c77..c2b9c9d7d7 100644 --- a/state-chain/pallets/cf-witnesser/src/lib.rs +++ b/state-chain/pallets/cf-witnesser/src/lib.rs @@ -305,9 +305,7 @@ pub mod pallet { .ok_or(Error::::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::::try_mutate::<_, _, _, Error, _>( &epoch_index, &call_hash, @@ -409,6 +407,7 @@ pub mod pallet { impl Pallet { fn split_calldata(call: &mut ::RuntimeCall) -> (Option>, 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())) } diff --git a/state-chain/runtime/src/chainflip/decompose_recompose.rs b/state-chain/runtime/src/chainflip/decompose_recompose.rs index 1ae62eef15..461edda9b8 100644 --- a/state-chain/runtime/src/chainflip/decompose_recompose.rs +++ b/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(data: &mut [Vec]) -> Option { +fn select_median(mut data: Vec) -> Option { 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(data: &mut [Vec]) -> Vec { + data.iter_mut() + .map(|entry| T::decode(&mut entry.as_slice())) + .filter_map(Result::ok) + .collect() +} + +fn select_median_btc_info(data: Vec) -> Option { + 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,19 +82,23 @@ impl WitnessDataExtraction for RuntimeCall { EthereumInstance, >::update_chain_state { new_chain_state, - }) => - if let Some(median) = select_median(data) { + }) => { + let fee_votes = decode_many::(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::(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, @@ -84,7 +106,8 @@ impl WitnessDataExtraction for RuntimeCall { >::update_chain_state { new_chain_state, }) => { - if let Some(median) = select_median(data) { + let tip_votes = decode_many::(data); + if let Some(median) = select_median(tip_votes) { new_chain_state.tracked_data.median_tip = median; }; }, @@ -240,4 +263,44 @@ 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::(values).unwrap(), 32); + } + + // 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 + }) + ); + } } From d2a5ee23fa7827f0d9b94c2cc4559b2b1850dbf1 Mon Sep 17 00:00:00 2001 From: Maxim Shishmarev Date: Mon, 4 Sep 2023 10:35:57 +1000 Subject: [PATCH 2/2] test: more select median tests --- .../runtime/src/chainflip/decompose_recompose.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/state-chain/runtime/src/chainflip/decompose_recompose.rs b/state-chain/runtime/src/chainflip/decompose_recompose.rs index 461edda9b8..6f5ee43b42 100644 --- a/state-chain/runtime/src/chainflip/decompose_recompose.rs +++ b/state-chain/runtime/src/chainflip/decompose_recompose.rs @@ -272,6 +272,17 @@ mod tests { assert_eq!(select_median::(values).unwrap(), 32); } + #[test] + fn select_median_out_of_order() { + let values = vec![4, 1, 8, 7, 100]; + assert_eq!(select_median::(values).unwrap(), 7); + } + + #[test] + fn select_median_empty() { + assert_eq!(select_median::(vec![]), None); + } + // For BTC, we witness multiple values, and median should be // selected for each value independently: #[test] @@ -303,4 +314,9 @@ mod tests { }) ); } + + #[test] + fn select_median_btc_info_empty() { + assert_eq!(select_median_btc_info(vec![]), None); + } }