cli: degrade -getinfo gracefully for older servers #19874

pull jonatack wants to merge 1 commits into bitcoin:master from jonatack:getinfo-handle-older-servers-gracefully changing 1 files +6 −2

jonatack commented at 6:49 PM on September 4, 2020: member
Follow-up to #19405. Degrade gracefully when -getinfo is called on an older version of bitcoind where getnetworkinfo does not return in/out connections.

before
```
  "connections": {
    "in": null,
    "out": null,
    "total": 125
  }
```
after
```
  "connections": {
    "total": 125
  }
```
luke-jr commented at 6:54 PM on September 4, 2020: member

This opens a can of worms IMO, and isn't worth it just to hide null values (which should be considered equivalent to omitted ones anyway)
jonatack commented at 6:57 PM on September 4, 2020: member

@luke-jr that could be. I felt I'd be remiss to not at least propose a patch.
laanwj commented at 8:03 PM on September 4, 2020: member

Code review ACK. No opionion on the concept, I think it's fine to leave these as null when you want to use bitcoin-cli with an older server, but also to hide them.

in src/bitcoin-cli.cpp:280 in ab338a192b outdated

 272 | @@ -273,8 +273,10 @@ class GetinfoRequestHandler: public BaseRequestHandler
 273 |          result.pushKV("timeoffset", batch[ID_NETWORKINFO]["result"]["timeoffset"]);
 274 |  
 275 |          UniValue connections(UniValue::VOBJ);
 276 | -        connections.pushKV("in", batch[ID_NETWORKINFO]["result"]["connections_in"]);
 277 | -        connections.pushKV("out", batch[ID_NETWORKINFO]["result"]["connections_out"]);
 278 | +        const UniValue in{batch[ID_NETWORKINFO]["result"]["connections_in"]};
 279 | +        const UniValue out{batch[ID_NETWORKINFO]["result"]["connections_out"]};
 280 | +        if (!in.isNull()) connections.pushKV("in", in);

laanwj commented at 8:05 PM on September 4, 2020:

If we do this, I'd prefer to add a comment describing why these isNull checks are done for these fields and not for, say connections itself. It won't be obvious in the future.

jonatack commented at 12:26 PM on October 2, 2020:

Good idea. Added.

laanwj added the label RPC/REST/ZMQ on Sep 4, 2020
cli: degrade -getinfo gracefully for older servers aa5f41948f
jonatack force-pushed on Oct 2, 2020
jonatack renamed this:
~~cli: degrade -getinfo gracefully for older servers~~
cli, bugfix: degrade -getinfo gracefully for older servers
on Oct 15, 2020
jonatack closed this on Oct 15, 2020
jonatack reopened this on Oct 15, 2020
jonatack renamed this:
~~cli, bugfix: degrade -getinfo gracefully for older servers~~
cli: degrade -getinfo gracefully for older servers
on Oct 21, 2020
jonatack closed this on Oct 24, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#19405 rpc, cli: add network in/out connections to `getnetworkinfo` and `-getinfo`