Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

ICMP message-routing gossip #304

Merged
merged 39 commits into from
Aug 29, 2019
Merged

ICMP message-routing gossip #304

merged 39 commits into from
Aug 29, 2019

Conversation

rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Jun 27, 2019

A description of the protocol can be found at https://research.web3.foundation/en/latest/polkadot/networking/4-icmp/ as well as in the module documentation of gossip.rs and message_routing.rs.

The goal is to create an everyone-sees-everything gossip protocol for routing inter-chain message queues. Each node will gossip only the queues it perceives as un-routed w.r.t. their best chain heads, and only send any given queue to peers who have some overlap in those chain heads - where the overlap's perspective of un-routed queues contains that queue.

This makes the protocol anti DoS-able and resilient against arbitrarily large Sybil attack, enabling us to open the gossip up to fishermen and collators instead of limiting it to validators.

@rphmeier rphmeier added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jun 27, 2019
}
}

/// whether it's allowed to send a statement to a peer with given knowledge
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// whether it's allowed to send a statement to a peer with given knowledge
/// Whether it's allowed to send a statement to a peer with given knowledge

/// A view of which queue roots are current for a given set of leaves.
pub struct View {
leaves: LeavesVec,
leaf_topics: HashMap<Hash, HashSet<Hash>>, // leaf_hash -> { topics }

This comment was marked as resolved.

This comment was marked as resolved.

pub struct View {
leaves: LeavesVec,
leaf_topics: HashMap<Hash, HashSet<Hash>>, // leaf_hash -> { topics }
expected_queues: HashMap<Hash, Hash>, // topic -> queue-root
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we declare and then import as necessary some type aliases like the following to avoid having to add comments to describe the implementation:

pub type ExpectedQueueRootHash = T::Hash;
pub type ExpectedQueueTopicHash = T::Hash;
Suggested change
expected_queues: HashMap<Hash, Hash>, // topic -> queue-root
expected_queues: HashMap<ExpectedQueueTopicHash, ExpectedQueueRootHash>,

Copy link
Contributor

@ltfschoen ltfschoen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

various nitpicks

@rphmeier rphmeier removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Aug 12, 2019
encoded.clone(),
);

tx.put_vec(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is storing the message data ready for some later recall under some circumstances? If those circumstances are not already documented, maybe put them here. If they are then a pointer to them somewhere near here would be most useful for anyone coming to this code.

relay_parent,
targets,
collation,
extrinsic,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this change for?

self.live_sessions.iter().take(MAX_CHAIN_HEADS).map(|(p, _)| p.clone())
}

/// Note a new session.
Copy link
Member

@gavofyork gavofyork Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useless docs. function level docs should generally try to include:

  • typical context or situation in which the call is expected to be made
  • any expected preconditions to calling
  • what it is meant to do
  • how it is meant to interact with other functions
  • clear description of each argument

}

/// Prune old sessions that fail the leaf predicate.
pub(super) fn prune_old_sessions<F: Fn(&Hash) -> bool>(&mut self, is_leaf: F) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, not super clear what this is meant to do. "sessions" clashes with runtime terminology and is_leaf's meaning is unclear (should it be renamed to keep_leaf?).

self.topics.retain(|_, v| live_sessions.iter().find(|(p, _)| p == v).is_some());
}

/// Whether a topic is live.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does "live" mean in this context? is it defined in this file? (i don't see a definition anywhere.)

}


/// Validate an attestation statement of some kind.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fuller docs needed (as above). what does it mean to "validate" in this context?

network/src/gossip/message_routing.rs Outdated Show resolved Hide resolved
network/src/gossip/message_routing.rs Outdated Show resolved Hide resolved
network/src/gossip/message_routing.rs Show resolved Hide resolved
@rphmeier rphmeier added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Aug 15, 2019
@rphmeier rphmeier added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Aug 15, 2019
@rphmeier rphmeier merged commit 03cfa5e into master Aug 29, 2019
@rphmeier rphmeier deleted the rh-icmp-gossip-primitives branch August 29, 2019 09:50
gui1117 pushed a commit that referenced this pull request Sep 3, 2019
* core logic for ICMP gossip

* refactor gossip to make more extension friendly

* move files aroun

* extract attestation-gossip logic to its own module

* message validation and broadcast logic

* fix upstream crates' compilation

* add a test

* another test for overlapping

* Some grammar and phrasing tweaks

Co-Authored-By: Luke Schoen <[email protected]>

* add since parameter to ingress runtime API

* broadcast out known unrouted message queues

* fix compilation of service and collator

* remove useless index_mapping

* some tests for icmp propagation

* fix decoding bug and test icmp queue validation

* simplify engine-id definition

Co-Authored-By: Sergei Pepyakin <[email protected]>

* address some grumbles

* some cleanup of old circulation code

* give network a handle to extrinsic store on startup

* an honest collator ensures data available as well

* address some grumbles

* add docs; rename the attestation session to "leaf work"

* module docs

* move gossip back to gossip.rs

* clean up and document attestation-gossip a bit

* some more docs on the availability store

* store all outgoing message queues in the availability store

* filter `Extrinsic` out of validation crate

* expunge Extrinsic from network

* expunge Extrinsic from erasure-coding

* expunge Extrinsic from collator

* expunge from adder-collator

* rename ExtrinsicStore to AvailabilityStore everywhere

* annotate and clean up message-routing tests
gavofyork pushed a commit that referenced this pull request Sep 3, 2019
* Update to latest Substrate (#407)

* Update to latest Substrate

* Fix main.rs

* Update substrate master (#411)

* in progress impl

* im_online authorityid

* fix

* fix

* use polkadot-master

* trigger CI

* trigger CI

* fix removal

* storage reorganize included

* lock version

* remove unused

* increment spec version

* ICMP message-routing gossip (#304)

* core logic for ICMP gossip

* refactor gossip to make more extension friendly

* move files aroun

* extract attestation-gossip logic to its own module

* message validation and broadcast logic

* fix upstream crates' compilation

* add a test

* another test for overlapping

* Some grammar and phrasing tweaks

Co-Authored-By: Luke Schoen <[email protected]>

* add since parameter to ingress runtime API

* broadcast out known unrouted message queues

* fix compilation of service and collator

* remove useless index_mapping

* some tests for icmp propagation

* fix decoding bug and test icmp queue validation

* simplify engine-id definition

Co-Authored-By: Sergei Pepyakin <[email protected]>

* address some grumbles

* some cleanup of old circulation code

* give network a handle to extrinsic store on startup

* an honest collator ensures data available as well

* address some grumbles

* add docs; rename the attestation session to "leaf work"

* module docs

* move gossip back to gossip.rs

* clean up and document attestation-gossip a bit

* some more docs on the availability store

* store all outgoing message queues in the availability store

* filter `Extrinsic` out of validation crate

* expunge Extrinsic from network

* expunge Extrinsic from erasure-coding

* expunge Extrinsic from collator

* expunge from adder-collator

* rename ExtrinsicStore to AvailabilityStore everywhere

* annotate and clean up message-routing tests
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants