Merge 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#25648 (comment), bitcoin#27632 (comment), bitcoin#27632 (comment), 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

Redundant std::move

Author: fanquake

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)};
 
     const auto start{SteadyClock::now()};
     const auto path_addr{gArgs.GetDataDirNet() / "peers.dat"};
@@ -208,19 +208,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; // 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 <fs.h>
 #include <net_types.h> // For banmap_t
 #include <univalue.h>
+#include <util/result.h>
 
 #include <memory>
 #include <optional>
@@ -50,7 +51,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/init.cpp

@@ -1647,9 +1647,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);
     }
 
     std::string sem_str = args.GetArg("-socketevents", DEFAULT_SOCKETEVENTS);

src/test/util/txmempool.cpp

@@ -20,8 +20,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);
+    const auto result{ApplyArgsManOptions(*node.args, ::Params(), mempool_opts)};
+    Assert(result);
     return mempool_opts;
 }
 */

src/util/result.h

@@ -31,16 +31,19 @@ struct Error {
 //! `std::optional<T>` can be updated to return `util::Result<T>` and return
 //! error strings usually just replacing `return std::nullopt;` with `return
 //! util::Error{error_string};`.
-template <class T>
+template <class M>
 class Result
 {
 private:
+    using T = std::conditional_t<std::is_same_v<M, void>, std::monostate, M>;
+
     std::variant<bilingual_str, T> m_variant;
 
     template <typename FT>
     friend bilingual_str ErrorString(const Result<FT>& result);
 
 public:
+    Result() : m_variant{std::in_place_index_t<1>{}, std::monostate{}} {}  // constructor for void
     Result(T obj) : m_variant{std::in_place_index_t<1>{}, std::move(obj)} {}
     Result(Error error) : m_variant{std::in_place_index_t<0>{}, std::move(error.message)} {}