Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add "req-res protocol reliability" spec #18
Add "req-res protocol reliability" spec #18
Changes from 35 commits
1383a91
3edb500
7371dab
8582646
d0827cf
3249e7d
057c4f2
35fd44f
e5ba255
f2802e7
174dffb
d5abdb3
ba103d2
9a4bb3d
c23dfe9
ef1c08a
988a022
9eb63db
594a1ec
6a14926
1f433b9
7e030f8
7ba0897
f2b4a77
67462e7
b264454
092b18e
59e225d
fb3e69b
dc4d40f
6296d12
d8dcb53
642a539
dca926a
0669283
687d7e0
6436cdc
6a80762
5ad85b7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Wouldn't this just be a "Service failure" - to me a node failure implies some issue with the node itself (such as bad config, unreachability, etc.". Service failure can include both node and protocol failures
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.
This seems like a blanket rule here (any failure = replace service node), but just below we qualify which types of failures should be considered serious enough to look for another service node. Perhaps this clause can rather be something like "In case a service node fails to serve...light node MAY drop the connection... We RECOMMEND that service nodes be replaced when the failure conditions below are met."
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.
Good concept.
However, I'd would have 2 types of filter:
And focus only required for now
strict:
preference:
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.
added for strict
for preference - we need to somehow expose this information to be available, ideally, before connection is established, for some it is there already but - I believe it is tricky for others like latency and reputation.
also, this is useful to have for incentivisation, but then there should be some guarantee (validators?)
sound like a new feature :)
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.
Nice direction!
It's tricky to do this because protocol handshake & information exchange happens once there is a connection with the service node. We already wait for information like shards & cluster, supported protocols (from the strict portion) until a connection is established with the service node.
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.
At this point in time, we should aim for single version protocol support in light clients IMO.
We can look into multiple version support as the network grows and we have less control on it.
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.
I think it is still worth mentioning as it might be not only version but needed protocol depending on the strategies used: Store + LightPush or Filter + LightPush or all
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.
Another possible mechanism we should describe is regular "refresh" of subscriptions to ensure active subscriptions are still fully synchronised. Think of it as a much more occasional, more expensive "ping". Described here: https://github.com/vacp2p/rfc-index/blob/7b443c1aab627894e3f22f5adfbb93f4c4eac4f6/waku/standards/core/12/filter.md#subscribe
We do not have to recommend using this mechanism, as it's quite expensive and I'm not sure we'll gain much by implementing it. However, it is a mechanism affecting reliability and I think it's useful to have the entire toolbox briefly described.
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.
I think it partially covered with:
specs/informational/req-res-reliability.md
Line 120 in 0669283
but there is still room for improvement if following is true: can a subscription on a service node degrade over time / be dropped with regular pings? if so, is it fixable by just re-creating subscription by init query.
so, like service node is fine, just code responsible for Filter needs a bit of turn off and on
do you think it is viable, @jm-clius ?