Merge bitcoin/bitcoin#34006: Add util::Expected (std::expected)

faa23738fc refactor: Enable clang-tidy bugprone-unused-return-value (MarcoFalke)
fa114be27b Add util::Expected (std::expected) (MarcoFalke)

Pull request description:

  Some low-level code could benefit from being able to use `std::expected` from C++23:

  * Currently, some code is using `std::optional<E>` to denote an optional error. This is fine, but a bit confusing, because `std::optional` is normally used for values, not errors. Using `std::expected<void, E>` is clearer.
  * Currently, some code is using `std::variant<V, E>` to denote either a value or an error. This is fine, but a bit verbose, because `std::variant` requires a visitor or get_if/holds_alternative instead of a simple call of the `operator bool` for `std::expected`.

  In theory, `util::Result` could be taught to behave similar to `std::expected` (see https://github.com/bitcoin/bitcoin/pull/34005). However, it is unclear if this is the right approach:

  * `util::Result` is mostly meant for higher level code, where errors come with translated error messages.
  * `std::expected` is mostly meant for lower level code, where errors could be an enum, or any other type.
  * https://github.com/bitcoin/bitcoin/pull/25665 aims to minimize the memory footprint of the error by wrapping it in a unique_ptr internally. `std::expected` requires the value and error to be "nested within it" (https://cplusplus.github.io/LWG/issue4141). So from a memory-layout perspective, the two are not compatible.
  * `std::expected` also comes with `std::unexpected`, which also does not map cleanly to `util::Result`.

  So just add a minimal drop-in port of `std::expected`.

ACKs for top commit:
  romanz:
    tACK faa23738fc
  sedited:
    Re-ACK faa23738fc
  hodlinator:
    ACK faa23738fc
  rkrux:
    light Code Review ACK faa23738fc
  ryanofsky:
    Code review ACK faa23738fc, only adding `value_or` implementation and `CheckedReturnTypes` clang-tidy commit since last review.
  stickies-v:
    ACK faa23738fc

Tree-SHA512: fdbd0f6bf439738ffe6a68da5522f1051537f8df9c308eb90bef6bd2e06931d79f1c5da22d5500765e9cb1d801d5be39e11e10d47c9659fec1a8c8804cb7c872

src/.clang-tidy

@@ -7,6 +7,7 @@ bugprone-string-constructor,
 bugprone-use-after-move,
 bugprone-lambda-function-name,
 bugprone-unhandled-self-assignment,
+bugprone-unused-return-value,
 misc-unused-using-decls,
 misc-no-recursion,
 modernize-deprecated-headers,
@@ -36,3 +37,5 @@ CheckOptions:
    value: false
  - key: bugprone-unhandled-self-assignment.WarnOnlyIfThisHasSuspiciousField
    value: false
+ - key: bugprone-unused-return-value.CheckedReturnTypes
+   value: '^::std::error_code$;^::std::error_condition$;^::std::errc$;^::std::expected$;^::util::Result$;^::util::Expected$'

src/bench/coin_selection.cpp

@@ -130,7 +130,7 @@ static void BnBExhaustion(benchmark::Bench& bench)
     bench.run([&] {
         // Benchmark
         CAmount target = make_hard_case(17, utxo_pool);
-        SelectCoinsBnB(utxo_pool, target, 0, MAX_STANDARD_TX_WEIGHT); // Should exhaust
+        [[maybe_unused]] auto _{SelectCoinsBnB(utxo_pool, target, /*cost_of_change=*/0, MAX_STANDARD_TX_WEIGHT)}; // Should exhaust
 
         // Cleanup
         utxo_pool.clear();

src/bitcoin-cli.cpp

@@ -899,8 +899,7 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co
             throw CConnectionFailed("uri-encode failed");
         }
     }
-    int r = evhttp_make_request(evcon.get(), req.get(), EVHTTP_REQ_POST, endpoint.c_str());
-    req.release(); // ownership moved to evcon in above call
+    int r = evhttp_make_request(evcon.get(), req.release(), EVHTTP_REQ_POST, endpoint.c_str());
     if (r != 0) {
         throw CConnectionFailed("send http request failed");
     }

src/httpserver.cpp

@@ -330,7 +330,7 @@ static void http_request_cb(struct evhttp_request* req, void* arg)
         std::unique_ptr<HTTPWorkItem> item(new HTTPWorkItem(std::move(hreq), path, i->handler));
         assert(g_work_queue);
         if (g_work_queue->Enqueue(item.get())) {
-            item.release(); /* if true, queue took ownership */
+            [[maybe_unused]] auto _{item.release()}; /* if true, queue took ownership */
         } else {
             LogWarning("Request rejected because http work queue depth exceeded, it can be increased with the -rpcworkqueue= setting");
             item->req->WriteReply(HTTP_SERVICE_UNAVAILABLE, "Work queue depth exceeded");

src/ipc/test/ipc_test.cpp

@@ -63,7 +63,9 @@ void IpcPipeTest()
         auto foo_client = std::make_unique<mp::ProxyClient<gen::FooInterface>>(
             connection_client->m_rpc_system->bootstrap(mp::ServerVatId().vat_id).castAs<gen::FooInterface>(),
             connection_client.get(), /* destroy_connection= */ true);
-        connection_client.release();
+        {
+            [[maybe_unused]] auto _{connection_client.release()};
+        }
         foo_promise.set_value(std::move(foo_client));
 
         auto connection_server = std::make_unique<mp::Connection>(loop, kj::mv(pipe.ends[1]), [&](mp::Connection& connection) {

src/kernel/CMakeLists.txt

@@ -63,6 +63,7 @@ add_library(bitcoinkernel
   ../uint256.cpp
   ../util/chaintype.cpp
   ../util/check.cpp
+  ../util/expected.cpp
   ../util/feefrac.cpp
   ../util/fs.cpp
   ../util/fs_helpers.cpp

src/net_processing.cpp

@@ -530,7 +530,7 @@ public:
     /** Implement PeerManager */
     void StartScheduledTasks(CScheduler& scheduler) override;
     void CheckForStaleTipAndEvictPeers() override;
-    std::optional<std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override
+    util::Expected<void, std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override
         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     std::vector<node::TxOrphanage::OrphanInfo> GetOrphanTransactions() override EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex);
@@ -1844,16 +1844,16 @@ bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
            (GetBlockProofEquivalentTime(*m_chainman.m_best_header, *pindex, *m_chainman.m_best_header, m_chainparams.GetConsensus()) < STALE_RELAY_AGE_LIMIT);
 }
 
-std::optional<std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, const CBlockIndex& block_index)
+util::Expected<void, std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, const CBlockIndex& block_index)
 {
-    if (m_chainman.m_blockman.LoadingBlocks()) return "Loading blocks ...";
+    if (m_chainman.m_blockman.LoadingBlocks()) return util::Unexpected{"Loading blocks ..."};
 
     // Ensure this peer exists and hasn't been disconnected
     PeerRef peer = GetPeerRef(peer_id);
-    if (peer == nullptr) return "Peer does not exist";
+    if (peer == nullptr) return util::Unexpected{"Peer does not exist"};
 
     // Ignore pre-segwit peers
-    if (!CanServeWitnesses(*peer)) return "Pre-SegWit peer";
+    if (!CanServeWitnesses(*peer)) return util::Unexpected{"Pre-SegWit peer"};
 
     LOCK(cs_main);
 
@@ -1861,7 +1861,7 @@ std::optional<std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, const CBl
     RemoveBlockRequest(block_index.GetBlockHash(), std::nullopt);
 
     // Mark block as in-flight
-    if (!BlockRequested(peer_id, block_index)) return "Already requested from this peer";
+    if (!BlockRequested(peer_id, block_index)) return util::Unexpected{"Already requested from this peer"};
 
     // Construct message to request the block
     const uint256& hash{block_index.GetBlockHash()};
@@ -1873,11 +1873,11 @@ std::optional<std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, const CBl
         return true;
     });
 
-    if (!success) return "Peer not fully connected";
+    if (!success) return util::Unexpected{"Peer not fully connected"};
 
     LogDebug(BCLog::NET, "Requesting block %s from peer=%d\n",
                  hash.ToString(), peer_id);
-    return std::nullopt;
+    return {};
 }
 
 std::unique_ptr<PeerManager> PeerManager::make(CConnman& connman, AddrMan& addrman,

src/net_processing.h

@@ -11,6 +11,7 @@
 #include <node/txorphanage.h>
 #include <protocol.h>
 #include <threadsafety.h>
+#include <util/expected.h>
 #include <validationinterface.h>
 
 #include <atomic>
@@ -101,9 +102,8 @@ public:
      *
      * @param[in]  peer_id      The peer id
      * @param[in]  block_index  The blockindex
-     * @returns std::nullopt if a request was successfully made, otherwise an error message
      */
-    virtual std::optional<std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) = 0;
+    virtual util::Expected<void, std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) = 0;
 
     /** Begin running background tasks, should only be called once */
     virtual void StartScheduledTasks(CScheduler& scheduler) = 0;

src/rpc/blockchain.cpp

@@ -536,8 +536,8 @@ static RPCHelpMan getblockfrompeer()
         throw JSONRPCError(RPC_MISC_ERROR, "Block already downloaded");
     }
 
-    if (const auto err{peerman.FetchBlock(peer_id, *index)}) {
-        throw JSONRPCError(RPC_MISC_ERROR, err.value());
+    if (const auto res{peerman.FetchBlock(peer_id, *index)}; !res) {
+        throw JSONRPCError(RPC_MISC_ERROR, res.error());
     }
     return UniValue::VOBJ;
 },

src/test/CMakeLists.txt

@@ -119,6 +119,7 @@ add_executable(test_bitcoin
   txvalidationcache_tests.cpp
   uint256_tests.cpp
   util_check_tests.cpp
+  util_expected_tests.cpp
   util_string_tests.cpp
   util_tests.cpp
   util_threadnames_tests.cpp

src/test/util_expected_tests.cpp

@@ -0,0 +1,68 @@
+// Copyright (c) The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or https://opensource.org/license/mit.
+
+#include <test/util/setup_common.h>
+#include <util/expected.h>
+
+#include <boost/test/unit_test.hpp>
+
+using namespace util;
+
+BOOST_AUTO_TEST_SUITE(util_expected_tests)
+
+BOOST_AUTO_TEST_CASE(expected_value)
+{
+    struct Obj {
+        int x;
+    };
+    Expected<Obj, int> e{};
+    BOOST_CHECK(e.value().x == 0);
+
+    e = Obj{42};
+
+    BOOST_CHECK(e.has_value());
+    BOOST_CHECK(static_cast<bool>(e));
+    BOOST_CHECK(e.value().x == 42);
+    BOOST_CHECK((*e).x == 42);
+    BOOST_CHECK(e->x == 42);
+
+    // modify value
+    e.value().x += 1;
+    (*e).x += 1;
+    e->x += 1;
+
+    const auto& read{e};
+    BOOST_CHECK(read.value().x == 45);
+    BOOST_CHECK((*read).x == 45);
+    BOOST_CHECK(read->x == 45);
+}
+
+BOOST_AUTO_TEST_CASE(expected_value_or)
+{
+    Expected<std::unique_ptr<int>, int> no_copy{std::make_unique<int>(1)};
+    const int one{*std::move(no_copy).value_or(std::make_unique<int>(2))};
+    BOOST_CHECK_EQUAL(one, 1);
+
+    const Expected<std::string, int> const_val{Unexpected{-1}};
+    BOOST_CHECK_EQUAL(const_val.value_or("fallback"), "fallback");
+}
+
+BOOST_AUTO_TEST_CASE(expected_error)
+{
+    Expected<void, std::string> e{};
+    BOOST_CHECK(e.has_value());
+
+    e = Unexpected{"fail"};
+    BOOST_CHECK(!e.has_value());
+    BOOST_CHECK(!static_cast<bool>(e));
+    BOOST_CHECK(e.error() == "fail");
+
+    // modify error
+    e.error() += "1";
+
+    const auto& read{e};
+    BOOST_CHECK(read.error() == "fail1");
+}
+
+BOOST_AUTO_TEST_SUITE_END()

src/util/expected.cpp

@@ -0,0 +1,5 @@
+// Copyright (c) The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or https://opensource.org/license/mit.
+
+#include <util/expected.h>

src/util/expected.h

@@ -0,0 +1,92 @@
+// Copyright (c) The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or https://opensource.org/license/mit.
+
+#ifndef BITCOIN_UTIL_EXPECTED_H
+#define BITCOIN_UTIL_EXPECTED_H
+
+#include <attributes.h>
+
+#include <cassert>
+#include <type_traits>
+#include <utility>
+#include <variant>
+
+namespace util {
+
+/// The util::Unexpected class represents an unexpected value stored in
+/// util::Expected.
+template <class E>
+class Unexpected
+{
+public:
+    constexpr explicit Unexpected(E e) : err(std::move(e)) {}
+    E err;
+};
+
+/// The util::Expected class provides a standard way for low-level functions to
+/// return either error values or result values.
+///
+/// It provides a smaller version of std::expected from C++23. Missing features
+/// can be added, if needed.
+template <class T, class E>
+class Expected
+{
+private:
+    using ValueType = std::conditional_t<std::is_same_v<T, void>, std::monostate, T>;
+    std::variant<ValueType, E> m_data;
+
+public:
+    constexpr Expected() : m_data{std::in_place_index_t<0>{}, ValueType{}} {}
+    constexpr Expected(ValueType v) : m_data{std::in_place_index_t<0>{}, std::move(v)} {}
+    template <class Err>
+    constexpr Expected(Unexpected<Err> u) : m_data{std::in_place_index_t<1>{}, std::move(u.err)}
+    {
+    }
+
+    constexpr bool has_value() const noexcept { return m_data.index() == 0; }
+    constexpr explicit operator bool() const noexcept { return has_value(); }
+
+    constexpr const ValueType& value() const LIFETIMEBOUND
+    {
+        assert(has_value());
+        return std::get<0>(m_data);
+    }
+    constexpr ValueType& value() LIFETIMEBOUND
+    {
+        assert(has_value());
+        return std::get<0>(m_data);
+    }
+
+    template <class U>
+    ValueType value_or(U&& default_value) const&
+    {
+        return has_value() ? value() : std::forward<U>(default_value);
+    }
+    template <class U>
+    ValueType value_or(U&& default_value) &&
+    {
+        return has_value() ? std::move(value()) : std::forward<U>(default_value);
+    }
+
+    constexpr const E& error() const LIFETIMEBOUND
+    {
+        assert(!has_value());
+        return std::get<1>(m_data);
+    }
+    constexpr E& error() LIFETIMEBOUND
+    {
+        assert(!has_value());
+        return std::get<1>(m_data);
+    }
+
+    constexpr ValueType& operator*() LIFETIMEBOUND { return value(); }
+    constexpr const ValueType& operator*() const LIFETIMEBOUND { return value(); }
+
+    constexpr ValueType* operator->() LIFETIMEBOUND { return &value(); }
+    constexpr const ValueType* operator->() const LIFETIMEBOUND { return &value(); }
+};
+
+} // namespace util
+
+#endif // BITCOIN_UTIL_EXPECTED_H

src/util/result.h

@@ -21,8 +21,8 @@ struct Error {
 //!
 //! It is intended for high-level functions that need to report error strings to
 //! end users. Lower-level functions that don't need this error-reporting and
-//! only need error-handling should avoid util::Result and instead use standard
-//! classes like std::optional, std::variant, and std::tuple, or custom structs
+//! that only need error-handling should avoid util::Result and instead use
+//! util::Expected, std::optional, std::variant, or custom structs
 //! and enum types to return function results.
 //!
 //! Usage examples can be found in \example ../test/result_tests.cpp, but in

src/wallet/test/fuzz/spend.cpp

@@ -98,7 +98,7 @@ FUZZ_TARGET(wallet_create_transaction, .init = initialize_setup)
 
     std::optional<unsigned int> change_pos;
     if (fuzzed_data_provider.ConsumeBool()) change_pos = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
-    (void)CreateTransaction(*fuzzed_wallet.wallet, recipients, change_pos, coin_control);
+    [[maybe_unused]] auto _{CreateTransaction(*fuzzed_wallet.wallet, recipients, change_pos, coin_control)};
 }
 } // namespace
 } // namespace wallet