multi: report protocol version #186
Conversation
| bytes last_hop = 5; | ||
|
|
||
| /// The protocol version that the client adheres to. | ||
| ProtocolVersion protocol_version = 6; |
There was a problem hiding this comment.
Question is, should we version all RPC calls? Or just loops?
Should we use server reflection instead? (https://github.com/grpc/grpc-go/tree/master/examples/features/reflection)
There was a problem hiding this comment.
Can you get a version number with server reflection?
What is versioned here is just the protocol version (scripts etc), not the RPC interface. For that, we'd need to have a separate endpoint like GetVersion in lnd.
There was a problem hiding this comment.
Going to look into the grpc context, maybe that can carry the version number.
There was a problem hiding this comment.
Discussed with @guggero and grpc metadata doesn't seem a great idea. It is all string based (no nice enum) and also proxies may change the keys 😨
There was a problem hiding this comment.
Added version to all RPC calls.
There was a problem hiding this comment.
Server reflection may be used for protocol discovery, which would be useful for clients to check if their protocol description is compatible with the server protocol description. Afaik context may be used to store a version number (it's similar to http headers) but that also is somewhat hidden (as in not reflected in the proto definitions).
There was a problem hiding this comment.
The point is that not every aspect of the protocol (which for example also includes the on-chain htlc bitcoin script), can be observed from the proto signatures.
Using the grpc metadata in the context seems not very attractive (see comment above)
joostjager
force-pushed
the
report-version
branch
from
6ff6eec to
2870f4a
Compare
carlaKC
left a comment
carlaKC
left a comment
There was a problem hiding this comment.
This PR adds that version as a field to the loop in and out initiation calls.
Are we sure we won't need version fields on the RPC endpoints? If it's feasible, I can see some regrets down the line that we didn't just add them all now. It might also be useful to have them for all calls for meta information; perhaps a certain version is always calling for quotes but never looping, might pick up an unreported bug?
Also uncertain of the current enum's meaning (might just be a naming issue, if I have the right idea).
looprpc/server.proto
Outdated
| LEGACY = 0; | ||
|
|
||
| /// Client reports its supported protocol version. | ||
| REPORT_VERSION = 1; |
There was a problem hiding this comment.
So this enum will be extended to have additional protocol versions? If that's the case the name REPORT_VERSION and its comment don't make sense to me - makes me think we're going to provide a specific version value somewhere.
Also, how are these protocol versions tracked? By adding an enum to this proto?
Thinking about the case of a non-loop client (if we're accounting for that), how do I know what version I'm supporting?
There was a problem hiding this comment.
I can agree with @carlaKC here, the naming is a bit confusing. Perhaps instead of calling it ProtocolVersion we could call itt SwapProtocol (if I understand the purpose correctly), and have better naming for the individual protocol identifiers.
There was a problem hiding this comment.
I think that the confusion is that REPORT_VERSION itself is a feature that a client can support?
The idea is indeed that with every protocol change we do on the server, we add a value to the enum. Then the next client release will start sending that enum value.
There was a problem hiding this comment.
Loop is all about swaps, so SwapProtocol doesn't add more information than just Protocol in my opinion.
Suggestions for what to rename REPORT_VERSION to?
There was a problem hiding this comment.
I think a better question is how will we use REPORT_VERSION? Isn't LEGACY the only version we use now?
There was a problem hiding this comment.
Can also call it MULTI_LOOP_OUT?
There was a problem hiding this comment.
That is much better :) I'd be happy with that.
joostjager
force-pushed
the
report-version
branch
from
2870f4a to
def436f
Compare
joostjager
force-pushed
the
report-version
branch
from
def436f to
92730e0
Compare
3 participants
To keep supporting older clients, it is required for the server to know the protocol version that a client adheres to. This PR adds that version as a field to the loop in and out initiation calls.