Unverified Commit 7f813f52 authored by Robert Klotzner's avatar Robert Klotzner Committed by GitHub
Browse files

Fixes/improvements for disputes (#3753)

* More debugging output.

* Fix chain selection in case of disputes.

* Fix flaky test.
parent ae6ee12c
Pipeline #155143 failed with stages
in 28 minutes and 4 seconds
......@@ -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);
},
......
......@@ -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<BlockDescription>,
) -> Result<Option<(BlockNumber, Hash)>, 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))
}
}
}
......
......@@ -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;
......
......@@ -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 {
......
......@@ -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);
......
......@@ -383,18 +383,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();
});
}
......
......@@ -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<BlockDescription>,
/// A response channel - `None` to vote on base, `Some` to vote higher.
tx: oneshot::Sender<Option<(BlockNumber, Hash)>>,
/// The block to vote on, might be base in case there is no better.
tx: oneshot::Sender<(BlockNumber, Hash)>,
},
}
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment