Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Match gossip v1.1 D_low, extend gossip_history param, add FAQ section #1958

Merged
merged 4 commits into from
Jul 22, 2020

Conversation

protolambda
Copy link
Contributor

@protolambda protolambda commented Jul 7, 2020

This PR updates the gossip sub parameters in the spec:

  • Match the D_low default recommend parameter which changed in v1.1
  • rename gossip_history to mcache_len to match param names in spec
  • rename gossip_advertise to mcache_gossip to match param names in spec
  • add seen_ttl as seen in spec
  • Change heartbeat_interval from 1s to 0.7s as per the recommendation of v1.1 PL report to allow for more responsiveness in the event that gossip is not performing optimally
  • Change mcache_len from 5 to 6 to allow for at least ~2s for IWANT queries after IHAVEs have been sent. This it to match original time window but to compensate or our faster heartbeat_interval
  • Set seen_ttl to 550 to match the length of an epoch (contextualized by heartbeat_interval)
  • Add a FAQ section to the spec, explaining the parameter choices.

@djrtwo
Copy link
Contributor

djrtwo commented Jul 8, 2020

I think we should define gossip_history as 1 + SLOTS_PER_EPOCH * SECONDS_PER_SLOT / heartbeat_interval to default just cover the length of time we want (an epoch) as a function of those params.

Considering dropping heartbeat_interval to 700ms as per the recommendation of the gossipsub v1.1 report (page 6)

@djrtwo
Copy link
Contributor

djrtwo commented Jul 8, 2020

We've also conflated seen_ttl and mcache_len from the gossipsub spec into just one param here -- gossip_history

arguably seen_ttl shuld be the 385 as recommended above and mcache_len/mcache_gossip should be considered independently

I'm not sure the exact trade-off at play when increasing/decreasing mcache params. mcache_len keeps more full messages in cache to be able to serve to other peers when they send IWANT requests. mcache_gossip dictates how many history windows you mention messages in IHAVE gossip.

Writing some quick notes on the intuition on mcache:

If we increase mcache_len far beyond mcache_gossip, the old items in the cache will not frequently be used because you'll quickly not be telling peers about the old messages so they won't know to ask for them.

If we increase both mcache_len and mcache_gossip too much, we'll overly redundantly gossiping via IHAVE about stale messages that everyone already has. This presumably would make each IHAVE message larger for little gain because it would include much older messages.

Also seems to make sense to have mcache_len be slightly larger than mcache_gossip so the nodes that have been gossiped to (via IHAVE) have a chance to request missing messages before you remove them from the cache.

@lsankar4033
Copy link
Contributor

Re:fanout_ttl - subnet_ids are re-used every subsequent epoch, so waiting an epoch for ttl might result in publishing attestations to peers in the previous epoch's committee of the same id.

This isn't breaking obviously but does result in spam; might be an argument to keep the ttl closer to 1-2 slots.

@protolambda protolambda marked this pull request as ready for review July 21, 2020 23:45
Co-authored-by: Diederik Loerakker <[email protected]>
@djrtwo djrtwo added this to the v0.12.2 milestone Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants