Merge bitcoin/bitcoin#25977: refactor: Replace std::optional<bilingual_str> with util::Result

8aa8f73 refactor: Replace std::optional<bilingual_str> with util::Result (Ryan Ofsky)
5f49cb1 util: Add void support to util::Result (MarcoFalke)

Pull request description:

  Replace uses of `std::optional<bilingual_str>` with `util::Result` as suggested bitcoin/bitcoin#25648 (comment), bitcoin/bitcoin#27632 (comment), bitcoin/bitcoin#27632 (comment), bitcoin/bitcoin#24313 (comment)

ACKs for top commit:
  MarcoFalke:
    very nice ACK 8aa8f73 🏏
  TheCharlatan:
    ACK 8aa8f73
  hebasto:
    ACK 8aa8f73, I have reviewed the code and it looks OK.

Tree-SHA512: 6c2f218bc445184ce93ec2b907e61666a05f26f2c9447f69fcb504aa8291b0c693d913f659dfd19813a9e24615546c72cbe2ca419218fd867ff0694c4a1b6a30

Author: fanquake

src/addrdb.cpp

@@ -183,10 +183,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)};
 
     const auto start{SteadyClock::now()};
     const auto path_addr{args.GetDataDirNet() / "peers.dat"};
@@ -200,19 +200,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 std::move(addrman); // std::move should be unneccessary but is temporarily needed to work around clang bug (https://github.com/bitcoin/bitcoin/pull/25977#issuecomment-1561270092)
 }
 
 void DumpAnchors(const fs::path& anchors_db_path, const std::vector<CAddress>& anchors)

src/addrdb.h

@@ -9,6 +9,7 @@
 #include <net_types.h> // For banmap_t
 #include <univalue.h>
 #include <util/fs.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)

src/bitcoin-chainstate.cpp

@@ -60,7 +60,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));
 
     // Necessary for CheckInputScripts (eventually called by ProcessNewBlock),
     // which will try the script cache first and fall back to actually

src/init.cpp

@@ -1023,15 +1023,17 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
             .chainparams = chainparams,
             .datadir = args.GetDataDirNet(),
         };
-        if (const auto error{ApplyArgsManOptions(args, chainman_opts_dummy)}) {
-            return InitError(*error);
+        auto chainman_result{ApplyArgsManOptions(args, chainman_opts_dummy)};
+        if (!chainman_result) {
+            return InitError(util::ErrorString(chainman_result));
         }
         BlockManager::Options blockman_opts_dummy{
             .chainparams = chainman_opts_dummy.chainparams,
             .blocks_dir = args.GetBlocksDirPath(),
         };
-        if (const auto error{ApplyArgsManOptions(args, blockman_opts_dummy)}) {
-            return InitError(*error);
+        auto blockman_result{ApplyArgsManOptions(args, blockman_opts_dummy)};
+        if (!blockman_result) {
+            return InitError(util::ErrorString(blockman_result));
         }
     }
 
@@ -1054,8 +1056,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));
     }
 
@@ -1230,9 +1233,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);
@@ -1434,13 +1437,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         .datadir = args.GetDataDirNet(),
         .adjusted_time_callback = GetAdjustedTime,
     };
-    Assert(!ApplyArgsManOptions(args, chainman_opts)); // no error can happen, already checked in AppInitParameterInteraction
+    Assert(ApplyArgsManOptions(args, chainman_opts)); // no error can happen, already checked in AppInitParameterInteraction
 
     BlockManager::Options blockman_opts{
         .chainparams = chainman_opts.chainparams,
         .blocks_dir = args.GetBlocksDirPath(),
     };
-    Assert(!ApplyArgsManOptions(args, blockman_opts)); // no error can happen, already checked in AppInitParameterInteraction
+    Assert(ApplyArgsManOptions(args, blockman_opts)); // no error can happen, already checked in AppInitParameterInteraction
 
     // cache size calculations
     CacheSizes cache_sizes = CalculateCacheSizes(args, g_enabled_filter_types.size());
@@ -1463,8 +1466,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);
 
@@ -1563,8 +1567,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     // If reindex-chainstate was specified, delay syncing indexes until ThreadImport has reindexed the chain
     if (!fReindexChainState) g_indexes_ready_to_sync = true;
     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);

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 {};
 }
 
 }

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&);
 
 }
 

src/node/blockmanager_args.cpp

@@ -7,33 +7,33 @@
 #include <common/args.h>
 #include <node/blockstorage.h>
 #include <tinyformat.h>
+#include <util/result.h>
 #include <util/translation.h>
 #include <validation.h>
 
 #include <cstdint>
-#include <optional>
 
 namespace node {
-std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& args, BlockManager::Options& opts)
+util::Result<void> ApplyArgsManOptions(const ArgsManager& args, BlockManager::Options& opts)
 {
     // block pruning; get the amount of disk space (in MiB) to allot for block & undo files
     int64_t nPruneArg{args.GetIntArg("-prune", opts.prune_target)};
     if (nPruneArg < 0) {
-        return _("Prune cannot be configured with a negative value.");
+        return util::Error{_("Prune cannot be configured with a negative value.")};
     }
     uint64_t nPruneTarget{uint64_t(nPruneArg) * 1024 * 1024};
     if (nPruneArg == 1) { // manual pruning: -prune=1
         nPruneTarget = BlockManager::PRUNE_TARGET_MANUAL;
     } else if (nPruneTarget) {
         if (nPruneTarget < MIN_DISK_SPACE_FOR_BLOCK_FILES) {
-            return strprintf(_("Prune configured below the minimum of %d MiB.  Please use a higher number."), MIN_DISK_SPACE_FOR_BLOCK_FILES / 1024 / 1024);
+            return util::Error{strprintf(_("Prune configured below the minimum of %d MiB.  Please use a higher number."), MIN_DISK_SPACE_FOR_BLOCK_FILES / 1024 / 1024)};
         }
     }
     opts.prune_target = nPruneTarget;
 
     if (auto value{args.GetBoolArg("-fastprune")}) opts.fast_prune = *value;
     if (auto value{args.GetBoolArg("-stopafterblockimport")}) opts.stop_after_block_import = *value;
 
-    return std::nullopt;
+    return {};
 }
 } // namespace node

src/node/blockmanager_args.h

@@ -7,14 +7,12 @@
 #define BITCOIN_NODE_BLOCKMANAGER_ARGS_H
 
 #include <node/blockstorage.h>
-
-#include <optional>
+#include <util/result.h>
 
 class ArgsManager;
-struct bilingual_str;
 
 namespace node {
-std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& args, BlockManager::Options& opts);
+util::Result<void> ApplyArgsManOptions(const ArgsManager& args, BlockManager::Options& opts);
 } // namespace node
 
 #endif // BITCOIN_NODE_BLOCKMANAGER_ARGS_H

src/node/chainstatemanager_args.cpp

@@ -11,24 +11,24 @@
 #include <node/database_args.h>
 #include <tinyformat.h>
 #include <uint256.h>
+#include <util/result.h>
 #include <util/strencodings.h>
 #include <util/translation.h>
 #include <validation.h>
 
 #include <chrono>
-#include <optional>
 #include <string>
 
 namespace node {
-std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& args, ChainstateManager::Options& opts)
+util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManager::Options& opts)
 {
     if (auto value{args.GetBoolArg("-checkblockindex")}) opts.check_block_index = *value;
 
     if (auto value{args.GetBoolArg("-checkpoints")}) opts.checkpoints_enabled = *value;
 
     if (auto value{args.GetArg("-minimumchainwork")}) {
         if (!IsHexNumber(*value)) {
-            return strprintf(Untranslated("Invalid non-hex (%s) minimum chain work value specified"), *value);
+            return util::Error{strprintf(Untranslated("Invalid non-hex (%s) minimum chain work value specified"), *value)};
         }
         opts.minimum_chain_work = UintToArith256(uint256S(*value));
     }
@@ -41,6 +41,6 @@ std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& args, Chains
     ReadDatabaseArgs(args, opts.coins_db);
     ReadCoinsViewArgs(args, opts.coins_view);
 
-    return std::nullopt;
+    return {};
 }
 } // namespace node

src/node/chainstatemanager_args.h

@@ -5,15 +5,13 @@
 #ifndef BITCOIN_NODE_CHAINSTATEMANAGER_ARGS_H
 #define BITCOIN_NODE_CHAINSTATEMANAGER_ARGS_H
 
+#include <util/result.h>
 #include <validation.h>
 
-#include <optional>
-
 class ArgsManager;
-struct bilingual_str;
 
 namespace node {
-std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& args, ChainstateManager::Options& opts);
+util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManager::Options& opts);
 } // namespace node
 
 #endif // BITCOIN_NODE_CHAINSTATEMANAGER_ARGS_H