Merge bitcoin#29845: rpc: return warnings as an array instead of just a single one

42fb531 rpc: return warnings as an array instead of just a single one (stickies-v)

Pull request description:

  The RPC documentation for `getblockchaininfo`, `getmininginfo` and `getnetworkinfo` states that "warnings" returns "any network and blockchain warnings". In practice, only a single warning (i.e. the latest one that is set) is returned, the other ones are ignored.

  Fix that by returning all warnings as an array.

  As a side benefit, clean up the GetWarnings() logic.

  Since this PR changes the RPC result schema, I've added release notes. Users can temporarily revert to the old results by using `-deprecatedrpc=warnings`, until it's removed in a future version.

  ---

  Some historical context from git log:

  - when `GetWarnings` was introduced in 4019262, it was used in the `getinfo` RPC, where only a [single error/warning was returned](bitcoin@4019262#diff-7442c48d42cd5455a79915a0f00cce5e13359db46437a32b812876edb0a5ccddR250) (similar to how it is now).
  - later on, "warnings" RPC response fields were introduced, e.g. in ef2a3de, with the description [stating](bitcoin@ef2a3de#diff-1021bd3c74415ad9719bd764ad6ca35af5dfb33b1cd863c0be49bdf52518af54R411) that it returned "any network warnings" but in practice still only a single warning was returned

ACKs for top commit:
  achow101:
    re-ACK 42fb531
  tdb3:
    Re ACK for 42fb531
  TheCharlatan:
    ACK 42fb531
  maflcko:
    ACK 42fb531 🔺

Tree-SHA512: 4225ed8979cd5f030dec785a80e7452a041ad5703445da79d2906ada983ed0bbe7b15889d663d75aae4a77d92e302c93e93eca185c7bd47c9cce29e12f752bd3

Author: achow101

doc/release-notes-29845.md

@@ -0,0 +1,8 @@
+RPC
+---
+
+- the `warnings` field in `getblockchaininfo`, `getmininginfo` and
+  `getnetworkinfo` now returns all the active node warnings as an array
+  of strings, instead of just a single warning. The current behaviour
+  can temporarily be restored by running bitcoind with configuration
+  option `-deprecatedrpc=warnings`.

src/node/interfaces.cpp

@@ -47,6 +47,7 @@
 #include <util/check.h>
 #include <util/result.h>
 #include <util/signalinterrupt.h>
+#include <util/string.h>
 #include <util/translation.h>
 #include <validation.h>
 #include <validationinterface.h>
@@ -91,7 +92,7 @@ class NodeImpl : public Node
     explicit NodeImpl(NodeContext& context) { setContext(&context); }
     void initLogging() override { InitLogging(args()); }
     void initParameterInteraction() override { InitParameterInteraction(args()); }
-    bilingual_str getWarnings() override { return GetWarnings(true); }
+    bilingual_str getWarnings() override { return Join(GetWarnings(), Untranslated("<hr />")); }
     int getExitStatus() override { return Assert(m_context)->exit_status.load(); }
     uint32_t getLogCategories() override { return LogInstance().GetCategoryMask(); }
     bool baseInitialize() override

src/rpc/blockchain.cpp

@@ -47,7 +47,6 @@
 #include <validation.h>
 #include <validationinterface.h>
 #include <versionbits.h>
-#include <warnings.h>
 
 #include <stdint.h>
 
@@ -1260,7 +1259,14 @@ RPCHelpMan getblockchaininfo()
                 {RPCResult::Type::NUM, "pruneheight", /*optional=*/true, "height of the last block pruned, plus one (only present if pruning is enabled)"},
                 {RPCResult::Type::BOOL, "automatic_pruning", /*optional=*/true, "whether automatic pruning is enabled (only present if pruning is enabled)"},
                 {RPCResult::Type::NUM, "prune_target_size", /*optional=*/true, "the target size used by pruning (only present if automatic pruning is enabled)"},
-                {RPCResult::Type::STR, "warnings", "any network and blockchain warnings"},
+                (IsDeprecatedRPCEnabled("warnings") ?
+                    RPCResult{RPCResult::Type::STR, "warnings", "any network and blockchain warnings (DEPRECATED)"} :
+                    RPCResult{RPCResult::Type::ARR, "warnings", "any network and blockchain warnings (run with `-deprecatedrpc=warnings` to return the latest warning as a single string)",
+                    {
+                        {RPCResult::Type::STR, "", "warning"},
+                    }
+                    }
+                ),
             }},
         RPCExamples{
             HelpExampleCli("getblockchaininfo", "")
@@ -1298,7 +1304,7 @@ RPCHelpMan getblockchaininfo()
         }
     }
 
-    obj.pushKV("warnings", GetWarnings(false).original);
+    obj.pushKV("warnings", GetNodeWarnings(IsDeprecatedRPCEnabled("warnings")));
     return obj;
 },
     };

src/rpc/mining.cpp

@@ -39,7 +39,6 @@
 #include <util/translation.h>
 #include <validation.h>
 #include <validationinterface.h>
-#include <warnings.h>
 
 #include <memory>
 #include <stdint.h>
@@ -426,7 +425,14 @@ static RPCHelpMan getmininginfo()
                         {RPCResult::Type::NUM, "networkhashps", "The network hashes per second"},
                         {RPCResult::Type::NUM, "pooledtx", "The size of the mempool"},
                         {RPCResult::Type::STR, "chain", "current network name (main, test, signet, regtest)"},
-                        {RPCResult::Type::STR, "warnings", "any network and blockchain warnings"},
+                        (IsDeprecatedRPCEnabled("warnings") ?
+                            RPCResult{RPCResult::Type::STR, "warnings", "any network and blockchain warnings (DEPRECATED)"} :
+                            RPCResult{RPCResult::Type::ARR, "warnings", "any network and blockchain warnings (run with `-deprecatedrpc=warnings` to return the latest warning as a single string)",
+                            {
+                                {RPCResult::Type::STR, "", "warning"},
+                            }
+                            }
+                        ),
                     }},
                 RPCExamples{
                     HelpExampleCli("getmininginfo", "")
@@ -448,7 +454,7 @@ static RPCHelpMan getmininginfo()
     obj.pushKV("networkhashps",    getnetworkhashps().HandleRequest(request));
     obj.pushKV("pooledtx",         (uint64_t)mempool.size());
     obj.pushKV("chain", chainman.GetParams().GetChainTypeString());
-    obj.pushKV("warnings",         GetWarnings(false).original);
+    obj.pushKV("warnings", GetNodeWarnings(IsDeprecatedRPCEnabled("warnings")));
     return obj;
 },
     };

src/rpc/net.cpp

@@ -29,7 +29,6 @@
 #include <util/time.h>
 #include <util/translation.h>
 #include <validation.h>
-#include <warnings.h>
 
 #include <optional>
 
@@ -657,7 +656,14 @@ static RPCHelpMan getnetworkinfo()
                                 {RPCResult::Type::NUM, "score", "relative score"},
                             }},
                         }},
-                        {RPCResult::Type::STR, "warnings", "any network and blockchain warnings"},
+                        (IsDeprecatedRPCEnabled("warnings") ?
+                            RPCResult{RPCResult::Type::STR, "warnings", "any network and blockchain warnings (DEPRECATED)"} :
+                            RPCResult{RPCResult::Type::ARR, "warnings", "any network and blockchain warnings (run with `-deprecatedrpc=warnings` to return the latest warning as a single string)",
+                            {
+                                {RPCResult::Type::STR, "", "warning"},
+                            }
+                            }
+                        ),
                     }
                 },
                 RPCExamples{
@@ -707,7 +713,7 @@ static RPCHelpMan getnetworkinfo()
         }
     }
     obj.pushKV("localaddresses", localAddresses);
-    obj.pushKV("warnings",       GetWarnings(false).original);
+    obj.pushKV("warnings", GetNodeWarnings(IsDeprecatedRPCEnabled("warnings")));
     return obj;
 },
     };

src/rpc/util.cpp

@@ -18,16 +18,19 @@
 #include <script/signingprovider.h>
 #include <script/solver.h>
 #include <tinyformat.h>
+#include <univalue.h>
 #include <util/check.h>
 #include <util/result.h>
 #include <util/strencodings.h>
 #include <util/string.h>
 #include <util/translation.h>
+#include <warnings.h>
 
 #include <algorithm>
 #include <iterator>
 #include <string_view>
 #include <tuple>
+#include <utility>
 
 const std::string UNIX_EPOCH_TIME = "UNIX epoch time";
 const std::string EXAMPLE_ADDRESS[2] = {"bc1q09vm5lfy0j5reeulh4x5752q25uqqvz34hufdl", "bc1q02ad21edsxd23d32dfgqqsz4vv4nmtfzuklhy3"};
@@ -1357,3 +1360,17 @@ void PushWarnings(const std::vector<bilingual_str>& warnings, UniValue& obj)
     if (warnings.empty()) return;
     obj.pushKV("warnings", BilingualStringsToUniValue(warnings));
 }
+
+UniValue GetNodeWarnings(bool use_deprecated)
+{
+    if (use_deprecated) {
+        const auto all_warnings{GetWarnings()};
+        return all_warnings.empty() ? "" : all_warnings.back().original;
+    }
+
+    UniValue warnings{UniValue::VARR};
+    for (auto&& warning : GetWarnings()) {
+        warnings.push_back(std::move(warning.original));
+    }
+    return warnings;
+}

src/rpc/util.h

@@ -513,4 +513,6 @@ class RPCHelpMan
 void PushWarnings(const UniValue& warnings, UniValue& obj);
 void PushWarnings(const std::vector<bilingual_str>& warnings, UniValue& obj);
 
+UniValue GetNodeWarnings(bool use_deprecated);
+
 #endif // BITCOIN_RPC_UTIL_H

src/warnings.cpp

@@ -11,7 +11,6 @@
 
 #include <common/system.h>
 #include <sync.h>
-#include <util/string.h>
 #include <util/translation.h>
 
 #include <optional>
@@ -39,37 +38,30 @@ void SetMedianTimeOffsetWarning(std::optional<bilingual_str> warning)
     LOCK(g_warnings_mutex);
     g_timeoffset_warning = warning;
 }
-bilingual_str GetWarnings(bool verbose)
+
+std::vector<bilingual_str> GetWarnings()
 {
-    bilingual_str warnings_concise;
-    std::vector<bilingual_str> warnings_verbose;
+    std::vector<bilingual_str> warnings;
 
     LOCK(g_warnings_mutex);
 
     // Pre-release build warning
     if (!CLIENT_VERSION_IS_RELEASE) {
-        warnings_concise = _("This is a pre-release test build - use at your own risk - do not use for mining or merchant applications");
-        warnings_verbose.emplace_back(warnings_concise);
+        warnings.emplace_back(_("This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"));
     }
 
     // Misc warnings like out of disk space and clock is wrong
     if (!g_misc_warnings.empty()) {
-        warnings_concise = g_misc_warnings;
-        warnings_verbose.emplace_back(warnings_concise);
+        warnings.emplace_back(g_misc_warnings);
     }
 
     if (fLargeWorkInvalidChainFound) {
-        warnings_concise = _("Warning: We do not appear to fully agree with our peers! You may need to upgrade, or other nodes may need to upgrade.");
-        warnings_verbose.emplace_back(warnings_concise);
+        warnings.emplace_back(_("Warning: We do not appear to fully agree with our peers! You may need to upgrade, or other nodes may need to upgrade."));
     }
 
     if (g_timeoffset_warning) {
-        warnings_verbose.emplace_back(g_timeoffset_warning.value());
-    }
-
-    if (verbose) {
-        return Join(warnings_verbose, Untranslated("<hr />"));
+        warnings.emplace_back(g_timeoffset_warning.value());
     }
 
-    return warnings_concise;
+    return warnings;
 }

src/warnings.h

@@ -8,19 +8,15 @@
 
 #include <optional>
 #include <string>
+#include <vector>
 
 struct bilingual_str;
 
 void SetMiscWarning(const bilingual_str& warning);
 void SetfLargeWorkInvalidChainFound(bool flag);
 /** Pass std::nullopt to disable the warning */
 void SetMedianTimeOffsetWarning(std::optional<bilingual_str> warning);
-/** Format a string that describes several potential problems detected by the core.
- * @param[in] verbose bool
- * - if true, get all warnings separated by <hr />
- * - if false, get the most important warning
- * @returns the warning string
- */
-bilingual_str GetWarnings(bool verbose);
+/** Return potential problems detected by the node. */
+std::vector<bilingual_str> GetWarnings();
 
 #endif // BITCOIN_WARNINGS_H

test/functional/feature_versionbits_warning.py

@@ -73,8 +73,8 @@ def run_test(self):
         self.generatetoaddress(node, VB_PERIOD - VB_THRESHOLD + 1, node_deterministic_address)
 
         # Check that we're not getting any versionbit-related errors in get*info()
-        assert not VB_PATTERN.match(node.getmininginfo()["warnings"])
-        assert not VB_PATTERN.match(node.getnetworkinfo()["warnings"])
+        assert not VB_PATTERN.match(",".join(node.getmininginfo()["warnings"]))
+        assert not VB_PATTERN.match(",".join(node.getnetworkinfo()["warnings"]))
 
         # Build one period of blocks with VB_THRESHOLD blocks signaling some unknown bit
         self.send_blocks_with_version(peer, VB_THRESHOLD, VB_UNKNOWN_VERSION)
@@ -94,8 +94,8 @@ def run_test(self):
         # Generating one more block will be enough to generate an error.
         self.generatetoaddress(node, 1, node_deterministic_address)
         # Check that get*info() shows the versionbits unknown rules warning
-        assert WARN_UNKNOWN_RULES_ACTIVE in node.getmininginfo()["warnings"]
-        assert WARN_UNKNOWN_RULES_ACTIVE in node.getnetworkinfo()["warnings"]
+        assert WARN_UNKNOWN_RULES_ACTIVE in ",".join(node.getmininginfo()["warnings"])
+        assert WARN_UNKNOWN_RULES_ACTIVE in ",".join(node.getnetworkinfo()["warnings"])
         # Check that the alert file shows the versionbits unknown rules warning
         self.wait_until(lambda: self.versionbits_in_alert_file())