Document how fork choice actually works

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

@@ -3,7 +3,7 @@
 use parity_scale_codec::{Decode, Encode};
 use sc_client_api::backend::AuxStore;
 use sp_blockchain::{Error as ClientError, Result as ClientResult};
-use subspace_core_primitives::BlockWeight;
+use subspace_core_primitives::BlockForkWeight;
 
 fn load_decode<B, T>(backend: &B, key: &[u8]) -> ClientResult<Option<T>>
 where
@@ -28,7 +28,7 @@ fn block_weight_key<H: Encode>(block_hash: H) -> Vec<u8> {
 /// Write the cumulative chain-weight of a block to aux storage.
 pub(crate) fn write_block_weight<H, F, R>(
     block_hash: H,
-    block_weight: BlockWeight,
+    block_weight: BlockForkWeight,
     write_aux: F,
 ) -> R
 where
@@ -43,6 +43,6 @@ where
 pub(crate) fn load_block_weight<H: Encode, B: AuxStore>(
     backend: &B,
     block_hash: H,
-) -> ClientResult<Option<BlockWeight>> {
+) -> ClientResult<Option<BlockForkWeight>> {
     load_decode(backend, block_weight_key(block_hash).as_slice())
 }

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

@@ -44,7 +44,7 @@ use subspace_core_primitives::segments::{HistorySize, SegmentHeader, SegmentInde
 use subspace_core_primitives::solutions::SolutionRange;
 use subspace_core_primitives::{BlockNumber, PublicKey};
 use subspace_proof_of_space::Table;
-use subspace_verification::{PieceCheckParams, VerifySolutionParams, calculate_block_weight};
+use subspace_verification::{PieceCheckParams, VerifySolutionParams, calculate_block_fork_weight};
 use tracing::warn;
 
 /// Notification with number of the block that is about to be imported and acknowledgement sender
@@ -601,7 +601,7 @@ where
         let block_hash = block.post_hash();
         let block_number = *block.header.number();
 
-        // Early exit if block already in chain
+        // Early exit if the block is already in the chain, and never treat it as the best block.
         match self.client.status(block_hash)? {
             sp_blockchain::BlockStatus::InChain => {
                 block.fork_choice = Some(ForkChoiceStrategy::Custom(false));
@@ -637,16 +637,27 @@ where
             .await?;
         }
 
+        // Find the solution range weight of the chain with the parent block at its tip.
         let parent_weight = if block_number.is_one() {
+            // The genesis block is given a zero fork weight.
             0
         } else {
-            // Parent block weight might be missing in special sync modes where block is imported in
-            // the middle of the blockchain history directly
+            // Parent block fork weight might be missing in special sync modes where the block is
+            // imported in the middle of the blockchain history directly. For forks off the same
+            // parent, this doesn't change the comparison outcome.
+            //
+            // For forks off different parents, this only changes the outcome if the fork is over an
+            // era transition. (Solution ranges are fixed within the same era.) In this case, the
+            // rest of the connected nodes will quickly converge, because they have weights starting
+            // further back. This convergence will overwhelm the inconsistent fork choices of any
+            // nodes that are currently snap syncing.
             aux_schema::load_block_weight(self.client.as_ref(), block.header.parent_hash())?
                 .unwrap_or_default()
         };
 
-        let added_weight = calculate_block_weight(subspace_digest_items.solution_range);
+        // We prioritise narrower (numerically smaller) solution ranges, using an inverse
+        // calculation.
+        let added_weight = calculate_block_fork_weight(subspace_digest_items.solution_range);
         let total_weight = parent_weight.saturating_add(added_weight);
 
         aux_schema::write_block_weight(block_hash, total_weight, |values| {
@@ -672,18 +683,39 @@ 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
+        // The fork choice rule is the largest fork solution range weight.
+        //
+        // This almost always prioritises:
+        // - the longest chain (the largest number of solutions), and if there is a tie
+        // - the strictest solutions (the numerically smallest solution ranges).
+        //
+        // If these totals are equal:
+        // - each node keeps the block it already chose (the one that it processed first).
+        //
+        // If there is no previous best block, or the old best block is missing a weight, or has a
+        // zero weight:
+        // - the new block is chosen as the best block, as long as it has a non-zero weight.
+        // (The only blocks with zero weights are in the test runtime.)
+        //
+        // Solution ranges only change at the end of each era, where different block times can make
+        // the range in each fork different. This can lead to some edge cases:
+        // - one fork accepts a solution as within its range, but another with a narrower range
+        //   does not, or
+        // - a fork with a narrower range outweighs a fork with a wider range, leading to a reorg
+        //   to a fork with fewer blocks.
+        //
+        // But these will be resolved with very high probability after a few blocks, assuming the
+        // network is well-connected.
         let fork_choice = {
             let info = self.client.info();
 
             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
+                // The "parent is genesis" case is already covered when loading parent weight, so we don't
+                // need to cover it again here.
                 parent_weight
             } else {
-                // Best block weight might be missing in special sync modes where block is imported
-                // in the middle of the blockchain history right after genesis
+                // The best block weight might be missing in special sync modes where the block is
+                // imported in the middle of the blockchain history, right after importing genesis.
                 aux_schema::load_block_weight(&*self.client, info.best_hash)?.unwrap_or_default()
             };
 

crates/subspace-core-primitives/src/lib.rs

@@ -128,10 +128,10 @@ pub type BlockHash = [u8; 32];
 /// Slot number in Subspace network.
 pub type SlotNumber = u64;
 
-/// BlockWeight type for fork choice rules.
+/// BlockForkWeight type for fork choice rules.
 ///
-/// The closer solution's tag is to the target, the heavier it is.
-pub type BlockWeight = u128;
+/// The narrower the solution range, the heavier the block is.
+pub type BlockForkWeight = u128;
 
 /// A Ristretto Schnorr public key as bytes produced by `schnorrkel` crate.
 #[derive(

crates/subspace-verification/Cargo.toml

@@ -33,3 +33,5 @@ std = [
     "subspace-kzg?/std",
     "thiserror/std"
 ]
+# Activate test APIs and workarounds
+testing = []

crates/subspace-verification/src/lib.rs

@@ -39,7 +39,7 @@ use subspace_core_primitives::segments::{HistorySize, SegmentCommitment};
 #[cfg(feature = "kzg")]
 use subspace_core_primitives::solutions::Solution;
 use subspace_core_primitives::solutions::{RewardSignature, SolutionRange};
-use subspace_core_primitives::{BlockNumber, BlockWeight, PublicKey, ScalarBytes, SlotNumber};
+use subspace_core_primitives::{BlockForkWeight, BlockNumber, PublicKey, ScalarBytes, SlotNumber};
 #[cfg(feature = "kzg")]
 use subspace_kzg::{Commitment, Kzg, Scalar, Witness};
 #[cfg(feature = "kzg")]
@@ -186,9 +186,19 @@ pub struct VerifySolutionParams {
     pub piece_check_params: Option<PieceCheckParams>,
 }
 
-/// Calculate weight derived from provided solution range
-pub fn calculate_block_weight(solution_range: SolutionRange) -> BlockWeight {
-    BlockWeight::from(SolutionRange::MAX - solution_range)
+/// Calculate the block's contribution to the fork weight, which is derived from the provided
+/// solution range.
+pub fn calculate_block_fork_weight(solution_range: SolutionRange) -> BlockForkWeight {
+    // Work around the test runtime accepting all solutions, which causes blocks to have zero
+    // fork weight. This makes each node keep its own blocks, and never reorg to a common chain.
+    #[cfg(feature = "testing")]
+    let solution_range = if solution_range == SolutionRange::MAX {
+        SolutionRange::MAX - 1
+    } else {
+        solution_range
+    };
+
+    BlockForkWeight::from(SolutionRange::MAX - solution_range)
 }
 
 /// Verify whether solution is valid, returns solution distance that is `<= solution_range/2` on

test/subspace-test-runtime/Cargo.toml

@@ -16,11 +16,11 @@ include = [
 targets = ["x86_64-unknown-linux-gnu"]
 
 [dependencies]
-parity-scale-codec = { workspace = true, features = ["derive"] }
 domain-runtime-primitives.workspace = true
 frame-executive.workspace = true
 frame-support.workspace = true
 frame-system.workspace = true
+frame-system-rpc-runtime-api.workspace = true
 pallet-balances.workspace = true
 pallet-domains.workspace = true
 pallet-messenger.workspace = true
@@ -34,8 +34,10 @@ pallet-sudo.workspace = true
 pallet-timestamp.workspace = true
 pallet-transaction-fees.workspace = true
 pallet-transaction-payment.workspace = true
+pallet-transaction-payment-rpc-runtime-api.workspace = true
 pallet-transporter.workspace = true
 pallet-utility.workspace = true
+parity-scale-codec = { workspace = true, features = ["derive"] }
 scale-info = { workspace = true, features = ["derive"] }
 sp-api.workspace = true
 sp-block-builder.workspace = true
@@ -61,16 +63,15 @@ static_assertions.workspace = true
 subspace-core-primitives.workspace = true
 subspace-runtime-primitives.workspace = true
 subspace-test-primitives.workspace = true
-frame-system-rpc-runtime-api.workspace = true
-pallet-transaction-payment-rpc-runtime-api.workspace = true
+# Activate a testing workaround for the fork choice rule
+subspace-verification = { workspace = true, features = ["testing"] }
 
 [build-dependencies]
 substrate-wasm-builder = { workspace = true, optional = true }
 
 [features]
 default = ["std"]
 std = [
-    "parity-scale-codec/std",
     "domain-runtime-primitives/std",
     "frame-executive/std",
     "frame-support/std",
@@ -88,10 +89,11 @@ std = [
     "pallet-sudo/std",
     "pallet-timestamp/std",
     "pallet-transaction-fees/std",
-    "pallet-transaction-payment-rpc-runtime-api/std",
     "pallet-transaction-payment/std",
+    "pallet-transaction-payment-rpc-runtime-api/std",
     "pallet-transporter/std",
     "pallet-utility/std",
+    "parity-scale-codec/std",
     "scale-info/std",
     "sp-api/std",
     "sp-block-builder/std",
@@ -112,7 +114,6 @@ std = [
     "sp-std/std",
     "sp-subspace-mmr/std",
     "sp-transaction-pool/std",
-    "sp-subspace-mmr/std",
     "sp-version/std",
     "subspace-core-primitives/std",
     "subspace-runtime-primitives/std",