diff options
Diffstat (limited to 'src')
64 files changed, 742 insertions, 412 deletions
diff --git a/src/.clang-format b/src/.clang-format index 791b3b8f9f..2e5d5c6449 100644 --- a/src/.clang-format +++ b/src/.clang-format @@ -43,5 +43,5 @@ SpacesInAngles: false SpacesInContainerLiterals: true SpacesInCStyleCastParentheses: false SpacesInParentheses: false -Standard: c++17 +Standard: c++20 UseTab: Never diff --git a/src/Makefile.bench.include b/src/Makefile.bench.include index 28b779a5a8..217f123b16 100644 --- a/src/Makefile.bench.include +++ b/src/Makefile.bench.include @@ -84,6 +84,7 @@ endif if ENABLE_WALLET bench_bench_bitcoin_SOURCES += bench/coin_selection.cpp bench_bench_bitcoin_SOURCES += bench/wallet_balance.cpp +bench_bench_bitcoin_SOURCES += bench/wallet_create.cpp bench_bench_bitcoin_SOURCES += bench/wallet_loading.cpp bench_bench_bitcoin_SOURCES += bench/wallet_create_tx.cpp bench_bench_bitcoin_LDADD += $(BDB_LIBS) $(SQLITE_LIBS) diff --git a/src/addrman.cpp b/src/addrman.cpp index 5a11526471..a8206de6ee 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -802,7 +802,7 @@ int AddrManImpl::GetEntry(bool use_tried, size_t bucket, size_t position) const return -1; } -std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network) const +std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const { AssertLockHeld(cs); @@ -832,7 +832,7 @@ std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct if (network != std::nullopt && ai.GetNetClass() != network) continue; // Filter for quality - if (ai.IsTerrible(now)) continue; + if (ai.IsTerrible(now) && filtered) continue; addresses.push_back(ai); } @@ -1216,11 +1216,11 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select(bool new_only, std::optiona return addrRet; } -std::vector<CAddress> AddrManImpl::GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const +std::vector<CAddress> AddrManImpl::GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const { LOCK(cs); Check(); - auto addresses = GetAddr_(max_addresses, max_pct, network); + auto addresses = GetAddr_(max_addresses, max_pct, network, filtered); Check(); return addresses; } @@ -1319,9 +1319,9 @@ std::pair<CAddress, NodeSeconds> AddrMan::Select(bool new_only, std::optional<Ne return m_impl->Select(new_only, network); } -std::vector<CAddress> AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const +std::vector<CAddress> AddrMan::GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const { - return m_impl->GetAddr(max_addresses, max_pct, network); + return m_impl->GetAddr(max_addresses, max_pct, network, filtered); } std::vector<std::pair<AddrInfo, AddressPosition>> AddrMan::GetEntries(bool use_tried) const diff --git a/src/addrman.h b/src/addrman.h index 67dc7604a4..ef9c766eff 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -164,10 +164,11 @@ public: * @param[in] max_addresses Maximum number of addresses to return (0 = all). * @param[in] max_pct Maximum percentage of addresses to return (0 = all). * @param[in] network Select only addresses of this network (nullopt = all). + * @param[in] filtered Select only addresses that are considered good quality (false = all). * * @return A vector of randomly selected addresses from vRandom. */ - std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const; + std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const; /** * Returns an information-location pair for all addresses in the selected addrman table. diff --git a/src/addrman_impl.h b/src/addrman_impl.h index 512f085a21..867c894d01 100644 --- a/src/addrman_impl.h +++ b/src/addrman_impl.h @@ -129,7 +129,7 @@ public: std::pair<CAddress, NodeSeconds> Select(bool new_only, std::optional<Network> network) const EXCLUSIVE_LOCKS_REQUIRED(!cs); - std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const + std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const EXCLUSIVE_LOCKS_REQUIRED(!cs); std::vector<std::pair<AddrInfo, AddressPosition>> GetEntries(bool from_tried) const @@ -261,7 +261,7 @@ private: * */ int GetEntry(bool use_tried, size_t bucket, size_t position) const EXCLUSIVE_LOCKS_REQUIRED(cs); - std::vector<CAddress> GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network) const EXCLUSIVE_LOCKS_REQUIRED(cs); + std::vector<CAddress> GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const EXCLUSIVE_LOCKS_REQUIRED(cs); std::vector<std::pair<AddrInfo, AddressPosition>> GetEntries_(bool from_tried) const EXCLUSIVE_LOCKS_REQUIRED(cs); diff --git a/src/arith_uint256.cpp b/src/arith_uint256.cpp index 3776cfb6de..0d5b3d5b0e 100644 --- a/src/arith_uint256.cpp +++ b/src/arith_uint256.cpp @@ -8,14 +8,7 @@ #include <uint256.h> #include <crypto/common.h> - -template <unsigned int BITS> -base_uint<BITS>::base_uint(const std::string& str) -{ - static_assert(BITS/32 > 0 && BITS%32 == 0, "Template parameter BITS must be a positive multiple of 32."); - - SetHex(str); -} +#include <cassert> template <unsigned int BITS> base_uint<BITS>& base_uint<BITS>::operator<<=(unsigned int shift) @@ -154,22 +147,6 @@ std::string base_uint<BITS>::GetHex() const } template <unsigned int BITS> -void base_uint<BITS>::SetHex(const char* psz) -{ - base_blob<BITS> b; - b.SetHex(psz); - for (int x = 0; x < this->WIDTH; ++x) { - this->pn[x] = ReadLE32(b.begin() + x*4); - } -} - -template <unsigned int BITS> -void base_uint<BITS>::SetHex(const std::string& str) -{ - SetHex(str.c_str()); -} - -template <unsigned int BITS> std::string base_uint<BITS>::ToString() const { return GetHex(); diff --git a/src/arith_uint256.h b/src/arith_uint256.h index c710fe9471..ba36cebbdc 100644 --- a/src/arith_uint256.h +++ b/src/arith_uint256.h @@ -6,10 +6,10 @@ #ifndef BITCOIN_ARITH_UINT256_H #define BITCOIN_ARITH_UINT256_H +#include <cstdint> #include <cstring> #include <limits> #include <stdexcept> -#include <stdint.h> #include <string> class uint256; @@ -56,8 +56,6 @@ public: pn[i] = 0; } - explicit base_uint(const std::string& str); - base_uint operator~() const { base_uint ret; @@ -219,8 +217,6 @@ public: friend inline bool operator!=(const base_uint& a, uint64_t b) { return !a.EqualTo(b); } std::string GetHex() const; - void SetHex(const char* psz); - void SetHex(const std::string& str); std::string ToString() const; unsigned int size() const @@ -247,7 +243,6 @@ public: arith_uint256() {} arith_uint256(const base_uint<256>& b) : base_uint<256>(b) {} arith_uint256(uint64_t b) : base_uint<256>(b) {} - explicit arith_uint256(const std::string& str) : base_uint<256>(str) {} /** * The "compact" format is a representation of a whole diff --git a/src/bench/rpc_blockchain.cpp b/src/bench/rpc_blockchain.cpp index 2416d40798..713853e8c5 100644 --- a/src/bench/rpc_blockchain.cpp +++ b/src/bench/rpc_blockchain.cpp @@ -41,7 +41,7 @@ static void BlockToJsonVerbose(benchmark::Bench& bench) { TestBlockAndIndex data; bench.run([&] { - auto univalue = blockToJSON(data.testing_setup->m_node.chainman->m_blockman, data.block, &data.blockindex, &data.blockindex, TxVerbosity::SHOW_DETAILS_AND_PREVOUT); + auto univalue = blockToJSON(data.testing_setup->m_node.chainman->m_blockman, data.block, data.blockindex, data.blockindex, TxVerbosity::SHOW_DETAILS_AND_PREVOUT); ankerl::nanobench::doNotOptimizeAway(univalue); }); } @@ -51,7 +51,7 @@ BENCHMARK(BlockToJsonVerbose, benchmark::PriorityLevel::HIGH); static void BlockToJsonVerboseWrite(benchmark::Bench& bench) { TestBlockAndIndex data; - auto univalue = blockToJSON(data.testing_setup->m_node.chainman->m_blockman, data.block, &data.blockindex, &data.blockindex, TxVerbosity::SHOW_DETAILS_AND_PREVOUT); + auto univalue = blockToJSON(data.testing_setup->m_node.chainman->m_blockman, data.block, data.blockindex, data.blockindex, TxVerbosity::SHOW_DETAILS_AND_PREVOUT); bench.run([&] { auto str = univalue.write(); ankerl::nanobench::doNotOptimizeAway(str); diff --git a/src/bench/wallet_create.cpp b/src/bench/wallet_create.cpp new file mode 100644 index 0000000000..ba3c25d25e --- /dev/null +++ b/src/bench/wallet_create.cpp @@ -0,0 +1,55 @@ +// Copyright (c) 2023-present The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://www.opensource.org/licenses/mit-license.php. + +#include <bench/bench.h> +#include <node/context.h> +#include <random.h> +#include <test/util/setup_common.h> +#include <wallet/context.h> +#include <wallet/wallet.h> + +namespace wallet { +static void WalletCreate(benchmark::Bench& bench, bool encrypted) +{ + auto test_setup = MakeNoLogFileContext<TestingSetup>(); + FastRandomContext random; + + WalletContext context; + context.args = &test_setup->m_args; + context.chain = test_setup->m_node.chain.get(); + + DatabaseOptions options; + options.require_format = DatabaseFormat::SQLITE; + options.require_create = true; + options.create_flags = WALLET_FLAG_DESCRIPTORS; + + if (encrypted) { + options.create_passphrase = random.rand256().ToString(); + } + + DatabaseStatus status; + bilingual_str error_string; + std::vector<bilingual_str> warnings; + + fs::path wallet_path = test_setup->m_path_root / strprintf("test_wallet_%d", random.rand32()).c_str(); + bench.run([&] { + auto wallet = CreateWallet(context, wallet_path.u8string(), /*load_on_start=*/std::nullopt, options, status, error_string, warnings); + assert(status == DatabaseStatus::SUCCESS); + assert(wallet != nullptr); + + // Cleanup + wallet.reset(); + fs::remove_all(wallet_path); + }); +} + +static void WalletCreatePlain(benchmark::Bench& bench) { WalletCreate(bench, /*encrypted=*/false); } +static void WalletCreateEncrypted(benchmark::Bench& bench) { WalletCreate(bench, /*encrypted=*/true); } + +#ifdef USE_SQLITE +BENCHMARK(WalletCreatePlain, benchmark::PriorityLevel::LOW); +BENCHMARK(WalletCreateEncrypted, benchmark::PriorityLevel::LOW); +#endif + +} // namespace wallet diff --git a/src/bench/wallet_create_tx.cpp b/src/bench/wallet_create_tx.cpp index 632918c0ca..c0ca8f983d 100644 --- a/src/bench/wallet_create_tx.cpp +++ b/src/bench/wallet_create_tx.cpp @@ -16,7 +16,6 @@ using wallet::CWallet; using wallet::CreateMockableWalletDatabase; -using wallet::DBErrors; using wallet::WALLET_FLAG_DESCRIPTORS; struct TipBlock @@ -90,7 +89,6 @@ static void WalletCreateTx(benchmark::Bench& bench, const OutputType output_type LOCK(wallet.cs_wallet); wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); wallet.SetupDescriptorScriptPubKeyMans(); - if (wallet.LoadWallet() != DBErrors::LOAD_OK) assert(false); } // Generate destinations @@ -146,7 +144,6 @@ static void AvailableCoins(benchmark::Bench& bench, const std::vector<OutputType LOCK(wallet.cs_wallet); wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); wallet.SetupDescriptorScriptPubKeyMans(); - if (wallet.LoadWallet() != DBErrors::LOAD_OK) assert(false); } // Generate destinations diff --git a/src/init.cpp b/src/init.cpp index 526a8d6271..ac52b34fc5 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -155,6 +155,11 @@ static const char* DEFAULT_ASMAP_FILENAME="ip_asn.map"; * The PID file facilities. */ static const char* BITCOIN_PID_FILENAME = "bitcoind.pid"; +/** + * True if this process has created a PID file. + * Used to determine whether we should remove the PID file on shutdown. + */ +static bool g_generated_pid{false}; static fs::path GetPidFile(const ArgsManager& args) { @@ -170,12 +175,24 @@ static fs::path GetPidFile(const ArgsManager& args) #else tfm::format(file, "%d\n", getpid()); #endif + g_generated_pid = true; return true; } else { return InitError(strprintf(_("Unable to create the PID file '%s': %s"), fs::PathToString(GetPidFile(args)), SysErrorString(errno))); } } +static void RemovePidFile(const ArgsManager& args) +{ + if (!g_generated_pid) return; + const auto pid_path{GetPidFile(args)}; + if (std::error_code error; !fs::remove(pid_path, error)) { + std::string msg{error ? error.message() : "File does not exist"}; + LogPrintf("Unable to remove PID file (%s): %s\n", fs::PathToString(pid_path), msg); + } +} + + ////////////////////////////////////////////////////////////////////////////// // // Shutdown @@ -352,13 +369,7 @@ void Shutdown(NodeContext& node) node.scheduler.reset(); node.kernel.reset(); - try { - if (!fs::remove(GetPidFile(*node.args))) { - LogPrintf("%s: Unable to remove PID file: File does not exist\n", __func__); - } - } catch (const fs::filesystem_error& e) { - LogPrintf("%s: Unable to remove PID file: %s\n", __func__, fsbridge::get_filesystem_error_message(e)); - } + RemovePidFile(*node.args); LogPrintf("%s: done\n", __func__); } diff --git a/src/net.cpp b/src/net.cpp index dc76fdfb44..102d81579f 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -3278,6 +3278,12 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) // Dump network addresses scheduler.scheduleEvery([this] { DumpAddresses(); }, DUMP_PEERS_INTERVAL); + // Run the ASMap Health check once and then schedule it to run every 24h. + if (m_netgroupman.UsingASMap()) { + ASMapHealthCheck(); + scheduler.scheduleEvery([this] { ASMapHealthCheck(); }, ASMAP_HEALTH_CHECK_INTERVAL); + } + return true; } @@ -3383,9 +3389,9 @@ CConnman::~CConnman() Stop(); } -std::vector<CAddress> CConnman::GetAddresses(size_t max_addresses, size_t max_pct, std::optional<Network> network) const +std::vector<CAddress> CConnman::GetAddresses(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const { - std::vector<CAddress> addresses = addrman.GetAddr(max_addresses, max_pct, network); + std::vector<CAddress> addresses = addrman.GetAddr(max_addresses, max_pct, network, filtered); if (m_banman) { addresses.erase(std::remove_if(addresses.begin(), addresses.end(), [this](const CAddress& addr){return m_banman->IsDiscouraged(addr) || m_banman->IsBanned(addr);}), @@ -3840,6 +3846,19 @@ void CConnman::PerformReconnections() } } +void CConnman::ASMapHealthCheck() +{ + const std::vector<CAddress> v4_addrs{GetAddresses(/*max_addresses=*/ 0, /*max_pct=*/ 0, Network::NET_IPV4, /*filtered=*/ false)}; + const std::vector<CAddress> v6_addrs{GetAddresses(/*max_addresses=*/ 0, /*max_pct=*/ 0, Network::NET_IPV6, /*filtered=*/ false)}; + std::vector<CNetAddr> clearnet_addrs; + clearnet_addrs.reserve(v4_addrs.size() + v6_addrs.size()); + std::transform(v4_addrs.begin(), v4_addrs.end(), std::back_inserter(clearnet_addrs), + [](const CAddress& addr) { return static_cast<CNetAddr>(addr); }); + std::transform(v6_addrs.begin(), v6_addrs.end(), std::back_inserter(clearnet_addrs), + [](const CAddress& addr) { return static_cast<CNetAddr>(addr); }); + m_netgroupman.ASMapHealthCheck(clearnet_addrs); +} + // Dump binary message to file, with timestamp. static void CaptureMessageToFile(const CAddress& addr, const std::string& msg_type, @@ -88,6 +88,8 @@ static const bool DEFAULT_BLOCKSONLY = false; static const int64_t DEFAULT_PEER_CONNECT_TIMEOUT = 60; /** Number of file descriptors required for message capture **/ static const int NUM_FDS_MESSAGE_CAPTURE = 1; +/** Interval for ASMap Health Check **/ +static constexpr std::chrono::hours ASMAP_HEALTH_CHECK_INTERVAL{24}; static constexpr bool DEFAULT_FORCEDNSSEED{false}; static constexpr bool DEFAULT_DNSSEED{true}; @@ -1112,6 +1114,7 @@ public: void SetNetworkActive(bool active); void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant&& grant_outbound, const char* strDest, ConnectionType conn_type, bool use_v2transport) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex); bool CheckIncomingNonce(uint64_t nonce); + void ASMapHealthCheck(); // alias for thread safety annotations only, not defined RecursiveMutex& GetNodesMutex() const LOCK_RETURNED(m_nodes_mutex); @@ -1146,8 +1149,9 @@ public: * @param[in] max_addresses Maximum number of addresses to return (0 = all). * @param[in] max_pct Maximum percentage of addresses to return (0 = all). * @param[in] network Select only addresses of this network (nullopt = all). + * @param[in] filtered Select only addresses that are considered high quality (false = all). */ - std::vector<CAddress> GetAddresses(size_t max_addresses, size_t max_pct, std::optional<Network> network) const; + std::vector<CAddress> GetAddresses(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered = true) const; /** * Cache is used to minimize topology leaks, so it should * be used for all non-trusted calls, for example, p2p. diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1067341495..df54a62f28 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4298,7 +4298,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, m_txrequest.ForgetTxHash(tx.GetWitnessHash()); // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789) - m_orphanage.LimitOrphans(m_opts.max_orphan_txs); + m_orphanage.LimitOrphans(m_opts.max_orphan_txs, m_rng); } else { LogPrint(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s (wtxid=%s)\n", tx.GetHash().ToString(), diff --git a/src/netgroup.cpp b/src/netgroup.cpp index a03927b152..0ae229b3f3 100644 --- a/src/netgroup.cpp +++ b/src/netgroup.cpp @@ -5,6 +5,7 @@ #include <netgroup.h> #include <hash.h> +#include <logging.h> #include <util/asmap.h> uint256 NetGroupManager::GetAsmapChecksum() const @@ -109,3 +110,23 @@ uint32_t NetGroupManager::GetMappedAS(const CNetAddr& address) const uint32_t mapped_as = Interpret(m_asmap, ip_bits); return mapped_as; } + +void NetGroupManager::ASMapHealthCheck(const std::vector<CNetAddr>& clearnet_addrs) const { + std::set<uint32_t> clearnet_asns{}; + int unmapped_count{0}; + + for (const auto& addr : clearnet_addrs) { + uint32_t asn = GetMappedAS(addr); + if (asn == 0) { + ++unmapped_count; + continue; + } + clearnet_asns.insert(asn); + } + + LogPrintf("ASMap Health Check: %i clearnet peers are mapped to %i ASNs with %i peers being unmapped\n", clearnet_addrs.size(), clearnet_asns.size(), unmapped_count); +} + +bool NetGroupManager::UsingASMap() const { + return m_asmap.size() > 0; +} diff --git a/src/netgroup.h b/src/netgroup.h index 2dd63ec66b..5aa6ef7742 100644 --- a/src/netgroup.h +++ b/src/netgroup.h @@ -41,6 +41,16 @@ public: */ uint32_t GetMappedAS(const CNetAddr& address) const; + /** + * Analyze and log current health of ASMap based buckets. + */ + void ASMapHealthCheck(const std::vector<CNetAddr>& clearnet_addrs) const; + + /** + * Indicates whether ASMap is being used for clearnet bucketing. + */ + bool UsingASMap() const; + private: /** Compressed IP->ASN mapping, loaded from a file when a node starts. * diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index ebc08d7567..e6164c2e59 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -582,10 +582,10 @@ const CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpointData& data) return nullptr; } -bool BlockManager::IsBlockPruned(const CBlockIndex* pblockindex) +bool BlockManager::IsBlockPruned(const CBlockIndex& block) { AssertLockHeld(::cs_main); - return (m_have_pruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0); + return m_have_pruned && !(block.nStatus & BLOCK_HAVE_DATA) && (block.nTx > 0); } const CBlockIndex* BlockManager::GetFirstStoredBlock(const CBlockIndex& upper_block, const CBlockIndex* lower_block) diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index d540406ae5..ce514cc645 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -344,7 +344,7 @@ public: bool m_have_pruned = false; //! Check whether the block associated with this index entry is pruned or not. - bool IsBlockPruned(const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + bool IsBlockPruned(const CBlockIndex& block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); //! Create or update a prune lock identified by its name void UpdatePruneLock(const std::string& name, const PruneLockInfo& lock_info) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); diff --git a/src/rest.cpp b/src/rest.cpp index bb54e780b2..e47b52fb53 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -264,7 +264,7 @@ static bool rest_headers(const std::any& context, case RESTResponseFormat::JSON: { UniValue jsonHeaders(UniValue::VARR); for (const CBlockIndex *pindex : headers) { - jsonHeaders.push_back(blockheaderToJSON(tip, pindex)); + jsonHeaders.push_back(blockheaderToJSON(*tip, *pindex)); } std::string strJSON = jsonHeaders.write() + "\n"; req->WriteHeader("Content-Type", "application/json"); @@ -304,10 +304,9 @@ static bool rest_block(const std::any& context, if (!pblockindex) { return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found"); } - - if (chainman.m_blockman.IsBlockPruned(pblockindex)) + if (chainman.m_blockman.IsBlockPruned(*pblockindex)) { return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not available (pruned data)"); - + } } if (!chainman.m_blockman.ReadBlockFromDisk(block, *pblockindex)) { @@ -334,7 +333,7 @@ static bool rest_block(const std::any& context, } case RESTResponseFormat::JSON: { - UniValue objBlock = blockToJSON(chainman.m_blockman, block, tip, pblockindex, tx_verbosity); + UniValue objBlock = blockToJSON(chainman.m_blockman, block, *tip, *pblockindex, tx_verbosity); std::string strJSON = objBlock.write() + "\n"; req->WriteHeader("Content-Type", "application/json"); req->WriteReply(HTTP_OK, strJSON); diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index be6a8c47fd..6f20b1031c 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -73,13 +73,11 @@ static CUpdatedBlock latestblock GUARDED_BY(cs_blockchange); /* Calculate the difficulty for a given block index. */ -double GetDifficulty(const CBlockIndex* blockindex) +double GetDifficulty(const CBlockIndex& blockindex) { - CHECK_NONFATAL(blockindex); - - int nShift = (blockindex->nBits >> 24) & 0xff; + int nShift = (blockindex.nBits >> 24) & 0xff; double dDiff = - (double)0x0000ffff / (double)(blockindex->nBits & 0x00ffffff); + (double)0x0000ffff / (double)(blockindex.nBits & 0x00ffffff); while (nShift < 29) { @@ -95,14 +93,14 @@ double GetDifficulty(const CBlockIndex* blockindex) return dDiff; } -static int ComputeNextBlockAndDepth(const CBlockIndex* tip, const CBlockIndex* blockindex, const CBlockIndex*& next) +static int ComputeNextBlockAndDepth(const CBlockIndex& tip, const CBlockIndex& blockindex, const CBlockIndex*& next) { - next = tip->GetAncestor(blockindex->nHeight + 1); - if (next && next->pprev == blockindex) { - return tip->nHeight - blockindex->nHeight + 1; + next = tip.GetAncestor(blockindex.nHeight + 1); + if (next && next->pprev == &blockindex) { + return tip.nHeight - blockindex.nHeight + 1; } next = nullptr; - return blockindex == tip ? 1 : -1; + return &blockindex == &tip ? 1 : -1; } static const CBlockIndex* ParseHashOrHeight(const UniValue& param, ChainstateManager& chainman) @@ -133,36 +131,36 @@ static const CBlockIndex* ParseHashOrHeight(const UniValue& param, ChainstateMan } } -UniValue blockheaderToJSON(const CBlockIndex* tip, const CBlockIndex* blockindex) +UniValue blockheaderToJSON(const CBlockIndex& tip, const CBlockIndex& blockindex) { // Serialize passed information without accessing chain state of the active chain! AssertLockNotHeld(cs_main); // For performance reasons UniValue result(UniValue::VOBJ); - result.pushKV("hash", blockindex->GetBlockHash().GetHex()); + result.pushKV("hash", blockindex.GetBlockHash().GetHex()); const CBlockIndex* pnext; int confirmations = ComputeNextBlockAndDepth(tip, blockindex, pnext); result.pushKV("confirmations", confirmations); - result.pushKV("height", blockindex->nHeight); - result.pushKV("version", blockindex->nVersion); - result.pushKV("versionHex", strprintf("%08x", blockindex->nVersion)); - result.pushKV("merkleroot", blockindex->hashMerkleRoot.GetHex()); - result.pushKV("time", (int64_t)blockindex->nTime); - result.pushKV("mediantime", (int64_t)blockindex->GetMedianTimePast()); - result.pushKV("nonce", (uint64_t)blockindex->nNonce); - result.pushKV("bits", strprintf("%08x", blockindex->nBits)); + result.pushKV("height", blockindex.nHeight); + result.pushKV("version", blockindex.nVersion); + result.pushKV("versionHex", strprintf("%08x", blockindex.nVersion)); + result.pushKV("merkleroot", blockindex.hashMerkleRoot.GetHex()); + result.pushKV("time", blockindex.nTime); + result.pushKV("mediantime", blockindex.GetMedianTimePast()); + result.pushKV("nonce", blockindex.nNonce); + result.pushKV("bits", strprintf("%08x", blockindex.nBits)); result.pushKV("difficulty", GetDifficulty(blockindex)); - result.pushKV("chainwork", blockindex->nChainWork.GetHex()); - result.pushKV("nTx", (uint64_t)blockindex->nTx); + result.pushKV("chainwork", blockindex.nChainWork.GetHex()); + result.pushKV("nTx", blockindex.nTx); - if (blockindex->pprev) - result.pushKV("previousblockhash", blockindex->pprev->GetBlockHash().GetHex()); + if (blockindex.pprev) + result.pushKV("previousblockhash", blockindex.pprev->GetBlockHash().GetHex()); if (pnext) result.pushKV("nextblockhash", pnext->GetBlockHash().GetHex()); return result; } -UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIndex* tip, const CBlockIndex* blockindex, TxVerbosity verbosity) +UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIndex& tip, const CBlockIndex& blockindex, TxVerbosity verbosity) { UniValue result = blockheaderToJSON(tip, blockindex); @@ -182,7 +180,7 @@ UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIn case TxVerbosity::SHOW_DETAILS_AND_PREVOUT: CBlockUndo blockUndo; const bool is_not_pruned{WITH_LOCK(::cs_main, return !blockman.IsBlockPruned(blockindex))}; - const bool have_undo{is_not_pruned && blockman.UndoReadFromDisk(blockUndo, *blockindex)}; + const bool have_undo{is_not_pruned && blockman.UndoReadFromDisk(blockUndo, blockindex)}; for (size_t i = 0; i < block.vtx.size(); ++i) { const CTransactionRef& tx = block.vtx.at(i); @@ -418,7 +416,7 @@ static RPCHelpMan getdifficulty() { ChainstateManager& chainman = EnsureAnyChainman(request.context); LOCK(cs_main); - return GetDifficulty(chainman.ActiveChain().Tip()); + return GetDifficulty(*CHECK_NONFATAL(chainman.ActiveChain().Tip())); }, }; } @@ -571,22 +569,22 @@ static RPCHelpMan getblockheader() return strHex; } - return blockheaderToJSON(tip, pblockindex); + return blockheaderToJSON(*tip, *pblockindex); }, }; } -static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex* pblockindex) +static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex& blockindex) { CBlock block; { LOCK(cs_main); - if (blockman.IsBlockPruned(pblockindex)) { + if (blockman.IsBlockPruned(blockindex)) { throw JSONRPCError(RPC_MISC_ERROR, "Block not available (pruned data)"); } } - if (!blockman.ReadBlockFromDisk(block, *pblockindex)) { + if (!blockman.ReadBlockFromDisk(block, blockindex)) { // Block not found on disk. This could be because we have the block // header in our index but not yet have the block or did not accept the // block. Or if the block was pruned right after we released the lock above. @@ -596,21 +594,21 @@ static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex* pblocki return block; } -static CBlockUndo GetUndoChecked(BlockManager& blockman, const CBlockIndex* pblockindex) +static CBlockUndo GetUndoChecked(BlockManager& blockman, const CBlockIndex& blockindex) { CBlockUndo blockUndo; // The Genesis block does not have undo data - if (pblockindex->nHeight == 0) return blockUndo; + if (blockindex.nHeight == 0) return blockUndo; { LOCK(cs_main); - if (blockman.IsBlockPruned(pblockindex)) { + if (blockman.IsBlockPruned(blockindex)) { throw JSONRPCError(RPC_MISC_ERROR, "Undo data not available (pruned data)"); } } - if (!blockman.UndoReadFromDisk(blockUndo, *pblockindex)) { + if (!blockman.UndoReadFromDisk(blockUndo, blockindex)) { throw JSONRPCError(RPC_MISC_ERROR, "Can't read undo data from disk"); } @@ -736,7 +734,7 @@ static RPCHelpMan getblock() } } - const CBlock block{GetBlockChecked(chainman.m_blockman, pblockindex)}; + const CBlock block{GetBlockChecked(chainman.m_blockman, *pblockindex)}; if (verbosity <= 0) { @@ -755,7 +753,7 @@ static RPCHelpMan getblock() tx_verbosity = TxVerbosity::SHOW_DETAILS_AND_PREVOUT; } - return blockToJSON(chainman.m_blockman, block, tip, pblockindex, tx_verbosity); + return blockToJSON(chainman.m_blockman, block, *tip, *pblockindex, tx_verbosity); }, }; } @@ -1257,7 +1255,7 @@ RPCHelpMan getblockchaininfo() obj.pushKV("blocks", height); obj.pushKV("headers", chainman.m_best_header ? chainman.m_best_header->nHeight : -1); obj.pushKV("bestblockhash", tip.GetBlockHash().GetHex()); - obj.pushKV("difficulty", GetDifficulty(&tip)); + obj.pushKV("difficulty", GetDifficulty(tip)); obj.pushKV("time", tip.GetBlockTime()); obj.pushKV("mediantime", tip.GetMedianTimePast()); obj.pushKV("verificationprogress", GuessVerificationProgress(chainman.GetParams().TxData(), &tip)); @@ -1815,8 +1813,8 @@ static RPCHelpMan getblockstats() } } - const CBlock& block = GetBlockChecked(chainman.m_blockman, &pindex); - const CBlockUndo& blockUndo = GetUndoChecked(chainman.m_blockman, &pindex); + const CBlock& block = GetBlockChecked(chainman.m_blockman, pindex); + const CBlockUndo& blockUndo = GetUndoChecked(chainman.m_blockman, pindex); const bool do_all = stats.size() == 0; // Calculate everything if nothing selected (default) const bool do_mediantxsize = do_all || stats.count("mediantxsize") != 0; @@ -2275,8 +2273,8 @@ public: static bool CheckBlockFilterMatches(BlockManager& blockman, const CBlockIndex& blockindex, const GCSFilter::ElementSet& needles) { - const CBlock block{GetBlockChecked(blockman, &blockindex)}; - const CBlockUndo block_undo{GetUndoChecked(blockman, &blockindex)}; + const CBlock block{GetBlockChecked(blockman, blockindex)}; + const CBlockUndo block_undo{GetUndoChecked(blockman, blockindex)}; // Check if any of the outputs match the scriptPubKey for (const auto& tx : block.vtx) { @@ -2845,7 +2843,7 @@ return RPCHelpMan{ data.pushKV("blocks", (int)chain.Height()); data.pushKV("bestblockhash", tip->GetBlockHash().GetHex()); - data.pushKV("difficulty", (double)GetDifficulty(tip)); + data.pushKV("difficulty", GetDifficulty(*tip)); data.pushKV("verificationprogress", GuessVerificationProgress(Params().TxData(), tip)); data.pushKV("coins_db_cache_bytes", cs.m_coinsdb_cache_size_bytes); data.pushKV("coins_tip_cache_bytes", cs.m_coinstip_cache_size_bytes); diff --git a/src/rpc/blockchain.h b/src/rpc/blockchain.h index 0a86085db0..c2021c3608 100644 --- a/src/rpc/blockchain.h +++ b/src/rpc/blockchain.h @@ -32,16 +32,16 @@ static constexpr int NUM_GETBLOCKSTATS_PERCENTILES = 5; * @return A floating point number that is a multiple of the main net minimum * difficulty (4295032833 hashes). */ -double GetDifficulty(const CBlockIndex* blockindex); +double GetDifficulty(const CBlockIndex& blockindex); /** Callback for when block tip changed. */ void RPCNotifyBlockChange(const CBlockIndex*); /** Block description to JSON */ -UniValue blockToJSON(node::BlockManager& blockman, const CBlock& block, const CBlockIndex* tip, const CBlockIndex* blockindex, TxVerbosity verbosity) LOCKS_EXCLUDED(cs_main); +UniValue blockToJSON(node::BlockManager& blockman, const CBlock& block, const CBlockIndex& tip, const CBlockIndex& blockindex, TxVerbosity verbosity) LOCKS_EXCLUDED(cs_main); /** Block header to JSON */ -UniValue blockheaderToJSON(const CBlockIndex* tip, const CBlockIndex* blockindex) LOCKS_EXCLUDED(cs_main); +UniValue blockheaderToJSON(const CBlockIndex& tip, const CBlockIndex& blockindex) LOCKS_EXCLUDED(cs_main); /** Used by getblockstats to get feerates at different percentiles by weight */ void CalculatePercentilesByWeight(CAmount result[NUM_GETBLOCKSTATS_PERCENTILES], std::vector<std::pair<CAmount, int64_t>>& scores, int64_t total_weight); diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 3d80656507..b2332c5fdc 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -441,7 +441,7 @@ static RPCHelpMan getmininginfo() obj.pushKV("blocks", active_chain.Height()); if (BlockAssembler::m_last_block_weight) obj.pushKV("currentblockweight", *BlockAssembler::m_last_block_weight); if (BlockAssembler::m_last_block_num_txs) obj.pushKV("currentblocktx", *BlockAssembler::m_last_block_num_txs); - obj.pushKV("difficulty", (double)GetDifficulty(active_chain.Tip())); + obj.pushKV("difficulty", GetDifficulty(*CHECK_NONFATAL(active_chain.Tip()))); obj.pushKV("networkhashps", getnetworkhashps().HandleRequest(request)); obj.pushKV("pooledtx", (uint64_t)mempool.size()); obj.pushKV("chain", chainman.GetParams().GetChainTypeString()); diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 2df2999523..b1e6d677f8 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -394,7 +394,7 @@ static RPCHelpMan getrawtransaction() // If request is verbosity >= 1 but no blockhash was given, then look up the blockindex if (request.params[2].isNull()) { LOCK(cs_main); - blockindex = chainman.m_blockman.LookupBlockIndex(hash_block); + blockindex = chainman.m_blockman.LookupBlockIndex(hash_block); // May be nullptr for mempool transactions } if (verbosity == 1) { TxToJSON(*tx, hash_block, result, chainman.ActiveChainstate()); @@ -403,10 +403,8 @@ static RPCHelpMan getrawtransaction() CBlockUndo blockUndo; CBlock block; - const bool is_block_pruned{WITH_LOCK(cs_main, return chainman.m_blockman.IsBlockPruned(blockindex))}; - if (tx->IsCoinBase() || - !blockindex || is_block_pruned || + if (tx->IsCoinBase() || !blockindex || WITH_LOCK(::cs_main, return chainman.m_blockman.IsBlockPruned(*blockindex)) || !(chainman.m_blockman.UndoReadFromDisk(blockUndo, *blockindex) && chainman.m_blockman.ReadBlockFromDisk(block, *blockindex))) { TxToJSON(*tx, hash_block, result, chainman.ActiveChainstate()); return result; diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index bfefc3ff97..5bd4f271cd 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -429,6 +429,24 @@ BOOST_AUTO_TEST_CASE(addrman_getaddr) BOOST_CHECK_EQUAL(addrman->Size(), 2006U); } +BOOST_AUTO_TEST_CASE(getaddr_unfiltered) +{ + auto addrman = std::make_unique<AddrMan>(EMPTY_NETGROUPMAN, DETERMINISTIC, GetCheckRatio(m_node)); + + // Set time on this addr so isTerrible = false + CAddress addr1 = CAddress(ResolveService("250.250.2.1", 8333), NODE_NONE); + addr1.nTime = Now<NodeSeconds>(); + // Not setting time so this addr should be isTerrible = true + CAddress addr2 = CAddress(ResolveService("250.251.2.2", 9999), NODE_NONE); + + CNetAddr source = ResolveIP("250.1.2.1"); + BOOST_CHECK(addrman->Add({addr1, addr2}, source)); + + // Filtered GetAddr should only return addr1 + BOOST_CHECK_EQUAL(addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt).size(), 1U); + // Unfiltered GetAddr should return addr1 and addr2 + BOOST_CHECK_EQUAL(addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt, /*filtered=*/false).size(), 2U); +} BOOST_AUTO_TEST_CASE(caddrinfo_get_tried_bucket_legacy) { diff --git a/src/test/arith_uint256_tests.cpp b/src/test/arith_uint256_tests.cpp index 6a37b7d83b..10028c7c93 100644 --- a/src/test/arith_uint256_tests.cpp +++ b/src/test/arith_uint256_tests.cpp @@ -22,6 +22,7 @@ static inline arith_uint256 arith_uint256V(const std::vector<unsigned char>& vch { return UintToArith256(uint256(vch)); } +static inline arith_uint256 arith_uint256S(const std::string& str) { return UintToArith256(uint256S(str)); } const unsigned char R1Array[] = "\x9c\x52\x4a\xdb\xcf\x56\x11\x12\x2b\x29\x12\x5e\x5d\x35\xd2\xd2" @@ -95,25 +96,25 @@ BOOST_AUTO_TEST_CASE( basics ) // constructors, equality, inequality BOOST_CHECK(ZeroL == (OneL << 256)); // String Constructor and Copy Constructor - BOOST_CHECK(arith_uint256("0x"+R1L.ToString()) == R1L); - BOOST_CHECK(arith_uint256("0x"+R2L.ToString()) == R2L); - BOOST_CHECK(arith_uint256("0x"+ZeroL.ToString()) == ZeroL); - BOOST_CHECK(arith_uint256("0x"+OneL.ToString()) == OneL); - BOOST_CHECK(arith_uint256("0x"+MaxL.ToString()) == MaxL); - BOOST_CHECK(arith_uint256(R1L.ToString()) == R1L); - BOOST_CHECK(arith_uint256(" 0x"+R1L.ToString()+" ") == R1L); - BOOST_CHECK(arith_uint256("") == ZeroL); - BOOST_CHECK(R1L == arith_uint256(R1ArrayHex)); + BOOST_CHECK(arith_uint256S("0x" + R1L.ToString()) == R1L); + BOOST_CHECK(arith_uint256S("0x" + R2L.ToString()) == R2L); + BOOST_CHECK(arith_uint256S("0x" + ZeroL.ToString()) == ZeroL); + BOOST_CHECK(arith_uint256S("0x" + OneL.ToString()) == OneL); + BOOST_CHECK(arith_uint256S("0x" + MaxL.ToString()) == MaxL); + BOOST_CHECK(arith_uint256S(R1L.ToString()) == R1L); + BOOST_CHECK(arith_uint256S(" 0x" + R1L.ToString() + " ") == R1L); + BOOST_CHECK(arith_uint256S("") == ZeroL); + BOOST_CHECK(R1L == arith_uint256S(R1ArrayHex)); BOOST_CHECK(arith_uint256(R1L) == R1L); BOOST_CHECK((arith_uint256(R1L^R2L)^R2L) == R1L); BOOST_CHECK(arith_uint256(ZeroL) == ZeroL); BOOST_CHECK(arith_uint256(OneL) == OneL); // uint64_t constructor - BOOST_CHECK( (R1L & arith_uint256("0xffffffffffffffff")) == arith_uint256(R1LLow64)); + BOOST_CHECK((R1L & arith_uint256S("0xffffffffffffffff")) == arith_uint256(R1LLow64)); BOOST_CHECK(ZeroL == arith_uint256(0)); BOOST_CHECK(OneL == arith_uint256(1)); - BOOST_CHECK(arith_uint256("0xffffffffffffffff") == arith_uint256(0xffffffffffffffffULL)); + BOOST_CHECK(arith_uint256S("0xffffffffffffffff") == arith_uint256(0xffffffffffffffffULL)); // Assignment (from base_uint) arith_uint256 tmpL = ~ZeroL; BOOST_CHECK(tmpL == ~ZeroL); @@ -282,7 +283,7 @@ BOOST_AUTO_TEST_CASE( comparison ) // <= >= < > BOOST_AUTO_TEST_CASE( plusMinus ) { arith_uint256 TmpL = 0; - BOOST_CHECK(R1L+R2L == arith_uint256(R1LplusR2L)); + BOOST_CHECK(R1L + R2L == arith_uint256S(R1LplusR2L)); TmpL += R1L; BOOST_CHECK(TmpL == R1L); TmpL += R2L; @@ -346,8 +347,8 @@ BOOST_AUTO_TEST_CASE( multiply ) BOOST_AUTO_TEST_CASE( divide ) { - arith_uint256 D1L("AD7133AC1977FA2B7"); - arith_uint256 D2L("ECD751716"); + arith_uint256 D1L{arith_uint256S("AD7133AC1977FA2B7")}; + arith_uint256 D2L{arith_uint256S("ECD751716")}; BOOST_CHECK((R1L / D1L).ToString() == "00000000000000000b8ac01106981635d9ed112290f8895545a7654dde28fb3a"); BOOST_CHECK((R1L / D2L).ToString() == "000000000873ce8efec5b67150bad3aa8c5fcb70e947586153bf2cec7c37c57a"); BOOST_CHECK(R1L / OneL == R1L); @@ -368,7 +369,7 @@ static bool almostEqual(double d1, double d2) return fabs(d1-d2) <= 4*fabs(d1)*std::numeric_limits<double>::epsilon(); } -BOOST_AUTO_TEST_CASE( methods ) // GetHex SetHex size() GetLow64 GetSerializeSize, Serialize, Unserialize +BOOST_AUTO_TEST_CASE(methods) // GetHex operator= size() GetLow64 GetSerializeSize, Serialize, Unserialize { BOOST_CHECK(R1L.GetHex() == R1L.ToString()); BOOST_CHECK(R2L.GetHex() == R2L.ToString()); @@ -376,11 +377,14 @@ BOOST_AUTO_TEST_CASE( methods ) // GetHex SetHex size() GetLow64 GetSerializeSiz BOOST_CHECK(MaxL.GetHex() == MaxL.ToString()); arith_uint256 TmpL(R1L); BOOST_CHECK(TmpL == R1L); - TmpL.SetHex(R2L.ToString()); BOOST_CHECK(TmpL == R2L); - TmpL.SetHex(ZeroL.ToString()); BOOST_CHECK(TmpL == 0); - TmpL.SetHex(HalfL.ToString()); BOOST_CHECK(TmpL == HalfL); + TmpL = R2L; + BOOST_CHECK(TmpL == R2L); + TmpL = ZeroL; + BOOST_CHECK(TmpL == 0); + TmpL = HalfL; + BOOST_CHECK(TmpL == HalfL); - TmpL.SetHex(R1L.ToString()); + TmpL = R1L; BOOST_CHECK(R1L.size() == 32); BOOST_CHECK(R2L.size() == 32); BOOST_CHECK(ZeroL.size() == 32); diff --git a/src/test/blockchain_tests.cpp b/src/test/blockchain_tests.cpp index b590467a43..be515a9eac 100644 --- a/src/test/blockchain_tests.cpp +++ b/src/test/blockchain_tests.cpp @@ -41,7 +41,7 @@ static void RejectDifficultyMismatch(double difficulty, double expected_difficul static void TestDifficulty(uint32_t nbits, double expected_difficulty) { CBlockIndex* block_index = CreateBlockIndexWithNbits(nbits); - double difficulty = GetDifficulty(block_index); + double difficulty = GetDifficulty(*block_index); delete block_index; RejectDifficultyMismatch(difficulty, expected_difficulty); diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index 1b11ff6fdf..ece396aadf 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -286,7 +286,8 @@ FUZZ_TARGET(addrman, .init = initialize_addrman) (void)const_addr_man.GetAddr( /*max_addresses=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096), /*max_pct=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 4096), - network); + network, + /*filtered=*/fuzzed_data_provider.ConsumeBool()); (void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool(), network); std::optional<bool> in_new; if (fuzzed_data_provider.ConsumeBool()) { diff --git a/src/test/fuzz/bitdeque.cpp b/src/test/fuzz/bitdeque.cpp index 65f5cb3fd0..d5cc9cfd34 100644 --- a/src/test/fuzz/bitdeque.cpp +++ b/src/test/fuzz/bitdeque.cpp @@ -53,21 +53,11 @@ FUZZ_TARGET(bitdeque, .init = InitRandData) --initlen; } - LIMITED_WHILE(provider.remaining_bytes() > 0, 900) + const auto iter_limit{maxlen > 6000 ? 90U : 900U}; + LIMITED_WHILE(provider.remaining_bytes() > 0, iter_limit) { - { - assert(deq.size() == bitdeq.size()); - auto it = deq.begin(); - auto bitit = bitdeq.begin(); - auto itend = deq.end(); - while (it != itend) { - assert(*it == *bitit); - ++it; - ++bitit; - } - } - - CallOneOf(provider, + CallOneOf( + provider, [&] { // constructor() deq = std::deque<bool>{}; @@ -535,7 +525,17 @@ FUZZ_TARGET(bitdeque, .init = InitRandData) assert(it == deq.begin() + before); assert(bitit == bitdeq.begin() + before); } - } - ); + }); + } + { + assert(deq.size() == bitdeq.size()); + auto it = deq.begin(); + auto bitit = bitdeq.begin(); + auto itend = deq.end(); + while (it != itend) { + assert(*it == *bitit); + ++it; + ++bitit; + } } } diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index 551e1c526d..24f91abd25 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -88,7 +88,8 @@ FUZZ_TARGET(connman, .init = initialize_connman) (void)connman.GetAddresses( /*max_addresses=*/fuzzed_data_provider.ConsumeIntegral<size_t>(), /*max_pct=*/fuzzed_data_provider.ConsumeIntegral<size_t>(), - /*network=*/std::nullopt); + /*network=*/std::nullopt, + /*filtered=*/fuzzed_data_provider.ConsumeBool()); }, [&] { (void)connman.GetAddresses( diff --git a/src/test/fuzz/fuzz.h b/src/test/fuzz/fuzz.h index 1f0fa5527a..ca74d53de7 100644 --- a/src/test/fuzz/fuzz.h +++ b/src/test/fuzz/fuzz.h @@ -33,11 +33,7 @@ struct FuzzTargetOptions { void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target, FuzzTargetOptions opts); -#if defined(__clang__) -#define FUZZ_TARGET(...) _Pragma("clang diagnostic push") _Pragma("clang diagnostic ignored \"-Wgnu-zero-variadic-macro-arguments\"") DETAIL_FUZZ(__VA_ARGS__) _Pragma("clang diagnostic pop") -#else #define FUZZ_TARGET(...) DETAIL_FUZZ(__VA_ARGS__) -#endif #define DETAIL_FUZZ(name, ...) \ void name##_fuzz_target(FuzzBufferType); \ diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index 6392f03d4e..acb03ac5fc 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -79,14 +79,23 @@ FUZZ_TARGET(process_message, .init = initialize_process_message) const auto mock_time = ConsumeTime(fuzzed_data_provider); SetMockTime(mock_time); + CSerializedNetMsg net_msg; + net_msg.m_type = random_message_type; // fuzzed_data_provider is fully consumed after this call, don't use it - DataStream random_bytes_data_stream{fuzzed_data_provider.ConsumeRemainingBytes<unsigned char>()}; - try { - g_setup->m_node.peerman->ProcessMessage(p2p_node, random_message_type, random_bytes_data_stream, - GetTime<std::chrono::microseconds>(), std::atomic<bool>{false}); - } catch (const std::ios_base::failure&) { + net_msg.data = fuzzed_data_provider.ConsumeRemainingBytes<unsigned char>(); + + connman.FlushSendBuffer(p2p_node); + (void)connman.ReceiveMsgFrom(p2p_node, std::move(net_msg)); + + bool more_work{true}; + while (more_work) { + p2p_node.fPauseSend = false; + try { + more_work = connman.ProcessMessagesOnce(p2p_node); + } catch (const std::ios_base::failure&) { + } + g_setup->m_node.peerman->SendMessages(&p2p_node); } - g_setup->m_node.peerman->SendMessages(&p2p_node); SyncWithValidationInterfaceQueue(); g_setup->m_node.connman->StopNodes(); } diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp index b7e2e04b4b..3f722f60ee 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -78,13 +78,17 @@ FUZZ_TARGET(process_messages, .init = initialize_process_messages) connman.FlushSendBuffer(random_node); (void)connman.ReceiveMsgFrom(random_node, std::move(net_msg)); - random_node.fPauseSend = false; - try { - connman.ProcessMessagesOnce(random_node); - } catch (const std::ios_base::failure&) { + bool more_work{true}; + while (more_work) { // Ensure that every message is eventually processed in some way or another + random_node.fPauseSend = false; + + try { + more_work = connman.ProcessMessagesOnce(random_node); + } catch (const std::ios_base::failure&) { + } + g_setup->m_node.peerman->SendMessages(&random_node); } - g_setup->m_node.peerman->SendMessages(&random_node); } SyncWithValidationInterfaceQueue(); g_setup->m_node.connman->StopNodes(); diff --git a/src/test/fuzz/rpc.cpp b/src/test/fuzz/rpc.cpp index 2189dc0067..2325bf0941 100644 --- a/src/test/fuzz/rpc.cpp +++ b/src/test/fuzz/rpc.cpp @@ -380,9 +380,7 @@ FUZZ_TARGET(rpc, .init = initialize_rpc) rpc_testing_setup->CallRPC(rpc_command, arguments); } catch (const UniValue& json_rpc_error) { const std::string error_msg{json_rpc_error.find_value("message").get_str()}; - // Once c++20 is allowed, starts_with can be used. - // if (error_msg.starts_with("Internal bug detected")) { - if (0 == error_msg.rfind("Internal bug detected", 0)) { + if (error_msg.starts_with("Internal bug detected")) { // Only allow the intentional internal bug assert(error_msg.find("trigger_internal_bug") != std::string::npos); } diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index e9ceb299fe..5423ba8920 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -33,6 +33,7 @@ void initialize_orphanage() FUZZ_TARGET(txorphan, .init = initialize_orphanage) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); + FastRandomContext limit_orphans_rng{/*fDeterministic=*/true}; SetMockTime(ConsumeTime(fuzzed_data_provider)); TxOrphanage orphanage; @@ -91,13 +92,13 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) { CTransactionRef ref = orphanage.GetTxToReconsider(peer_id); if (ref) { - bool have_tx = orphanage.HaveTx(GenTxid::Txid(ref->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(ref->GetHash())); + bool have_tx = orphanage.HaveTx(GenTxid::Txid(ref->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(ref->GetWitnessHash())); Assert(have_tx); } } }, [&] { - bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetHash())); + bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash())); // AddTx should return false if tx is too big or already have it // tx weight is unknown, we only check when tx is already in orphanage { @@ -105,7 +106,7 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) // have_tx == true -> add_tx == false Assert(!have_tx || !add_tx); } - have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetHash())); + have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash())); { bool add_tx = orphanage.AddTx(tx, peer_id); // if have_tx is still false, it must be too big @@ -114,12 +115,12 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) } }, [&] { - bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetHash())); + bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash())); // EraseTx should return 0 if m_orphans doesn't have the tx { Assert(have_tx == orphanage.EraseTx(tx->GetHash())); } - have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetHash())); + have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash())); // have_tx should be false and EraseTx should fail { Assert(!have_tx && !orphanage.EraseTx(tx->GetHash())); @@ -132,7 +133,7 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) // test mocktime and expiry SetMockTime(ConsumeTime(fuzzed_data_provider)); auto limit = fuzzed_data_provider.ConsumeIntegral<unsigned int>(); - orphanage.LimitOrphans(limit); + orphanage.LimitOrphans(limit, limit_orphans_rng); Assert(orphanage.Size() <= limit); }); } diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index bf465c0c64..4231fcc909 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -129,11 +129,12 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) } // Test LimitOrphanTxSize() function: - orphanage.LimitOrphans(40); + FastRandomContext rng{/*fDeterministic=*/true}; + orphanage.LimitOrphans(40, rng); BOOST_CHECK(orphanage.CountOrphans() <= 40); - orphanage.LimitOrphans(10); + orphanage.LimitOrphans(10, rng); BOOST_CHECK(orphanage.CountOrphans() <= 10); - orphanage.LimitOrphans(0); + orphanage.LimitOrphans(0, rng); BOOST_CHECK(orphanage.CountOrphans() == 0); } diff --git a/src/test/pow_tests.cpp b/src/test/pow_tests.cpp index 5bd14f22c6..3a44d1da49 100644 --- a/src/test/pow_tests.cpp +++ b/src/test/pow_tests.cpp @@ -177,7 +177,7 @@ void sanity_check_chainparams(const ArgsManager& args, ChainType chain_type) // check max target * 4*nPowTargetTimespan doesn't overflow -- see pow.cpp:CalculateNextWorkRequired() if (!consensus.fPowNoRetargeting) { - arith_uint256 targ_max("0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"); + arith_uint256 targ_max{UintToArith256(uint256S("0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"))}; targ_max /= consensus.nPowTargetTimespan*4; BOOST_CHECK(UintToArith256(consensus.powLimit) < targ_max); } diff --git a/src/test/system_tests.cpp b/src/test/system_tests.cpp index 740f461548..6a96b60db0 100644 --- a/src/test/system_tests.cpp +++ b/src/test/system_tests.cpp @@ -90,7 +90,13 @@ BOOST_AUTO_TEST_CASE(run_command) }); } { - BOOST_REQUIRE_THROW(RunCommandParseJSON("echo \"{\""), std::runtime_error); // Unable to parse JSON + // Unable to parse JSON +#ifdef WIN32 + const std::string command{"cmd.exe /c echo {"}; +#else + const std::string command{"echo {"}; +#endif + BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, HasReason("Unable to parse JSON: {")); } // Test std::in, except for Windows #ifndef WIN32 diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp index 3373f00741..5746961550 100644 --- a/src/test/uint256_tests.cpp +++ b/src/test/uint256_tests.cpp @@ -259,8 +259,8 @@ BOOST_AUTO_TEST_CASE( conversion ) BOOST_CHECK(UintToArith256(OneL) == 1); BOOST_CHECK(ArithToUint256(0) == ZeroL); BOOST_CHECK(ArithToUint256(1) == OneL); - BOOST_CHECK(arith_uint256(R1L.GetHex()) == UintToArith256(R1L)); - BOOST_CHECK(arith_uint256(R2L.GetHex()) == UintToArith256(R2L)); + BOOST_CHECK(arith_uint256(UintToArith256(uint256S(R1L.GetHex()))) == UintToArith256(R1L)); + BOOST_CHECK(arith_uint256(UintToArith256(uint256S(R2L.GetHex()))) == UintToArith256(R2L)); BOOST_CHECK(R1L.GetHex() == UintToArith256(R1L).GetHex()); BOOST_CHECK(R2L.GetHex() == UintToArith256(R2L).GetHex()); } @@ -278,6 +278,34 @@ BOOST_AUTO_TEST_CASE( operator_with_self ) BOOST_CHECK(v == UintToArith256(uint256S("0"))); } +BOOST_AUTO_TEST_CASE(parse) +{ + { + std::string s_12{"0000000000000000000000000000000000000000000000000000000000000012"}; + BOOST_CHECK_EQUAL(uint256S("12\0").GetHex(), s_12); + BOOST_CHECK_EQUAL(uint256S(std::string{"12\0", 3}).GetHex(), s_12); + BOOST_CHECK_EQUAL(uint256S("0x12").GetHex(), s_12); + BOOST_CHECK_EQUAL(uint256S(" 0x12").GetHex(), s_12); + BOOST_CHECK_EQUAL(uint256S(" 12").GetHex(), s_12); + } + { + std::string s_1{uint256::ONE.GetHex()}; + BOOST_CHECK_EQUAL(uint256S("1\0").GetHex(), s_1); + BOOST_CHECK_EQUAL(uint256S(std::string{"1\0", 2}).GetHex(), s_1); + BOOST_CHECK_EQUAL(uint256S("0x1").GetHex(), s_1); + BOOST_CHECK_EQUAL(uint256S(" 0x1").GetHex(), s_1); + BOOST_CHECK_EQUAL(uint256S(" 1").GetHex(), s_1); + } + { + std::string s_0{uint256::ZERO.GetHex()}; + BOOST_CHECK_EQUAL(uint256S("\0").GetHex(), s_0); + BOOST_CHECK_EQUAL(uint256S(std::string{"\0", 1}).GetHex(), s_0); + BOOST_CHECK_EQUAL(uint256S("0x").GetHex(), s_0); + BOOST_CHECK_EQUAL(uint256S(" 0x").GetHex(), s_0); + BOOST_CHECK_EQUAL(uint256S(" ").GetHex(), s_0); + } +} + BOOST_AUTO_TEST_CASE( check_ONE ) { uint256 one = uint256S("0000000000000000000000000000000000000000000000000000000000000001"); diff --git a/src/test/util/net.h b/src/test/util/net.h index 59c4ddb4b1..d91d801132 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -70,7 +70,10 @@ struct ConnmanTestMsg : public CConnman { bool relay_txs) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex); - void ProcessMessagesOnce(CNode& node) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) { m_msgproc->ProcessMessages(&node, flagInterruptMsgProc); } + bool ProcessMessagesOnce(CNode& node) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) + { + return m_msgproc->ProcessMessages(&node, flagInterruptMsgProc); + } void NodeReceiveMsgBytes(CNode& node, Span<const uint8_t> msg_bytes, bool& complete) const; diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 16f54644c2..e907fffd4f 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -113,7 +113,7 @@ void TxOrphanage::EraseForPeer(NodeId peer) if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx from peer=%d\n", nErased, peer); } -void TxOrphanage::LimitOrphans(unsigned int max_orphans) +void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) { LOCK(m_mutex); @@ -138,7 +138,6 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans) nNextSweep = nMinExpTime + ORPHAN_TX_EXPIRE_INTERVAL; if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx due to expiration\n", nErased); } - FastRandomContext rng; while (m_orphans.size() > max_orphans) { // Evict a random orphan: diff --git a/src/txorphanage.h b/src/txorphanage.h index 2196ed4c85..2fd14e6fd2 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -43,7 +43,7 @@ public: void EraseForBlock(const CBlock& block) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); /** Limit the orphanage to the given maximum */ - void LimitOrphans(unsigned int max_orphans) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + void LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); /** Add any orphans that list a particular tx as a parent into the from peer's work set */ void AddChildrenToWorkSet(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);; diff --git a/src/txrequest.cpp b/src/txrequest.cpp index b9a41f26ff..ce5fbd9a7f 100644 --- a/src/txrequest.cpp +++ b/src/txrequest.cpp @@ -73,7 +73,7 @@ struct Announcement { const bool m_is_wtxid : 1; /** What state this announcement is in. */ - State m_state : 3; + State m_state : 3 {State::CANDIDATE_DELAYED}; State GetState() const { return m_state; } void SetState(State state) { m_state = state; } @@ -97,9 +97,9 @@ struct Announcement { /** Construct a new announcement from scratch, initially in CANDIDATE_DELAYED state. */ Announcement(const GenTxid& gtxid, NodeId peer, bool preferred, std::chrono::microseconds reqtime, - SequenceNumber sequence) : - m_txhash(gtxid.GetHash()), m_time(reqtime), m_peer(peer), m_sequence(sequence), m_preferred(preferred), - m_is_wtxid{gtxid.IsWtxid()}, m_state{State::CANDIDATE_DELAYED} {} + SequenceNumber sequence) + : m_txhash(gtxid.GetHash()), m_time(reqtime), m_peer(peer), m_sequence(sequence), m_preferred(preferred), + m_is_wtxid{gtxid.IsWtxid()} {} }; //! Type alias for priorities. diff --git a/src/util/trace.h b/src/util/trace.h index 1fe743f043..b7c275f19b 100644 --- a/src/util/trace.h +++ b/src/util/trace.h @@ -13,28 +13,19 @@ #include <sys/sdt.h> -// Disabling this warning can be removed once we switch to C++20 -#if defined(__clang__) && __cplusplus < 202002L -#define BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH _Pragma("clang diagnostic push") _Pragma("clang diagnostic ignored \"-Wgnu-zero-variadic-macro-arguments\"") -#define BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP _Pragma("clang diagnostic pop") -#else -#define BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH -#define BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#endif - -#define TRACE(context, event) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE(context, event) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE1(context, event, a) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE1(context, event, a) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE2(context, event, a, b) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE2(context, event, a, b) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE3(context, event, a, b, c) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE3(context, event, a, b, c) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE4(context, event, a, b, c, d) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE4(context, event, a, b, c, d) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE5(context, event, a, b, c, d, e) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE5(context, event, a, b, c, d, e) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE6(context, event, a, b, c, d, e, f) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE6(context, event, a, b, c, d, e, f) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE7(context, event, a, b, c, d, e, f, g) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE7(context, event, a, b, c, d, e, f, g) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE8(context, event, a, b, c, d, e, f, g, h) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE8(context, event, a, b, c, d, e, f, g, h) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE9(context, event, a, b, c, d, e, f, g, h, i) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE9(context, event, a, b, c, d, e, f, g, h, i) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE10(context, event, a, b, c, d, e, f, g, h, i, j) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE10(context, event, a, b, c, d, e, f, g, h, i, j) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE11(context, event, a, b, c, d, e, f, g, h, i, j, k) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE11(context, event, a, b, c, d, e, f, g, h, i, j, k) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP -#define TRACE12(context, event, a, b, c, d, e, f, g, h, i, j, k, l) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH DTRACE_PROBE12(context, event, a, b, c, d, e, f, g, h, i, j, k, l) BITCOIN_DISABLE_WARN_ZERO_VARIADIC_POP +#define TRACE(context, event) DTRACE_PROBE(context, event) +#define TRACE1(context, event, a) DTRACE_PROBE1(context, event, a) +#define TRACE2(context, event, a, b) DTRACE_PROBE2(context, event, a, b) +#define TRACE3(context, event, a, b, c) DTRACE_PROBE3(context, event, a, b, c) +#define TRACE4(context, event, a, b, c, d) DTRACE_PROBE4(context, event, a, b, c, d) +#define TRACE5(context, event, a, b, c, d, e) DTRACE_PROBE5(context, event, a, b, c, d, e) +#define TRACE6(context, event, a, b, c, d, e, f) DTRACE_PROBE6(context, event, a, b, c, d, e, f) +#define TRACE7(context, event, a, b, c, d, e, f, g) DTRACE_PROBE7(context, event, a, b, c, d, e, f, g) +#define TRACE8(context, event, a, b, c, d, e, f, g, h) DTRACE_PROBE8(context, event, a, b, c, d, e, f, g, h) +#define TRACE9(context, event, a, b, c, d, e, f, g, h, i) DTRACE_PROBE9(context, event, a, b, c, d, e, f, g, h, i) +#define TRACE10(context, event, a, b, c, d, e, f, g, h, i, j) DTRACE_PROBE10(context, event, a, b, c, d, e, f, g, h, i, j) +#define TRACE11(context, event, a, b, c, d, e, f, g, h, i, j, k) DTRACE_PROBE11(context, event, a, b, c, d, e, f, g, h, i, j, k) +#define TRACE12(context, event, a, b, c, d, e, f, g, h, i, j, k, l) DTRACE_PROBE12(context, event, a, b, c, d, e, f, g, h, i, j, k, l) #else diff --git a/src/wallet/coincontrol.cpp b/src/wallet/coincontrol.cpp index 2087119db9..873c5ab383 100644 --- a/src/wallet/coincontrol.cpp +++ b/src/wallet/coincontrol.cpp @@ -14,69 +14,142 @@ CCoinControl::CCoinControl() bool CCoinControl::HasSelected() const { - return !m_selected_inputs.empty(); + return !m_selected.empty(); } -bool CCoinControl::IsSelected(const COutPoint& output) const +bool CCoinControl::IsSelected(const COutPoint& outpoint) const { - return m_selected_inputs.count(output) > 0; + return m_selected.count(outpoint) > 0; } -bool CCoinControl::IsExternalSelected(const COutPoint& output) const +bool CCoinControl::IsExternalSelected(const COutPoint& outpoint) const { - return m_external_txouts.count(output) > 0; + const auto it = m_selected.find(outpoint); + return it != m_selected.end() && it->second.HasTxOut(); } std::optional<CTxOut> CCoinControl::GetExternalOutput(const COutPoint& outpoint) const { - const auto ext_it = m_external_txouts.find(outpoint); - if (ext_it == m_external_txouts.end()) { + const auto it = m_selected.find(outpoint); + if (it == m_selected.end() || !it->second.HasTxOut()) { return std::nullopt; } + return it->second.GetTxOut(); +} - return std::make_optional(ext_it->second); +PreselectedInput& CCoinControl::Select(const COutPoint& outpoint) +{ + auto& input = m_selected[outpoint]; + input.SetPosition(m_selection_pos); + ++m_selection_pos; + return input; +} +void CCoinControl::UnSelect(const COutPoint& outpoint) +{ + m_selected.erase(outpoint); } -void CCoinControl::Select(const COutPoint& output) +void CCoinControl::UnSelectAll() { - m_selected_inputs.insert(output); + m_selected.clear(); } -void CCoinControl::SelectExternal(const COutPoint& outpoint, const CTxOut& txout) +std::vector<COutPoint> CCoinControl::ListSelected() const { - m_selected_inputs.insert(outpoint); - m_external_txouts.emplace(outpoint, txout); + std::vector<COutPoint> outpoints; + std::transform(m_selected.begin(), m_selected.end(), std::back_inserter(outpoints), + [](const std::map<COutPoint, PreselectedInput>::value_type& pair) { + return pair.first; + }); + return outpoints; } -void CCoinControl::UnSelect(const COutPoint& output) +void CCoinControl::SetInputWeight(const COutPoint& outpoint, int64_t weight) { - m_selected_inputs.erase(output); + m_selected[outpoint].SetInputWeight(weight); } -void CCoinControl::UnSelectAll() +std::optional<int64_t> CCoinControl::GetInputWeight(const COutPoint& outpoint) const { - m_selected_inputs.clear(); + const auto it = m_selected.find(outpoint); + return it != m_selected.end() ? it->second.GetInputWeight() : std::nullopt; } -std::vector<COutPoint> CCoinControl::ListSelected() const +std::optional<uint32_t> CCoinControl::GetSequence(const COutPoint& outpoint) const { - return {m_selected_inputs.begin(), m_selected_inputs.end()}; + const auto it = m_selected.find(outpoint); + return it != m_selected.end() ? it->second.GetSequence() : std::nullopt; } -void CCoinControl::SetInputWeight(const COutPoint& outpoint, int64_t weight) +std::pair<std::optional<CScript>, std::optional<CScriptWitness>> CCoinControl::GetScripts(const COutPoint& outpoint) const +{ + const auto it = m_selected.find(outpoint); + return it != m_selected.end() ? m_selected.at(outpoint).GetScripts() : std::make_pair(std::nullopt, std::nullopt); +} + +void PreselectedInput::SetTxOut(const CTxOut& txout) +{ + m_txout = txout; +} + +CTxOut PreselectedInput::GetTxOut() const +{ + assert(m_txout.has_value()); + return m_txout.value(); +} + +bool PreselectedInput::HasTxOut() const +{ + return m_txout.has_value(); +} + +void PreselectedInput::SetInputWeight(int64_t weight) +{ + m_weight = weight; +} + +std::optional<int64_t> PreselectedInput::GetInputWeight() const +{ + return m_weight; +} + +void PreselectedInput::SetSequence(uint32_t sequence) +{ + m_sequence = sequence; +} + +std::optional<uint32_t> PreselectedInput::GetSequence() const +{ + return m_sequence; +} + +void PreselectedInput::SetScriptSig(const CScript& script) +{ + m_script_sig = script; +} + +void PreselectedInput::SetScriptWitness(const CScriptWitness& script_wit) +{ + m_script_witness = script_wit; +} + +bool PreselectedInput::HasScripts() const +{ + return m_script_sig.has_value() || m_script_witness.has_value(); +} + +std::pair<std::optional<CScript>, std::optional<CScriptWitness>> PreselectedInput::GetScripts() const { - m_input_weights[outpoint] = weight; + return {m_script_sig, m_script_witness}; } -bool CCoinControl::HasInputWeight(const COutPoint& outpoint) const +void PreselectedInput::SetPosition(unsigned int pos) { - return m_input_weights.count(outpoint) > 0; + m_pos = pos; } -int64_t CCoinControl::GetInputWeight(const COutPoint& outpoint) const +std::optional<unsigned int> PreselectedInput::GetPosition() const { - auto it = m_input_weights.find(outpoint); - assert(it != m_input_weights.end()); - return it->second; + return m_pos; } } // namespace wallet diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h index 71593e236f..b2f813383d 100644 --- a/src/wallet/coincontrol.h +++ b/src/wallet/coincontrol.h @@ -24,6 +24,58 @@ const int DEFAULT_MAX_DEPTH = 9999999; //! Default for -avoidpartialspends static constexpr bool DEFAULT_AVOIDPARTIALSPENDS = false; +class PreselectedInput +{ +private: + //! The previous output being spent by this input + std::optional<CTxOut> m_txout; + //! The input weight for spending this input + std::optional<int64_t> m_weight; + //! The sequence number for this input + std::optional<uint32_t> m_sequence; + //! The scriptSig for this input + std::optional<CScript> m_script_sig; + //! The scriptWitness for this input + std::optional<CScriptWitness> m_script_witness; + //! The position in the inputs vector for this input + std::optional<unsigned int> m_pos; + +public: + /** + * Set the previous output for this input. + * Only necessary if the input is expected to be an external input. + */ + void SetTxOut(const CTxOut& txout); + /** Retrieve the previous output for this input. */ + CTxOut GetTxOut() const; + /** Return whether the previous output is set for this input. */ + bool HasTxOut() const; + + /** Set the weight for this input. */ + void SetInputWeight(int64_t weight); + /** Retrieve the input weight for this input. */ + std::optional<int64_t> GetInputWeight() const; + + /** Set the sequence for this input. */ + void SetSequence(uint32_t sequence); + /** Retrieve the sequence for this input. */ + std::optional<uint32_t> GetSequence() const; + + /** Set the scriptSig for this input. */ + void SetScriptSig(const CScript& script); + /** Set the scriptWitness for this input. */ + void SetScriptWitness(const CScriptWitness& script_wit); + /** Return whether either the scriptSig or scriptWitness are set for this input. */ + bool HasScripts() const; + /** Retrieve both the scriptSig and the scriptWitness. */ + std::pair<std::optional<CScript>, std::optional<CScriptWitness>> GetScripts() const; + + /** Store the position of this input. */ + void SetPosition(unsigned int pos); + /** Retrieve the position of this input. */ + std::optional<unsigned int> GetPosition() const; +}; + /** Coin Control Features. */ class CCoinControl { @@ -59,6 +111,10 @@ public: int m_max_depth = DEFAULT_MAX_DEPTH; //! SigningProvider that has pubkeys and scripts to do spend size estimation for external inputs FlatSigningProvider m_external_provider; + //! Locktime + std::optional<uint32_t> m_locktime; + //! Version + std::optional<uint32_t> m_version; CCoinControl(); @@ -69,11 +125,11 @@ public: /** * Returns true if the given output is pre-selected. */ - bool IsSelected(const COutPoint& output) const; + bool IsSelected(const COutPoint& outpoint) const; /** * Returns true if the given output is selected as an external input. */ - bool IsExternalSelected(const COutPoint& output) const; + bool IsExternalSelected(const COutPoint& outpoint) const; /** * Returns the external output for the given outpoint if it exists. */ @@ -82,16 +138,11 @@ public: * Lock-in the given output for spending. * The output will be included in the transaction even if it's not the most optimal choice. */ - void Select(const COutPoint& output); - /** - * Lock-in the given output as an external input for spending because it is not in the wallet. - * The output will be included in the transaction even if it's not the most optimal choice. - */ - void SelectExternal(const COutPoint& outpoint, const CTxOut& txout); + PreselectedInput& Select(const COutPoint& outpoint); /** * Unselects the given output. */ - void UnSelect(const COutPoint& output); + void UnSelect(const COutPoint& outpoint); /** * Unselects all outputs. */ @@ -105,22 +156,32 @@ public: */ void SetInputWeight(const COutPoint& outpoint, int64_t weight); /** - * Returns true if the input weight is set. - */ - bool HasInputWeight(const COutPoint& outpoint) const; - /** * Returns the input weight. */ - int64_t GetInputWeight(const COutPoint& outpoint) const; + std::optional<int64_t> GetInputWeight(const COutPoint& outpoint) const; + /** Retrieve the sequence for an input */ + std::optional<uint32_t> GetSequence(const COutPoint& outpoint) const; + /** Retrieves the scriptSig and scriptWitness for an input. */ + std::pair<std::optional<CScript>, std::optional<CScriptWitness>> GetScripts(const COutPoint& outpoint) const; + + bool HasSelectedOrder() const + { + return m_selection_pos > 0; + } + + std::optional<unsigned int> GetSelectionPos(const COutPoint& outpoint) const + { + const auto it = m_selected.find(outpoint); + if (it == m_selected.end()) { + return std::nullopt; + } + return it->second.GetPosition(); + } private: //! Selected inputs (inputs that will be used, regardless of whether they're optimal or not) - std::set<COutPoint> m_selected_inputs; - //! Map of external inputs to include in the transaction - //! These are not in the wallet, so we need to track them separately - std::map<COutPoint, CTxOut> m_external_txouts; - //! Map of COutPoints to the maximum weight for that input - std::map<COutPoint, int64_t> m_input_weights; + std::map<COutPoint, PreselectedInput> m_selected; + unsigned int m_selection_pos{0}; }; } // namespace wallet diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index 6d026d02c1..a71f8f9fbc 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -16,7 +16,7 @@ #include <vector> namespace wallet { -bool ExternalSignerScriptPubKeyMan::SetupDescriptor(std::unique_ptr<Descriptor> desc) +bool ExternalSignerScriptPubKeyMan::SetupDescriptor(WalletBatch& batch, std::unique_ptr<Descriptor> desc) { LOCK(cs_desc_man); assert(m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); @@ -29,13 +29,12 @@ bool ExternalSignerScriptPubKeyMan::SetupDescriptor(std::unique_ptr<Descriptor> m_wallet_descriptor = w_desc; // Store the descriptor - WalletBatch batch(m_storage.GetDatabase()); if (!batch.WriteDescriptor(GetID(), m_wallet_descriptor)) { throw std::runtime_error(std::string(__func__) + ": writing descriptor failed"); } // TopUp - TopUp(); + TopUpWithDB(batch); m_storage.UnsetBlankWalletFlag(batch); return true; diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index 01dc80b1ca..c052ce6129 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -23,7 +23,7 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan /** Provide a descriptor at setup time * Returns false if already setup or setup fails, true if setup is successful */ - bool SetupDescriptor(std::unique_ptr<Descriptor>desc); + bool SetupDescriptor(WalletBatch& batch, std::unique_ptr<Descriptor>desc); static ExternalSigner GetExternalSigner(); diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index d9a08310a8..c6ed0fe11c 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -203,10 +203,9 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo errors.push_back(Untranslated(strprintf("%s:%u is already spent", txin.prevout.hash.GetHex(), txin.prevout.n))); return Result::MISC_ERROR; } - if (wallet.IsMine(txin.prevout)) { - new_coin_control.Select(txin.prevout); - } else { - new_coin_control.SelectExternal(txin.prevout, coin.out); + PreselectedInput& preset_txin = new_coin_control.Select(txin.prevout); + if (!wallet.IsMine(txin.prevout)) { + preset_txin.SetTxOut(coin.out); } input_value += coin.out.nValue; spent_outputs.push_back(coin.out); @@ -317,8 +316,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo // We cannot source new unconfirmed inputs(bip125 rule 2) new_coin_control.m_min_depth = 1; - constexpr int RANDOM_CHANGE_POSITION = -1; - auto res = CreateTransaction(wallet, recipients, RANDOM_CHANGE_POSITION, new_coin_control, false); + auto res = CreateTransaction(wallet, recipients, std::nullopt, new_coin_control, false); if (!res) { errors.push_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + util::ErrorString(res)); return Result::WALLET_ERROR; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 4ca5e29229..d15273dfc9 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -281,12 +281,12 @@ public: CAmount& fee) override { LOCK(m_wallet->cs_wallet); - auto res = CreateTransaction(*m_wallet, recipients, change_pos, + auto res = CreateTransaction(*m_wallet, recipients, change_pos == -1 ? std::nullopt : std::make_optional(change_pos), coin_control, sign); if (!res) return util::Error{util::ErrorString(res)}; const auto& txr = *res; fee = txr.fee; - change_pos = txr.change_pos; + change_pos = txr.change_pos ? *txr.change_pos : -1; return txr.tx; } diff --git a/src/wallet/rpc/encrypt.cpp b/src/wallet/rpc/encrypt.cpp index 0226d15698..e237286603 100644 --- a/src/wallet/rpc/encrypt.cpp +++ b/src/wallet/rpc/encrypt.cpp @@ -220,7 +220,11 @@ RPCHelpMan encryptwallet() "After this, any calls that interact with private keys such as sending or signing \n" "will require the passphrase to be set prior the making these calls.\n" "Use the walletpassphrase call for this, and then walletlock call.\n" - "If the wallet is already encrypted, use the walletpassphrasechange call.\n", + "If the wallet is already encrypted, use the walletpassphrasechange call.\n" + "** IMPORTANT **\n" + "For security reasons, the encryption process will generate a new HD seed, resulting\n" + "in the creation of a fresh set of active descriptors. Therefore, it is crucial to\n" + "securely back up the newly generated wallet file using the backupwallet RPC.\n", { {"passphrase", RPCArg::Type::STR, RPCArg::Optional::NO, "The pass phrase to encrypt the wallet with. It must be at least 1 character, but should be long."}, }, @@ -268,7 +272,7 @@ RPCHelpMan encryptwallet() throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Error: Failed to encrypt the wallet."); } - return "wallet encrypted; The keypool has been flushed and a new HD seed was generated (if you are using HD). You need to make a new backup."; + return "wallet encrypted; The keypool has been flushed and a new HD seed was generated. You need to make a new backup with the backupwallet RPC."; }, }; } diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index e7884af8b0..b121c8e1a7 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -155,8 +155,7 @@ UniValue SendMoney(CWallet& wallet, const CCoinControl &coin_control, std::vecto std::shuffle(recipients.begin(), recipients.end(), FastRandomContext()); // Send - constexpr int RANDOM_CHANGE_POSITION = -1; - auto res = CreateTransaction(wallet, recipients, RANDOM_CHANGE_POSITION, coin_control, true); + auto res = CreateTransaction(wallet, recipients, std::nullopt, coin_control, true); if (!res) { throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, util::ErrorString(res).original); } @@ -489,13 +488,13 @@ static std::vector<RPCArg> FundTxDoc(bool solving_data = true) return args; } -void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, const UniValue& options, CCoinControl& coinControl, bool override_min_fee) +CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const UniValue& options, CCoinControl& coinControl, bool override_min_fee) { // Make sure the results are valid at least up to the most recent block // the user could have gotten from another RPC command prior to now wallet.BlockUntilSyncedToCurrentChain(); - change_position = -1; + std::optional<unsigned int> change_position; bool lockUnspents = false; UniValue subtractFeeFromOutputs; std::set<int> setSubtractFeeFromOutputs; @@ -553,7 +552,11 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, } if (options.exists("changePosition") || options.exists("change_position")) { - change_position = (options.exists("change_position") ? options["change_position"] : options["changePosition"]).getInt<int>(); + int pos = (options.exists("change_position") ? options["change_position"] : options["changePosition"]).getInt<int>(); + if (pos < 0 || (unsigned int)pos > tx.vout.size()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "changePosition out of bounds"); + } + change_position = (unsigned int)pos; } if (options.exists("change_type")) { @@ -703,9 +706,6 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, if (tx.vout.size() == 0) throw JSONRPCError(RPC_INVALID_PARAMETER, "TX must have at least one output"); - if (change_position != -1 && (change_position < 0 || (unsigned int)change_position > tx.vout.size())) - throw JSONRPCError(RPC_INVALID_PARAMETER, "changePosition out of bounds"); - for (unsigned int idx = 0; idx < subtractFeeFromOutputs.size(); idx++) { int pos = subtractFeeFromOutputs[idx].getInt<int>(); if (setSubtractFeeFromOutputs.count(pos)) @@ -717,11 +717,11 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, setSubtractFeeFromOutputs.insert(pos); } - bilingual_str error; - - if (!FundTransaction(wallet, tx, fee_out, change_position, error, lockUnspents, setSubtractFeeFromOutputs, coinControl)) { - throw JSONRPCError(RPC_WALLET_ERROR, error.original); + auto txr = FundTransaction(wallet, tx, change_position, lockUnspents, setSubtractFeeFromOutputs, coinControl); + if (!txr) { + throw JSONRPCError(RPC_WALLET_ERROR, ErrorString(txr).original); } + return *txr; } static void SetOptionsInputWeights(const UniValue& inputs, UniValue& options) @@ -844,17 +844,15 @@ RPCHelpMan fundrawtransaction() throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); } - CAmount fee; - int change_position; CCoinControl coin_control; // Automatically select (additional) coins. Can be overridden by options.add_inputs. coin_control.m_allow_other_inputs = true; - FundTransaction(*pwallet, tx, fee, change_position, request.params[1], coin_control, /*override_min_fee=*/true); + auto txr = FundTransaction(*pwallet, tx, request.params[1], coin_control, /*override_min_fee=*/true); UniValue result(UniValue::VOBJ); - result.pushKV("hex", EncodeHexTx(CTransaction(tx))); - result.pushKV("fee", ValueFromAmount(fee)); - result.pushKV("changepos", change_position); + result.pushKV("hex", EncodeHexTx(*txr.tx)); + result.pushKV("fee", ValueFromAmount(txr.fee)); + result.pushKV("changepos", txr.change_pos ? (int)*txr.change_pos : -1); return result; }, @@ -1276,8 +1274,6 @@ RPCHelpMan send() PreventOutdatedOptions(options); - CAmount fee; - int change_position; bool rbf{options.exists("replaceable") ? options["replaceable"].get_bool() : pwallet->m_signal_rbf}; CMutableTransaction rawTx = ConstructTransaction(options["inputs"], request.params[0], options["locktime"], rbf); CCoinControl coin_control; @@ -1285,9 +1281,9 @@ RPCHelpMan send() // be overridden by options.add_inputs. coin_control.m_allow_other_inputs = rawTx.vin.size() == 0; SetOptionsInputWeights(options["inputs"], options); - FundTransaction(*pwallet, rawTx, fee, change_position, options, coin_control, /*override_min_fee=*/false); + auto txr = FundTransaction(*pwallet, rawTx, options, coin_control, /*override_min_fee=*/false); - return FinishTransaction(pwallet, options, rawTx); + return FinishTransaction(pwallet, options, CMutableTransaction(*txr.tx)); } }; } @@ -1712,8 +1708,6 @@ RPCHelpMan walletcreatefundedpsbt() UniValue options{request.params[3].isNull() ? UniValue::VOBJ : request.params[3]}; - CAmount fee; - int change_position; const UniValue &replaceable_arg = options["replaceable"]; const bool rbf{replaceable_arg.isNull() ? wallet.m_signal_rbf : replaceable_arg.get_bool()}; CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], rbf); @@ -1722,10 +1716,10 @@ RPCHelpMan walletcreatefundedpsbt() // be overridden by options.add_inputs. coin_control.m_allow_other_inputs = rawTx.vin.size() == 0; SetOptionsInputWeights(request.params[0], options); - FundTransaction(wallet, rawTx, fee, change_position, options, coin_control, /*override_min_fee=*/true); + auto txr = FundTransaction(wallet, rawTx, options, coin_control, /*override_min_fee=*/true); // Make a blank psbt - PartiallySignedTransaction psbtx(rawTx); + PartiallySignedTransaction psbtx(CMutableTransaction(*txr.tx)); // Fill transaction with out data but don't sign bool bip32derivs = request.params[4].isNull() ? true : request.params[4].get_bool(); @@ -1741,8 +1735,8 @@ RPCHelpMan walletcreatefundedpsbt() UniValue result(UniValue::VOBJ); result.pushKV("psbt", EncodeBase64(ssTx.str())); - result.pushKV("fee", ValueFromAmount(fee)); - result.pushKV("changepos", change_position); + result.pushKV("fee", ValueFromAmount(txr.fee)); + result.pushKV("changepos", txr.change_pos ? (int)*txr.change_pos : -1); return result; }, }; diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index d2b2801aa8..0b4800b848 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -333,7 +333,8 @@ bool LegacyScriptPubKeyMan::TopUpInactiveHDChain(const CKeyID seed_id, int64_t i chain.m_next_external_index = std::max(chain.m_next_external_index, index + 1); } - TopUpChain(chain, 0); + WalletBatch batch(m_storage.GetDatabase()); + TopUpChain(batch, chain, 0); return true; } @@ -1274,19 +1275,22 @@ bool LegacyScriptPubKeyMan::TopUp(unsigned int kpSize) return false; } - if (!TopUpChain(m_hd_chain, kpSize)) { + WalletBatch batch(m_storage.GetDatabase()); + if (!batch.TxnBegin()) return false; + if (!TopUpChain(batch, m_hd_chain, kpSize)) { return false; } for (auto& [chain_id, chain] : m_inactive_hd_chains) { - if (!TopUpChain(chain, kpSize)) { + if (!TopUpChain(batch, chain, kpSize)) { return false; } } + if (!batch.TxnCommit()) throw std::runtime_error(strprintf("Error during keypool top up. Cannot commit changes for wallet %s", m_storage.GetDisplayName())); NotifyCanGetAddressesChanged(); return true; } -bool LegacyScriptPubKeyMan::TopUpChain(CHDChain& chain, unsigned int kpSize) +bool LegacyScriptPubKeyMan::TopUpChain(WalletBatch& batch, CHDChain& chain, unsigned int kpSize) { LOCK(cs_KeyStore); @@ -1318,7 +1322,6 @@ bool LegacyScriptPubKeyMan::TopUpChain(CHDChain& chain, unsigned int kpSize) missingInternal = 0; } bool internal = false; - WalletBatch batch(m_storage.GetDatabase()); for (int64_t i = missingInternal + missingExternal; i--;) { if (i < missingInternal) { internal = true; @@ -2136,6 +2139,15 @@ std::map<CKeyID, CKey> DescriptorScriptPubKeyMan::GetKeys() const bool DescriptorScriptPubKeyMan::TopUp(unsigned int size) { + WalletBatch batch(m_storage.GetDatabase()); + if (!batch.TxnBegin()) return false; + bool res = TopUpWithDB(batch, size); + if (!batch.TxnCommit()) throw std::runtime_error(strprintf("Error during descriptors keypool top up. Cannot commit changes for wallet %s", m_storage.GetDisplayName())); + return res; +} + +bool DescriptorScriptPubKeyMan::TopUpWithDB(WalletBatch& batch, unsigned int size) +{ LOCK(cs_desc_man); unsigned int target_size; if (size > 0) { @@ -2157,7 +2169,6 @@ bool DescriptorScriptPubKeyMan::TopUp(unsigned int size) FlatSigningProvider provider; provider.keys = GetKeys(); - WalletBatch batch(m_storage.GetDatabase()); uint256 id = GetID(); for (int32_t i = m_max_cached_index + 1; i < new_range_end; ++i) { FlatSigningProvider out_keys; @@ -2264,7 +2275,7 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const } } -bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_key, OutputType addr_type, bool internal) +bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal) { LOCK(cs_desc_man); assert(m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); @@ -2325,7 +2336,6 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_ m_wallet_descriptor = w_desc; // Store the master private key, and descriptor - WalletBatch batch(m_storage.GetDatabase()); if (!AddDescriptorKeyWithDB(batch, master_key.key, master_key.key.GetPubKey())) { throw std::runtime_error(std::string(__func__) + ": writing descriptor master private key failed"); } @@ -2334,7 +2344,7 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_ } // TopUp - TopUp(); + TopUpWithDB(batch); m_storage.UnsetBlankWalletFlag(batch); return true; diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 7c0eca1475..dccbf3ced6 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -366,7 +366,7 @@ private: */ bool TopUpInactiveHDChain(const CKeyID seed_id, int64_t index, bool internal); - bool TopUpChain(CHDChain& chain, unsigned int size); + bool TopUpChain(WalletBatch& batch, CHDChain& chain, unsigned int size); public: LegacyScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size) : ScriptPubKeyMan(storage), m_keypool_size(keypool_size) {} @@ -583,7 +583,10 @@ private: std::unique_ptr<FlatSigningProvider> GetSigningProvider(int32_t index, bool include_private = false) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); protected: - WalletDescriptor m_wallet_descriptor GUARDED_BY(cs_desc_man); + WalletDescriptor m_wallet_descriptor GUARDED_BY(cs_desc_man); + + //! Same as 'TopUp' but designed for use within a batch transaction context + bool TopUpWithDB(WalletBatch& batch, unsigned int size = 0); public: DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size) @@ -618,12 +621,7 @@ public: bool IsHDEnabled() const override; //! Setup descriptors based on the given CExtkey - bool SetupDescriptorGeneration(const CExtKey& master_key, OutputType addr_type, bool internal); - - /** Provide a descriptor at setup time - * Returns false if already setup or setup fails, true if setup is successful - */ - bool SetupDescriptor(std::unique_ptr<Descriptor>desc); + bool SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal); bool HavePrivateKeys() const override; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index ece5a5c5b7..e97e658f38 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -117,8 +117,9 @@ static std::optional<int64_t> GetSignedTxinWeight(const CWallet* wallet, const C const bool can_grind_r) { // If weight was provided, use that. - if (coin_control && coin_control->HasInputWeight(txin.prevout)) { - return coin_control->GetInputWeight(txin.prevout); + std::optional<int64_t> weight; + if (coin_control && (weight = coin_control->GetInputWeight(txin.prevout))) { + return weight.value(); } // Otherwise, use the maximum satisfaction size provided by the descriptor. @@ -261,7 +262,10 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const const bool can_grind_r = wallet.CanGrindR(); std::map<COutPoint, CAmount> map_of_bump_fees = wallet.chain().calculateIndividualBumpFees(coin_control.ListSelected(), coin_selection_params.m_effective_feerate); for (const COutPoint& outpoint : coin_control.ListSelected()) { - int input_bytes = -1; + int64_t input_bytes = coin_control.GetInputWeight(outpoint).value_or(-1); + if (input_bytes != -1) { + input_bytes = GetVirtualTransactionSize(input_bytes, 0, 0); + } CTxOut txout; if (auto ptr_wtx = wallet.GetWalletTx(outpoint.hash)) { // Clearly invalid input, fail @@ -269,7 +273,9 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const return util::Error{strprintf(_("Invalid pre-selected input %s"), outpoint.ToString())}; } txout = ptr_wtx->tx->vout.at(outpoint.n); - input_bytes = CalculateMaximumSignedInputSize(txout, &wallet, &coin_control); + if (input_bytes == -1) { + input_bytes = CalculateMaximumSignedInputSize(txout, &wallet, &coin_control); + } } else { // The input is external. We did not find the tx in mapWallet. const auto out{coin_control.GetExternalOutput(outpoint)}; @@ -284,11 +290,6 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const input_bytes = CalculateMaximumSignedInputSize(txout, outpoint, &coin_control.m_external_provider, can_grind_r, &coin_control); } - // If available, override calculated size with coin control specified size - if (coin_control.HasInputWeight(outpoint)) { - input_bytes = GetVirtualTransactionSize(coin_control.GetInputWeight(outpoint), 0, 0); - } - if (input_bytes == -1) { return util::Error{strprintf(_("Not solvable pre-selected input %s"), outpoint.ToString())}; // Not solvable, can't estimate size for fee } @@ -967,18 +968,19 @@ static void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng static util::Result<CreatedTransactionResult> CreateTransactionInternal( CWallet& wallet, const std::vector<CRecipient>& vecSend, - int change_pos, + std::optional<unsigned int> change_pos, const CCoinControl& coin_control, bool sign) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { AssertLockHeld(wallet.cs_wallet); - // out variables, to be packed into returned result structure - int nChangePosInOut = change_pos; - FastRandomContext rng_fast; CMutableTransaction txNew; // The resulting transaction that we make + if (coin_control.m_version) { + txNew.nVersion = coin_control.m_version.value(); + } + CoinSelectionParams coin_selection_params{rng_fast}; // Parameters for coin selection, init with dummy coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends; coin_selection_params.m_include_unsafe_inputs = coin_control.m_include_unsafe_inputs; @@ -1130,20 +1132,39 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( const CAmount change_amount = result.GetChange(coin_selection_params.min_viable_change, coin_selection_params.m_change_fee); if (change_amount > 0) { CTxOut newTxOut(change_amount, scriptChange); - if (nChangePosInOut == -1) { + if (!change_pos) { // Insert change txn at random position: - nChangePosInOut = rng_fast.randrange(txNew.vout.size() + 1); - } else if ((unsigned int)nChangePosInOut > txNew.vout.size()) { + change_pos = rng_fast.randrange(txNew.vout.size() + 1); + } else if ((unsigned int)*change_pos > txNew.vout.size()) { return util::Error{_("Transaction change output index out of range")}; } - txNew.vout.insert(txNew.vout.begin() + nChangePosInOut, newTxOut); + txNew.vout.insert(txNew.vout.begin() + *change_pos, newTxOut); } else { - nChangePosInOut = -1; + change_pos = std::nullopt; } // Shuffle selected coins and fill in final vin std::vector<std::shared_ptr<COutput>> selected_coins = result.GetShuffledInputVector(); + if (coin_control.HasSelected() && coin_control.HasSelectedOrder()) { + // When there are preselected inputs, we need to move them to be the first UTXOs + // and have them be in the order selected. We can use stable_sort for this, where we + // compare with the positions stored in coin_control. The COutputs that have positions + // will be placed before those that don't, and those positions will be in order. + std::stable_sort(selected_coins.begin(), selected_coins.end(), + [&coin_control](const std::shared_ptr<COutput>& a, const std::shared_ptr<COutput>& b) { + auto a_pos = coin_control.GetSelectionPos(a->outpoint); + auto b_pos = coin_control.GetSelectionPos(b->outpoint); + if (a_pos.has_value() && b_pos.has_value()) { + return a_pos.value() < b_pos.value(); + } else if (a_pos.has_value() && !b_pos.has_value()) { + return true; + } else { + return false; + } + }); + } + // The sequence number is set to non-maxint so that DiscourageFeeSniping // works. // @@ -1152,11 +1173,32 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( // to avoid conflicting with other possible uses of nSequence, // and in the spirit of "smallest possible change from prior // behavior." - const uint32_t nSequence{coin_control.m_signal_bip125_rbf.value_or(wallet.m_signal_rbf) ? MAX_BIP125_RBF_SEQUENCE : CTxIn::MAX_SEQUENCE_NONFINAL}; + bool use_anti_fee_sniping = true; + const uint32_t default_sequence{coin_control.m_signal_bip125_rbf.value_or(wallet.m_signal_rbf) ? MAX_BIP125_RBF_SEQUENCE : CTxIn::MAX_SEQUENCE_NONFINAL}; for (const auto& coin : selected_coins) { - txNew.vin.emplace_back(coin->outpoint, CScript(), nSequence); + std::optional<uint32_t> sequence = coin_control.GetSequence(coin->outpoint); + if (sequence) { + // If an input has a preset sequence, we can't do anti-fee-sniping + use_anti_fee_sniping = false; + } + txNew.vin.emplace_back(coin->outpoint, CScript{}, sequence.value_or(default_sequence)); + + auto scripts = coin_control.GetScripts(coin->outpoint); + if (scripts.first) { + txNew.vin.back().scriptSig = *scripts.first; + } + if (scripts.second) { + txNew.vin.back().scriptWitness = *scripts.second; + } + } + if (coin_control.m_locktime) { + txNew.nLockTime = coin_control.m_locktime.value(); + // If we have a locktime set, we can't use anti-fee-sniping + use_anti_fee_sniping = false; + } + if (use_anti_fee_sniping) { + DiscourageFeeSniping(txNew, rng_fast, wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight()); } - DiscourageFeeSniping(txNew, rng_fast, wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight()); // Calculate the transaction fee TxSize tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, &coin_control); @@ -1175,8 +1217,8 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( } // If there is a change output and we overpay the fees then increase the change to match the fee needed - if (nChangePosInOut != -1 && fee_needed < current_fee) { - auto& change = txNew.vout.at(nChangePosInOut); + if (change_pos && fee_needed < current_fee) { + auto& change = txNew.vout.at(*change_pos); change.nValue += current_fee - fee_needed; current_fee = result.GetSelectedValue() - CalculateOutputValue(txNew); if (fee_needed != current_fee) { @@ -1187,11 +1229,11 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( // Reduce output values for subtractFeeFromAmount if (coin_selection_params.m_subtract_fee_outputs) { CAmount to_reduce = fee_needed - current_fee; - int i = 0; + unsigned int i = 0; bool fFirst = true; for (const auto& recipient : vecSend) { - if (i == nChangePosInOut) { + if (change_pos && i == *change_pos) { ++i; } CTxOut& txout = txNew.vout[i]; @@ -1230,7 +1272,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( } // Give up if change keypool ran out and change is required - if (scriptChange.empty() && nChangePosInOut != -1) { + if (scriptChange.empty() && change_pos) { return util::Error{error}; } @@ -1272,13 +1314,13 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal( feeCalc.est.fail.start, feeCalc.est.fail.end, (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) > 0.0 ? 100 * feeCalc.est.fail.withinTarget / (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool) : 0.0, feeCalc.est.fail.withinTarget, feeCalc.est.fail.totalConfirmed, feeCalc.est.fail.inMempool, feeCalc.est.fail.leftMempool); - return CreatedTransactionResult(tx, current_fee, nChangePosInOut, feeCalc); + return CreatedTransactionResult(tx, current_fee, change_pos, feeCalc); } util::Result<CreatedTransactionResult> CreateTransaction( CWallet& wallet, const std::vector<CRecipient>& vecSend, - int change_pos, + std::optional<unsigned int> change_pos, const CCoinControl& coin_control, bool sign) { @@ -1294,7 +1336,7 @@ util::Result<CreatedTransactionResult> CreateTransaction( auto res = CreateTransactionInternal(wallet, vecSend, change_pos, coin_control, sign); TRACE4(coin_selection, normal_create_tx_internal, wallet.GetName().c_str(), bool(res), - res ? res->fee : 0, res ? res->change_pos : 0); + res ? res->fee : 0, res && res->change_pos.has_value() ? *res->change_pos : 0); if (!res) return res; const auto& txr_ungrouped = *res; // try with avoidpartialspends unless it's enabled already @@ -1304,16 +1346,15 @@ util::Result<CreatedTransactionResult> CreateTransaction( tmp_cc.m_avoid_partial_spends = true; // Reuse the change destination from the first creation attempt to avoid skipping BIP44 indexes - const int ungrouped_change_pos = txr_ungrouped.change_pos; - if (ungrouped_change_pos != -1) { - ExtractDestination(txr_ungrouped.tx->vout[ungrouped_change_pos].scriptPubKey, tmp_cc.destChange); + if (txr_ungrouped.change_pos) { + ExtractDestination(txr_ungrouped.tx->vout[*txr_ungrouped.change_pos].scriptPubKey, tmp_cc.destChange); } auto txr_grouped = CreateTransactionInternal(wallet, vecSend, change_pos, tmp_cc, sign); // if fee of this alternative one is within the range of the max fee, we use this one const bool use_aps{txr_grouped.has_value() ? (txr_grouped->fee <= txr_ungrouped.fee + wallet.m_max_aps_fee) : false}; TRACE5(coin_selection, aps_create_tx_internal, wallet.GetName().c_str(), use_aps, txr_grouped.has_value(), - txr_grouped.has_value() ? txr_grouped->fee : 0, txr_grouped.has_value() ? txr_grouped->change_pos : 0); + txr_grouped.has_value() ? txr_grouped->fee : 0, txr_grouped.has_value() && txr_grouped->change_pos.has_value() ? *txr_grouped->change_pos : 0); if (txr_grouped) { wallet.WalletLogPrintf("Fee non-grouped = %lld, grouped = %lld, using %s\n", txr_ungrouped.fee, txr_grouped->fee, use_aps ? "grouped" : "non-grouped"); @@ -1323,7 +1364,7 @@ util::Result<CreatedTransactionResult> CreateTransaction( return res; } -bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl coinControl) +util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional<unsigned int> change_pos, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl coinControl) { std::vector<CRecipient> vecSend; @@ -1336,6 +1377,12 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, vecSend.push_back(recipient); } + // Set the user desired locktime + coinControl.m_locktime = tx.nLockTime; + + // Set the user desired version + coinControl.m_version = tx.nVersion; + // Acquire the locks to prevent races to the new locked unspents between the // CreateTransaction call and LockCoin calls (when lockUnspents is true). LOCK(wallet.cs_wallet); @@ -1350,50 +1397,31 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, for (const CTxIn& txin : tx.vin) { const auto& outPoint = txin.prevout; - if (wallet.IsMine(outPoint)) { - // The input was found in the wallet, so select as internal - coinControl.Select(outPoint); - } else if (coins[outPoint].out.IsNull()) { - error = _("Unable to find UTXO for external input"); - return false; - } else { + PreselectedInput& preset_txin = coinControl.Select(outPoint); + if (!wallet.IsMine(outPoint)) { + if (coins[outPoint].out.IsNull()) { + return util::Error{_("Unable to find UTXO for external input")}; + } + // The input was not in the wallet, but is in the UTXO set, so select as external - coinControl.SelectExternal(outPoint, coins[outPoint].out); + preset_txin.SetTxOut(coins[outPoint].out); } + preset_txin.SetSequence(txin.nSequence); + preset_txin.SetScriptSig(txin.scriptSig); + preset_txin.SetScriptWitness(txin.scriptWitness); } - auto res = CreateTransaction(wallet, vecSend, nChangePosInOut, coinControl, false); + auto res = CreateTransaction(wallet, vecSend, change_pos, coinControl, false); if (!res) { - error = util::ErrorString(res); - return false; - } - const auto& txr = *res; - CTransactionRef tx_new = txr.tx; - nFeeRet = txr.fee; - nChangePosInOut = txr.change_pos; - - if (nChangePosInOut != -1) { - tx.vout.insert(tx.vout.begin() + nChangePosInOut, tx_new->vout[nChangePosInOut]); - } - - // Copy output sizes from new transaction; they may have had the fee - // subtracted from them. - for (unsigned int idx = 0; idx < tx.vout.size(); idx++) { - tx.vout[idx].nValue = tx_new->vout[idx].nValue; + return res; } - // Add new txins while keeping original txin scriptSig/order. - for (const CTxIn& txin : tx_new->vin) { - if (!coinControl.IsSelected(txin.prevout)) { - tx.vin.push_back(txin); - - } - if (lockUnspents) { + if (lockUnspents) { + for (const CTxIn& txin : res->tx->vin) { wallet.LockCoin(txin.prevout); } - } - return true; + return res; } } // namespace wallet diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 407627b5f1..504c078b80 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -207,9 +207,9 @@ struct CreatedTransactionResult CTransactionRef tx; CAmount fee; FeeCalculation fee_calc; - int change_pos; + std::optional<unsigned int> change_pos; - CreatedTransactionResult(CTransactionRef _tx, CAmount _fee, int _change_pos, const FeeCalculation& _fee_calc) + CreatedTransactionResult(CTransactionRef _tx, CAmount _fee, std::optional<unsigned int> _change_pos, const FeeCalculation& _fee_calc) : tx(_tx), fee(_fee), fee_calc(_fee_calc), change_pos(_change_pos) {} }; @@ -218,13 +218,13 @@ struct CreatedTransactionResult * selected by SelectCoins(); Also create the change output, when needed * @note passing change_pos as -1 will result in setting a random position */ -util::Result<CreatedTransactionResult> CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, int change_pos, const CCoinControl& coin_control, bool sign = true); +util::Result<CreatedTransactionResult> CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, std::optional<unsigned int> change_pos, const CCoinControl& coin_control, bool sign = true); /** * Insert additional inputs into the transaction by * calling CreateTransaction(); */ -bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl); +util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional<unsigned int> change_pos, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl); } // namespace wallet #endif // BITCOIN_WALLET_SPEND_H diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 2b7ae888d8..9fea14145f 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -1326,7 +1326,7 @@ BOOST_AUTO_TEST_CASE(SelectCoins_effective_value_test) cc.m_allow_other_inputs = false; COutput output = available_coins.All().at(0); cc.SetInputWeight(output.outpoint, 148); - cc.SelectExternal(output.outpoint, output.txout); + cc.Select(output.outpoint).SetTxOut(output.txout); LOCK(wallet->cs_wallet); const auto preset_inputs = *Assert(FetchSelectedInputs(*wallet, cc, cs_params)); diff --git a/src/wallet/test/fuzz/coincontrol.cpp b/src/wallet/test/fuzz/coincontrol.cpp index 0f71f28df2..f1efbc1cb8 100644 --- a/src/wallet/test/fuzz/coincontrol.cpp +++ b/src/wallet/test/fuzz/coincontrol.cpp @@ -60,7 +60,7 @@ FUZZ_TARGET(coincontrol, .init = initialize_coincontrol) }, [&] { const CTxOut tx_out{ConsumeMoney(fuzzed_data_provider), ConsumeScript(fuzzed_data_provider)}; - (void)coin_control.SelectExternal(out_point, tx_out); + (void)coin_control.Select(out_point).SetTxOut(tx_out); }, [&] { (void)coin_control.UnSelect(out_point); @@ -76,10 +76,7 @@ FUZZ_TARGET(coincontrol, .init = initialize_coincontrol) (void)coin_control.SetInputWeight(out_point, weight); }, [&] { - // Condition to avoid the assertion in GetInputWeight - if (coin_control.HasInputWeight(out_point)) { - (void)coin_control.GetInputWeight(out_point); - } + (void)coin_control.GetInputWeight(out_point); }); } } diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index 82cdd32302..203ab5f606 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -156,10 +156,9 @@ struct FuzzedWallet { coin_control.fOverrideFeeRate = fuzzed_data_provider.ConsumeBool(); // Add solving data (m_external_provider and SelectExternal)? - CAmount fee_out; int change_position{fuzzed_data_provider.ConsumeIntegralInRange<int>(-1, tx.vout.size() - 1)}; bilingual_str error; - (void)FundTransaction(*wallet, tx, fee_out, change_position, error, /*lockUnspents=*/false, subtract_fee_from_outputs, coin_control); + (void)FundTransaction(*wallet, tx, change_position, /*lockUnspents=*/false, subtract_fee_from_outputs, coin_control); } }; diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp index 5926d88129..b2d252b3f9 100644 --- a/src/wallet/test/spend_tests.cpp +++ b/src/wallet/test/spend_tests.cpp @@ -28,13 +28,12 @@ BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup) // instead of the miner. auto check_tx = [&wallet](CAmount leftover_input_amount) { CRecipient recipient{PubKeyDestination({}), 50 * COIN - leftover_input_amount, /*subtract_fee=*/true}; - constexpr int RANDOM_CHANGE_POSITION = -1; CCoinControl coin_control; coin_control.m_feerate.emplace(10000); coin_control.fOverrideFeeRate = true; // We need to use a change type with high cost of change so that the leftover amount will be dropped to fee instead of added as a change output coin_control.m_change_type = OutputType::LEGACY; - auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, coin_control); + auto res = CreateTransaction(*wallet, {recipient}, std::nullopt, coin_control); BOOST_CHECK(res); const auto& txr = *res; BOOST_CHECK_EQUAL(txr.tx->vout.size(), 1); diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp index ad8613d515..cbf3ccd1ec 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -24,7 +24,6 @@ std::unique_ptr<CWallet> CreateSyncedWallet(interfaces::Chain& chain, CChain& cc LOCK2(wallet->cs_wallet, ::cs_main); wallet->SetLastBlockProcessed(cchain.Height(), cchain.Tip()->GetBlockHash()); } - wallet->LoadWallet(); { LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index dd43705a84..3d1cbe36a8 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -558,8 +558,7 @@ public: CTransactionRef tx; CCoinControl dummy; { - constexpr int RANDOM_CHANGE_POSITION = -1; - auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, dummy); + auto res = CreateTransaction(*wallet, {recipient}, std::nullopt, dummy); BOOST_CHECK(res); tx = res->tx; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c34bf96b5e..892b9d51e4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2292,6 +2292,8 @@ DBErrors CWallet::LoadWallet() { LOCK(cs_wallet); + Assert(m_spk_managers.empty()); + Assert(m_wallet_flags == 0); DBErrors nLoadWalletRet = WalletBatch(GetDatabase()).LoadWallet(this); if (nLoadWalletRet == DBErrors::NEED_REWRITE) { @@ -3535,6 +3537,10 @@ void CWallet::SetupDescriptorScriptPubKeyMans(const CExtKey& master_key) { AssertLockHeld(cs_wallet); + // Create single batch txn + WalletBatch batch(GetDatabase()); + if (!batch.TxnBegin()) throw std::runtime_error("Error: cannot create db transaction for descriptors setup"); + for (bool internal : {false, true}) { for (OutputType t : OUTPUT_TYPES) { auto spk_manager = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, m_keypool_size)); @@ -3542,16 +3548,19 @@ void CWallet::SetupDescriptorScriptPubKeyMans(const CExtKey& master_key) if (IsLocked()) { throw std::runtime_error(std::string(__func__) + ": Wallet is locked, cannot setup new descriptors"); } - if (!spk_manager->CheckDecryptionKey(vMasterKey) && !spk_manager->Encrypt(vMasterKey, nullptr)) { + if (!spk_manager->CheckDecryptionKey(vMasterKey) && !spk_manager->Encrypt(vMasterKey, &batch)) { throw std::runtime_error(std::string(__func__) + ": Could not encrypt new descriptors"); } } - spk_manager->SetupDescriptorGeneration(master_key, t, internal); + spk_manager->SetupDescriptorGeneration(batch, master_key, t, internal); uint256 id = spk_manager->GetID(); AddScriptPubKeyMan(id, std::move(spk_manager)); - AddActiveScriptPubKeyMan(id, t, internal); + AddActiveScriptPubKeyManWithDb(batch, id, t, internal); } } + + // Ensure information is committed to disk + if (!batch.TxnCommit()) throw std::runtime_error("Error: cannot commit db transaction for descriptors setup"); } void CWallet::SetupDescriptorScriptPubKeyMans() @@ -3578,6 +3587,10 @@ void CWallet::SetupDescriptorScriptPubKeyMans() UniValue signer_res = signer.GetDescriptors(account); if (!signer_res.isObject()) throw std::runtime_error(std::string(__func__) + ": Unexpected result"); + + WalletBatch batch(GetDatabase()); + if (!batch.TxnBegin()) throw std::runtime_error("Error: cannot create db transaction for descriptors import"); + for (bool internal : {false, true}) { const UniValue& descriptor_vals = signer_res.find_value(internal ? "internal" : "receive"); if (!descriptor_vals.isArray()) throw std::runtime_error(std::string(__func__) + ": Unexpected result"); @@ -3594,18 +3607,26 @@ void CWallet::SetupDescriptorScriptPubKeyMans() } OutputType t = *desc->GetOutputType(); auto spk_manager = std::unique_ptr<ExternalSignerScriptPubKeyMan>(new ExternalSignerScriptPubKeyMan(*this, m_keypool_size)); - spk_manager->SetupDescriptor(std::move(desc)); + spk_manager->SetupDescriptor(batch, std::move(desc)); uint256 id = spk_manager->GetID(); AddScriptPubKeyMan(id, std::move(spk_manager)); - AddActiveScriptPubKeyMan(id, t, internal); + AddActiveScriptPubKeyManWithDb(batch, id, t, internal); } } + + // Ensure imported descriptors are committed to disk + if (!batch.TxnCommit()) throw std::runtime_error("Error: cannot commit db transaction for descriptors import"); } } void CWallet::AddActiveScriptPubKeyMan(uint256 id, OutputType type, bool internal) { WalletBatch batch(GetDatabase()); + return AddActiveScriptPubKeyManWithDb(batch, id, type, internal); +} + +void CWallet::AddActiveScriptPubKeyManWithDb(WalletBatch& batch, uint256 id, OutputType type, bool internal) +{ if (!batch.WriteActiveScriptPubKeyMan(static_cast<uint8_t>(type), id, internal)) { throw std::runtime_error(std::string(__func__) + ": writing active ScriptPubKeyMan id failed"); } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index bc45010200..11b7964316 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -419,6 +419,9 @@ private: // Must be the only method adding data to it. void AddScriptPubKeyMan(const uint256& id, std::unique_ptr<ScriptPubKeyMan> spkm_man); + // Same as 'AddActiveScriptPubKeyMan' but designed for use within a batch transaction context + void AddActiveScriptPubKeyManWithDb(WalletBatch& batch, uint256 id, OutputType type, bool internal); + /** * Catch wallet up to current chain, scanning new blocks, updating the best * block locator and m_last_block_processed, and registering for diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 96999de97b..ba453b47e7 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1499,17 +1499,19 @@ std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const Databas if (format == DatabaseFormat::SQLITE) { #ifdef USE_SQLITE return MakeSQLiteDatabase(path, options, status, error); -#endif +#else error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support SQLite database format.", fs::PathToString(path))); status = DatabaseStatus::FAILED_BAD_FORMAT; return nullptr; +#endif } #ifdef USE_BDB return MakeBerkeleyDatabase(path, options, status, error); -#endif +#else error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", fs::PathToString(path))); status = DatabaseStatus::FAILED_BAD_FORMAT; return nullptr; +#endif } } // namespace wallet |