[pull] master from bitcoin:master

src/.clang-tidy

@@ -10,6 +10,7 @@ bugprone-unhandled-self-assignment,
 bugprone-unused-return-value,
 misc-unused-using-decls,
 misc-no-recursion,
+modernize-avoid-bind,
 modernize-deprecated-headers,
 modernize-use-default-member-init,
 modernize-use-emplace,

src/chain.h

@@ -428,6 +428,15 @@ class CChain
         return int(vChain.size()) - 1;
     }
 
+    /** Check whether this chain's tip exists, has enough work, and is recent. */
+    bool IsTipRecent(const arith_uint256& min_chain_work, std::chrono::seconds max_tip_age) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
+    {
+        const auto tip{Tip()};
+        return tip &&
+               tip->nChainWork >= min_chain_work &&
+               tip->Time() >= Now<NodeSeconds>() - max_tip_age;
+    }
+
     /** Set/initialize a chain with a given tip. */
     void SetTip(CBlockIndex& block);
 

src/init.cpp

@@ -1978,6 +1978,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         ScheduleBatchPriority();
         // Import blocks and ActivateBestChain()
         ImportBlocks(chainman, vImportFiles);
+        WITH_LOCK(::cs_main, chainman.UpdateIBDStatus());
         if (args.GetBoolArg("-stopafterblockimport", DEFAULT_STOPAFTERBLOCKIMPORT)) {
             LogInfo("Stopping after block import");
             if (!(Assert(node.shutdown_request))()) {

src/kernel/bitcoinkernel.cpp

@@ -1076,7 +1076,9 @@ int btck_chainstate_manager_import_blocks(btck_ChainstateManager* chainman, cons
                 import_files.emplace_back(std::string{block_file_paths_data[i], block_file_paths_lens[i]}.c_str());
             }
         }
-        node::ImportBlocks(*btck_ChainstateManager::get(chainman).m_chainman, import_files);
+        auto& chainman_ref{*btck_ChainstateManager::get(chainman).m_chainman};
+        node::ImportBlocks(chainman_ref, import_files);
+        WITH_LOCK(::cs_main, chainman_ref.UpdateIBDStatus());
     } catch (const std::exception& e) {
         LogError("Failed to import blocks: %s", e.what());
         return -1;

src/qt/splashscreen.cpp

@@ -180,8 +180,12 @@ static void ShowProgress(SplashScreen *splash, const std::string &title, int nPr
 void SplashScreen::subscribeToCoreSignals()
 {
     // Connect signals to client
-    m_handler_init_message = m_node->handleInitMessage(std::bind(InitMessage, this, std::placeholders::_1));
-    m_handler_show_progress = m_node->handleShowProgress(std::bind(ShowProgress, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3));
+    m_handler_init_message = m_node->handleInitMessage([this](const std::string& message) {
+        InitMessage(this, message);
+    });
+    m_handler_show_progress = m_node->handleShowProgress([this](const std::string& title, int nProgress, bool resume_possible) {
+        ShowProgress(this, title, nProgress, resume_possible);
+    });
     m_handler_init_wallet = m_node->handleInitWallet([this]() { handleLoadWallet(); });
 }
 
@@ -190,7 +194,9 @@ void SplashScreen::handleLoadWallet()
 #ifdef ENABLE_WALLET
     if (!WalletModel::isWalletEnabled()) return;
     m_handler_load_wallet = m_node->walletLoader().handleLoadWallet([this](std::unique_ptr<interfaces::Wallet> wallet) {
-        m_connected_wallet_handlers.emplace_back(wallet->handleShowProgress(std::bind(ShowProgress, this, std::placeholders::_1, std::placeholders::_2, false)));
+        m_connected_wallet_handlers.emplace_back(wallet->handleShowProgress([this](const std::string& title, int nProgress) {
+            ShowProgress(this, title, nProgress, /*resume_possible=*/false);
+        }));
         m_connected_wallets.emplace_back(std::move(wallet));
     });
 #endif

src/qt/transactiontablemodel.cpp

@@ -712,7 +712,9 @@ void TransactionTablePriv::DispatchNotifications()
 void TransactionTableModel::subscribeToCoreSignals()
 {
     // Connect signals to wallet
-    m_handler_transaction_changed = walletModel->wallet().handleTransactionChanged(std::bind(&TransactionTablePriv::NotifyTransactionChanged, priv, std::placeholders::_1, std::placeholders::_2));
+    m_handler_transaction_changed = walletModel->wallet().handleTransactionChanged([this](const Txid& hash, ChangeType status) {
+        priv->NotifyTransactionChanged(hash, status);
+    });
     m_handler_show_progress = walletModel->wallet().handleShowProgress([this](const std::string&, int progress) {
         priv->m_loading = progress < 100;
         priv->DispatchNotifications();

src/test/fuzz/block_index_tree.cpp

@@ -207,7 +207,7 @@ FUZZ_TARGET(block_index_tree, .init = initialize_block_index_tree)
         chainman.nBlockSequenceId = 2;
         chainman.ActiveChain().SetTip(*genesis);
         chainman.ActiveChainstate().setBlockIndexCandidates.clear();
-        chainman.m_cached_finished_ibd = false;
+        chainman.m_cached_is_ibd = true;
         blockman.m_blocks_unlinked.clear();
         blockman.m_have_pruned = false;
         blockman.CleanupForFuzzing();

src/test/util/validation.cpp

@@ -32,14 +32,14 @@ void TestChainstateManager::DisableNextWrite()
 
 void TestChainstateManager::ResetIbd()
 {
-    m_cached_finished_ibd = false;
+    m_cached_is_ibd = true;
     assert(IsInitialBlockDownload());
 }
 
 void TestChainstateManager::JumpOutOfIbd()
 {
     Assert(IsInitialBlockDownload());
-    m_cached_finished_ibd = true;
+    m_cached_is_ibd = false;
     Assert(!IsInitialBlockDownload());
 }
 

src/test/validation_chainstatemanager_tests.cpp

@@ -143,11 +143,11 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_rebalance_caches, TestChain100Setup)
         /*cache_size_bytes=*/1 << 23, /*in_memory=*/true, /*should_wipe=*/false);
 
     // Reset IBD state so IsInitialBlockDownload() returns true and causes
-    // MaybeRebalancesCaches() to prioritize the snapshot chainstate, giving it
+    // MaybeRebalanceCaches() to prioritize the snapshot chainstate, giving it
     // more cache space than the snapshot chainstate. Calling ResetIbd() is
-    // necessary because m_cached_finished_ibd is already latched to true before
-    // the test starts due to the test setup. After ResetIbd() is called.
-    // IsInitialBlockDownload will return true because at this point the active
+    // necessary because m_cached_is_ibd is already latched to false before
+    // the test starts due to the test setup. After ResetIbd() is called,
+    // IsInitialBlockDownload() will return true because at this point the active
     // chainstate has a null chain tip.
     static_cast<TestChainstateManager&>(manager).ResetIbd();
 
@@ -163,6 +163,44 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_rebalance_caches, TestChain100Setup)
     BOOST_CHECK_CLOSE(double(c2.m_coinsdb_cache_size_bytes), max_cache * 0.95, 1);
 }
 
+BOOST_FIXTURE_TEST_CASE(chainstatemanager_ibd_exit_after_loading_blocks, ChainTestingSetup)
+{
+    CBlockIndex tip;
+    ChainstateManager& chainman{*Assert(m_node.chainman)};
+    auto apply{[&](bool cached_is_ibd, bool loading_blocks, bool tip_exists, bool enough_work, bool tip_recent) {
+        LOCK(::cs_main);
+        chainman.ResetChainstates();
+        chainman.InitializeChainstate(m_node.mempool.get());
+
+        const auto recent_time{Now<NodeSeconds>() - chainman.m_options.max_tip_age};
+
+        chainman.m_cached_is_ibd.store(cached_is_ibd, std::memory_order_relaxed);
+        chainman.m_blockman.m_importing = loading_blocks;
+        if (tip_exists) {
+            tip.nChainWork = chainman.MinimumChainWork() - (enough_work ? 0 : 1);
+            tip.nTime = (recent_time - (tip_recent ? 0h : 100h)).time_since_epoch().count();
+            chainman.ActiveChain().SetTip(tip);
+        } else {
+            assert(!chainman.ActiveChain().Tip());
+        }
+        chainman.UpdateIBDStatus();
+    }};
+
+    for (const bool cached_is_ibd : {false, true}) {
+        for (const bool loading_blocks : {false, true}) {
+            for (const bool tip_exists : {false, true}) {
+                for (const bool enough_work : {false, true}) {
+                    for (const bool tip_recent : {false, true}) {
+                        apply(cached_is_ibd, loading_blocks, tip_exists, enough_work, tip_recent);
+                        const bool expected_ibd = cached_is_ibd && (loading_blocks || !tip_exists || !enough_work || !tip_recent);
+                        BOOST_CHECK_EQUAL(chainman.IsInitialBlockDownload(), expected_ibd);
+                    }
+                }
+            }
+        }
+    }
+}
+
 struct SnapshotTestSetup : TestChain100Setup {
     // Run with coinsdb on the filesystem to support, e.g., moving invalidated
     // chainstate dirs to "*_invalid".

src/validation.cpp

@@ -1934,36 +1934,15 @@ void Chainstate::InitCoinsCache(size_t cache_size_bytes)
     m_coins_views->InitCache();
 }
 
-// Note that though this is marked const, we may end up modifying `m_cached_finished_ibd`, which
-// is a performance-related implementation detail. This function must be marked
-// `const` so that `CValidationInterface` clients (which are given a `const Chainstate*`)
-// can call it.
+// This function must be marked `const` so that `CValidationInterface` clients
+// (which are given a `const Chainstate*`) can call it.
+//
+// It is lock-free and depends on `m_cached_is_ibd`, which is latched by
+// `UpdateIBDStatus()`.
 //
 bool ChainstateManager::IsInitialBlockDownload() const
 {
-    // Optimization: pre-test latch before taking the lock.
-    if (m_cached_finished_ibd.load(std::memory_order_relaxed))
-        return false;
-
-    LOCK(cs_main);
-    if (m_cached_finished_ibd.load(std::memory_order_relaxed))
-        return false;
-    if (m_blockman.LoadingBlocks()) {
-        return true;
-    }
-    CChain& chain{ActiveChain()};
-    if (chain.Tip() == nullptr) {
-        return true;
-    }
-    if (chain.Tip()->nChainWork < MinimumChainWork()) {
-        return true;
-    }
-    if (chain.Tip()->Time() < Now<NodeSeconds>() - m_options.max_tip_age) {
-        return true;
-    }
-    LogInfo("Leaving InitialBlockDownload (latching to false)");
-    m_cached_finished_ibd.store(true, std::memory_order_relaxed);
-    return false;
+    return m_cached_is_ibd.load(std::memory_order_relaxed);
 }
 
 void Chainstate::CheckForkWarningConditions()
@@ -3007,6 +2986,7 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra
     }
 
     m_chain.SetTip(*pindexDelete->pprev);
+    m_chainman.UpdateIBDStatus();
 
     UpdateTip(pindexDelete->pprev);
     // Let wallets know transactions went from 1-confirmed to
@@ -3136,6 +3116,7 @@ bool Chainstate::ConnectTip(
     }
     // Update m_chain & related variables.
     m_chain.SetTip(*pindexNew);
+    m_chainman.UpdateIBDStatus();
     UpdateTip(pindexNew);
 
     const auto time_6{SteadyClock::now()};
@@ -3339,6 +3320,15 @@ static SynchronizationState GetSynchronizationState(bool init, bool blockfiles_i
     return SynchronizationState::INIT_DOWNLOAD;
 }
 
+void ChainstateManager::UpdateIBDStatus()
+{
+    if (!m_cached_is_ibd.load(std::memory_order_relaxed)) return;
+    if (m_blockman.LoadingBlocks()) return;
+    if (!CurrentChainstate().m_chain.IsTipRecent(MinimumChainWork(), m_options.max_tip_age)) return;
+    LogInfo("Leaving InitialBlockDownload (latching to false)");
+    m_cached_is_ibd.store(false, std::memory_order_relaxed);
+}
+
 bool ChainstateManager::NotifyHeaderTip()
 {
     bool fNotify = false;
@@ -4621,6 +4611,7 @@ bool Chainstate::LoadChainTip()
         return false;
     }
     m_chain.SetTip(*pindex);
+    m_chainman.UpdateIBDStatus();
     tip = m_chain.Tip();
 
     // Make sure our chain tip before shutting down scores better than any other candidate

src/validation.h

@@ -1030,13 +1030,13 @@ class ChainstateManager
     ValidationCache m_validation_cache;
 
     /**
-     * Whether initial block download has ended and IsInitialBlockDownload
-     * should return false from now on.
+     * Whether initial block download (IBD) is ongoing.
      *
-     * Mutable because we need to be able to mark IsInitialBlockDownload()
-     * const, which latches this for caching purposes.
+     * This value is used for lock-free IBD checks, and latches from true to
+     * false once block loading has finished and the current chain tip has
+     * enough work and is recent.
      */
-    mutable std::atomic<bool> m_cached_finished_ibd{false};
+    std::atomic_bool m_cached_is_ibd{true};
 
     /**
      * Every received block is assigned a unique and increasing identifier, so we
@@ -1157,6 +1157,19 @@ class ChainstateManager
     CBlockIndex* ActiveTip() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChain().Tip(); }
     //! @}
 
+    /**
+     * Update and possibly latch the IBD status.
+     *
+     * If block loading has finished and the current chain tip has enough work
+     * and is recent, set `m_cached_is_ibd` to false. This function never sets
+     * the flag back to true.
+     *
+     * This should be called after operations that may affect IBD exit
+     * conditions (e.g. after updating the active chain tip, or after
+     * `ImportBlocks()` finishes).
+     */
+    void UpdateIBDStatus() EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+
     node::BlockMap& BlockIndex() EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
     {
         AssertLockHeld(::cs_main);

src/wallet/wallet.cpp

@@ -3550,7 +3550,9 @@ void CWallet::ConnectScriptPubKeyManNotifiers()
 {
     for (const auto& spk_man : GetActiveScriptPubKeyMans()) {
         spk_man->NotifyCanGetAddressesChanged.connect(NotifyCanGetAddressesChanged);
-        spk_man->NotifyFirstKeyTimeChanged.connect(std::bind(&CWallet::MaybeUpdateBirthTime, this, std::placeholders::_2));
+        spk_man->NotifyFirstKeyTimeChanged.connect([this](const ScriptPubKeyMan*, int64_t time) {
+            MaybeUpdateBirthTime(time);
+        });
     }
 }
 

test/functional/test_framework/netutil.py

@@ -181,3 +181,22 @@ def format_addr_port(addr, port):
         return f"[{addr}]:{port}"
     else:
         return f"{addr}:{port}"
+
+
+def set_ephemeral_port_range(sock):
+    '''On FreeBSD, set socket to use the high ephemeral port range (49152-65535).
+
+    FreeBSD's default ephemeral port range (10000-65535) overlaps with the test
+    framework's static port range starting at TEST_RUNNER_PORT_MIN (default=11000).
+    Using IP_PORTRANGE_HIGH avoids this overlap when binding to port 0 for dynamic
+    port allocation.
+    '''
+    if sys.platform.startswith('freebsd'):
+        # Constants from FreeBSD's netinet/in.h and netinet6/in6.h
+        IP_PORTRANGE = 19
+        IPV6_PORTRANGE = 14
+        IP_PORTRANGE_HIGH = 1  # Same value for both IPv4 and IPv6
+        if sock.family == socket.AF_INET6:
+            sock.setsockopt(socket.IPPROTO_IPV6, IPV6_PORTRANGE, IP_PORTRANGE_HIGH)
+        else:
+            sock.setsockopt(socket.IPPROTO_IP, IP_PORTRANGE, IP_PORTRANGE_HIGH)

test/functional/test_framework/p2p.py

@@ -22,9 +22,11 @@
 
 import asyncio
 from collections import defaultdict
+import ipaddress
 from io import BytesIO
 import logging
 import platform
+import socket
 import struct
 import sys
 import threading
@@ -76,6 +78,9 @@
     MAGIC_BYTES,
     sha256,
 )
+from test_framework.netutil import (
+    set_ephemeral_port_range,
+)
 from test_framework.util import (
     assert_not_equal,
     MAX_NODES,
@@ -793,8 +798,22 @@ def peer_protocol():
             # connections, we can accomplish this by providing different
             # `proto` functions
 
-            listener = await cls.network_event_loop.create_server(peer_protocol, addr, port)
-            port = listener.sockets[0].getsockname()[1]
+            if port == 0:
+                # Manually create the socket in order to set the range to be
+                # used for the port before the bind() call.
+                if ipaddress.ip_address(addr).version == 4:
+                    address_family = socket.AF_INET
+                else:
+                    address_family = socket.AF_INET6
+                s = socket.socket(address_family)
+                set_ephemeral_port_range(s)
+                s.bind((addr, 0))
+                s.listen()
+                listener = await cls.network_event_loop.create_server(peer_protocol, sock=s)
+                port = listener.sockets[0].getsockname()[1]
+            else:
+                listener = await cls.network_event_loop.create_server(peer_protocol, addr, port)
+
             logger.debug("Listening server on %s:%d should be started" % (addr, port))
             cls.listeners[(addr, port)] = listener
 

test/functional/test_framework/socks5.py

@@ -11,7 +11,8 @@
 import logging
 
 from .netutil import (
-    format_addr_port
+    format_addr_port,
+    set_ephemeral_port_range,
 )
 
 logger = logging.getLogger("TestFramework.socks5")
@@ -202,6 +203,10 @@ def __init__(self, conf):
         self.conf = conf
         self.s = socket.socket(conf.af)
         self.s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
+        # When using dynamic port allocation (port=0), ensure we don't get a
+        # port that conflicts with the test framework's static port range.
+        if conf.addr[1] == 0:
+            set_ephemeral_port_range(self.s)
         self.s.bind(conf.addr)
         # When port=0, the OS assigns an available port. Update conf.addr
         # to reflect the actual bound address so callers can use it.