diff options
Diffstat (limited to 'src')
58 files changed, 669 insertions, 575 deletions
diff --git a/src/.clang-tidy b/src/.clang-tidy new file mode 100644 index 0000000000..27616ad072 --- /dev/null +++ b/src/.clang-tidy @@ -0,0 +1,2 @@ +Checks: '-*,bugprone-argument-comment' +WarningsAsErrors: bugprone-argument-comment diff --git a/src/Makefile.am b/src/Makefile.am index a8d6591e98..eea98c7f22 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -268,6 +268,7 @@ BITCOIN_CORE_H = \ util/tokenpipe.h \ util/trace.h \ util/translation.h \ + util/types.h \ util/ui_change_type.h \ util/url.h \ util/vector.h \ @@ -548,6 +549,7 @@ libbitcoin_common_a_SOURCES = \ key.cpp \ key_io.cpp \ merkleblock.cpp \ + net_types.cpp \ netaddress.cpp \ netbase.cpp \ net_permissions.cpp \ diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 6af5ead443..a85a359960 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -337,8 +337,8 @@ bitcoin_test_clean : FORCE check-local: $(BITCOIN_TESTS:.cpp=.cpp.test) if BUILD_BITCOIN_TX - @echo "Running test/util/bitcoin-util-test.py..." - $(PYTHON) $(top_builddir)/test/util/bitcoin-util-test.py + @echo "Running test/util/test_runner.py..." + $(PYTHON) $(top_builddir)/test/util/test_runner.py endif @echo "Running test/util/rpcauth-test.py..." $(PYTHON) $(top_builddir)/test/util/rpcauth-test.py diff --git a/src/addrdb.cpp b/src/addrdb.cpp index a5383be7cf..1e73750ce3 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -18,64 +18,14 @@ #include <univalue.h> #include <util/settings.h> #include <util/system.h> - -CBanEntry::CBanEntry(const UniValue& json) - : nVersion(json["version"].get_int()), nCreateTime(json["ban_created"].get_int64()), - nBanUntil(json["banned_until"].get_int64()) -{ -} - -UniValue CBanEntry::ToJson() const -{ - UniValue json(UniValue::VOBJ); - json.pushKV("version", nVersion); - json.pushKV("ban_created", nCreateTime); - json.pushKV("banned_until", nBanUntil); - return json; -} +#include <util/translation.h> namespace { -static const char* BANMAN_JSON_ADDR_KEY = "address"; - -/** - * Convert a `banmap_t` object to a JSON array. - * @param[in] bans Bans list to convert. - * @return a JSON array, similar to the one returned by the `listbanned` RPC. Suitable for - * passing to `BanMapFromJson()`. - */ -UniValue BanMapToJson(const banmap_t& bans) +class DbNotFoundError : public std::exception { - UniValue bans_json(UniValue::VARR); - for (const auto& it : bans) { - const auto& address = it.first; - const auto& ban_entry = it.second; - UniValue j = ban_entry.ToJson(); - j.pushKV(BANMAN_JSON_ADDR_KEY, address.ToString()); - bans_json.push_back(j); - } - return bans_json; -} - -/** - * Convert a JSON array to a `banmap_t` object. - * @param[in] bans_json JSON to convert, must be as returned by `BanMapToJson()`. - * @param[out] bans Bans list to create from the JSON. - * @throws std::runtime_error if the JSON does not have the expected fields or they contain - * unparsable values. - */ -void BanMapFromJson(const UniValue& bans_json, banmap_t& bans) -{ - for (const auto& ban_entry_json : bans_json.getValues()) { - CSubNet subnet; - const auto& subnet_str = ban_entry_json[BANMAN_JSON_ADDR_KEY].get_str(); - if (!LookupSubNet(subnet_str, subnet)) { - throw std::runtime_error( - strprintf("Cannot parse banned address or subnet: %s", subnet_str)); - } - bans.insert_or_assign(subnet, CBanEntry{ban_entry_json}); - } -} + using std::exception::exception; +}; template <typename Stream, typename Data> bool SerializeDB(Stream& stream, const Data& data) @@ -134,47 +84,40 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data } template <typename Stream, typename Data> -bool DeserializeDB(Stream& stream, Data& data, bool fCheckSum = true) -{ - try { - CHashVerifier<Stream> verifier(&stream); - // de-serialize file header (network specific magic number) and .. - unsigned char pchMsgTmp[4]; - verifier >> pchMsgTmp; - // ... verify the network matches ours - if (memcmp(pchMsgTmp, Params().MessageStart(), sizeof(pchMsgTmp))) - return error("%s: Invalid network magic number", __func__); - - // de-serialize data - verifier >> data; - - // verify checksum - if (fCheckSum) { - uint256 hashTmp; - stream >> hashTmp; - if (hashTmp != verifier.GetHash()) { - return error("%s: Checksum mismatch, data corrupted", __func__); - } +void DeserializeDB(Stream& stream, Data& data, bool fCheckSum = true) +{ + CHashVerifier<Stream> verifier(&stream); + // de-serialize file header (network specific magic number) and .. + unsigned char pchMsgTmp[4]; + verifier >> pchMsgTmp; + // ... verify the network matches ours + if (memcmp(pchMsgTmp, Params().MessageStart(), sizeof(pchMsgTmp))) { + throw std::runtime_error{"Invalid network magic number"}; + } + + // de-serialize data + verifier >> data; + + // verify checksum + if (fCheckSum) { + uint256 hashTmp; + stream >> hashTmp; + if (hashTmp != verifier.GetHash()) { + throw std::runtime_error{"Checksum mismatch, data corrupted"}; } } - catch (const std::exception& e) { - return error("%s: Deserialize or I/O error - %s", __func__, e.what()); - } - - return true; } template <typename Data> -bool DeserializeFileDB(const fs::path& path, Data& data, int version) +void DeserializeFileDB(const fs::path& path, Data& data, int version) { // open input file, and associate with CAutoFile FILE* file = fsbridge::fopen(path, "rb"); CAutoFile filein(file, SER_DISK, version); if (filein.IsNull()) { - LogPrintf("Missing or invalid file %s\n", path.string()); - return false; + throw DbNotFoundError{}; } - return DeserializeDB(filein, data); + DeserializeDB(filein, data); } } // namespace @@ -227,24 +170,38 @@ bool CBanDB::Read(banmap_t& banSet) return true; } -CAddrDB::CAddrDB() -{ - pathAddr = gArgs.GetDataDirNet() / "peers.dat"; -} - -bool CAddrDB::Write(const CAddrMan& addr) +bool DumpPeerAddresses(const ArgsManager& args, const CAddrMan& addr) { + const auto pathAddr = args.GetDataDirNet() / "peers.dat"; return SerializeFileDB("peers", pathAddr, addr, CLIENT_VERSION); } -bool CAddrDB::Read(CAddrMan& addr) +void ReadFromStream(CAddrMan& addr, CDataStream& ssPeers) { - return DeserializeFileDB(pathAddr, addr, CLIENT_VERSION); + DeserializeDB(ssPeers, addr, false); } -bool CAddrDB::Read(CAddrMan& addr, CDataStream& ssPeers) +std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const ArgsManager& args, std::unique_ptr<CAddrMan>& addrman) { - return DeserializeDB(ssPeers, addr, false); + auto check_addrman = std::clamp<int32_t>(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000); + addrman = std::make_unique<CAddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman); + + int64_t nStart = GetTimeMillis(); + const auto path_addr{args.GetDataDirNet() / "peers.dat"}; + try { + DeserializeFileDB(path_addr, *addrman, CLIENT_VERSION); + LogPrintf("Loaded %i addresses from peers.dat %dms\n", addrman->size(), GetTimeMillis() - nStart); + } catch (const DbNotFoundError&) { + // Addrman can be in an inconsistent state after failure, reset it + addrman = std::make_unique<CAddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman); + LogPrintf("Creating peers.dat because the file was not found (%s)\n", path_addr); + DumpPeerAddresses(args, *addrman); + } catch (const std::exception& e) { + addrman = nullptr; + return strprintf(_("Invalid or corrupt peers.dat (%s). If you believe this is a bug, please report it to %s. As a workaround, you can move the file (%s) out of the way (rename, move, or delete) to have a new one created on the next start."), + e.what(), PACKAGE_BUGREPORT, path_addr); + } + return std::nullopt; } void DumpAnchors(const fs::path& anchors_db_path, const std::vector<CAddress>& anchors) @@ -256,9 +213,10 @@ void DumpAnchors(const fs::path& anchors_db_path, const std::vector<CAddress>& a std::vector<CAddress> ReadAnchors(const fs::path& anchors_db_path) { std::vector<CAddress> anchors; - if (DeserializeFileDB(anchors_db_path, anchors, CLIENT_VERSION | ADDRV2_FORMAT)) { + try { + DeserializeFileDB(anchors_db_path, anchors, CLIENT_VERSION | ADDRV2_FORMAT); LogPrintf("Loaded %i addresses from %s\n", anchors.size(), anchors_db_path.filename()); - } else { + } catch (const std::exception&) { anchors.clear(); } diff --git a/src/addrdb.h b/src/addrdb.h index 26400ee0b6..33cc1f9204 100644 --- a/src/addrdb.h +++ b/src/addrdb.h @@ -10,50 +10,18 @@ #include <net_types.h> // For banmap_t #include <univalue.h> +#include <optional> #include <vector> -class CAddress; +class ArgsManager; class CAddrMan; +class CAddress; class CDataStream; +struct bilingual_str; -class CBanEntry -{ -public: - static constexpr int CURRENT_VERSION{1}; - int nVersion{CBanEntry::CURRENT_VERSION}; - int64_t nCreateTime{0}; - int64_t nBanUntil{0}; - - CBanEntry() {} - - explicit CBanEntry(int64_t nCreateTimeIn) - : nCreateTime{nCreateTimeIn} {} - - /** - * Create a ban entry from JSON. - * @param[in] json A JSON representation of a ban entry, as created by `ToJson()`. - * @throw std::runtime_error if the JSON does not have the expected fields. - */ - explicit CBanEntry(const UniValue& json); - - /** - * Generate a JSON representation of this ban entry. - * @return JSON suitable for passing to the `CBanEntry(const UniValue&)` constructor. - */ - UniValue ToJson() const; -}; - -/** Access to the (IP) address database (peers.dat) */ -class CAddrDB -{ -private: - fs::path pathAddr; -public: - CAddrDB(); - bool Write(const CAddrMan& addr); - bool Read(CAddrMan& addr); - static bool Read(CAddrMan& addr, CDataStream& ssPeers); -}; +bool DumpPeerAddresses(const ArgsManager& args, const CAddrMan& addr); +/** Only used by tests. */ +void ReadFromStream(CAddrMan& addr, CDataStream& ssPeers); /** Access to the banlist database (banlist.json) */ class CBanDB @@ -79,6 +47,9 @@ public: bool Read(banmap_t& banSet); }; +/** Returns an error string on failure */ +std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const ArgsManager& args, std::unique_ptr<CAddrMan>& addrman); + /** * Dump the anchor IP address database (anchors.dat) * diff --git a/src/addrman.cpp b/src/addrman.cpp index 48e79c64ed..772c34ae77 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -5,10 +5,12 @@ #include <addrman.h> +#include <clientversion.h> #include <hash.h> #include <logging.h> #include <netaddress.h> #include <serialize.h> +#include <streams.h> #include <cmath> #include <optional> @@ -98,10 +100,11 @@ double CAddrInfo::GetChance(int64_t nNow) const return fChance; } -CAddrMan::CAddrMan(bool deterministic, int32_t consistency_check_ratio) +CAddrMan::CAddrMan(std::vector<bool> asmap, bool deterministic, int32_t consistency_check_ratio) : insecure_rand{deterministic} , nKey{deterministic ? uint256{1} : insecure_rand.rand256()} , m_consistency_check_ratio{consistency_check_ratio} + , m_asmap{std::move(asmap)} { for (auto& bucket : vvNew) { for (auto& entry : bucket) { @@ -242,9 +245,9 @@ void CAddrMan::Unserialize(Stream& s_) const uint8_t lowest_compatible = compat - INCOMPATIBILITY_BASE; if (lowest_compatible > FILE_FORMAT) { throw std::ios_base::failure(strprintf( - "Unsupported format of addrman database: %u. It is compatible with formats >=%u, " - "but the maximum supported by this version of %s is %u.", - format, lowest_compatible, PACKAGE_NAME, static_cast<uint8_t>(FILE_FORMAT))); + "Unsupported format of addrman database: %u. It is compatible with formats >=%u, " + "but the maximum supported by this version of %s is %u.", + uint8_t{format}, uint8_t{lowest_compatible}, PACKAGE_NAME, uint8_t{FILE_FORMAT})); } s >> nKey; @@ -1006,30 +1009,3 @@ CAddrInfo CAddrMan::SelectTriedCollision_() return mapInfo[id_old]; } - -std::vector<bool> CAddrMan::DecodeAsmap(fs::path path) -{ - std::vector<bool> bits; - FILE *filestr = fsbridge::fopen(path, "rb"); - CAutoFile file(filestr, SER_DISK, CLIENT_VERSION); - if (file.IsNull()) { - LogPrintf("Failed to open asmap file from disk\n"); - return bits; - } - fseek(filestr, 0, SEEK_END); - int length = ftell(filestr); - LogPrintf("Opened asmap file %s (%d bytes) from disk\n", path, length); - fseek(filestr, 0, SEEK_SET); - uint8_t cur_byte; - for (int i = 0; i < length; ++i) { - file >> cur_byte; - for (int bit = 0; bit < 8; ++bit) { - bits.push_back((cur_byte >> bit) & 1); - } - } - if (!SanityCheckASMap(bits)) { - LogPrintf("Sanity check of asmap file %s failed\n", path); - return {}; - } - return bits; -} diff --git a/src/addrman.h b/src/addrman.h index 2548b891ba..0885231ebc 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -6,23 +6,16 @@ #ifndef BITCOIN_ADDRMAN_H #define BITCOIN_ADDRMAN_H -#include <clientversion.h> -#include <config/bitcoin-config.h> #include <fs.h> -#include <hash.h> +#include <logging.h> #include <netaddress.h> #include <protocol.h> -#include <random.h> -#include <streams.h> #include <sync.h> #include <timedata.h> -#include <tinyformat.h> -#include <util/system.h> -#include <iostream> +#include <cstdint> #include <optional> #include <set> -#include <stdint.h> #include <unordered_map> #include <vector> @@ -149,32 +142,13 @@ static constexpr int ADDRMAN_BUCKET_SIZE{1 << ADDRMAN_BUCKET_SIZE_LOG2}; class CAddrMan { public: - // Compressed IP->ASN mapping, loaded from a file when a node starts. - // Should be always empty if no file was provided. - // This mapping is then used for bucketing nodes in Addrman. - // - // If asmap is provided, nodes will be bucketed by - // AS they belong to, in order to make impossible for a node - // to connect to several nodes hosted in a single AS. - // This is done in response to Erebus attack, but also to generally - // diversify the connections every node creates, - // especially useful when a large fraction of nodes - // operate under a couple of cloud providers. - // - // If a new asmap was provided, the existing records - // would be re-bucketed accordingly. - std::vector<bool> m_asmap; - - // Read asmap from provided binary file - static std::vector<bool> DecodeAsmap(fs::path path); - template <typename Stream> void Serialize(Stream& s_) const EXCLUSIVE_LOCKS_REQUIRED(!cs); template <typename Stream> void Unserialize(Stream& s_) EXCLUSIVE_LOCKS_REQUIRED(!cs); - explicit CAddrMan(bool deterministic, int32_t consistency_check_ratio); + explicit CAddrMan(std::vector<bool> asmap, bool deterministic, int32_t consistency_check_ratio); ~CAddrMan() { @@ -296,6 +270,8 @@ public: Check(); } + const std::vector<bool>& GetAsmap() const { return m_asmap; } + private: //! A mutex to protect the inner data structures. mutable Mutex cs; @@ -363,6 +339,22 @@ private: /** Perform consistency checks every m_consistency_check_ratio operations (if non-zero). */ const int32_t m_consistency_check_ratio; + // Compressed IP->ASN mapping, loaded from a file when a node starts. + // Should be always empty if no file was provided. + // This mapping is then used for bucketing nodes in Addrman. + // + // If asmap is provided, nodes will be bucketed by + // AS they belong to, in order to make impossible for a node + // to connect to several nodes hosted in a single AS. + // This is done in response to Erebus attack, but also to generally + // diversify the connections every node creates, + // especially useful when a large fraction of nodes + // operate under a couple of cloud providers. + // + // If a new asmap was provided, the existing records + // would be re-bucketed accordingly. + const std::vector<bool> m_asmap; + //! Find an entry. CAddrInfo* Find(const CNetAddr& addr, int *pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs); diff --git a/src/bench/addrman.cpp b/src/bench/addrman.cpp index d69a651811..e5dd571a4c 100644 --- a/src/bench/addrman.cpp +++ b/src/bench/addrman.cpp @@ -5,6 +5,7 @@ #include <addrman.h> #include <bench/bench.h> #include <random.h> +#include <util/check.h> #include <util/time.h> #include <optional> @@ -73,14 +74,14 @@ static void AddrManAdd(benchmark::Bench& bench) CreateAddresses(); bench.run([&] { - CAddrMan addrman{/* deterministic */ false, /* consistency_check_ratio */ 0}; + CAddrMan addrman{/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0}; AddAddressesToAddrMan(addrman); }); } static void AddrManSelect(benchmark::Bench& bench) { - CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0); + CAddrMan addrman(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0); FillAddrMan(addrman); @@ -92,7 +93,7 @@ static void AddrManSelect(benchmark::Bench& bench) static void AddrManGetAddr(benchmark::Bench& bench) { - CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0); + CAddrMan addrman(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0); FillAddrMan(addrman); @@ -110,11 +111,12 @@ static void AddrManGood(benchmark::Bench& bench) * we want to do the same amount of work in every loop iteration. */ bench.epochs(5).epochIterations(1); - const size_t addrman_count{bench.epochs() * bench.epochIterations()}; + const uint64_t addrman_count{bench.epochs() * bench.epochIterations()}; + Assert(addrman_count == 5U); std::vector<std::unique_ptr<CAddrMan>> addrmans(addrman_count); for (size_t i{0}; i < addrman_count; ++i) { - addrmans[i] = std::make_unique<CAddrMan>(/* deterministic */ false, /* consistency_check_ratio */ 0); + addrmans[i] = std::make_unique<CAddrMan>(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0); FillAddrMan(*addrmans[i]); } diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index aa79aab755..934b574f8b 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -53,7 +53,7 @@ static void CoinSelection(benchmark::Bench& bench) const CoinSelectionParams coin_selection_params(/* change_output_size= */ 34, /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0), /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), - /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); + /* tx_noinputs_size= */ 0, /* avoid_partial= */ false); bench.run([&] { std::set<CInputCoin> setCoinsRet; CAmount nValueRet; diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index 38af0bd120..e75ba81b54 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -93,9 +93,6 @@ static void SetupCliArgs(ArgsManager& argsman) /** libevent event log callback */ static void libevent_log_cb(int severity, const char *msg) { -#ifndef EVENT_LOG_ERR // EVENT_LOG_ERR was added in 2.0.19; but before then _EVENT_LOG_ERR existed. -# define EVENT_LOG_ERR _EVENT_LOG_ERR -#endif // Ignore everything other than errors if (severity >= EVENT_LOG_ERR) { throw std::runtime_error(strprintf("libevent error: %s", msg)); @@ -514,7 +511,7 @@ public: } // Generate report header. - std::string result{strprintf("%s %s%s - %i%s\n\n", PACKAGE_NAME, FormatFullVersion(), ChainToString(), networkinfo["protocolversion"].get_int(), networkinfo["subversion"].get_str())}; + std::string result{strprintf("%s client %s%s - server %i%s\n\n", PACKAGE_NAME, FormatFullVersion(), ChainToString(), networkinfo["protocolversion"].get_int(), networkinfo["subversion"].get_str())}; // Report detailed peer connections list sorted by direction and minimum ping time. if (DetailsRequested() && !m_peers.empty()) { diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 8741ad9b86..fa0379f612 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -338,10 +338,6 @@ static void HTTPWorkQueueRun(WorkQueue<HTTPClosure>* queue, int worker_num) /** libevent event log callback */ static void libevent_log_cb(int severity, const char *msg) { -#ifndef EVENT_LOG_WARN -// EVENT_LOG_WARN was added in 2.0.19; but before then _EVENT_LOG_WARN existed. -# define EVENT_LOG_WARN _EVENT_LOG_WARN -#endif if (severity >= EVENT_LOG_WARN) // Log warn messages and higher without debug category LogPrintf("libevent: %s\n", msg); else diff --git a/src/init.cpp b/src/init.cpp index d4a3c891b1..d4ba441b0c 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1171,23 +1171,41 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) fDiscover = args.GetBoolArg("-discover", true); const bool ignores_incoming_txs{args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)}; - assert(!node.addrman); - auto check_addrman = std::clamp<int32_t>(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000); - node.addrman = std::make_unique<CAddrMan>(/* deterministic */ false, /* consistency_check_ratio */ check_addrman); { - // Load addresses from peers.dat - uiInterface.InitMessage(_("Loading P2P addresses…").translated); - int64_t nStart = GetTimeMillis(); - CAddrDB adb; - if (adb.Read(*node.addrman)) { - LogPrintf("Loaded %i addresses from peers.dat %dms\n", node.addrman->size(), GetTimeMillis() - nStart); + // Initialize addrman + assert(!node.addrman); + + // Read asmap file if configured + std::vector<bool> asmap; + if (args.IsArgSet("-asmap")) { + fs::path asmap_path = fs::path(args.GetArg("-asmap", "")); + if (asmap_path.empty()) { + asmap_path = DEFAULT_ASMAP_FILENAME; + } + if (!asmap_path.is_absolute()) { + asmap_path = gArgs.GetDataDirNet() / asmap_path; + } + if (!fs::exists(asmap_path)) { + InitError(strprintf(_("Could not find asmap file %s"), asmap_path)); + return false; + } + asmap = DecodeAsmap(asmap_path); + if (asmap.size() == 0) { + InitError(strprintf(_("Could not parse asmap file %s"), asmap_path)); + return false; + } + const uint256 asmap_version = SerializeHash(asmap); + LogPrintf("Using asmap version %s for IP bucketing\n", asmap_version.ToString()); } else { - // Addrman can be in an inconsistent state after failure, reset it - node.addrman = std::make_unique<CAddrMan>(/* deterministic */ false, /* consistency_check_ratio */ check_addrman); - LogPrintf("Recreating peers.dat\n"); - adb.Write(*node.addrman); + LogPrintf("Using /16 prefix for IP bucketing\n"); + } + + uiInterface.InitMessage(_("Loading P2P addresses…").translated); + if (const auto error{LoadAddrman(asmap, args, node.addrman)}) { + return InitError(*error); } } + assert(!node.banman); node.banman = std::make_unique<BanMan>(gArgs.GetDataDirNet() / "banlist", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME)); assert(!node.connman); @@ -1292,31 +1310,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) return InitError(ResolveErrMsg("externalip", strAddr)); } - // Read asmap file if configured - if (args.IsArgSet("-asmap")) { - fs::path asmap_path = fs::path(args.GetArg("-asmap", "")); - if (asmap_path.empty()) { - asmap_path = DEFAULT_ASMAP_FILENAME; - } - if (!asmap_path.is_absolute()) { - asmap_path = gArgs.GetDataDirNet() / asmap_path; - } - if (!fs::exists(asmap_path)) { - InitError(strprintf(_("Could not find asmap file %s"), asmap_path)); - return false; - } - std::vector<bool> asmap = CAddrMan::DecodeAsmap(asmap_path); - if (asmap.size() == 0) { - InitError(strprintf(_("Could not parse asmap file %s"), asmap_path)); - return false; - } - const uint256 asmap_version = SerializeHash(asmap); - node.connman->SetAsmap(std::move(asmap)); - LogPrintf("Using asmap version %s for IP bucketing\n", asmap_version.ToString()); - } else { - LogPrintf("Using /16 prefix for IP bucketing\n"); - } - #if ENABLE_ZMQ g_zmq_notification_interface = CZMQNotificationInterface::Create(); diff --git a/src/logging.cpp b/src/logging.cpp index b456108b61..eb2c750296 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -159,6 +159,7 @@ const CLogCategoryDesc LogCategories[] = {BCLog::VALIDATION, "validation"}, {BCLog::I2P, "i2p"}, {BCLog::IPC, "ipc"}, + {BCLog::LOCK, "lock"}, {BCLog::ALL, "1"}, {BCLog::ALL, "all"}, }; diff --git a/src/logging.h b/src/logging.h index 38d73863e7..53a89d28bd 100644 --- a/src/logging.h +++ b/src/logging.h @@ -59,6 +59,7 @@ namespace BCLog { VALIDATION = (1 << 21), I2P = (1 << 22), IPC = (1 << 23), + LOCK = (1 << 24), ALL = ~(uint32_t)0, }; diff --git a/src/logging/timer.h b/src/logging/timer.h index 159920e397..79627b1fe3 100644 --- a/src/logging/timer.h +++ b/src/logging/timer.h @@ -9,6 +9,7 @@ #include <logging.h> #include <util/macros.h> #include <util/time.h> +#include <util/types.h> #include <chrono> #include <string> @@ -58,21 +59,15 @@ public: return strprintf("%s: %s", m_prefix, msg); } - std::string units = ""; - float divisor = 1; - - if (std::is_same<TimeType, std::chrono::microseconds>::value) { - units = "μs"; - } else if (std::is_same<TimeType, std::chrono::milliseconds>::value) { - units = "ms"; - divisor = 1000.; - } else if (std::is_same<TimeType, std::chrono::seconds>::value) { - units = "s"; - divisor = 1000. * 1000.; + if constexpr (std::is_same<TimeType, std::chrono::microseconds>::value) { + return strprintf("%s: %s (%iμs)", m_prefix, msg, end_time.count()); + } else if constexpr (std::is_same<TimeType, std::chrono::milliseconds>::value) { + return strprintf("%s: %s (%.2fms)", m_prefix, msg, end_time.count() * 0.001); + } else if constexpr (std::is_same<TimeType, std::chrono::seconds>::value) { + return strprintf("%s: %s (%.2fs)", m_prefix, msg, end_time.count() * 0.000001); + } else { + static_assert(ALWAYS_FALSE<TimeType>, "Error: unexpected time type"); } - - const float time_ms = end_time.count() / divisor; - return strprintf("%s: %s (%.2f%s)", m_prefix, msg, time_ms, units); } private: @@ -87,12 +82,13 @@ private: //! Forwarded on to LogPrint if specified - has the effect of only //! outputting the timing log when a particular debug= category is specified. const BCLog::LogFlags m_log_category{}; - }; } // namespace BCLog +#define LOG_TIME_MICROS_WITH_CATEGORY(end_msg, log_category) \ + BCLog::Timer<std::chrono::microseconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, log_category) #define LOG_TIME_MILLIS_WITH_CATEGORY(end_msg, log_category) \ BCLog::Timer<std::chrono::milliseconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, log_category) #define LOG_TIME_SECONDS(end_msg) \ diff --git a/src/net.cpp b/src/net.cpp index 9b1e17c587..c72cd75ba7 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -9,6 +9,7 @@ #include <net.h> +#include <addrdb.h> #include <banman.h> #include <clientversion.h> #include <compat.h> @@ -24,6 +25,7 @@ #include <scheduler.h> #include <util/sock.h> #include <util/strencodings.h> +#include <util/system.h> #include <util/thread.h> #include <util/trace.h> #include <util/translation.h> @@ -552,14 +554,13 @@ Network CNode::ConnectedThroughNetwork() const #undef X #define X(name) stats.name = name -void CNode::copyStats(CNodeStats &stats, const std::vector<bool> &m_asmap) +void CNode::CopyStats(CNodeStats& stats) { stats.nodeid = this->GetId(); X(nServices); X(addr); X(addrBind); stats.m_network = ConnectedThroughNetwork(); - stats.m_mapped_as = addr.GetMappedAS(m_asmap); if (m_tx_relay != nullptr) { LOCK(m_tx_relay->cs_filter); stats.fRelayTxes = m_tx_relay->fRelayTxes; @@ -1747,8 +1748,7 @@ void CConnman::DumpAddresses() { int64_t nStart = GetTimeMillis(); - CAddrDB adb; - adb.Write(addrman); + DumpPeerAddresses(::gArgs, addrman); LogPrint(BCLog::NET, "Flushed %d addresses to peers.dat %dms\n", addrman.size(), GetTimeMillis() - nStart); @@ -1921,7 +1921,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect) case ConnectionType::BLOCK_RELAY: case ConnectionType::ADDR_FETCH: case ConnectionType::FEELER: - setConnected.insert(pnode->addr.GetGroup(addrman.m_asmap)); + setConnected.insert(pnode->addr.GetGroup(addrman.GetAsmap())); } // no default case, so the compiler can warn about missing cases } } @@ -1995,7 +1995,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect) m_anchors.pop_back(); if (!addr.IsValid() || IsLocal(addr) || !IsReachable(addr) || !HasAllDesirableServiceFlags(addr.nServices) || - setConnected.count(addr.GetGroup(addrman.m_asmap))) continue; + setConnected.count(addr.GetGroup(addrman.GetAsmap()))) continue; addrConnect = addr; LogPrint(BCLog::NET, "Trying to make an anchor connection to %s\n", addrConnect.ToString()); break; @@ -2035,7 +2035,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect) } // Require outbound connections, other than feelers, to be to distinct network groups - if (!fFeeler && setConnected.count(addr.GetGroup(addrman.m_asmap))) { + if (!fFeeler && setConnected.count(addr.GetGroup(addrman.GetAsmap()))) { break; } @@ -2804,7 +2804,8 @@ void CConnman::GetNodeStats(std::vector<CNodeStats>& vstats) const vstats.reserve(vNodes.size()); for (CNode* pnode : vNodes) { vstats.emplace_back(); - pnode->copyStats(vstats.back(), addrman.m_asmap); + pnode->CopyStats(vstats.back()); + vstats.back().m_mapped_as = pnode->addr.GetMappedAS(addrman.GetAsmap()); } } @@ -3067,7 +3068,7 @@ CSipHasher CConnman::GetDeterministicRandomizer(uint64_t id) const uint64_t CConnman::CalculateKeyedNetGroup(const CAddress& ad) const { - std::vector<unsigned char> vchNetGroup(ad.GetGroup(addrman.m_asmap)); + std::vector<unsigned char> vchNetGroup(ad.GetGroup(addrman.GetAsmap())); return GetDeterministicRandomizer(RANDOMIZER_ID_NETGROUP).Write(vchNetGroup.data(), vchNetGroup.size()).Finalize(); } @@ -6,7 +6,6 @@ #ifndef BITCOIN_NET_H #define BITCOIN_NET_H -#include <addrdb.h> #include <addrman.h> #include <amount.h> #include <bloom.h> @@ -652,7 +651,7 @@ public: void CloseSocketDisconnect(); - void copyStats(CNodeStats &stats, const std::vector<bool> &m_asmap); + void CopyStats(CNodeStats& stats); ServiceFlags GetLocalServices() const { @@ -768,7 +767,6 @@ public: bool m_use_addrman_outgoing = true; std::vector<std::string> m_specified_outgoing; std::vector<std::string> m_added_nodes; - std::vector<bool> m_asmap; bool m_i2p_accept_incoming; }; @@ -943,8 +941,6 @@ public: */ std::chrono::microseconds PoissonNextSendInbound(std::chrono::microseconds now, std::chrono::seconds average_interval); - void SetAsmap(std::vector<bool> asmap) { addrman.m_asmap = std::move(asmap); } - /** Return true if we should disconnect the peer for failing an inactivity check. */ bool ShouldRunInactivityChecks(const CNode& node, std::optional<int64_t> now=std::nullopt) const; diff --git a/src/net_types.cpp b/src/net_types.cpp new file mode 100644 index 0000000000..c8f57fe6c6 --- /dev/null +++ b/src/net_types.cpp @@ -0,0 +1,65 @@ +// Copyright (c) 2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include <net_types.h> + +#include <netaddress.h> +#include <netbase.h> +#include <univalue.h> + +CBanEntry::CBanEntry(const UniValue& json) + : nVersion(json["version"].get_int()), nCreateTime(json["ban_created"].get_int64()), + nBanUntil(json["banned_until"].get_int64()) +{ +} + +UniValue CBanEntry::ToJson() const +{ + UniValue json(UniValue::VOBJ); + json.pushKV("version", nVersion); + json.pushKV("ban_created", nCreateTime); + json.pushKV("banned_until", nBanUntil); + return json; +} + +static const char* BANMAN_JSON_ADDR_KEY = "address"; + +/** + * Convert a `banmap_t` object to a JSON array. + * @param[in] bans Bans list to convert. + * @return a JSON array, similar to the one returned by the `listbanned` RPC. Suitable for + * passing to `BanMapFromJson()`. + */ +UniValue BanMapToJson(const banmap_t& bans) +{ + UniValue bans_json(UniValue::VARR); + for (const auto& it : bans) { + const auto& address = it.first; + const auto& ban_entry = it.second; + UniValue j = ban_entry.ToJson(); + j.pushKV(BANMAN_JSON_ADDR_KEY, address.ToString()); + bans_json.push_back(j); + } + return bans_json; +} + +/** + * Convert a JSON array to a `banmap_t` object. + * @param[in] bans_json JSON to convert, must be as returned by `BanMapToJson()`. + * @param[out] bans Bans list to create from the JSON. + * @throws std::runtime_error if the JSON does not have the expected fields or they contain + * unparsable values. + */ +void BanMapFromJson(const UniValue& bans_json, banmap_t& bans) +{ + for (const auto& ban_entry_json : bans_json.getValues()) { + CSubNet subnet; + const auto& subnet_str = ban_entry_json[BANMAN_JSON_ADDR_KEY].get_str(); + if (!LookupSubNet(subnet_str, subnet)) { + throw std::runtime_error( + strprintf("Cannot parse banned address or subnet: %s", subnet_str)); + } + bans.insert_or_assign(subnet, CBanEntry{ban_entry_json}); + } +} diff --git a/src/net_types.h b/src/net_types.h index d55a8cde6c..ffdc24c772 100644 --- a/src/net_types.h +++ b/src/net_types.h @@ -5,11 +5,56 @@ #ifndef BITCOIN_NET_TYPES_H #define BITCOIN_NET_TYPES_H +#include <cstdint> #include <map> -class CBanEntry; class CSubNet; +class UniValue; + +class CBanEntry +{ +public: + static constexpr int CURRENT_VERSION{1}; + int nVersion{CBanEntry::CURRENT_VERSION}; + int64_t nCreateTime{0}; + int64_t nBanUntil{0}; + + CBanEntry() {} + + explicit CBanEntry(int64_t nCreateTimeIn) + : nCreateTime{nCreateTimeIn} {} + + /** + * Create a ban entry from JSON. + * @param[in] json A JSON representation of a ban entry, as created by `ToJson()`. + * @throw std::runtime_error if the JSON does not have the expected fields. + */ + explicit CBanEntry(const UniValue& json); + + /** + * Generate a JSON representation of this ban entry. + * @return JSON suitable for passing to the `CBanEntry(const UniValue&)` constructor. + */ + UniValue ToJson() const; +}; using banmap_t = std::map<CSubNet, CBanEntry>; +/** + * Convert a `banmap_t` object to a JSON array. + * @param[in] bans Bans list to convert. + * @return a JSON array, similar to the one returned by the `listbanned` RPC. Suitable for + * passing to `BanMapFromJson()`. + */ +UniValue BanMapToJson(const banmap_t& bans); + +/** + * Convert a JSON array to a `banmap_t` object. + * @param[in] bans_json JSON to convert, must be as returned by `BanMapToJson()`. + * @param[out] bans Bans list to create from the JSON. + * @throws std::runtime_error if the JSON does not have the expected fields or they contain + * unparsable values. + */ +void BanMapFromJson(const UniValue& bans_json, banmap_t& bans); + #endif // BITCOIN_NET_TYPES_H diff --git a/src/netaddress.cpp b/src/netaddress.cpp index e7b3377475..b2f4945e3b 100644 --- a/src/netaddress.cpp +++ b/src/netaddress.cpp @@ -1242,8 +1242,3 @@ bool operator<(const CSubNet& a, const CSubNet& b) { return (a.network < b.network || (a.network == b.network && memcmp(a.netmask, b.netmask, 16) < 0)); } - -bool SanityCheckASMap(const std::vector<bool>& asmap) -{ - return SanityCheckASMap(asmap, 128); // For IP address lookups, the input is 128 bits -} diff --git a/src/netaddress.h b/src/netaddress.h index eb35ed3fac..cfb2edcd34 100644 --- a/src/netaddress.h +++ b/src/netaddress.h @@ -567,6 +567,4 @@ public: } }; -bool SanityCheckASMap(const std::vector<bool>& asmap); - #endif // BITCOIN_NETADDRESS_H diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index 43624c7993..15527afb8a 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -13,7 +13,7 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) { AssertLockHeld(pool.cs); - CTxMemPool::setEntries setAncestors; + CTxMemPool::setEntries ancestors; // First check the transaction itself. if (SignalsOptInRBF(tx)) { @@ -31,9 +31,9 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) uint64_t noLimit = std::numeric_limits<uint64_t>::max(); std::string dummy; CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash()); - pool.CalculateMemPoolAncestors(entry, setAncestors, noLimit, noLimit, noLimit, noLimit, dummy, false); + pool.CalculateMemPoolAncestors(entry, ancestors, noLimit, noLimit, noLimit, noLimit, dummy, false); - for (CTxMemPool::txiter it : setAncestors) { + for (CTxMemPool::txiter it : ancestors) { if (SignalsOptInRBF(it->GetTx())) { return RBFTransactionState::REPLACEABLE_BIP125; } @@ -47,33 +47,127 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx) return SignalsOptInRBF(tx) ? RBFTransactionState::REPLACEABLE_BIP125 : RBFTransactionState::UNKNOWN; } -bool GetEntriesForConflicts(const CTransaction& tx, - CTxMemPool& m_pool, - const CTxMemPool::setEntries& setIterConflicting, - CTxMemPool::setEntries& allConflicting, - std::string& err_string) +std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx, + CTxMemPool& pool, + const CTxMemPool::setEntries& iters_conflicting, + CTxMemPool::setEntries& all_conflicts) { - AssertLockHeld(m_pool.cs); - const uint256 hash = tx.GetHash(); + AssertLockHeld(pool.cs); + const uint256 txid = tx.GetHash(); uint64_t nConflictingCount = 0; - for (const auto& mi : setIterConflicting) { + for (const auto& mi : iters_conflicting) { nConflictingCount += mi->GetCountWithDescendants(); - // This potentially overestimates the number of actual descendants - // but we just want to be conservative to avoid doing too much - // work. + // This potentially overestimates the number of actual descendants but we just want to be + // conservative to avoid doing too much work. if (nConflictingCount > MAX_BIP125_REPLACEMENT_CANDIDATES) { - err_string = strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n", - hash.ToString(), - nConflictingCount, - MAX_BIP125_REPLACEMENT_CANDIDATES); - return false; + return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n", + txid.ToString(), + nConflictingCount, + MAX_BIP125_REPLACEMENT_CANDIDATES); } } // If not too many to replace, then calculate the set of // transactions that would have to be evicted - for (CTxMemPool::txiter it : setIterConflicting) { - m_pool.CalculateDescendants(it, allConflicting); + for (CTxMemPool::txiter it : iters_conflicting) { + pool.CalculateDescendants(it, all_conflicts); } - return true; + return std::nullopt; } +std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx, + const CTxMemPool& pool, + const CTxMemPool::setEntries& iters_conflicting) +{ + AssertLockHeld(pool.cs); + std::set<uint256> parents_of_conflicts; + for (const auto& mi : iters_conflicting) { + for (const CTxIn &txin : mi->GetTx().vin) { + parents_of_conflicts.insert(txin.prevout.hash); + } + } + + for (unsigned int j = 0; j < tx.vin.size(); j++) { + // We don't want to accept replacements that require low feerate junk to be mined first. + // Ideally we'd keep track of the ancestor feerates and make the decision based on that, but + // for now requiring all new inputs to be confirmed works. + // + // Note that if you relax this to make RBF a little more useful, this may break the + // CalculateMempoolAncestors RBF relaxation, above. See the comment above the first + // CalculateMempoolAncestors call for more info. + if (!parents_of_conflicts.count(tx.vin[j].prevout.hash)) { + // Rather than check the UTXO set - potentially expensive - it's cheaper to just check + // if the new input refers to a tx that's in the mempool. + if (pool.exists(tx.vin[j].prevout.hash)) { + return strprintf("replacement %s adds unconfirmed input, idx %d", + tx.GetHash().ToString(), j); + } + } + } + return std::nullopt; +} + +std::optional<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors, + const std::set<uint256>& direct_conflicts, + const uint256& txid) +{ + for (CTxMemPool::txiter ancestorIt : ancestors) { + const uint256 &hashAncestor = ancestorIt->GetTx().GetHash(); + if (direct_conflicts.count(hashAncestor)) { + return strprintf("%s spends conflicting transaction %s", + txid.ToString(), + hashAncestor.ToString()); + } + } + return std::nullopt; +} + +std::optional<std::string> PaysMoreThanConflicts(const CTxMemPool::setEntries& iters_conflicting, + CFeeRate replacement_feerate, + const uint256& txid) +{ + for (const auto& mi : iters_conflicting) { + // Don't allow the replacement to reduce the feerate of the mempool. + // + // We usually don't want to accept replacements with lower feerates than what they replaced + // as that would lower the feerate of the next block. Requiring that the feerate always be + // increased is also an easy-to-reason about way to prevent DoS attacks via replacements. + // + // We only consider the feerates of transactions being directly replaced, not their indirect + // descendants. While that does mean high feerate children are ignored when deciding whether + // or not to replace, we do require the replacement to pay more overall fees too, mitigating + // most cases. + CFeeRate original_feerate(mi->GetModifiedFee(), mi->GetTxSize()); + if (replacement_feerate <= original_feerate) { + return strprintf("rejecting replacement %s; new feerate %s <= old feerate %s", + txid.ToString(), + replacement_feerate.ToString(), + original_feerate.ToString()); + } + } + return std::nullopt; +} + +std::optional<std::string> PaysForRBF(CAmount original_fees, + CAmount replacement_fees, + size_t replacement_vsize, + const uint256& txid) +{ + // The replacement must pay greater fees than the transactions it + // replaces - if we did the bandwidth used by those conflicting + // transactions would not be paid for. + if (replacement_fees < original_fees) { + return strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s", + txid.ToString(), FormatMoney(replacement_fees), FormatMoney(original_fees)); + } + + // Finally in addition to paying more fees than the conflicts the + // new transaction must pay for its own bandwidth. + CAmount additional_fees = replacement_fees - original_fees; + if (additional_fees < ::incrementalRelayFee.GetFee(replacement_vsize)) { + return strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s", + txid.ToString(), + FormatMoney(additional_fees), + FormatMoney(::incrementalRelayFee.GetFee(replacement_vsize))); + } + return std::nullopt; +} diff --git a/src/policy/rbf.h b/src/policy/rbf.h index a67e9058df..56468a09b2 100644 --- a/src/policy/rbf.h +++ b/src/policy/rbf.h @@ -35,19 +35,61 @@ enum class RBFTransactionState { RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(pool.cs); RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx); -/** Get all descendants of setIterConflicting. Also enforce BIP125 Rule #5, "The number of original +/** Get all descendants of iters_conflicting. Also enforce BIP125 Rule #5, "The number of original * transactions to be replaced and their descendant transactions which will be evicted from the * mempool must not exceed a total of 100 transactions." Quit as early as possible. There cannot be * more than MAX_BIP125_REPLACEMENT_CANDIDATES potential entries. - * @param[in] setIterConflicting The set of iterators to mempool entries. - * @param[out] err_string Used to return errors, if any. - * @param[out] allConflicting Populated with all the mempool entries that would be replaced, - * which includes descendants of setIterConflicting. Not cleared at + * @param[in] iters_conflicting The set of iterators to mempool entries. + * @param[out] all_conflicts Populated with all the mempool entries that would be replaced, + * which includes descendants of iters_conflicting. Not cleared at * the start; any existing mempool entries will remain in the set. - * @returns false if Rule 5 is broken. + * @returns an error message if Rule #5 is broken, otherwise a std::nullopt. */ -bool GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& m_pool, - const CTxMemPool::setEntries& setIterConflicting, - CTxMemPool::setEntries& allConflicting, - std::string& err_string) EXCLUSIVE_LOCKS_REQUIRED(m_pool.cs); +std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& pool, + const CTxMemPool::setEntries& iters_conflicting, + CTxMemPool::setEntries& all_conflicts) + EXCLUSIVE_LOCKS_REQUIRED(pool.cs); + +/** BIP125 Rule #2: "The replacement transaction may only include an unconfirmed input if that input + * was included in one of the original transactions." + * @returns error message if Rule #2 is broken, otherwise std::nullopt. */ +std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx, const CTxMemPool& pool, + const CTxMemPool::setEntries& iters_conflicting) + EXCLUSIVE_LOCKS_REQUIRED(pool.cs); + +/** Check the intersection between two sets of transactions (a set of mempool entries and a set of + * txids) to make sure they are disjoint. + * @param[in] ancestors Set of mempool entries corresponding to ancestors of the + * replacement transactions. + * @param[in] direct_conflicts Set of txids corresponding to the mempool conflicts + * (candidates to be replaced). + * @param[in] txid Transaction ID, included in the error message if violation occurs. + * @returns error message if the sets intersect, std::nullopt if they are disjoint. + */ +std::optional<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors, + const std::set<uint256>& direct_conflicts, + const uint256& txid); + +/** Check that the feerate of the replacement transaction(s) is higher than the feerate of each + * of the transactions in iters_conflicting. + * @param[in] iters_conflicting The set of mempool entries. + * @returns error message if fees insufficient, otherwise std::nullopt. + */ +std::optional<std::string> PaysMoreThanConflicts(const CTxMemPool::setEntries& iters_conflicting, + CFeeRate replacement_feerate, const uint256& txid); + +/** Enforce BIP125 Rule #3 "The replacement transaction pays an absolute fee of at least the sum + * paid by the original transactions." Enforce BIP125 Rule #4 "The replacement transaction must also + * pay for its own bandwidth at or above the rate set by the node's minimum relay fee setting." + * @param[in] original_fees Total modified fees of original transaction(s). + * @param[in] replacement_fees Total modified fees of replacement transaction(s). + * @param[in] replacement_vsize Total virtual size of replacement transaction(s). + * @param[in] txid Transaction ID, included in the error message if violation occurs. + * @returns error string if fees are insufficient, otherwise std::nullopt. + */ +std::optional<std::string> PaysForRBF(CAmount original_fees, + CAmount replacement_fees, + size_t replacement_vsize, + const uint256& txid); + #endif // BITCOIN_POLICY_RBF_H diff --git a/src/qt/bantablemodel.h b/src/qt/bantablemodel.h index 57f559fc14..4b5b38e43f 100644 --- a/src/qt/bantablemodel.h +++ b/src/qt/bantablemodel.h @@ -5,6 +5,7 @@ #ifndef BITCOIN_QT_BANTABLEMODEL_H #define BITCOIN_QT_BANTABLEMODEL_H +#include <addrdb.h> #include <net.h> #include <memory> diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp index 55eba60bcd..862bdd3bfe 100644 --- a/src/qt/bitcoingui.cpp +++ b/src/qt/bitcoingui.cpp @@ -676,8 +676,8 @@ void BitcoinGUI::addWallet(WalletModel* walletModel) { if (!walletFrame) return; - WalletView* wallet_view = new WalletView(platformStyle, walletFrame); - if (!walletFrame->addWallet(walletModel, wallet_view)) return; + WalletView* wallet_view = new WalletView(walletModel, platformStyle, walletFrame); + if (!walletFrame->addView(wallet_view)) return; rpcConsole->addWallet(walletModel); if (m_wallet_selector->count() == 0) { diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp index 022f367422..f4d561286e 100644 --- a/src/qt/test/addressbooktests.cpp +++ b/src/qt/test/addressbooktests.cpp @@ -112,7 +112,7 @@ void TestAddAddressesToSendBook(interfaces::Node& node) WalletContext& context = *node.walletClient().context(); AddWallet(context, wallet); WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get()); - RemoveWallet(context, wallet, /* load_on_startup= */ std::nullopt); + RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt); EditAddressDialog editAddressDialog(EditAddressDialog::NewSendingAddress); editAddressDialog.setModel(walletModel.getAddressTableModel()); diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 1976bee74b..89f2258c0d 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -167,7 +167,7 @@ void TestGUI(interfaces::Node& node) WalletContext& context = *node.walletClient().context(); AddWallet(context, wallet); WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get()); - RemoveWallet(context, wallet, /* load_on_startup= */ std::nullopt); + RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt); sendCoinsDialog.setModel(&walletModel); transactionView.setModel(&walletModel); diff --git a/src/qt/walletframe.cpp b/src/qt/walletframe.cpp index 30c29eb356..5eeb2d5308 100644 --- a/src/qt/walletframe.cpp +++ b/src/qt/walletframe.cpp @@ -11,6 +11,7 @@ #include <qt/psbtoperationsdialog.h> #include <qt/walletmodel.h> #include <qt/walletview.h> +#include <util/system.h> #include <cassert> @@ -64,14 +65,13 @@ void WalletFrame::setClientModel(ClientModel *_clientModel) } } -bool WalletFrame::addWallet(WalletModel* walletModel, WalletView* walletView) +bool WalletFrame::addView(WalletView* walletView) { - if (!clientModel || !walletModel) return false; + if (!clientModel) return false; - if (mapWalletViews.count(walletModel) > 0) return false; + if (mapWalletViews.count(walletView->getWalletModel()) > 0) return false; walletView->setClientModel(clientModel); - walletView->setWalletModel(walletModel); walletView->showOutOfSyncWarning(bOutOfSync); WalletView* current_wallet_view = currentWalletView(); @@ -82,7 +82,7 @@ bool WalletFrame::addWallet(WalletModel* walletModel, WalletView* walletView) } walletStack->addWidget(walletView); - mapWalletViews[walletModel] = walletView; + mapWalletViews[walletView->getWalletModel()] = walletView; return true; } diff --git a/src/qt/walletframe.h b/src/qt/walletframe.h index cbf6af95ec..cfca5c4c5c 100644 --- a/src/qt/walletframe.h +++ b/src/qt/walletframe.h @@ -35,7 +35,7 @@ public: void setClientModel(ClientModel *clientModel); - bool addWallet(WalletModel* walletModel, WalletView* walletView); + bool addView(WalletView* walletView); void setCurrentWallet(WalletModel* wallet_model); void removeWallet(WalletModel* wallet_model); void removeAllWallets(); diff --git a/src/qt/walletview.cpp b/src/qt/walletview.cpp index 7e96e85c0c..309806a1c4 100644 --- a/src/qt/walletview.cpp +++ b/src/qt/walletview.cpp @@ -30,19 +30,24 @@ #include <QPushButton> #include <QVBoxLayout> -WalletView::WalletView(const PlatformStyle *_platformStyle, QWidget *parent): - QStackedWidget(parent), - clientModel(nullptr), - walletModel(nullptr), - platformStyle(_platformStyle) +WalletView::WalletView(WalletModel* wallet_model, const PlatformStyle* _platformStyle, QWidget* parent) + : QStackedWidget(parent), + clientModel(nullptr), + walletModel(wallet_model), + platformStyle(_platformStyle) { + assert(walletModel); + // Create tabs overviewPage = new OverviewPage(platformStyle); + overviewPage->setWalletModel(walletModel); transactionsPage = new QWidget(this); QVBoxLayout *vbox = new QVBoxLayout(); QHBoxLayout *hbox_buttons = new QHBoxLayout(); transactionView = new TransactionView(platformStyle, this); + transactionView->setModel(walletModel); + vbox->addWidget(transactionView); QPushButton *exportButton = new QPushButton(tr("&Export"), this); exportButton->setToolTip(tr("Export the data in the current tab to a file")); @@ -55,10 +60,16 @@ WalletView::WalletView(const PlatformStyle *_platformStyle, QWidget *parent): transactionsPage->setLayout(vbox); receiveCoinsPage = new ReceiveCoinsDialog(platformStyle); + receiveCoinsPage->setModel(walletModel); + sendCoinsPage = new SendCoinsDialog(platformStyle); + sendCoinsPage->setModel(walletModel); usedSendingAddressesPage = new AddressBookPage(platformStyle, AddressBookPage::ForEditing, AddressBookPage::SendingTab, this); + usedSendingAddressesPage->setModel(walletModel->getAddressTableModel()); + usedReceivingAddressesPage = new AddressBookPage(platformStyle, AddressBookPage::ForEditing, AddressBookPage::ReceivingTab, this); + usedReceivingAddressesPage->setModel(walletModel->getAddressTableModel()); addWidget(overviewPage); addWidget(transactionsPage); @@ -84,6 +95,21 @@ WalletView::WalletView(const PlatformStyle *_platformStyle, QWidget *parent): connect(transactionView, &TransactionView::message, this, &WalletView::message); connect(this, &WalletView::setPrivacy, overviewPage, &OverviewPage::setPrivacy); + + // Receive and pass through messages from wallet model + connect(walletModel, &WalletModel::message, this, &WalletView::message); + + // Handle changes in encryption status + connect(walletModel, &WalletModel::encryptionStatusChanged, this, &WalletView::encryptionStatusChanged); + + // Balloon pop-up for new transaction + connect(walletModel->getTransactionTableModel(), &TransactionTableModel::rowsInserted, this, &WalletView::processNewTransaction); + + // Ask for passphrase if needed + connect(walletModel, &WalletModel::requireUnlock, this, &WalletView::unlockWallet); + + // Show progress dialog + connect(walletModel, &WalletModel::showProgress, this, &WalletView::showProgress); } WalletView::~WalletView() @@ -96,45 +122,15 @@ void WalletView::setClientModel(ClientModel *_clientModel) overviewPage->setClientModel(_clientModel); sendCoinsPage->setClientModel(_clientModel); - if (walletModel) walletModel->setClientModel(_clientModel); -} - -void WalletView::setWalletModel(WalletModel *_walletModel) -{ - this->walletModel = _walletModel; - - // Put transaction list in tabs - transactionView->setModel(_walletModel); - overviewPage->setWalletModel(_walletModel); - receiveCoinsPage->setModel(_walletModel); - sendCoinsPage->setModel(_walletModel); - usedReceivingAddressesPage->setModel(_walletModel ? _walletModel->getAddressTableModel() : nullptr); - usedSendingAddressesPage->setModel(_walletModel ? _walletModel->getAddressTableModel() : nullptr); - - if (_walletModel) - { - // Receive and pass through messages from wallet model - connect(_walletModel, &WalletModel::message, this, &WalletView::message); - - // Handle changes in encryption status - connect(_walletModel, &WalletModel::encryptionStatusChanged, this, &WalletView::encryptionStatusChanged); - - // Balloon pop-up for new transaction - connect(_walletModel->getTransactionTableModel(), &TransactionTableModel::rowsInserted, this, &WalletView::processNewTransaction); - - // Ask for passphrase if needed - connect(_walletModel, &WalletModel::requireUnlock, this, &WalletView::unlockWallet); - - // Show progress dialog - connect(_walletModel, &WalletModel::showProgress, this, &WalletView::showProgress); - } + walletModel->setClientModel(_clientModel); } void WalletView::processNewTransaction(const QModelIndex& parent, int start, int /*end*/) { // Prevent balloon-spam when initial block download is in progress - if (!walletModel || !clientModel || clientModel->node().isInitialBlockDownload()) + if (!clientModel || clientModel->node().isInitialBlockDownload()) { return; + } TransactionTableModel *ttm = walletModel->getTransactionTableModel(); if (!ttm || ttm->processingQueuedTransactions()) @@ -209,8 +205,6 @@ void WalletView::showOutOfSyncWarning(bool fShow) void WalletView::encryptWallet() { - if(!walletModel) - return; AskPassphraseDialog dlg(AskPassphraseDialog::Encrypt, this); dlg.setModel(walletModel); dlg.exec(); @@ -247,8 +241,6 @@ void WalletView::changePassphrase() void WalletView::unlockWallet() { - if(!walletModel) - return; // Unlock wallet when requested by wallet model if (walletModel->getEncryptionStatus() == WalletModel::Locked) { @@ -260,17 +252,11 @@ void WalletView::unlockWallet() void WalletView::usedSendingAddresses() { - if(!walletModel) - return; - GUIUtil::bringToFront(usedSendingAddressesPage); } void WalletView::usedReceivingAddresses() { - if(!walletModel) - return; - GUIUtil::bringToFront(usedReceivingAddressesPage); } diff --git a/src/qt/walletview.h b/src/qt/walletview.h index bb6ad0f69e..eebc163624 100644 --- a/src/qt/walletview.h +++ b/src/qt/walletview.h @@ -35,19 +35,14 @@ class WalletView : public QStackedWidget Q_OBJECT public: - explicit WalletView(const PlatformStyle *platformStyle, QWidget *parent); + explicit WalletView(WalletModel* wallet_model, const PlatformStyle* platformStyle, QWidget* parent); ~WalletView(); /** Set the client model. The client model represents the part of the core that communicates with the P2P network, and is wallet-agnostic. */ void setClientModel(ClientModel *clientModel); - WalletModel *getWalletModel() { return walletModel; } - /** Set the wallet model. - The wallet model represents a bitcoin wallet, and offers access to the list of transactions, address book and sending - functionality. - */ - void setWalletModel(WalletModel *walletModel); + WalletModel* getWalletModel() const noexcept { return walletModel; } bool handlePaymentRequest(const SendCoinsRecipient& recipient); @@ -55,7 +50,12 @@ public: private: ClientModel *clientModel; - WalletModel *walletModel; + + //! + //! The wallet model represents a bitcoin wallet, and offers access to + //! the list of transactions, address book and sending functionality. + //! + WalletModel* const walletModel; OverviewPage *overviewPage; QWidget *transactionsPage; diff --git a/src/signet.cpp b/src/signet.cpp index 1ba8502287..aafd1999ee 100644 --- a/src/signet.cpp +++ b/src/signet.cpp @@ -141,7 +141,7 @@ bool CheckSignetBlockSolution(const CBlock& block, const Consensus::Params& cons PrecomputedTransactionData txdata; txdata.Init(signet_txs->m_to_sign, {signet_txs->m_to_spend.vout[0]}); - TransactionSignatureChecker sigcheck(&signet_txs->m_to_sign, /*nIn=*/ 0, /*amount=*/ signet_txs->m_to_spend.vout[0].nValue, txdata, MissingDataBehavior::ASSERT_FAIL); + TransactionSignatureChecker sigcheck(&signet_txs->m_to_sign, /* nInIn= */ 0, /* amountIn= */ signet_txs->m_to_spend.vout[0].nValue, txdata, MissingDataBehavior::ASSERT_FAIL); if (!VerifyScript(scriptSig, signet_txs->m_to_spend.vout[0].scriptPubKey, &witness, BLOCK_SCRIPT_VERIFY_FLAGS, sigcheck)) { LogPrint(BCLog::VALIDATION, "CheckSignetBlockSolution: Errors in block (block solution invalid)\n"); diff --git a/src/sync.cpp b/src/sync.cpp index a2b62c2286..98e6d3d65d 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -23,17 +23,6 @@ #include <utility> #include <vector> -#ifdef DEBUG_LOCKCONTENTION -#if !defined(HAVE_THREAD_LOCAL) -static_assert(false, "thread_local is not supported"); -#endif -void PrintLockContention(const char* pszName, const char* pszFile, int nLine) -{ - LogPrintf("LOCKCONTENTION: %s\n", pszName); - LogPrintf("Locker: %s:%d\n", pszFile, nLine); -} -#endif /* DEBUG_LOCKCONTENTION */ - #ifdef DEBUG_LOCKORDER // // Early deadlock detection. diff --git a/src/sync.h b/src/sync.h index 146c228592..6ba63d5e4d 100644 --- a/src/sync.h +++ b/src/sync.h @@ -6,6 +6,8 @@ #ifndef BITCOIN_SYNC_H #define BITCOIN_SYNC_H +#include <logging.h> +#include <logging/timer.h> #include <threadsafety.h> #include <util/macros.h> @@ -126,10 +128,6 @@ using RecursiveMutex = AnnotatedMixin<std::recursive_mutex>; /** Wrapped mutex: supports waiting but not recursive locking */ typedef AnnotatedMixin<std::mutex> Mutex; -#ifdef DEBUG_LOCKCONTENTION -void PrintLockContention(const char* pszName, const char* pszFile, int nLine); -#endif - /** Wrapper around std::unique_lock style lock for Mutex. */ template <typename Mutex, typename Base = typename Mutex::UniqueLock> class SCOPED_LOCKABLE UniqueLock : public Base @@ -138,22 +136,18 @@ private: void Enter(const char* pszName, const char* pszFile, int nLine) { EnterCritical(pszName, pszFile, nLine, Base::mutex()); -#ifdef DEBUG_LOCKCONTENTION - if (!Base::try_lock()) { - PrintLockContention(pszName, pszFile, nLine); -#endif - Base::lock(); -#ifdef DEBUG_LOCKCONTENTION - } -#endif + if (Base::try_lock()) return; + LOG_TIME_MICROS_WITH_CATEGORY(strprintf("lock contention %s, %s:%d", pszName, pszFile, nLine), BCLog::LOCK); + Base::lock(); } bool TryEnter(const char* pszName, const char* pszFile, int nLine) { EnterCritical(pszName, pszFile, nLine, Base::mutex(), true); Base::try_lock(); - if (!Base::owns_lock()) + if (!Base::owns_lock()) { LeaveCritical(); + } return Base::owns_lock(); } diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index cd5dc2370f..01a492a20b 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -5,13 +5,14 @@ #include <addrdb.h> #include <addrman.h> #include <chainparams.h> +#include <clientversion.h> +#include <hash.h> +#include <netbase.h> +#include <random.h> #include <test/data/asmap.raw.h> #include <test/util/setup_common.h> #include <util/asmap.h> #include <util/string.h> -#include <hash.h> -#include <netbase.h> -#include <random.h> #include <boost/test/unit_test.hpp> @@ -26,7 +27,7 @@ public: virtual void Serialize(CDataStream& s) const = 0; CAddrManSerializationMock() - : CAddrMan(/* deterministic */ true, /* consistency_check_ratio */ 100) + : CAddrMan(/* asmap */ std::vector<bool>(), /* deterministic */ true, /* consistency_check_ratio */ 100) {} }; @@ -82,10 +83,9 @@ private: public: explicit CAddrManTest(bool makeDeterministic = true, std::vector<bool> asmap = std::vector<bool>()) - : CAddrMan(makeDeterministic, /* consistency_check_ratio */ 100) + : CAddrMan(asmap, makeDeterministic, /* consistency_check_ratio */ 100) { deterministic = makeDeterministic; - m_asmap = asmap; } CAddrInfo* Find(const CNetAddr& addr, int* pnId = nullptr) @@ -1003,7 +1003,7 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks) BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); } -BOOST_AUTO_TEST_CASE(caddrdb_read) +BOOST_AUTO_TEST_CASE(load_addrman) { CAddrManUncorrupted addrmanUncorrupted; @@ -1024,7 +1024,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read) // Test that the de-serialization does not throw an exception. CDataStream ssPeers1 = AddrmanToStream(addrmanUncorrupted); bool exceptionThrown = false; - CAddrMan addrman1(/* deterministic */ false, /* consistency_check_ratio */ 100); + CAddrMan addrman1(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman1.size() == 0); try { @@ -1038,24 +1038,24 @@ BOOST_AUTO_TEST_CASE(caddrdb_read) BOOST_CHECK(addrman1.size() == 3); BOOST_CHECK(exceptionThrown == false); - // Test that CAddrDB::Read creates an addrman with the correct number of addrs. + // Test that ReadFromStream creates an addrman with the correct number of addrs. CDataStream ssPeers2 = AddrmanToStream(addrmanUncorrupted); - CAddrMan addrman2(/* deterministic */ false, /* consistency_check_ratio */ 100); + CAddrMan addrman2(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman2.size() == 0); - BOOST_CHECK(CAddrDB::Read(addrman2, ssPeers2)); + ReadFromStream(addrman2, ssPeers2); BOOST_CHECK(addrman2.size() == 3); } -BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted) +BOOST_AUTO_TEST_CASE(load_addrman_corrupted) { CAddrManCorrupted addrmanCorrupted; // Test that the de-serialization of corrupted addrman throws an exception. CDataStream ssPeers1 = AddrmanToStream(addrmanCorrupted); bool exceptionThrown = false; - CAddrMan addrman1(/* deterministic */ false, /* consistency_check_ratio */ 100); + CAddrMan addrman1(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman1.size() == 0); try { unsigned char pchMsgTmp[4]; @@ -1064,16 +1064,16 @@ BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted) } catch (const std::exception&) { exceptionThrown = true; } - // Even through de-serialization failed addrman is not left in a clean state. + // Even though de-serialization failed addrman is not left in a clean state. BOOST_CHECK(addrman1.size() == 1); BOOST_CHECK(exceptionThrown); - // Test that CAddrDB::Read fails if peers.dat is corrupt + // Test that ReadFromStream fails if peers.dat is corrupt CDataStream ssPeers2 = AddrmanToStream(addrmanCorrupted); - CAddrMan addrman2(/* deterministic */ false, /* consistency_check_ratio */ 100); + CAddrMan addrman2(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 100); BOOST_CHECK(addrman2.size() == 0); - BOOST_CHECK(!CAddrDB::Read(addrman2, ssPeers2)); + BOOST_CHECK_THROW(ReadFromStream(addrman2, ssPeers2), std::ios_base::failure); } diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp index 95aa53bff4..fdbfb3b93b 100644 --- a/src/test/fuzz/addrman.cpp +++ b/src/test/fuzz/addrman.cpp @@ -28,17 +28,11 @@ class CAddrManDeterministic : public CAddrMan public: FuzzedDataProvider& m_fuzzed_data_provider; - explicit CAddrManDeterministic(FuzzedDataProvider& fuzzed_data_provider) - : CAddrMan(/* deterministic */ true, /* consistency_check_ratio */ 0) + explicit CAddrManDeterministic(std::vector<bool> asmap, FuzzedDataProvider& fuzzed_data_provider) + : CAddrMan(std::move(asmap), /* deterministic */ true, /* consistency_check_ratio */ 0) , m_fuzzed_data_provider(fuzzed_data_provider) { WITH_LOCK(cs, insecure_rand = FastRandomContext{ConsumeUInt256(fuzzed_data_provider)}); - if (fuzzed_data_provider.ConsumeBool()) { - m_asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider); - if (!SanityCheckASMap(m_asmap)) { - m_asmap.clear(); - } - } } /** @@ -224,11 +218,19 @@ public: } }; +[[nodiscard]] inline std::vector<bool> ConsumeAsmap(FuzzedDataProvider& fuzzed_data_provider) noexcept +{ + std::vector<bool> asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider); + if (!SanityCheckASMap(asmap, 128)) asmap.clear(); + return asmap; +} + FUZZ_TARGET_INIT(addrman, initialize_addrman) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); SetMockTime(ConsumeTime(fuzzed_data_provider)); - auto addr_man_ptr = std::make_unique<CAddrManDeterministic>(fuzzed_data_provider); + std::vector<bool> asmap = ConsumeAsmap(fuzzed_data_provider); + auto addr_man_ptr = std::make_unique<CAddrManDeterministic>(asmap, fuzzed_data_provider); if (fuzzed_data_provider.ConsumeBool()) { const std::vector<uint8_t> serialized_data{ConsumeRandomLengthByteVector(fuzzed_data_provider)}; CDataStream ds(serialized_data, SER_DISK, INIT_PROTO_VERSION); @@ -237,7 +239,7 @@ FUZZ_TARGET_INIT(addrman, initialize_addrman) try { ds >> *addr_man_ptr; } catch (const std::ios_base::failure&) { - addr_man_ptr = std::make_unique<CAddrManDeterministic>(fuzzed_data_provider); + addr_man_ptr = std::make_unique<CAddrManDeterministic>(asmap, fuzzed_data_provider); } } CAddrManDeterministic& addr_man = *addr_man_ptr; @@ -306,9 +308,9 @@ FUZZ_TARGET_INIT(addrman_serdeser, initialize_addrman) FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); SetMockTime(ConsumeTime(fuzzed_data_provider)); - CAddrManDeterministic addr_man1{fuzzed_data_provider}; - CAddrManDeterministic addr_man2{fuzzed_data_provider}; - addr_man2.m_asmap = addr_man1.m_asmap; + std::vector<bool> asmap = ConsumeAsmap(fuzzed_data_provider); + CAddrManDeterministic addr_man1{asmap, fuzzed_data_provider}; + CAddrManDeterministic addr_man2{asmap, fuzzed_data_provider}; CDataStream data_stream(SER_NETWORK, PROTOCOL_VERSION); diff --git a/src/test/fuzz/asmap.cpp b/src/test/fuzz/asmap.cpp index 4c5bc0cbf2..d402f8632c 100644 --- a/src/test/fuzz/asmap.cpp +++ b/src/test/fuzz/asmap.cpp @@ -4,6 +4,7 @@ #include <netaddress.h> #include <test/fuzz/fuzz.h> +#include <util/asmap.h> #include <cstdint> #include <vector> @@ -42,7 +43,7 @@ FUZZ_TARGET(asmap) asmap.push_back((buffer[1 + i] >> j) & 1); } } - if (!SanityCheckASMap(asmap)) return; + if (!SanityCheckASMap(asmap, 128)) return; const uint8_t* addr_data = buffer.data() + 1 + asmap_size; CNetAddr net_addr; diff --git a/src/test/fuzz/connman.cpp b/src/test/fuzz/connman.cpp index 0e323ddc20..01741103e4 100644 --- a/src/test/fuzz/connman.cpp +++ b/src/test/fuzz/connman.cpp @@ -25,7 +25,7 @@ FUZZ_TARGET_INIT(connman, initialize_connman) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; SetMockTime(ConsumeTime(fuzzed_data_provider)); - CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0); + CAddrMan addrman(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0); CConnman connman{fuzzed_data_provider.ConsumeIntegral<uint64_t>(), fuzzed_data_provider.ConsumeIntegral<uint64_t>(), addrman, fuzzed_data_provider.ConsumeBool()}; CNetAddr random_netaddr; CNode random_node = ConsumeNode(fuzzed_data_provider); @@ -104,12 +104,6 @@ FUZZ_TARGET_INIT(connman, initialize_connman) connman.RemoveAddedNode(random_string); }, [&] { - const std::vector<bool> asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider); - if (SanityCheckASMap(asmap)) { - connman.SetAsmap(asmap); - } - }, - [&] { connman.SetNetworkActive(fuzzed_data_provider.ConsumeBool()); }, [&] { diff --git a/src/test/fuzz/data_stream.cpp b/src/test/fuzz/data_stream.cpp index 53400082ab..323090e041 100644 --- a/src/test/fuzz/data_stream.cpp +++ b/src/test/fuzz/data_stream.cpp @@ -2,6 +2,7 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include <addrdb.h> #include <addrman.h> #include <net.h> #include <test/fuzz/FuzzedDataProvider.h> @@ -21,6 +22,9 @@ FUZZ_TARGET_INIT(data_stream_addr_man, initialize_data_stream_addr_man) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; CDataStream data_stream = ConsumeDataStream(fuzzed_data_provider); - CAddrMan addr_man(/* deterministic */ false, /* consistency_check_ratio */ 0); - CAddrDB::Read(addr_man, data_stream); + CAddrMan addr_man(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0); + try { + ReadFromStream(addr_man, data_stream); + } catch (const std::exception&) { + } } diff --git a/src/test/fuzz/deserialize.cpp b/src/test/fuzz/deserialize.cpp index cfbbe77311..83ae1680e3 100644 --- a/src/test/fuzz/deserialize.cpp +++ b/src/test/fuzz/deserialize.cpp @@ -188,7 +188,7 @@ FUZZ_TARGET_DESERIALIZE(blockmerkleroot, { BlockMerkleRoot(block, &mutated); }) FUZZ_TARGET_DESERIALIZE(addrman_deserialize, { - CAddrMan am(/* deterministic */ false, /* consistency_check_ratio */ 0); + CAddrMan am(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0); DeserializeFromFuzzingInput(buffer, am); }) FUZZ_TARGET_DESERIALIZE(blockheader_deserialize, { diff --git a/src/test/fuzz/integer.cpp b/src/test/fuzz/integer.cpp index 5d26529837..5a732aeeff 100644 --- a/src/test/fuzz/integer.cpp +++ b/src/test/fuzz/integer.cpp @@ -23,6 +23,7 @@ #include <test/fuzz/fuzz.h> #include <test/fuzz/util.h> #include <uint256.h> +#include <univalue.h> #include <util/check.h> #include <util/moneystr.h> #include <util/strencodings.h> diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp index 9a579c053f..bd1bb79d0e 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -14,6 +14,7 @@ #include <test/fuzz/util.h> #include <test/util/net.h> #include <test/util/setup_common.h> +#include <util/asmap.h> #include <cstdint> #include <optional> @@ -38,12 +39,8 @@ FUZZ_TARGET_INIT(net, initialize_net) node.CloseSocketDisconnect(); }, [&] { - const std::vector<bool> asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider); - if (!SanityCheckASMap(asmap)) { - return; - } CNodeStats stats; - node.copyStats(stats, asmap); + node.CopyStats(stats); }, [&] { const CNode* add_ref_node = node.AddRef(); diff --git a/src/test/fuzz/versionbits.cpp b/src/test/fuzz/versionbits.cpp index 9186821836..73a7d24971 100644 --- a/src/test/fuzz/versionbits.cpp +++ b/src/test/fuzz/versionbits.cpp @@ -6,6 +6,7 @@ #include <chainparams.h> #include <consensus/params.h> #include <primitives/block.h> +#include <util/system.h> #include <versionbits.h> #include <test/fuzz/FuzzedDataProvider.h> diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp index e99c6e0fc8..84ddbc50c6 100644 --- a/src/test/logging_tests.cpp +++ b/src/test/logging_tests.cpp @@ -15,9 +15,9 @@ BOOST_FIXTURE_TEST_SUITE(logging_tests, BasicTestingSetup) BOOST_AUTO_TEST_CASE(logging_timer) { SetMockTime(1); - auto sec_timer = BCLog::Timer<std::chrono::seconds>("tests", "end_msg"); + auto micro_timer = BCLog::Timer<std::chrono::microseconds>("tests", "end_msg"); SetMockTime(2); - BOOST_CHECK_EQUAL(sec_timer.LogMsg("test secs"), "tests: test secs (1.00s)"); + BOOST_CHECK_EQUAL(micro_timer.LogMsg("test micros"), "tests: test micros (1000000μs)"); SetMockTime(1); auto ms_timer = BCLog::Timer<std::chrono::milliseconds>("tests", "end_msg"); @@ -25,9 +25,9 @@ BOOST_AUTO_TEST_CASE(logging_timer) BOOST_CHECK_EQUAL(ms_timer.LogMsg("test ms"), "tests: test ms (1000.00ms)"); SetMockTime(1); - auto micro_timer = BCLog::Timer<std::chrono::microseconds>("tests", "end_msg"); + auto sec_timer = BCLog::Timer<std::chrono::seconds>("tests", "end_msg"); SetMockTime(2); - BOOST_CHECK_EQUAL(micro_timer.LogMsg("test micros"), "tests: test micros (1000000.00μs)"); + BOOST_CHECK_EQUAL(sec_timer.LogMsg("test secs"), "tests: test secs (1.00s)"); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index c9bb863a7b..ba6b3e32ea 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -193,7 +193,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", state.ToString())); } - m_node.addrman = std::make_unique<CAddrMan>(/* deterministic */ false, /* consistency_check_ratio */ 0); + m_node.addrman = std::make_unique<CAddrMan>(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 0); m_node.banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME); m_node.connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman); // Deterministic randomness for tests. m_node.peerman = PeerManager::make(chainparams, *m_node.connman, *m_node.addrman, diff --git a/src/util/asmap.cpp b/src/util/asmap.cpp index bacc3690a2..5695c62012 100644 --- a/src/util/asmap.cpp +++ b/src/util/asmap.cpp @@ -2,10 +2,16 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include <util/asmap.h> + +#include <clientversion.h> +#include <crypto/common.h> +#include <logging.h> +#include <streams.h> + +#include <cassert> #include <map> #include <vector> -#include <assert.h> -#include <crypto/common.h> namespace { @@ -183,3 +189,31 @@ bool SanityCheckASMap(const std::vector<bool>& asmap, int bits) } return false; // Reached EOF without RETURN instruction } + +std::vector<bool> DecodeAsmap(fs::path path) +{ + std::vector<bool> bits; + FILE *filestr = fsbridge::fopen(path, "rb"); + CAutoFile file(filestr, SER_DISK, CLIENT_VERSION); + if (file.IsNull()) { + LogPrintf("Failed to open asmap file from disk\n"); + return bits; + } + fseek(filestr, 0, SEEK_END); + int length = ftell(filestr); + LogPrintf("Opened asmap file %s (%d bytes) from disk\n", path, length); + fseek(filestr, 0, SEEK_SET); + uint8_t cur_byte; + for (int i = 0; i < length; ++i) { + file >> cur_byte; + for (int bit = 0; bit < 8; ++bit) { + bits.push_back((cur_byte >> bit) & 1); + } + } + if (!SanityCheckASMap(bits, 128)) { + LogPrintf("Sanity check of asmap file %s failed\n", path); + return {}; + } + return bits; +} + diff --git a/src/util/asmap.h b/src/util/asmap.h index d0588bc8c3..810d70b9a1 100644 --- a/src/util/asmap.h +++ b/src/util/asmap.h @@ -5,11 +5,16 @@ #ifndef BITCOIN_UTIL_ASMAP_H #define BITCOIN_UTIL_ASMAP_H -#include <stdint.h> +#include <fs.h> + +#include <cstdint> #include <vector> uint32_t Interpret(const std::vector<bool> &asmap, const std::vector<bool> &ip); bool SanityCheckASMap(const std::vector<bool>& asmap, int bits); +/** Read asmap from provided binary file */ +std::vector<bool> DecodeAsmap(fs::path path); + #endif // BITCOIN_UTIL_ASMAP_H diff --git a/src/util/getuniquepath.cpp b/src/util/getuniquepath.cpp index 9839d2f624..6776e7785b 100644 --- a/src/util/getuniquepath.cpp +++ b/src/util/getuniquepath.cpp @@ -1,3 +1,7 @@ +// Copyright (c) 2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + #include <random.h> #include <fs.h> #include <util/strencodings.h> diff --git a/src/util/rbf.h b/src/util/rbf.h index 4eb44b904f..6d44a2cb83 100644 --- a/src/util/rbf.h +++ b/src/util/rbf.h @@ -11,15 +11,13 @@ class CTransaction; static const uint32_t MAX_BIP125_RBF_SEQUENCE = 0xfffffffd; -/** Check whether the sequence numbers on this transaction are signaling -* opt-in to replace-by-fee, according to BIP 125. -* Allow opt-out of transaction replacement by setting -* nSequence > MAX_BIP125_RBF_SEQUENCE (SEQUENCE_FINAL-2) on all inputs. +/** Check whether the sequence numbers on this transaction are signaling opt-in to replace-by-fee, + * according to BIP 125. Allow opt-out of transaction replacement by setting nSequence > + * MAX_BIP125_RBF_SEQUENCE (SEQUENCE_FINAL-2) on all inputs. * -* SEQUENCE_FINAL-1 is picked to still allow use of nLockTime by -* non-replaceable transactions. All inputs rather than just one -* is for the sake of multi-party protocols, where we don't -* want a single party to be able to disable replacement. */ +* SEQUENCE_FINAL-1 is picked to still allow use of nLockTime by non-replaceable transactions. All +* inputs rather than just one is for the sake of multi-party protocols, where we don't want a single +* party to be able to disable replacement. */ bool SignalsOptInRBF(const CTransaction &tx); #endif // BITCOIN_UTIL_RBF_H diff --git a/src/util/system.cpp b/src/util/system.cpp index 4e16a83c87..08f62f1da7 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -1306,7 +1306,7 @@ void SetupEnvironment() #endif // On most POSIX systems (e.g. Linux, but not BSD) the environment's locale // may be invalid, in which case the "C.UTF-8" locale is used as fallback. -#if !defined(WIN32) && !defined(MAC_OSX) && !defined(__FreeBSD__) && !defined(__OpenBSD__) +#if !defined(WIN32) && !defined(MAC_OSX) && !defined(__FreeBSD__) && !defined(__OpenBSD__) && !defined(__NetBSD__) try { std::locale(""); // Raises a runtime error if current locale is invalid } catch (const std::runtime_error&) { diff --git a/src/util/types.h b/src/util/types.h new file mode 100644 index 0000000000..0047b00026 --- /dev/null +++ b/src/util/types.h @@ -0,0 +1,11 @@ +// Copyright (c) 2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_UTIL_TYPES_H +#define BITCOIN_UTIL_TYPES_H + +template <class> +inline constexpr bool ALWAYS_FALSE{false}; + +#endif // BITCOIN_UTIL_TYPES_H diff --git a/src/validation.cpp b/src/validation.cpp index 753b824167..8696f1af85 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -415,7 +415,7 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationS } // Call CheckInputScripts() to cache signature and script validity against current tip consensus rules. - return CheckInputScripts(tx, state, view, flags, /* cacheSigStore = */ true, /* cacheFullSciptStore = */ true, txdata); + return CheckInputScripts(tx, state, view, flags, /* cacheSigStore= */ true, /* cacheFullScriptStore= */ true, txdata); } namespace { @@ -770,16 +770,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // that we have the set of all ancestors we can detect this // pathological case by making sure setConflicts and setAncestors don't // intersect. - for (CTxMemPool::txiter ancestorIt : setAncestors) - { - const uint256 &hashAncestor = ancestorIt->GetTx().GetHash(); - if (setConflicts.count(hashAncestor)) - { - return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-spends-conflicting-tx", - strprintf("%s spends conflicting transaction %s", - hash.ToString(), - hashAncestor.ToString())); - } + if (const auto err_string{EntriesAndTxidsDisjoint(setAncestors, setConflicts, hash)}) { + return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-spends-conflicting-tx", *err_string); } @@ -789,98 +781,30 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) fReplacementTransaction = setConflicts.size(); if (fReplacementTransaction) { - std::string err_string; CFeeRate newFeeRate(nModifiedFees, nSize); - for (const auto& mi : setIterConflicting) { - // Don't allow the replacement to reduce the feerate of the - // mempool. - // - // We usually don't want to accept replacements with lower - // feerates than what they replaced as that would lower the - // feerate of the next block. Requiring that the feerate always - // be increased is also an easy-to-reason about way to prevent - // DoS attacks via replacements. - // - // We only consider the feerates of transactions being directly - // replaced, not their indirect descendants. While that does - // mean high feerate children are ignored when deciding whether - // or not to replace, we do require the replacement to pay more - // overall fees too, mitigating most cases. - CFeeRate oldFeeRate(mi->GetModifiedFee(), mi->GetTxSize()); - if (newFeeRate <= oldFeeRate) - { - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", - strprintf("rejecting replacement %s; new feerate %s <= old feerate %s", - hash.ToString(), - newFeeRate.ToString(), - oldFeeRate.ToString())); - } + if (const auto err_string{PaysMoreThanConflicts(setIterConflicting, newFeeRate, hash)}) { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string); } // Calculate all conflicting entries and enforce Rule #5. - if (!GetEntriesForConflicts(tx, m_pool, setIterConflicting, allConflicting, err_string)) { - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too many potential replacements", err_string); + if (const auto err_string{GetEntriesForConflicts(tx, m_pool, setIterConflicting, allConflicting)}) { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, + "too many potential replacements", *err_string); + } + // Enforce Rule #2. + if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, setIterConflicting)}) { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, + "replacement-adds-unconfirmed", *err_string); } // Check if it's economically rational to mine this transaction rather - // than the ones it replaces. + // than the ones it replaces. Enforce Rules #3 and #4. for (CTxMemPool::txiter it : allConflicting) { nConflictingFees += it->GetModifiedFee(); nConflictingSize += it->GetTxSize(); } - - std::set<uint256> setConflictsParents; - for (const auto& mi : setIterConflicting) { - for (const CTxIn &txin : mi->GetTx().vin) - { - setConflictsParents.insert(txin.prevout.hash); - } - } - - for (unsigned int j = 0; j < tx.vin.size(); j++) - { - // We don't want to accept replacements that require low - // feerate junk to be mined first. Ideally we'd keep track of - // the ancestor feerates and make the decision based on that, - // but for now requiring all new inputs to be confirmed works. - // - // Note that if you relax this to make RBF a little more useful, - // this may break the CalculateMempoolAncestors RBF relaxation, - // above. See the comment above the first CalculateMempoolAncestors - // call for more info. - if (!setConflictsParents.count(tx.vin[j].prevout.hash)) - { - // Rather than check the UTXO set - potentially expensive - - // it's cheaper to just check if the new input refers to a - // tx that's in the mempool. - if (m_pool.exists(tx.vin[j].prevout.hash)) { - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "replacement-adds-unconfirmed", - strprintf("replacement %s adds unconfirmed input, idx %d", - hash.ToString(), j)); - } - } - } - - // The replacement must pay greater fees than the transactions it - // replaces - if we did the bandwidth used by those conflicting - // transactions would not be paid for. - if (nModifiedFees < nConflictingFees) - { - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", - strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s", - hash.ToString(), FormatMoney(nModifiedFees), FormatMoney(nConflictingFees))); - } - - // Finally in addition to paying more fees than the conflicts the - // new transaction must pay for its own bandwidth. - CAmount nDeltaFees = nModifiedFees - nConflictingFees; - if (nDeltaFees < ::incrementalRelayFee.GetFee(nSize)) - { - return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", - strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s", - hash.ToString(), - FormatMoney(nDeltaFees), - FormatMoney(::incrementalRelayFee.GetFee(nSize)))); + if (const auto err_string{PaysForRBF(nConflictingFees, nModifiedFees, nSize, hash)}) { + return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string); } } return true; diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index a994976394..1b841026b8 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -165,7 +165,7 @@ void UnloadWallets(WalletContext& context) auto wallet = wallets.back(); wallets.pop_back(); std::vector<bilingual_str> warnings; - RemoveWallet(context, wallet, /* load_on_startup= */ std::nullopt, warnings); + RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt, warnings); UnloadWallet(std::move(wallet)); } } diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index f901679efc..5d51809241 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -40,7 +40,7 @@ CoinEligibilityFilter filter_standard_extra(6, 6, 0); CoinSelectionParams coin_selection_params(/* change_output_size= */ 0, /* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(0), /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), - /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); + /* tx_noinputs_size= */ 0, /* avoid_partial= */ false); static void add_coin(const CAmount& nValue, int nInput, std::vector<CInputCoin>& set) { @@ -287,7 +287,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) CoinSelectionParams coin_selection_params_bnb(/* change_output_size= */ 0, /* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(3000), /* long_term_feerate= */ CFeeRate(1000), /* discard_feerate= */ CFeeRate(1000), - /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); + /* tx_noinputs_size= */ 0, /* avoid_partial= */ false); CoinSet setCoinsRet; CAmount nValueRet; empty_wallet(); @@ -654,7 +654,7 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test) CoinSelectionParams cs_params(/* change_output_size= */ 34, /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0), /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), - /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); + /* tx_noinputs_size= */ 0, /* avoid_partial= */ false); CoinSet out_set; CAmount out_value = 0; CCoinControl cc; diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 12a22f458a..5431a38bee 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -235,7 +235,7 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup) "downloading and rescanning the relevant blocks (see -reindex and -rescan " "options).\"}},{\"success\":true}]", 0, oldTip->GetBlockTimeMax(), TIMESTAMP_WINDOW)); - RemoveWallet(context, wallet, /* load_on_startup= */ std::nullopt); + RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt); } } @@ -280,7 +280,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) request.params.push_back(backup_file); ::dumpwallet().HandleRequest(request); - RemoveWallet(context, wallet, /* load_on_startup= */ std::nullopt); + RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt); } // Call importwallet RPC and verify all blocks with timestamps >= BLOCK_TIME @@ -299,7 +299,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) AddWallet(context, wallet); wallet->SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash()); ::importwallet().HandleRequest(request); - RemoveWallet(context, wallet, /* load_on_startup= */ std::nullopt); + RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt); BOOST_CHECK_EQUAL(wallet->mapWallet.size(), 3U); BOOST_CHECK_EQUAL(m_coinbase_txns.size(), 103U); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index f3af9885a2..70349b2455 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2191,7 +2191,7 @@ void CWallet::MarkDestinationsDirty(const std::set<CTxDestination>& destinations std::set<CTxDestination> CWallet::GetLabelAddresses(const std::string& label) const { - LOCK(cs_wallet); + AssertLockHeld(cs_wallet); std::set<CTxDestination> result; for (const std::pair<const CTxDestination, CAddressBookData>& item : m_address_book) { @@ -3252,12 +3252,13 @@ DescriptorScriptPubKeyMan* CWallet::GetDescriptorScriptPubKeyMan(const WalletDes ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const FlatSigningProvider& signing_provider, const std::string& label, bool internal) { + AssertLockHeld(cs_wallet); + if (!IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { WalletLogPrintf("Cannot add WalletDescriptor to a non-descriptor wallet\n"); return nullptr; } - LOCK(cs_wallet); auto spk_man = GetDescriptorScriptPubKeyMan(desc); if (spk_man) { WalletLogPrintf("Update existing descriptor: %s\n", desc.descriptor->ToString()); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index fbeec2aa30..2dc9eff712 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -630,7 +630,7 @@ public: int64_t GetOldestKeyPoolTime() const; - std::set<CTxDestination> GetLabelAddresses(const std::string& label) const; + std::set<CTxDestination> GetLabelAddresses(const std::string& label) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** * Marks all outputs in each one of the destinations dirty, so their cache is @@ -876,7 +876,7 @@ public: DescriptorScriptPubKeyMan* GetDescriptorScriptPubKeyMan(const WalletDescriptor& desc) const; //! Add a descriptor to the wallet, return a ScriptPubKeyMan & associated output type - ScriptPubKeyMan* AddWalletDescriptor(WalletDescriptor& desc, const FlatSigningProvider& signing_provider, const std::string& label, bool internal); + ScriptPubKeyMan* AddWalletDescriptor(WalletDescriptor& desc, const FlatSigningProvider& signing_provider, const std::string& label, bool internal) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); }; /** diff --git a/src/zmq/zmqpublishnotifier.cpp b/src/zmq/zmqpublishnotifier.cpp index 6ae866cc07..56f4c98317 100644 --- a/src/zmq/zmqpublishnotifier.cpp +++ b/src/zmq/zmqpublishnotifier.cpp @@ -6,6 +6,7 @@ #include <chain.h> #include <chainparams.h> +#include <netbase.h> #include <node/blockstorage.h> #include <rpc/server.h> #include <streams.h> @@ -73,6 +74,20 @@ static int zmq_send_multipart(void *sock, const void* data, size_t size, ...) return 0; } +static bool IsZMQAddressIPV6(const std::string &zmq_address) +{ + const std::string tcp_prefix = "tcp://"; + const size_t tcp_index = zmq_address.rfind(tcp_prefix); + const size_t colon_index = zmq_address.rfind(":"); + if (tcp_index == 0 && colon_index != std::string::npos) { + const std::string ip = zmq_address.substr(tcp_prefix.length(), colon_index - tcp_prefix.length()); + CNetAddr addr; + LookupHost(ip, addr, false); + if (addr.IsIPv6()) return true; + } + return false; +} + bool CZMQAbstractPublishNotifier::Initialize(void *pcontext) { assert(!psocket); @@ -107,6 +122,15 @@ bool CZMQAbstractPublishNotifier::Initialize(void *pcontext) return false; } + // On some systems (e.g. OpenBSD) the ZMQ_IPV6 must not be enabled, if the address to bind isn't IPv6 + const int enable_ipv6 { IsZMQAddressIPV6(address) ? 1 : 0}; + rc = zmq_setsockopt(psocket, ZMQ_IPV6, &enable_ipv6, sizeof(enable_ipv6)); + if (rc != 0) { + zmqError("Failed to set ZMQ_IPV6"); + zmq_close(psocket); + return false; + } + rc = zmq_bind(psocket, address.c_str()); if (rc != 0) { |