1258@@ -1260,7 +1259,11 @@ RPCHelpMan getblockchaininfo()
1259 {RPCResult::Type::NUM, "pruneheight", /*optional=*/true, "height of the last block pruned, plus one (only present if pruning is enabled)"},
1260 {RPCResult::Type::BOOL, "automatic_pruning", /*optional=*/true, "whether automatic pruning is enabled (only present if pruning is enabled)"},
1261 {RPCResult::Type::NUM, "prune_target_size", /*optional=*/true, "the target size used by pruning (only present if automatic pruning is enabled)"},
1262- {RPCResult::Type::STR, "warnings", "any network and blockchain warnings"},
1263+ {RPCResult::Type::ARR, "warnings", "any network and blockchain warnings",
We should stop gratuitously breaking the RPC interface by changing existing field types like this.
What do you suggest? Leave things as is? Concatenate everything into a single string? Both seem less desirable than just fixing the interface, imo. (I use the term fix since the RPC behaves different to how it is documented).
~Another option is to keep warnings a concatenated STR of all messages , add a all_warnings ARR (open to better name suggestions) field, mark warnings as deprecated and remove it in v29?~
edit: that’s just a worse way of doing -deprecatedrpc=warnings
Another option is to keep warnings a concatenated STR of all messages , add a all_warnings ARR (open to better name suggestions) field, mark warnings as deprecated and remove it in v29?
Yes, actually. We did this for various warning fields in the wallet in #27279.
Yes, that’s usually how these changes are done: add a new field with the new format and a new name, keep or emulate the old format (eg concatenate the warnings), deprecate the old field after a few releases.
I don’t think adding a new field makes sense here.
warnings is the best name, unlike
#27279 where we upgraded from
warning to
warnings, this PR is more of a bugfix. Is it fair to assume you’d both be happy with keeping default behaviour as it is in this PR but allowing users to revert
warnings to a (concatenated) string with a
-deprecatedrpc=warnings option, until we remove it in some future version?
i agree that introducing a new name is unfortunate here, as “warnings” is consistent with naming for the wallet warnings
a deprecatedrpc flag that switches the format would work, i suppose
-deprecatedrpc=warnings is acceptable, but not ideal
I’ve added a -rpcdeprecated=warnings config option and updated the release notes.
I’d say that a new field, named node_warnings would also be acceptable. There shouldn’t be a need to keep the name in sync with the wallet warnings, as the wallet warnings are of a different topic than the node warnings.
But happy to re-ACK either approach.
