verrpc: add GetVersion endpoint #4163

cfromknecht · 2020-04-08T01:33:31Z

This PR adds a minimal verrpc subserver that is purely responsible for serving
version information about the running daemon. Though some of this information is
displayed via GetInfo, it is useful to have a single endpoint and service for this
purpose so that the chances of breaking changes are mitigated.

Third-party applications may not have the ability to be tightly synced with the
daemon, e.g. a mobile app connecting remotely via RPC. In order to support
proto version desynchronization, these apps typically come installed with several
historical versions of the proto files (especially when breaking changes have
been introduced). Apps can then determine the version of the daemon by trying
to decode various requests and looking for signals that indicate the version.

With verrpc, these headaches are largely eliminated. There may still be a need
to package multiple proto versions, but this makes it dead simple to map the
correct version. The intent (crosses fingers) is that this endpoint would never
undergo breaking changes so that app developers have a stable API they rely
on to determine versioning info.

In the future, we could consider separating the initialization of the verrpc
subserver to be active for wallet unlocking as well, but those protos don't
experience nearly as much churn as the others.

guggero

Great to see this, we probably should have added this a long time ago.
Did a quick first pass, looks good!

I was wondering if now was the time to also start tackling the wallet unlocker state problem.
We could add a simple Unlocked boolean flag to this new version service.
Then we only need a way to signal that a sub server should also be registered when the WalletUnlocker service is spun up. I was thinking of a marker interface. If the subserver implements it, it's added/registered to the wallet unlocker, if not, it's skipped.
What do you think? It probably also depends on the release you were targeting with this. If this PR should go in with 0.10.0 then the whole unlocker thing is probably out of scope.

cmd/lncli/cmd_version.go

lnrpc/rpc.proto

lnrpc/verrpc/verrpc.proto

joostjager

As you say, making sure the endpoint is always reachable (also if wallet is still locked) would be nice.

lnrpc/verrpc/verrpc.proto

cfromknecht · 2020-04-08T19:18:17Z

updated to compile build tags and go version into lnd and lncli, new output looks like this:

$ lncli version                 
{
    "lncli": {
        "commit": "v0.9.0-beta-551-ge89f4907f3a3d3ae034b400a44fd10c1567546c7-dirty",
        "commit_hash": "e89f4907f3a3d3ae034b400a44fd10c1567546c7",
        "version": "0.9.0-beta",
        "app_major": 0,
        "app_minor": 9,
        "app_patch": 0,
        "app_pre_release": "beta",
        "build_tags": [
            "routerrpc",
            "invoicesrpc"
        ],
        "go_version": "go1.14"
    },
    "lnd": {
        "commit": "v0.9.0-beta-551-ge89f4907f3a3d3ae034b400a44fd10c1567546c7-dirty",
        "commit_hash": "e89f4907f3a3d3ae034b400a44fd10c1567546c7",
        "version": "0.9.0-beta",
        "app_major": 0,
        "app_minor": 9,
        "app_patch": 0,
        "app_pre_release": "beta",
        "build_tags": [
            "routerrpc",
            "invoicesrpc"
        ],
        "go_version": "go1.14"
    }
}

Makefile

guggero

LGTM, only nits at this point 🎉

lnrpc/verrpc/verrpc.proto

cmd/lncli/cmd_version.go

Makefile

cmd/lncli/cmd_version.go

We'll do the validation during construction of the runtime so that we can safely use the AppPreRelease field externally without needing to normalize it.

The version field in getinfo is kept the same for backwards compatibility.

Roasbeef

LGTM 💎

I think we should also tack on the "unlocked" status as that resolves existing issues users have with the current RPC lifecycle with a single field. However, we can do this in the rc phase, not enough to block this PR IMO.

Roasbeef · 2020-04-13T22:56:06Z

build/version.go

+	// strings. In particular it MUST only contain characters in
+	// semanticAlphabet.
+	for _, r := range AppPreRelease {
+		if !strings.ContainsRune(semanticAlphabet, r) {


Nice change to panic instead of silently omit characters.

Roasbeef · 2020-04-13T23:00:08Z

lnrpc/verrpc/log.go

+var log btclog.Logger
+
+// Subsystem defines the logging code for this subsystem.
+const Subsystem = "VRPC"


Pretty cool that lnd will now ship with an integrated VR PC for the next major release 😎

githorray · 2020-04-14T07:47:52Z

Building on Windows is no longer working.

go build -v -tags="dev" -o lnd-debug -ldflags \-X github.com/ligh
tningnetwork/lnd/build.Commit=v0.9.0-beta-595-ga53a6f160e25dac4696685d5b5edfb799
194e2d7 -X github.com/lightningnetwork/lnd/build.CommitHash=a53a6f160e25dac46966
85d5b5edfb799194e2d7 -X github.com/lightningnetwork/lnd/build.GoVersion=go1.14.2
 -X github.com/lightningnetwork/lnd/build.RawTags=dev" github.com/lightningnetwo
rk/lnd/cmd/lnd
/usr/bin/sh: -c: line 0: unexpected EOF while looking for matching `"'
/usr/bin/sh: -c: line 1: syntax error: unexpected end of file
Makefile:107: recipe for target 'build' failed
make: *** [build] Error 258

guggero · 2020-04-14T10:44:56Z

Thanks for the report, @githorray! Can you please try if #4190 fixes the problem?

joostjager · 2020-04-22T15:10:04Z

lnrpc/verrpc/verrpc.proto

+    string app_pre_release = 7;
+
+    /// The list of build tags that were supplied during comilation.
+    repeated string build_tags = 8;


Looking at how this field is used in lightninglabs/loop#181 with the string matching and all, shouldn't we replace this with a compatibility flagged enum? It is a compilation symbol leaking into the runtime interface.

joostjager · 2020-04-28T18:58:22Z

@guggero and I ran into the problem that go get -tags doesn't set the build tags compiler variable. So even though the subservers are enabled, they aren't reported. Maybe conditional compilation of boolean vars would have been better for that.

Worth mentioning in release notes @cfromknecht?

cfromknecht · 2020-05-01T03:44:15Z

I don't think it's necessary. Our install docs are pretty clear that you should build with the makefile. There are a host of other things that you'd be missing if you install via go get like version, commit hash, etc...

cfromknecht requested a review from Roasbeef as a code owner April 8, 2020 01:33

cfromknecht added the rpc Related to the RPC interface label Apr 8, 2020

guggero reviewed Apr 8, 2020

View reviewed changes

cmd/lncli/cmd_version.go Outdated Show resolved Hide resolved

lnrpc/rpc.proto Outdated Show resolved Hide resolved

lnrpc/verrpc/verrpc.proto Outdated Show resolved Hide resolved

joostjager reviewed Apr 8, 2020

View reviewed changes

lnrpc/verrpc/verrpc.proto Outdated Show resolved Hide resolved

lnrpc/verrpc/verrpc.proto Show resolved Hide resolved

cfromknecht force-pushed the version-rpc branch from 59db6b6 to 05dca1a Compare April 8, 2020 19:03

cfromknecht requested review from guggero and joostjager April 8, 2020 19:18

joostjager reviewed Apr 8, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

guggero approved these changes Apr 9, 2020

View reviewed changes

lnrpc/verrpc/verrpc.proto Outdated Show resolved Hide resolved

cmd/lncli/cmd_version.go Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

cmd/lncli/cmd_version.go Outdated Show resolved Hide resolved

cmd/lncli/cmd_version.go Outdated Show resolved Hide resolved

Roasbeef added this to the 0.10.0 milestone Apr 9, 2020

Roasbeef added the v0.10 label Apr 9, 2020

cfromknecht force-pushed the version-rpc branch 4 times, most recently from f8d161c to 38b2dea Compare April 10, 2020 00:11

cfromknecht requested review from guggero and joostjager April 10, 2020 00:24

cfromknecht added 10 commits April 10, 2020 16:39

build/version: fix incomplete semanticAlphabet godoc

06bd645

build/version: expose AppMajor, AppMinor, AppPatch, etc.

640aadb

build/version: check AppPreRelease semantics in init()

b68764f

We'll do the validation during construction of the runtime so that we can safely use the AppPreRelease field externally without needing to normalize it.

make+build: compile SHA1 commit hash into binary

75a1a1f

make+build: compile build tags into binary

0742083

make+build: compile go version into binary

d66f15d

lnrpc/verrpc: add Versioner RPC server

a25269c

bulid: only return semantic version from Version()

5c04038

The version field in getinfo is kept the same for backwards compatibility.

lnrpc+rpcserver: expose git commit hash in getinfo

91cd7e6

cmd/lncli: add version command that reads verrpc.GetVersion

f93a8ab

cfromknecht force-pushed the version-rpc branch from 38b2dea to f93a8ab Compare April 10, 2020 23:39

Roasbeef approved these changes Apr 13, 2020

View reviewed changes

Roasbeef merged commit 786e278 into lightningnetwork:master Apr 13, 2020

cfromknecht deleted the version-rpc branch April 13, 2020 23:04

guggero mentioned this pull request Apr 14, 2020

make+lnd+config: fix Windows build and version string #4190

Merged

joostjager reviewed Apr 22, 2020

View reviewed changes

joostjager mentioned this pull request May 2, 2020

[Feature Request] Version endpoint should return active sub-servers #4238

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

verrpc: add GetVersion endpoint #4163

verrpc: add GetVersion endpoint #4163

cfromknecht commented Apr 8, 2020 •

edited

Loading

guggero left a comment

joostjager left a comment

cfromknecht commented Apr 8, 2020

guggero left a comment

Roasbeef left a comment

Roasbeef Apr 13, 2020

Roasbeef Apr 13, 2020

githorray commented Apr 14, 2020

guggero commented Apr 14, 2020

joostjager Apr 22, 2020 •

edited

Loading

joostjager commented Apr 28, 2020

cfromknecht commented May 1, 2020

verrpc: add GetVersion endpoint #4163

verrpc: add GetVersion endpoint #4163

Conversation

cfromknecht commented Apr 8, 2020 • edited Loading

guggero left a comment

Choose a reason for hiding this comment

joostjager left a comment

Choose a reason for hiding this comment

cfromknecht commented Apr 8, 2020

guggero left a comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

Roasbeef Apr 13, 2020

Choose a reason for hiding this comment

Roasbeef Apr 13, 2020

Choose a reason for hiding this comment

githorray commented Apr 14, 2020

guggero commented Apr 14, 2020

joostjager Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

joostjager commented Apr 28, 2020

cfromknecht commented May 1, 2020

cfromknecht commented Apr 8, 2020 •

edited

Loading

joostjager Apr 22, 2020 •

edited

Loading