dexcctl: Add supported rpc version.

[JoeGruffins]

If dexc and dexcctl are built from a different commit, dexcctl may work and it may not. The only way to know is to try methods until maybe you hit an error. For this reason I propose adding the supported rpc version to -V returns so that a user can know if their dexcctl should be compatible with the rpcserver they are communicating with. This version should be kept up to date with the dexc rpcserver version.

More discussion here: https://github.com/decred/dcrdex/pull/1149#discussion_r682739382

[xaur]

Just an idea, couldn't dexcctl detect protocol version mismatch automatically and warn the user?

[ukane-philemon]

@JoeGruffins anyone on this yet? Would like to take it up.

Just an idea, couldn't dexcctl detect protocol version mismatch automatically and warn the user?

@xaur a logged warning is nice during dexcctl start up(a onetime check for compatibility), a users might not do a manual check.

[chappjc]

Looks resolved already in https://github.com/decred/dcrdex/commit/f4bfdbfd95a44206871bcf27e9f97f2f20e3b8b5

$  ./dexcctl -V
dexcctl version 0.4.3-pre+dev (Go version go1.18 linux/amd64)

I'm not keen on the idea of bloating the requests with versions though to automatically detect a mismatch.

[ukane-philemon]

the idea of bloating the requests with versions though to automatically detect a mismatch

Isn't the check for rpcserver version? I assume the product version is different from rpcserver version? https://github.com/decred/dcrdex/blob/faa60915201ff39ba04d7dbd4d15c1fe00e51d8a/client/rpcserver/rpcserver.go#L39-L43 Checking this once is doable, just like dcrwallet.

[chappjc]

Checking this once is doable, just like dcrwallet.

What do you mean? Check when? What does dcrwallet do?

Keep in mind dexcctl is more like dcrctl, with a one-shot HTTP POST.

[ukane-philemon]

What do you mean? Check when? What does dcrwallet do?

https://github.com/decred/dcrwallet/blob/HEAD/chain/sync.go#L276-L287 was looking at the possibility of doing the same with dexcctl.

[chappjc]

That makes sense for a websocket connection, and we do that when dexc connects to dcrwallet and when dcrdex connects to dcrd, but with the HTTP requests from dexcctl, I don't know how we'd do that other than making an extra version RPC before every command. I don't think dcrctl has any version checking for the same reason.

[ukane-philemon]

Is running an init() that makes a request for rpcserver version, does the check and log a warning an option?

[ukane-philemon]

Keep in mind dexcctl is more like dcrctl, with a one-shot HTTP POST.

Alternatively, we could have a rpcversion(cfg *config) which would check for rpc semver compatibility. This could be done when a user checks for dcrcctl version. However, the -v flag is handled early in configure(). Checking dcrcctl would include a warning if rpc semver is not compatible, otherwise only dcrcctl version info is shown. e.g

$  ./dexcctl -V
dexcctl version 0.4.3-pre+dev (Go version go1.18 linux/amd64)
RPC Semantic Version: advertised rpc semantic version  0.1.0 is not compatible with required version 0.2.0

@chappjc what do you think about this approach?

[ukane-philemon]

I propose adding the supported rpc version to -V returns

@JoeGruffins , I'm keen on understanding how knowing the supported RPC version can help the user if they can't auto check from dexcctl.

@chappjc is not keen on the idea of bloating requests with version check.

[JoeGruffins]

Sorry I didn't notice this was being discussed.

I am open up to whatever. But stating the problem again, we want to know if dexcctl and the rpcserver are compatible.

Right now we can get:

$ ./dexcctl -V
dexcctl version 0.5.0-pre+c3327eacf (Go version go1.18 linux/amd64)
$ ./dexcctl --simnet version
0.2.0

This isn't so helpful because one is build version, the other is rpc api version.

I think the simplest answer is to add the rpc api version to the -V command, preferably as a json so that automated users can easily parse the version. so, build version and rpc api version returns. This allows us to compare to the version returned from the rpc server, assuming the version command still works.

For discussion, this is what you get with dcrd and dcrctl:

$ dcrctl -V
dcrctl version 1.7.0+release.local (Go version go1.17.6 linux/amd64)
[joe@hyrule ~]$ dcrctl --testnet version
{
  "dcrd": {
    "versionstring": "1.7.1+release.local",
    "major": 1,
    "minor": 7,
    "patch": 1,
    "prerelease": "",
    "buildmetadata": "release.local.go1-18"
  },
  "dcrdjsonrpcapi": {
    "versionstring": "7.0.0",
    "major": 7,
    "minor": 0,
    "patch": 0,
    "prerelease": "",
    "buildmetadata": ""
  }
}

So you know the api version of dcrd, but not of the communicating tool dcrctl! I don't really understand why this is.

Knowing the build version of client might also be helpful though.

If anyone is wondering why communication might fail, there are some functions that use the # arg to parse a file or give a password prompt, and the passwords are separated in the request. If there was one of these types of args either moved or added, dexcctl would fail to send the correct data.

[JoeGruffins]

I'm keen on understanding how knowing the supported RPC version can help the user if they can't auto check from dexcctl.

Well, can check the version before you start doing other stuff. Just need to check once probably.

[JoeGruffins]

1611 is a good example of why this would be useful.

After those changes, if a user with the dexcctl before these changes were to do --help they would see new send endpoint but if they tried to use it they would get a cryptic error about the number of args being wrong. Ideally, dexcctl and the dexc rpcserver would have a Semver and we would update the Minor here. Then the user can be expected to check the versions match before trying to communicate. In this case they would see that the server Minor is higher for the server so there may be some functionality that they cannot access.

[ukane-philemon]

if they tried to use it they would get a cryptic error about the number of args being wrong

Both send and withdraw rpc endpoint use the same form though(no extra arg). Will update the rpcserver semver minor in #1611.

[chappjc]

Then the user can be expected to check the versions match before trying to communicate

I'm fine with having -V return both app and protocol version, then updating the version RPC to report the RPC server's protocol version. It could even warn or error if it detects a mismatch.

Just as long as we aren't' talking about every single RPC requiring a version check first.

[JoeGruffins]

Both send and withdraw rpc endpoint use the same form though(no extra arg).

dexcctl has to include the password argument in the password args. The old version does not while the new one does. This is the error when trying the old dexcctl with the new rpcserver:

$ ./dexcctl --simnet send 42 100000000 SsoiAXujKA2aj3zsVVTNztfEPugwscyUrVz
password arguments: unable to parse arguments: wanted 1 but got 0