From fa114be27b17ed32c1d9a7106f313a0df8755fa2 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 4 Dec 2025 15:54:19 +0100 Subject: [PATCH 1/2] Add util::Expected (std::expected) --- src/kernel/CMakeLists.txt | 1 + src/net_processing.cpp | 16 +++--- src/net_processing.h | 4 +- src/rpc/blockchain.cpp | 4 +- src/test/CMakeLists.txt | 1 + src/test/util_expected_tests.cpp | 68 +++++++++++++++++++++++ src/util/expected.cpp | 5 ++ src/util/expected.h | 92 ++++++++++++++++++++++++++++++++ src/util/result.h | 4 +- 9 files changed, 181 insertions(+), 14 deletions(-) create mode 100644 src/test/util_expected_tests.cpp create mode 100644 src/util/expected.cpp create mode 100644 src/util/expected.h diff --git a/src/kernel/CMakeLists.txt b/src/kernel/CMakeLists.txt index 6b9565e814e..85011f4488a 100644 --- a/src/kernel/CMakeLists.txt +++ b/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 diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 9cab2461763..8da34575d12 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -530,7 +530,7 @@ public: /** Implement PeerManager */ void StartScheduledTasks(CScheduler& scheduler) override; void CheckForStaleTipAndEvictPeers() override; - std::optional FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override + util::Expected 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 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 PeerManagerImpl::FetchBlock(NodeId peer_id, const CBlockIndex& block_index) +util::Expected 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 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 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::make(CConnman& connman, AddrMan& addrman, diff --git a/src/net_processing.h b/src/net_processing.h index 6eb4a5e16a2..cf75e8f6da3 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -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 FetchBlock(NodeId peer_id, const CBlockIndex& block_index) = 0; + virtual util::Expected FetchBlock(NodeId peer_id, const CBlockIndex& block_index) = 0; /** Begin running background tasks, should only be called once */ virtual void StartScheduledTasks(CScheduler& scheduler) = 0; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index ff8cb119183..8f283ff4f03 100644 --- a/src/rpc/blockchain.cpp +++ b/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; }, diff --git a/src/test/CMakeLists.txt b/src/test/CMakeLists.txt index 8d27e46250d..83cb989aa9b 100644 --- a/src/test/CMakeLists.txt +++ b/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 diff --git a/src/test/util_expected_tests.cpp b/src/test/util_expected_tests.cpp new file mode 100644 index 00000000000..61a6d2e0745 --- /dev/null +++ b/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 +#include + +#include + +using namespace util; + +BOOST_AUTO_TEST_SUITE(util_expected_tests) + +BOOST_AUTO_TEST_CASE(expected_value) +{ + struct Obj { + int x; + }; + Expected e{}; + BOOST_CHECK(e.value().x == 0); + + e = Obj{42}; + + BOOST_CHECK(e.has_value()); + BOOST_CHECK(static_cast(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, int> no_copy{std::make_unique(1)}; + const int one{*std::move(no_copy).value_or(std::make_unique(2))}; + BOOST_CHECK_EQUAL(one, 1); + + const Expected const_val{Unexpected{-1}}; + BOOST_CHECK_EQUAL(const_val.value_or("fallback"), "fallback"); +} + +BOOST_AUTO_TEST_CASE(expected_error) +{ + Expected e{}; + BOOST_CHECK(e.has_value()); + + e = Unexpected{"fail"}; + BOOST_CHECK(!e.has_value()); + BOOST_CHECK(!static_cast(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() diff --git a/src/util/expected.cpp b/src/util/expected.cpp new file mode 100644 index 00000000000..f339f99950b --- /dev/null +++ b/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 diff --git a/src/util/expected.h b/src/util/expected.h new file mode 100644 index 00000000000..0e7256f8677 --- /dev/null +++ b/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 + +#include +#include +#include +#include + +namespace util { + +/// The util::Unexpected class represents an unexpected value stored in +/// util::Expected. +template +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 Expected +{ +private: + using ValueType = std::conditional_t, std::monostate, T>; + std::variant 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 + constexpr Expected(Unexpected 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 + ValueType value_or(U&& default_value) const& + { + return has_value() ? value() : std::forward(default_value); + } + template + ValueType value_or(U&& default_value) && + { + return has_value() ? std::move(value()) : std::forward(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 diff --git a/src/util/result.h b/src/util/result.h index 122a7638fad..fa774fb3e87 100644 --- a/src/util/result.h +++ b/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 From faa23738fc2576e412edb04a4004fab537a3098e Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Sat, 6 Dec 2025 10:31:54 +0100 Subject: [PATCH 2/2] refactor: Enable clang-tidy bugprone-unused-return-value This requires some small refactors to silence false-positive warnings. Also, expand the bugprone-unused-return-value.CheckedReturnTypes option to include util::Result, and util::Expected. --- src/.clang-tidy | 3 +++ src/bench/coin_selection.cpp | 2 +- src/bitcoin-cli.cpp | 3 +-- src/httpserver.cpp | 2 +- src/ipc/test/ipc_test.cpp | 4 +++- src/wallet/test/fuzz/spend.cpp | 2 +- 6 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/.clang-tidy b/src/.clang-tidy index 544bd5ac0c1..da53a588c2e 100644 --- a/src/.clang-tidy +++ b/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$' diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index dfefa1b5306..2150d800c9f 100644 --- a/src/bench/coin_selection.cpp +++ b/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(); diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index 279aa89e88e..d6fcaa841a6 100644 --- a/src/bitcoin-cli.cpp +++ b/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"); } diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 5644ddbcc13..abfcb455e14 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -330,7 +330,7 @@ static void http_request_cb(struct evhttp_request* req, void* arg) std::unique_ptr 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"); diff --git a/src/ipc/test/ipc_test.cpp b/src/ipc/test/ipc_test.cpp index a350d60b5d8..506facdecf3 100644 --- a/src/ipc/test/ipc_test.cpp +++ b/src/ipc/test/ipc_test.cpp @@ -63,7 +63,9 @@ void IpcPipeTest() auto foo_client = std::make_unique>( connection_client->m_rpc_system->bootstrap(mp::ServerVatId().vat_id).castAs(), 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(loop, kj::mv(pipe.ends[1]), [&](mp::Connection& connection) { diff --git a/src/wallet/test/fuzz/spend.cpp b/src/wallet/test/fuzz/spend.cpp index 552364a667d..99bc5345a34 100644 --- a/src/wallet/test/fuzz/spend.cpp +++ b/src/wallet/test/fuzz/spend.cpp @@ -98,7 +98,7 @@ FUZZ_TARGET(wallet_create_transaction, .init = initialize_setup) std::optional change_pos; if (fuzzed_data_provider.ConsumeBool()) change_pos = fuzzed_data_provider.ConsumeIntegral(); - (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