Skip to content
This repository has been archived by the owner on Jun 25, 2021. It is now read-only.

Commit

Permalink
fix(delivery_targets): deliver to all when targets are final dst
Browse files Browse the repository at this point in the history
- Also adds test cases for this, however these are currently missing proper precondition setup, thus ignored for now.
  • Loading branch information
oetyng authored and dirvine committed Apr 21, 2021
1 parent cd0e4a0 commit f26722b
Showing 1 changed file with 73 additions and 14 deletions.
87 changes: 73 additions & 14 deletions src/delivery_group.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,17 @@ fn candidates(
// gives at least 1 honest target among recipients.
let min_dg_size = 1 + ELDER_SIZE - supermajority(ELDER_SIZE);
let mut dg_size = min_dg_size;
let mut nodes_to_send = Vec::new();
let mut candidates = Vec::new();
for (idx, (prefix, len, connected)) in sections.enumerate() {
nodes_to_send.extend(connected.cloned());
// If we don't have enough contacts send to as many as possible
// up to dg_size of Elders
dg_size = cmp::min(len, dg_size);
candidates.extend(connected.cloned());
if prefix.matches(target_name) {
// If we are last hop before final dst, send to all candidates.
dg_size = len;
} else {
// If we don't have enough contacts send to as many as possible
// up to dg_size of Elders
dg_size = cmp::min(len, dg_size);
}
if len < min_dg_size {
warn!(
"Delivery group only {:?} when it should be {:?}",
Expand All @@ -129,19 +134,19 @@ fn candidates(

if prefix == section.prefix() {
// Send to all connected targets so they can forward the message
nodes_to_send.retain(|node| node.name() != our_name);
dg_size = nodes_to_send.len();
candidates.retain(|node| node.name() != our_name);
dg_size = candidates.len();
break;
}
if idx == 0 && nodes_to_send.len() >= dg_size {
if idx == 0 && candidates.len() >= dg_size {
// can deliver to enough of the closest section
break;
}
}
nodes_to_send.sort_by(|lhs, rhs| target_name.cmp_distance(lhs.name(), rhs.name()));
candidates.sort_by(|lhs, rhs| target_name.cmp_distance(lhs.name(), rhs.name()));

if dg_size > 0 && nodes_to_send.len() >= dg_size {
Ok((nodes_to_send, dg_size))
if dg_size > 0 && candidates.len() >= dg_size {
Ok((candidates, dg_size))
} else {
Err(Error::CannotRoute)
}
Expand Down Expand Up @@ -253,7 +258,7 @@ mod tests {
}

#[test]
fn delivery_targets_elder_to_unknown_remote_peer() -> Result<()> {
fn delivery_targets_elder_to_final_hop_unknown_remote_peer() -> Result<()> {
let (our_name, section, network, _) = setup_elder()?;

let elders_info1 = network
Expand All @@ -264,6 +269,32 @@ mod tests {
let dst = DstLocation::Node(dst_name);
let (recipients, dg_size) = delivery_targets(&dst, &our_name, &section, &network)?;

// Send to all elders in the dst section
let expected_recipients = elders_info1
.peers()
.sorted_by(|lhs, rhs| dst_name.cmp_distance(lhs.name(), rhs.name()));
assert_eq!(dg_size, elders_info1.elders.len());
itertools::assert_equal(&recipients, expected_recipients);

Ok(())
}

#[test]
#[ignore = "Need to setup network so that we do not locate final dst, as to trigger correct outcome."]
fn delivery_targets_elder_to_intermediary_hop_unknown_remote_peer() -> Result<()> {
let (our_name, section, network, _) = setup_elder()?;

let elders_info1 = network
.get(&Prefix::default().pushed(true))
.context("unknown section")?;

let dst_name = elders_info1
.prefix
.pushed(false)
.substituted_in(rand::random());
let dst = DstLocation::Node(dst_name);
let (recipients, dg_size) = delivery_targets(&dst, &our_name, &section, &network)?;

// Send to all elders in the dst section
let expected_recipients = elders_info1
.peers()
Expand All @@ -276,7 +307,7 @@ mod tests {
}

#[test]
fn delivery_targets_elder_to_remote_section() -> Result<()> {
fn delivery_targets_elder_final_hop_to_remote_section() -> Result<()> {
let (our_name, section, network, _) = setup_elder()?;

let elders_info1 = network
Expand All @@ -287,11 +318,39 @@ mod tests {
let dst = DstLocation::Section(dst_name);
let (recipients, dg_size) = delivery_targets(&dst, &our_name, &section, &network)?;

// Send to all elders in the dst section
// Send to all elders in the final dst section
let expected_recipients = elders_info1
.peers()
.sorted_by(|lhs, rhs| dst_name.cmp_distance(lhs.name(), rhs.name()));
assert_eq!(dg_size, elders_info1.elders.len());
itertools::assert_equal(&recipients, expected_recipients);

Ok(())
}

#[test]
#[ignore = "Need to setup network so that we do not locate final dst, as to trigger correct outcome."]
fn delivery_targets_elder_intermediary_hop_to_remote_section() -> Result<()> {
let (our_name, section, network, _) = setup_elder()?;

let elders_info1 = network
.get(&Prefix::default().pushed(true))
.context("unknown section")?;

let dst_name = elders_info1
.prefix
.pushed(false)
.substituted_in(rand::random());
let dst = DstLocation::Section(dst_name);
let (recipients, dg_size) = delivery_targets(&dst, &our_name, &section, &network)?;

// Send to a subset of elders in the intermediary dst section
let min_dg_size = 1 + elders_info1.elders.len() - supermajority(elders_info1.elders.len());
let expected_recipients = elders_info1
.peers()
.sorted_by(|lhs, rhs| dst_name.cmp_distance(lhs.name(), rhs.name()))
.take(min_dg_size);

assert_eq!(dg_size, min_dg_size);
itertools::assert_equal(&recipients, expected_recipients);

Expand Down

0 comments on commit f26722b

Please sign in to comment.