Merge #6260: backport: merge bitcoin#20018, #22141, #22221, #22653, #23054, #23398, #23175, #22362, #23769, #23936, #24238, #24331 (auxiliary backports: part 15)

6c7335e002 merge bitcoin#24331: Revert back `MoveFileExW` call for MinGW-w64 (Kittywhiskers Van Gogh)
15e794bdd8 merge bitcoin#24238: use arc4random on OpenBSD (Kittywhiskers Van Gogh)
e039aecbdc merge bitcoin#23936: Add and use EnsureArgsman helper (Kittywhiskers Van Gogh)
b4bfacfd24 merge bitcoin#23769: Disallow copies of CChain (Kittywhiskers Van Gogh)
5b66688160 merge bitcoin#22362: Drop only invalid entries when reading banlist.json (Kittywhiskers Van Gogh)
109c963f6a merge bitcoin#23175: Add CJDNS network to -addrinfo and -netinfo (Kittywhiskers Van Gogh)
d57c96ea37 merge bitcoin#23398: add return message to savemempool RPC (Kittywhiskers Van Gogh)
22e59fb464 merge bitcoin#23054: Use C++11 member initializer in CTxMemPoolEntry (Kittywhiskers Van Gogh)
d158063b6d merge bitcoin#22653: Rename JoinErrors and re-use it (Kittywhiskers Van Gogh)
e24324d266 merge bitcoin#22221: Pass block reference instead of pointer to PeerManagerImpl::BlockRequested (Kittywhiskers Van Gogh)
68657efc03 merge bitcoin#22141: net processing: Remove hash and fValidatedHeaders from QueuedBlock (Kittywhiskers Van Gogh)
c0e6792e27 merge bitcoin#20018: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * When backporting [bitcoin#23054](https://github.com/bitcoin/bitcoin/pull/23054), `sigOpCount` and `nSigOpCountWithAncestors` were switched from `unsigned int` to `int64_t`. This change was done to prevent integer narrowing when accepting the `int64_t` taken from the constructor.

    This isn't a problem upstream as the same changes were as part of [bitcoin#8149](https://github.com/bitcoin/bitcoin/pull/8149/files#diff-8a2230436880b65a205db9299ab2e4e4adb1d4069146791b5db566f3fb752adaL90-L107), which was omitted but the type changes remain valid as sigop count cannot be a negative number.

  ## Breaking Changes

  None observed.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK 6c7335e002
  UdjinM6:
    utACK 6c7335e002

Tree-SHA512: 29cae42dd82235305d3558562bae346170e742ce0b65897e396b331294b39cad0dd831fa9a09b34780a67844e55292e5b4e784cf544a894cc3f8897afe617ca1

configure.ac

@@ -1171,13 +1171,6 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <unistd.h>
  [ AC_MSG_RESULT(no)]
 )
 
-AC_MSG_CHECKING(for getentropy)
-AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <unistd.h>]],
- [[ getentropy(nullptr, 32) ]])],
- [ AC_MSG_RESULT(yes); AC_DEFINE(HAVE_GETENTROPY, 1,[Define this symbol if the BSD getentropy system call is available]) ],
- [ AC_MSG_RESULT(no)]
-)
-
 AC_MSG_CHECKING(for getentropy via random.h)
 AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <unistd.h>
  #include <sys/random.h>]],

src/Makefile.test.include

@@ -77,6 +77,7 @@ BITCOIN_TESTS =\
   test/addrman_tests.cpp \
   test/amount_tests.cpp \
   test/allocator_tests.cpp \
+  test/banman_tests.cpp \
   test/base32_tests.cpp \
   test/base58_tests.cpp \
   test/base64_tests.cpp \

src/bitcoin-cli.cpp

@@ -52,6 +52,7 @@ static constexpr int DEFAULT_WAIT_CLIENT_TIMEOUT = 0;
 static const bool DEFAULT_NAMED=false;
 static const int CONTINUE_EXECUTION=-1;
 static constexpr int8_t UNKNOWN_NETWORK{-1};
+static constexpr std::array NETWORKS{"ipv4", "ipv6", "onion", "i2p", "cjdns"};
 
 /** Default number of blocks to generate for RPC generatetoaddress. */
 static const std::string DEFAULT_NBLOCKS = "1";
@@ -259,11 +260,10 @@ public:
 class AddrinfoRequestHandler : public BaseRequestHandler
 {
 private:
-    static constexpr std::array m_networks{"ipv4", "ipv6", "onion", "i2p"};
     int8_t NetworkStringToId(const std::string& str) const
     {
-        for (size_t i = 0; i < m_networks.size(); ++i) {
+        for (size_t i = 0; i < NETWORKS.size(); ++i) {
-            if (str == m_networks.at(i)) return i;
+            if (str == NETWORKS[i]) return i;
         }
         return UNKNOWN_NETWORK;
     }
@@ -286,7 +286,7 @@ public:
             throw std::runtime_error("-addrinfo requires dashd server to be running v21.0 and up");
         }
         // Count the number of peers known to our node, by network.
-        std::array<uint64_t, m_networks.size()> counts{{}};
+        std::array<uint64_t, NETWORKS.size()> counts{{}};
         for (const UniValue& node : nodes) {
             std::string network_name{node["network"].get_str()};
             const int8_t network_id{NetworkStringToId(network_name)};
@@ -296,8 +296,8 @@ public:
         // Prepare result to return to user.
         UniValue result{UniValue::VOBJ}, addresses{UniValue::VOBJ};
         uint64_t total{0}; // Total address count
-        for (size_t i = 0; i < m_networks.size(); ++i) {
+        for (size_t i = 0; i < NETWORKS.size(); ++i) {
-            addresses.pushKV(m_networks.at(i), counts.at(i));
+            addresses.pushKV(NETWORKS[i], counts.at(i));
             total += counts.at(i);
         }
         addresses.pushKV("total", total);
@@ -384,14 +384,13 @@ class NetinfoRequestHandler : public BaseRequestHandler
 {
 private:
     static constexpr uint8_t MAX_DETAIL_LEVEL{4};
-    static constexpr std::array m_networks{"ipv4", "ipv6", "onion", "i2p"};
+    std::array<std::array<uint16_t, NETWORKS.size() + 1>, 3> m_counts{{{}}}; //!< Peer counts by (in/out/total, networks/total)
-    std::array<std::array<uint16_t, m_networks.size() + 1>, 3> m_counts{{{}}}; //!< Peer counts by (in/out/total, networks/total)
     uint8_t m_block_relay_peers_count{0};
     uint8_t m_manual_peers_count{0};
     int8_t NetworkStringToId(const std::string& str) const
     {
-        for (size_t i = 0; i < m_networks.size(); ++i) {
+        for (size_t i = 0; i < NETWORKS.size(); ++i) {
-            if (str == m_networks.at(i)) return i;
+            if (str == NETWORKS[i]) return i;
         }
         return UNKNOWN_NETWORK;
     }
@@ -493,9 +492,9 @@ public:
             const bool is_block_relay{peer["relaytxes"].isNull() ? false : !peer["relaytxes"].get_bool()};
             const std::string conn_type{peer["connection_type"].get_str()};
             ++m_counts.at(is_outbound).at(network_id);        // in/out by network
-            ++m_counts.at(is_outbound).at(m_networks.size()); // in/out overall
+            ++m_counts.at(is_outbound).at(NETWORKS.size());   // in/out overall
             ++m_counts.at(2).at(network_id);                  // total by network
-            ++m_counts.at(2).at(m_networks.size());           // total overall
+            ++m_counts.at(2).at(NETWORKS.size());             // total overall
             if (is_block_relay) ++m_block_relay_peers_count;
             if (conn_type == "manual") ++m_manual_peers_count;
             if (DetailsRequested()) {
@@ -592,7 +591,7 @@ public:
             for (int8_t n : reachable_networks) {
                 result += strprintf("%8i", m_counts.at(i).at(n)); // network peers count
             }
-            result += strprintf("   %5i", m_counts.at(i).at(m_networks.size())); // total peers count
+            result += strprintf("   %5i", m_counts.at(i).at(NETWORKS.size())); // total peers count
             if (i == 1) { // the outbound row has two extra columns for block relay and manual peer counts
                 result += strprintf("   %5i", m_block_relay_peers_count);
                 if (m_manual_peers_count) result += strprintf("   %5i", m_manual_peers_count);

src/chain.h

@@ -59,37 +59,40 @@ public:
         READWRITE(VARINT(obj.nTimeLast));
     }
 
-     void SetNull() {
+    void SetNull()
-         nBlocks = 0;
+    {
-         nSize = 0;
+        nBlocks = 0;
-         nUndoSize = 0;
+        nSize = 0;
-         nHeightFirst = 0;
+        nUndoSize = 0;
-         nHeightLast = 0;
+        nHeightFirst = 0;
-         nTimeFirst = 0;
+        nHeightLast = 0;
-         nTimeLast = 0;
+        nTimeFirst = 0;
-     }
+        nTimeLast = 0;
+    }
 
-     CBlockFileInfo() {
+    CBlockFileInfo()
-         SetNull();
+    {
-     }
+        SetNull();
+    }
 
-     std::string ToString() const;
+    std::string ToString() const;
 
-     /** update statistics (does not update nSize) */
+    /** update statistics (does not update nSize) */
-     void AddBlock(unsigned int nHeightIn, uint64_t nTimeIn) {
+    void AddBlock(unsigned int nHeightIn, uint64_t nTimeIn)
-         if (nBlocks==0 || nHeightFirst > nHeightIn)
+    {
-             nHeightFirst = nHeightIn;
+        if (nBlocks == 0 || nHeightFirst > nHeightIn)
-         if (nBlocks==0 || nTimeFirst > nTimeIn)
+            nHeightFirst = nHeightIn;
-             nTimeFirst = nTimeIn;
+        if (nBlocks == 0 || nTimeFirst > nTimeIn)
-         nBlocks++;
+            nTimeFirst = nTimeIn;
-         if (nHeightIn > nHeightLast)
+        nBlocks++;
-             nHeightLast = nHeightIn;
+        if (nHeightIn > nHeightLast)
-         if (nTimeIn > nTimeLast)
+            nHeightLast = nHeightIn;
-             nTimeLast = nTimeIn;
+        if (nTimeIn > nTimeLast)
-     }
+            nTimeLast = nTimeIn;
+    }
 };
 
-enum BlockStatus: uint32_t {
+enum BlockStatus : uint32_t {
     //! Unused.
     BLOCK_VALID_UNKNOWN      =    0,
 
@@ -226,7 +229,7 @@ public:
         FlatFilePos ret;
         if (nStatus & BLOCK_HAVE_DATA) {
             ret.nFile = nFile;
-            ret.nPos  = nDataPos;
+            ret.nPos = nDataPos;
         }
         return ret;
     }
@@ -237,7 +240,7 @@ public:
         FlatFilePos ret;
         if (nStatus & BLOCK_HAVE_UNDO) {
             ret.nFile = nFile;
-            ret.nPos  = nUndoPos;
+            ret.nPos = nUndoPos;
         }
         return ret;
     }
@@ -245,13 +248,13 @@ public:
     CBlockHeader GetBlockHeader() const
     {
         CBlockHeader block;
-        block.nVersion       = nVersion;
+        block.nVersion = nVersion;
         if (pprev)
             block.hashPrevBlock = pprev->GetBlockHash();
         block.hashMerkleRoot = hashMerkleRoot;
-        block.nTime          = nTime;
+        block.nTime = nTime;
-        block.nBits          = nBits;
+        block.nBits = nBits;
-        block.nNonce         = nNonce;
+        block.nNonce = nNonce;
         return block;
     }
 
@@ -293,7 +296,7 @@ public:
             *(--pbegin) = pindex->GetBlockTime();
 
         std::sort(pbegin, pend);
-        return pbegin[(pend - pbegin)/2];
+        return pbegin[(pend - pbegin) / 2];
     }
 
     std::string ToString() const;
@@ -360,12 +363,14 @@ public:
     uint256 hash;
     uint256 hashPrev;
 
-    CDiskBlockIndex() {
+    CDiskBlockIndex()
+    {
         hash = uint256();
         hashPrev = uint256();
     }
 
-    explicit CDiskBlockIndex(const CBlockIndex* pindex) : CBlockIndex(*pindex) {
+    explicit CDiskBlockIndex(const CBlockIndex* pindex) : CBlockIndex(*pindex)
+    {
         hash = (hash == uint256() ? pindex->GetBlockHash() : hash);
         hashPrev = (pprev ? pprev->GetBlockHash() : uint256());
     }
@@ -399,12 +404,12 @@ public:
         if(hash != uint256()) return hash;
         // should never really get here, keeping this as a fallback
         CBlockHeader block;
-        block.nVersion        = nVersion;
+        block.nVersion = nVersion;
-        block.hashPrevBlock   = hashPrev;
+        block.hashPrevBlock = hashPrev;
-        block.hashMerkleRoot  = hashMerkleRoot;
+        block.hashMerkleRoot = hashMerkleRoot;
-        block.nTime           = nTime;
+        block.nTime = nTime;
-        block.nBits           = nBits;
+        block.nBits = nBits;
-        block.nNonce          = nNonce;
+        block.nNonce = nNonce;
         return block.GetHash();
     }
 
@@ -413,35 +418,45 @@ public:
 };
 
 /** An in-memory indexed chain of blocks. */
-class CChain {
+class CChain
+{
 private:
     std::vector<CBlockIndex*> vChain;
 
 public:
+    CChain() = default;
+    CChain(const CChain&) = delete;
+    CChain& operator=(const CChain&) = delete;
+
     /** Returns the index entry for the genesis block of this chain, or nullptr if none. */
-    CBlockIndex *Genesis() const {
+    CBlockIndex* Genesis() const
+    {
         return vChain.size() > 0 ? vChain[0] : nullptr;
     }
 
     /** Returns the index entry for the tip of this chain, or nullptr if none. */
-    CBlockIndex *Tip() const {
+    CBlockIndex* Tip() const
+    {
         return vChain.size() > 0 ? vChain[vChain.size() - 1] : nullptr;
     }
 
     /** Returns the index entry at a particular height in this chain, or nullptr if no such height exists. */
-    CBlockIndex *operator[](int nHeight) const {
+    CBlockIndex* operator[](int nHeight) const
+    {
         if (nHeight < 0 || nHeight >= (int)vChain.size())
             return nullptr;
         return vChain[nHeight];
     }
 
     /** Efficiently check whether a block is present in this chain. */
-    bool Contains(const CBlockIndex *pindex) const {
+    bool Contains(const CBlockIndex* pindex) const
+    {
         return (*this)[pindex->nHeight] == pindex;
     }
 
     /** Find the successor of a block in this chain, or nullptr if the given index is not found or is the tip. */
-    CBlockIndex *Next(const CBlockIndex *pindex) const {
+    CBlockIndex* Next(const CBlockIndex* pindex) const
+    {
         if (Contains(pindex))
             return (*this)[pindex->nHeight + 1];
         else
@@ -449,18 +464,19 @@ public:
     }
 
     /** Return the maximal height in the chain. Is equal to chain.Tip() ? chain.Tip()->nHeight : -1. */
-    int Height() const {
+    int Height() const
+    {
         return vChain.size() - 1;
     }
 
     /** Set/initialize a chain with a given tip. */
-    void SetTip(CBlockIndex *pindex);
+    void SetTip(CBlockIndex* pindex);
 
     /** Return a CBlockLocator that refers to a block in this chain (by default the tip). */
-    CBlockLocator GetLocator(const CBlockIndex *pindex = nullptr) const;
+    CBlockLocator GetLocator(const CBlockIndex* pindex = nullptr) const;
 
     /** Find the last common block between this chain and a block index entry. */
-    const CBlockIndex *FindFork(const CBlockIndex *pindex) const;
+    const CBlockIndex* FindFork(const CBlockIndex* pindex) const;
 
     /** Find the earliest block with timestamp equal or greater than the given time and height equal or greater than the given height. */
     CBlockIndex* FindEarliestAtLeast(int64_t nTime, int height) const;

src/context.h

@@ -8,6 +8,7 @@
 #include <functional>
 #include <variant>
 
+class ArgsManager;
 class ChainstateManager;
 class CTxMemPool;
 class CBlockPolicyEstimator;
@@ -16,6 +17,7 @@ struct NodeContext;
 struct WalletContext;
 
 using CoreContext = std::variant<std::monostate,
+                                 std::reference_wrapper<ArgsManager>,
                                  std::reference_wrapper<NodeContext>,
                                  std::reference_wrapper<WalletContext>,
                                  std::reference_wrapper<CTxMemPool>,

src/init.cpp

@@ -2439,6 +2439,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         if (connect.size() != 1 || connect[0] != "0") {
             connOptions.m_specified_outgoing = connect;
         }
+        if (!connOptions.m_specified_outgoing.empty() && !connOptions.vSeedNodes.empty()) {
+            LogPrintf("-seednode is ignored when -connect is used\n");
+        }
+
+        if (args.IsArgSet("-dnsseed") && args.GetBoolArg("-dnsseed", DEFAULT_DNSSEED) && args.IsArgSet("-proxy")) {
+            LogPrintf("-dnsseed is ignored when -connect is used and -proxy is specified\n");
+        }
     }
 
     std::string sem_str = args.GetArg("-socketevents", DEFAULT_SOCKETEVENTS);

src/net.cpp

@@ -2471,6 +2471,8 @@ void CConnman::ThreadDNSAddressSeed()
         }
 
         LogPrintf("Loading addresses from DNS seed %s\n", seed);
+        // If -proxy is in use, we make an ADDR_FETCH connection to the DNS resolved peer address
+        // for the base dns seed domain in chainparams
         if (HaveNameProxy()) {
             AddAddrFetch(seed);
         } else {
@@ -2493,8 +2495,9 @@ void CConnman::ThreadDNSAddressSeed()
                 }
                 addrman.Add(vAdd, resolveSource);
             } else {
-                // We now avoid directly using results from DNS Seeds which do not support service bit filtering,
+                // If the seed does not support a subdomain with our desired service bits,
-                // instead using them as a addrfetch to get nodes with our desired service bits.
+                // we make an ADDR_FETCH connection to the DNS resolved peer address for the
+                // base dns seed domain in chainparams
                 AddAddrFetch(seed);
             }
         }
@@ -2589,7 +2592,6 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, CDe
     {
         for (int64_t nLoop = 0;; nLoop++)
         {
-            ProcessAddrFetch();
             for (const std::string& strAddr : connect)
             {
                 CAddress addr(CService(), NODE_NONE);

src/net_processing.cpp

@@ -193,10 +193,10 @@ static constexpr uint64_t CMPCTBLOCKS_VERSION{1};
 namespace {
 /** Blocks that are in flight, and that are in the queue to be downloaded. */
 struct QueuedBlock {
-    uint256 hash;
+    /** BlockIndex. We must have this since we only request blocks when we've already validated the header. */
-    const CBlockIndex* pindex;                               //!< Optional.
+    const CBlockIndex* pindex;
-    bool fValidatedHeaders;                                  //!< Whether this block has validated headers at the time of request.
+    /** Optional, used for CMPCTBLOCK downloads */
-    std::unique_ptr<PartiallyDownloadedBlock> partialBlock;  //!< Optional, used for CMPCTBLOCK downloads
+    std::unique_ptr<PartiallyDownloadedBlock> partialBlock;
 };
 
 /**
@@ -398,7 +398,6 @@ struct CNodeState {
     //! When the first entry in vBlocksInFlight started downloading. Don't care when vBlocksInFlight is empty.
     std::chrono::microseconds m_downloading_since{0us};
     int nBlocksInFlight{0};
-    int nBlocksInFlightValidHeaders{0};
     //! Whether we consider this a preferred download peer.
     bool fPreferredDownload{false};
     //! Whether this peer wants invs or headers (when possible) for block announcements.
@@ -898,16 +897,20 @@ private:
     /** Height of the highest block announced using BIP 152 high-bandwidth mode. */
     int m_highest_fast_announce GUARDED_BY(::cs_main){0};
 
-    /* Returns a bool indicating whether we requested this block.
+    /** Have we requested this block from a peer */
-     * Also used if a block was /not/ received and timed out or started with another peer
+    bool IsBlockRequested(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+
+    /** Remove this block from our tracked requested blocks. Called if:
+     *  - the block has been recieved from a peer
+     *  - the request for the block has timed out
      */
-    bool MarkBlockAsReceived(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    void RemoveBlockRequest(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     /* Mark a block as in flight
      * Returns false, still setting pit, if the block was already in flight from the same peer
      * pit will only be valid as long as the same cs_main lock is being held
      */
-    bool MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlockIndex* pindex = nullptr, std::list<QueuedBlock>::iterator** pit = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    bool BlockRequested(NodeId nodeid, const CBlockIndex& block, std::list<QueuedBlock>::iterator** pit = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     bool TipMayBeStale() EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
@@ -946,7 +949,7 @@ private:
     std::list<NodeId> lNodesAnnouncingHeaderAndIDs GUARDED_BY(cs_main);
 
     /** Number of peers from which we're downloading blocks. */
-    int nPeersWithValidatedDownloads GUARDED_BY(cs_main) = 0;
+    int m_peers_downloading_from GUARDED_BY(cs_main) = 0;
 
     /** Storage for orphan information */
     TxOrphanage m_orphanage;
@@ -1074,32 +1077,42 @@ std::chrono::microseconds PeerManagerImpl::NextInvToInbounds(std::chrono::micros
     return m_next_inv_to_inbounds;
 }
 
-bool PeerManagerImpl::MarkBlockAsReceived(const uint256& hash)
+bool PeerManagerImpl::IsBlockRequested(const uint256& hash)
 {
-    std::map<uint256, std::pair<NodeId, std::list<QueuedBlock>::iterator> >::iterator itInFlight = mapBlocksInFlight.find(hash);
+    return mapBlocksInFlight.find(hash) != mapBlocksInFlight.end();
-    if (itInFlight != mapBlocksInFlight.end()) {
-        CNodeState *state = State(itInFlight->second.first);
-        assert(state != nullptr);
-        state->nBlocksInFlightValidHeaders -= itInFlight->second.second->fValidatedHeaders;
-        if (state->nBlocksInFlightValidHeaders == 0 && itInFlight->second.second->fValidatedHeaders) {
-            // Last validated block on the queue was received.
-            nPeersWithValidatedDownloads--;
-        }
-        if (state->vBlocksInFlight.begin() == itInFlight->second.second) {
-            // First block on the queue was received, update the start download time for the next one
-            state->m_downloading_since = std::max(state->m_downloading_since, GetTime<std::chrono::microseconds>());
-        }
-        state->vBlocksInFlight.erase(itInFlight->second.second);
-        state->nBlocksInFlight--;
-        state->m_stalling_since = 0us;
-        mapBlocksInFlight.erase(itInFlight);
-        return true;
-    }
-    return false;
 }
 
-bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlockIndex *pindex, std::list<QueuedBlock>::iterator **pit)
+void PeerManagerImpl::RemoveBlockRequest(const uint256& hash)
 {
+    auto it = mapBlocksInFlight.find(hash);
+    if (it == mapBlocksInFlight.end()) {
+        // Block was not requested
+        return;
+    }
+
+    auto [node_id, list_it] = it->second;
+    CNodeState *state = State(node_id);
+    assert(state != nullptr);
+
+    if (state->vBlocksInFlight.begin() == list_it) {
+        // First block on the queue was received, update the start download time for the next one
+        state->m_downloading_since = std::max(state->m_downloading_since, GetTime<std::chrono::microseconds>());
+    }
+    state->vBlocksInFlight.erase(list_it);
+
+    state->nBlocksInFlight--;
+    if (state->nBlocksInFlight == 0) {
+        // Last validated block on the queue was received.
+        m_peers_downloading_from--;
+    }
+    state->m_stalling_since = 0us;
+    mapBlocksInFlight.erase(it);
+}
+
+bool PeerManagerImpl::BlockRequested(NodeId nodeid, const CBlockIndex& block, std::list<QueuedBlock>::iterator **pit)
+{
+    const uint256& hash{block.GetBlockHash()};
+
     CNodeState *state = State(nodeid);
     assert(state != nullptr);
 
@@ -1113,22 +1126,20 @@ bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, co
     }
 
     // Make sure it's not listed somewhere already.
-    MarkBlockAsReceived(hash);
+    RemoveBlockRequest(hash);
 
     std::list<QueuedBlock>::iterator it = state->vBlocksInFlight.insert(state->vBlocksInFlight.end(),
-            {hash, pindex, pindex != nullptr, std::unique_ptr<PartiallyDownloadedBlock>(pit ? new PartiallyDownloadedBlock(&m_mempool) : nullptr)});
+            {&block, std::unique_ptr<PartiallyDownloadedBlock>(pit ? new PartiallyDownloadedBlock(&m_mempool) : nullptr)});
     state->nBlocksInFlight++;
-    state->nBlocksInFlightValidHeaders += it->fValidatedHeaders;
     if (state->nBlocksInFlight == 1) {
         // We're starting a block download (batch) from this peer.
         state->m_downloading_since = GetTime<std::chrono::microseconds>();
-    }
+        m_peers_downloading_from++;
-    if (state->nBlocksInFlightValidHeaders == 1 && pindex != nullptr) {
-        nPeersWithValidatedDownloads++;
     }
     itInFlight = mapBlocksInFlight.insert(std::make_pair(hash, std::make_pair(nodeid, it))).first;
-    if (pit)
+    if (pit) {
         *pit = &itInFlight->second.second;
+    }
     return true;
 }
 
@@ -1310,7 +1321,7 @@ void PeerManagerImpl::FindNextBlocksToDownload(const Peer& peer, unsigned int co
             if (pindex->nStatus & BLOCK_HAVE_DATA || m_chainman.ActiveChain().Contains(pindex)) {
                 if (pindex->HaveTxsDownloaded())
                     state->pindexLastCommonBlock = pindex;
-            } else if (mapBlocksInFlight.count(pindex->GetBlockHash()) == 0) {
+            } else if (!IsBlockRequested(pindex->GetBlockHash())) {
                 // The block is not already downloaded, and not yet in flight.
                 if (pindex->nHeight > nWindowEnd) {
                     // We reached the end of the window.
@@ -1565,12 +1576,12 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) {
         nSyncStarted--;
 
     for (const QueuedBlock& entry : state->vBlocksInFlight) {
-        mapBlocksInFlight.erase(entry.hash);
+        mapBlocksInFlight.erase(entry.pindex->GetBlockHash());
     }
     WITH_LOCK(g_cs_orphans, m_orphanage.EraseForPeer(nodeid));
     m_num_preferred_download_peers -= state->fPreferredDownload;
-    nPeersWithValidatedDownloads -= (state->nBlocksInFlightValidHeaders != 0);
+    m_peers_downloading_from -= (state->nBlocksInFlight != 0);
-    assert(nPeersWithValidatedDownloads >= 0);
+    assert(m_peers_downloading_from >= 0);
     m_outbound_peers_with_protect_from_disconnect -= state->m_chain_sync.m_protect;
     assert(m_outbound_peers_with_protect_from_disconnect >= 0);
 
@@ -1580,7 +1591,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) {
         // Do a consistency check after the last peer is removed.
         assert(mapBlocksInFlight.empty());
         assert(m_num_preferred_download_peers == 0);
-        assert(nPeersWithValidatedDownloads == 0);
+        assert(m_peers_downloading_from == 0);
         assert(m_outbound_peers_with_protect_from_disconnect == 0);
     }
     } // cs_main
@@ -1811,10 +1822,10 @@ std::optional<std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, const CBl
     // Mark block as in-flight unless it already is (for this peer).
     // If a block was already in-flight for a different peer, its BLOCKTXN
     // response will be dropped.
-    const uint256& hash{block_index.GetBlockHash()};
+    if (!BlockRequested(peer_id, block_index)) return "Already requested from this peer";
-    if (!MarkBlockAsInFlight(peer_id, hash, &block_index)) return "Already requested from this peer";
 
     // Construct message to request the block
+    const uint256& hash{block_index.GetBlockHash()};
     std::vector<CInv> invs{CInv(MSG_BLOCK, hash)};
 
     // Send block request message to the peer
@@ -2797,7 +2808,7 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c
         // Calculate all the blocks we'd need to switch to pindexLast, up to a limit.
         while (pindexWalk && !m_chainman.ActiveChain().Contains(pindexWalk) && vToFetch.size() <= MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
             if (!(pindexWalk->nStatus & BLOCK_HAVE_DATA) &&
-                    !mapBlocksInFlight.count(pindexWalk->GetBlockHash())) {
+                    !IsBlockRequested(pindexWalk->GetBlockHash())) {
                 // We don't have this block, and it's not yet in flight.
                 vToFetch.push_back(pindexWalk);
             }
@@ -2820,7 +2831,7 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c
                     break;
                 }
                 vGetData.push_back(CInv(MSG_BLOCK, pindex->GetBlockHash()));
-                MarkBlockAsInFlight(pfrom.GetId(), pindex->GetBlockHash(), pindex);
+                BlockRequested(pfrom.GetId(), *pindex);
                 LogPrint(BCLog::NET, "Requesting block %s from  peer=%d\n",
                         pindex->GetBlockHash().ToString(), pfrom.GetId());
             }
@@ -3781,7 +3792,7 @@ void PeerManagerImpl::ProcessMessage(
                 statsClient.inc(strprintf("message.received.inv_%s", inv.GetCommand()), 1.0f);
 
                 UpdateBlockAvailability(pfrom.GetId(), inv.hash);
-                if (!fAlreadyHave && !fImporting && !fReindex && !mapBlocksInFlight.count(inv.hash)) {
+                if (!fAlreadyHave && !fImporting && !fReindex && !IsBlockRequested(inv.hash)) {
                     // Headers-first is the primary method of announcement on
                     // the network. If a node fell back to sending blocks by
                     // inv, it may be for a re-org, or because we haven't
@@ -4361,7 +4372,7 @@ void PeerManagerImpl::ProcessMessage(
             if ((!fAlreadyInFlight && nodestate->nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) ||
                  (fAlreadyInFlight && blockInFlightIt->second.first == pfrom.GetId())) {
                 std::list<QueuedBlock>::iterator *queuedBlockIt = nullptr;
-                if (!MarkBlockAsInFlight(pfrom.GetId(), pindex->GetBlockHash(), pindex, &queuedBlockIt)) {
+                if (!BlockRequested(pfrom.GetId(), *pindex, &queuedBlockIt)) {
                     if (!(*queuedBlockIt)->partialBlock)
                         (*queuedBlockIt)->partialBlock.reset(new PartiallyDownloadedBlock(&m_mempool));
                     else {
@@ -4374,7 +4385,7 @@ void PeerManagerImpl::ProcessMessage(
                 PartiallyDownloadedBlock& partialBlock = *(*queuedBlockIt)->partialBlock;
                 ReadStatus status = partialBlock.InitData(cmpctblock, vExtraTxnForCompact);
                 if (status == READ_STATUS_INVALID) {
-                    MarkBlockAsReceived(pindex->GetBlockHash()); // Reset in-flight state in case Misbehaving does not result in a disconnect
+                    RemoveBlockRequest(pindex->GetBlockHash()); // Reset in-flight state in case Misbehaving does not result in a disconnect
                     Misbehaving(pfrom.GetId(), 100, "invalid compact block");
                     return;
                 } else if (status == READ_STATUS_FAILED) {
@@ -4468,7 +4479,7 @@ void PeerManagerImpl::ProcessMessage(
                 // process from some other peer.  We do this after calling
                 // ProcessNewBlock so that a malleated cmpctblock announcement
                 // can't be used to interfere with block relay.
-                MarkBlockAsReceived(pblock->GetHash());
+                RemoveBlockRequest(pblock->GetHash());
             }
         }
         return;
@@ -4500,7 +4511,7 @@ void PeerManagerImpl::ProcessMessage(
             PartiallyDownloadedBlock& partialBlock = *it->second.second->partialBlock;
             ReadStatus status = partialBlock.FillBlock(*pblock, resp.txn);
             if (status == READ_STATUS_INVALID) {
-                MarkBlockAsReceived(resp.blockhash); // Reset in-flight state in case Misbehaving does not result in a disconnect
+                RemoveBlockRequest(resp.blockhash); // Reset in-flight state in case Misbehaving does not result in a disconnect
                 Misbehaving(pfrom.GetId(), 100, "invalid compact block/non-matching block transactions");
                 return;
             } else if (status == READ_STATUS_FAILED) {
@@ -4526,7 +4537,7 @@ void PeerManagerImpl::ProcessMessage(
                 // though the block was successfully read, and rely on the
                 // handling in ProcessNewBlock to ensure the block index is
                 // updated, etc.
-                MarkBlockAsReceived(resp.blockhash); // it is now an empty pointer
+                RemoveBlockRequest(resp.blockhash); // it is now an empty pointer
                 fBlockRead = true;
                 // mapBlockSource is used for potentially punishing peers and
                 // updating which peers send us compact blocks, so the race
@@ -4605,9 +4616,10 @@ void PeerManagerImpl::ProcessMessage(
         const uint256 hash(pblock->GetHash());
         {
             LOCK(cs_main);
-            // Also always process if we requested the block explicitly, as we may
+            // Always process the block if we requested it, since we may
-            // need it even though it is not a candidate for a new best tip.
+            // need it even when it's not a candidate for a new best tip.
-            forceProcessing |= MarkBlockAsReceived(hash);
+            forceProcessing = IsBlockRequested(hash);
+            RemoveBlockRequest(hash);
             // mapBlockSource is only used for punishing peers and setting
             // which peers send us compact blocks, so the race between here and
             // cs_main in ProcessNewBlock is fine.
@@ -5863,9 +5875,9 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
         // to unreasonably increase our timeout.
         if (state.vBlocksInFlight.size() > 0) {
             QueuedBlock &queuedBlock = state.vBlocksInFlight.front();
-            int nOtherPeersWithValidatedDownloads = nPeersWithValidatedDownloads - (state.nBlocksInFlightValidHeaders > 0);
+            int nOtherPeersWithValidatedDownloads = m_peers_downloading_from - 1;
             if (current_time > state.m_downloading_since + std::chrono::seconds{consensusParams.nPowTargetSpacing} * (BLOCK_DOWNLOAD_TIMEOUT_BASE + BLOCK_DOWNLOAD_TIMEOUT_PER_PEER * nOtherPeersWithValidatedDownloads)) {
-                LogPrintf("Timeout downloading block %s from peer=%d, disconnecting\n", queuedBlock.hash.ToString(), pto->GetId());
+                LogPrintf("Timeout downloading block %s from peer=%d, disconnecting\n", queuedBlock.pindex->GetBlockHash().ToString(), pto->GetId());
                 pto->fDisconnect = true;
                 return true;
             }
@@ -5917,7 +5929,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
             FindNextBlocksToDownload(*peer, MAX_BLOCKS_IN_TRANSIT_PER_PEER - state.nBlocksInFlight, vToDownload, staller);
             for (const CBlockIndex *pindex : vToDownload) {
                 vGetData.push_back(CInv(MSG_BLOCK, pindex->GetBlockHash()));
-                MarkBlockAsInFlight(pto->GetId(), pindex->GetBlockHash(), pindex);
+                BlockRequested(pto->GetId(), *pindex);
                 LogPrint(BCLog::NET, "Requesting block %s (%d) peer=%d\n", pindex->GetBlockHash().ToString(),
                     pindex->nHeight, pto->GetId());
             }

src/net_types.cpp

@@ -4,12 +4,16 @@
 
 #include <net_types.h>
 
+#include <logging.h>
 #include <netaddress.h>
 #include <netbase.h>
 #include <univalue.h>
 
+static const char* BANMAN_JSON_VERSION_KEY{"version"};
+
 CBanEntry::CBanEntry(const UniValue& json)
-    : nVersion(json["version"].get_int()), nCreateTime(json["ban_created"].get_int64()),
+    : nVersion(json[BANMAN_JSON_VERSION_KEY].get_int()),
+      nCreateTime(json["ban_created"].get_int64()),
       nBanUntil(json["banned_until"].get_int64())
 {
 }
@@ -17,7 +21,7 @@ CBanEntry::CBanEntry(const UniValue& json)
 UniValue CBanEntry::ToJson() const
 {
     UniValue json(UniValue::VOBJ);
-    json.pushKV("version", nVersion);
+    json.pushKV(BANMAN_JSON_VERSION_KEY, nVersion);
     json.pushKV("ban_created", nCreateTime);
     json.pushKV("banned_until", nBanUntil);
     return json;
@@ -54,11 +58,16 @@ UniValue BanMapToJson(const banmap_t& bans)
 void BanMapFromJson(const UniValue& bans_json, banmap_t& bans)
 {
     for (const auto& ban_entry_json : bans_json.getValues()) {
+        const int version{ban_entry_json[BANMAN_JSON_VERSION_KEY].get_int()};
+        if (version != CBanEntry::CURRENT_VERSION) {
+            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)) {
-            throw std::runtime_error(
+            LogPrintf("Dropping entry with unparseable address or subnet (%s) from ban list\n", subnet_str);
-                strprintf("Cannot parse banned address or subnet: %s", subnet_str));
+            continue;
         }
         bans.insert_or_assign(subnet, CBanEntry{ban_entry_json});
     }

src/qt/bitcoin.cpp

@@ -22,6 +22,7 @@
 #include <qt/splashscreen.h>
 #include <qt/utilitydialog.h>
 #include <qt/winshutdownmonitor.h>
+#include <util/string.h>
 
 #ifdef ENABLE_WALLET
 #include <qt/paymentserver.h>
@@ -148,11 +149,6 @@ static void initTranslations(QTranslator &qtTranslatorBase, QTranslator &qtTrans
         QApplication::installTranslator(&translator);
 }
 
-static std::string JoinErrors(const std::vector<std::string>& errors)
-{
-    return Join(errors, "\n", [](const std::string& error) { return "- " + error; });
-}
-
 static bool InitSettings()
 {
     if (!gArgs.GetSettingsPath()) {
@@ -162,13 +158,13 @@ static bool InitSettings()
     std::vector<std::string> errors;
     if (!gArgs.ReadSettingsFile(&errors)) {
         bilingual_str error = _("Settings file could not be read");
-        InitError(Untranslated(strprintf("%s:\n%s\n", error.original, JoinErrors(errors))));
+        InitError(Untranslated(strprintf("%s:\n%s\n", error.original, MakeUnorderedList(errors))));
 
         QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error.translated)), QMessageBox::Reset | QMessageBox::Abort);
         /*: Explanatory text shown on startup when the settings file cannot be read.
             Prompts user to make a choice between resetting or aborting. */
         messagebox.setInformativeText(QObject::tr("Do you want to reset settings to default values, or to abort without making changes?"));
-        messagebox.setDetailedText(QString::fromStdString(JoinErrors(errors)));
+        messagebox.setDetailedText(QString::fromStdString(MakeUnorderedList(errors)));
         messagebox.setTextFormat(Qt::PlainText);
         messagebox.setDefaultButton(QMessageBox::Reset);
         switch (messagebox.exec()) {
@@ -184,14 +180,14 @@ static bool InitSettings()
     errors.clear();
     if (!gArgs.WriteSettingsFile(&errors)) {
         bilingual_str error = _("Settings file could not be written");
-        InitError(Untranslated(strprintf("%s:\n%s\n", error.original, JoinErrors(errors))));
+        InitError(Untranslated(strprintf("%s:\n%s\n", error.original, MakeUnorderedList(errors))));
 
         QMessageBox messagebox(QMessageBox::Critical, PACKAGE_NAME, QString::fromStdString(strprintf("%s.", error.translated)), QMessageBox::Ok);
         /*: Explanatory text shown on startup when the settings file could not be written.
             Prompts user to check that we have the ability to write to the file.
             Explains that the user has the option of running without a settings file.*/
-        messagebox.setInformativeText(QObject::tr("A fatal error occured. Check that settings file is writable, or try running with -nosettings."));
+        messagebox.setInformativeText(QObject::tr("A fatal error occurred. Check that settings file is writable, or try running with -nosettings."));
-        messagebox.setDetailedText(QString::fromStdString(JoinErrors(errors)));
+        messagebox.setDetailedText(QString::fromStdString(MakeUnorderedList(errors)));
         messagebox.setTextFormat(Qt::PlainText);
         messagebox.setDefaultButton(QMessageBox::Ok);
         messagebox.exec();

src/random.cpp

@@ -34,10 +34,8 @@
 #include <sys/syscall.h>
 #include <linux/random.h>
 #endif
-#if defined(HAVE_GETENTROPY) || (defined(HAVE_GETENTROPY_RAND) && defined(MAC_OSX))
-#include <unistd.h>
-#endif
 #if defined(HAVE_GETENTROPY_RAND) && defined(MAC_OSX)
+#include <unistd.h>
 #include <sys/random.h>
 #endif
 #ifdef HAVE_SYSCTL_ARND
@@ -307,16 +305,14 @@ void GetOSRand(unsigned char *ent32)
             RandFailure();
         }
     }
-#elif defined(HAVE_GETENTROPY) && defined(__OpenBSD__)
+#elif defined(__OpenBSD__)
-    /* On OpenBSD this can return up to 256 bytes of entropy, will return an
+    /* OpenBSD. From the arc4random(3) man page:
-     * error if more are requested.
+       "Use of these functions is encouraged for almost all random number
-     * The call cannot return less than the requested number of bytes.
+        consumption because the other interfaces are deficient in either
-       getentropy is explicitly limited to openbsd here, as a similar (but not
+        quality, portability, standardization, or availability."
-       the same) function may exist on other platforms via glibc.
+       The function call is always successful.
      */
-    if (getentropy(ent32, NUM_OS_RANDOM_BYTES) != 0) {
+    arc4random_buf(ent32, NUM_OS_RANDOM_BYTES);
-        RandFailure();
-    }
     // Silence a compiler warning about unused function.
     (void)GetDevURandom;
 #elif defined(HAVE_GETENTROPY_RAND) && defined(MAC_OSX)

src/rpc/blockchain.cpp

@@ -40,6 +40,7 @@
 #include <txmempool.h>
 #include <undo.h>
 #include <util/strencodings.h>
+#include <util/string.h>
 #include <util/system.h>
 #include <util/translation.h>
 #include <validation.h>
@@ -1619,7 +1620,7 @@ static RPCHelpMan verifychain()
         "\nVerifies blockchain database.\n",
         {
             {"checklevel", RPCArg::Type::NUM, /* default */ strprintf("%d, range=0-4", DEFAULT_CHECKLEVEL),
-                        strprintf("How thorough the block verification is:\n - %s", Join(CHECKLEVEL_DOC, "\n- "))},
+                        strprintf("How thorough the block verification is:\n - %s", MakeUnorderedList(CHECKLEVEL_DOC))},
             {"nblocks", RPCArg::Type::NUM, /* default */ strprintf("%d, 0=all", DEFAULT_CHECKBLOCKS), "The number of blocks to check."},
         },
         RPCResult{
@@ -1775,9 +1776,8 @@ RPCHelpMan getblockchaininfo()
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
 
-    std::string strChainName = gArgs.IsArgSet("-devnet") ? gArgs.GetDevNetName() : Params().NetworkIDString();
-
     const NodeContext& node = EnsureAnyNodeContext(request.context);
+    const ArgsManager& args{EnsureArgsman(node)};
     ChainstateManager& chainman = EnsureChainman(node);
 
     LOCK(cs_main);
@@ -1791,7 +1791,11 @@ RPCHelpMan getblockchaininfo()
     const auto ehfSignals = node.mnhf_manager->GetSignalsStage(tip);
 
     UniValue obj(UniValue::VOBJ);
-    obj.pushKV("chain", strChainName);
+    if (args.IsArgSet("-devnet")) {
+        obj.pushKV("chain", args.GetDevNetName());
+    } else {
+        obj.pushKV("chain", Params().NetworkIDString());
+    }
     obj.pushKV("blocks", height);
     obj.pushKV("headers", chainman.m_best_header ? chainman.m_best_header->nHeight : -1);
     obj.pushKV("bestblockhash", tip->GetBlockHash().GetHex());
@@ -1813,7 +1817,7 @@ RPCHelpMan getblockchaininfo()
         obj.pushKV("pruneheight",        block->nHeight);
 
         // if 0, execution bypasses the whole if block.
-        bool automatic_pruning = (gArgs.GetArg("-prune", 0) != 1);
+        bool automatic_pruning{args.GetArg("-prune", 0) != 1};
         obj.pushKV("automatic_pruning",  automatic_pruning);
         if (automatic_pruning) {
             obj.pushKV("prune_target_size",  nPruneTarget);
@@ -2642,13 +2646,18 @@ static RPCHelpMan savemempool()
     return RPCHelpMan{"savemempool",
         "\nDumps the mempool to disk. It will fail until the previous dump is fully loaded.\n",
         {},
-        RPCResult{RPCResult::Type::NONE, "", ""},
+        RPCResult{
+            RPCResult::Type::OBJ, "", "",
+            {
+                {RPCResult::Type::STR, "filename", "the directory and file where the mempool was saved"},
+            }},
         RPCExamples{
             HelpExampleCli("savemempool", "")
     + HelpExampleRpc("savemempool", "")
         },
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
+    const ArgsManager& args{EnsureAnyArgsman(request.context)};
     const CTxMemPool& mempool = EnsureAnyMemPool(request.context);
 
     if (!mempool.IsLoaded()) {
@@ -2659,7 +2668,10 @@ static RPCHelpMan savemempool()
         throw JSONRPCError(RPC_MISC_ERROR, "Unable to dump mempool to disk");
     }
 
-    return NullUniValue;
+    UniValue ret(UniValue::VOBJ);
+    ret.pushKV("filename", fs::path((args.GetDataDirNet() / "mempool.dat")).u8string());
+
+    return ret;
 },
     };
 }
@@ -3001,10 +3013,11 @@ static RPCHelpMan dumptxoutset()
         },
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
-    const fs::path path = fsbridge::AbsPathJoin(gArgs.GetDataDirNet(), fs::u8path(request.params[0].get_str()));
+    const ArgsManager& args{EnsureAnyArgsman(request.context)};
+    const fs::path path = fsbridge::AbsPathJoin(args.GetDataDirNet(), fs::u8path(request.params[0].get_str()));
     // Write to a temporary path and then move into `path` on completion
     // to avoid confusion due to an interruption.
-    const fs::path temppath = fsbridge::AbsPathJoin(gArgs.GetDataDirNet(), fs::u8path(request.params[0].get_str() + ".incomplete"));
+    const fs::path temppath = fsbridge::AbsPathJoin(args.GetDataDirNet(), fs::u8path(request.params[0].get_str() + ".incomplete"));
 
     if (fs::exists(path)) {
         throw JSONRPCError(

src/rpc/server_util.cpp

@@ -37,6 +37,19 @@ CTxMemPool& EnsureAnyMemPool(const CoreContext& context)
     return EnsureMemPool(EnsureAnyNodeContext(context));
 }
 
+ArgsManager& EnsureArgsman(const NodeContext& node)
+{
+    if (!node.args) {
+        throw JSONRPCError(RPC_INTERNAL_ERROR, "Node args not found");
+    }
+    return *node.args;
+}
+
+ArgsManager& EnsureAnyArgsman(const CoreContext& context)
+{
+    return EnsureArgsman(EnsureAnyNodeContext(context));
+}
+
 ChainstateManager& EnsureChainman(const NodeContext& node)
 {
     if (!node.chainman) {

src/rpc/server_util.h

@@ -7,10 +7,11 @@
 
 #include <context.h>
 
+class ArgsManager;
 class CBlockPolicyEstimator;
 class CConnman;
-class ChainstateManager;
 class CTxMemPool;
+class ChainstateManager;
 class PeerManager;
 struct NodeContext;
 struct LLMQContext;
@@ -18,6 +19,8 @@ struct LLMQContext;
 NodeContext& EnsureAnyNodeContext(const CoreContext& context);
 CTxMemPool& EnsureMemPool(const NodeContext& node);
 CTxMemPool& EnsureAnyMemPool(const CoreContext& context);
+ArgsManager& EnsureArgsman(const NodeContext& node);
+ArgsManager& EnsureAnyArgsman(const CoreContext& context);
 ChainstateManager& EnsureChainman(const NodeContext& node);
 ChainstateManager& EnsureAnyChainman(const CoreContext& context);
 CBlockPolicyEstimator& EnsureFeeEstimator(const NodeContext& node);

src/test/banman_tests.cpp

@@ -0,0 +1,43 @@
+// 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 <banman.h>
+#include <chainparams.h>
+#include <netbase.h>
+#include <streams.h>
+#include <test/util/logging.h>
+#include <test/util/setup_common.h>
+#include <util/readwritefile.h>
+
+
+#include <boost/test/unit_test.hpp>
+
+BOOST_FIXTURE_TEST_SUITE(banman_tests, BasicTestingSetup)
+
+BOOST_AUTO_TEST_CASE(file)
+{
+    SetMockTime(777s);
+    const fs::path banlist_path{m_args.GetDataDirBase() / "banlist_test"};
+    {
+        const std::string entries_write{
+            "{ \"banned_nets\": ["
+            "  { \"version\": 1, \"ban_created\": 0, \"banned_until\": 778, \"address\": \"aaaaaaaaa\" },"
+            "  { \"version\": 2, \"ban_created\": 0, \"banned_until\": 778, \"address\": \"bbbbbbbbb\" },"
+            "  { \"version\": 1, \"ban_created\": 0, \"banned_until\": 778, \"address\": \"1.0.0.0/8\" }"
+            "] }",
+        };
+        assert(WriteBinaryFile(banlist_path + ".json", entries_write));
+        {
+            // The invalid entries will be dropped, but the valid one remains
+            ASSERT_DEBUG_LOG("Dropping entry with unparseable address or subnet (aaaaaaaaa) from ban list");
+            ASSERT_DEBUG_LOG("Dropping entry with unknown version (2) from ban list");
+            BanMan banman{banlist_path, /*client_interface=*/nullptr, /*default_ban_time=*/0};
+            banmap_t entries_read;
+            banman.GetBanned(entries_read);
+            assert(entries_read.size() == 1);
+        }
+    }
+}
+
+BOOST_AUTO_TEST_SUITE_END()

src/test/fs_tests.cpp

@@ -118,6 +118,40 @@ BOOST_AUTO_TEST_CASE(fsbridge_fstream)
     }
 }
 
+BOOST_AUTO_TEST_CASE(rename)
+{
+    const fs::path tmpfolder{m_args.GetDataDirBase()};
+
+    const fs::path path1{GetUniquePath(tmpfolder)};
+    const fs::path path2{GetUniquePath(tmpfolder)};
+
+    const std::string path1_contents{"1111"};
+    const std::string path2_contents{"2222"};
+
+    {
+        std::ofstream file{path1};
+        file << path1_contents;
+    }
+
+    {
+        std::ofstream file{path2};
+        file << path2_contents;
+    }
+
+    // Rename path1 -> path2.
+    BOOST_CHECK(RenameOver(path1, path2));
+
+    BOOST_CHECK(!fs::exists(path1));
+
+    {
+        std::ifstream file{path2};
+        std::string contents;
+        file >> contents;
+        BOOST_CHECK_EQUAL(contents, path1_contents);
+    }
+    fs::remove(path2);
+}
+
 #ifndef WIN32
 BOOST_AUTO_TEST_CASE(create_directories)
 {

src/txmempool.cpp

@@ -29,23 +29,23 @@
 #include <cmath>
 #include <optional>
 
-CTxMemPoolEntry::CTxMemPoolEntry(const CTransactionRef& _tx, const CAmount& _nFee,
+CTxMemPoolEntry::CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee,
-                                 int64_t _nTime, unsigned int _entryHeight,
+                                 int64_t time, unsigned int entry_height,
-                                 bool _spendsCoinbase, unsigned int _sigOps, LockPoints lp)
+                                 bool spends_coinbase, int64_t sigops_count, LockPoints lp)
-    : tx(_tx), nFee(_nFee), nTxSize(tx->GetTotalSize()), nUsageSize(RecursiveDynamicUsage(tx)), nTime(_nTime), entryHeight(_entryHeight),
+    : tx{tx},
-    spendsCoinbase(_spendsCoinbase), sigOpCount(_sigOps), lockPoints(lp)
+      nFee{fee},
-{
+      nTxSize(tx->GetTotalSize()),
-    nCountWithDescendants = 1;
+      nUsageSize{RecursiveDynamicUsage(tx)},
-    nSizeWithDescendants = GetTxSize();
+      nTime{time},
-    nModFeesWithDescendants = nFee;
+      entryHeight{entry_height},
-
+      spendsCoinbase{spends_coinbase},
-    feeDelta = 0;
+      sigOpCount{sigops_count},
-
+      lockPoints{lp},
-    nCountWithAncestors = 1;
+      nSizeWithDescendants{GetTxSize()},
-    nSizeWithAncestors = GetTxSize();
+      nModFeesWithDescendants{nFee},
-    nModFeesWithAncestors = nFee;
+      nSizeWithAncestors{GetTxSize()},
-    nSigOpCountWithAncestors = sigOpCount;
+      nModFeesWithAncestors{nFee},
-}
+      nSigOpCountWithAncestors{sigOpCount} {}
 
 void CTxMemPoolEntry::UpdateFeeDelta(int64_t newFeeDelta)
 {

src/txmempool.h

@@ -47,19 +47,16 @@ using CBLSLazyPublicKey = CBLSLazyWrapper<CBLSPublicKey>;
 /** Fake height value used in Coin to signify they are only in the memory pool (since 0.8) */
 static const uint32_t MEMPOOL_HEIGHT = 0x7FFFFFFF;
 
-struct LockPoints
+struct LockPoints {
-{
     // Will be set to the blockchain height and median time past
     // values that would be necessary to satisfy all relative locktime
     // constraints (BIP68) of this tx given our view of block chain history
-    int height;
+    int height{0};
-    int64_t time;
+    int64_t time{0};
     // As long as the current chain descends from the highest height block
     // containing one of the inputs used in the calculation, then the cached
     // values are still valid even after a reorg.
-    CBlockIndex* maxInputBlock;
+    CBlockIndex* maxInputBlock{nullptr};
-
-    LockPoints() : height(0), time(0), maxInputBlock(nullptr) { }
 };
 
 struct CompareIteratorByHash {
@@ -106,28 +103,28 @@ private:
     const int64_t nTime;            //!< Local time when entering the mempool
     const unsigned int entryHeight; //!< Chain height when entering the mempool
     const bool spendsCoinbase;      //!< keep track of transactions that spend a coinbase
-    const unsigned int sigOpCount;  //!< Legacy sig ops plus P2SH sig op count
+    const int64_t sigOpCount;       //!< Legacy sig ops plus P2SH sig op count
-    int64_t feeDelta;          //!< Used for determining the priority of the transaction for mining in a block
+    int64_t feeDelta{0};            //!< Used for determining the priority of the transaction for mining in a block
-    LockPoints lockPoints;     //!< Track the height and time at which tx was final
+    LockPoints lockPoints;          //!< Track the height and time at which tx was final
 
     // Information about descendants of this transaction that are in the
     // mempool; if we remove this transaction we must remove all of these
     // descendants as well.
-    uint64_t nCountWithDescendants;  //!< number of descendant transactions
+    uint64_t nCountWithDescendants{1}; //!< number of descendant transactions
     uint64_t nSizeWithDescendants;   //!< ... and size
     CAmount nModFeesWithDescendants; //!< ... and total fees (all including us)
 
     // Analogous statistics for ancestor transactions
-    uint64_t nCountWithAncestors;
+    uint64_t nCountWithAncestors{1};
     uint64_t nSizeWithAncestors;
     CAmount nModFeesWithAncestors;
-    unsigned int nSigOpCountWithAncestors;
+    int64_t nSigOpCountWithAncestors;
 
 public:
-    CTxMemPoolEntry(const CTransactionRef& _tx, const CAmount& _nFee,
+    CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee,
-                    int64_t _nTime, unsigned int _entryHeight,
+                    int64_t time, unsigned int entry_height,
-                    bool spendsCoinbase,
+                    bool spends_coinbase,
-                    unsigned int nSigOps, LockPoints lp);
+                    int64_t sigops_count, LockPoints lp);
 
     const CTransaction& GetTx() const { return *this->tx; }
     CTransactionRef GetSharedTx() const { return this->tx; }
@@ -135,7 +132,7 @@ public:
     size_t GetTxSize() const;
     std::chrono::seconds GetTime() const { return std::chrono::seconds{nTime}; }
     unsigned int GetHeight() const { return entryHeight; }
-    unsigned int GetSigOpCount() const { return sigOpCount; }
+    int64_t GetSigOpCount() const { return sigOpCount; }
     int64_t GetModifiedFee() const { return nFee + feeDelta; }
     size_t DynamicMemoryUsage() const { return nUsageSize; }
     const LockPoints& GetLockPoints() const { return lockPoints; }
@@ -159,7 +156,7 @@ public:
     uint64_t GetCountWithAncestors() const { return nCountWithAncestors; }
     uint64_t GetSizeWithAncestors() const { return nSizeWithAncestors; }
     CAmount GetModFeesWithAncestors() const { return nModFeesWithAncestors; }
-    unsigned int GetSigOpCountWithAncestors() const { return nSigOpCountWithAncestors; }
+    int64_t GetSigOpCountWithAncestors() const { return nSigOpCountWithAncestors; }
 
     const Parents& GetMemPoolParentsConst() const { return m_parents; }
     const Children& GetMemPoolChildrenConst() const { return m_children; }

src/util/string.h

@@ -78,6 +78,14 @@ inline std::string Join(const std::vector<std::string>& list, const std::string&
     return Join<std::string>(list, separator);
 }
 
+/**
+ * Create an unordered multi-line list of items.
+ */
+inline std::string MakeUnorderedList(const std::vector<std::string>& items)
+{
+    return Join(items, "\n", [](const std::string& item) { return "- " + item; });
+}
+
 /**
  * Check if a string does not contain any embedded NUL (\0) characters
  */

src/util/system.cpp

@@ -539,11 +539,11 @@ bool ArgsManager::InitSettings(std::string& error)
 
     std::vector<std::string> errors;
     if (!ReadSettingsFile(&errors)) {
-        error = strprintf("Failed loading settings file:\n- %s\n", Join(errors, "\n- "));
+        error = strprintf("Failed loading settings file:\n%s\n", MakeUnorderedList(errors));
         return false;
     }
     if (!WriteSettingsFile(&errors)) {
-        error = strprintf("Failed saving settings file:\n- %s\n", Join(errors, "\n- "));
+        error = strprintf("Failed saving settings file:\n%s\n", MakeUnorderedList(errors));
         return false;
     }
     return true;
@@ -1138,9 +1138,20 @@ std::vector<util::SettingsValue> ArgsManager::GetSettingsList(const std::string&
 
 bool RenameOver(fs::path src, fs::path dest)
 {
+#ifdef __MINGW64__
+    // This is a workaround for a bug in libstdc++ which
+    // implements std::filesystem::rename with _wrename function.
+    // This bug has been fixed in upstream:
+    //  - GCC 10.3: 8dd1c1085587c9f8a21bb5e588dfe1e8cdbba79e
+    //  - GCC 11.1: 1dfd95f0a0ca1d9e6cbc00e6cbfd1fa20a98f312
+    // For more details see the commits mentioned above.
+    return MoveFileExW(src.wstring().c_str(), dest.wstring().c_str(),
+                       MOVEFILE_REPLACE_EXISTING) != 0;
+#else
     std::error_code error;
     fs::rename(src, dest, error);
     return !error;
+#endif
 }
 
 /**

test/functional/feature_config_args.py

@@ -215,11 +215,43 @@ class ConfArgsTest(BitcoinTestFramework):
         ]):
             self.nodes[0].setmocktime(start + 65)
 
+    def test_connect_with_seednode(self):
+        self.log.info('Test -connect with -seednode')
+        seednode_ignored = ['-seednode is ignored when -connect is used\n']
+        dnsseed_ignored = ['-dnsseed is ignored when -connect is used and -proxy is specified\n']
+        addcon_thread_started = ['addcon thread start\n']
+        self.stop_node(0)
+
+        # When -connect is supplied, expanding addrman via getaddr calls to ADDR_FETCH(-seednode)
+        # nodes is irrelevant and -seednode is ignored.
+        with self.nodes[0].assert_debug_log(expected_msgs=seednode_ignored):
+            self.start_node(0, extra_args=['-connect=fakeaddress1', '-seednode=fakeaddress2'])
+
+        # With -proxy, an ADDR_FETCH connection is made to a peer that the dns seed resolves to.
+        # ADDR_FETCH connections are not used when -connect is used.
+        with self.nodes[0].assert_debug_log(expected_msgs=dnsseed_ignored):
+            self.restart_node(0, extra_args=['-connect=fakeaddress1', '-dnsseed=1', '-proxy=1.2.3.4'])
+
+        # If the user did not disable -dnsseed, but it was soft-disabled because they provided -connect,
+        # they shouldn't see a warning about -dnsseed being ignored.
+        with self.nodes[0].assert_debug_log(expected_msgs=addcon_thread_started,
+                unexpected_msgs=dnsseed_ignored):
+            self.restart_node(0, extra_args=['-connect=fakeaddress1', '-proxy=1.2.3.4'])
+
+        # We have to supply expected_msgs as it's a required argument
+        # The expected_msg must be something we are confident will be logged after the unexpected_msg
+        # These cases test for -connect being supplied but only to disable it
+        for connect_arg in ['-connect=0', '-noconnect']:
+            with self.nodes[0].assert_debug_log(expected_msgs=addcon_thread_started,
+                    unexpected_msgs=seednode_ignored):
+                self.restart_node(0, extra_args=[connect_arg, '-seednode=fakeaddress2'])
+
     def run_test(self):
         self.test_log_buffer()
         self.test_args_log()
         self.test_seed_peers()
         self.test_networkactive()
+        self.test_connect_with_seednode()
 
 
         self.test_config_file_parser()

test/functional/mempool_persist.py

@@ -133,8 +133,9 @@ class MempoolPersistTest(BitcoinTestFramework):
         mempooldat1 = os.path.join(self.nodes[1].datadir, self.chain, 'mempool.dat')
         self.log.debug("Remove the mempool.dat file. Verify that savemempool to disk via RPC re-creates it")
         os.remove(mempooldat0)
-        self.nodes[0].savemempool()
+        result0 = self.nodes[0].savemempool()
         assert os.path.isfile(mempooldat0)
+        assert_equal(result0['filename'], mempooldat0)
 
         self.log.debug("Stop nodes, make node1 use mempool.dat from node0. Verify it has 6 transactions")
         os.rename(mempooldat0, mempooldat1)