Fix fork choice rule to prefer the heaviest chain

crates/sc-consensus-subspace/src/lib.rs

@@ -69,7 +69,6 @@ use sp_consensus_subspace::{
 use sp_core::H256;
 use sp_inherents::{CreateInherentDataProviders, InherentDataProvider};
 use sp_runtime::traits::One;
-use std::cmp::Ordering;
 use std::future::Future;
 use std::marker::PhantomData;
 use std::num::NonZeroUsize;
@@ -1121,8 +1120,6 @@ where
         .await
         .map_err(|error| ConsensusError::ClientImport(error.to_string()))?;
 
-        let pre_digest = subspace_digest_items.pre_digest;
-
         let parent_weight = if block_number.is_one() {
             0
         } else {
@@ -1136,12 +1133,7 @@ where
                 })?
         };
 
-        let added_weight = calculate_block_weight::<PosTable, _, _>(
-            &pre_digest.solution,
-            pre_digest.slot.into(),
-            &subspace_digest_items.global_randomness,
-        )
-        .map_err(|error| ConsensusError::ClientImport(error.to_string()))?;
+        let added_weight = calculate_block_weight(subspace_digest_items.solution_range);
         let total_weight = parent_weight + added_weight;
 
         let info = self.client.info();
@@ -1175,14 +1167,12 @@ where
         // The fork choice rule is that we pick the heaviest chain (i.e. smallest solution
         // range), if there's a tie we go with the longest chain.
         let fork_choice = {
-            let (last_best, last_best_number) = (info.best_hash, info.best_number);
-
-            let last_best_weight = if &last_best == block.header.parent_hash() {
+            let last_best_weight = if &info.best_hash == block.header.parent_hash() {
                 // the parent=genesis case is already covered for loading parent weight,
                 // so we don't need to cover again here.
                 parent_weight
             } else {
-                aux_schema::load_block_weight(&*self.client, last_best)
+                aux_schema::load_block_weight(&*self.client, info.best_hash)
                     .map_err(|e| ConsensusError::ChainLookup(e.to_string()))?
                     .ok_or_else(|| {
                         ConsensusError::ChainLookup(
@@ -1191,11 +1181,7 @@ where
                     })?
             };
 
-            ForkChoiceStrategy::Custom(match total_weight.cmp(&last_best_weight) {
-                Ordering::Greater => true,
-                Ordering::Equal => block_number > last_best_number,
-                Ordering::Less => false,
-            })
+            ForkChoiceStrategy::Custom(total_weight > last_best_weight)
         };
         block.fork_choice = Some(fork_choice);
 

crates/sp-consensus-subspace/src/lib.rs

@@ -431,14 +431,13 @@ impl PosExtension {
 /// Consensus-related runtime interface
 #[runtime_interface]
 pub trait Consensus {
-    /// Verify whether solution is valid, returns solution distance that is `<= solution_range/2` on
-    /// success.
+    /// Verify whether solution is valid.
     fn verify_solution(
         &mut self,
         solution: WrappedSolution,
         slot: u64,
         params: WrappedVerifySolutionParams<'_>,
-    ) -> Result<SolutionRange, String> {
+    ) -> Result<(), String> {
         use sp_externalities::ExternalitiesExt;
         use subspace_proof_of_space::PosTableType;
 
@@ -453,21 +452,27 @@ pub trait Consensus {
             .0;
 
         match pos_table_type {
-            PosTableType::Chia => subspace_verification::verify_solution::<ChiaTable, _, _>(
-                &solution.0,
-                slot,
-                &params.0,
-                kzg,
-            )
-            .map_err(|error| error.to_string()),
-            PosTableType::Shim => subspace_verification::verify_solution::<ShimTable, _, _>(
-                &solution.0,
-                slot,
-                &params.0,
-                kzg,
-            )
-            .map_err(|error| error.to_string()),
+            PosTableType::Chia => {
+                subspace_verification::verify_solution::<ChiaTable, _, _>(
+                    &solution.0,
+                    slot,
+                    &params.0,
+                    kzg,
+                )
+                .map_err(|error| error.to_string())?;
+            }
+            PosTableType::Shim => {
+                subspace_verification::verify_solution::<ShimTable, _, _>(
+                    &solution.0,
+                    slot,
+                    &params.0,
+                    kzg,
+                )
+                .map_err(|error| error.to_string())?;
+            }
         }
+
+        Ok(())
     }
 }
 

crates/sp-lightclient/src/lib.rs

@@ -40,7 +40,9 @@ use subspace_core_primitives::{
     SegmentCommitment, SegmentIndex, SolutionRange,
 };
 use subspace_solving::REWARD_SIGNING_CONTEXT;
-use subspace_verification::{check_reward_signature, PieceCheckParams, VerifySolutionParams};
+use subspace_verification::{
+    calculate_block_weight, check_reward_signature, PieceCheckParams, VerifySolutionParams,
+};
 
 #[cfg(test)]
 mod tests;
@@ -359,7 +361,7 @@ impl<Header: HeaderT, Store: Storage<Header>> HeaderImporter<Header, Store> {
             parent_header.header.hash(),
         )?;
 
-        let solution_distance = verify_solution(
+        verify_solution(
             (&header_digests.pre_digest.solution).into(),
             header_digests.pre_digest.slot.into(),
             (&VerifySolutionParams {
@@ -374,25 +376,13 @@ impl<Header: HeaderT, Store: Storage<Header>> HeaderImporter<Header, Store> {
         )
         .map_err(ImportError::InvalidSolution)?;
 
-        let total_weight =
-            parent_header.total_weight + BlockWeight::from(SolutionRange::MAX - solution_distance);
+        let added_weight = calculate_block_weight(header_digests.solution_range);
+        let total_weight = parent_header.total_weight + added_weight;
 
         // last best header should ideally be parent header. if not check for forks and pick the best chain
         let last_best_header = self.store.best_header();
-        let is_best_header = if last_best_header.header.hash() == parent_header.header.hash() {
-            // header is extending the current best header. consider this best header
-            true
-        } else {
-            let last_best_weight = last_best_header.total_weight;
-            match total_weight.cmp(&last_best_weight) {
-                // current weight is greater than last best. pick this header as best
-                Ordering::Greater => true,
-                // if weights are equal, pick the longest chain
-                Ordering::Equal => header.number() > last_best_header.header.number(),
-                // we already are on the best chain
-                Ordering::Less => false,
-            }
-        };
+        let last_best_weight = last_best_header.total_weight;
+        let is_best_header = total_weight > last_best_weight;
 
         // check if era has changed
         let era_start_slot = if Self::has_era_changed(&header, constants.era_duration) {

crates/sp-lightclient/src/tests.rs

@@ -216,8 +216,7 @@ fn valid_header(
         )
         .unwrap();
         let solution_range = solution_distance * 2;
-        let block_weight =
-            calculate_block_weight::<PosTable, _, _>(&solution, slot, &global_randomness).unwrap();
+        let block_weight = calculate_block_weight(solution_range);
 
         let pre_digest = PreDigest {
             slot: slot.into(),
@@ -618,8 +617,9 @@ fn test_header_import_non_canonical_with_equal_block_weight() {
     });
 }
 
+// TODO: This test doesn't actually reorg, but probably should
 #[test]
-fn test_chain_reorg_to_longer_chain() {
+fn test_chain_reorg_to_heavier_chain() {
     new_test_ext().execute_with(|| {
         let keypair = Keypair::generate();
         let farmer = FarmerParameters::new();

crates/subspace-verification/src/lib.rs

@@ -89,7 +89,7 @@ pub fn check_reward_signature(
 
 /// Calculates solution distance for given parameters, is used as a primitive to check whether
 /// solution distance is within solution range (see [`is_within_solution_range()`]).
-pub fn calculate_solution_distance(
+fn calculate_solution_distance(
     global_challenge: &Blake2b256Hash,
     audit_chunk: SolutionRange,
     sector_slot_challenge: &SectorSlotChallenge,
@@ -145,53 +145,9 @@ pub struct VerifySolutionParams {
     pub piece_check_params: Option<PieceCheckParams>,
 }
 
-/// Calculate weight derived from provided solution, used during block production
-pub fn calculate_block_weight<'a, PosTable, FarmerPublicKey, RewardAddress>(
-    solution: &'a Solution<FarmerPublicKey, RewardAddress>,
-    slot: SlotNumber,
-    global_randomness: &Randomness,
-) -> Result<BlockWeight, Error>
-where
-    PosTable: Table,
-    PublicKey: From<&'a FarmerPublicKey>,
-{
-    let sector_id = SectorId::new(
-        PublicKey::from(&solution.public_key).hash(),
-        solution.sector_index,
-    );
-
-    let global_challenge = global_randomness.derive_global_challenge(slot);
-    let sector_slot_challenge = sector_id.derive_sector_slot_challenge(&global_challenge);
-    let s_bucket_audit_index = sector_slot_challenge.s_bucket_audit_index();
-
-    // Check that proof of space is valid
-    let quality = match PosTable::is_proof_valid(
-        &sector_id.evaluation_seed(solution.piece_offset, solution.history_size),
-        s_bucket_audit_index.into(),
-        &solution.proof_of_space,
-    ) {
-        Some(quality) => quality,
-        None => {
-            return Err(Error::InvalidProofOfSpace);
-        }
-    };
-
-    let masked_chunk = (Simd::from(solution.chunk.to_bytes()) ^ Simd::from(*quality)).to_array();
-    // Extract audit chunk from masked chunk
-    let audit_chunk = match masked_chunk
-        .array_chunks::<{ mem::size_of::<SolutionRange>() }>()
-        .nth(usize::from(solution.audit_chunk_offset))
-    {
-        Some(audit_chunk) => SolutionRange::from_le_bytes(*audit_chunk),
-        None => {
-            return Err(Error::InvalidAuditChunkOffset);
-        }
-    };
-
-    Ok(BlockWeight::from(
-        SolutionRange::MAX
-            - calculate_solution_distance(&global_challenge, audit_chunk, &sector_slot_challenge),
-    ))
+/// Calculate weight derived from provided solution range
+pub fn calculate_block_weight(solution_range: SolutionRange) -> BlockWeight {
+    BlockWeight::from(SolutionRange::MAX - solution_range)
 }
 
 /// Verify whether solution is valid, returns solution distance that is `<= solution_range/2` on