refactor: Replace std::optional<bilingual_str> with util::Result

ryanofsky · ryanofsky · commit 9d3918d47daf · 2022-09-01T15:35:23.000-04:00
diff --git a/src/addrdb.cpp b/src/addrdb.cpp
@@ -182,10 +182,10 @@ void ReadFromStream(AddrMan& addr, CDataStream& ssPeers)
     DeserializeDB(ssPeers, addr, false);
 }
 
-std::optional<bilingual_str> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args, std::unique_ptr<AddrMan>& addrman)
+util::Result<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args)
 {
     auto check_addrman = std::clamp<int32_t>(args.GetIntArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000);
-    addrman = std::make_unique<AddrMan>(netgroupman, /*deterministic=*/false, /*consistency_check_ratio=*/check_addrman);
+    auto addrman = std::make_unique<AddrMan>(netgroupman, /*deterministic=*/false, /*consistency_check_ratio=*/check_addrman);
 
     int64_t nStart = GetTimeMillis();
     const auto path_addr{args.GetDataDirNet() / "peers.dat"};
@@ -199,19 +199,17 @@ std::optional<bilingual_str> LoadAddrman(const NetGroupManager& netgroupman, con
         DumpPeerAddresses(args, *addrman);
     } catch (const InvalidAddrManVersionError&) {
         if (!RenameOver(path_addr, (fs::path)path_addr + ".bak")) {
-            addrman = nullptr;
-            return strprintf(_("Failed to rename invalid peers.dat file. Please move or delete it and try again."));
+            return util::Error{strprintf(_("Failed to rename invalid peers.dat file. Please move or delete it and try again."))};
         }
         // Addrman can be in an inconsistent state after failure, reset it
         addrman = std::make_unique<AddrMan>(netgroupman, /*deterministic=*/false, /*consistency_check_ratio=*/check_addrman);
         LogPrintf("Creating new peers.dat because the file version was not compatible (%s). Original backed up to peers.dat.bak\n", fs::quoted(fs::PathToString(path_addr)));
         DumpPeerAddresses(args, *addrman);
     } catch (const std::exception& e) {
-        addrman = nullptr;
-        return strprintf(_("Invalid or corrupt peers.dat (%s). If you believe this is a bug, please report it to %s. As a workaround, you can move the file (%s) out of the way (rename, move, or delete) to have a new one created on the next start."),
-                         e.what(), PACKAGE_BUGREPORT, fs::quoted(fs::PathToString(path_addr)));
+        return util::Error{strprintf(_("Invalid or corrupt peers.dat (%s). If you believe this is a bug, please report it to %s. As a workaround, you can move the file (%s) out of the way (rename, move, or delete) to have a new one created on the next start."),
+                                     e.what(), PACKAGE_BUGREPORT, fs::quoted(fs::PathToString(path_addr)))};
     }
-    return std::nullopt;
+    return addrman;
 }
 
 void DumpAnchors(const fs::path& anchors_db_path, const std::vector<CAddress>& anchors)
diff --git a/src/addrdb.h b/src/addrdb.h
@@ -9,6 +9,7 @@
 #include <fs.h>
 #include <net_types.h> // For banmap_t
 #include <univalue.h>
+#include <util/result.h>
 
 #include <optional>
 #include <vector>
@@ -49,7 +50,7 @@ class CBanDB
 };
 
 /** Returns an error string on failure */
-std::optional<bilingual_str> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args, std::unique_ptr<AddrMan>& addrman);
+util::Result<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgroupman, const ArgsManager& args);
 
 /**
  * Dump the anchor IP address database (anchors.dat)
diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
@@ -58,7 +58,7 @@ int main(int argc, char* argv[])
     // We can't use a goto here, but we can use an assert since none of the
     // things instantiated so far requires running the epilogue to be torn down
     // properly
-    assert(!kernel::SanityChecks(kernel_context).has_value());
+    assert(kernel::SanityChecks(kernel_context).has_value());
 
     // Necessary for CheckInputScripts (eventually called by ProcessNewBlock),
     // which will try the script cache first and fall back to actually
diff --git a/src/init.cpp b/src/init.cpp
@@ -1059,8 +1059,9 @@ static bool LockDataDirectory(bool probeOnly)
 bool AppInitSanityChecks(const kernel::Context& kernel)
 {
     // ********************************************************* Step 4: sanity checks
-    if (auto error = kernel::SanityChecks(kernel)) {
-        InitError(*error);
+    auto result = kernel::SanityChecks(kernel);
+    if (!result) {
+        InitError(util::ErrorString(result));
         return InitError(strprintf(_("Initialization sanity check failed. %s is shutting down."), PACKAGE_NAME));
     }
 
@@ -1236,9 +1237,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         // Initialize addrman
         assert(!node.addrman);
         uiInterface.InitMessage(_("Loading P2P addresses…").translated);
-        if (const auto error{LoadAddrman(*node.netgroupman, args, node.addrman)}) {
-            return InitError(*error);
-        }
+        auto addrman = LoadAddrman(*node.netgroupman, args);
+        if (!addrman) return InitError(util::ErrorString(addrman));
+        node.addrman = std::move(*addrman);
     }
 
     assert(!node.banman);
@@ -1386,8 +1387,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         .estimator = node.fee_estimator.get(),
         .check_ratio = chainparams.DefaultConsistencyChecks() ? 1 : 0,
     };
-    if (const auto err{ApplyArgsManOptions(args, chainparams, mempool_opts)}) {
-        return InitError(*err);
+    auto result = ApplyArgsManOptions(args, chainparams, mempool_opts);
+    if (!result) {
+        return InitError(util::ErrorString(result));
     }
     mempool_opts.check_ratio = std::clamp<int>(mempool_opts.check_ratio, 0, 1'000'000);
 
@@ -1487,8 +1489,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
     // ********************************************************* Step 8: start indexers
     if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
-        if (const auto error{WITH_LOCK(cs_main, return CheckLegacyTxindex(*Assert(chainman.m_blockman.m_block_tree_db)))}) {
-            return InitError(*error);
+        auto result = WITH_LOCK(cs_main, return CheckLegacyTxindex(*Assert(chainman.m_blockman.m_block_tree_db)));
+        if (!result) {
+            return InitError(util::ErrorString(result));
         }
 
         g_txindex = std::make_unique<TxIndex>(interfaces::MakeChain(node), cache_sizes.tx_index, false, fReindex);
diff --git a/src/kernel/checks.cpp b/src/kernel/checks.cpp
@@ -13,21 +13,21 @@
 
 namespace kernel {
 
-std::optional<bilingual_str> SanityChecks(const Context&)
+util::Result<void> SanityChecks(const Context&)
 {
     if (!ECC_InitSanityCheck()) {
-        return Untranslated("Elliptic curve cryptography sanity check failure. Aborting.");
+        return util::Error{Untranslated("Elliptic curve cryptography sanity check failure. Aborting.")};
     }
 
     if (!Random_SanityCheck()) {
-        return Untranslated("OS cryptographic RNG sanity check failure. Aborting.");
+        return util::Error{Untranslated("OS cryptographic RNG sanity check failure. Aborting.")};
     }
 
     if (!ChronoSanityCheck()) {
-        return Untranslated("Clock epoch mismatch. Aborting.");
+        return util::Error{Untranslated("Clock epoch mismatch. Aborting.")};
     }
 
-    return std::nullopt;
+    return {};
 }
 
 }
diff --git a/src/kernel/checks.h b/src/kernel/checks.h
@@ -5,7 +5,7 @@
 #ifndef BITCOIN_KERNEL_CHECKS_H
 #define BITCOIN_KERNEL_CHECKS_H
 
-#include <optional>
+#include <util/result.h>
 
 struct bilingual_str;
 
@@ -16,7 +16,7 @@ struct Context;
 /**
  *  Ensure a usable environment with all necessary library support.
  */
-std::optional<bilingual_str> SanityChecks(const Context&);
+util::Result<void> SanityChecks(const Context&);
 
 }
 
diff --git a/src/node/mempool_args.cpp b/src/node/mempool_args.cpp
@@ -38,7 +38,7 @@ void ApplyArgsManOptions(const ArgsManager& argsman, MemPoolLimits& mempool_limi
 }
 }
 
-std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& argsman, const CChainParams& chainparams, MemPoolOptions& mempool_opts)
+util::Result<void> ApplyArgsManOptions(const ArgsManager& argsman, const CChainParams& chainparams, MemPoolOptions& mempool_opts)
 {
     mempool_opts.check_ratio = argsman.GetIntArg("-checkmempool", mempool_opts.check_ratio);
 
@@ -52,7 +52,7 @@ std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& argsman, con
         if (std::optional<CAmount> inc_relay_fee = ParseMoney(argsman.GetArg("-incrementalrelayfee", ""))) {
             mempool_opts.incremental_relay_feerate = CFeeRate{inc_relay_fee.value()};
         } else {
-            return AmountErrMsg("incrementalrelayfee", argsman.GetArg("-incrementalrelayfee", ""));
+            return util::Error{AmountErrMsg("incrementalrelayfee", argsman.GetArg("-incrementalrelayfee", ""))};
         }
     }
 
@@ -61,7 +61,7 @@ std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& argsman, con
             // High fee check is done afterward in CWallet::Create()
             mempool_opts.min_relay_feerate = CFeeRate{min_relay_feerate.value()};
         } else {
-            return AmountErrMsg("minrelaytxfee", argsman.GetArg("-minrelaytxfee", ""));
+            return util::Error{AmountErrMsg("minrelaytxfee", argsman.GetArg("-minrelaytxfee", ""))};
         }
     } else if (mempool_opts.incremental_relay_feerate > mempool_opts.min_relay_feerate) {
         // Allow only setting incremental fee to control both
@@ -75,7 +75,7 @@ std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& argsman, con
         if (std::optional<CAmount> parsed = ParseMoney(argsman.GetArg("-dustrelayfee", ""))) {
             mempool_opts.dust_relay_feerate = CFeeRate{parsed.value()};
         } else {
-            return AmountErrMsg("dustrelayfee", argsman.GetArg("-dustrelayfee", ""));
+            return util::Error{AmountErrMsg("dustrelayfee", argsman.GetArg("-dustrelayfee", ""))};
         }
     }
 
@@ -89,12 +89,12 @@ std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& argsman, con
 
     mempool_opts.require_standard = !argsman.GetBoolArg("-acceptnonstdtxn", !chainparams.RequireStandard());
     if (!chainparams.IsTestChain() && !mempool_opts.require_standard) {
-        return strprintf(Untranslated("acceptnonstdtxn is not currently supported for %s chain"), chainparams.NetworkIDString());
+        return util::Error{strprintf(Untranslated("acceptnonstdtxn is not currently supported for %s chain"), chainparams.NetworkIDString())};
     }
 
     mempool_opts.full_rbf = argsman.GetBoolArg("-mempoolfullrbf", mempool_opts.full_rbf);
 
     ApplyArgsManOptions(argsman, mempool_opts.limits);
 
-    return std::nullopt;
+    return {};
 }
diff --git a/src/node/mempool_args.h b/src/node/mempool_args.h
@@ -5,7 +5,7 @@
 #ifndef BITCOIN_NODE_MEMPOOL_ARGS_H
 #define BITCOIN_NODE_MEMPOOL_ARGS_H
 
-#include <optional>
+#include <util/result.h>
 
 class ArgsManager;
 class CChainParams;
@@ -21,7 +21,7 @@ struct MemPoolOptions;
  * @param[in]  argsman The ArgsManager in which to check set options.
  * @param[in,out] mempool_opts The MemPoolOptions to modify according to \p argsman.
  */
-[[nodiscard]] std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& argsman, const CChainParams& chainparams, kernel::MemPoolOptions& mempool_opts);
+[[nodiscard]] util::Result<void> ApplyArgsManOptions(const ArgsManager& argsman, const CChainParams& chainparams, kernel::MemPoolOptions& mempool_opts);
 
 
 #endif // BITCOIN_NODE_MEMPOOL_ARGS_H
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
@@ -169,8 +169,8 @@ CTxMemPool::Options MemPoolOptionsForTest(const NodeContext& node)
         // chainparams.DefaultConsistencyChecks for tests
         .check_ratio = 1,
     };
-    const auto err{ApplyArgsManOptions(*node.args, ::Params(), mempool_opts)};
-    Assert(!err);
+    auto result = ApplyArgsManOptions(*node.args, ::Params(), mempool_opts);
+    Assert(result);
     return mempool_opts;
 }
 
diff --git a/src/txdb.cpp b/src/txdb.cpp
@@ -31,22 +31,22 @@ static constexpr uint8_t DB_COINS{'c'};
 static constexpr uint8_t DB_TXINDEX_BLOCK{'T'};
 //               uint8_t DB_TXINDEX{'t'}
 
-std::optional<bilingual_str> CheckLegacyTxindex(CBlockTreeDB& block_tree_db)
+util::Result<void> CheckLegacyTxindex(CBlockTreeDB& block_tree_db)
 {
     CBlockLocator ignored{};
     if (block_tree_db.Read(DB_TXINDEX_BLOCK, ignored)) {
-        return _("The -txindex upgrade started by a previous version cannot be completed. Restart with the previous version or run a full -reindex.");
+        return util::Error{_("The -txindex upgrade started by a previous version cannot be completed. Restart with the previous version or run a full -reindex.")};
     }
     bool txindex_legacy_flag{false};
     block_tree_db.ReadFlag("txindex", txindex_legacy_flag);
     if (txindex_legacy_flag) {
         // Disable legacy txindex and warn once about occupied disk space
         if (!block_tree_db.WriteFlag("txindex", false)) {
-            return Untranslated("Failed to write block index db flag 'txindex'='0'");
+            return util::Error{Untranslated("Failed to write block index db flag 'txindex'='0'")};
         }
-        return _("The block index db contains a legacy 'txindex'. To clear the occupied disk space, run a full -reindex, otherwise ignore this error. This error message will not be displayed again.");
+        return util::Error{_("The block index db contains a legacy 'txindex'. To clear the occupied disk space, run a full -reindex, otherwise ignore this error. This error message will not be displayed again.")};
     }
-    return std::nullopt;
+    return {};
 }
 
 bool CCoinsViewDB::NeedsUpgrade()
diff --git a/src/txdb.h b/src/txdb.h
@@ -9,6 +9,7 @@
 #include <coins.h>
 #include <dbwrapper.h>
 #include <sync.h>
+#include <util/result.h>
 
 #include <memory>
 #include <optional>
@@ -91,6 +92,6 @@ class CBlockTreeDB : public CDBWrapper
         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 };
 
-std::optional<bilingual_str> CheckLegacyTxindex(CBlockTreeDB& block_tree_db);
+util::Result<void> CheckLegacyTxindex(CBlockTreeDB& block_tree_db);
 
 #endif // BITCOIN_TXDB_H

Original file line number	Diff line number	Diff line change
`@@ -13,21 +13,21 @@`
`13`	`13`
`14`	`14`	`namespace kernel {`
`15`	`15`
`16`		`-std::optional<bilingual_str> SanityChecks(const Context&)`
	`16`	`+util::Result<void> SanityChecks(const Context&)`
`17`	`17`	`{`
`18`	`18`	`if (!ECC_InitSanityCheck()) {`
`19`		`- return Untranslated("Elliptic curve cryptography sanity check failure. Aborting.");`
	`19`	`+ return util::Error{Untranslated("Elliptic curve cryptography sanity check failure. Aborting.")};`
`20`	`20`	`}`
`21`	`21`
`22`	`22`	`if (!Random_SanityCheck()) {`
`23`		`- return Untranslated("OS cryptographic RNG sanity check failure. Aborting.");`
	`23`	`+ return util::Error{Untranslated("OS cryptographic RNG sanity check failure. Aborting.")};`
`24`	`24`	`}`
`25`	`25`
`26`	`26`	`if (!ChronoSanityCheck()) {`
`27`		`- return Untranslated("Clock epoch mismatch. Aborting.");`
	`27`	`+ return util::Error{Untranslated("Clock epoch mismatch. Aborting.")};`
`28`	`28`	`}`
`29`	`29`
`30`		`- return std::nullopt;`
	`30`	`+ return {};`
`31`	`31`	`}`
`32`	`32`
`33`	`33`	`}`
Original file line number	Diff line number	Diff line change
`@@ -38,7 +38,7 @@ void ApplyArgsManOptions(const ArgsManager& argsman, MemPoolLimits& mempool_limi`
`38`	`38`	`}`
`39`	`39`	`}`
`40`	`40`
`41`		`-std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& argsman, const CChainParams& chainparams, MemPoolOptions& mempool_opts)`
	`41`	`+util::Result<void> ApplyArgsManOptions(const ArgsManager& argsman, const CChainParams& chainparams, MemPoolOptions& mempool_opts)`
`42`	`42`	`{`
`43`	`43`	`mempool_opts.check_ratio = argsman.GetIntArg("-checkmempool", mempool_opts.check_ratio);`
`44`	`44`
`@@ -52,7 +52,7 @@ std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& argsman, con`
`52`	`52`	`if (std::optional<CAmount> inc_relay_fee = ParseMoney(argsman.GetArg("-incrementalrelayfee", ""))) {`
`53`	`53`	`mempool_opts.incremental_relay_feerate = CFeeRate{inc_relay_fee.value()};`
`54`	`54`	`} else {`
`55`		`- return AmountErrMsg("incrementalrelayfee", argsman.GetArg("-incrementalrelayfee", ""));`
	`55`	`+ return util::Error{AmountErrMsg("incrementalrelayfee", argsman.GetArg("-incrementalrelayfee", ""))};`
`56`	`56`	`}`
`57`	`57`	`}`
`58`	`58`
`@@ -61,7 +61,7 @@ std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& argsman, con`
`61`	`61`	`// High fee check is done afterward in CWallet::Create()`
`62`	`62`	`mempool_opts.min_relay_feerate = CFeeRate{min_relay_feerate.value()};`
`63`	`63`	`} else {`
`64`		`- return AmountErrMsg("minrelaytxfee", argsman.GetArg("-minrelaytxfee", ""));`
	`64`	`+ return util::Error{AmountErrMsg("minrelaytxfee", argsman.GetArg("-minrelaytxfee", ""))};`
`65`	`65`	`}`
`66`	`66`	`} else if (mempool_opts.incremental_relay_feerate > mempool_opts.min_relay_feerate) {`
`67`	`67`	`// Allow only setting incremental fee to control both`
`@@ -75,7 +75,7 @@ std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& argsman, con`
`75`	`75`	`if (std::optional<CAmount> parsed = ParseMoney(argsman.GetArg("-dustrelayfee", ""))) {`
`76`	`76`	`mempool_opts.dust_relay_feerate = CFeeRate{parsed.value()};`
`77`	`77`	`} else {`
`78`		`- return AmountErrMsg("dustrelayfee", argsman.GetArg("-dustrelayfee", ""));`
	`78`	`+ return util::Error{AmountErrMsg("dustrelayfee", argsman.GetArg("-dustrelayfee", ""))};`
`79`	`79`	`}`
`80`	`80`	`}`
`81`	`81`
`@@ -89,12 +89,12 @@ std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& argsman, con`
`89`	`89`
`90`	`90`	`mempool_opts.require_standard = !argsman.GetBoolArg("-acceptnonstdtxn", !chainparams.RequireStandard());`
`91`	`91`	`if (!chainparams.IsTestChain() && !mempool_opts.require_standard) {`
`92`		`- return strprintf(Untranslated("acceptnonstdtxn is not currently supported for %s chain"), chainparams.NetworkIDString());`
	`92`	`+ return util::Error{strprintf(Untranslated("acceptnonstdtxn is not currently supported for %s chain"), chainparams.NetworkIDString())};`
`93`	`93`	`}`
`94`	`94`
`95`	`95`	`mempool_opts.full_rbf = argsman.GetBoolArg("-mempoolfullrbf", mempool_opts.full_rbf);`
`96`	`96`
`97`	`97`	`ApplyArgsManOptions(argsman, mempool_opts.limits);`
`98`	`98`
`99`		`- return std::nullopt;`
	`99`	`+ return {};`
`100`	`100`	`}`
Original file line number	Diff line number	Diff line change
`@@ -31,22 +31,22 @@ static constexpr uint8_t DB_COINS{'c'};`
`31`	`31`	`static constexpr uint8_t DB_TXINDEX_BLOCK{'T'};`
`32`	`32`	`// uint8_t DB_TXINDEX{'t'}`
`33`	`33`
`34`		`-std::optional<bilingual_str> CheckLegacyTxindex(CBlockTreeDB& block_tree_db)`
	`34`	`+util::Result<void> CheckLegacyTxindex(CBlockTreeDB& block_tree_db)`
`35`	`35`	`{`
`36`	`36`	`CBlockLocator ignored{};`
`37`	`37`	`if (block_tree_db.Read(DB_TXINDEX_BLOCK, ignored)) {`
`38`		`- return _("The -txindex upgrade started by a previous version cannot be completed. Restart with the previous version or run a full -reindex.");`
	`38`	`+ return util::Error{_("The -txindex upgrade started by a previous version cannot be completed. Restart with the previous version or run a full -reindex.")};`
`39`	`39`	`}`
`40`	`40`	`bool txindex_legacy_flag{false};`
`41`	`41`	`block_tree_db.ReadFlag("txindex", txindex_legacy_flag);`
`42`	`42`	`if (txindex_legacy_flag) {`
`43`	`43`	`// Disable legacy txindex and warn once about occupied disk space`
`44`	`44`	`if (!block_tree_db.WriteFlag("txindex", false)) {`
`45`		`- return Untranslated("Failed to write block index db flag 'txindex'='0'");`
	`45`	`+ return util::Error{Untranslated("Failed to write block index db flag 'txindex'='0'")};`
`46`	`46`	`}`
`47`		`- return _("The block index db contains a legacy 'txindex'. To clear the occupied disk space, run a full -reindex, otherwise ignore this error. This error message will not be displayed again.");`
	`47`	`+ return util::Error{_("The block index db contains a legacy 'txindex'. To clear the occupied disk space, run a full -reindex, otherwise ignore this error. This error message will not be displayed again.")};`
`48`	`48`	`}`
`49`		`- return std::nullopt;`
	`49`	`+ return {};`
`50`	`50`	`}`
`51`	`51`
`52`	`52`	`bool CCoinsViewDB::NeedsUpgrade()`