0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-02 09:46:52 -05:00

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

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

Pull request description:

  Replace uses of `std::optional<bilingual_str>` with `util::Result` as suggested https://github.com/bitcoin/bitcoin/pull/25648#discussion_r936311768, https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1192007516, https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1194858242, https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1204047087

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

Tree-SHA512: 6c2f218bc445184ce93ec2b907e61666a05f26f2c9447f69fcb504aa8291b0c693d913f659dfd19813a9e24615546c72cbe2ca419218fd867ff0694c4a1b6a30
This commit is contained in:
fanquake 2023-05-26 12:15:35 +01:00
commit 4d13fe47be
No known key found for this signature in database
GPG key ID: 2EEB9F5CC09526C1
16 changed files with 70 additions and 66 deletions

View file

@ -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)

View file

@ -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 @@ public:
};
/** 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)

View file

@ -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

View file

@ -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);

View file

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

View file

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

View file

@ -7,26 +7,26 @@
#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;
@ -34,6 +34,6 @@ std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& args, BlockM
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

View file

@ -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

View file

@ -11,16 +11,16 @@
#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;
@ -28,7 +28,7 @@ std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& args, Chains
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

View file

@ -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

View file

@ -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.GetChainTypeString());
return util::Error{strprintf(Untranslated("acceptnonstdtxn is not currently supported for %s chain"), chainparams.GetChainTypeString())};
}
mempool_opts.full_rbf = argsman.GetBoolArg("-mempoolfullrbf", mempool_opts.full_rbf);
ApplyArgsManOptions(argsman, mempool_opts.limits);
return std::nullopt;
return {};
}

View file

@ -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

View file

@ -22,8 +22,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;
}

View file

@ -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()

View file

@ -11,6 +11,7 @@
#include <kernel/cs_main.h>
#include <sync.h>
#include <util/fs.h>
#include <util/result.h>
#include <memory>
#include <optional>
@ -98,6 +99,6 @@ public:
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

View file

@ -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)} {}