I think that the RPC result structure (i.e. the potentially nested JSON types) should not depend on the return value, as this makes the deserialization from json to a language-specific struct harder.
For example, gettransaction has a key "generated", that can never be false. It either exists (and is true) or does not exists (and is implicitly false).
generated (and other fields in gettransaction) should be changed to always return the value.
Other fields in gettransaction not always present are e.g. blockhash, blockindex and blocktime – they only exist for confirmed transactions, for unconfirmed the trusted field is generated instead (snippet starts one line below yours about generated, by the way):
So it doesn’t seem too trivial to solve this (what values should blockhash, blockindex and blocktime get for unconfirmed transactions?). Or is it enough for a first step to approach this just for boolean values?
what values should blockhash, blockindex and blocktime get for unconfirmed transactions?
I think it is fine to solve it by mentioning it in the help text, which is missing right now.
invovesWatchonly should probably not be included for non-watchonly wallets, but always be included for watchonly wallets. See also slightly related #16383 which changed the fallbacks for RPC inputs when the wallet is watchonly.
.pushKV() calls anymore), to get rid of all “only present if….” fields?
blockhash to a JSON-None value to keep the structure static. This will make the RPC response slightly larger, but is preferable I think.
You can set
blockhashto a JSON-None value to keep the structure static. This will make the RPC response slightly larger, but is preferable I think.
That would make sense. That “JSON-None value” can be created with the empty constructur of UniValue UniValue(), leading to null, as I just found out (as information for anyone wanting to work on this, I’m not sure if I will have time before end of the month).
What confused me was this part:
Similarly,
invovesWatchonlyshould probably not be included for non-watchonly wallets, but always be included for watchonly wallets.
Shouldn’t this field involvesWatchonly also always be generated? Right now it is only add to the response if it is true.
getpeerinfo.
I think response schemas dependent of the actual result are fine. Missing keys and null values are things that any JSON RPC client must handle. So it should be fine especially if the result is well documented (and tested).
But I also don’t mind otherwise. My concern is that we can’t go thru all RPC and make this change lightly - clients are expecting missing keys and we should avoid breaking changes. So we will end up with a non-consistent API anyway. For this reason I also don’t think this is a good first issue.
Concept ACK
I fully agree that static response structures make the lives of people developing against the RPC easier. While responses may be heavier we can overall probably reduce the number of LoC and making this a general rule prevents future discussions whether certain RPC return values could be made conditional, which mostly seems like a waste of energy IMO.
I fully agree that static response structures make the lives of people developing against the RPC easier.
I think the key advantage of static structures is reduced ambiguity. If field is set conditionally and absent, it’s ambiguous whether the field has no value, or the backend just doesn’t support returning it. It can make it harder than it needs to be write an RPC client that isn’t tied to specific bitcoind version.
clients are expecting missing keys and we should avoid breaking changes. So we will end up with a non-consistent API anyway. For this reason I also don’t think this is a good first issue.
Good point @promag . Removed “good first issue” for now.
My view is that well-documented cases where values are conditionally present based on other fields are fine (e.g. getaddressinfo only contains the sub-structure that recurses into p2sh when it also indicates you have a p2sh address). That avoids the issue @ryanofsky brings up above where you don’t know if something is not present because the software doesn’t support it, or because it is intended to be missing.
In other words: I think it would be ideal that fields being missing or not itself should never be used to convey information. But that does not mean that all responses should be completely static.