Unverified Commit 302d6582 authored by Bernhard Schuster's avatar Bernhard Schuster Committed by GitHub
Browse files

feat/view: assure heads in a view are sorted (#2493)



* feat/view: assure heads in a view are sorted

Allows O(n) comparisons, adds an alternate equiv relation
which takes O(n^2) for integrity verification.

Ref #2133

* revert: remove custom PartialEq impl, there are no duplicates

* fix: do not sort the live_heads, that alters the local view

* refactor/view: heads should not be public

* chore/spellcheck: add unfinalized

* fix/view: add missing len() and is_empty() fns

* quirk

* vec is not view

* Update node/network/approval-distribution/src/tests.rs
Co-authored-by: Andronik Ordian's avatarAndronik Ordian <write@reusable.software>

* Update node/network/bridge/src/lib.rs
Co-authored-by: Andronik Ordian's avatarAndronik Ordian <write@reusable.software>

* Update node/network/protocol/src/lib.rs
Co-authored-by: Andronik Ordian's avatarAndronik Ordian <write@reusable.software>

* fixup comment

* fix botched test
Co-authored-by: Andronik Ordian's avatarAndronik Ordian <write@reusable.software>
parent e638cdba
Pipeline #125339 canceled with stages
in 21 minutes and 12 seconds
......@@ -82,6 +82,7 @@ UDP
UI
union/MSG
unservable/B
unfinalize/B
validator/SM
w3f/MS
wasm/M
......
......@@ -186,11 +186,11 @@ impl State {
self.blocks_by_number.entry(meta.number).or_default().push(meta.hash);
}
for (peer_id, view) in self.peer_views.iter() {
let intersection = view.heads.iter().filter(|h| new_hashes.contains(h));
let view_intersection = View {
heads: intersection.cloned().collect(),
finalized_number: view.finalized_number,
};
let intersection = view.iter().filter(|h| new_hashes.contains(h));
let view_intersection = View::new(
intersection.cloned(),
view.finalized_number,
);
Self::unify_with_peer(
&mut self.blocks,
ctx,
......@@ -634,7 +634,7 @@ impl State {
let mut to_send = HashSet::new();
let view_finalized_number = view.finalized_number;
for head in view.heads.into_iter() {
for head in view.into_iter() {
let mut block = head;
let interesting_blocks = std::iter::from_fn(|| {
// step 2.
......
......@@ -330,7 +330,7 @@ fn spam_attack_results_in_negative_reputation_change() {
overseer_send(
overseer,
ApprovalDistributionMessage::NetworkBridgeUpdateV1(
NetworkBridgeEvent::PeerViewChange(peer.clone(), View { heads: Default::default(), finalized_number: 2 })
NetworkBridgeEvent::PeerViewChange(peer.clone(), View::with_finalized(2))
)
).await;
......@@ -666,7 +666,7 @@ fn update_peer_view() {
overseer_send(
overseer,
ApprovalDistributionMessage::NetworkBridgeUpdateV1(
NetworkBridgeEvent::PeerViewChange(peer.clone(), View { heads: vec![hash_b, hash_c, hash_d], finalized_number: 2 })
NetworkBridgeEvent::PeerViewChange(peer.clone(), View::new(vec![hash_b, hash_c, hash_d], 2))
)
).await;
......@@ -713,7 +713,7 @@ fn update_peer_view() {
overseer_send(
overseer,
ApprovalDistributionMessage::NetworkBridgeUpdateV1(
NetworkBridgeEvent::PeerViewChange(peer.clone(), View { heads: vec![], finalized_number })
NetworkBridgeEvent::PeerViewChange(peer.clone(), View::with_finalized(finalized_number))
)
).await;
});
......
......@@ -417,7 +417,7 @@ where
.filter(|(_peer, view)| {
// collect all direct interests of a peer w/o ancestors
state
.cached_live_candidates_unioned(view.heads.iter())
.cached_live_candidates_unioned(view.iter())
.contains(&candidate_hash)
})
.map(|(peer, _view)| peer.clone())
......@@ -623,7 +623,7 @@ where
let _timer = metrics.time_process_incoming_peer_message();
// obtain the set of candidates we are interested in based on our current view
let live_candidates = state.cached_live_candidates_unioned(state.view.heads.iter());
let live_candidates = state.cached_live_candidates_unioned(state.view.iter());
// check if the candidate is of interest
let candidate_hash = message.candidate_hash;
......@@ -764,7 +764,7 @@ where
// peers view must contain the candidate hash too
cached_live_candidates_unioned(
per_relay_parent,
view.heads.iter(),
view.iter(),
).contains(&message.candidate_hash)
})
.map(|(peer, _)| -> PeerId { peer.clone() })
......
......@@ -37,7 +37,7 @@ use maplit::hashmap;
macro_rules! view {
( $( $hash:expr ),* $(,)? ) => {
// Finalized number unimportant for availability distribution.
View { heads: vec![ $( $hash.clone() ),* ], finalized_number: 0 }
View::new(vec![ $( $hash.clone() ),* ], 0)
};
}
......
......@@ -835,7 +835,7 @@ mod test {
let validator = SyncCryptoStore::sr25519_generate_new(&*keystore, ValidatorId::ID, None)
.expect("generating sr25519 key not to fail");
state.per_relay_parent = view.heads.iter().map(|relay_parent| {(
state.per_relay_parent = view.iter().map(|relay_parent| {(
relay_parent.clone(),
PerRelayParentData {
signing_context: signing_context.clone(),
......
......@@ -409,10 +409,10 @@ where
}
fn construct_view(live_heads: impl DoubleEndedIterator<Item = Hash>, finalized_number: BlockNumber) -> View {
View {
heads: live_heads.rev().take(MAX_VIEW_HEADS).collect(),
View::new(
live_heads.rev().take(MAX_VIEW_HEADS),
finalized_number
}
)
}
#[tracing::instrument(level = "trace", skip(net, ctx, validation_peers, collation_peers), fields(subsystem = LOG_TARGET))]
......@@ -427,8 +427,9 @@ async fn update_our_view(
) -> SubsystemResult<()> {
let new_view = construct_view(live_heads.iter().map(|v| v.0), finalized_number);
// We only want to send a view update when the heads changed, not when only the finalized block changed.
if local_view.heads == new_view.heads {
// We only want to send a view update when the heads changed.
// A change in finalized block number only is _not_ sufficient.
if local_view.check_heads_eq(&new_view) {
return Ok(())
}
......@@ -477,7 +478,7 @@ async fn handle_peer_messages<M>(
for message in messages {
outgoing_messages.push(match message {
WireMessage::ViewUpdate(new_view) => {
if new_view.heads.len() > MAX_VIEW_HEADS ||
if new_view.len() > MAX_VIEW_HEADS ||
new_view.finalized_number < peer_data.view.finalized_number
{
net.report_peer(
......@@ -486,7 +487,7 @@ async fn handle_peer_messages<M>(
).await?;
continue
} else if new_view.heads.is_empty() {
} else if new_view.is_empty() {
net.report_peer(
peer.clone(),
EMPTY_VIEW_COST,
......@@ -996,7 +997,7 @@ mod tests {
let actions = network_handle.next_network_actions(4).await;
let wire_message = WireMessage::<protocol_v1::ValidationProtocol>::ViewUpdate(
View { heads: vec![hash_a], finalized_number: 5 }
View::new(vec![hash_a], 5)
).encode();
assert_network_actions_contains(
......@@ -1378,10 +1379,7 @@ mod tests {
let actions = network_handle.next_network_actions(2).await;
let wire_message = WireMessage::<protocol_v1::ValidationProtocol>::ViewUpdate(
View {
heads: vec![hash_b],
finalized_number: 1,
}
View::new(vec![hash_b], 1)
).encode();
assert_network_actions_contains(
......@@ -1412,7 +1410,7 @@ mod tests {
peer_a.clone(),
PeerSet::Validation,
WireMessage::<protocol_v1::ValidationProtocol>::ViewUpdate(
View { heads: vec![Hash::repeat_byte(0x01)], finalized_number: 1 },
View::new(vec![Hash::repeat_byte(0x01)], 1),
).encode(),
).await;
......@@ -1420,7 +1418,7 @@ mod tests {
peer_a.clone(),
PeerSet::Validation,
WireMessage::<protocol_v1::ValidationProtocol>::ViewUpdate(
View { heads: vec![], finalized_number: 0 },
View::new(vec![], 0),
).encode(),
).await;
......
......@@ -1342,7 +1342,7 @@ mod tests {
overseer_send(
virtual_overseer,
CollatorProtocolMessage::NetworkBridgeUpdateV1(
NetworkBridgeEvent::PeerViewChange(peer.clone(), View { heads: hashes, finalized_number: 0 }),
NetworkBridgeEvent::PeerViewChange(peer.clone(), View::new(hashes, 0)),
),
).await;
}
......
......@@ -684,7 +684,7 @@ async fn handle_network_update(
peer_state.awaited.retain(|relay_parent, _| view.contains(&relay_parent));
// introduce things from the new view.
for relay_parent in view.heads.iter() {
for relay_parent in view.iter() {
if let Entry::Vacant(entry) = peer_state.awaited.entry(*relay_parent) {
entry.insert(HashSet::new());
......
......@@ -129,12 +129,12 @@ impl OurView {
/// Creates a new instance.
pub fn new(heads: impl IntoIterator<Item = (Hash, Arc<jaeger::Span>)>, finalized_number: BlockNumber) -> Self {
let state_per_head = heads.into_iter().collect::<HashMap<_, _>>();
let view = View::new(
state_per_head.keys().cloned(),
finalized_number,
);
Self {
view: View {
heads: state_per_head.keys().cloned().collect(),
finalized_number,
},
view,
span_per_head: state_per_head,
}
}
......@@ -189,7 +189,8 @@ macro_rules! our_view {
#[derive(Default, Debug, Clone, PartialEq, Eq, Encode, Decode)]
pub struct View {
/// A bounded amount of chain heads.
pub heads: Vec<Hash>,
/// Invariant: Sorted.
heads: Vec<Hash>,
/// The highest known finalized block number.
pub finalized_number: BlockNumber,
}
......@@ -208,11 +209,50 @@ pub struct View {
#[macro_export]
macro_rules! view {
( $( $hash:expr ),* $(,)? ) => {
$crate::View { heads: vec![ $( $hash.clone() ),* ], finalized_number: 0 }
$crate::View::new(vec![ $( $hash.clone() ),* ], 0)
};
}
impl View {
/// Construct a new view based on heads and a finalized block number.
pub fn new(heads: impl IntoIterator<Item=Hash>, finalized_number: BlockNumber) -> Self
{
let mut heads = heads.into_iter().collect::<Vec<Hash>>();
heads.sort();
Self {
heads,
finalized_number,
}
}
/// Start with no heads, but only a finalized block number.
pub fn with_finalized(finalized_number: BlockNumber) -> Self {
Self {
heads: Vec::new(),
finalized_number,
}
}
/// Obtain the number of heads that are in view.
pub fn len(&self) -> usize {
self.heads.len()
}
/// Check if the number of heads contained, is null.
pub fn is_empty(&self) -> bool {
self.heads.is_empty()
}
/// Obtain an iterator over all heads.
pub fn iter<'a>(&'a self) -> impl Iterator<Item=&'a Hash> {
self.heads.iter()
}
/// Obtain an iterator over all heads.
pub fn into_iter(self) -> impl Iterator<Item=Hash> {
self.heads.into_iter()
}
/// Replace `self` with `new`.
///
/// Returns an iterator that will yield all elements of `new` that were not part of `self`.
......@@ -236,6 +276,14 @@ impl View {
pub fn contains(&self, hash: &Hash) -> bool {
self.heads.contains(hash)
}
/// Check if two views have the same heads.
///
/// Equivalent to the `PartialEq` fn,
/// but ignores the `finalized_number` field.
pub fn check_heads_eq(&self, other: &Self) -> bool {
self.heads == other.heads
}
}
/// v1 protocol types.
......
......@@ -1510,7 +1510,7 @@ mod tests {
let peer_data_from_view = |view: View| PeerData {
view: view.clone(),
view_knowledge: view.heads.iter().map(|v| (v.clone(), Default::default())).collect(),
view_knowledge: view.iter().map(|v| (v.clone(), Default::default())).collect(),
};
let mut peer_data: HashMap<_, _> = vec![
......
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