Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Add SingleGossip commitment level to use for subscriptions #10147

Merged
merged 2 commits into from
May 22, 2020

Conversation

CriesofCarrots
Copy link
Contributor

@CriesofCarrots CriesofCarrots commented May 20, 2020

Problem

#10137 investigates triggering bank-related notifications from gossip, but the notification thread will get bogged down if it has to check the entire notifications list twice per slot. It would be better to separate subscriptions that should be notified via gossip and those that should be notified vie replay stage, so they can be checked and notified separately.

Summary of Changes

Open Questions:

  • Do we need to support the SingleGossip commitment level in rpc.rs? If so, how will we get the most recent 2/3 slot from cluster_info_vote_listener?
  • Is there a better variant name than SingleGossip?

CriesofCarrots pushed a commit to sakridge/solana that referenced this pull request May 20, 2020
@codecov
Copy link

codecov bot commented May 20, 2020

Codecov Report

Merging #10147 into master will increase coverage by 0.0%.
The diff coverage is 94.8%.

@@           Coverage Diff           @@
##           master   #10147   +/-   ##
=======================================
  Coverage    81.4%    81.5%           
=======================================
  Files         288      283    -5     
  Lines       66211    66176   -35     
=======================================
+ Hits        53938    53949   +11     
+ Misses      12273    12227   -46     

CriesofCarrots pushed a commit to CriesofCarrots/solana that referenced this pull request May 20, 2020
CriesofCarrots pushed a commit to CriesofCarrots/solana that referenced this pull request May 20, 2020
CriesofCarrots pushed a commit to sakridge/solana that referenced this pull request May 20, 2020
@CriesofCarrots CriesofCarrots marked this pull request as ready for review May 21, 2020 20:16
@CriesofCarrots
Copy link
Contributor Author

@mvines , can you take a quick peek at this? This lays the groundwork for #10137 , which implements notifications on seeing 2/3+ stake votes in gossip.
In particular, I would like your take on my open questions above.
Thank you!

@CriesofCarrots CriesofCarrots requested a review from jstarry May 21, 2020 23:53
Copy link
Contributor

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

I think this is great that we are moving towards the direction of only checking subscriptions that need to be checked!

I think a more generalized solution could be implemented (now or later) where we track the last checked slot for a particular commitment level. Right now, it looks like Max, Root, and Single could all be double checked in the current implementation. A nice side effect of tracking the last checked slot for each commitment level would be that we could use it to initialize last_notified_slot for account subscriptions which seems kind of tricky for single gossip

core/src/rpc_subscriptions.rs Outdated Show resolved Hide resolved
@@ -491,7 +533,16 @@ impl RpcSubscriptions {

pub fn remove_program_subscription(&self, id: &SubscriptionId) -> bool {
let mut subscriptions = self.subscriptions.program_subscriptions.write().unwrap();
remove_subscription(&mut subscriptions, id)
if remove_subscription(&mut subscriptions, id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be time to add a subscription id lookup map, thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed on slack, agreed! Will file an issue for follow-up refactoring work.

@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label May 22, 2020
@solana-grimes solana-grimes merged commit 2928c5d into solana-labs:master May 22, 2020
CriesofCarrots pushed a commit to sakridge/solana that referenced this pull request May 22, 2020
CriesofCarrots added a commit to CriesofCarrots/solana that referenced this pull request May 26, 2020
@CriesofCarrots CriesofCarrots deleted the pubsub-gossip branch May 26, 2020 23:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants