From d88c0d8440cf640ef4f2c7a40b8b8b31bfd38f23 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 18 Jan 2022 19:02:05 +0200 Subject: [PATCH 01/80] refactor: Get rid of `BanMan::BannedSetIsDirty()` --- src/banman.cpp | 8 +------- src/banman.h | 1 - 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/src/banman.cpp b/src/banman.cpp index 2a6e0e010f..50dc0750e1 100644 --- a/src/banman.cpp +++ b/src/banman.cpp @@ -53,7 +53,7 @@ void BanMan::DumpBanlist() { LOCK(m_cs_banned); SweepBanned(); - if (!BannedSetIsDirty()) return; + if (!m_is_dirty) return; banmap = m_banned; SetBannedSetDirty(false); } @@ -203,12 +203,6 @@ void BanMan::SweepBanned() } } -bool BanMan::BannedSetIsDirty() -{ - LOCK(m_cs_banned); - return m_is_dirty; -} - void BanMan::SetBannedSetDirty(bool dirty) { LOCK(m_cs_banned); //reuse m_banned lock for the m_is_dirty flag diff --git a/src/banman.h b/src/banman.h index 77b043f081..7f3c74733e 100644 --- a/src/banman.h +++ b/src/banman.h @@ -81,7 +81,6 @@ class BanMan private: void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_cs_banned); - bool BannedSetIsDirty(); //!set the "dirty" flag for the banlist void SetBannedSetDirty(bool dirty = true); //!clean unused entries (if bantime has expired) From 46709c5f27bf6cbc8eba1298b04bd079da2cdded Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 18 Jan 2022 19:18:02 +0200 Subject: [PATCH 02/80] refactor: Get rid of `BanMan::SetBannedSetDirty()` --- src/banman.cpp | 11 +++-------- src/banman.h | 2 -- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/src/banman.cpp b/src/banman.cpp index 50dc0750e1..5b2a179543 100644 --- a/src/banman.cpp +++ b/src/banman.cpp @@ -55,12 +55,13 @@ void BanMan::DumpBanlist() SweepBanned(); if (!m_is_dirty) return; banmap = m_banned; - SetBannedSetDirty(false); + m_is_dirty = false; } int64_t n_start = GetTimeMillis(); if (!m_ban_db.Write(banmap)) { - SetBannedSetDirty(true); + LOCK(m_cs_banned); + m_is_dirty = true; } LogPrint(BCLog::NET, "Flushed %d banned node addresses/subnets to disk %dms\n", banmap.size(), @@ -202,9 +203,3 @@ void BanMan::SweepBanned() m_client_interface->BannedListChanged(); } } - -void BanMan::SetBannedSetDirty(bool dirty) -{ - LOCK(m_cs_banned); //reuse m_banned lock for the m_is_dirty flag - m_is_dirty = dirty; -} diff --git a/src/banman.h b/src/banman.h index 7f3c74733e..7a032dfdd0 100644 --- a/src/banman.h +++ b/src/banman.h @@ -81,8 +81,6 @@ class BanMan private: void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_cs_banned); - //!set the "dirty" flag for the banlist - void SetBannedSetDirty(bool dirty = true); //!clean unused entries (if bantime has expired) void SweepBanned() EXCLUSIVE_LOCKS_REQUIRED(m_cs_banned); From 784c316f9cb664c9577cbfed1873bae573efd1b4 Mon Sep 17 00:00:00 2001 From: w0xlt <94266259+w0xlt@users.noreply.github.com> Date: Tue, 18 Jan 2022 03:26:37 -0300 Subject: [PATCH 03/80] scripted-diff: rename m_cs_banned -> m_banned_mutex -BEGIN VERIFY SCRIPT- s() { sed -i 's/m_cs_banned/m_banned_mutex/g' $1; } s src/banman.cpp s src/banman.h -END VERIFY SCRIPT- --- src/banman.cpp | 24 ++++++++++++------------ src/banman.h | 12 ++++++------ 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/banman.cpp b/src/banman.cpp index 5b2a179543..3286ca8965 100644 --- a/src/banman.cpp +++ b/src/banman.cpp @@ -27,7 +27,7 @@ BanMan::~BanMan() void BanMan::LoadBanlist() { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); if (m_client_interface) m_client_interface->InitMessage(_("Loading banlist…").translated); @@ -51,7 +51,7 @@ void BanMan::DumpBanlist() banmap_t banmap; { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); SweepBanned(); if (!m_is_dirty) return; banmap = m_banned; @@ -60,7 +60,7 @@ void BanMan::DumpBanlist() int64_t n_start = GetTimeMillis(); if (!m_ban_db.Write(banmap)) { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); m_is_dirty = true; } @@ -71,7 +71,7 @@ void BanMan::DumpBanlist() void BanMan::ClearBanned() { { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); m_banned.clear(); m_is_dirty = true; } @@ -81,14 +81,14 @@ void BanMan::ClearBanned() bool BanMan::IsDiscouraged(const CNetAddr& net_addr) { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); return m_discouraged.contains(net_addr.GetAddrBytes()); } bool BanMan::IsBanned(const CNetAddr& net_addr) { auto current_time = GetTime(); - LOCK(m_cs_banned); + LOCK(m_banned_mutex); for (const auto& it : m_banned) { CSubNet sub_net = it.first; CBanEntry ban_entry = it.second; @@ -103,7 +103,7 @@ bool BanMan::IsBanned(const CNetAddr& net_addr) bool BanMan::IsBanned(const CSubNet& sub_net) { auto current_time = GetTime(); - LOCK(m_cs_banned); + LOCK(m_banned_mutex); banmap_t::iterator i = m_banned.find(sub_net); if (i != m_banned.end()) { CBanEntry ban_entry = (*i).second; @@ -122,7 +122,7 @@ void BanMan::Ban(const CNetAddr& net_addr, int64_t ban_time_offset, bool since_u void BanMan::Discourage(const CNetAddr& net_addr) { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); m_discouraged.insert(net_addr.GetAddrBytes()); } @@ -139,7 +139,7 @@ void BanMan::Ban(const CSubNet& sub_net, int64_t ban_time_offset, bool since_uni ban_entry.nBanUntil = (normalized_since_unix_epoch ? 0 : GetTime()) + normalized_ban_time_offset; { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); if (m_banned[sub_net].nBanUntil < ban_entry.nBanUntil) { m_banned[sub_net] = ban_entry; m_is_dirty = true; @@ -161,7 +161,7 @@ bool BanMan::Unban(const CNetAddr& net_addr) bool BanMan::Unban(const CSubNet& sub_net) { { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); if (m_banned.erase(sub_net) == 0) return false; m_is_dirty = true; } @@ -172,7 +172,7 @@ bool BanMan::Unban(const CSubNet& sub_net) void BanMan::GetBanned(banmap_t& banmap) { - LOCK(m_cs_banned); + LOCK(m_banned_mutex); // Sweep the banlist so expired bans are not returned SweepBanned(); banmap = m_banned; //create a thread safe copy @@ -180,7 +180,7 @@ void BanMan::GetBanned(banmap_t& banmap) void BanMan::SweepBanned() { - AssertLockHeld(m_cs_banned); + AssertLockHeld(m_banned_mutex); int64_t now = GetTime(); bool notify_ui = false; diff --git a/src/banman.h b/src/banman.h index 7a032dfdd0..e037722744 100644 --- a/src/banman.h +++ b/src/banman.h @@ -80,17 +80,17 @@ class BanMan void DumpBanlist(); private: - void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_cs_banned); + void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); //!clean unused entries (if bantime has expired) - void SweepBanned() EXCLUSIVE_LOCKS_REQUIRED(m_cs_banned); + void SweepBanned() EXCLUSIVE_LOCKS_REQUIRED(m_banned_mutex); - RecursiveMutex m_cs_banned; - banmap_t m_banned GUARDED_BY(m_cs_banned); - bool m_is_dirty GUARDED_BY(m_cs_banned){false}; + RecursiveMutex m_banned_mutex; + banmap_t m_banned GUARDED_BY(m_banned_mutex); + bool m_is_dirty GUARDED_BY(m_banned_mutex){false}; CClientUIInterface* m_client_interface = nullptr; CBanDB m_ban_db; const int64_t m_default_ban_time; - CRollingBloomFilter m_discouraged GUARDED_BY(m_cs_banned) {50000, 0.000001}; + CRollingBloomFilter m_discouraged GUARDED_BY(m_banned_mutex) {50000, 0.000001}; }; #endif // BITCOIN_BANMAN_H From 0fb29087080a4e60d7c709ff5edf14e830ef3a69 Mon Sep 17 00:00:00 2001 From: w0xlt <94266259+w0xlt@users.noreply.github.com> Date: Tue, 18 Jan 2022 03:29:14 -0300 Subject: [PATCH 04/80] refactor: replace RecursiveMutex m_banned_mutex with Mutex --- src/banman.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/banman.h b/src/banman.h index e037722744..c0e07b866d 100644 --- a/src/banman.h +++ b/src/banman.h @@ -84,7 +84,7 @@ class BanMan //!clean unused entries (if bantime has expired) void SweepBanned() EXCLUSIVE_LOCKS_REQUIRED(m_banned_mutex); - RecursiveMutex m_banned_mutex; + Mutex m_banned_mutex; banmap_t m_banned GUARDED_BY(m_banned_mutex); bool m_is_dirty GUARDED_BY(m_banned_mutex){false}; CClientUIInterface* m_client_interface = nullptr; From 37d150d8c5ffcb2bddcd99951a739e97571194c7 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 24 May 2022 10:27:30 +0200 Subject: [PATCH 05/80] refactor: Add more negative `!m_banned_mutex` thread safety annotations Could be verified with $ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative' $ make clean $ make 2>&1 | grep m_banned_mutex --- src/banman.h | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/banman.h b/src/banman.h index c0e07b866d..9200f07aaf 100644 --- a/src/banman.h +++ b/src/banman.h @@ -60,24 +60,24 @@ class BanMan public: ~BanMan(); BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time); - void Ban(const CNetAddr& net_addr, int64_t ban_time_offset = 0, bool since_unix_epoch = false); - void Ban(const CSubNet& sub_net, int64_t ban_time_offset = 0, bool since_unix_epoch = false); - void Discourage(const CNetAddr& net_addr); - void ClearBanned(); + void Ban(const CNetAddr& net_addr, int64_t ban_time_offset = 0, bool since_unix_epoch = false) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void Ban(const CSubNet& sub_net, int64_t ban_time_offset = 0, bool since_unix_epoch = false) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void Discourage(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void ClearBanned() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); //! Return whether net_addr is banned - bool IsBanned(const CNetAddr& net_addr); + bool IsBanned(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); //! Return whether sub_net is exactly banned - bool IsBanned(const CSubNet& sub_net); + bool IsBanned(const CSubNet& sub_net) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); //! Return whether net_addr is discouraged. - bool IsDiscouraged(const CNetAddr& net_addr); + bool IsDiscouraged(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); - bool Unban(const CNetAddr& net_addr); - bool Unban(const CSubNet& sub_net); - void GetBanned(banmap_t& banmap); - void DumpBanlist(); + bool Unban(const CNetAddr& net_addr) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + bool Unban(const CSubNet& sub_net) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void GetBanned(banmap_t& banmap) EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); + void DumpBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); private: void LoadBanlist() EXCLUSIVE_LOCKS_REQUIRED(!m_banned_mutex); From a5e39d325da4eeb9273fb7c919fcbfbc721ed75d Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Sat, 13 Feb 2021 17:38:34 +1000 Subject: [PATCH 06/80] Fee estimation: extend bucket ranges consistently When calculating a median fee for a confirmation target at a particular threshold, we analyse buckets in ranges rather than individually in case some buckets have very little data. This patch ensures the breaks between ranges are independent of the the confirmation target. --- src/policy/fees.cpp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 2b940be07e..6a83f4980a 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -259,6 +259,11 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal, unsigned int curFarBucket = maxbucketindex; unsigned int bestFarBucket = maxbucketindex; + // We'll always group buckets into sets that meet sufficientTxVal -- + // this ensures that we're using consistent groups between different + // confirmation targets. + double partialNum = 0; + bool foundAnswer = false; unsigned int bins = unconfTxs.size(); bool newBucketRange = true; @@ -274,6 +279,7 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal, } curFarBucket = bucket; nConf += confAvg[periodTarget - 1][bucket]; + partialNum += txCtAvg[bucket]; totalNum += txCtAvg[bucket]; failNum += failAvg[periodTarget - 1][bucket]; for (unsigned int confct = confTarget; confct < GetMaxConfirms(); confct++) @@ -283,7 +289,14 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal, // we can test for success // (Only count the confirmed data points, so that each confirmation count // will be looking at the same amount of data and same bucket breaks) - if (totalNum >= sufficientTxVal / (1 - decay)) { + + if (partialNum < sufficientTxVal / (1 - decay)) { + // the buckets we've added in this round aren't sufficient + // so keep adding + continue; + } else { + partialNum = 0; // reset for the next range we'll add + double curPct = nConf / (totalNum + failNum + extraNum); // Check to see if we are no longer getting confirmed at the success rate From 3df37e0c78c3d5139c963a74eda56c331355ef72 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 17 Feb 2023 11:37:56 +0100 Subject: [PATCH 07/80] doc: clarify that LOCK() does AssertLockNotHeld() internally Constructs like ```cpp AssertLockNotHeld(m); LOCK(m); ``` are equivalent to ```cpp LOCK(m); ``` for non-recursive mutexes, so it is ok to omit `AssertLockNotHeld()` in such cases. --- doc/developer-notes.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index e2e54e13d3..d41543ab1c 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -941,7 +941,9 @@ Threads and synchronization internal to a class (private or protected) rather than public. - Combine annotations in function declarations with run-time asserts in - function definitions: + function definitions (`AssertLockNotHeld()` can be omitted if `LOCK()` is + called unconditionally after it because `LOCK()` does the same check as + `AssertLockNotHeld()` internally, for non-recursive mutexes): ```C++ // txmempool.h From 91d08889218e06631f43a3dab0bae576aa46e43c Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Thu, 16 Feb 2023 14:33:57 +0100 Subject: [PATCH 08/80] sync: unpublish LocksHeld() which is used only in sync.cpp --- src/sync.cpp | 2 +- src/sync.h | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index 4621805653..58752a9f18 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -246,7 +246,7 @@ void LeaveCritical() pop_lock(); } -std::string LocksHeld() +static std::string LocksHeld() { LockData& lockdata = GetLockData(); std::lock_guard lock(lockdata.dd_mutex); diff --git a/src/sync.h b/src/sync.h index 7242a793ab..09ec0d1255 100644 --- a/src/sync.h +++ b/src/sync.h @@ -57,7 +57,6 @@ template void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexType* cs, bool fTry = false); void LeaveCritical(); void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line); -std::string LocksHeld(); template void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs); template From fb3e812277041f239b97b88689a5076796d75b9b Mon Sep 17 00:00:00 2001 From: brunoerg Date: Tue, 13 Sep 2022 18:19:40 -0300 Subject: [PATCH 09/80] p2p: return `CSubNet` in `LookupSubNet` --- src/httpserver.cpp | 3 +- src/net_permissions.cpp | 3 +- src/net_types.cpp | 4 +- src/netbase.cpp | 18 +-- src/netbase.h | 6 +- src/rpc/net.cpp | 2 +- src/test/fuzz/netbase_dns_lookup.cpp | 5 +- src/test/netbase_tests.cpp | 175 +++++++++++++-------------- 8 files changed, 101 insertions(+), 115 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 128c4e3c56..e4ba6c3a95 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -173,8 +173,7 @@ static bool InitHTTPAllowList() rpc_allow_subnets.push_back(CSubNet{LookupHost("127.0.0.1", false).value(), 8}); // always allow IPv4 local subnet rpc_allow_subnets.push_back(CSubNet{LookupHost("::1", false).value()}); // always allow IPv6 localhost for (const std::string& strAllow : gArgs.GetArgs("-rpcallowip")) { - CSubNet subnet; - LookupSubNet(strAllow, subnet); + const CSubNet subnet{LookupSubNet(strAllow)}; if (!subnet.IsValid()) { uiInterface.ThreadSafeMessageBox( strprintf(Untranslated("Invalid -rpcallowip subnet specification: %s. Valid are a single IP (e.g. 1.2.3.4), a network/netmask (e.g. 1.2.3.4/255.255.255.0) or a network/CIDR (e.g. 1.2.3.4/24)."), strAllow), diff --git a/src/net_permissions.cpp b/src/net_permissions.cpp index 23226bbb4f..7de7a9dae3 100644 --- a/src/net_permissions.cpp +++ b/src/net_permissions.cpp @@ -111,8 +111,7 @@ bool NetWhitelistPermissions::TryParse(const std::string& str, NetWhitelistPermi if (!TryParsePermissionFlags(str, flags, offset, error)) return false; const std::string net = str.substr(offset); - CSubNet subnet; - LookupSubNet(net, subnet); + const CSubNet subnet{LookupSubNet(net)}; if (!subnet.IsValid()) { error = strprintf(_("Invalid netmask specified in -whitelist: '%s'"), net); return false; diff --git a/src/net_types.cpp b/src/net_types.cpp index 2cdc10d8c9..fd6ad80404 100644 --- a/src/net_types.cpp +++ b/src/net_types.cpp @@ -63,9 +63,9 @@ void BanMapFromJson(const UniValue& bans_json, banmap_t& bans) LogPrintf("Dropping entry with unknown version (%s) from ban list\n", version); continue; } - CSubNet subnet; const auto& subnet_str = ban_entry_json[BANMAN_JSON_ADDR_KEY].get_str(); - if (!LookupSubNet(subnet_str, subnet)) { + const CSubNet subnet{LookupSubNet(subnet_str)}; + if (!subnet.IsValid()) { LogPrintf("Dropping entry with unparseable address or subnet (%s) from ban list\n", subnet_str); continue; } diff --git a/src/netbase.cpp b/src/netbase.cpp index 8f6f92ea7d..d66721f2f9 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -655,10 +655,12 @@ bool ConnectThroughProxy(const Proxy& proxy, const std::string& strDest, uint16_ return true; } -bool LookupSubNet(const std::string& subnet_str, CSubNet& subnet_out) +CSubNet LookupSubNet(const std::string& subnet_str) { + CSubNet subnet; + assert(!subnet.IsValid()); if (!ContainsNoNUL(subnet_str)) { - return false; + return subnet; } const size_t slash_pos{subnet_str.find_last_of('/')}; @@ -671,23 +673,21 @@ bool LookupSubNet(const std::string& subnet_str, CSubNet& subnet_out) uint8_t netmask; if (ParseUInt8(netmask_str, &netmask)) { // Valid number; assume CIDR variable-length subnet masking. - subnet_out = CSubNet{addr.value(), netmask}; - return subnet_out.IsValid(); + subnet = CSubNet{addr.value(), netmask}; } else { // Invalid number; try full netmask syntax. Never allow lookup for netmask. const std::optional full_netmask{LookupHost(netmask_str, /*fAllowLookup=*/false)}; if (full_netmask.has_value()) { - subnet_out = CSubNet{addr.value(), full_netmask.value()}; - return subnet_out.IsValid(); + subnet = CSubNet{addr.value(), full_netmask.value()}; } } } else { // Single IP subnet (/32 or /128). - subnet_out = CSubNet{addr.value()}; - return subnet_out.IsValid(); + subnet = CSubNet{addr.value()}; } } - return false; + + return subnet; } void InterruptSocks5(bool interrupt) diff --git a/src/netbase.h b/src/netbase.h index 1da4f5c51d..dbcc126885 100644 --- a/src/netbase.h +++ b/src/netbase.h @@ -171,11 +171,9 @@ CService LookupNumeric(const std::string& name, uint16_t portDefault = 0, DNSLoo * @param[in] subnet_str A string representation of a subnet of the form * `network address [ "/", ( CIDR-style suffix | netmask ) ]` * e.g. "2001:db8::/32", "192.0.2.0/255.255.255.0" or "8.8.8.8". - * @param[out] subnet_out Internal subnet representation, if parsable/resolvable - * from `subnet_str`. - * @returns whether the operation succeeded or not. + * @returns a CSubNet object (that may or may not be valid). */ -bool LookupSubNet(const std::string& subnet_str, CSubNet& subnet_out); +CSubNet LookupSubNet(const std::string& subnet_str); /** * Create a TCP socket in the given address family. diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index a2a46ef32f..b01fcbe949 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -719,7 +719,7 @@ static RPCHelpMan setban() } } else - LookupSubNet(request.params[0].get_str(), subNet); + subNet = LookupSubNet(request.params[0].get_str()); if (! (isSubnet ? subNet.IsValid() : netAddr.IsValid()) ) throw JSONRPCError(RPC_CLIENT_INVALID_IP_OR_SUBNET, "Error: Invalid IP/Subnet"); diff --git a/src/test/fuzz/netbase_dns_lookup.cpp b/src/test/fuzz/netbase_dns_lookup.cpp index dcf500acc3..ba31315297 100644 --- a/src/test/fuzz/netbase_dns_lookup.cpp +++ b/src/test/fuzz/netbase_dns_lookup.cpp @@ -59,9 +59,6 @@ FUZZ_TARGET(netbase_dns_lookup) assert(!resolved_service.IsInternal()); } { - CSubNet resolved_subnet; - if (LookupSubNet(name, resolved_subnet)) { - assert(resolved_subnet.IsValid()); - } + (void)LookupSubNet(name); } } diff --git a/src/test/netbase_tests.cpp b/src/test/netbase_tests.cpp index 05953bfd10..878d008f54 100644 --- a/src/test/netbase_tests.cpp +++ b/src/test/netbase_tests.cpp @@ -27,13 +27,6 @@ static CNetAddr ResolveIP(const std::string& ip) return LookupHost(ip, false).value_or(CNetAddr{}); } -static CSubNet ResolveSubNet(const std::string& subnet) -{ - CSubNet ret; - LookupSubNet(subnet, ret); - return ret; -} - static CNetAddr CreateInternal(const std::string& host) { CNetAddr addr; @@ -159,49 +152,49 @@ BOOST_AUTO_TEST_CASE(embedded_test) BOOST_AUTO_TEST_CASE(subnet_test) { - BOOST_CHECK(ResolveSubNet("1.2.3.0/24") == ResolveSubNet("1.2.3.0/255.255.255.0")); - BOOST_CHECK(ResolveSubNet("1.2.3.0/24") != ResolveSubNet("1.2.4.0/255.255.255.0")); - BOOST_CHECK(ResolveSubNet("1.2.3.0/24").Match(ResolveIP("1.2.3.4"))); - BOOST_CHECK(!ResolveSubNet("1.2.2.0/24").Match(ResolveIP("1.2.3.4"))); - BOOST_CHECK(ResolveSubNet("1.2.3.4").Match(ResolveIP("1.2.3.4"))); - BOOST_CHECK(ResolveSubNet("1.2.3.4/32").Match(ResolveIP("1.2.3.4"))); - BOOST_CHECK(!ResolveSubNet("1.2.3.4").Match(ResolveIP("5.6.7.8"))); - BOOST_CHECK(!ResolveSubNet("1.2.3.4/32").Match(ResolveIP("5.6.7.8"))); - BOOST_CHECK(ResolveSubNet("::ffff:127.0.0.1").Match(ResolveIP("127.0.0.1"))); - BOOST_CHECK(ResolveSubNet("1:2:3:4:5:6:7:8").Match(ResolveIP("1:2:3:4:5:6:7:8"))); - BOOST_CHECK(!ResolveSubNet("1:2:3:4:5:6:7:8").Match(ResolveIP("1:2:3:4:5:6:7:9"))); - BOOST_CHECK(ResolveSubNet("1:2:3:4:5:6:7:0/112").Match(ResolveIP("1:2:3:4:5:6:7:1234"))); - BOOST_CHECK(ResolveSubNet("192.168.0.1/24").Match(ResolveIP("192.168.0.2"))); - BOOST_CHECK(ResolveSubNet("192.168.0.20/29").Match(ResolveIP("192.168.0.18"))); - BOOST_CHECK(ResolveSubNet("1.2.2.1/24").Match(ResolveIP("1.2.2.4"))); - BOOST_CHECK(ResolveSubNet("1.2.2.110/31").Match(ResolveIP("1.2.2.111"))); - BOOST_CHECK(ResolveSubNet("1.2.2.20/26").Match(ResolveIP("1.2.2.63"))); + BOOST_CHECK(LookupSubNet("1.2.3.0/24") == LookupSubNet("1.2.3.0/255.255.255.0")); + BOOST_CHECK(LookupSubNet("1.2.3.0/24") != LookupSubNet("1.2.4.0/255.255.255.0")); + BOOST_CHECK(LookupSubNet("1.2.3.0/24").Match(ResolveIP("1.2.3.4"))); + BOOST_CHECK(!LookupSubNet("1.2.2.0/24").Match(ResolveIP("1.2.3.4"))); + BOOST_CHECK(LookupSubNet("1.2.3.4").Match(ResolveIP("1.2.3.4"))); + BOOST_CHECK(LookupSubNet("1.2.3.4/32").Match(ResolveIP("1.2.3.4"))); + BOOST_CHECK(!LookupSubNet("1.2.3.4").Match(ResolveIP("5.6.7.8"))); + BOOST_CHECK(!LookupSubNet("1.2.3.4/32").Match(ResolveIP("5.6.7.8"))); + BOOST_CHECK(LookupSubNet("::ffff:127.0.0.1").Match(ResolveIP("127.0.0.1"))); + BOOST_CHECK(LookupSubNet("1:2:3:4:5:6:7:8").Match(ResolveIP("1:2:3:4:5:6:7:8"))); + BOOST_CHECK(!LookupSubNet("1:2:3:4:5:6:7:8").Match(ResolveIP("1:2:3:4:5:6:7:9"))); + BOOST_CHECK(LookupSubNet("1:2:3:4:5:6:7:0/112").Match(ResolveIP("1:2:3:4:5:6:7:1234"))); + BOOST_CHECK(LookupSubNet("192.168.0.1/24").Match(ResolveIP("192.168.0.2"))); + BOOST_CHECK(LookupSubNet("192.168.0.20/29").Match(ResolveIP("192.168.0.18"))); + BOOST_CHECK(LookupSubNet("1.2.2.1/24").Match(ResolveIP("1.2.2.4"))); + BOOST_CHECK(LookupSubNet("1.2.2.110/31").Match(ResolveIP("1.2.2.111"))); + BOOST_CHECK(LookupSubNet("1.2.2.20/26").Match(ResolveIP("1.2.2.63"))); // All-Matching IPv6 Matches arbitrary IPv6 - BOOST_CHECK(ResolveSubNet("::/0").Match(ResolveIP("1:2:3:4:5:6:7:1234"))); + BOOST_CHECK(LookupSubNet("::/0").Match(ResolveIP("1:2:3:4:5:6:7:1234"))); // But not `::` or `0.0.0.0` because they are considered invalid addresses - BOOST_CHECK(!ResolveSubNet("::/0").Match(ResolveIP("::"))); - BOOST_CHECK(!ResolveSubNet("::/0").Match(ResolveIP("0.0.0.0"))); + BOOST_CHECK(!LookupSubNet("::/0").Match(ResolveIP("::"))); + BOOST_CHECK(!LookupSubNet("::/0").Match(ResolveIP("0.0.0.0"))); // Addresses from one network (IPv4) don't belong to subnets of another network (IPv6) - BOOST_CHECK(!ResolveSubNet("::/0").Match(ResolveIP("1.2.3.4"))); + BOOST_CHECK(!LookupSubNet("::/0").Match(ResolveIP("1.2.3.4"))); // All-Matching IPv4 does not Match IPv6 - BOOST_CHECK(!ResolveSubNet("0.0.0.0/0").Match(ResolveIP("1:2:3:4:5:6:7:1234"))); + BOOST_CHECK(!LookupSubNet("0.0.0.0/0").Match(ResolveIP("1:2:3:4:5:6:7:1234"))); // Invalid subnets Match nothing (not even invalid addresses) BOOST_CHECK(!CSubNet().Match(ResolveIP("1.2.3.4"))); - BOOST_CHECK(!ResolveSubNet("").Match(ResolveIP("4.5.6.7"))); - BOOST_CHECK(!ResolveSubNet("bloop").Match(ResolveIP("0.0.0.0"))); - BOOST_CHECK(!ResolveSubNet("bloop").Match(ResolveIP("hab"))); + BOOST_CHECK(!LookupSubNet("").Match(ResolveIP("4.5.6.7"))); + BOOST_CHECK(!LookupSubNet("bloop").Match(ResolveIP("0.0.0.0"))); + BOOST_CHECK(!LookupSubNet("bloop").Match(ResolveIP("hab"))); // Check valid/invalid - BOOST_CHECK(ResolveSubNet("1.2.3.0/0").IsValid()); - BOOST_CHECK(!ResolveSubNet("1.2.3.0/-1").IsValid()); - BOOST_CHECK(ResolveSubNet("1.2.3.0/32").IsValid()); - BOOST_CHECK(!ResolveSubNet("1.2.3.0/33").IsValid()); - BOOST_CHECK(!ResolveSubNet("1.2.3.0/300").IsValid()); - BOOST_CHECK(ResolveSubNet("1:2:3:4:5:6:7:8/0").IsValid()); - BOOST_CHECK(ResolveSubNet("1:2:3:4:5:6:7:8/33").IsValid()); - BOOST_CHECK(!ResolveSubNet("1:2:3:4:5:6:7:8/-1").IsValid()); - BOOST_CHECK(ResolveSubNet("1:2:3:4:5:6:7:8/128").IsValid()); - BOOST_CHECK(!ResolveSubNet("1:2:3:4:5:6:7:8/129").IsValid()); - BOOST_CHECK(!ResolveSubNet("fuzzy").IsValid()); + BOOST_CHECK(LookupSubNet("1.2.3.0/0").IsValid()); + BOOST_CHECK(!LookupSubNet("1.2.3.0/-1").IsValid()); + BOOST_CHECK(LookupSubNet("1.2.3.0/32").IsValid()); + BOOST_CHECK(!LookupSubNet("1.2.3.0/33").IsValid()); + BOOST_CHECK(!LookupSubNet("1.2.3.0/300").IsValid()); + BOOST_CHECK(LookupSubNet("1:2:3:4:5:6:7:8/0").IsValid()); + BOOST_CHECK(LookupSubNet("1:2:3:4:5:6:7:8/33").IsValid()); + BOOST_CHECK(!LookupSubNet("1:2:3:4:5:6:7:8/-1").IsValid()); + BOOST_CHECK(LookupSubNet("1:2:3:4:5:6:7:8/128").IsValid()); + BOOST_CHECK(!LookupSubNet("1:2:3:4:5:6:7:8/129").IsValid()); + BOOST_CHECK(!LookupSubNet("fuzzy").IsValid()); //CNetAddr constructor test BOOST_CHECK(CSubNet(ResolveIP("127.0.0.1")).IsValid()); @@ -247,85 +240,85 @@ BOOST_AUTO_TEST_CASE(subnet_test) BOOST_CHECK(!CSubNet(tor_addr, 200).IsValid()); BOOST_CHECK(!CSubNet(tor_addr, ResolveIP("255.0.0.0")).IsValid()); - subnet = ResolveSubNet("1.2.3.4/255.255.255.255"); + subnet = LookupSubNet("1.2.3.4/255.255.255.255"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.3.4/32"); - subnet = ResolveSubNet("1.2.3.4/255.255.255.254"); + subnet = LookupSubNet("1.2.3.4/255.255.255.254"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.3.4/31"); - subnet = ResolveSubNet("1.2.3.4/255.255.255.252"); + subnet = LookupSubNet("1.2.3.4/255.255.255.252"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.3.4/30"); - subnet = ResolveSubNet("1.2.3.4/255.255.255.248"); + subnet = LookupSubNet("1.2.3.4/255.255.255.248"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.3.0/29"); - subnet = ResolveSubNet("1.2.3.4/255.255.255.240"); + subnet = LookupSubNet("1.2.3.4/255.255.255.240"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.3.0/28"); - subnet = ResolveSubNet("1.2.3.4/255.255.255.224"); + subnet = LookupSubNet("1.2.3.4/255.255.255.224"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.3.0/27"); - subnet = ResolveSubNet("1.2.3.4/255.255.255.192"); + subnet = LookupSubNet("1.2.3.4/255.255.255.192"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.3.0/26"); - subnet = ResolveSubNet("1.2.3.4/255.255.255.128"); + subnet = LookupSubNet("1.2.3.4/255.255.255.128"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.3.0/25"); - subnet = ResolveSubNet("1.2.3.4/255.255.255.0"); + subnet = LookupSubNet("1.2.3.4/255.255.255.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.3.0/24"); - subnet = ResolveSubNet("1.2.3.4/255.255.254.0"); + subnet = LookupSubNet("1.2.3.4/255.255.254.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.2.0/23"); - subnet = ResolveSubNet("1.2.3.4/255.255.252.0"); + subnet = LookupSubNet("1.2.3.4/255.255.252.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.0.0/22"); - subnet = ResolveSubNet("1.2.3.4/255.255.248.0"); + subnet = LookupSubNet("1.2.3.4/255.255.248.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.0.0/21"); - subnet = ResolveSubNet("1.2.3.4/255.255.240.0"); + subnet = LookupSubNet("1.2.3.4/255.255.240.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.0.0/20"); - subnet = ResolveSubNet("1.2.3.4/255.255.224.0"); + subnet = LookupSubNet("1.2.3.4/255.255.224.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.0.0/19"); - subnet = ResolveSubNet("1.2.3.4/255.255.192.0"); + subnet = LookupSubNet("1.2.3.4/255.255.192.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.0.0/18"); - subnet = ResolveSubNet("1.2.3.4/255.255.128.0"); + subnet = LookupSubNet("1.2.3.4/255.255.128.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.0.0/17"); - subnet = ResolveSubNet("1.2.3.4/255.255.0.0"); + subnet = LookupSubNet("1.2.3.4/255.255.0.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.0.0/16"); - subnet = ResolveSubNet("1.2.3.4/255.254.0.0"); + subnet = LookupSubNet("1.2.3.4/255.254.0.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.0.0/15"); - subnet = ResolveSubNet("1.2.3.4/255.252.0.0"); + subnet = LookupSubNet("1.2.3.4/255.252.0.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.0.0.0/14"); - subnet = ResolveSubNet("1.2.3.4/255.248.0.0"); + subnet = LookupSubNet("1.2.3.4/255.248.0.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.0.0.0/13"); - subnet = ResolveSubNet("1.2.3.4/255.240.0.0"); + subnet = LookupSubNet("1.2.3.4/255.240.0.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.0.0.0/12"); - subnet = ResolveSubNet("1.2.3.4/255.224.0.0"); + subnet = LookupSubNet("1.2.3.4/255.224.0.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.0.0.0/11"); - subnet = ResolveSubNet("1.2.3.4/255.192.0.0"); + subnet = LookupSubNet("1.2.3.4/255.192.0.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.0.0.0/10"); - subnet = ResolveSubNet("1.2.3.4/255.128.0.0"); + subnet = LookupSubNet("1.2.3.4/255.128.0.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.0.0.0/9"); - subnet = ResolveSubNet("1.2.3.4/255.0.0.0"); + subnet = LookupSubNet("1.2.3.4/255.0.0.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.0.0.0/8"); - subnet = ResolveSubNet("1.2.3.4/254.0.0.0"); + subnet = LookupSubNet("1.2.3.4/254.0.0.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "0.0.0.0/7"); - subnet = ResolveSubNet("1.2.3.4/252.0.0.0"); + subnet = LookupSubNet("1.2.3.4/252.0.0.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "0.0.0.0/6"); - subnet = ResolveSubNet("1.2.3.4/248.0.0.0"); + subnet = LookupSubNet("1.2.3.4/248.0.0.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "0.0.0.0/5"); - subnet = ResolveSubNet("1.2.3.4/240.0.0.0"); + subnet = LookupSubNet("1.2.3.4/240.0.0.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "0.0.0.0/4"); - subnet = ResolveSubNet("1.2.3.4/224.0.0.0"); + subnet = LookupSubNet("1.2.3.4/224.0.0.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "0.0.0.0/3"); - subnet = ResolveSubNet("1.2.3.4/192.0.0.0"); + subnet = LookupSubNet("1.2.3.4/192.0.0.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "0.0.0.0/2"); - subnet = ResolveSubNet("1.2.3.4/128.0.0.0"); + subnet = LookupSubNet("1.2.3.4/128.0.0.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "0.0.0.0/1"); - subnet = ResolveSubNet("1.2.3.4/0.0.0.0"); + subnet = LookupSubNet("1.2.3.4/0.0.0.0"); BOOST_CHECK_EQUAL(subnet.ToString(), "0.0.0.0/0"); - subnet = ResolveSubNet("1:2:3:4:5:6:7:8/ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff"); + subnet = LookupSubNet("1:2:3:4:5:6:7:8/ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff"); BOOST_CHECK_EQUAL(subnet.ToString(), "1:2:3:4:5:6:7:8/128"); - subnet = ResolveSubNet("1:2:3:4:5:6:7:8/ffff:0000:0000:0000:0000:0000:0000:0000"); + subnet = LookupSubNet("1:2:3:4:5:6:7:8/ffff:0000:0000:0000:0000:0000:0000:0000"); BOOST_CHECK_EQUAL(subnet.ToString(), "1::/16"); - subnet = ResolveSubNet("1:2:3:4:5:6:7:8/0000:0000:0000:0000:0000:0000:0000:0000"); + subnet = LookupSubNet("1:2:3:4:5:6:7:8/0000:0000:0000:0000:0000:0000:0000:0000"); BOOST_CHECK_EQUAL(subnet.ToString(), "::/0"); // Invalid netmasks (with 1-bits after 0-bits) - subnet = ResolveSubNet("1.2.3.4/255.255.232.0"); + subnet = LookupSubNet("1.2.3.4/255.255.232.0"); BOOST_CHECK(!subnet.IsValid()); - subnet = ResolveSubNet("1.2.3.4/255.0.255.255"); + subnet = LookupSubNet("1.2.3.4/255.0.255.255"); BOOST_CHECK(!subnet.IsValid()); - subnet = ResolveSubNet("1:2:3:4:5:6:7:8/ffff:ffff:ffff:fffe:ffff:ffff:ffff:ff0f"); + subnet = LookupSubNet("1:2:3:4:5:6:7:8/ffff:ffff:ffff:fffe:ffff:ffff:ffff:ff0f"); BOOST_CHECK(!subnet.IsValid()); } @@ -479,15 +472,15 @@ BOOST_AUTO_TEST_CASE(netbase_dont_resolve_strings_with_embedded_nul_characters) BOOST_CHECK(!LookupHost("127.0.0.1\0"s, false).has_value()); BOOST_CHECK(!LookupHost("127.0.0.1\0example.com"s, false).has_value()); BOOST_CHECK(!LookupHost("127.0.0.1\0example.com\0"s, false).has_value()); - CSubNet ret; - BOOST_CHECK(LookupSubNet("1.2.3.0/24"s, ret)); - BOOST_CHECK(!LookupSubNet("1.2.3.0/24\0"s, ret)); - BOOST_CHECK(!LookupSubNet("1.2.3.0/24\0example.com"s, ret)); - BOOST_CHECK(!LookupSubNet("1.2.3.0/24\0example.com\0"s, ret)); - BOOST_CHECK(LookupSubNet("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"s, ret)); - BOOST_CHECK(!LookupSubNet("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion\0"s, ret)); - BOOST_CHECK(!LookupSubNet("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion\0example.com"s, ret)); - BOOST_CHECK(!LookupSubNet("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion\0example.com\0"s, ret)); + + BOOST_CHECK(LookupSubNet("1.2.3.0/24"s).IsValid()); + BOOST_CHECK(!LookupSubNet("1.2.3.0/24\0"s).IsValid()); + BOOST_CHECK(!LookupSubNet("1.2.3.0/24\0example.com"s).IsValid()); + BOOST_CHECK(!LookupSubNet("1.2.3.0/24\0example.com\0"s).IsValid()); + BOOST_CHECK(LookupSubNet("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"s).IsValid()); + BOOST_CHECK(!LookupSubNet("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion\0"s).IsValid()); + BOOST_CHECK(!LookupSubNet("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion\0example.com"s).IsValid()); + BOOST_CHECK(!LookupSubNet("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion\0example.com\0"s).IsValid()); } // Since CNetAddr (un)ser is tested separately in net_tests.cpp here we only From 51e4dc49f5335b5bae6c14606d1cc653a08a96b1 Mon Sep 17 00:00:00 2001 From: John Moffett Date: Thu, 29 Jun 2023 12:19:18 -0400 Subject: [PATCH 10/80] gui: Show error if unrecognized command line args are present Starting bitcoin-qt with non-dash ("-") arguments causes it to silently ignore any later valid options. This change makes the client exit with an error message if any such "loose" arguments are encountered. However, allow BIP-21 'bitcoin:' URIs only if no other options follow. --- src/qt/bitcoin.cpp | 28 ++++++++++++++++++++++++++++ src/qt/paymentserver.h | 2 ++ 2 files changed, 30 insertions(+) diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index 8f45af9485..96d5e1180c 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -543,6 +543,34 @@ int GuiMain(int argc, char* argv[]) return EXIT_FAILURE; } + // Error out when loose non-argument tokens are encountered on command line + // However, allow BIP-21 URIs only if no options follow + bool payment_server_token_seen = false; + for (int i = 1; i < argc; i++) { + QString arg(argv[i]); + bool invalid_token = !arg.startsWith("-"); +#ifdef ENABLE_WALLET + if (arg.startsWith(BITCOIN_IPC_PREFIX, Qt::CaseInsensitive)) { + invalid_token &= false; + payment_server_token_seen = true; + } +#endif + if (payment_server_token_seen && arg.startsWith("-")) { + InitError(Untranslated(strprintf("Options ('%s') cannot follow a BIP-21 payment URI", argv[i]))); + QMessageBox::critical(nullptr, PACKAGE_NAME, + // message cannot be translated because translations have not been initialized + QString::fromStdString("Options ('%1') cannot follow a BIP-21 payment URI").arg(QString::fromStdString(argv[i]))); + return EXIT_FAILURE; + } + if (invalid_token) { + InitError(Untranslated(strprintf("Command line contains unexpected token '%s', see bitcoin-qt -h for a list of options.", argv[i]))); + QMessageBox::critical(nullptr, PACKAGE_NAME, + // message cannot be translated because translations have not been initialized + QString::fromStdString("Command line contains unexpected token '%1', see bitcoin-qt -h for a list of options.").arg(QString::fromStdString(argv[i]))); + return EXIT_FAILURE; + } + } + // Now that the QApplication is setup and we have parsed our parameters, we can set the platform style app.setupPlatformStyle(); diff --git a/src/qt/paymentserver.h b/src/qt/paymentserver.h index 63f4faa772..f73aa1e61e 100644 --- a/src/qt/paymentserver.h +++ b/src/qt/paymentserver.h @@ -54,6 +54,8 @@ class QLocalServer; class QUrl; QT_END_NAMESPACE +extern const QString BITCOIN_IPC_PREFIX; + class PaymentServer : public QObject { Q_OBJECT From bdb2e8d4aef927efd4e74d31b5d4dffe73470d1f Mon Sep 17 00:00:00 2001 From: iamcarlos94 <62184065+iamcarlos94@users.noreply.github.com> Date: Thu, 17 Aug 2023 13:46:18 +0100 Subject: [PATCH 11/80] Update JSON-RPC-interface.md clarifying when the .cookie file is generated --- doc/JSON-RPC-interface.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/JSON-RPC-interface.md b/doc/JSON-RPC-interface.md index 6cbb6ebd72..ec332d23eb 100644 --- a/doc/JSON-RPC-interface.md +++ b/doc/JSON-RPC-interface.md @@ -128,7 +128,7 @@ RPC interface will be abused. Instead, expose it only on the host system's localhost, for example: `-p 127.0.0.1:8332:8332` -- **Secure authentication:** By default, Bitcoin Core generates unique +- **Secure authentication:** By default, when no `rpcpassword` is specified, Bitcoin Core generates unique login credentials each time it restarts and puts them into a file readable only by the user that started Bitcoin Core, allowing any of that user's RPC clients with read access to the file to login From 7d494a48ddf4248ef3b1753b6e7f2eeab3a8ecb7 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Thu, 27 Jul 2023 12:47:28 -0600 Subject: [PATCH 12/80] refactor: use string_view to pass string literals to Parse{Hash,Hex} as string_view is optimized to be trivially copiable, and in these use cases we only perform read operations on the passed object. These utility methods are called by quite a few RPCs and tests, as well as by each other. $ git grep "ParseHashV\|ParseHashO\|ParseHexV\|ParseHexO" | wc -l 61 --- src/rpc/util.cpp | 15 ++++++++------- src/rpc/util.h | 9 +++++---- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 45b7d89a7b..79089e8aef 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -20,6 +20,7 @@ #include #include +#include #include const std::string UNIX_EPOCH_TIME = "UNIX epoch time"; @@ -74,29 +75,29 @@ CAmount AmountFromValue(const UniValue& value, int decimals) return amount; } -uint256 ParseHashV(const UniValue& v, std::string strName) +uint256 ParseHashV(const UniValue& v, std::string_view name) { const std::string& strHex(v.get_str()); if (64 != strHex.length()) - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be of length %d (not %d, for '%s')", strName, 64, strHex.length(), strHex)); + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be of length %d (not %d, for '%s')", name, 64, strHex.length(), strHex)); if (!IsHex(strHex)) // Note: IsHex("") is false - throw JSONRPCError(RPC_INVALID_PARAMETER, strName+" must be hexadecimal string (not '"+strHex+"')"); + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be hexadecimal string (not '%s')", name, strHex)); return uint256S(strHex); } -uint256 ParseHashO(const UniValue& o, std::string strKey) +uint256 ParseHashO(const UniValue& o, std::string_view strKey) { return ParseHashV(o.find_value(strKey), strKey); } -std::vector ParseHexV(const UniValue& v, std::string strName) +std::vector ParseHexV(const UniValue& v, std::string_view name) { std::string strHex; if (v.isStr()) strHex = v.get_str(); if (!IsHex(strHex)) - throw JSONRPCError(RPC_INVALID_PARAMETER, strName+" must be hexadecimal string (not '"+strHex+"')"); + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be hexadecimal string (not '%s')", name, strHex)); return ParseHex(strHex); } -std::vector ParseHexO(const UniValue& o, std::string strKey) +std::vector ParseHexO(const UniValue& o, std::string_view strKey) { return ParseHexV(o.find_value(strKey), strKey); } diff --git a/src/rpc/util.h b/src/rpc/util.h index 392540ffad..addf9000d0 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -91,10 +92,10 @@ void RPCTypeCheckObj(const UniValue& o, * Utilities: convert hex-encoded Values * (throws error if not hex). */ -uint256 ParseHashV(const UniValue& v, std::string strName); -uint256 ParseHashO(const UniValue& o, std::string strKey); -std::vector ParseHexV(const UniValue& v, std::string strName); -std::vector ParseHexO(const UniValue& o, std::string strKey); +uint256 ParseHashV(const UniValue& v, std::string_view name); +uint256 ParseHashO(const UniValue& o, std::string_view strKey); +std::vector ParseHexV(const UniValue& v, std::string_view name); +std::vector ParseHexO(const UniValue& o, std::string_view strKey); /** * Validate and return a CAmount from a UniValue number or string. From bb91131d545d986aab81c4bb13676c4520169259 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Thu, 27 Jul 2023 13:52:29 -0600 Subject: [PATCH 13/80] doc: remove out-of-date external link in src/util/strencodings.h --- src/util/strencodings.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/util/strencodings.h b/src/util/strencodings.h index d792562735..439678c24a 100644 --- a/src/util/strencodings.h +++ b/src/util/strencodings.h @@ -260,7 +260,6 @@ bool TimingResistantEqual(const T& a, const T& b) } /** Parse number as fixed point according to JSON number syntax. - * See https://json.org/number.gif * @returns true on success, false on error. * @note The result must be in the range (-10^18,10^18), otherwise an overflow error will trigger. */ From fa8996b930886da712c09ffe4b58016b36c2ae5b Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 3 Aug 2023 14:20:00 +0200 Subject: [PATCH 14/80] ci: Bump i686_multiprocess.sh to latest Ubuntu LTS There is no need to have it stuck on the previous one. This is needed for the next commit. --- ci/test/00_setup_env_i686_multiprocess.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/test/00_setup_env_i686_multiprocess.sh b/ci/test/00_setup_env_i686_multiprocess.sh index b11a387660..2cecf8510e 100755 --- a/ci/test/00_setup_env_i686_multiprocess.sh +++ b/ci/test/00_setup_env_i686_multiprocess.sh @@ -8,7 +8,7 @@ export LC_ALL=C.UTF-8 export HOST=i686-pc-linux-gnu export CONTAINER_NAME=ci_i686_multiprocess -export CI_IMAGE_NAME_TAG="docker.io/amd64/ubuntu:20.04" +export CI_IMAGE_NAME_TAG="docker.io/amd64/ubuntu:22.04" export PACKAGES="cmake llvm clang g++-multilib" export DEP_OPTS="DEBUG=1 MULTIPROCESS=1" export GOAL="install" From faf70c1f330a92612cf381d32c791e9ba445d3f2 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 3 Aug 2023 15:20:05 +0200 Subject: [PATCH 15/80] Bump python minimum version to 3.9 --- .python-version | 2 +- ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh | 8 ++++---- ci/test/00_setup_env_native_qt5.sh | 4 ++-- configure.ac | 4 ++-- doc/dependencies.md | 2 +- test/functional/test_framework/util.py | 4 ++-- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/.python-version b/.python-version index e29d80998a..2739029233 100644 --- a/.python-version +++ b/.python-version @@ -1 +1 @@ -3.8.17 +3.9.17 diff --git a/ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh b/ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh index 454a4b8dff..2d181f1bb8 100755 --- a/ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh +++ b/ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh @@ -7,9 +7,9 @@ export LC_ALL=C.UTF-8 export CONTAINER_NAME=ci_native_nowallet_libbitcoinkernel -export CI_IMAGE_NAME_TAG="docker.io/ubuntu:20.04" -# Use minimum supported python3.8 and clang-10, see doc/dependencies.md -export PACKAGES="python3-zmq clang-10 llvm-10 libc++abi-10-dev libc++-10-dev" -export DEP_OPTS="NO_WALLET=1 CC=clang-10 CXX='clang++-10 -stdlib=libc++'" +export CI_IMAGE_NAME_TAG="docker.io/debian:bullseye" +# Use minimum supported python3.9 and clang-10 (or best-effort clang-11), see doc/dependencies.md +export PACKAGES="python3-zmq clang-11 llvm-11 libc++abi-11-dev libc++-11-dev" +export DEP_OPTS="NO_WALLET=1 CC=clang-11 CXX='clang++-11 -stdlib=libc++'" export GOAL="install" export BITCOIN_CONFIG="--enable-reduce-exports --enable-experimental-util-chainstate --with-experimental-kernel-lib --enable-shared" diff --git a/ci/test/00_setup_env_native_qt5.sh b/ci/test/00_setup_env_native_qt5.sh index 0fad95e2cb..32c8557eeb 100755 --- a/ci/test/00_setup_env_native_qt5.sh +++ b/ci/test/00_setup_env_native_qt5.sh @@ -7,8 +7,8 @@ export LC_ALL=C.UTF-8 export CONTAINER_NAME=ci_native_qt5 -export CI_IMAGE_NAME_TAG="docker.io/ubuntu:20.04" -# Use minimum supported python3.8 and gcc-9, see doc/dependencies.md +export CI_IMAGE_NAME_TAG="docker.io/debian:bullseye" +# Use minimum supported python3.9 and gcc-9, see doc/dependencies.md export PACKAGES="gcc-9 g++-9 python3-zmq qtbase5-dev qttools5-dev-tools libdbus-1-dev libharfbuzz-dev" export DEP_OPTS="NO_QT=1 NO_UPNP=1 NO_NATPMP=1 DEBUG=1 ALLOW_HOST_PACKAGES=1 CC=gcc-9 CXX=g++-9" export TEST_RUNNER_EXTRA="--previous-releases --coverage --extended --exclude feature_dbcrash" # Run extended tests so that coverage does not fail, but exclude the very slow dbcrash diff --git a/configure.ac b/configure.ac index f0ebc50623..3693a76ea2 100644 --- a/configure.ac +++ b/configure.ac @@ -128,8 +128,8 @@ AC_PATH_TOOL([AR], [ar]) AC_PATH_TOOL([GCOV], [gcov]) AC_PATH_TOOL([LLVM_COV], [llvm-cov]) AC_PATH_PROG([LCOV], [lcov]) -dnl Python 3.8 is specified in .python-version and should be used if available, see doc/dependencies.md -AC_PATH_PROGS([PYTHON], [python3.8 python3.9 python3.10 python3.11 python3.12 python3 python]) +dnl The minimum supported version is specified in .python-version and should be used if available, see doc/dependencies.md +AC_PATH_PROGS([PYTHON], [python3.9 python3.10 python3.11 python3.12 python3 python]) AC_PATH_PROG([GENHTML], [genhtml]) AC_PATH_PROG([GIT], [git]) AC_PATH_PROG([CCACHE], [ccache]) diff --git a/doc/dependencies.md b/doc/dependencies.md index 804f796abe..af2621652c 100644 --- a/doc/dependencies.md +++ b/doc/dependencies.md @@ -10,7 +10,7 @@ You can find installation instructions in the `build-*.md` file for your platfor | [Automake](https://www.gnu.org/software/automake/) | [1.13](https://github.com/bitcoin/bitcoin/pull/18290) | | [Clang](https://clang.llvm.org) | [10.0](https://github.com/bitcoin/bitcoin/pull/27682) | | [GCC](https://gcc.gnu.org) | [9.1](https://github.com/bitcoin/bitcoin/pull/27662) | -| [Python](https://www.python.org) (scripts, tests) | [3.8](https://github.com/bitcoin/bitcoin/pull/27483) | +| [Python](https://www.python.org) (scripts, tests) | [3.9](https://github.com/bitcoin/bitcoin/pull/28211) | | [systemtap](https://sourceware.org/systemtap/) ([tracing](tracing.md))| N/A | ## Required diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index 9143397042..54361a3aab 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -287,10 +287,10 @@ def sha256sum_file(filename): return h.digest() -# TODO: Remove and use random.randbytes(n) instead, available in Python 3.9 +# TODO: Remove and use random.randbytes(n) directly def random_bytes(n): """Return a random bytes object of length n.""" - return bytes(random.getrandbits(8) for i in range(n)) + return random.randbytes(n) # RPC/P2P connection constants and functions From fa25e8b0a1610553014c786428f146ef9c694678 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 18 Aug 2023 15:23:04 +0200 Subject: [PATCH 16/80] doc: Recommend lint image build on every call --- test/lint/README.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/test/lint/README.md b/test/lint/README.md index d9cfeb50ed..c0889b59af 100644 --- a/test/lint/README.md +++ b/test/lint/README.md @@ -7,13 +7,11 @@ To run linters locally with the same versions as the CI environment, use the inc Dockerfile: ```sh -DOCKER_BUILDKIT=1 docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ - -docker run --rm -v $(pwd):/bitcoin -it bitcoin-linter +DOCKER_BUILDKIT=1 docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ && docker run --rm -v $(pwd):/bitcoin -it bitcoin-linter ``` -After building the container once, you can simply run the last command any time you -want to lint. +Building the container can be done every time, because it is fast when the +result is cached and it prevents issues when the image changes. check-doc.py From 376dc2cfb32806a8aa450589effe4d384e648398 Mon Sep 17 00:00:00 2001 From: kevkevin Date: Sat, 10 Jun 2023 15:54:13 -0500 Subject: [PATCH 17/80] test: add coverage to rpc_blockchain.py Included a test that checks the functionality of setting the first param of getnetworkhashps to negative value returns the average network hashes per second from the last difficulty change. Co-authored-by: ismaelsadeeq --- test/functional/rpc_blockchain.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py index 18a0a0c6cc..45e5cff8cd 100755 --- a/test/functional/rpc_blockchain.py +++ b/test/functional/rpc_blockchain.py @@ -58,6 +58,7 @@ TIME_RANGE_MTP = TIME_GENESIS_BLOCK + (HEIGHT - 6) * TIME_RANGE_STEP TIME_RANGE_TIP = TIME_GENESIS_BLOCK + (HEIGHT - 1) * TIME_RANGE_STEP TIME_RANGE_END = TIME_GENESIS_BLOCK + HEIGHT * TIME_RANGE_STEP +DIFFICULTY_ADJUSTMENT_INTERVAL = 2016 class BlockchainTest(BitcoinTestFramework): @@ -451,6 +452,15 @@ def _test_getnetworkhashps(self): # This should be 2 hashes every 10 minutes or 1/300 assert abs(hashes_per_second * 300 - 1) < 0.0001 + # Test setting the first param of getnetworkhashps to negative value returns the average network + # hashes per second from the last difficulty change. + current_block_height = self.nodes[0].getmininginfo()['blocks'] + blocks_since_last_diff_change = current_block_height % DIFFICULTY_ADJUSTMENT_INTERVAL + 1 + expected_hashes_per_second_since_diff_change = self.nodes[0].getnetworkhashps(blocks_since_last_diff_change) + + assert_equal(self.nodes[0].getnetworkhashps(-1), expected_hashes_per_second_since_diff_change) + assert_equal(self.nodes[0].getnetworkhashps(-2), expected_hashes_per_second_since_diff_change) + def _test_stopatheight(self): self.log.info("Test stopping at height") assert_equal(self.nodes[0].getblockcount(), HEIGHT) From cdb14d79e809bf7d1612b21b554a9fcfb2ab1c91 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Fri, 21 Jul 2023 15:53:55 +0200 Subject: [PATCH 18/80] [net processing] Use HasWitness over comparing (w)txids --- src/net_processing.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index b046b3ac16..dea257f7cc 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1852,7 +1852,7 @@ void PeerManagerImpl::BlockConnected(const std::shared_ptr& pblock LOCK(m_recent_confirmed_transactions_mutex); for (const auto& ptx : pblock->vtx) { m_recent_confirmed_transactions.insert(ptx->GetHash()); - if (ptx->GetHash() != ptx->GetWitnessHash()) { + if (ptx->HasWitness()) { m_recent_confirmed_transactions.insert(ptx->GetWitnessHash()); } } @@ -2976,7 +2976,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) // processing of this transaction in the event that child // transactions are later received (resulting in // parent-fetching by txid via the orphan-handling logic). - if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && porphanTx->GetWitnessHash() != porphanTx->GetHash()) { + if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && porphanTx->HasWitness()) { // We only add the txid if it differs from the wtxid, to // avoid wasting entries in the rolling bloom filter. m_recent_rejects.insert(porphanTx->GetHash()); @@ -4260,7 +4260,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // processing of this transaction in the event that child // transactions are later received (resulting in // parent-fetching by txid via the orphan-handling logic). - if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && tx.GetWitnessHash() != tx.GetHash()) { + if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && tx.HasWitness()) { m_recent_rejects.insert(tx.GetHash()); m_txrequest.ForgetTxHash(tx.GetHash()); } From 88c33c6748da3c4fdadc554ebca43ce7267e9178 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Tue, 10 Oct 2023 12:11:00 -0400 Subject: [PATCH 19/80] test: make python p2p not send getaddr messages when it's being connected to Bitcoind nodes send getaddr msgs only to outbound nodes (and ignore those received by outgoing connections). The python p2p node should mirror this behavior by not sending a getaddr message when it is not the initiator of the connection. --- test/functional/test_framework/p2p.py | 3 ++- test/functional/test_framework/test_node.py | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py index be4ed624fc..352becb367 100755 --- a/test/functional/test_framework/p2p.py +++ b/test/functional/test_framework/p2p.py @@ -456,7 +456,8 @@ def on_version(self, message): self.send_message(msg_verack()) self.nServices = message.nServices self.relay = message.relay - self.send_message(msg_getaddr()) + if self.p2p_connected_to_node: + self.send_message(msg_getaddr()) # Connection helper methods diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 33a7539641..87f6196ae6 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -646,6 +646,7 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, **kwargs): if 'dstaddr' not in kwargs: kwargs['dstaddr'] = '127.0.0.1' + p2p_conn.p2p_connected_to_node = True p2p_conn.peer_connect(**kwargs, net=self.chain, timeout_factor=self.timeout_factor)() self.p2ps.append(p2p_conn) p2p_conn.wait_until(lambda: p2p_conn.is_connected, check_connected=False) @@ -691,6 +692,7 @@ def addconnection_callback(address, port): self.log.debug("Connecting to %s:%d %s" % (address, port, connection_type)) self.addconnection('%s:%d' % (address, port), connection_type) + p2p_conn.p2p_connected_to_node = False p2p_conn.peer_accept_connection(connect_cb=addconnection_callback, connect_id=p2p_idx + 1, net=self.chain, timeout_factor=self.timeout_factor, **kwargs)() if connection_type == "feeler": From 9cfc1c94407359379f10906affd2b837851c1b84 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Wed, 11 Oct 2023 14:28:37 -0400 Subject: [PATCH 20/80] test: check that we don't send a getaddr msg to an inbound peer Co-authored-by: pablomartin4btc --- test/functional/p2p_addr_relay.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/functional/p2p_addr_relay.py b/test/functional/p2p_addr_relay.py index 63cd10896d..2adcaf178c 100755 --- a/test/functional/p2p_addr_relay.py +++ b/test/functional/p2p_addr_relay.py @@ -270,15 +270,16 @@ def getaddr_tests(self): full_outbound_peer.sync_with_ping() assert full_outbound_peer.getaddr_received() - self.log.info('Check that we do not send a getaddr message upon connecting to a block-relay-only peer') + self.log.info('Check that we do not send a getaddr message to a block-relay-only or inbound peer') block_relay_peer = self.nodes[0].add_outbound_p2p_connection(AddrReceiver(), p2p_idx=1, connection_type="block-relay-only") block_relay_peer.sync_with_ping() assert_equal(block_relay_peer.getaddr_received(), False) - self.log.info('Check that we answer getaddr messages only from inbound peers') inbound_peer = self.nodes[0].add_p2p_connection(AddrReceiver(send_getaddr=False)) inbound_peer.sync_with_ping() + assert_equal(inbound_peer.getaddr_received(), False) + self.log.info('Check that we answer getaddr messages only from inbound peers') # Add some addresses to addrman for i in range(1000): first_octet = i >> 8 From ed70e6501648466b9ca91a39b83775363e9a726d Mon Sep 17 00:00:00 2001 From: dergoegge Date: Wed, 19 Jul 2023 15:37:29 +0200 Subject: [PATCH 21/80] Introduce types for txids & wtxids --- src/Makefile.am | 1 + src/net_processing.cpp | 29 +++++++------ src/primitives/transaction.cpp | 16 ++++---- src/primitives/transaction.h | 15 +++---- src/test/fuzz/package_eval.cpp | 2 +- src/test/fuzz/tx_pool.cpp | 4 +- src/util/transaction_identifier.h | 67 +++++++++++++++++++++++++++++++ src/validation.cpp | 4 +- src/wallet/transaction.h | 4 +- 9 files changed, 109 insertions(+), 33 deletions(-) create mode 100644 src/util/transaction_identifier.h diff --git a/src/Makefile.am b/src/Makefile.am index 8905c0ad1c..8508d13b34 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -328,6 +328,7 @@ BITCOIN_CORE_H = \ util/time.h \ util/tokenpipe.h \ util/trace.h \ + util/transaction_identifier.h \ util/translation.h \ util/types.h \ util/ui_change_type.h \ diff --git a/src/net_processing.cpp b/src/net_processing.cpp index dea257f7cc..58bf3ec5d1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1851,9 +1851,9 @@ void PeerManagerImpl::BlockConnected(const std::shared_ptr& pblock { LOCK(m_recent_confirmed_transactions_mutex); for (const auto& ptx : pblock->vtx) { - m_recent_confirmed_transactions.insert(ptx->GetHash()); + m_recent_confirmed_transactions.insert(ptx->GetHash().ToUint256()); if (ptx->HasWitness()) { - m_recent_confirmed_transactions.insert(ptx->GetWitnessHash()); + m_recent_confirmed_transactions.insert(ptx->GetWitnessHash().ToUint256()); } } } @@ -2967,7 +2967,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) // See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034 // for concerns around weakening security of unupgraded nodes // if we start doing this too early. - m_recent_rejects.insert(porphanTx->GetWitnessHash()); + m_recent_rejects.insert(porphanTx->GetWitnessHash().ToUint256()); // If the transaction failed for TX_INPUTS_NOT_STANDARD, // then we know that the witness was irrelevant to the policy // failure, since this check depends only on the txid @@ -2979,7 +2979,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && porphanTx->HasWitness()) { // We only add the txid if it differs from the wtxid, to // avoid wasting entries in the rolling bloom filter. - m_recent_rejects.insert(porphanTx->GetHash()); + m_recent_rejects.insert(porphanTx->GetHash().ToUint256()); } } m_orphanage.EraseTx(orphanHash); @@ -4230,8 +4230,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // regardless of what witness is provided, we will not accept // this, so we don't need to allow for redownload of this txid // from any of our non-wtxidrelay peers. - m_recent_rejects.insert(tx.GetHash()); - m_recent_rejects.insert(tx.GetWitnessHash()); + m_recent_rejects.insert(tx.GetHash().ToUint256()); + m_recent_rejects.insert(tx.GetWitnessHash().ToUint256()); m_txrequest.ForgetTxHash(tx.GetHash()); m_txrequest.ForgetTxHash(tx.GetWitnessHash()); } @@ -4250,7 +4250,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034 // for concerns around weakening security of unupgraded nodes // if we start doing this too early. - m_recent_rejects.insert(tx.GetWitnessHash()); + m_recent_rejects.insert(tx.GetWitnessHash().ToUint256()); m_txrequest.ForgetTxHash(tx.GetWitnessHash()); // If the transaction failed for TX_INPUTS_NOT_STANDARD, // then we know that the witness was irrelevant to the policy @@ -4261,7 +4261,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // transactions are later received (resulting in // parent-fetching by txid via the orphan-handling logic). if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && tx.HasWitness()) { - m_recent_rejects.insert(tx.GetHash()); + m_recent_rejects.insert(tx.GetHash().ToUint256()); m_txrequest.ForgetTxHash(tx.GetHash()); } if (RecursiveDynamicUsage(*ptx) < 100000) { @@ -5694,9 +5694,14 @@ bool PeerManagerImpl::SendMessages(CNode* pto) LOCK(tx_relay->m_bloom_filter_mutex); for (const auto& txinfo : vtxinfo) { - const uint256& hash = peer->m_wtxid_relay ? txinfo.tx->GetWitnessHash() : txinfo.tx->GetHash(); - CInv inv(peer->m_wtxid_relay ? MSG_WTX : MSG_TX, hash); - tx_relay->m_tx_inventory_to_send.erase(hash); + CInv inv{ + peer->m_wtxid_relay ? MSG_WTX : MSG_TX, + peer->m_wtxid_relay ? + txinfo.tx->GetWitnessHash().ToUint256() : + txinfo.tx->GetHash().ToUint256(), + }; + tx_relay->m_tx_inventory_to_send.erase(inv.hash); + // Don't send transactions that peers will not put into their mempool if (txinfo.fee < filterrate.GetFee(txinfo.vsize)) { continue; @@ -5704,7 +5709,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) if (tx_relay->m_bloom_filter) { if (!tx_relay->m_bloom_filter->IsRelevantAndUpdate(*txinfo.tx)) continue; } - tx_relay->m_tx_inventory_known_filter.insert(hash); + tx_relay->m_tx_inventory_known_filter.insert(inv.hash); vInv.push_back(inv); if (vInv.size() == MAX_INV_SZ) { m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp index 2c913bf432..1ad8345fcb 100644 --- a/src/primitives/transaction.cpp +++ b/src/primitives/transaction.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include @@ -65,22 +66,23 @@ std::string CTxOut::ToString() const CMutableTransaction::CMutableTransaction() : nVersion(CTransaction::CURRENT_VERSION), nLockTime(0) {} CMutableTransaction::CMutableTransaction(const CTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime) {} -uint256 CMutableTransaction::GetHash() const +Txid CMutableTransaction::GetHash() const { - return (CHashWriter{SERIALIZE_TRANSACTION_NO_WITNESS} << *this).GetHash(); + return Txid::FromUint256((CHashWriter{SERIALIZE_TRANSACTION_NO_WITNESS} << *this).GetHash()); } -uint256 CTransaction::ComputeHash() const +Txid CTransaction::ComputeHash() const { - return (CHashWriter{SERIALIZE_TRANSACTION_NO_WITNESS} << *this).GetHash(); + return Txid::FromUint256((CHashWriter{SERIALIZE_TRANSACTION_NO_WITNESS} << *this).GetHash()); } -uint256 CTransaction::ComputeWitnessHash() const +Wtxid CTransaction::ComputeWitnessHash() const { if (!HasWitness()) { - return hash; + return Wtxid::FromUint256(hash.ToUint256()); } - return (CHashWriter{0} << *this).GetHash(); + + return Wtxid::FromUint256((CHashWriter{0} << *this).GetHash()); } CTransaction::CTransaction(const CMutableTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime), hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {} diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index bd7eb16bec..2516647a84 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -11,6 +11,7 @@ #include