From 0f0a5c0b4567175a5611c373cf8644442fbe6da4 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Tue, 31 Aug 2021 18:18:12 +0200 Subject: [PATCH 1/3] More debugging output. --- node/core/dispute-coordinator/src/real/mod.rs | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/node/core/dispute-coordinator/src/real/mod.rs b/node/core/dispute-coordinator/src/real/mod.rs index 3c2c4e6b788b..498a675bec84 100644 --- a/node/core/dispute-coordinator/src/real/mod.rs +++ b/node/core/dispute-coordinator/src/real/mod.rs @@ -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(()) From e8aae8bee3b50e421327e2e81d89a1c5fe57e27a Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Tue, 31 Aug 2021 18:56:01 +0200 Subject: [PATCH 2/3] Fix chain selection in case of disputes. --- node/core/dispute-coordinator/src/dummy.rs | 5 ++-- node/core/dispute-coordinator/src/real/mod.rs | 25 +++++++++---------- .../dispute-coordinator/src/real/tests.rs | 18 +++++++------ node/service/src/relay_chain_selection.rs | 5 ++-- node/service/src/tests.rs | 6 +++-- node/subsystem-types/src/messages.rs | 10 ++++---- 6 files changed, 36 insertions(+), 33 deletions(-) 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 498a675bec84..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); }, @@ -896,14 +896,14 @@ async fn issue_local_statement( ) .await?; match rx.await { - Err(_) => { + 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, @@ -911,7 +911,7 @@ async fn issue_local_statement( ?session, "handle_import_statements` considers our own votes invalid!" ); - } + }, Ok(ImportStatementsResult::ValidImport) => { tracing::trace!( target: LOG_TARGET, @@ -919,7 +919,7 @@ async fn issue_local_statement( ?session, "handle_import_statements` successfully imported our vote!" ); - } + }, } } @@ -996,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()? { @@ -1018,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/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)>, }, } From f4d19c7c58ea687536cc08cb3e2af2f201db1287 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 1 Sep 2021 12:43:38 +0200 Subject: [PATCH 3/3] Fix flaky test. --- node/network/availability-recovery/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 {