Repository: paritytech/parity-bitcoin (language: rust)

Issue:
SegWit bug stalls syncing
I tried to sync this implementation with the --btc and --db-cache=24000 parameters.

Everything was going fine until I got to the SegWit activation block at height 481824. Then parity spit out:

> Block 0000000000000000001c8018d9cb3b742ef25114f27563e3fc4a1902167f9893 verification failed with error "WitnessInvalidNonceSize"

And every block after that resulted in: verification failed with error "Database(UnknownParent)" 

From what I can tell the above block is most certainly valid, so it looks like there's a consensus bug in this implementation.


Discussion / hints:

Fix patch:
diff --git a/message/src/common/inventory.rs b/message/src/common/inventory.rs
index 996ec41de..7e43a7b87 100644
--- a/message/src/common/inventory.rs
+++ b/message/src/common/inventory.rs
@@ -64,12 +64,26 @@ impl InventoryVector {
 		}
 	}
 
+	pub fn witness_tx(hash: H256) -> Self {
+		InventoryVector {
+			inv_type: InventoryType::MessageWitnessTx,
+			hash: hash,
+		}
+	}
+
 	pub fn block(hash: H256) -> Self {
 		InventoryVector {
 			inv_type: InventoryType::MessageBlock,
 			hash: hash,
 		}
 	}
+
+	pub fn witness_block(hash: H256) -> Self {
+		InventoryVector {
+			inv_type: InventoryType::MessageWitnessBlock,
+			hash: hash,
+		}
+	}
 }
 
 impl Serializable for InventoryVector {
diff --git a/network/src/consensus.rs b/network/src/consensus.rs
index bfc460ece..7be6cd798 100644
--- a/network/src/consensus.rs
+++ b/network/src/consensus.rs
@@ -152,6 +152,15 @@ impl ConsensusParams {
 		(height == 91842 && hash == &H256::from_reversed_str("00000000000a4d0a398161ffc163c503763b1f4360639393e0e4c8e300e0caec")) ||
 		(height == 91880 && hash == &H256::from_reversed_str("00000000000743f190a18c5577a3c2d2a1f610ae9601ac046a38084ccb7cd721"))
 	}
+
+	/// Returns true if SegWit is possible on this chain.
+	pub fn is_segwit_possible(&self) -> bool {
+		match self.fork {
+			// SegWit is not supported in (our?) regtests
+			ConsensusFork::BitcoinCore if self.network != Network::Regtest => true,
+			ConsensusFork::BitcoinCore | ConsensusFork::BitcoinCash(_) => false,
+		}
+	}
 }
 
 impl ConsensusFork {
diff --git a/sync/src/lib.rs b/sync/src/lib.rs
index f41c10c58..76dc3f5dd 100644
--- a/sync/src/lib.rs
+++ b/sync/src/lib.rs
@@ -110,7 +110,7 @@ pub fn create_local_sync_node(consensus: ConsensusParams, db: storage::SharedSto
 	let memory_pool = Arc::new(RwLock::new(MemoryPool::new()));
 	let sync_state = SynchronizationStateRef::new(SynchronizationState::with_storage(db.clone()));
 	let sync_chain = SyncChain::new(db.clone(), consensus.clone(), memory_pool.clone());
-	if sync_chain.is_segwit_active() {
+	if sync_chain.is_segwit_possible() {
 		peers.require_peer_services(Services::default().with_witness(true));
 	}
 
diff --git a/sync/src/synchronization_chain.rs b/sync/src/synchronization_chain.rs
index f96392621..18d5c12c7 100644
--- a/sync/src/synchronization_chain.rs
+++ b/sync/src/synchronization_chain.rs
@@ -9,7 +9,6 @@ use primitives::bytes::Bytes;
 use primitives::hash::H256;
 use utils::{BestHeadersChain, BestHeadersChainInformation, HashQueueChain, HashPosition};
 use types::{BlockHeight, StorageRef, MemoryPoolRef};
-use verification::Deployments;
 
 /// Index of 'verifying' queue
 const VERIFYING_QUEUE: usize = 0;
@@ -106,8 +105,6 @@ pub struct Chain {
 	best_storage_block: storage::BestBlock,
 	/// Local blocks storage
 	storage: StorageRef,
-	/// Consensus params.
-	consensus: ConsensusParams,
 	/// In-memory queue of blocks hashes
 	hash_chain: HashQueueChain,
 	/// In-memory queue of blocks headers
@@ -118,10 +115,9 @@ pub struct Chain {
 	memory_pool: MemoryPoolRef,
 	/// Blocks that have been marked as dead-ends
 	dead_end_blocks: HashSet<H256>,
-	/// Deployments cache
-	deployments: Deployments,
-	/// Is SegWit active?
-	is_segwit_active: bool,
+	/// Is SegWit is possible on this chain? SegWit inventory types are used when block/tx-es are
+	/// requested and this flag is true.
+	is_segwit_possible: bool,
 }
 
 impl BlockState {
@@ -152,21 +148,18 @@ impl Chain {
 			.expect("storage with genesis block is required");
 		let best_storage_block = storage.best_block();
 		let best_storage_block_hash = best_storage_block.hash.clone();
-		let deployments = Deployments::new();
-		let is_segwit_active = deployments.segwit(best_storage_block.number, storage.as_block_header_provider(), &consensus);
+		let is_segwit_possible = consensus.is_segwit_possible();
 
 		Chain {
 			genesis_block_hash: genesis_block_hash,
 			best_storage_block: best_storage_block,
 			storage: storage,
-			consensus: consensus,
 			hash_chain: HashQueueChain::with_number_of_queues(NUMBER_OF_QUEUES),
 			headers_chain: BestHeadersChain::new(best_storage_block_hash),
 			verifying_transactions: LinkedHashMap::new(),
 			memory_pool: memory_pool,
 			dead_end_blocks: HashSet::new(),
-			deployments: deployments,
-			is_segwit_active: is_segwit_active,
+			is_segwit_possible,
 		}
 	}
 
@@ -193,8 +186,8 @@ impl Chain {
 	}
 
 	/// Is segwit active
-	pub fn is_segwit_active(&self) -> bool {
-		self.is_segwit_active
+	pub fn is_segwit_possible(&self) -> bool {
+		self.is_segwit_possible
 	}
 
 	/// Get number of blocks in given state
@@ -367,7 +360,6 @@ impl Chain {
 
 				// remember new best block hash
 				self.best_storage_block = self.storage.as_store().best_block();
-				self.is_segwit_active = self.deployments.segwit(self.best_storage_block.number, self.storage.as_block_header_provider(), &self.consensus);
 
 				// remove inserted block + handle possible reorganization in headers chain
 				// TODO: mk, not sure if we need both of those params
@@ -403,7 +395,6 @@ impl Chain {
 
 				// remember new best block hash
 				self.best_storage_block = self.storage.best_block();
-				self.is_segwit_active = self.deployments.segwit(self.best_storage_block.number, self.storage.as_block_header_provider(), &self.consensus);
 
 				// remove inserted block + handle possible reorganization in headers chain
 				// TODO: mk, not sure if we need both of those params
diff --git a/sync/src/synchronization_client_core.rs b/sync/src/synchronization_client_core.rs
index 62282458c..453b619f6 100644
--- a/sync/src/synchronization_client_core.rs
+++ b/sync/src/synchronization_client_core.rs
@@ -6,7 +6,7 @@ use futures::Future;
 use parking_lot::Mutex;
 use time::precise_time_s;
 use chain::{IndexedBlockHeader, IndexedTransaction, Transaction, IndexedBlock};
-use message::{types, Services};
+use message::types;
 use message::common::{InventoryType, InventoryVector};
 use miner::transaction_fee_rate;
 use primitives::hash::H256;
@@ -226,16 +226,16 @@ impl<T> ClientCore for SynchronizationClientCore<T> where T: TaskExecutor {
 		}
 
 		// else ask for all unknown transactions and blocks
-		let is_segwit_active = self.chain.is_segwit_active();
-		let ask_for_witness = is_segwit_active && self.peers.is_segwit_enabled(peer_index);
+		let is_segwit_possible = self.chain.is_segwit_possible();
 		let unknown_inventory: Vec<_> = message.inventory.into_iter()
 			.filter(|item| {
 				match item.inv_type {
 					// check that transaction is unknown to us
-					InventoryType::MessageTx => self.chain.transaction_state(&item.hash) == TransactionState::Unknown
-						&& !self.orphaned_transactions_pool.contains(&item.hash),
+					InventoryType::MessageTx| InventoryType::MessageWitnessTx =>
+						self.chain.transaction_state(&item.hash) == TransactionState::Unknown
+							&& !self.orphaned_transactions_pool.contains(&item.hash),
 					// check that block is unknown to us
-					InventoryType::MessageBlock => match self.chain.block_state(&item.hash) {
+					InventoryType::MessageBlock | InventoryType::MessageWitnessBlock => match self.chain.block_state(&item.hash) {
 						BlockState::Unknown => !self.orphaned_blocks_pool.contains_unknown_block(&item.hash),
 						BlockState::DeadEnd if !self.config.close_connection_on_bad_block => true,
 						BlockState::DeadEnd if self.config.close_connection_on_bad_block => {
@@ -246,8 +246,8 @@ impl<T> ClientCore for SynchronizationClientCore<T> where T: TaskExecutor {
 					},
 					// we never ask for merkle blocks && we never ask for compact blocks
 					InventoryType::MessageCompactBlock | InventoryType::MessageFilteredBlock
-						| InventoryType::MessageWitnessBlock | InventoryType::MessageWitnessFilteredBlock
-						| InventoryType::MessageWitnessTx => false,
+						| InventoryType::MessageWitnessFilteredBlock
+						 => false,
 					// unknown inventory type
 					InventoryType::Error => {
 						self.peers.misbehaving(peer_index, &format!("Provided unknown inventory type {:?}", item.hash.to_reversed_str()));
@@ -258,7 +258,7 @@ impl<T> ClientCore for SynchronizationClientCore<T> where T: TaskExecutor {
 			// we are not synchronizing =>
 			// 1) either segwit is active and we are connected to segwit-enabled nodes => we could ask for witness
 			// 2) or segwit is inactive => we shall not ask for witness
-			.map(|item| if !ask_for_witness {
+			.map(|item| if !is_segwit_possible {
 					item
 				} else {
 					match item.inv_type {
@@ -973,8 +973,8 @@ impl<T> SynchronizationClientCore<T> where T: TaskExecutor {
 		let chunk_size = min(limits.max_blocks_in_request, max(hashes.len() as BlockHeight, limits.min_blocks_in_request));
 		let last_peer_index = peers.len() - 1;
 		let mut tasks: Vec<Task> = Vec::new();
-		let is_segwit_active = self.chain.is_segwit_active();
-		let inv_type = if is_segwit_active { InventoryType::MessageWitnessBlock } else { InventoryType::MessageBlock };
+		let is_segwit_possible = self.chain.is_segwit_possible();
+		let inv_type = if is_segwit_possible { InventoryType::MessageWitnessBlock } else { InventoryType::MessageBlock };
 		for (peer_index, peer) in peers.into_iter().enumerate() {
 			// we have to request all blocks => we will request last peer for all remaining blocks
 			let peer_chunk_size = if peer_index == last_peer_index { hashes.len() } else { min(hashes.len(), chunk_size as usize) };
@@ -1073,9 +1073,6 @@ impl<T> SynchronizationClientCore<T> where T: TaskExecutor {
 		// update block processing speed
 		self.block_speed_meter.checkpoint();
 
-		// remember if SegWit was active before this block
-		let segwit_was_active = self.chain.is_segwit_active();
-
 		// remove flags
 		let needs_relay = !self.do_not_relay.remove(block.hash());
 
@@ -1096,13 +1093,6 @@ impl<T> SynchronizationClientCore<T> where T: TaskExecutor {
 				// update shared state
 				self.shared_state.update_best_storage_block_height(self.chain.best_storage_block().number);
 
-				// if SegWit activated after this block insertion:
-				// 1) no more connections to !NODE_WITNESS nodes
-				// 2) disconnect from all nodes without NODE_WITNESS support
-				if !segwit_was_active && self.chain.is_segwit_active() {
-					self.peers.require_peer_services(Services::default().with_witness(true));
-				}
-
 				// notify listener
 				if let Some(best_block_hash) = insert_result.canonized_blocks_hashes.last() {
 					if let Some(ref listener) = self.listener {
@@ -1349,7 +1339,7 @@ pub mod tests {
 
 	fn request_blocks(peer_index: PeerIndex, hashes: Vec<H256>) -> Task {
 		Task::GetData(peer_index, types::GetData {
-			inventory: hashes.into_iter().map(InventoryVector::block).collect(),
+			inventory: hashes.into_iter().map(InventoryVector::witness_block).collect(),
 		})
 	}
 
@@ -1749,13 +1739,13 @@ pub mod tests {
 
 		sync.on_block(1, test_data::block_h2().into());
 		sync.on_inventory(1, types::Inv::with_inventory(vec![
-			InventoryVector::block(test_data::block_h1().hash()),
-			InventoryVector::block(test_data::block_h2().hash()),
+			InventoryVector::witness_block(test_data::block_h1().hash()),
+			InventoryVector::witness_block(test_data::block_h2().hash()),
 		]));
 
 		let tasks = executor.take_tasks();
 		assert_eq!(tasks, vec![Task::GetData(1, types::GetData::with_inventory(vec![
-			InventoryVector::block(test_data::block_h1().hash())
+			InventoryVector::witness_block(test_data::block_h1().hash())
 		]))]);
 	}
 
@@ -1883,11 +1873,11 @@ pub mod tests {
 	fn transaction_is_requested_when_not_synchronizing() {
 		let (executor, core, sync) = create_sync(None, None);
 
-		sync.on_inventory(0, types::Inv::with_inventory(vec![InventoryVector::tx(H256::from(0))]));
+		sync.on_inventory(0, types::Inv::with_inventory(vec![InventoryVector::witness_tx(H256::from(0))]));
 
 		{
 			let tasks = executor.take_tasks();
-			assert_eq!(tasks, vec![Task::GetData(0, types::GetData::with_inventory(vec![InventoryVector::tx(H256::from(0))]))]);
+			assert_eq!(tasks, vec![Task::GetData(0, types::GetData::with_inventory(vec![InventoryVector::witness_tx(H256::from(0))]))]);
 		}
 
 		let b1 = test_data::block_h1();
@@ -1896,28 +1886,28 @@ pub mod tests {
 		assert!(core.lock().information().state.is_nearly_saturated());
 		{ executor.take_tasks(); } // forget tasks
 
-		sync.on_inventory(0, types::Inv::with_inventory(vec![InventoryVector::tx(H256::from(1))]));
+		sync.on_inventory(0, types::Inv::with_inventory(vec![InventoryVector::witness_tx(H256::from(1))]));
 
 		let tasks = executor.take_tasks();
-		assert_eq!(tasks, vec![Task::GetData(0, types::GetData::with_inventory(vec![InventoryVector::tx(H256::from(1))]))]);
+		assert_eq!(tasks, vec![Task::GetData(0, types::GetData::with_inventory(vec![InventoryVector::witness_tx(H256::from(1))]))]);
 	}
 
 	#[test]
 	fn same_transaction_can_be_requested_twice() {
 		let (executor, _, sync) = create_sync(None, None);
 
-		sync.on_inventory(0, types::Inv::with_inventory(vec![InventoryVector::tx(H256::from(0))]));
+		sync.on_inventory(0, types::Inv::with_inventory(vec![InventoryVector::witness_tx(H256::from(0))]));
 
 		let tasks = executor.take_tasks();
 		assert_eq!(tasks, vec![Task::GetData(0, types::GetData::with_inventory(vec![
-			InventoryVector::tx(H256::from(0))
+			InventoryVector::witness_tx(H256::from(0))
 		]))]);
 
-		sync.on_inventory(0, types::Inv::with_inventory(vec![InventoryVector::tx(H256::from(0))]));
+		sync.on_inventory(0, types::Inv::with_inventory(vec![InventoryVector::witness_tx(H256::from(0))]));
 
 		let tasks = executor.take_tasks();
 		assert_eq!(tasks, vec![Task::GetData(0, types::GetData::with_inventory(vec![
-			InventoryVector::tx(H256::from(0))
+			InventoryVector::witness_tx(H256::from(0))
 		]))]);
 	}
 
@@ -1926,11 +1916,11 @@ pub mod tests {
 		let (executor, _, sync) = create_sync(None, None);
 
 		sync.on_inventory(0, types::Inv::with_inventory(vec![
-			InventoryVector::tx(test_data::genesis().transactions[0].hash()),
-			InventoryVector::tx(H256::from(0)),
+			InventoryVector::witness_tx(test_data::genesis().transactions[0].hash()),
+			InventoryVector::witness_tx(H256::from(0)),
 		]));
 		assert_eq!(executor.take_tasks(), vec![Task::GetData(0, types::GetData::with_inventory(vec![
-			InventoryVector::tx(H256::from(0))
+			InventoryVector::witness_tx(H256::from(0))
 		]))]);
 	}
 


Test patch: