-
Notifications
You must be signed in to change notification settings - Fork 2.2k
discovery+rpc: expose graph synced status within GetInfo #3355
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
Conversation
This'll have an affect on tools such as |
rpcserver.go
Outdated
@@ -4029,6 +4029,7 @@ func (r *rpcServer) GetNetworkInfo(ctx context.Context, | |||
MaxChannelSize: int64(maxChannelSize), | |||
MedianChannelSizeSat: int64(medianChanSize), | |||
NumZombieChans: numZombies, | |||
SyncedToGraph: r.server.authGossiper.SyncManager().IsGraphSynced(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you make the return value channel, then we don't need to block here and can fall through to false
in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went with a different approach instead.
for some reason i was under the impression we wanted to expose a percentage rather than just a boolean, was that not the case? other the code changes look pretty good |
@cfromknecht I went for the simplest approach possible as exposing a percentage seemed a bit more involved, though I'm not opposed to doing that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👨🚀 We can defer on exposing a percentage-based indicator if it proves useful
@cfromknecht IMO, a percentage would be much more useful. The boolean is ok, but feels incomplete to me. |
@sergioabril could you provide more insight for your use case of the percentage? Would it just be to expose the percentage to users in a higher level application? With the boolean, it should quickly flip to true unless it's your first time syncing the graph or you've been offline for a while. |
Yes, exactly for that; but I'm thinking of mobile wallets, which can be offline for a few days/week, and where a more precise state description is needed for the user to wait. Desktop instances are probably fine with a boolean.
… On 31 Jul 2019, at 17:07, Wilmer Paulino ***@***.***> wrote:
@sergioabril could you provide more insight for your use case of the percentage? Would it just be to expose the percentage to users in a higher level application? With the boolean, it should quickly flip to true unless it's your first time syncing the graph or you've been offline for a while.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
If we connect to a peer that doesn't respond, will this continue to show |
@cfromknecht wasn't the case previously, but should be fixed in the latest version! |
just looked at the latest diff, how is this solved? if we send a |
See fcb6a8b. We’ll pick up on the status of the next historical sync if our initial one hasn’t completed by then. Did you have another solution in mind? |
i don't think we're talking about the same issue. the
one option is to add a deadline after which it will flip to true regardless, that way if a peer doesn't respond we're still covered. in my mind, there's really three states:
when we come up, we start at
my proposal is to add a timeout where if we stay in alternatively, these enums could be exposed over rpc to allow higher-level applications to act on. for example, if an app sees that we're still in |
Why not just alert users rather than preventing them from completely making payments? Higher level applications can choose to interpret this field as they wish. They can decide to only gate payments up to some timeout as you suggested, gate payments regardless of how long it takes, alert users, or something else entirely.
Transitioning to true even though we haven't synced the graph seems to defeat the purpose of the flag, which is just to give an indication that we have done our part to sync the graph with at least one peer fully and will continue to do so through the consequent historical syncs. Just because they replied once doesn't mean we can guarantee they'll continue to do so, so we can still be in the
They can already do this with how things are at the moment, i.e. |
continue | ||
} | ||
|
||
// Otherwise, we'll track the peer we've performed a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting. So this is really an existing bug, but it wouldn't really matter that the initialHistoricalSyncCompleted
was never set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. It would matter as it could prevent us from transitioning to active syncers.
…l sync This ensures that the graph synced status is marked true at some point once a historical sync has completed. Before this commit, a stalled historical sync could cause us to never mark the graph as synced.
On the question of if we need to care about a peer stalling: that seems to be more of an issue for our peer selection, and eclipse attack mitigations, no? If a peer isn't responding, then we can move to time them out, and rotate to another peer. Our initial use case for this is also pretty simple, in just greying things out, or displaying this as an active indicator. Also practically, in the case of the application, they're connected to mostly known "golden" peers as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTB 💰
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🌋
I get |
Thanks @githorray, fixed with #3400. |
Fixes #3289.
One thing to note is that the current implementation will lead to GetNetworkInfo blocking if it's called while lnd is waiting for the backend to be considered synced. This is because we don't initialize lnd until that's done first. This can be addressed, but I've decided to leave it out unless we think it's something definitely worth addressing.