Merge #19176: refactor: Error message bilingual_str consistency

6fe989054f refactor: Change Node::initError to take bilingual_str (Wladimir J. van der Laan)
425e7cb8cf refactor: Put`TryParsePermissionFlags` in anonymous namespace (Wladimir J. van der Laan)
77b79fa6ef refactor: Error message bilingual_str consistency (Wladimir J. van der Laan)

Pull request description:

  A straightforward and hopefully uncontroversial refactor to improve consistency.

  - Move the decision whether to translate an individual error message to where it is defined. This simplifies call sites: no more `InitError(Untranslated(SomeFunction(...)))`.

  - Make all functions in `util/error.h` consistently return a   `bilingual_str`. We've decided to use this as error message type so let's roll with it.

  This has no functional changes: no messages are changed, no new translation messages are defined.

  Also make a function static that can be static.

ACKs for top commit:
  MarcoFalke:
    ACK 6fe989054f 🔣
  hebasto:
    ACK 6fe989054f, tested on Linux Mint 19.3 (x86_64).

Tree-SHA512: 1dd123ef285c4b50bbc429b2f11c9a63aaa669a84955a0a9b8134e9dc141bc38f863f798e8982ac68bbe83170e1067a87d1a87fe7f791928b7914e10bbc2ef8d

src/init.cpp

@@ -1465,7 +1465,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
         if (Lookup(strAddr, addrLocal, GetListenPort(), fNameLookup) && addrLocal.IsValid())
             AddLocal(addrLocal, LOCAL_MANUAL);
         else
-            return InitError(Untranslated(ResolveErrMsg("externalip", strAddr)));
+            return InitError(ResolveErrMsg("externalip", strAddr));
     }
 
     // Read asmap file if configured
@@ -1904,21 +1904,21 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
     for (const std::string& strBind : gArgs.GetArgs("-bind")) {
         CService addrBind;
         if (!Lookup(strBind, addrBind, GetListenPort(), false)) {
-            return InitError(Untranslated(ResolveErrMsg("bind", strBind)));
+            return InitError(ResolveErrMsg("bind", strBind));
         }
         connOptions.vBinds.push_back(addrBind);
     }
     for (const std::string& strBind : gArgs.GetArgs("-whitebind")) {
         NetWhitebindPermissions whitebind;
-        std::string error;
+        bilingual_str error;
-        if (!NetWhitebindPermissions::TryParse(strBind, whitebind, error)) return InitError(Untranslated(error));
+        if (!NetWhitebindPermissions::TryParse(strBind, whitebind, error)) return InitError(error);
         connOptions.vWhiteBinds.push_back(whitebind);
     }
 
     for (const auto& net : gArgs.GetArgs("-whitelist")) {
         NetWhitelistPermissions subnet;
-        std::string error;
+        bilingual_str error;
-        if (!NetWhitelistPermissions::TryParse(net, subnet, error)) return InitError(Untranslated(error));
+        if (!NetWhitelistPermissions::TryParse(net, subnet, error)) return InitError(error);
         connOptions.vWhitelistedRange.push_back(subnet);
     }
 

src/interfaces/node.cpp

@@ -56,7 +56,7 @@ namespace {
 class NodeImpl : public Node
 {
 public:
-    void initError(const std::string& message) override { InitError(Untranslated(message)); }
+    void initError(const bilingual_str& message) override { InitError(message); }
     bool parseParameters(int argc, const char* const argv[], std::string& error) override
     {
         return gArgs.ParseParameters(argc, argv, error);

src/interfaces/node.h

@@ -45,7 +45,7 @@ public:
     virtual ~Node() {}
 
     //! Send init error.
-    virtual void initError(const std::string& message) = 0;
+    virtual void initError(const bilingual_str& message) = 0;
 
     //! Set command line arguments.
     virtual bool parseParameters(int argc, const char* const argv[], std::string& error) = 0;

src/net_permissions.cpp

@@ -16,8 +16,10 @@ const std::vector<std::string> NET_PERMISSIONS_DOC{
     "mempool (allow requesting BIP35 mempool contents)",
 };
 
+namespace {
+
 // The parse the following format "perm1,perm2@xxxxxx"
-bool TryParsePermissionFlags(const std::string str, NetPermissionFlags& output, size_t& readen, std::string& error)
+bool TryParsePermissionFlags(const std::string str, NetPermissionFlags& output, size_t& readen, bilingual_str& error)
 {
     NetPermissionFlags flags = PF_NONE;
     const auto atSeparator = str.find('@');
@@ -48,7 +50,7 @@ bool TryParsePermissionFlags(const std::string str, NetPermissionFlags& output,
             else if (permission == "relay") NetPermissions::AddFlag(flags, PF_RELAY);
             else if (permission.length() == 0); // Allow empty entries
             else {
-                error = strprintf(_("Invalid P2P permission: '%s'").translated, permission);
+                error = strprintf(_("Invalid P2P permission: '%s'"), permission);
                 return false;
             }
         }
@@ -56,10 +58,12 @@ bool TryParsePermissionFlags(const std::string str, NetPermissionFlags& output,
     }
 
     output = flags;
-    error = "";
+    error = Untranslated("");
     return true;
 }
 
+}
+
 std::vector<std::string> NetPermissions::ToStrings(NetPermissionFlags flags)
 {
     std::vector<std::string> strings;
@@ -71,7 +75,7 @@ std::vector<std::string> NetPermissions::ToStrings(NetPermissionFlags flags)
     return strings;
 }
 
-bool NetWhitebindPermissions::TryParse(const std::string str, NetWhitebindPermissions& output, std::string& error)
+bool NetWhitebindPermissions::TryParse(const std::string str, NetWhitebindPermissions& output, bilingual_str& error)
 {
     NetPermissionFlags flags;
     size_t offset;
@@ -84,17 +88,17 @@ bool NetWhitebindPermissions::TryParse(const std::string str, NetWhitebindPermis
         return false;
     }
     if (addrBind.GetPort() == 0) {
-        error = strprintf(_("Need to specify a port with -whitebind: '%s'").translated, strBind);
+        error = strprintf(_("Need to specify a port with -whitebind: '%s'"), strBind);
         return false;
     }
 
     output.m_flags = flags;
     output.m_service = addrBind;
-    error = "";
+    error = Untranslated("");
     return true;
 }
 
-bool NetWhitelistPermissions::TryParse(const std::string str, NetWhitelistPermissions& output, std::string& error)
+bool NetWhitelistPermissions::TryParse(const std::string str, NetWhitelistPermissions& output, bilingual_str& error)
 {
     NetPermissionFlags flags;
     size_t offset;
@@ -104,12 +108,12 @@ bool NetWhitelistPermissions::TryParse(const std::string str, NetWhitelistPermis
     CSubNet subnet;
     LookupSubNet(net, subnet);
     if (!subnet.IsValid()) {
-        error = strprintf(_("Invalid netmask specified in -whitelist: '%s'").translated, net);
+        error = strprintf(_("Invalid netmask specified in -whitelist: '%s'"), net);
         return false;
     }
 
     output.m_flags = flags;
     output.m_subnet = subnet;
-    error = "";
+    error = Untranslated("");
     return true;
 }

src/net_permissions.h

@@ -10,6 +10,8 @@
 #ifndef BITCOIN_NET_PERMISSIONS_H
 #define BITCOIN_NET_PERMISSIONS_H
 
+struct bilingual_str;
+
 extern const std::vector<std::string> NET_PERMISSIONS_DOC;
 
 enum NetPermissionFlags
@@ -54,14 +56,14 @@ public:
 class NetWhitebindPermissions : public NetPermissions
 {
 public:
-    static bool TryParse(const std::string str, NetWhitebindPermissions& output, std::string& error);
+    static bool TryParse(const std::string str, NetWhitebindPermissions& output, bilingual_str& error);
     CService m_service;
 };
 
 class NetWhitelistPermissions : public NetPermissions
 {
 public:
-    static bool TryParse(const std::string str, NetWhitelistPermissions& output, std::string& error);
+    static bool TryParse(const std::string str, NetWhitelistPermissions& output, bilingual_str& error);
     CSubNet m_subnet;
 };
 

src/qt/bitcoin.cpp

@@ -457,7 +457,7 @@ int GuiMain(int argc, char* argv[])
     SetupUIArgs();
     std::string error;
     if (!node->parseParameters(argc, argv, error)) {
-        node->initError(strprintf("Error parsing command line arguments: %s\n", error));
+        node->initError(strprintf(Untranslated("Error parsing command line arguments: %s\n"), error));
         // Create a message box, because the gui has neither been created nor has subscribed to core signals
         QMessageBox::critical(nullptr, PACKAGE_NAME,
             // message can not be translated because translations have not been initialized
@@ -498,13 +498,13 @@ int GuiMain(int argc, char* argv[])
     /// 6. Determine availability of data directory and parse bitcoin.conf
     /// - Do not call GetDataDir(true) before this step finishes
     if (!CheckDataDirOption()) {
-        node->initError(strprintf("Specified data directory \"%s\" does not exist.\n", gArgs.GetArg("-datadir", "")));
+        node->initError(strprintf(Untranslated("Specified data directory \"%s\" does not exist.\n"), gArgs.GetArg("-datadir", "")));
         QMessageBox::critical(nullptr, PACKAGE_NAME,
             QObject::tr("Error: Specified data directory \"%1\" does not exist.").arg(QString::fromStdString(gArgs.GetArg("-datadir", ""))));
         return EXIT_FAILURE;
     }
     if (!node->readConfigFiles(error)) {
-        node->initError(strprintf("Error reading configuration file: %s\n", error));
+        node->initError(strprintf(Untranslated("Error reading configuration file: %s\n"), error));
         QMessageBox::critical(nullptr, PACKAGE_NAME,
             QObject::tr("Error: Cannot parse configuration file: %1.").arg(QString::fromStdString(error)));
         return EXIT_FAILURE;
@@ -520,7 +520,7 @@ int GuiMain(int argc, char* argv[])
     try {
         node->selectParams(gArgs.GetChainName());
     } catch(std::exception &e) {
-        node->initError(strprintf("%s\n", e.what()));
+        node->initError(Untranslated(strprintf("%s\n", e.what())));
         QMessageBox::critical(nullptr, PACKAGE_NAME, QObject::tr("Error: %1").arg(e.what()));
         return EXIT_FAILURE;
     }

src/rpc/util.cpp

@@ -10,6 +10,7 @@
 #include <tinyformat.h>
 #include <util/strencodings.h>
 #include <util/string.h>
+#include <util/translation.h>
 
 #include <tuple>
 
@@ -285,7 +286,7 @@ UniValue JSONRPCTransactionError(TransactionError terr, const std::string& err_s
     if (err_string.length() > 0) {
         return JSONRPCError(RPCErrorFromTransactionError(terr), err_string);
     } else {
-        return JSONRPCError(RPCErrorFromTransactionError(terr), TransactionErrorString(terr));
+        return JSONRPCError(RPCErrorFromTransactionError(terr), TransactionErrorString(terr).original);
     }
 }
 

src/test/fuzz/kitchen_sink.cpp

@@ -7,6 +7,7 @@
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
 #include <util/error.h>
+#include <util/translation.h>
 
 #include <cstdint>
 #include <vector>

src/test/fuzz/net_permissions.cpp

@@ -6,6 +6,7 @@
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
+#include <util/translation.h>
 
 #include <cassert>
 #include <cstdint>
@@ -29,7 +30,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
                                                                                          static_cast<NetPermissionFlags>(fuzzed_data_provider.ConsumeIntegral<uint32_t>());
 
     NetWhitebindPermissions net_whitebind_permissions;
-    std::string error_net_whitebind_permissions;
+    bilingual_str error_net_whitebind_permissions;
     if (NetWhitebindPermissions::TryParse(s, net_whitebind_permissions, error_net_whitebind_permissions)) {
         (void)NetPermissions::ToStrings(net_whitebind_permissions.m_flags);
         (void)NetPermissions::AddFlag(net_whitebind_permissions.m_flags, net_permission_flags);
@@ -39,7 +40,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
     }
 
     NetWhitelistPermissions net_whitelist_permissions;
-    std::string error_net_whitelist_permissions;
+    bilingual_str error_net_whitelist_permissions;
     if (NetWhitelistPermissions::TryParse(s, net_whitelist_permissions, error_net_whitelist_permissions)) {
         (void)NetPermissions::ToStrings(net_whitelist_permissions.m_flags);
         (void)NetPermissions::AddFlag(net_whitelist_permissions.m_flags, net_permission_flags);

src/test/netbase_tests.cpp

@@ -6,6 +6,7 @@
 #include <netbase.h>
 #include <test/util/setup_common.h>
 #include <util/strencodings.h>
+#include <util/translation.h>
 
 #include <string>
 
@@ -325,15 +326,15 @@ BOOST_AUTO_TEST_CASE(netbase_parsenetwork)
 
 BOOST_AUTO_TEST_CASE(netpermissions_test)
 {
-    std::string error;
+    bilingual_str error;
     NetWhitebindPermissions whitebindPermissions;
     NetWhitelistPermissions whitelistPermissions;
 
     // Detect invalid white bind
     BOOST_CHECK(!NetWhitebindPermissions::TryParse("", whitebindPermissions, error));
-    BOOST_CHECK(error.find("Cannot resolve -whitebind address") != std::string::npos);
+    BOOST_CHECK(error.original.find("Cannot resolve -whitebind address") != std::string::npos);
     BOOST_CHECK(!NetWhitebindPermissions::TryParse("127.0.0.1", whitebindPermissions, error));
-    BOOST_CHECK(error.find("Need to specify a port with -whitebind") != std::string::npos);
+    BOOST_CHECK(error.original.find("Need to specify a port with -whitebind") != std::string::npos);
     BOOST_CHECK(!NetWhitebindPermissions::TryParse("", whitebindPermissions, error));
 
     // If no permission flags, assume backward compatibility
@@ -377,11 +378,11 @@ BOOST_AUTO_TEST_CASE(netpermissions_test)
 
     // Detect invalid flag
     BOOST_CHECK(!NetWhitebindPermissions::TryParse("bloom,forcerelay,oopsie@1.2.3.4:32", whitebindPermissions, error));
-    BOOST_CHECK(error.find("Invalid P2P permission") != std::string::npos);
+    BOOST_CHECK(error.original.find("Invalid P2P permission") != std::string::npos);
 
     // Check whitelist error
     BOOST_CHECK(!NetWhitelistPermissions::TryParse("bloom,forcerelay,noban@1.2.3.4:32", whitelistPermissions, error));
-    BOOST_CHECK(error.find("Invalid netmask specified in -whitelist") != std::string::npos);
+    BOOST_CHECK(error.original.find("Invalid netmask specified in -whitelist") != std::string::npos);
 
     // Happy path for whitelist parsing
     BOOST_CHECK(NetWhitelistPermissions::TryParse("noban@1.2.3.4", whitelistPermissions, error));

src/util/error.cpp

@@ -8,37 +8,37 @@
 #include <util/system.h>
 #include <util/translation.h>
 
-std::string TransactionErrorString(const TransactionError err)
+bilingual_str TransactionErrorString(const TransactionError err)
 {
     switch (err) {
         case TransactionError::OK:
-            return "No error";
+            return Untranslated("No error");
         case TransactionError::MISSING_INPUTS:
-            return "Missing inputs";
+            return Untranslated("Missing inputs");
         case TransactionError::ALREADY_IN_CHAIN:
-            return "Transaction already in block chain";
+            return Untranslated("Transaction already in block chain");
         case TransactionError::P2P_DISABLED:
-            return "Peer-to-peer functionality missing or disabled";
+            return Untranslated("Peer-to-peer functionality missing or disabled");
         case TransactionError::MEMPOOL_REJECTED:
-            return "Transaction rejected by AcceptToMemoryPool";
+            return Untranslated("Transaction rejected by AcceptToMemoryPool");
         case TransactionError::MEMPOOL_ERROR:
-            return "AcceptToMemoryPool failed";
+            return Untranslated("AcceptToMemoryPool failed");
         case TransactionError::INVALID_PSBT:
-            return "PSBT is not sane";
+            return Untranslated("PSBT is not sane");
         case TransactionError::PSBT_MISMATCH:
-            return "PSBTs not compatible (different transactions)";
+            return Untranslated("PSBTs not compatible (different transactions)");
         case TransactionError::SIGHASH_MISMATCH:
-            return "Specified sighash value does not match existing value";
+            return Untranslated("Specified sighash value does not match existing value");
         case TransactionError::MAX_FEE_EXCEEDED:
-            return "Fee exceeds maximum configured by -maxtxfee";
+            return Untranslated("Fee exceeds maximum configured by -maxtxfee");
         // no default case, so the compiler can warn about missing cases
     }
     assert(false);
 }
 
-std::string ResolveErrMsg(const std::string& optname, const std::string& strBind)
+bilingual_str ResolveErrMsg(const std::string& optname, const std::string& strBind)
 {
-    return strprintf(_("Cannot resolve -%s address: '%s'").translated, optname, strBind);
+    return strprintf(_("Cannot resolve -%s address: '%s'"), optname, strBind);
 }
 
 bilingual_str AmountHighWarn(const std::string& optname)

src/util/error.h

@@ -32,9 +32,9 @@ enum class TransactionError {
     MAX_FEE_EXCEEDED,
 };
 
-std::string TransactionErrorString(const TransactionError error);
+bilingual_str TransactionErrorString(const TransactionError error);
 
-std::string ResolveErrMsg(const std::string& optname, const std::string& strBind);
+bilingual_str ResolveErrMsg(const std::string& optname, const std::string& strBind);
 
 bilingual_str AmountHighWarn(const std::string& optname);
 

src/wallet/wallet.cpp

@@ -2997,7 +2997,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac
     }
 
     if (nFeeRet > m_default_max_tx_fee) {
-        error = Untranslated(TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED));
+        error = TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED);
         return false;
     }