Median by Position (#533)

* use median price instead of the highest sustained

* add test for lexicographically reversing a byte slice

* fix pr comments

* fix CI fail

* fix dex tests

* Use a fuzz-tested list of prices

* Working median algorithm based on position + lints

---------

Co-authored-by: akildemir <aeg_asd@hotmail.com>

substrate/dex/pallet/Cargo.toml

@@ -36,6 +36,9 @@ coins-pallet = { package = "serai-coins-pallet", path = "../../coins/pallet", de
 
 serai-primitives = { path = "../../primitives", default-features = false }
 
+[dev-dependencies]
+rand_core = { version = "0.6", default-features = false, features = ["getrandom"] }
+
 [features]
 default = ["std"]
 std = [

substrate/dex/pallet/src/lib.rs

@@ -106,7 +106,7 @@ pub mod pallet {
 
   use coins_pallet::{Pallet as CoinsPallet, Config as CoinsConfig};
 
-  use serai_primitives::{Coin, Amount, Balance, SubstrateAmount};
+  use serai_primitives::{Coin, Amount, Balance, SubstrateAmount, reverse_lexicographic_order};
 
   /// Pool ID.
   ///
@@ -144,6 +144,10 @@ pub mod pallet {
     #[pallet::constant]
     type MaxSwapPathLength: Get<u32>;
 
+    /// Last N number of blocks that oracle keeps track of the prices.
+    #[pallet::constant]
+    type MedianPriceWindowLength: Get<u16>;
+
     /// Weight information for extrinsics in this pallet.
     type WeightInfo: WeightInfo;
   }
@@ -156,34 +160,157 @@ pub mod pallet {
   #[pallet::storage]
   #[pallet::getter(fn spot_price_for_block)]
   pub type SpotPriceForBlock<T: Config> =
-    StorageDoubleMap<_, Identity, BlockNumberFor<T>, Identity, Coin, [u8; 8], ValueQuery>;
+    StorageDoubleMap<_, Identity, BlockNumberFor<T>, Identity, Coin, Amount, OptionQuery>;
 
-  /// Moving window of oracle prices.
+  /// Moving window of prices from each block.
   ///
-  /// The second [u8; 8] key is the amount's big endian bytes, and u16 is the amount of inclusions
-  /// in this multi-set.
+  /// The [u8; 8] key is the amount's big endian bytes, and u16 is the amount of inclusions in this
+  /// multi-set. Since the underlying map is lexicographically sorted, this map stores amounts from
+  /// low to high.
   #[pallet::storage]
-  #[pallet::getter(fn oracle_prices)]
-  pub type OraclePrices<T: Config> =
+  pub type SpotPrices<T: Config> =
     StorageDoubleMap<_, Identity, Coin, Identity, [u8; 8], u16, OptionQuery>;
+
+  // SpotPrices, yet with keys stored in reverse lexicographic order.
+  #[pallet::storage]
+  pub type ReverseSpotPrices<T: Config> =
+    StorageDoubleMap<_, Identity, Coin, Identity, [u8; 8], (), OptionQuery>;
+
+  /// Current length of the `SpotPrices` map.
+  #[pallet::storage]
+  pub type SpotPricesLength<T: Config> = StorageMap<_, Identity, Coin, u16, OptionQuery>;
+
+  /// Current position of the median within the `SpotPrices` map;
+  #[pallet::storage]
+  pub type CurrentMedianPosition<T: Config> = StorageMap<_, Identity, Coin, u16, OptionQuery>;
+
+  /// Current median price of the prices in the `SpotPrices` map at any given time.
+  #[pallet::storage]
+  #[pallet::getter(fn median_price)]
+  pub type MedianPrice<T: Config> = StorageMap<_, Identity, Coin, Amount, OptionQuery>;
+
+  /// The price used for evaluating economic security, which is the highest observed median price.
+  #[pallet::storage]
+  #[pallet::getter(fn security_oracle_value)]
+  pub type SecurityOracleValue<T: Config> = StorageMap<_, Identity, Coin, Amount, OptionQuery>;
+
   impl<T: Config> Pallet<T> {
-    // TODO: consider an algorithm which removes outliers? This algorithm might work a good bit
-    // better if we remove the bottom n values (so some value sustained over 90% of blocks instead
-    // of all blocks in the window).
-    /// Get the highest sustained value for this window.
-    /// This is actually the lowest price observed during the windows, as it's the price
-    /// all prices are greater than or equal to.
-    pub fn highest_sustained_price(coin: &Coin) -> Option<Amount> {
-      let mut iter = OraclePrices::<T>::iter_key_prefix(coin);
-      // the first key will be the lowest price due to the keys being lexicographically ordered.
-      iter.next().map(|amount| Amount(u64::from_be_bytes(amount)))
+    fn restore_median(
+      coin: Coin,
+      mut current_median_pos: u16,
+      mut current_median: Amount,
+      length: u16,
+    ) {
+      // 1 -> 0 (the only value)
+      // 2 -> 1 (the higher element), 4 -> 2 (the higher element)
+      // 3 -> 1 (the true median)
+      let target_median_pos = length / 2;
+      while current_median_pos < target_median_pos {
+        // Get the amount of presences for the current element
+        let key = current_median.0.to_be_bytes();
+        let presences = SpotPrices::<T>::get(coin, key).unwrap();
+        // > is correct, not >=.
+        // Consider:
+        // - length = 1, current_median_pos = 0, presences = 1, target_median_pos = 0
+        // - length = 2, current_median_pos = 0, presences = 2, target_median_pos = 1
+        // - length = 2, current_median_pos = 0, presences = 1, target_median_pos = 1
+        if (current_median_pos + presences) > target_median_pos {
+          break;
+        }
+        current_median_pos += presences;
+
+        let key = SpotPrices::<T>::hashed_key_for(coin, key);
+        let next_price = SpotPrices::<T>::iter_key_prefix_from(coin, key).next().unwrap();
+        current_median = Amount(u64::from_be_bytes(next_price));
+      }
+
+      while current_median_pos > target_median_pos {
+        // Get the next element
+        let key = reverse_lexicographic_order(current_median.0.to_be_bytes());
+        let key = ReverseSpotPrices::<T>::hashed_key_for(coin, key);
+        let next_price = ReverseSpotPrices::<T>::iter_key_prefix_from(coin, key).next().unwrap();
+        let next_price = reverse_lexicographic_order(next_price);
+        current_median = Amount(u64::from_be_bytes(next_price));
+
+        // Get its amount of presences
+        let presences = SpotPrices::<T>::get(coin, current_median.0.to_be_bytes()).unwrap();
+        // Adjust from next_value_first_pos to this_value_first_pos by substracting this value's
+        // amount of times present
+        current_median_pos -= presences;
+
+        if current_median_pos <= target_median_pos {
+          break;
+        }
+      }
+
+      CurrentMedianPosition::<T>::set(coin, Some(current_median_pos));
+      MedianPrice::<T>::set(coin, Some(current_median));
+    }
+
+    pub(crate) fn insert_into_median(coin: Coin, amount: Amount) {
+      let new_quantity_of_presences =
+        SpotPrices::<T>::get(coin, amount.0.to_be_bytes()).unwrap_or(0) + 1;
+      SpotPrices::<T>::set(coin, amount.0.to_be_bytes(), Some(new_quantity_of_presences));
+      if new_quantity_of_presences == 1 {
+        ReverseSpotPrices::<T>::set(
+          coin,
+          reverse_lexicographic_order(amount.0.to_be_bytes()),
+          Some(()),
+        );
+      }
+
+      let new_length = SpotPricesLength::<T>::get(coin).unwrap_or(0) + 1;
+      SpotPricesLength::<T>::set(coin, Some(new_length));
+
+      let Some(current_median) = MedianPrice::<T>::get(coin) else {
+        MedianPrice::<T>::set(coin, Some(amount));
+        CurrentMedianPosition::<T>::set(coin, Some(0));
+        return;
+      };
+
+      let mut current_median_pos = CurrentMedianPosition::<T>::get(coin).unwrap();
+      // If this is being inserted before the current median, the current median's position has
+      // increased
+      if amount < current_median {
+        current_median_pos += 1;
+      }
+      Self::restore_median(coin, current_median_pos, current_median, new_length);
+    }
+
+    pub(crate) fn remove_from_median(coin: Coin, amount: Amount) {
+      let mut current_median = MedianPrice::<T>::get(coin).unwrap();
+
+      let mut current_median_pos = CurrentMedianPosition::<T>::get(coin).unwrap();
+      if amount < current_median {
+        current_median_pos -= 1;
+      }
+
+      let new_quantity_of_presences =
+        SpotPrices::<T>::get(coin, amount.0.to_be_bytes()).unwrap() - 1;
+      if new_quantity_of_presences == 0 {
+        let normal_key = amount.0.to_be_bytes();
+        SpotPrices::<T>::remove(coin, normal_key);
+        ReverseSpotPrices::<T>::remove(coin, reverse_lexicographic_order(amount.0.to_be_bytes()));
+
+        // If we've removed the current item at this position, update to the item now at this
+        // position
+        if amount == current_median {
+          let key = SpotPrices::<T>::hashed_key_for(coin, normal_key);
+          current_median = Amount(u64::from_be_bytes(
+            SpotPrices::<T>::iter_key_prefix_from(coin, key).next().unwrap(),
+          ));
+        }
+      } else {
+        SpotPrices::<T>::set(coin, amount.0.to_be_bytes(), Some(new_quantity_of_presences));
+      }
+
+      let new_length = SpotPricesLength::<T>::get(coin).unwrap() - 1;
+      SpotPricesLength::<T>::set(coin, Some(new_length));
+
+      Self::restore_median(coin, current_median_pos, current_median, new_length);
     }
   }
 
-  #[pallet::storage]
-  #[pallet::getter(fn oracle_value)]
-  pub type OracleValue<T: Config> = StorageMap<_, Identity, Coin, Amount, OptionQuery>;
-
   // Pallet's events.
   #[pallet::event]
   #[pallet::generate_deposit(pub(super) fn deposit_event)]
@@ -264,12 +391,6 @@ pub mod pallet {
   #[pallet::genesis_build]
   impl<T: Config> BuildGenesisConfig for GenesisConfig<T> {
     fn build(&self) {
-      // assert that oracle windows size can fit into u16. Otherwise number of observants
-      // for a price in the `OraclePrices` map can overflow
-      // We don't want to make this const directly a u16 because it is used the block number
-      // calculations (which are done as u32s)
-      u16::try_from(ORACLE_WINDOW_SIZE).unwrap();
-
       // create the pools
       for coin in &self.pools {
         Pallet::<T>::create_pool(*coin).unwrap();
@@ -362,35 +483,22 @@ pub mod pallet {
           } else {
             0
           };
-        let sri_per_coin = sri_per_coin.to_be_bytes();
 
-        SpotPriceForBlock::<T>::set(n, coin, sri_per_coin);
-
-        // Include this spot price into the multiset
-        {
-          let observed = OraclePrices::<T>::get(coin, sri_per_coin).unwrap_or(0);
-          OraclePrices::<T>::set(coin, sri_per_coin, Some(observed + 1));
-        }
-
-        // pop the earliest key from the window once we reach its full size.
-        if n >= ORACLE_WINDOW_SIZE.into() {
-          let start_of_window = n - ORACLE_WINDOW_SIZE.into();
-          let start_spot_price = Self::spot_price_for_block(start_of_window, coin);
-          SpotPriceForBlock::<T>::remove(start_of_window, coin);
-          // Remove this price from the multiset
-          OraclePrices::<T>::mutate_exists(coin, start_spot_price, |v| {
-            *v = Some(v.unwrap_or(1) - 1);
-            if *v == Some(0) {
-              *v = None;
-            }
-          });
+        let sri_per_coin = Amount(sri_per_coin);
+        SpotPriceForBlock::<T>::set(n, coin, Some(sri_per_coin));
+        Self::insert_into_median(coin, sri_per_coin);
+        if SpotPricesLength::<T>::get(coin).unwrap() > T::MedianPriceWindowLength::get() {
+          let old = n - T::MedianPriceWindowLength::get().into();
+          let old_price = SpotPriceForBlock::<T>::get(old, coin).unwrap();
+          SpotPriceForBlock::<T>::remove(old, coin);
+          Self::remove_from_median(coin, old_price);
         }
 
         // update the oracle value
-        let highest_sustained = Self::highest_sustained_price(&coin).unwrap_or(Amount(0));
-        let oracle_value = Self::oracle_value(coin).unwrap_or(Amount(0));
-        if highest_sustained > oracle_value {
-          OracleValue::<T>::set(coin, Some(highest_sustained));
+        let median = Self::median_price(coin).unwrap_or(Amount(0));
+        let oracle_value = Self::security_oracle_value(coin).unwrap_or(Amount(0));
+        if median > oracle_value {
+          SecurityOracleValue::<T>::set(coin, Some(median));
         }
       }
     }
@@ -422,7 +530,7 @@ pub mod pallet {
     pub fn on_new_session(network: NetworkId) {
       // reset the oracle value
       for coin in network.coins() {
-        OracleValue::<T>::set(*coin, Self::highest_sustained_price(coin));
+        SecurityOracleValue::<T>::set(*coin, Self::median_price(coin));
       }
     }
   }

substrate/dex/pallet/src/mock.rs

@@ -25,7 +25,7 @@ use crate as dex;
 
 use frame_support::{
   construct_runtime,
-  traits::{ConstU32, ConstU64},
+  traits::{ConstU16, ConstU32, ConstU64},
 };
 
 use sp_core::{H256, sr25519::Public};
@@ -40,6 +40,8 @@ pub use coins_pallet as coins;
 
 type Block = frame_system::mocking::MockBlock<Test>;
 
+pub const MEDIAN_PRICE_WINDOW_LENGTH: u16 = 10;
+
 construct_runtime!(
   pub enum Test
   {
@@ -92,6 +94,9 @@ impl Config for Test {
   type WeightInfo = ();
   type LPFee = ConstU32<3>; // means 0.3%
   type MaxSwapPathLength = ConstU32<4>;
+
+  type MedianPriceWindowLength = ConstU16<{ MEDIAN_PRICE_WINDOW_LENGTH }>;
+
   // 100 is good enough when the main currency has 12 decimals.
   type MintMinLiquidity = ConstU64<100>;
 }

substrate/dex/pallet/src/tests.rs

@@ -1267,3 +1267,53 @@ fn cannot_block_pool_creation() {
     assert_ok!(Dex::add_liquidity(RuntimeOrigin::signed(user), coin2, 100, 9900, 10, 9900, user,));
   });
 }
+
+#[test]
+fn test_median_price() {
+  new_test_ext().execute_with(|| {
+    use rand_core::{RngCore, OsRng};
+
+    let mut prices = vec![];
+    for i in 0 .. 100 {
+      // Randomly use an active number
+      if (i != 0) && (OsRng.next_u64() % u64::from(MEDIAN_PRICE_WINDOW_LENGTH / 3) == 0) {
+        let old_index = usize::try_from(
+          OsRng.next_u64() %
+            u64::from(MEDIAN_PRICE_WINDOW_LENGTH) %
+            u64::try_from(prices.len()).unwrap(),
+        )
+        .unwrap();
+        let window_base = prices.len().saturating_sub(MEDIAN_PRICE_WINDOW_LENGTH.into());
+        prices.push(prices[window_base + old_index]);
+      } else {
+        prices.push(OsRng.next_u64());
+      }
+    }
+    let coin = Coin::Bitcoin;
+
+    assert!(prices.len() >= (2 * usize::from(MEDIAN_PRICE_WINDOW_LENGTH)));
+    for i in 0 .. prices.len() {
+      let price = Amount(prices[i]);
+
+      let n = BlockNumberFor::<Test>::from(u32::try_from(i).unwrap());
+      SpotPriceForBlock::<Test>::set(n, coin, Some(price));
+      Dex::insert_into_median(coin, price);
+      if SpotPricesLength::<Test>::get(coin).unwrap() > MEDIAN_PRICE_WINDOW_LENGTH {
+        let old = n - u64::from(MEDIAN_PRICE_WINDOW_LENGTH);
+        let old_price = SpotPriceForBlock::<Test>::get(old, coin).unwrap();
+        SpotPriceForBlock::<Test>::remove(old, coin);
+        Dex::remove_from_median(coin, old_price);
+      }
+
+      // get the current window (cloning so our sort doesn't affect the original array)
+      let window_base = (i + 1).saturating_sub(MEDIAN_PRICE_WINDOW_LENGTH.into());
+      let mut window = Vec::from(&prices[window_base ..= i]);
+      assert!(window.len() <= MEDIAN_PRICE_WINDOW_LENGTH.into());
+
+      // get the median
+      window.sort();
+      let median_index = window.len() / 2;
+      assert_eq!(Dex::median_price(coin).unwrap(), Amount(window[median_index]));
+    }
+  });
+}

substrate/dex/pallet/src/types.rs

@@ -20,10 +20,6 @@
 
 use super::*;
 
-/// This needs to be long enough for arbitrage to occur and make holding
-/// any fake price up sufficiently unprofitable.
-pub const ORACLE_WINDOW_SIZE: u32 = 1000;
-
 /// Trait for providing methods to swap between the various coin classes.
 pub trait Swap<AccountId, Balance, MultiCoinId> {
   /// Swap exactly `amount_in` of coin `path[0]` for coin `path[1]`.