diff --git a/node/core/dispute-coordinator/src/dummy.rs b/node/core/dispute-coordinator/src/dummy.rs index 5b0ca15f78d4..fe3f50f81e8e 100644 --- a/node/core/dispute-coordinator/src/dummy.rs +++ b/node/core/dispute-coordinator/src/dummy.rs @@ -233,13 +233,14 @@ async fn handle_incoming( }, DisputeCoordinatorMessage::IssueLocalStatement(_, _, _, _) => {}, DisputeCoordinatorMessage::DetermineUndisputedChain { - base_number, + base: (base_number, base_hash), block_descriptions, tx, } => { let undisputed_chain = block_descriptions .last() - .map(|e| (base_number + block_descriptions.len() as BlockNumber, e.block_hash)); + .map(|e| (base_number + block_descriptions.len() as BlockNumber, e.block_hash)) + .unwrap_or((base_number, base_hash)); let _ = tx.send(undisputed_chain); }, diff --git a/node/core/dispute-coordinator/src/real/mod.rs b/node/core/dispute-coordinator/src/real/mod.rs index 3c2c4e6b788b..53f71697798c 100644 --- a/node/core/dispute-coordinator/src/real/mod.rs +++ b/node/core/dispute-coordinator/src/real/mod.rs @@ -582,12 +582,12 @@ async fn handle_incoming( .await?; }, DisputeCoordinatorMessage::DetermineUndisputedChain { - base_number, + base: (base_number, base_hash), block_descriptions, tx, } => { let undisputed_chain = - determine_undisputed_chain(overlay_db, base_number, block_descriptions)?; + determine_undisputed_chain(overlay_db, base_number, base_hash, block_descriptions)?; let _ = tx.send(undisputed_chain); }, @@ -882,7 +882,7 @@ async fn issue_local_statement( // Do import if !statements.is_empty() { - let (pending_confirmation, _rx) = oneshot::channel(); + let (pending_confirmation, rx) = oneshot::channel(); handle_import_statements( ctx, overlay_db, @@ -895,6 +895,32 @@ async fn issue_local_statement( pending_confirmation, ) .await?; + match rx.await { + Err(_) => { + tracing::error!( + target: LOG_TARGET, + ?candidate_hash, + ?session, + "pending confirmation receiver got dropped by `handle_import_statements` for our own votes!" + ); + }, + Ok(ImportStatementsResult::InvalidImport) => { + tracing::error!( + target: LOG_TARGET, + ?candidate_hash, + ?session, + "handle_import_statements` considers our own votes invalid!" + ); + }, + Ok(ImportStatementsResult::ValidImport) => { + tracing::trace!( + target: LOG_TARGET, + ?candidate_hash, + ?session, + "handle_import_statements` successfully imported our vote!" + ); + }, + } } Ok(()) @@ -970,11 +996,13 @@ fn make_dispute_message( fn determine_undisputed_chain( overlay_db: &mut OverlayedBackend<'_, impl Backend>, base_number: BlockNumber, + base_hash: Hash, block_descriptions: Vec, -) -> Result, Error> { +) -> Result<(BlockNumber, Hash), Error> { let last = block_descriptions .last() - .map(|e| (base_number + block_descriptions.len() as BlockNumber, e.block_hash)); + .map(|e| (base_number + block_descriptions.len() as BlockNumber, e.block_hash)) + .unwrap_or((base_number, base_hash)); // Fast path for no disputes. let recent_disputes = match overlay_db.load_recent_disputes()? { @@ -992,12 +1020,9 @@ fn determine_undisputed_chain( for (i, BlockDescription { session, candidates, .. }) in block_descriptions.iter().enumerate() { if candidates.iter().any(|c| is_possibly_invalid(*session, *c)) { if i == 0 { - return Ok(None) + return Ok((base_number, base_hash)) } else { - return Ok(Some(( - base_number + i as BlockNumber, - block_descriptions[i - 1].block_hash, - ))) + return Ok((base_number + i as BlockNumber, block_descriptions[i - 1].block_hash)) } } } diff --git a/node/core/dispute-coordinator/src/real/tests.rs b/node/core/dispute-coordinator/src/real/tests.rs index 08f4a8c7e47b..60d7ade2fd79 100644 --- a/node/core/dispute-coordinator/src/real/tests.rs +++ b/node/core/dispute-coordinator/src/real/tests.rs @@ -645,13 +645,14 @@ fn finality_votes_ignore_disputed_candidates() { { let (tx, rx) = oneshot::channel(); + let base_block = Hash::repeat_byte(0x0f); let block_hash_a = Hash::repeat_byte(0x0a); let block_hash_b = Hash::repeat_byte(0x0b); virtual_overseer .send(FromOverseer::Communication { msg: DisputeCoordinatorMessage::DetermineUndisputedChain { - base_number: 10, + base: (10, base_block), block_descriptions: vec![BlockDescription { block_hash: block_hash_a, session, @@ -662,13 +663,13 @@ fn finality_votes_ignore_disputed_candidates() { }) .await; - assert!(rx.await.unwrap().is_none()); + assert_eq!(rx.await.unwrap(), (10, base_block)); let (tx, rx) = oneshot::channel(); virtual_overseer .send(FromOverseer::Communication { msg: DisputeCoordinatorMessage::DetermineUndisputedChain { - base_number: 10, + base: (10, base_block), block_descriptions: vec![ BlockDescription { block_hash: block_hash_a, @@ -686,7 +687,7 @@ fn finality_votes_ignore_disputed_candidates() { }) .await; - assert_eq!(rx.await.unwrap(), Some((11, block_hash_a))); + assert_eq!(rx.await.unwrap(), (11, block_hash_a)); } virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await; @@ -777,13 +778,14 @@ fn supermajority_valid_dispute_may_be_finalized() { { let (tx, rx) = oneshot::channel(); + let base_hash = Hash::repeat_byte(0x0f); let block_hash_a = Hash::repeat_byte(0x0a); let block_hash_b = Hash::repeat_byte(0x0b); virtual_overseer .send(FromOverseer::Communication { msg: DisputeCoordinatorMessage::DetermineUndisputedChain { - base_number: 10, + base: (10, base_hash), block_descriptions: vec![BlockDescription { block_hash: block_hash_a, session, @@ -794,13 +796,13 @@ fn supermajority_valid_dispute_may_be_finalized() { }) .await; - assert_eq!(rx.await.unwrap(), Some((11, block_hash_a))); + assert_eq!(rx.await.unwrap(), (11, block_hash_a)); let (tx, rx) = oneshot::channel(); virtual_overseer .send(FromOverseer::Communication { msg: DisputeCoordinatorMessage::DetermineUndisputedChain { - base_number: 10, + base: (10, base_hash), block_descriptions: vec![ BlockDescription { block_hash: block_hash_a, @@ -818,7 +820,7 @@ fn supermajority_valid_dispute_may_be_finalized() { }) .await; - assert_eq!(rx.await.unwrap(), Some((12, block_hash_b))); + assert_eq!(rx.await.unwrap(), (12, block_hash_b)); } virtual_overseer.send(FromOverseer::Signal(OverseerSignal::Conclude)).await; diff --git a/node/network/availability-recovery/src/lib.rs b/node/network/availability-recovery/src/lib.rs index 1e1f6af1132d..1860f9a28ae3 100644 --- a/node/network/availability-recovery/src/lib.rs +++ b/node/network/availability-recovery/src/lib.rs @@ -93,7 +93,7 @@ const COST_INVALID_REQUEST: Rep = Rep::CostMajor("Peer sent unparsable request") #[cfg(not(test))] const TIMEOUT_START_NEW_REQUESTS: Duration = CHUNK_REQUEST_TIMEOUT; #[cfg(test)] -const TIMEOUT_START_NEW_REQUESTS: Duration = Duration::from_millis(4); +const TIMEOUT_START_NEW_REQUESTS: Duration = Duration::from_millis(10); /// The Availability Recovery Subsystem. pub struct AvailabilityRecoverySubsystem { diff --git a/node/service/src/relay_chain_selection.rs b/node/service/src/relay_chain_selection.rs index bd441288bade..184d526eac47 100644 --- a/node/service/src/relay_chain_selection.rs +++ b/node/service/src/relay_chain_selection.rs @@ -434,7 +434,7 @@ where overseer .send_msg( DisputeCoordinatorMessage::DetermineUndisputedChain { - base_number: target_number, + base: (target_number, target_hash), block_descriptions: subchain_block_descriptions, tx, }, @@ -444,8 +444,7 @@ where let (subchain_number, subchain_head) = rx .await .map_err(Error::OverseerDisconnected) - .map_err(|e| ConsensusError::Other(Box::new(e)))? - .unwrap_or_else(|| (subchain_number, subchain_head)); + .map_err(|e| ConsensusError::Other(Box::new(e)))?; // The the total lag accounting for disputes. let lag_disputes = initial_leaf_number.saturating_sub(subchain_number); diff --git a/node/service/src/tests.rs b/node/service/src/tests.rs index 269a7f2da055..fb2cb346537e 100644 --- a/node/service/src/tests.rs +++ b/node/service/src/tests.rs @@ -398,18 +398,20 @@ async fn test_skeleton( ); tracing::trace!("determine undisputed chain response: {:?}", undisputed_chain); + + let target_block_number = chain.number(target_block_hash).unwrap().unwrap(); assert_matches!( overseer_recv( virtual_overseer ).await, AllMessages::DisputeCoordinator( DisputeCoordinatorMessage::DetermineUndisputedChain { - base_number: _, + base: _, block_descriptions: _, tx, } ) => { - tx.send(undisputed_chain).unwrap(); + tx.send(undisputed_chain.unwrap_or((target_block_number, target_block_hash))).unwrap(); }); } diff --git a/node/subsystem-types/src/messages.rs b/node/subsystem-types/src/messages.rs index 147074fa1b1d..691289614d14 100644 --- a/node/subsystem-types/src/messages.rs +++ b/node/subsystem-types/src/messages.rs @@ -189,7 +189,7 @@ impl BoundToRelayParent for CollatorProtocolMessage { /// Messages received by the dispute coordinator subsystem. #[derive(Debug)] pub enum DisputeCoordinatorMessage { - /// Import a statement by a validator about a candidate. + /// Import statements by validators about a candidate. /// /// The subsystem will silently discard ancient statements or sets of only dispute-specific statements for /// candidates that are previously unknown to the subsystem. The former is simply because ancient @@ -251,12 +251,12 @@ pub enum DisputeCoordinatorMessage { /// is typically the number of the last finalized block but may be slightly higher. This block /// is inevitably going to be finalized so it is not accounted for by this function. DetermineUndisputedChain { - /// The number of the lowest possible block to vote on. - base_number: BlockNumber, + /// The lowest possible block to vote on. + base: (BlockNumber, Hash), /// Descriptions of all the blocks counting upwards from the block after the base number block_descriptions: Vec, - /// A response channel - `None` to vote on base, `Some` to vote higher. - tx: oneshot::Sender>, + /// The block to vote on, might be base in case there is no better. + tx: oneshot::Sender<(BlockNumber, Hash)>, }, }