diff --git a/Cargo.lock b/Cargo.lock index ce506d67b3..2ff0e11465 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -14117,6 +14117,7 @@ dependencies = [ "subspace-core-primitives", "subspace-runtime-primitives", "subspace-test-primitives", + "subspace-verification", "substrate-wasm-builder", ] diff --git a/crates/sc-consensus-subspace/src/aux_schema.rs b/crates/sc-consensus-subspace/src/aux_schema.rs index 9835a39ffa..db2be65d18 100644 --- a/crates/sc-consensus-subspace/src/aux_schema.rs +++ b/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(backend: &B, key: &[u8]) -> ClientResult> where @@ -28,7 +28,7 @@ fn block_weight_key(block_hash: H) -> Vec { /// Write the cumulative chain-weight of a block to aux storage. pub(crate) fn write_block_weight( 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( backend: &B, block_hash: H, -) -> ClientResult> { +) -> ClientResult> { load_decode(backend, block_weight_key(block_hash).as_slice()) } diff --git a/crates/sc-consensus-subspace/src/block_import.rs b/crates/sc-consensus-subspace/src/block_import.rs index 54d3cf8b38..fd964ecfc0 100644 --- a/crates/sc-consensus-subspace/src/block_import.rs +++ b/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() }; diff --git a/crates/subspace-core-primitives/src/lib.rs b/crates/subspace-core-primitives/src/lib.rs index f36c96bfd8..d35e805ec1 100644 --- a/crates/subspace-core-primitives/src/lib.rs +++ b/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( diff --git a/crates/subspace-verification/Cargo.toml b/crates/subspace-verification/Cargo.toml index 48b77407c5..afffa7cf1b 100644 --- a/crates/subspace-verification/Cargo.toml +++ b/crates/subspace-verification/Cargo.toml @@ -33,3 +33,5 @@ std = [ "subspace-kzg?/std", "thiserror/std" ] +# Activate test APIs and workarounds +testing = [] diff --git a/crates/subspace-verification/src/lib.rs b/crates/subspace-verification/src/lib.rs index 13bdd6f075..23bec5b7d8 100644 --- a/crates/subspace-verification/src/lib.rs +++ b/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, } -/// 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 diff --git a/test/subspace-test-runtime/Cargo.toml b/test/subspace-test-runtime/Cargo.toml index 132f45c028..5be8a27eb2 100644 --- a/test/subspace-test-runtime/Cargo.toml +++ b/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,8 +63,8 @@ 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 } @@ -70,7 +72,6 @@ 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",