From 9e07969d44a4a077be2c16cdeea7abef9b855a24 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Wed, 7 Nov 2018 09:34:30 +0300 Subject: [PATCH 1/2] always ask for witness when requesting b/tx on core chain --- network/src/consensus.rs | 8 ++++++++ sync/src/lib.rs | 2 +- sync/src/synchronization_chain.rs | 23 +++++++---------------- sync/src/synchronization_client_core.rs | 21 +++++---------------- 4 files changed, 21 insertions(+), 33 deletions(-) diff --git a/network/src/consensus.rs b/network/src/consensus.rs index bfc460ec..0811d559 100644 --- a/network/src/consensus.rs +++ b/network/src/consensus.rs @@ -170,6 +170,14 @@ impl ConsensusFork { 4 } + /// Returns true if SegWit is possible on this chain. + pub fn is_segwit_possible(&self) -> bool { + match *self { + ConsensusFork::BitcoinCore => true, + ConsensusFork::BitcoinCash(_) => false, + } + } + pub fn activation_height(&self) -> u32 { match *self { ConsensusFork::BitcoinCore => 0, diff --git a/sync/src/lib.rs b/sync/src/lib.rs index f41c10c5..76dc3f5d 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 f9639262..dca616a0 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, - /// 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.fork.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 62282458..58a7879b 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,8 +226,7 @@ impl ClientCore for SynchronizationClientCore 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 { @@ -258,7 +257,7 @@ impl ClientCore for SynchronizationClientCore 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 +972,8 @@ impl SynchronizationClientCore 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 = 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 +1072,6 @@ impl SynchronizationClientCore 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 +1092,6 @@ impl SynchronizationClientCore 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 { From 91208b4c885bb4f37b146010de02fecd9667f041 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Wed, 7 Nov 2018 10:46:03 +0300 Subject: [PATCH 2/2] tests fixed --- message/src/common/inventory.rs | 14 +++++++++ network/src/consensus.rs | 17 +++++----- sync/src/synchronization_chain.rs | 2 +- sync/src/synchronization_client_core.rs | 41 +++++++++++++------------ 4 files changed, 45 insertions(+), 29 deletions(-) diff --git a/message/src/common/inventory.rs b/message/src/common/inventory.rs index 996ec41d..7e43a7b8 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 0811d559..7be6cd79 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 { @@ -170,14 +179,6 @@ impl ConsensusFork { 4 } - /// Returns true if SegWit is possible on this chain. - pub fn is_segwit_possible(&self) -> bool { - match *self { - ConsensusFork::BitcoinCore => true, - ConsensusFork::BitcoinCash(_) => false, - } - } - pub fn activation_height(&self) -> u32 { match *self { ConsensusFork::BitcoinCore => 0, diff --git a/sync/src/synchronization_chain.rs b/sync/src/synchronization_chain.rs index dca616a0..18d5c12c 100644 --- a/sync/src/synchronization_chain.rs +++ b/sync/src/synchronization_chain.rs @@ -148,7 +148,7 @@ 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 is_segwit_possible = consensus.fork.is_segwit_possible(); + let is_segwit_possible = consensus.is_segwit_possible(); Chain { genesis_block_hash: genesis_block_hash, diff --git a/sync/src/synchronization_client_core.rs b/sync/src/synchronization_client_core.rs index 58a7879b..453b619f 100644 --- a/sync/src/synchronization_client_core.rs +++ b/sync/src/synchronization_client_core.rs @@ -231,10 +231,11 @@ impl ClientCore for SynchronizationClientCore where T: TaskExecutor { .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 => { @@ -245,8 +246,8 @@ impl ClientCore for SynchronizationClientCore 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())); @@ -1338,7 +1339,7 @@ pub mod tests { fn request_blocks(peer_index: PeerIndex, hashes: Vec) -> Task { Task::GetData(peer_index, types::GetData { - inventory: hashes.into_iter().map(InventoryVector::block).collect(), + inventory: hashes.into_iter().map(InventoryVector::witness_block).collect(), }) } @@ -1738,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()) ]))]); } @@ -1872,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(); @@ -1885,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)) ]))]); } @@ -1915,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)) ]))]); }