Some gossip improvements #1892
Some gossip improvements #1892
Conversation
rphmeier
commented
Feb 27, 2019
rphmeier
commented
- Allow gossip validators to specify that a message is not currently possible to check. This message will be kept and re-checked when a stream for that topic is requested
- gossip validators get a separate function for checking expired messages. this avoids re-doing other validation work like signature verification when collecting garbage
- Topic streams are localized to engine ID to avoid collisions.
core/network/src/consensus_gossip.rs
Outdated
| #[test] | ||
| fn collects_garbage() { | ||
|
|
||
| struct AllowAll; |
|
Shouldn't GRANDPA validator re-implement |
| fn validate(&self, data: &[u8]) -> ValidationResult<H>; | ||
|
|
||
| /// Produce a closure for validating messages on a given topic. | ||
| fn message_expired<'a>(&'a self) -> Box<FnMut(H, &[u8]) -> bool + 'a> { |
There was a problem hiding this comment.
Why return a closure instead of just calling the method directly like we do for validate?
There was a problem hiding this comment.
my intuition was that this can get called with a lot of topic messages at once, so the closure lets us hold locks
| } else if set_id == self.set_id + 1 { | ||
| // allow a few first rounds of future set. | ||
| if round > MESSAGE_ROUND_TOLERANCE { | ||
| trace!(target: "afg", "Expired: Message too far in the future set, round {} (ours set_id {})", round, self.set_id); |
There was a problem hiding this comment.
Shouldn't we tag messages within the tolerance as Future?
There was a problem hiding this comment.
I don't think so, because we have enough information to validate them
| } | ||
| } else if set_id == self.set_id { | ||
| if round < self.min_live_round.saturating_sub(MESSAGE_ROUND_TOLERANCE) { | ||
| trace!(target: "afg", "Expired: Message round is out of bounds {} (ours {}-{})", round, self.min_live_round, self.max_round); |
| /// Message is valid with this topic. | ||
| Valid(H), | ||
| /// Message is future with this topic. | ||
| Future(H), |
There was a problem hiding this comment.
Why not separate this into ValidationResult { Valid, Invalid } and ExpirationResult { Live, Expired, Future }? This would make sure that the interface we provide always starts by checking expiration, and only calls validate once which presumably might do heavier checks.
There was a problem hiding this comment.
With the current usage of validate in GRANDPA it doesn't make a difference since it always starts by doing the expiration checks, but I think it would make the interface cleaner.
There was a problem hiding this comment.
(I agree, but could we file a follow-up for that?)
* queue messages in future * use new gossip API in GRANDPA * implement message_expired for grandpa * fix indent
* Telemetry improvements (#1886) * Fix typo * Support multiple telemetry endpoints and verbosity levels * Bump substrate-telemetry version * Telemetrify Aura consensus * Telemetrify Grandpa * Fix CI version conflicts * Implement style remarks * Fix fixture * Implement style remarks * Clone only when necessary * Get rid of Arc for URL * Handle connection issues better * fixed instant finalization of genesis block on light client (#1898) * Some gossip improvements (#1892) * queue messages in future * use new gossip API in GRANDPA * implement message_expired for grandpa * fix indent
* queue messages in future * use new gossip API in GRANDPA * implement message_expired for grandpa * fix indent
