From abcd5169812a95362be97e42590faddcb0270e49 Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Tue, 5 Sep 2023 11:40:21 +0300
Subject: [PATCH 01/12] Merge bitcoin/bitcoin#28291: rpc: removed
 StrFormatInternalBug quote delimitation

6e8f6468cbf1320b70cf01333002a31b44cb7c33 removed StrFormatInternalBug quote delimitation (Reese Russell)

Pull request description:

  This PR rectifies an unnecessary set of quotes delimiting the contents of  ```StrFormatInternalBug```. This is a follow up to MarcoFalke https://github.com/bitcoin/bitcoin/pull/28123#discussion_r1297191493.  The method of action was to remove the escaped quotes that were a part of strprintf. A single functional test case was modified to reflect the new output format.

  ```STR_INTERNAL_BUG``` was applied to https://github.com/bitcoin/bitcoin/pull/28123 in ```std::string RPCArg::ToString(const bool oneline)``` in ```rpc/util.cpp```

  The results can be seen below.

  Previously
  ![image](https://github.com/bitcoin/bitcoin/assets/3104223/53f9ea59-317f-4c62-9fc1-04255eeb4641)

  This PR
  ![image](https://github.com/bitcoin/bitcoin/assets/3104223/5c6a3110-f1f3-4b3c-8e8a-9c8f1c3176e7)

  Additional context can be found here.
  https://github.com/bitcoin/bitcoin/pull/28123#discussion_r1271871716

  Thank you.

ACKs for top commit:
  MarcoFalke:
    review ACK 6e8f6468cbf1320b70cf01333002a31b44cb7c33
  stickies-v:
    ACK 6e8f6468cbf1320b70cf01333002a31b44cb7c33

Tree-SHA512: 35317e31a527630495b566407e37db9941dab7f81cfaeb1ea3309683c48e4273284645ad615f73e646a137b4f2ae35933603e9182a7dbdd22cac98d038c491dc

Co-authored-by: fanquake <fanquake@gmail.com>
---
 src/rpc/util.cpp | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index d76f94cab6c1..9c925cecd986 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -1147,10 +1147,9 @@ std::string RPCArg::ToString(const bool oneline) const
     if (oneline && !m_opts.oneline_description.empty()) {
         if (m_opts.oneline_description[0] == '\"' && m_type != Type::STR_HEX && m_type != Type::STR && gArgs.GetBoolArg("-rpcdoccheck", DEFAULT_RPC_DOC_CHECK)) {
             throw std::runtime_error{
-                strprintf("Internal bug detected: non-string RPC arg \"%s\" quotes oneline_description:\n%s\n%s %s\nPlease report this issue here: %s\n",
-                          m_names, m_opts.oneline_description,
-                          PACKAGE_NAME, FormatFullVersion(),
-                          PACKAGE_BUGREPORT)};
+                STR_INTERNAL_BUG(strprintf("non-string RPC arg \"%s\" quotes oneline_description:\n%s",
+                    m_names, m_opts.oneline_description)
+                )};
         }
         return m_opts.oneline_description;
     }

From f6f7b9959408f043e1264256e33b8cc8c1e11a68 Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Fri, 6 Nov 2026 22:32:25 +0700
Subject: [PATCH 02/12] refactor: always serialize CDeterministicMN::nType

CDeterministicMN gated nType on the stream version: it was written when the version was CLIENT_VERSION (taken to mean "disk") or at least DMN_TYPE_PROTO_VERSION (taken to mean "network"), and defaulted to MnType::Regular otherwise.
The network branch is dead: nothing sends CDeterministicMN over P2P, peers get CSimplifiedMNListEntry instead.

So nType is always written and read, and the on-disk bytes do not change.
Dropping the gate removes the last dependency of CDeterministicMN on the stream version, which the versionless DataStream used by dbwrapper after the following backports does not provide.
---
 src/evo/deterministicmns.h | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/src/evo/deterministicmns.h b/src/evo/deterministicmns.h
index ce4df7367fab..5131845a90d2 100644
--- a/src/evo/deterministicmns.h
+++ b/src/evo/deterministicmns.h
@@ -73,14 +73,7 @@ class CDeterministicMN
         READWRITE(obj.collateralOutpoint);
         READWRITE(obj.nOperatorReward);
         READWRITE(obj.pdmnState);
-        // We can't know if we are serialising for the Disk or for the Network here (s.GetType() is not accessible)
-        // Therefore if s.GetVersion() == CLIENT_VERSION -> Then we know we are serialising for the Disk
-        // Otherwise, we can safely check with protocol versioning logic so we won't break old clients
-        if (s.GetVersion() == CLIENT_VERSION || s.GetVersion() >= DMN_TYPE_PROTO_VERSION) {
-            READWRITE(obj.nType);
-        } else {
-            SER_READ(obj, obj.nType = MnType::Regular);
-        }
+        READWRITE(obj.nType);
     }
 
     [[nodiscard]] bool HasInternalId() const { return internalId != std::numeric_limits<uint64_t>::max(); }

From 637f5360cfff8903c0721be85e4fd6cbc09fc606 Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Sat, 9 Sep 2023 11:16:31 +0100
Subject: [PATCH 03/12] Merge bitcoin/bitcoin#28428: Hard-code version number
 value for CBlockLocator and CDiskBlockIndex

e73d2a8018def940afadb5d699b18f39e882c1fc refactor: remove clientversion include from dbwrapper.h (Cory Fields)
4240a082b81d8ceb7615b1b4ca0d2857382f317b refactor: Use DataStream now that version/type are unused (Cory Fields)
f15f790618d328abd207d55e6291229eb2a8643f Remove version/hashing options from CBlockLocator/CDiskBlockIndex (Cory Fields)

Pull request description:

  This is also a much simpler replacement for #28327.

  There are version fields in `CBlockLocator` and `CDiskBlockIndex` that have always been written but discarded when read.

  I intended to convert them to use SerParams as introduced by #25284, which [ended up looking like this](https://github.com/theuni/bitcoin/commit/3e3af451652322c92e8e41cf918e69d608ec7c77). However because we don't currently have any definition of what a hash value would mean for either one of those, and we've never assigned the version field any meaning, I think it's better to just not worry about them.

  If we ever need to assign meaning in the future, we can introduce `SerParams` as was done for `CAddress`.

  As for the dummy values chosen:

  `CDiskBlockIndex::DUMMY_VERSION` was easy as the highest ever client version, and I don't expect any objection there.

  `CBlockLocator::DUMMY_VERSION` is hard-coded to the higest _PROTOCOL_ version ever used. This is to avoid a sudden bump that would be visible on the network if CLIENT_VERSION were used instead. In the future, if we ever need to use the value, we can discard anything in the CLIENT_VERSION range (for a few years as needed), as it's quite a bit higher.

  While reviewing, I suggest looking at the throwaway `SerParams` commit above as it shows where the call-sites are. I believe that should be enough to convince one's self that hashing is never used.

ACKs for top commit:
  TheCharlatan:
    Re-ACK e73d2a8018def940afadb5d699b18f39e882c1fc
  ajtowns:
    reACK e73d2a8018def940afadb5d699b18f39e882c1fc

Tree-SHA512: 45b0dd7c2e918493e2ee92a8e35320ad17991cb8908cb811150a96c5fd584ce177c775baeeb8675a602c90b9ba9203b8cefc0a2a0c6a71078b1d9c2b41e1f3ba

Dash adaptations:
- CDiskBlockIndex::DUMMY_VERSION is 240100, Dash's current CLIENT_VERSION
  (10000 * 24 + 100 * 1 + 0 from _CLIENT_VERSION_MAJOR/MINOR/BUILD in
  configure.ac, per src/clientversion.h), instead of upstream's 259900.
- CBlockLocator::DUMMY_VERSION is 70242, Dash's highest PROTOCOL_VERSION
  (src/version.h), instead of upstream's 70016.
- Neither type is hashed with SER_GETHASH anywhere in Dash, so dropping
  that branch changes nothing.
- The CDBWrapper::ReadDataStream() overloads, which upstream does not
  have, now fill a DataStream as well, and so do their callers in
  evo/deterministicmns.cpp, llmq/dkgsessionmgr.cpp, llmq/quorums.cpp,
  llmq/quorumsman.cpp and llmq/signing.cpp. None of them needs a stream
  version or type: they decode BLS version wrappers, CRecoveredSig,
  recovered-sig markers, raw bytes and the legacy CDeterministicMNListDiff
  format.
- dbwrapper.h keeps including clientversion.h: Dash's CDBTransaction
  still estimates memory usage with ::GetSerializeSize(v, CLIENT_VERSION)
  and CDBWrapper::CompactRange() still builds SER_DISK/CLIENT_VERSION key
  streams. The upstream include additions to the files that use
  CLIENT_VERSION directly are kept.

Co-authored-by: fanquake <fanquake@gmail.com>
---
 src/chain.h                    | 12 ++++++++++--
 src/dbwrapper.h                | 10 +++++-----
 src/evo/deterministicmns.cpp   |  2 +-
 src/index/blockfilterindex.cpp |  1 +
 src/index/txindex.cpp          |  1 +
 src/init.cpp                   |  1 +
 src/llmq/dkgsessionmgr.cpp     |  2 +-
 src/llmq/quorums.cpp           |  2 +-
 src/llmq/quorumsman.cpp        |  2 +-
 src/llmq/signing.cpp           |  4 ++--
 src/primitives/block.h         | 14 +++++++++++---
 src/validation.cpp             |  1 +
 12 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/src/chain.h b/src/chain.h
index a67a88641e9f..7b82ea1c8762 100644
--- a/src/chain.h
+++ b/src/chain.h
@@ -372,6 +372,14 @@ const CBlockIndex* LastCommonAncestor(const CBlockIndex* pa, const CBlockIndex*
 /** Used to marshal pointers into hashes for db storage. */
 class CDiskBlockIndex : public CBlockIndex
 {
+    /** Historically CBlockLocator's version field has been written to disk
+     * streams as the client version, but the value has never been used.
+     *
+     * Hard-code to the highest client version ever written.
+     * SerParams can be used if the field requires any meaning in the future.
+     **/
+    static constexpr int DUMMY_VERSION = 240100;
+
 public:
     uint256 hash;
     uint256 hashPrev;
@@ -391,8 +399,8 @@ class CDiskBlockIndex : public CBlockIndex
     SERIALIZE_METHODS(CDiskBlockIndex, obj)
     {
         LOCK(::cs_main);
-        int _nVersion = s.GetVersion();
-        if (!(s.GetType() & SER_GETHASH)) READWRITE(VARINT_MODE(_nVersion, VarIntMode::NONNEGATIVE_SIGNED));
+        int _nVersion = DUMMY_VERSION;
+        READWRITE(VARINT_MODE(_nVersion, VarIntMode::NONNEGATIVE_SIGNED));
 
         READWRITE(VARINT_MODE(obj.nHeight, VarIntMode::NONNEGATIVE_SIGNED));
         READWRITE(VARINT(obj.nStatus));
diff --git a/src/dbwrapper.h b/src/dbwrapper.h
index affdb583438a..8c3364f1887c 100644
--- a/src/dbwrapper.h
+++ b/src/dbwrapper.h
@@ -199,7 +199,7 @@ class CDBIterator
 
     template<typename V> bool GetValue(V& value) {
         try {
-            CDataStream ssValue{GetValueImpl(), SER_DISK, CLIENT_VERSION};
+            DataStream ssValue{GetValueImpl()};
             ssValue.Xor(dbwrapper_private::GetObfuscateKey(parent));
             ssValue >> value;
         } catch (const std::exception&) {
@@ -252,7 +252,7 @@ class CDBWrapper
     CDBWrapper& operator=(const CDBWrapper&) = delete;
 
     template <typename K>
-    bool ReadDataStream(const K& key, CDataStream& ssValue) const
+    bool ReadDataStream(const K& key, DataStream& ssValue) const
     {
         DataStream ssKey{};
         ssKey.reserve(DBWRAPPER_PREALLOC_KEY_SIZE);
@@ -260,13 +260,13 @@ class CDBWrapper
         return ReadDataStream(ssKey, ssValue);
     }
 
-    bool ReadDataStream(const DataStream& ssKey, CDataStream& ssValue) const
+    bool ReadDataStream(const DataStream& ssKey, DataStream& ssValue) const
     {
         std::optional<std::string> strValue{ReadImpl(ssKey)};
         if (!strValue) {
             return false;
         }
-        CDataStream ssValueTmp{MakeByteSpan(*strValue), SER_DISK, CLIENT_VERSION};
+        DataStream ssValueTmp{MakeByteSpan(*strValue)};
         ssValueTmp.Xor(obfuscate_key);
         ssValue = std::move(ssValueTmp);
         return true;
@@ -284,7 +284,7 @@ class CDBWrapper
     template <typename V>
     bool Read(const DataStream& ssKey, V& value) const
     {
-        CDataStream ssValue(SER_DISK, CLIENT_VERSION);
+        DataStream ssValue{};
         if (!ReadDataStream(ssKey, ssValue)) {
             return false;
         }
diff --git a/src/evo/deterministicmns.cpp b/src/evo/deterministicmns.cpp
index 70288671d0a6..da21fb5175df 100644
--- a/src/evo/deterministicmns.cpp
+++ b/src/evo/deterministicmns.cpp
@@ -1068,7 +1068,7 @@ bool CDeterministicMNManager::MigrateLegacyDiffs(const CBlockIndex* const tip_in
         }
 
         // Use legacy-aware deserialization for DB_LIST_DIFF_LEGACY entries
-        CDataStream s(SER_DISK, CLIENT_VERSION);
+        DataStream s{};
         if (!m_evoDb.GetRawDB().ReadDataStream(key, s)) {
             break;
         }
diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp
index d2b03a992734..d41b8fe21799 100644
--- a/src/index/blockfilterindex.cpp
+++ b/src/index/blockfilterindex.cpp
@@ -4,6 +4,7 @@
 
 #include <map>
 
+#include <clientversion.h>
 #include <common/args.h>
 #include <dbwrapper.h>
 #include <hash.h>
diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp
index 67caf35574d1..e6b7c6358fae 100644
--- a/src/index/txindex.cpp
+++ b/src/index/txindex.cpp
@@ -4,6 +4,7 @@
 
 #include <index/txindex.h>
 
+#include <clientversion.h>
 #include <common/args.h>
 #include <index/disktxpos.h>
 #include <logging.h>
diff --git a/src/init.cpp b/src/init.cpp
index 4c4892f7054b..e6c3d0c56dda 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -21,6 +21,7 @@
 #include <blockfilter.h>
 #include <chain.h>
 #include <chainparams.h>
+#include <clientversion.h>
 #include <common/args.h>
 #include <common/system.h>
 #include <context.h>
diff --git a/src/llmq/dkgsessionmgr.cpp b/src/llmq/dkgsessionmgr.cpp
index 77f81eab13fa..b7225d162c4d 100644
--- a/src/llmq/dkgsessionmgr.cpp
+++ b/src/llmq/dkgsessionmgr.cpp
@@ -159,7 +159,7 @@ bool CDKGSessionManager::GetVerifiedContributions(Consensus::LLMQType llmqType,
             ContributionsCacheKey cacheKey = {llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash};
             auto it = contributionsCache.find(cacheKey);
             if (it == contributionsCache.end()) {
-                CDataStream s(SER_DISK, CLIENT_VERSION);
+                DataStream s{};
                 if (!db->ReadDataStream(std::make_tuple(DB_VVEC, llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash), s)) {
                     LogPrint(BCLog::LLMQ, "%s -- this node does not have vvec for llmq=%d block=%s protx=%s\n",
                              __func__, std23::to_underlying(llmqType), pQuorumBaseBlockIndex->GetBlockHash().ToString(),
diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp
index 3a0d466fa699..86e4b703704a 100644
--- a/src/llmq/quorums.cpp
+++ b/src/llmq/quorums.cpp
@@ -212,7 +212,7 @@ void CQuorum::WriteContributions(CDBWrapper& db) const
 bool CQuorum::ReadContributions(const CDBWrapper& db)
 {
     uint256 dbKey = MakeQuorumKey(*this);
-    CDataStream s(SER_DISK, CLIENT_VERSION);
+    DataStream s{};
 
     if (!db.ReadDataStream(std::make_pair(DB_QUORUM_QUORUM_VVEC, dbKey), s)) {
         return false;
diff --git a/src/llmq/quorumsman.cpp b/src/llmq/quorumsman.cpp
index 48a8fd5bbcdf..b91e04a41b3f 100644
--- a/src/llmq/quorumsman.cpp
+++ b/src/llmq/quorumsman.cpp
@@ -643,7 +643,7 @@ void CQuorumManager::MigrateOldQuorumDB(CEvoDB& evoDb) const
         int count{0};
         while (pcursor->Valid()) {
             decltype(start) k;
-            CDataStream s(SER_DISK, CLIENT_VERSION);
+            DataStream s{};
             CBLSSecretKey sk;
 
             if (!pcursor->GetKey(k) || std::get<0>(k) != prefix) {
diff --git a/src/llmq/signing.cpp b/src/llmq/signing.cpp
index f374d28963cf..d4587e61e206 100644
--- a/src/llmq/signing.cpp
+++ b/src/llmq/signing.cpp
@@ -96,7 +96,7 @@ bool CRecoveredSigsDb::ReadRecoveredSig(Consensus::LLMQType llmqType, const uint
 {
     auto k = std::make_tuple(std::string("rs_r"), llmqType, id);
 
-    CDataStream ds(SER_DISK, CLIENT_VERSION);
+    DataStream ds{};
     if (!db->ReadDataStream(k, ds)) {
         return false;
     }
@@ -239,7 +239,7 @@ struct RecoveredSigMarker {
 
 std::optional<RecoveredSigMarker> ReadRecoveredSigMarker(const CDBWrapper& db, const std::string& prefix, const uint256& hash)
 {
-    CDataStream value{SER_DISK, CLIENT_VERSION};
+    DataStream value{};
     if (!db.ReadDataStream(std::make_pair(prefix, hash), value)) return std::nullopt;
     RecoveredSigMarker marker;
     try {
diff --git a/src/primitives/block.h b/src/primitives/block.h
index 8789a21cbe37..486f855e6204 100644
--- a/src/primitives/block.h
+++ b/src/primitives/block.h
@@ -245,6 +245,15 @@ class CBlock : public CBlockHeader
  */
 struct CBlockLocator
 {
+    /** Historically CBlockLocator's version field has been written to network
+     * streams as the negotiated protocol version and to disk streams as the
+     * client version, but the value has never been used.
+     *
+     * Hard-code to the highest protocol version ever written to a network stream.
+     * SerParams can be used if the field requires any meaning in the future,
+     **/
+    static constexpr int DUMMY_VERSION = 70242;
+
     std::vector<uint256> vHave;
 
     CBlockLocator() {}
@@ -253,9 +262,8 @@ struct CBlockLocator
 
     SERIALIZE_METHODS(CBlockLocator, obj)
     {
-        int nVersion = s.GetVersion();
-        if (!(s.GetType() & SER_GETHASH))
-            READWRITE(nVersion);
+        int nVersion = DUMMY_VERSION;
+        READWRITE(nVersion);
         READWRITE(obj.vHave);
     }
 
diff --git a/src/validation.cpp b/src/validation.cpp
index bd394b24d84a..3a34f9547844 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -18,6 +18,7 @@
 #include <chain.h>
 #include <chainparams.h>
 #include <checkqueue.h>
+#include <clientversion.h>
 #include <common/args.h>
 #include <consensus/amount.h>
 #include <consensus/consensus.h>

From facfcd3425a8317330c45c937ec61903eef76c1e Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Thu, 14 Sep 2023 09:39:37 +0100
Subject: [PATCH 04/12] Merge bitcoin/bitcoin#28458: refactor: Remove unused
 GetType() from CBufferedFile and CAutoFile

BACKPORT NOTE:
CDeterministicMN, CDiskBlockIndex and CBlockLocator no longer read GetVersion() (see bitcoin#28428 and "refactor: always serialize CDeterministicMN::nType")

dbwrapper.cpp no longer uses CLIENT_VERSION, so its clientversion.h include is dropped. dbwrapper.h keeps it for CDBTransaction's GetSerializeSize() estimate and CompactRange().

Backport done in full now

----

fa19c914f7fe7be127c0fb330b41ff7c091f40aa scripted-diff: Rename CBufferedFile to BufferedFile (MarcoFalke)
fa2f2413b87f5fc1e5c92bf510beebdcd0091714 Remove unused GetType() from CBufferedFile and CAutoFile (MarcoFalke)
5c2b3cd4b856f1bb536daaf7f576b1b1b42293ca dbwrapper: Use DataStream for batch operations (TheCharlatan)

Pull request description:

  This refactor is required for https://github.com/bitcoin/bitcoin/pull/28052 and https://github.com/bitcoin/bitcoin/pull/28451

  Thus, split it out.

ACKs for top commit:
  ajtowns:
    utACK fa19c914f7fe7be127c0fb330b41ff7c091f40aa
  TheCharlatan:
    ACK fa19c914f7fe7be127c0fb330b41ff7c091f40aa

Tree-SHA512: d9c232324702512e45fd73ec3e3170f1e8a8c8f9c49cb613a6b693a9f83358914155527ace2517a2cd626a0cedcada56ef70a2a7812edafb1888fd6765eebba2

Co-authored-by: fanquake <fanquake@gmail.com>
---
 src/dbwrapper.cpp | 9 ++++-----
 src/dbwrapper.h   | 4 ++--
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp
index ecc406699b32..431161479487 100644
--- a/src/dbwrapper.cpp
+++ b/src/dbwrapper.cpp
@@ -4,7 +4,6 @@
 
 #include <dbwrapper.h>
 
-#include <clientversion.h>
 #include <common/system.h>
 #include <logging.h>
 #include <random.h>
@@ -157,9 +156,9 @@ struct CDBBatch::WriteBatchImpl {
     leveldb::WriteBatch batch;
 };
 
-CDBBatch::CDBBatch(const CDBWrapper& _parent) : parent(_parent),
-                                                m_impl_batch{std::make_unique<CDBBatch::WriteBatchImpl>()},
-                                                ssValue(SER_DISK, CLIENT_VERSION){};
+CDBBatch::CDBBatch(const CDBWrapper& _parent)
+    : parent{_parent},
+      m_impl_batch{std::make_unique<CDBBatch::WriteBatchImpl>()} {};
 
 CDBBatch::~CDBBatch() = default;
 
@@ -169,7 +168,7 @@ void CDBBatch::Clear()
     size_estimate = 0;
 }
 
-void CDBBatch::WriteImpl(Span<const std::byte> key, CDataStream& ssValue)
+void CDBBatch::WriteImpl(Span<const std::byte> key, DataStream& ssValue)
 {
     leveldb::Slice slKey(CharCast(key.data()), key.size());
     ssValue.Xor(dbwrapper_private::GetObfuscateKey(parent));
diff --git a/src/dbwrapper.h b/src/dbwrapper.h
index 8c3364f1887c..4c09a1174339 100644
--- a/src/dbwrapper.h
+++ b/src/dbwrapper.h
@@ -91,11 +91,11 @@ class CDBBatch
     const std::unique_ptr<WriteBatchImpl> m_impl_batch;
 
     DataStream ssKey{};
-    CDataStream ssValue;
+    DataStream ssValue{};
 
     size_t size_estimate{0};
 
-    void WriteImpl(Span<const std::byte> key, CDataStream& ssValue);
+    void WriteImpl(Span<const std::byte> key, DataStream& ssValue);
     void EraseImpl(Span<const std::byte> key);
 
 public:

From 17e22963a4cacfa5581dc920bf3ded5cec1812df Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Thu, 14 Sep 2023 13:15:24 -0400
Subject: [PATCH 05/12] Merge bitcoin/bitcoin#27850: test: Add unit &
 functional test coverage for blockstore
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

de8f9123afbecc3b4f59fa80af8148bc865d0588 test: cover read-only blockstore (Matthew Zipkin)
5c2185b3b624ce87320ec16412f98ab591a5860c ci: enable chattr +i capability inside containers (Matthew Zipkin)
e573f2420244c583e218f51cd0d3a3cac6731003 unit test: add coverage for BlockManager (Matthew Zipkin)

Pull request description:

  This PR adds unit and functional tests to cover the behavior described in #2039. In particular, that bitcoind will crash on startup if a reindex is requested but the `blk` files are read-only. Eventually this behavior can be updated with https://github.com/bitcoin/bitcoin/pull/27039. This PR just commits the test coverage from #27039 as suggested in https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1584915782

ACKs for top commit:
  jonatack:
    ACK de8f9123afbecc3b4f59fa80af8148bc865d0588 modulo suggestions in https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1319010039, tested on macOS, but not on Linux for the Linux-related change in the last push
  achow101:
    ACK de8f9123afbecc3b4f59fa80af8148bc865d0588
  MarcoFalke:
    lgtm ACK de8f9123afbecc3b4f59fa80af8148bc865d0588 📶

Tree-SHA512: b9bd684035dcea11c901b649fc39f397a2155a9a8459f3348e67947e387e45312fddeccb52981aef486f8a31deebb5356a7901c1bb94b78f82c24192a369af73

Co-authored-by: Andrew Chow <github@achow101.com>
---
 .github/workflows/test-src.yml              |  3 +-
 contrib/containers/ci/ci-slim.Dockerfile    |  1 +
 src/test/blockmanager_tests.cpp             | 69 +++++++++++++++++++++
 test/functional/feature_reindex_readonly.py | 64 +++++++++++++++++++
 test/functional/test_runner.py              |  1 +
 5 files changed, 137 insertions(+), 1 deletion(-)
 create mode 100755 test/functional/feature_reindex_readonly.py

diff --git a/.github/workflows/test-src.yml b/.github/workflows/test-src.yml
index d6d9b7efe58f..3b25c11a3ef8 100644
--- a/.github/workflows/test-src.yml
+++ b/.github/workflows/test-src.yml
@@ -41,7 +41,8 @@ jobs:
       image: ${{ inputs.container-path }} # zizmor: ignore[unpinned-images] built earlier in this run by build-container.yml
       # SYS_PTRACE is required by LeakSanitizer's stop-the-world
       # (https://github.com/google/sanitizers/issues/764)
-      options: --user root --cap-add SYS_PTRACE
+      # LINUX_IMMUTABLE lets feature_reindex_readonly.py run chattr +i
+      options: --user root --cap-add LINUX_IMMUTABLE --cap-add SYS_PTRACE
     steps:
       - name: Checkout code
         uses: actions/checkout@v6
diff --git a/contrib/containers/ci/ci-slim.Dockerfile b/contrib/containers/ci/ci-slim.Dockerfile
index 2cce5c2c705c..50fb86688d8a 100644
--- a/contrib/containers/ci/ci-slim.Dockerfile
+++ b/contrib/containers/ci/ci-slim.Dockerfile
@@ -40,6 +40,7 @@ RUN set -ex; \
     build-essential \
     ca-certificates \
     curl \
+    e2fsprogs \
     g++ \
     git \
     jq \
diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp
index 4ce0bd7bd1d5..052097dd0ceb 100644
--- a/src/test/blockmanager_tests.cpp
+++ b/src/test/blockmanager_tests.cpp
@@ -11,6 +11,7 @@
 #include <node/blockstorage.h>
 #include <node/context.h>
 #include <pow.h>
+#include <primitives/block.h>
 #include <script/script.h>
 #include <streams.h>
 #include <test/util/random.h>
@@ -23,6 +24,7 @@
 #include <validation.h>
 
 #include <boost/test/unit_test.hpp>
+#include <test/util/logging.h>
 #include <test/util/setup_common.h>
 
 using node::BlockManager;
@@ -292,4 +294,71 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_scan_unlink_already_pruned_files, TestChain
     BOOST_CHECK(!CAutoFile(blockman.OpenBlockFile(new_pos, true), CLIENT_VERSION).IsNull());
 }
 
+BOOST_AUTO_TEST_CASE(blockmanager_flush_block_file)
+{
+    node::BlockManager::Options blockman_opts{
+        .chainparams = Params(),
+        .blocks_dir = m_args.GetBlocksDirPath(),
+    };
+    BlockManager blockman{blockman_opts};
+
+    // Test blocks with no transactions, not even a coinbase
+    CBlock block1;
+    block1.nVersion = 1;
+    CBlock block2;
+    block2.nVersion = 2;
+    CBlock block3;
+    block3.nVersion = 3;
+
+    // They are 80 bytes header + 1 byte 0x00 for vtx length
+    constexpr int TEST_BLOCK_SIZE{81};
+
+    // Blockstore is empty
+    BOOST_CHECK_EQUAL(blockman.CalculateCurrentUsage(), 0);
+
+    // Write the first block; dbp=nullptr means this block doesn't already have a disk
+    // location, so allocate a free location and write it there.
+    FlatFilePos pos1{blockman.SaveBlockToDisk(block1, /*nHeight=*/1, /*dbp=*/nullptr)};
+
+    // Write second block
+    FlatFilePos pos2{blockman.SaveBlockToDisk(block2, /*nHeight=*/2, /*dbp=*/nullptr)};
+
+    // Two blocks in the file
+    BOOST_CHECK_EQUAL(blockman.CalculateCurrentUsage(), (TEST_BLOCK_SIZE + BLOCK_SERIALIZATION_HEADER_SIZE) * 2);
+
+    // First two blocks are written as expected
+    // Errors are expected because block data is junk, thrown AFTER successful read
+    CBlock read_block;
+    BOOST_CHECK_EQUAL(read_block.nVersion, 0);
+    {
+        ASSERT_DEBUG_LOG("ReadBlockFromDisk: Errors in block header");
+        BOOST_CHECK(!blockman.ReadBlockFromDisk(read_block, pos1));
+        BOOST_CHECK_EQUAL(read_block.nVersion, 1);
+    }
+    {
+        ASSERT_DEBUG_LOG("ReadBlockFromDisk: Errors in block header");
+        BOOST_CHECK(!blockman.ReadBlockFromDisk(read_block, pos2));
+        BOOST_CHECK_EQUAL(read_block.nVersion, 2);
+    }
+
+    // When FlatFilePos* dbp is given, SaveBlockToDisk() will not write or
+    // overwrite anything to the flat file block storage. It will, however,
+    // update the blockfile metadata. This is to facilitate reindexing
+    // when the user has the blocks on disk but the metadata is being rebuilt.
+    // Verify this behavior by attempting (and failing) to write block 3 data
+    // to block 2 location.
+    CBlockFileInfo* block_data = blockman.GetBlockFileInfo(0);
+    BOOST_CHECK_EQUAL(block_data->nBlocks, 2);
+    BOOST_CHECK(blockman.SaveBlockToDisk(block3, /*nHeight=*/3, /*dbp=*/&pos2) == pos2);
+    // Metadata is updated...
+    BOOST_CHECK_EQUAL(block_data->nBlocks, 3);
+    // ...but there are still only two blocks in the file
+    BOOST_CHECK_EQUAL(blockman.CalculateCurrentUsage(), (TEST_BLOCK_SIZE + BLOCK_SERIALIZATION_HEADER_SIZE) * 2);
+
+    // Block 2 was not overwritten:
+    //   SaveBlockToDisk() did not call WriteBlockToDisk() because `FlatFilePos* dbp` was non-null
+    blockman.ReadBlockFromDisk(read_block, pos2);
+    BOOST_CHECK_EQUAL(read_block.nVersion, 2);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/test/functional/feature_reindex_readonly.py b/test/functional/feature_reindex_readonly.py
new file mode 100755
index 000000000000..9f1bb30023dd
--- /dev/null
+++ b/test/functional/feature_reindex_readonly.py
@@ -0,0 +1,64 @@
+#!/usr/bin/env python3
+# Copyright (c) 2023-present The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test running bitcoind with -reindex from a read-only blockstore
+- Start a node, generate blocks, then restart with -reindex after setting blk files to read-only
+"""
+
+import platform
+import stat
+import subprocess
+from test_framework.test_framework import BitcoinTestFramework
+
+
+class BlockstoreReindexTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        self.num_nodes = 1
+        self.extra_args = [["-fastprune"]]
+
+    def reindex_readonly(self):
+        self.log.debug("Generate block big enough to start second block file")
+        fastprune_blockfile_size = 0x10000
+        opreturn = "6a"
+        nulldata = fastprune_blockfile_size * "ff"
+        self.generateblock(self.nodes[0], output=f"raw({opreturn}{nulldata})", transactions=[])
+        self.stop_node(0)
+
+        assert (self.nodes[0].chain_path / "blocks" / "blk00000.dat").exists()
+        assert (self.nodes[0].chain_path / "blocks" / "blk00001.dat").exists()
+
+        self.log.debug("Make the first block file read-only")
+        filename = self.nodes[0].chain_path / "blocks" / "blk00000.dat"
+        filename.chmod(stat.S_IREAD)
+
+        used_chattr = False
+        if platform.system() == "Linux":
+            try:
+                subprocess.run(['chattr', '+i', filename], capture_output=True, check=True)
+                used_chattr = True
+                self.log.info("Made file immutable with chattr")
+            except subprocess.CalledProcessError as e:
+                self.log.warning(str(e))
+                if e.stdout:
+                    self.log.warning(f"stdout: {e.stdout}")
+                if e.stderr:
+                    self.log.warning(f"stderr: {e.stderr}")
+
+        self.log.debug("Attempt to restart and reindex the node with the unwritable block file")
+        with self.nodes[0].assert_debug_log(expected_msgs=['FlushStateToDisk', 'failed to open file'], unexpected_msgs=[]):
+            self.nodes[0].assert_start_raises_init_error(extra_args=['-reindex', '-fastprune'],
+                expected_msg="Error: A fatal internal error occurred, see debug.log for details")
+
+        if used_chattr:
+            subprocess.check_call(['chattr', '-i', filename])
+
+        filename.chmod(0o777)
+
+    def run_test(self):
+        self.reindex_readonly()
+
+
+if __name__ == '__main__':
+    BlockstoreReindexTest().main()
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index dbfe209d58a2..17a2661d196e 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -208,6 +208,7 @@
     'wallet_abandonconflict.py --legacy-wallet',
     'wallet_abandonconflict.py --descriptors',
     'feature_reindex.py',
+    'feature_reindex_readonly.py',
     'wallet_labels.py --legacy-wallet',
     'wallet_labels.py --descriptors',
     'p2p_compactblocks.py',

From 03669bb6db44b086cfd7524bef0430659c65ab2b Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Thu, 28 Sep 2023 12:02:45 +0100
Subject: [PATCH 06/12] Merge bitcoin/bitcoin#28450: Add package evaluation
 fuzzer

262ab8ef7860d43cebc9d04721e3a075b4edf06e Add package evaluation fuzzer (Greg Sanders)

Pull request description:

  This fuzzer target caught the issue in https://github.com/bitcoin/bitcoin/pull/28251 within 5 minutes on master branch, and an additional issue which I've applied a preliminary patch to cover.

  Fuzzer target does the following:

  1) Picks mempool confgs, including max package size, count, mempool size, etc
  2) Generates 1 to 26 transactions with arbitrary coins/fees, the first N-1 spending only confirmed outpoints
  3) Nth transaction, if >1, sweeps all unconfirmed outpoints in mempool
  4) If N==1, it may submit it through single-tx submission path, to allow for more interesting topologies
  5) Otherwise submits through package submission interface
  6) Repeat 1-5  a few hundred times per mempool instance

  In other words, it ends up building chains of txns in the mempool using parents-and-children packages, which is currently the topology supported on master.

  The test itself is a direct rip of tx_pool.cpp, with a number of assertions removed because they were failing for unknown reasons, likely due to the notification changes of single tx submission to package, which is used to track addition/removal of transactions in the test. I'll continue working on re-adding these assertions for further invariant testing.

ACKs for top commit:
  murchandamus:
    ACK 262ab8ef7860d43cebc9d04721e3a075b4edf06e
  glozow:
    reACK 262ab8ef7860d43cebc9d04721e3a075b4edf06e
  dergoegge:
    tACK 262ab8ef7860d43cebc9d04721e3a075b4edf06e

Tree-SHA512: 190784777d0f2361b051b3271db8f79b7927e3cab88596d2c30e556da721510bd17f6cc96f6bb03403bbf0589ad3f799fa54e63c1b2bd92a2084485b5e3e96a5

Co-authored-by: glozow <gloriajzhao@gmail.com>
---
 src/Makefile.test.include      |   1 +
 src/test/fuzz/package_eval.cpp | 288 +++++++++++++++++++++++++++++++++
 2 files changed, 289 insertions(+)
 create mode 100644 src/test/fuzz/package_eval.cpp

diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index 73f39c029934..a6d42e7568f3 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -363,6 +363,7 @@ test_fuzz_fuzz_SOURCES = \
  test/fuzz/netbase_dns_lookup.cpp \
  test/fuzz/node_eviction.cpp \
  test/fuzz/p2p_transport_serialization.cpp \
+ test/fuzz/package_eval.cpp \
  test/fuzz/parse_hd_keypath.cpp \
  test/fuzz/parse_numbers.cpp \
  test/fuzz/parse_script.cpp \
diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
new file mode 100644
index 000000000000..07eb1684b803
--- /dev/null
+++ b/src/test/fuzz/package_eval.cpp
@@ -0,0 +1,288 @@
+// Copyright (c) 2023 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 <consensus/validation.h>
+#include <node/context.h>
+#include <node/mempool_args.h>
+#include <node/miner.h>
+#include <test/fuzz/FuzzedDataProvider.h>
+#include <test/fuzz/fuzz.h>
+#include <test/fuzz/util.h>
+#include <test/fuzz/util/mempool.h>
+#include <test/util/mining.h>
+#include <test/util/script.h>
+#include <test/util/setup_common.h>
+#include <test/util/txmempool.h>
+#include <validation.h>
+#include <validationinterface.h>
+
+using node::NodeContext;
+
+namespace {
+
+const TestingSetup* g_setup;
+std::vector<COutPoint> g_outpoints_coinbase_init_mature;
+
+struct MockedTxPool : public CTxMemPool {
+    void RollingFeeUpdate() EXCLUSIVE_LOCKS_REQUIRED(!cs)
+    {
+        LOCK(cs);
+        lastRollingFeeUpdate = GetTime();
+        blockSinceLastRollingFeeBump = true;
+    }
+};
+
+void initialize_tx_pool()
+{
+    static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
+    g_setup = testing_setup.get();
+
+    for (int i = 0; i < 2 * COINBASE_MATURITY; ++i) {
+        COutPoint prevout{MineBlock(g_setup->m_node, P2SH_OP_TRUE)};
+        if (i < COINBASE_MATURITY) {
+            // Remember the txids to avoid expensive disk access later on
+            g_outpoints_coinbase_init_mature.push_back(prevout);
+        }
+    }
+    SyncWithValidationInterfaceQueue();
+}
+
+struct OutpointsUpdater final : public CValidationInterface {
+    std::set<COutPoint>& m_mempool_outpoints;
+
+    explicit OutpointsUpdater(std::set<COutPoint>& r)
+        : m_mempool_outpoints{r} {}
+
+    void TransactionAddedToMempool(const CTransactionRef& tx, int64_t /* nAcceptTime */, uint64_t /* mempool_sequence */) override
+    {
+        // outputs from this tx can now be spent
+        for (uint32_t index{0}; index < tx->vout.size(); ++index) {
+            m_mempool_outpoints.insert(COutPoint{tx->GetHash(), index});
+        }
+    }
+
+    void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t /* mempool_sequence */) override
+    {
+        // outpoints spent by this tx are now available
+        for (const auto& input : tx->vin) {
+            m_mempool_outpoints.insert(input.prevout);
+        }
+        // outpoints created by this tx no longer exist
+        for (uint32_t index{0}; index < tx->vout.size(); ++index) {
+            m_mempool_outpoints.erase(COutPoint{tx->GetHash(), index});
+        }
+    }
+};
+
+struct TransactionsDelta final : public CValidationInterface {
+    std::set<CTransactionRef>& m_added;
+
+    explicit TransactionsDelta(std::set<CTransactionRef>& a)
+        : m_added{a} {}
+
+    void TransactionAddedToMempool(const CTransactionRef& tx, int64_t /* nAcceptTime */, uint64_t /* mempool_sequence */) override
+    {
+        // Transactions may be entered and booted any number of times
+        m_added.insert(tx);
+    }
+
+    void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t /* mempool_sequence */) override
+    {
+        // Transactions may be entered and booted any number of times
+         m_added.erase(tx);
+    }
+};
+
+void MockTime(FuzzedDataProvider& fuzzed_data_provider, const Chainstate& chainstate)
+{
+    const auto time = ConsumeTime(fuzzed_data_provider,
+                                  chainstate.m_chain.Tip()->GetMedianTimePast() + 1,
+                                  std::numeric_limits<decltype(chainstate.m_chain.Tip()->nTime)>::max());
+    SetMockTime(time);
+}
+
+CTxMemPool MakeMempool(FuzzedDataProvider& fuzzed_data_provider, const NodeContext& node)
+{
+    // Take the default options for tests...
+    CTxMemPool::Options mempool_opts{MemPoolOptionsForTest(node)};
+
+
+    // ...override specific options for this specific fuzz suite
+    mempool_opts.limits.ancestor_count = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 50);
+    mempool_opts.limits.ancestor_size_vbytes = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 202) * 1'000;
+    mempool_opts.limits.descendant_count = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 50);
+    mempool_opts.limits.descendant_size_vbytes = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 202) * 1'000;
+    mempool_opts.max_size_bytes = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 200) * 1'000'000;
+    mempool_opts.expiry = std::chrono::hours{fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(0, 999)};
+    nBytesPerSigOp = fuzzed_data_provider.ConsumeIntegralInRange<unsigned>(1, 999);
+
+    mempool_opts.estimator = nullptr;
+    mempool_opts.check_ratio = 1;
+    mempool_opts.require_standard = fuzzed_data_provider.ConsumeBool();
+
+    // ...and construct a CTxMemPool from it
+    return CTxMemPool{mempool_opts};
+}
+
+FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool)
+{
+    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
+    const auto& node = g_setup->m_node;
+    auto& chainstate{static_cast<DummyChainState&>(node.chainman->ActiveChainstate())};
+
+    MockTime(fuzzed_data_provider, chainstate);
+
+    // All spendable outpoints outside of the unsubmitted package
+    std::set<COutPoint> mempool_outpoints;
+    std::map<COutPoint, CAmount> outpoints_value;
+    for (const auto& outpoint : g_outpoints_coinbase_init_mature) {
+        Assert(mempool_outpoints.insert(outpoint).second);
+        outpoints_value[outpoint] = 500 * COIN;
+    }
+
+    auto outpoints_updater = std::make_shared<OutpointsUpdater>(mempool_outpoints);
+    RegisterSharedValidationInterface(outpoints_updater);
+
+    CTxMemPool tx_pool_{MakeMempool(fuzzed_data_provider, node)};
+    MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(&tx_pool_);
+
+    chainstate.SetMempool(&tx_pool);
+
+    LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 300)
+    {
+        Assert(!mempool_outpoints.empty());
+
+        std::vector<CTransactionRef> txs;
+
+        // Make packages of 1-to-26 transactions
+        const auto num_txs = (size_t) fuzzed_data_provider.ConsumeIntegralInRange<int>(1, 26);
+        std::set<COutPoint> package_outpoints;
+        while (txs.size() < num_txs) {
+
+            // Last transaction in a package needs to be a child of parents to get further in validation
+            // so the last transaction to be generated(in a >1 package) must spend all package-made outputs
+            // Note that this test currently only spends package outputs in last transaction.
+            bool last_tx = num_txs > 1 && txs.size() == num_txs - 1;
+
+            // Create transaction to add to the mempool
+            const CTransactionRef tx = [&] {
+                CMutableTransaction tx_mut;
+                tx_mut.nVersion = CTransaction::CURRENT_VERSION;
+                tx_mut.nLockTime = fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral<uint32_t>();
+                // Last tx will sweep all outpoints in package
+                const auto num_in = last_tx ? package_outpoints.size()  : fuzzed_data_provider.ConsumeIntegralInRange<int>(1, mempool_outpoints.size());
+                const auto num_out = fuzzed_data_provider.ConsumeIntegralInRange<int>(1, mempool_outpoints.size() * 2);
+
+                auto& outpoints = last_tx ? package_outpoints : mempool_outpoints;
+
+                Assert(!outpoints.empty());
+
+                CAmount amount_in{0};
+                for (size_t i = 0; i < num_in; ++i) {
+                    // Pop random outpoint
+                    auto pop = outpoints.begin();
+                    std::advance(pop, fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, outpoints.size() - 1));
+                    const auto outpoint = *pop;
+                    outpoints.erase(pop);
+                    // no need to update or erase from outpoints_value
+                    amount_in += outpoints_value.at(outpoint);
+
+                    // Create input
+                    const auto sequence = ConsumeSequence(fuzzed_data_provider);
+                    const auto script_sig = CScript{} << STACK_ELEM_OP_TRUE;
+                    CTxIn in;
+                    in.prevout = outpoint;
+                    in.nSequence = sequence;
+                    in.scriptSig = script_sig;
+
+                    tx_mut.vin.push_back(in);
+                }
+                const auto amount_fee = fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(0, amount_in);
+                const auto amount_out = (amount_in - amount_fee) / num_out;
+                for (int i = 0; i < num_out; ++i) {
+                    tx_mut.vout.emplace_back(amount_out, P2SH_OP_TRUE);
+                }
+                // TODO vary transaction sizes to catch size-related issues
+                auto tx = MakeTransactionRef(tx_mut);
+                // Restore previously removed outpoints, except in-package outpoints
+                if (!last_tx) {
+                    for (const auto& in : tx->vin) {
+                        Assert(outpoints.insert(in.prevout).second);
+                    }
+                    // Cache the in-package outpoints being made
+                    for (size_t i = 0; i < tx->vout.size(); ++i) {
+                        package_outpoints.emplace(tx->GetHash(), i);
+                    }
+                }
+                // We need newly-created values for the duration of this run
+                for (size_t i = 0; i < tx->vout.size(); ++i) {
+                    outpoints_value[COutPoint(tx->GetHash(), i)] = tx->vout[i].nValue;
+                }
+                return tx;
+            }();
+            txs.push_back(tx);
+        }
+
+        if (fuzzed_data_provider.ConsumeBool()) {
+            MockTime(fuzzed_data_provider, chainstate);
+        }
+        if (fuzzed_data_provider.ConsumeBool()) {
+            tx_pool.RollingFeeUpdate();
+        }
+        if (fuzzed_data_provider.ConsumeBool()) {
+            const auto& txid = fuzzed_data_provider.ConsumeBool() ?
+                                   txs.back()->GetHash() :
+                                   PickValue(fuzzed_data_provider, mempool_outpoints).hash;
+            const auto delta = fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(-50 * COIN, +50 * COIN);
+            tx_pool.PrioritiseTransaction(txid, delta);
+        }
+
+        // Remember all added transactions
+        std::set<CTransactionRef> added;
+        auto txr = std::make_shared<TransactionsDelta>(added);
+        RegisterSharedValidationInterface(txr);
+        const bool bypass_limits = fuzzed_data_provider.ConsumeBool();
+
+        // When there are multiple transactions in the package, we call ProcessNewPackage(txs, test_accept=false)
+        // and AcceptToMemoryPool(txs.back(), test_accept=true). When there is only 1 transaction, we might flip it
+        // (the package is a test accept and ATMP is a submission).
+        auto single_submit = txs.size() == 1 && fuzzed_data_provider.ConsumeBool();
+
+        const auto result_package = WITH_LOCK(::cs_main,
+                                    return ProcessNewPackage(chainstate, tx_pool, txs, /*test_accept=*/single_submit));
+        // If something went wrong due to a package-specific policy, it might not return a
+        // validation result for the transaction.
+        if (result_package.m_state.GetResult() != PackageValidationResult::PCKG_POLICY) {
+            auto it = result_package.m_tx_results.find(txs.back()->GetHash());
+            Assert(it != result_package.m_tx_results.end());
+            Assert(it->second.m_result_type == MempoolAcceptResult::ResultType::VALID ||
+                   it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID ||
+                   it->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY);
+        }
+
+        const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, txs.back(), GetTime(), bypass_limits, /*test_accept=*/!single_submit));
+        const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID;
+
+        SyncWithValidationInterfaceQueue();
+        UnregisterSharedValidationInterface(txr);
+
+        // There is only 1 transaction in the package. We did a test-package-accept and a ATMP
+        if (single_submit) {
+            Assert(accepted != added.empty());
+            Assert(accepted == res.m_state.IsValid());
+            if (accepted) {
+                Assert(added.size() == 1);
+                Assert(txs.back() == *added.begin());
+            }
+        } else {
+            // This is empty if it fails early checks, or "full" if transactions are looked at deeper
+            Assert(result_package.m_tx_results.size() == txs.size() || result_package.m_tx_results.empty());
+        }
+    }
+
+    UnregisterSharedValidationInterface(outpoints_updater);
+
+    WITH_LOCK(::cs_main, tx_pool.check(chainstate.CoinsTip(), chainstate.m_chain.Height() + 1));
+}
+} // namespace

From 022d734c8db6d1a56328f494ca2a839a31e3f87c Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Wed, 11 Nov 2026 03:50:40 +0700
Subject: [PATCH 07/12] test: drop RBF naming from the tx_pool fuzz target

Dash has no replace-by-fee, so the set holds every outpoint a fuzzed
transaction may spend, not the ones spendable through a replacement.
---
 src/test/fuzz/tx_pool.cpp | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp
index 176318a4a212..6e2479ce6182 100644
--- a/src/test/fuzz/tx_pool.cpp
+++ b/src/test/fuzz/tx_pool.cpp
@@ -137,14 +137,14 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
 
     MockTime(fuzzed_data_provider, chainstate);
 
-    // All RBF-spendable outpoints
-    std::set<COutPoint> outpoints_rbf;
-    // All outpoints counting toward the total supply (subset of outpoints_rbf)
+    // All spendable outpoints
+    std::set<COutPoint> outpoints_spendable;
+    // All outpoints counting toward the total supply (subset of outpoints_spendable)
     std::set<COutPoint> outpoints_supply;
     for (const auto& outpoint : g_outpoints_coinbase_init_mature) {
         Assert(outpoints_supply.insert(outpoint).second);
     }
-    outpoints_rbf = outpoints_supply;
+    outpoints_spendable = outpoints_supply;
 
     // The sum of the values of all spendable outpoints
     constexpr CAmount SUPPLY_TOTAL{COINBASE_MATURITY * 50 * COIN};
@@ -180,16 +180,16 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
             CMutableTransaction tx_mut;
             tx_mut.nVersion = CTransaction::CURRENT_VERSION;
             tx_mut.nLockTime = fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral<uint32_t>();
-            const auto num_in = fuzzed_data_provider.ConsumeIntegralInRange<int>(1, outpoints_rbf.size());
-            const auto num_out = fuzzed_data_provider.ConsumeIntegralInRange<int>(1, outpoints_rbf.size() * 2);
+            const auto num_in = fuzzed_data_provider.ConsumeIntegralInRange<int>(1, outpoints_spendable.size());
+            const auto num_out = fuzzed_data_provider.ConsumeIntegralInRange<int>(1, outpoints_spendable.size() * 2);
 
             CAmount amount_in{0};
             for (int i = 0; i < num_in; ++i) {
                 // Pop random outpoint
-                auto pop = outpoints_rbf.begin();
-                std::advance(pop, fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, outpoints_rbf.size() - 1));
+                auto pop = outpoints_spendable.begin();
+                std::advance(pop, fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, outpoints_spendable.size() - 1));
                 const auto outpoint = *pop;
-                outpoints_rbf.erase(pop);
+                outpoints_spendable.erase(pop);
                 amount_in += GetAmount(outpoint);
 
                 // Create input
@@ -210,7 +210,7 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
             auto tx = MakeTransactionRef(tx_mut);
             // Restore previously removed outpoints
             for (const auto& in : tx->vin) {
-                Assert(outpoints_rbf.insert(in.prevout).second);
+                Assert(outpoints_spendable.insert(in.prevout).second);
             }
             return tx;
         }();
@@ -224,7 +224,7 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
         if (fuzzed_data_provider.ConsumeBool()) {
             const auto& txid = fuzzed_data_provider.ConsumeBool() ?
                                    tx->GetHash() :
-                                   PickValue(fuzzed_data_provider, outpoints_rbf).hash;
+                                   PickValue(fuzzed_data_provider, outpoints_spendable).hash;
             const auto delta = fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(-50 * COIN, +50 * COIN);
             tx_pool.PrioritiseTransaction(txid, delta);
         }
@@ -289,11 +289,11 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
                 insert_tx(/*created_by_tx=*/{consumed_erased}, /*consumed_by_tx=*/{outpoints_supply}, /*tx=*/*removed_tx);
             }
             for (const auto& added_tx : added) {
-                insert_tx(/*created_by_tx=*/{outpoints_supply, outpoints_rbf}, /*consumed_by_tx=*/{consumed_supply}, /*tx=*/*added_tx);
+                insert_tx(/*created_by_tx=*/{outpoints_supply, outpoints_spendable}, /*consumed_by_tx=*/{consumed_supply}, /*tx=*/*added_tx);
             }
             for (const auto& p : consumed_erased) {
                 Assert(outpoints_supply.erase(p) == 1);
-                Assert(outpoints_rbf.erase(p) == 1);
+                Assert(outpoints_spendable.erase(p) == 1);
             }
             for (const auto& p : consumed_supply) {
                 Assert(outpoints_supply.erase(p) == 1);

From 6bd3bc4e3d622427f0e0dbcccfe10c8001be3b21 Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Thu, 21 Sep 2023 12:02:44 -0400
Subject: [PATCH 08/12] Merge bitcoin/bitcoin#28471: Fix virtual size limit
 enforcement in transaction package context

eb8f58f5e4a249d55338304099e8238896d2316d Add functional test to catch too large vsize packages (Greg Sanders)
1a579f9d01256b0b2570168496d129a8b2009b35 Handle over-sized (in virtual bytes) packages with no in-mempool ancestors (Greg Sanders)
bc013fe8e3d0bae2ab766a8248219aa3c9e344df Bugfix: Pass correct virtual size to CheckPackageLimits (Luke Dashjr)
533660c58ad5a218671a9bc1537299b1d67bb55d Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT to avoid vbyte confusion (Greg Sanders)

Pull request description:

  (Alternative) Minimal subset of https://github.com/bitcoin/bitcoin/pull/28345 to:

  1) Replace MAX_PACKAGE_SIZE with MAX_PACKAGE_WEIGHT which accounts for additional WU necessary to not exclude default chain limit transactions that would have been accepted individually. Avoids sigops vbyte confusion.
  2) pass correct vsize to chain limit evaluations in package context
  3) stop overly-large packages that have no existing mempool ancestors (also a bugfix by itself if someone sets non-standard chain limits)

  This should fix the known issues while not blocking additional refactoring later.

ACKs for top commit:
  achow101:
    ACK eb8f58f5e4a249d55338304099e8238896d2316d
  ariard:
    Re-Code ACK eb8f58f5e
  glozow:
    reACK eb8f58f5e4a249d55338304099e8238896d2316d

Tree-SHA512: 1b5cca1a526207e25d387fcc29a776a3198c3a013dc2b35c6275b9d5a64db2476c154ebf52e3a1aed0b9924c75613f21a946577aa760de28cadf0c9c7f68dc39

Co-authored-by: Andrew Chow <github@achow101.com>
---
 src/policy/packages.h                 |  9 ++---
 src/txmempool.cpp                     | 20 ++++++++---
 src/txmempool.h                       |  4 ++-
 src/validation.cpp                    |  6 ++--
 test/functional/mempool_sigoplimit.py | 49 +++++++++++++++++++++++++++
 5 files changed, 77 insertions(+), 11 deletions(-)

diff --git a/src/policy/packages.h b/src/policy/packages.h
index c6fe963042ab..e84e0c64c8a9 100644
--- a/src/policy/packages.h
+++ b/src/policy/packages.h
@@ -18,14 +18,15 @@ static constexpr uint32_t MAX_PACKAGE_COUNT{25};
 static constexpr uint32_t MAX_PACKAGE_SIZE{101};
 static_assert(MAX_PACKAGE_SIZE * 1000 >= MAX_STANDARD_TX_SIZE);
 
-// If a package is submitted, it must be within the mempool's ancestor/descendant limits. Since a
-// submitted package must be child-with-unconfirmed-parents (all of the transactions are an ancestor
+// If a package is to be evaluated, it must be at least as large as the mempool's ancestor/descendant limits,
+// otherwise transactions that would be individually accepted may be rejected in a package erroneously.
+// Since a submitted package must be child-with-unconfirmed-parents (all of the transactions are an ancestor
 // of the child), package limits are ultimately bounded by mempool package limits. Ensure that the
 // defaults reflect this constraint.
 static_assert(DEFAULT_DESCENDANT_LIMIT >= MAX_PACKAGE_COUNT);
 static_assert(DEFAULT_ANCESTOR_LIMIT >= MAX_PACKAGE_COUNT);
-static_assert(DEFAULT_ANCESTOR_SIZE_LIMIT_KVB >= MAX_PACKAGE_SIZE);
-static_assert(DEFAULT_DESCENDANT_SIZE_LIMIT_KVB >= MAX_PACKAGE_SIZE);
+static_assert(MAX_PACKAGE_SIZE >= DEFAULT_ANCESTOR_SIZE_LIMIT_KVB);
+static_assert(MAX_PACKAGE_SIZE >= DEFAULT_DESCENDANT_SIZE_LIMIT_KVB);
 
 /** A "reason" why a package was invalid. It may be that one or more of the included
  * transactions is invalid or the package itself violates our rules.
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 4b1199e69114..09aab283ebb8 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -204,12 +204,24 @@ util::Result<CTxMemPool::setEntries> CTxMemPool::CalculateAncestorsAndCheckLimit
     return ancestors;
 }
 
-util::Result<void> CTxMemPool::CheckPackageLimits(const Package& package) const
+util::Result<void> CTxMemPool::CheckPackageLimits(const Package& package,
+                                                  const int64_t total_vsize) const
 {
+    size_t pack_count = package.size();
+
+    // Package itself is busting mempool limits; should be rejected even if no staged_ancestors exist
+    if (pack_count > static_cast<uint64_t>(m_limits.ancestor_count)) {
+        return util::Error{Untranslated(strprintf("package count %u exceeds ancestor count limit [limit: %u]", pack_count, m_limits.ancestor_count))};
+    } else if (pack_count > static_cast<uint64_t>(m_limits.descendant_count)) {
+        return util::Error{Untranslated(strprintf("package count %u exceeds descendant count limit [limit: %u]", pack_count, m_limits.descendant_count))};
+    } else if (total_vsize > m_limits.ancestor_size_vbytes) {
+        return util::Error{Untranslated(strprintf("package size %u exceeds ancestor size limit [limit: %u]", total_vsize, m_limits.ancestor_size_vbytes))};
+    } else if (total_vsize > m_limits.descendant_size_vbytes) {
+        return util::Error{Untranslated(strprintf("package size %u exceeds descendant size limit [limit: %u]", total_vsize, m_limits.descendant_size_vbytes))};
+    }
+
     CTxMemPoolEntry::Parents staged_ancestors;
-    int64_t total_size = 0;
     for (const auto& tx : package) {
-        total_size += GetVirtualTransactionSize(*tx);
         for (const auto& input : tx->vin) {
             std::optional<txiter> piter = GetIter(input.prevout.hash);
             if (piter) {
@@ -223,7 +235,7 @@ util::Result<void> CTxMemPool::CheckPackageLimits(const Package& package) const
     // When multiple transactions are passed in, the ancestors and descendants of all transactions
     // considered together must be within limits even if they are not interdependent. This may be
     // stricter than the limits for each individual transaction.
-    const auto ancestors{CalculateAncestorsAndCheckLimits(total_size, package.size(),
+    const auto ancestors{CalculateAncestorsAndCheckLimits(total_vsize, package.size(),
                                                           staged_ancestors, m_limits)};
     // It's possible to overestimate the ancestor/descendant totals.
     if (!ancestors.has_value()) return util::Error{Untranslated("possibly " + util::ErrorString(ancestors).original)};
diff --git a/src/txmempool.h b/src/txmempool.h
index 3fd0fb4e170b..6dff8e0cd711 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -646,9 +646,11 @@ class CTxMemPool
      * @param[in]       package                 Transaction package being evaluated for acceptance
      *                                          to mempool. The transactions need not be direct
      *                                          ancestors/descendants of each other.
+     * @param[in]       total_vsize             Sum of virtual sizes for all transactions in package.
      * @returns {} or the error reason if a limit is hit.
      */
-    util::Result<void> CheckPackageLimits(const Package& package) const EXCLUSIVE_LOCKS_REQUIRED(cs);
+    util::Result<void> CheckPackageLimits(const Package& package,
+                                          int64_t total_vsize) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     /** Populate setDescendants with all in-mempool descendants of hash.
      *  Assumes that setDescendants includes all in-mempool descendants of anything
diff --git a/src/validation.cpp b/src/validation.cpp
index 3a34f9547844..6a8c3d79497f 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -750,6 +750,7 @@ class MemPoolAccept
     // Enforce package mempool ancestor/descendant limits (distinct from individual
     // ancestor/descendant limits done in PreChecks).
     bool PackageMempoolChecks(const std::vector<CTransactionRef>& txns,
+                              int64_t total_vsize,
                               PackageValidationState& package_state) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs);
 
     // Admitting an asset unlock evicts every other mempool claimant of its withdrawal index along
@@ -1135,6 +1136,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
 }
 
 bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txns,
+                                         const int64_t total_vsize,
                                          PackageValidationState& package_state)
 {
     AssertLockHeld(cs_main);
@@ -1144,7 +1146,7 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txn
     assert(std::all_of(txns.cbegin(), txns.cend(), [this](const auto& tx)
                        { return !m_pool.exists(tx->GetHash());}));
 
-    auto result = m_pool.CheckPackageLimits(txns);
+    auto result = m_pool.CheckPackageLimits(txns, total_vsize);
     if (!result) {
         // This is a package-wide error, separate from an individual transaction error.
         return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-mempool-limits", util::ErrorString(result).original);
@@ -1523,7 +1525,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
     // because it's unnecessary. Also, CPFP carve out can increase the limit for individual
     // transactions, but this exemption is not extended to packages in CheckPackageLimits().
     std::string err_string;
-    if (txns.size() > 1 && !PackageMempoolChecks(txns, package_state)) {
+    if (txns.size() > 1 && !PackageMempoolChecks(txns, m_total_vsize, package_state)) {
         return PackageMempoolAcceptResult(package_state, std::move(results));
     }
 
diff --git a/test/functional/mempool_sigoplimit.py b/test/functional/mempool_sigoplimit.py
index 3b4bfb45bde9..2b1bee5bd9b5 100755
--- a/test/functional/mempool_sigoplimit.py
+++ b/test/functional/mempool_sigoplimit.py
@@ -3,8 +3,11 @@
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test sigop limit mempool policy (`-bytespersigop` parameter)"""
+from decimal import Decimal
 from math import ceil
 
+from test_framework.key import ECKey
+
 from test_framework.messages import (
     COutPoint,
     CTransaction,
@@ -22,12 +25,15 @@
     OP_TRUE,
 )
 from test_framework.script_util import (
+    keys_to_multisig_script,
     script_to_p2sh_script,
 )
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
+    assert_greater_than,
     assert_greater_than_or_equal,
+    assert_raises_rpc_error,
 )
 from test_framework.wallet import MiniWallet
 
@@ -109,6 +115,47 @@ def test_sigops_limit(self, bytes_per_sigop, num_sigops):
         assert_equal(res['allowed'], True)
         assert_equal(res['vsize'], sigop_equivalent_vsize)
 
+    def test_sigops_package(self):
+        self.log.info("Test a overly-large sigops-vbyte hits package limits")
+        # Make a 2-transaction package which fails vbyte checks even though
+        # separately they would work.
+        self.restart_node(0, extra_args=["-bytespersigop=5000"] + self.extra_args[0])
+
+        def create_bare_multisig_tx(utxo_to_spend=None):
+            key = ECKey()
+            key.generate()
+            pubkey = key.get_pubkey().get_bytes()
+            amount_for_bare = 50000
+            tx_dict = self.wallet.create_self_transfer(fee=Decimal("3"), utxo_to_spend=utxo_to_spend)
+            tx_utxo = tx_dict["new_utxo"]
+            tx = tx_dict["tx"]
+            tx.vout.append(CTxOut(amount_for_bare, keys_to_multisig_script([pubkey], k=1)))
+            tx.vout[0].nValue -= amount_for_bare
+            tx_utxo["txid"] = tx.rehash()
+            tx_utxo["value"] -= Decimal("0.00005000")
+            return (tx_utxo, tx)
+
+        tx_parent_utxo, tx_parent = create_bare_multisig_tx()
+        tx_child_utxo, tx_child = create_bare_multisig_tx(tx_parent_utxo)
+
+        # Separately, the parent tx is ok
+        parent_individual_testres = self.nodes[0].testmempoolaccept([tx_parent.serialize().hex()])[0]
+        assert parent_individual_testres["allowed"]
+        # Multisig is counted as MAX_PUBKEYS_PER_MULTISIG = 20 sigops
+        assert_equal(parent_individual_testres["vsize"], 5000 * 20)
+
+        # But together, it's exceeding limits in the *package* context. If sigops adjusted vsize wasn't being checked
+        # here, it would get further in validation and give too-long-mempool-chain error instead.
+        packet_test = self.nodes[0].testmempoolaccept([tx_parent.serialize().hex(), tx_child.serialize().hex()])
+        assert_equal([x["package-error"] for x in packet_test], ["package-mempool-limits", "package-mempool-limits"])
+
+        # When we actually try to submit, the parent makes it into the mempool, but the child would exceed ancestor vsize limits
+        assert_raises_rpc_error(-26, "too-long-mempool-chain", self.nodes[0].submitpackage, [tx_parent.serialize().hex(), tx_child.serialize().hex()])
+        assert tx_parent.rehash() in self.nodes[0].getrawmempool()
+
+        # Transactions are tiny in weight
+        assert_greater_than(2000, tx_parent.get_weight() + tx_child.get_weight())
+
     def run_test(self):
         self.wallet = MiniWallet(self.nodes[0])
 
@@ -126,6 +173,8 @@ def run_test(self):
                     continue
                 self.test_sigops_limit(bytes_per_sigop, num_sigops)
 
+        self.test_sigops_package()
+
 
 if __name__ == '__main__':
     BytesPerSigOpTest().main()

From 0ba25de4b38195c9cc40543b81cae6b379479c17 Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Tue, 26 Sep 2023 13:46:19 +0100
Subject: [PATCH 09/12] Merge bitcoin/bitcoin#28483: refactor: Return CAutoFile
 from BlockManager::Open*File()

fa56c421be04af846f479c30749b17e6663ab418 Return CAutoFile from BlockManager::Open*File() (MarcoFalke)
9999b89cd37fb2a23c5ebcd91d9cb31d69375933 Make BufferedFile to be a CAutoFile wrapper (MarcoFalke)
fa389d902fbf5fac19fba8c5d0c5e0a25f15ca63 refactor: Drop unused fclose() from BufferedFile (MarcoFalke)

Pull request description:

  This is required for https://github.com/bitcoin/bitcoin/pull/28052, but makes sense on its own, because offloading logic to `CAutoFile` instead of re-implementing it allows to delete code and complexity.

ACKs for top commit:
  TheCharlatan:
    Re-ACK fa56c421be04af846f479c30749b17e6663ab418
  willcl-ark:
    tACK fa56c421be

Tree-SHA512: fe4638f3a6bd3f9d968cfb9ae3259c9d6cd278fe2912cbc90289851311c8c781099db4c160e775960975c4739098d9af801a8d2d12603f371f8edfe134d8f85a

Co-authored-by: fanquake <fanquake@gmail.com>
---
 src/bench/load_external.cpp                |  3 +-
 src/bench/streams_findbyte.cpp             | 16 ++++++----
 src/index/txindex.cpp                      |  2 +-
 src/node/blockstorage.cpp                  | 26 ++++++++--------
 src/node/blockstorage.h                    |  5 +--
 src/streams.h                              | 36 +++++-----------------
 src/test/blockmanager_tests.cpp            |  6 ++--
 src/test/fuzz/buffered_file.cpp            |  9 ++----
 src/test/fuzz/load_external_block_file.cpp |  5 +--
 src/test/streams_tests.cpp                 | 30 +++++++++---------
 src/validation.cpp                         |  5 ++-
 src/validation.h                           |  5 ++-
 12 files changed, 65 insertions(+), 83 deletions(-)

diff --git a/src/bench/load_external.cpp b/src/bench/load_external.cpp
index 33667b02cc5e..f2bc8346c0a8 100644
--- a/src/bench/load_external.cpp
+++ b/src/bench/load_external.cpp
@@ -5,6 +5,7 @@
 #include <bench/bench.h>
 #include <bench/data.h>
 #include <chainparams.h>
+#include <clientversion.h>
 #include <test/util/setup_common.h>
 #include <util/chaintype.h>
 #include <validation.h>
@@ -54,7 +55,7 @@ static void LoadExternalBlockFile(benchmark::Bench& bench)
     bench.run([&] {
         // "rb" is "binary, O_RDONLY", positioned to the start of the file.
         // The file will be closed by LoadExternalBlockFile().
-        FILE* file{fsbridge::fopen(blkfile, "rb")};
+        CAutoFile file{fsbridge::fopen(blkfile, "rb"), CLIENT_VERSION};
         testing_setup->m_node.chainman->LoadExternalBlockFile(file, &pos, &blocks_with_unknown_parent);
     });
     fs::remove(blkfile);
diff --git a/src/bench/streams_findbyte.cpp b/src/bench/streams_findbyte.cpp
index 22b8f1b356b9..4aaccb2af842 100644
--- a/src/bench/streams_findbyte.cpp
+++ b/src/bench/streams_findbyte.cpp
@@ -4,19 +4,23 @@
 
 #include <bench/bench.h>
 
-#include <util/fs.h>
 #include <streams.h>
+#include <util/fs.h>
+
+#include <cstddef>
+#include <cstdint>
+#include <cstdio>
 
 static void FindByte(benchmark::Bench& bench)
 {
     // Setup
-    FILE* file = fsbridge::fopen("streams_tmp", "w+b");
+    CAutoFile file{fsbridge::fopen("streams_tmp", "w+b"), 0};
     const size_t file_size = 200;
     uint8_t data[file_size] = {0};
     data[file_size-1] = 1;
-    fwrite(&data, sizeof(uint8_t), file_size, file);
-    rewind(file);
-    BufferedFile bf{file, /*nBufSize=*/file_size + 1, /*nRewindIn=*/file_size, 0};
+    file << data;
+    std::rewind(file.Get());
+    BufferedFile bf{file, /*nBufSize=*/file_size + 1, /*nRewindIn=*/file_size};
 
     bench.run([&] {
         bf.SetPos(0);
@@ -24,7 +28,7 @@ static void FindByte(benchmark::Bench& bench)
     });
 
     // Cleanup
-    bf.fclose();
+    file.fclose();
     fs::remove("streams_tmp");
 }
 
diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp
index e6b7c6358fae..73cb113415c1 100644
--- a/src/index/txindex.cpp
+++ b/src/index/txindex.cpp
@@ -84,7 +84,7 @@ bool TxIndex::FindTx(const uint256& tx_hash, uint256& block_hash, CTransactionRe
         return false;
     }
 
-    AutoFile file{m_chainstate->m_blockman.OpenBlockFile(RecordHeaderPos(postx), true)};
+    CAutoFile file{m_chainstate->m_blockman.OpenBlockFile(RecordHeaderPos(postx), true)};
     if (file.IsNull()) {
         LogError("%s: OpenBlockFile failed\n", __func__);
         return false;
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index 6c27552bcd89..df9a6077e0e2 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -390,7 +390,7 @@ bool BlockManager::LoadBlockIndexDB()
     }
     for (std::set<int>::iterator it = setBlkDataFiles.begin(); it != setBlkDataFiles.end(); it++) {
         FlatFilePos pos(*it, 0);
-        if (AutoFile{OpenBlockFile(pos, true)}.IsNull()) {
+        if (OpenBlockFile(pos, true).IsNull()) {
             return false;
         }
     }
@@ -509,7 +509,7 @@ CBlockFileInfo* BlockManager::GetBlockFileInfo(size_t n)
 bool BlockManager::UndoWriteToDisk(const CBlockUndo& blockundo, const std::optional<std::vector<std::byte>>& compressed, uint32_t size_field, FlatFilePos& pos, const uint256& hashBlock, const CMessageHeader::MessageStartChars& messageStart) const
 {
     // Open history file to append
-    AutoFile fileout{OpenUndoFile(pos)};
+    CAutoFile fileout{OpenUndoFile(pos)};
     if (fileout.IsNull()) {
         LogError("%s: OpenUndoFile failed\n", __func__);
         return false;
@@ -550,7 +550,7 @@ bool BlockManager::UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex& in
     }
 
     // Open history file to read
-    AutoFile filein{OpenUndoFile(RecordHeaderPos(pos), true)};
+    CAutoFile filein{OpenUndoFile(RecordHeaderPos(pos), true)};
     if (filein.IsNull()) {
         LogError("%s: OpenUndoFile failed\n", __func__);
         return false;
@@ -662,15 +662,15 @@ FlatFileSeq BlockManager::UndoFileSeq() const
     return FlatFileSeq(m_opts.blocks_dir, "rev", m_opts.tiny_blk ? 0x10000 /* 64kb */ : UNDOFILE_CHUNK_SIZE);
 }
 
-FILE* BlockManager::OpenBlockFile(const FlatFilePos& pos, bool fReadOnly) const
+CAutoFile BlockManager::OpenBlockFile(const FlatFilePos& pos, bool fReadOnly) const
 {
-    return BlockFileSeq().Open(pos, fReadOnly);
+    return CAutoFile{BlockFileSeq().Open(pos, fReadOnly), CLIENT_VERSION};
 }
 
 /** Open an undo file (rev?????.dat) */
-FILE* BlockManager::OpenUndoFile(const FlatFilePos& pos, bool fReadOnly) const
+CAutoFile BlockManager::OpenUndoFile(const FlatFilePos& pos, bool fReadOnly) const
 {
-    return UndoFileSeq().Open(pos, fReadOnly);
+    return CAutoFile{UndoFileSeq().Open(pos, fReadOnly), CLIENT_VERSION};
 }
 
 fs::path BlockManager::GetBlockPosFilename(const FlatFilePos& pos) const
@@ -782,7 +782,7 @@ bool BlockManager::FindUndoPos(BlockValidationState& state, int nFile, FlatFileP
 bool BlockManager::WriteBlockToDisk(Span<const std::byte> payload, uint32_t size_field, FlatFilePos& pos, const CMessageHeader::MessageStartChars& messageStart) const
 {
     // Open history file to append
-    AutoFile fileout{OpenBlockFile(pos)};
+    CAutoFile fileout{OpenBlockFile(pos)};
     if (fileout.IsNull()) {
         LogError("WriteBlockToDisk: OpenBlockFile failed\n");
         return false;
@@ -904,7 +904,7 @@ std::optional<uint256> BlockManager::ReadBlockFromDisk(CBlock& block, const Flat
     block.SetNull();
 
     // Open history file to read
-    AutoFile filein{OpenBlockFile(RecordHeaderPos(pos), true)};
+    CAutoFile filein{OpenBlockFile(RecordHeaderPos(pos), true)};
     if (filein.IsNull()) {
         LogError("ReadBlockFromDisk: OpenBlockFile failed for %s\n", pos.ToString());
         return std::nullopt;
@@ -1027,8 +1027,8 @@ void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFile
                 if (!fs::exists(chainman.m_blockman.GetBlockPosFilename(pos))) {
                     break; // No block files left to reindex
                 }
-                FILE* file = chainman.m_blockman.OpenBlockFile(pos, true);
-                if (!file) {
+                CAutoFile file = chainman.m_blockman.OpenBlockFile(pos, true);
+                if (file.IsNull()) {
                     break; // This error is logged in OpenBlockFile
                 }
                 LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile);
@@ -1048,8 +1048,8 @@ void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFile
 
         // -loadblock=
         for (const fs::path& path : vImportFiles) {
-            FILE *file = fsbridge::fopen(path, "rb");
-            if (file) {
+            CAutoFile file{fsbridge::fopen(path, "rb"), CLIENT_VERSION};
+            if (!file.IsNull()) {
                 LogPrintf("Importing blocks file %s...\n", fs::PathToString(path));
                 chainman.LoadExternalBlockFile(file);
                 if (ShutdownRequested()) {
diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h
index 8e70b2cdc156..4262aa969d18 100644
--- a/src/node/blockstorage.h
+++ b/src/node/blockstorage.h
@@ -22,6 +22,7 @@
 
 class AutoFile;
 class BlockValidationState;
+class CAutoFile;
 class CBlock;
 class CBlockUndo;
 class CChainParams;
@@ -114,7 +115,7 @@ class BlockManager
     FlatFileSeq BlockFileSeq() const;
     FlatFileSeq UndoFileSeq() const;
 
-    FILE* OpenUndoFile(const FlatFilePos& pos, bool fReadOnly = false) const;
+    CAutoFile OpenUndoFile(const FlatFilePos& pos, bool fReadOnly = false) const;
 
     bool WriteBlockToDisk(Span<const std::byte> payload, uint32_t size_field, FlatFilePos& pos, const CMessageHeader::MessageStartChars& messageStart) const;
     /** Size field of the record already stored at pos */
@@ -262,7 +263,7 @@ class BlockManager
     void UpdatePruneLock(const std::string& name, const PruneLockInfo& lock_info) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
     /** Open a block file (blk?????.dat) */
-    FILE* OpenBlockFile(const FlatFilePos& pos, bool fReadOnly = false) const;
+    CAutoFile OpenBlockFile(const FlatFilePos& pos, bool fReadOnly = false) const;
 
     /** Translation to a filesystem path */
     fs::path GetBlockPosFilename(const FlatFilePos& pos) const;
diff --git a/src/streams.h b/src/streams.h
index 4fd854b8ef8b..aabf4d506430 100644
--- a/src/streams.h
+++ b/src/streams.h
@@ -584,7 +584,7 @@ class CAutoFile : public AutoFile
     }
 };
 
-/** Non-refcounted RAII wrapper around a FILE* that implements a ring buffer to
+/** Wrapper around a CAutoFile& that implements a ring buffer to
  *  deserialize from. It guarantees the ability to rewind a given number of bytes.
  *
  *  Will automatically close the file when it goes out of scope if not null.
@@ -593,9 +593,7 @@ class CAutoFile : public AutoFile
 class BufferedFile
 {
 private:
-    const int nVersion;
-
-    FILE *src;            //!< source file
+    CAutoFile& m_src;
     uint64_t nSrcPos{0};  //!< how many bytes have been read from source
     uint64_t m_read_pos{0}; //!< how many bytes have been read from this
     uint64_t nReadLimit;  //!< up to which position we're allowed to read
@@ -611,9 +609,9 @@ class BufferedFile
             readNow = nAvail;
         if (readNow == 0)
             return false;
-        size_t nBytes = fread((void*)&vchBuf[pos], 1, readNow, src);
+        size_t nBytes{m_src.detail_fread(Span{vchBuf}.subspan(pos, readNow))};
         if (nBytes == 0) {
-            throw std::ios_base::failure(feof(src) ? "BufferedFile::Fill: end of file" : "BufferedFile::Fill: fread failed");
+            throw std::ios_base::failure{m_src.feof() ? "BufferedFile::Fill: end of file" : "BufferedFile::Fill: fread failed"};
         }
         nSrcPos += nBytes;
         return true;
@@ -642,36 +640,18 @@ class BufferedFile
     }
 
 public:
-    BufferedFile(FILE* fileIn, uint64_t nBufSize, uint64_t nRewindIn, int nVersionIn)
-        : nVersion{nVersionIn}, nReadLimit{std::numeric_limits<uint64_t>::max()}, nRewind{nRewindIn}, vchBuf(nBufSize, std::byte{0})
+    BufferedFile(CAutoFile& file, uint64_t nBufSize, uint64_t nRewindIn)
+        : m_src{file}, nReadLimit{std::numeric_limits<uint64_t>::max()}, nRewind{nRewindIn}, vchBuf(nBufSize, std::byte{0})
     {
         if (nRewindIn >= nBufSize)
             throw std::ios_base::failure("Rewind limit must be less than buffer size");
-        src = fileIn;
-    }
-
-    ~BufferedFile()
-    {
-        fclose();
     }
 
-    // Disallow copies
-    BufferedFile(const BufferedFile&) = delete;
-    BufferedFile& operator=(const BufferedFile&) = delete;
-
-    int GetVersion() const { return nVersion; }
-
-    void fclose()
-    {
-        if (src) {
-            ::fclose(src);
-            src = nullptr;
-        }
-    }
+    int GetVersion() const { return m_src.GetVersion(); }
 
     //! check whether we're at the end of the source file
     bool eof() const {
-        return m_read_pos == nSrcPos && feof(src);
+        return m_read_pos == nSrcPos && m_src.feof();
     }
 
     //! read a number of bytes
diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp
index 052097dd0ceb..a9b8c6f2e2e7 100644
--- a/src/test/blockmanager_tests.cpp
+++ b/src/test/blockmanager_tests.cpp
@@ -275,13 +275,13 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_scan_unlink_already_pruned_files, TestChain
     // Check that the file is not unlinked after ScanAndUnlinkAlreadyPrunedFiles
     // if m_have_pruned is not yet set
     WITH_LOCK(chainman.GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles());
-    BOOST_CHECK(!CAutoFile(blockman.OpenBlockFile(pos, true), CLIENT_VERSION).IsNull());
+    BOOST_CHECK(!blockman.OpenBlockFile(pos, true).IsNull());
 
     // Check that the file is unlinked after ScanAndUnlinkAlreadyPrunedFiles
     // once m_have_pruned is set
     blockman.m_have_pruned = true;
     WITH_LOCK(chainman.GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles());
-    BOOST_CHECK(CAutoFile(blockman.OpenBlockFile(pos, true), CLIENT_VERSION).IsNull());
+    BOOST_CHECK(blockman.OpenBlockFile(pos, true).IsNull());
 
     // Check that calling with already pruned files doesn't cause an error
     WITH_LOCK(chainman.GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles());
@@ -291,7 +291,7 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_scan_unlink_already_pruned_files, TestChain
     BOOST_CHECK_NE(old_tip, new_tip);
     const int new_file_number{WITH_LOCK(chainman.GetMutex(), return new_tip->GetBlockPos().nFile)};
     const FlatFilePos new_pos(new_file_number, 0);
-    BOOST_CHECK(!CAutoFile(blockman.OpenBlockFile(new_pos, true), CLIENT_VERSION).IsNull());
+    BOOST_CHECK(!blockman.OpenBlockFile(new_pos, true).IsNull());
 }
 
 BOOST_AUTO_TEST_CASE(blockmanager_flush_block_file)
diff --git a/src/test/fuzz/buffered_file.cpp b/src/test/fuzz/buffered_file.cpp
index 1116274e3d23..636f11b381b2 100644
--- a/src/test/fuzz/buffered_file.cpp
+++ b/src/test/fuzz/buffered_file.cpp
@@ -19,15 +19,12 @@ FUZZ_TARGET(buffered_file)
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
     FuzzedFileProvider fuzzed_file_provider = ConsumeFile(fuzzed_data_provider);
     std::optional<BufferedFile> opt_buffered_file;
-    FILE* fuzzed_file = fuzzed_file_provider.open();
+    CAutoFile fuzzed_file{fuzzed_file_provider.open(), 0};
     try {
-        opt_buffered_file.emplace(fuzzed_file, fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096), fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096), fuzzed_data_provider.ConsumeIntegral<int>());
+        opt_buffered_file.emplace(fuzzed_file, fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096), fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096));
     } catch (const std::ios_base::failure&) {
-        if (fuzzed_file != nullptr) {
-            fclose(fuzzed_file);
-        }
     }
-    if (opt_buffered_file && fuzzed_file != nullptr) {
+    if (opt_buffered_file && !fuzzed_file.IsNull()) {
         bool setpos_fail = false;
         LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) {
             CallOneOf(
diff --git a/src/test/fuzz/load_external_block_file.cpp b/src/test/fuzz/load_external_block_file.cpp
index ca7ae63db7d8..837cbc59f3d4 100644
--- a/src/test/fuzz/load_external_block_file.cpp
+++ b/src/test/fuzz/load_external_block_file.cpp
@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <chainparams.h>
+#include <clientversion.h>
 #include <flatfile.h>
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
@@ -27,8 +28,8 @@ FUZZ_TARGET(load_external_block_file, .init = initialize_load_external_block_fil
 {
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
     FuzzedFileProvider fuzzed_file_provider = ConsumeFile(fuzzed_data_provider);
-    FILE* fuzzed_block_file = fuzzed_file_provider.open();
-    if (fuzzed_block_file == nullptr) {
+    CAutoFile fuzzed_block_file{fuzzed_file_provider.open(), CLIENT_VERSION};
+    if (fuzzed_block_file.IsNull()) {
         return;
     }
     if (fuzzed_data_provider.ConsumeBool()) {
diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp
index 994b67605984..e3962737bb38 100644
--- a/src/test/streams_tests.cpp
+++ b/src/test/streams_tests.cpp
@@ -256,18 +256,18 @@ BOOST_AUTO_TEST_CASE(streams_serializedata_xor)
 BOOST_AUTO_TEST_CASE(streams_buffered_file)
 {
     fs::path streams_test_filename = m_args.GetDataDirBase() / "streams_test_tmp";
-    FILE* file = fsbridge::fopen(streams_test_filename, "w+b");
+    CAutoFile file{fsbridge::fopen(streams_test_filename, "w+b"), 333};
 
     // The value at each offset is the offset.
     for (uint8_t j = 0; j < 40; ++j) {
-        fwrite(&j, 1, 1, file);
+        file << j;
     }
-    rewind(file);
+    std::rewind(file.Get());
 
     // The buffer size (second arg) must be greater than the rewind
     // amount (third arg).
     try {
-        BufferedFile bfbad{file, 25, 25, 333};
+        BufferedFile bfbad{file, 25, 25};
         BOOST_CHECK(false);
     } catch (const std::exception& e) {
         BOOST_CHECK(strstr(e.what(),
@@ -275,7 +275,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file)
     }
 
     // The buffer is 25 bytes, allow rewinding 10 bytes.
-    BufferedFile bf{file, 25, 10, 333};
+    BufferedFile bf{file, 25, 10};
     BOOST_CHECK(!bf.eof());
 
     // This member has no functional effect.
@@ -382,7 +382,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file)
     BOOST_CHECK(bf.GetPos() <= 30U);
 
     // We can explicitly close the file, or the destructor will do it.
-    bf.fclose();
+    file.fclose();
 
     fs::remove(streams_test_filename);
 }
@@ -390,15 +390,15 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file)
 BOOST_AUTO_TEST_CASE(streams_buffered_file_skip)
 {
     fs::path streams_test_filename = m_args.GetDataDirBase() / "streams_test_tmp";
-    FILE* file = fsbridge::fopen(streams_test_filename, "w+b");
+    CAutoFile file{fsbridge::fopen(streams_test_filename, "w+b"), 333};
     // The value at each offset is the byte offset (e.g. byte 1 in the file has the value 0x01).
     for (uint8_t j = 0; j < 40; ++j) {
-        fwrite(&j, 1, 1, file);
+        file << j;
     }
-    rewind(file);
+    std::rewind(file.Get());
 
     // The buffer is 25 bytes, allow rewinding 10 bytes.
-    BufferedFile bf{file, 25, 10, 333};
+    BufferedFile bf{file, 25, 10};
 
     uint8_t i;
     // This is like bf >> (7-byte-variable), in that it will cause data
@@ -432,7 +432,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_skip)
     bf.SkipTo(13);
     BOOST_CHECK_EQUAL(bf.GetPos(), 13U);
 
-    bf.fclose();
+    file.fclose();
     fs::remove(streams_test_filename);
 }
 
@@ -443,16 +443,16 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_rand)
 
     fs::path streams_test_filename = m_args.GetDataDirBase() / "streams_test_tmp";
     for (int rep = 0; rep < 50; ++rep) {
-        FILE* file = fsbridge::fopen(streams_test_filename, "w+b");
+        CAutoFile file{fsbridge::fopen(streams_test_filename, "w+b"), 333};
         size_t fileSize = InsecureRandRange(256);
         for (uint8_t i = 0; i < fileSize; ++i) {
-            fwrite(&i, 1, 1, file);
+            file << i;
         }
-        rewind(file);
+        std::rewind(file.Get());
 
         size_t bufSize = InsecureRandRange(300) + 1;
         size_t rewindSize = InsecureRandRange(bufSize);
-        BufferedFile bf{file, bufSize, rewindSize, 333};
+        BufferedFile bf{file, bufSize, rewindSize};
         size_t currentPos = 0;
         size_t maxPos = 0;
         for (int step = 0; step < 100; ++step) {
diff --git a/src/validation.cpp b/src/validation.cpp
index 6a8c3d79497f..d4297821c109 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -5615,7 +5615,7 @@ bool Chainstate::LoadGenesisBlock()
 }
 
 void ChainstateManager::LoadExternalBlockFile(
-    FILE* fileIn,
+    CAutoFile& file_in,
     FlatFilePos* dbp,
     std::multimap<uint256, FlatFilePos>* blocks_with_unknown_parent)
 {
@@ -5629,8 +5629,7 @@ void ChainstateManager::LoadExternalBlockFile(
     int skipped_compressed{0};
     try {
         unsigned int nMaxBlockSize = MaxBlockSize();
-        // This takes over fileIn and calls fclose() on it in the BufferedFile destructor
-        BufferedFile blkdat{fileIn, 2 * nMaxBlockSize, nMaxBlockSize + 8, CLIENT_VERSION};
+        BufferedFile blkdat{file_in, 2 * nMaxBlockSize, nMaxBlockSize + 8};
         // nRewind indicates where to resume scanning in case something goes wrong,
         // such as a block fails to deserialize.
         uint64_t nRewind = blkdat.GetPos();
diff --git a/src/validation.h b/src/validation.h
index 80a4149e9166..9d1c99c4f627 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -644,7 +644,6 @@ class Chainstate
     bool ResizeCoinsCaches(size_t coinstip_size, size_t coinsdb_size)
         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
-
     /**
      * Update the on-disk chain state.
      * The caches and indexes are flushed depending on the mode we're called with
@@ -1161,14 +1160,14 @@ class ChainstateManager
      * -loadblock= option. There's no unknown-parent tracking, so the last two arguments are omitted.
      *
      *
-     * @param[in]     fileIn                        FILE handle to file containing blocks to read
+     * @param[in]     file_in                       File containing blocks to read
      * @param[in]     dbp                           (optional) Disk block position (only for reindex)
      * @param[in,out] blocks_with_unknown_parent    (optional) Map of disk positions for blocks with
      *                                              unknown parent, key is parent block hash
      *                                              (only used for reindex)
      * */
     void LoadExternalBlockFile(
-        FILE* fileIn,
+        CAutoFile& file_in,
         FlatFilePos* dbp = nullptr,
         std::multimap<uint256, FlatFilePos>* blocks_with_unknown_parent = nullptr);
 

From 57ba1ea0da38cb7e626616eb8fbb0ae3ab5d2357 Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Mon, 2 Oct 2023 11:00:49 +0200
Subject: [PATCH 10/12] Merge bitcoin/bitcoin#28545: test: Avoid test failure
 on Linux root without cap-add LINUX_IMMUTABLE

fa40b3ee22e78f58d7426dbc4343472ba40081e3 test: Avoid test failure on Linux root without cap-add LINUX_IMMUTABLE (MarcoFalke)

Pull request description:

  This turns a test failure on Linux when running the test as `root`, but without the `LINUX_IMMUTABLE` capability, into an early return, with a suggestion to turn on `LINUX_IMMUTABLE` next time (if possible).

ACKs for top commit:
  pinheadmz:
    utACK fa40b3ee22e78f58d7426dbc4343472ba40081e3
  jonatack:
    ACK fa40b3ee22e78f58d7426dbc4343472ba40081e3

Tree-SHA512: d986ff8aeae5f8267c21a23d5be16f7c5a4d4d3be045a6999d8b39c7b8672cfe915dedde762cc9965cdc4970940bffc4b0d1412833d8036d4425450eb6181f67

Co-authored-by: fanquake <fanquake@gmail.com>
---
 test/functional/feature_reindex_readonly.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/test/functional/feature_reindex_readonly.py b/test/functional/feature_reindex_readonly.py
index 9f1bb30023dd..26531f472bf8 100755
--- a/test/functional/feature_reindex_readonly.py
+++ b/test/functional/feature_reindex_readonly.py
@@ -6,6 +6,7 @@
 - Start a node, generate blocks, then restart with -reindex after setting blk files to read-only
 """
 
+import os
 import platform
 import stat
 import subprocess
@@ -45,6 +46,11 @@ def reindex_readonly(self):
                     self.log.warning(f"stdout: {e.stdout}")
                 if e.stderr:
                     self.log.warning(f"stderr: {e.stderr}")
+                if os.getuid() == 0:
+                    self.log.warning("Return early on Linux under root, because chattr failed.")
+                    self.log.warning("This should only happen due to missing capabilities in a container.")
+                    self.log.warning("Make sure to --cap-add LINUX_IMMUTABLE if you want to run this test.")
+                    return
 
         self.log.debug("Attempt to restart and reindex the node with the unwritable block file")
         with self.nodes[0].assert_debug_log(expected_msgs=['FlushStateToDisk', 'failed to open file'], unexpected_msgs=[]):

From bd31e53f1b19d1bacf08a745154eab132d025a88 Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Thu, 5 Oct 2023 19:02:02 -0400
Subject: [PATCH 11/12] Merge bitcoin/bitcoin#27609: rpc: allow submitpackage
 to be called outside of regtest
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

5b878be742dbfcd232d949d2df1fff4743aec3d8 [doc] add release note for submitpackage (glozow)
7a9bb2a2a59ba49f80519c8435229abec2432486 [rpc] allow submitpackage to be called outside of regtest (glozow)
5b9087a9a7da2602485e85e0b163dc3cbd2daf31 [rpc] require package to be a tree in submitpackage (glozow)
e32ba1599c599e75b1da3393f71f633de860505f [txpackages] IsChildWithParentsTree() (glozow)
b4f28cc345ef9c5261c4a8d743654a44784c7802 [doc] parent pay for child in aggregate CheckFeeRate (glozow)

Pull request description:

  Permit (restricted topology) submitpackage RPC outside of regtest. Suggested in https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1510851570

  This RPC should be safe but still experimental - interface may change, not all features (e.g. package RBF) are implemented, etc. If a miner wants to expose this to people, they can effectively use "package relay" before the p2p changes are implemented. However, please note **this is not package relay**; transactions submitted this way will not relay to other nodes if the feerates are below their mempool min fee. Users should put this behind some kind of rate limit or permissions.

ACKs for top commit:
  instagibbs:
    ACK 5b878be742dbfcd232d949d2df1fff4743aec3d8
  achow101:
    ACK 5b878be742dbfcd232d949d2df1fff4743aec3d8
  dergoegge:
    Code review ACK 5b878be742dbfcd232d949d2df1fff4743aec3d8
  ajtowns:
    ACK 5b878be742dbfcd232d949d2df1fff4743aec3d8
  ariard:
    Code Review ACK  5b878be742. Though didn’t manually test the PR.

Tree-SHA512: 610365c0b2ffcccd55dedd1151879c82de1027e3319712bcb11d54f2467afaae4d05dca5f4b25f03354c80845fef538d3938b958174dda8b14c10670537a6524

Co-authored-by: Andrew Chow <github@achow101.com>
---
 doc/release-notes-27609.md      | 13 +++++++++++++
 src/policy/packages.cpp         | 15 +++++++++++++++
 src/policy/packages.h           |  4 ++++
 src/rpc/mempool.cpp             | 14 +++++++-------
 src/test/txpackage_tests.cpp    |  3 +++
 src/validation.cpp              |  6 ++++++
 test/functional/rpc_packages.py |  2 +-
 7 files changed, 49 insertions(+), 8 deletions(-)
 create mode 100644 doc/release-notes-27609.md

diff --git a/doc/release-notes-27609.md b/doc/release-notes-27609.md
new file mode 100644
index 000000000000..a69bcaffaaa5
--- /dev/null
+++ b/doc/release-notes-27609.md
@@ -0,0 +1,13 @@
+- A new RPC, `submitpackage`, has been added. It can be used to submit a list of raw hex
+  transactions to the mempool to be evaluated as a package using consensus and mempool policy rules.
+These policies include package CPFP, allowing a child with high fees to bump a parent below the
+mempool minimum feerate (but not minimum relay feerate).
+
+    - Warning: successful submission does not mean the transactions will propagate throughout the
+      network, as package relay is not supported.
+
+    - Not all features are available. The package is limited to a child with all of its
+      unconfirmed parents, and no parent may spend the output of another parent. Refer to
+      doc/policy/packages.md for more details on package policies and limitations.
+
+    - This RPC is experimental. Its interface may change.
diff --git a/src/policy/packages.cpp b/src/policy/packages.cpp
index 6e70a94088a2..a656a4b5f06d 100644
--- a/src/policy/packages.cpp
+++ b/src/policy/packages.cpp
@@ -81,3 +81,18 @@ bool IsChildWithParents(const Package& package)
     return std::all_of(package.cbegin(), package.cend() - 1,
                        [&input_txids](const auto& ptx) { return input_txids.count(ptx->GetHash()) > 0; });
 }
+
+bool IsChildWithParentsTree(const Package& package)
+{
+    if (!IsChildWithParents(package)) return false;
+    std::unordered_set<uint256, SaltedTxidHasher> parent_txids;
+    std::transform(package.cbegin(), package.cend() - 1, std::inserter(parent_txids, parent_txids.end()),
+                   [](const auto& ptx) { return ptx->GetHash(); });
+    // Each parent must not have an input who is one of the other parents.
+    return std::all_of(package.cbegin(), package.cend() - 1, [&](const auto& ptx) {
+        for (const auto& input : ptx->vin) {
+            if (parent_txids.count(input.prevout.hash) > 0) return false;
+        }
+        return true;
+    });
+}
diff --git a/src/policy/packages.h b/src/policy/packages.h
index e84e0c64c8a9..e7160a14cf3b 100644
--- a/src/policy/packages.h
+++ b/src/policy/packages.h
@@ -59,4 +59,8 @@ bool CheckPackage(const Package& txns, PackageValidationState& state);
  */
 bool IsChildWithParents(const Package& package);
 
+/** Context-free check that a package IsChildWithParents() and none of the parents depend on each
+ * other (the package is a "tree").
+ */
+bool IsChildWithParentsTree(const Package& package);
 #endif // BITCOIN_POLICY_PACKAGES_H
diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
index ecc44e9e796a..dfe293434a0e 100644
--- a/src/rpc/mempool.cpp
+++ b/src/rpc/mempool.cpp
@@ -830,11 +830,11 @@ static RPCHelpMan savemempool()
 static RPCHelpMan submitpackage()
 {
     return RPCHelpMan{"submitpackage",
-        "Submit a package of raw transactions (serialized, hex-encoded) to local node (-regtest only).\n"
+        "Submit a package of raw transactions (serialized, hex-encoded) to local node.\n"
+        "The package must consist of a child with its parents, and none of the parents may depend on one another.\n"
         "The package will be validated according to consensus and mempool policy rules. If all transactions pass, they will be accepted to mempool.\n"
         "This RPC is experimental and the interface may be unstable. Refer to doc/policy/packages.md for documentation on package policies.\n"
-        "Warning: until package relay is in use, successful submission does not mean the transaction will propagate to other nodes on the network.\n"
-        "Currently, each transaction is broadcasted individually after submission, which means they must meet other nodes' feerate requirements alone.\n"
+        "Warning: successful submission does not mean the transactions will propagate throughout the network.\n"
         ,
         {
             {"package", RPCArg::Type::ARR, RPCArg::Optional::NO, "An array of raw transactions.",
@@ -868,9 +868,6 @@ static RPCHelpMan submitpackage()
         },
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
         {
-            if (Params().GetChainType() != ChainType::REGTEST) {
-                throw std::runtime_error("submitpackage is for regression testing (-regtest mode) only");
-            }
             const UniValue raw_transactions = request.params[0].get_array();
             if (raw_transactions.size() < 1 || raw_transactions.size() > MAX_PACKAGE_COUNT) {
                 throw JSONRPCError(RPC_INVALID_PARAMETER,
@@ -887,6 +884,9 @@ static RPCHelpMan submitpackage()
                 }
                 txns.emplace_back(MakeTransactionRef(std::move(mtx)));
             }
+            if (!IsChildWithParentsTree(txns)) {
+                throw JSONRPCTransactionError(TransactionError::INVALID_PACKAGE, "package topology disallowed. not child-with-parents or parents depend on each other.");
+            }
 
             NodeContext& node = EnsureAnyNodeContext(request.context);
             CTxMemPool& mempool = EnsureMemPool(node);
@@ -977,7 +977,7 @@ void RegisterMempoolRPCCommands(CRPCTable& t)
         {"blockchain", &getrawmempool},
         {"blockchain", &importmempool},
         {"blockchain", &savemempool},
-        {"hidden", &submitpackage},
+        {"rawtransactions", &submitpackage},
     };
     for (const auto& c : commands) {
         t.appendCommand(c.name, &c);
diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp
index 3a0af798c8d0..c1e6f92e4513 100644
--- a/src/test/txpackage_tests.cpp
+++ b/src/test/txpackage_tests.cpp
@@ -156,6 +156,7 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100NoDIP0001Setup)
         BOOST_CHECK_EQUAL(state.GetResult(), PackageValidationResult::PCKG_POLICY);
         BOOST_CHECK_EQUAL(state.GetRejectReason(), "package-not-sorted");
         BOOST_CHECK(IsChildWithParents({tx_parent, tx_child}));
+        BOOST_CHECK(IsChildWithParentsTree({tx_parent, tx_child}));
     }
 
     // 24 Parents and 1 Child
@@ -181,6 +182,7 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100NoDIP0001Setup)
         PackageValidationState state;
         BOOST_CHECK(CheckPackage(package, state));
         BOOST_CHECK(IsChildWithParents(package));
+        BOOST_CHECK(IsChildWithParentsTree(package));
 
         package.erase(package.begin());
         BOOST_CHECK(IsChildWithParents(package));
@@ -213,6 +215,7 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100NoDIP0001Setup)
         BOOST_CHECK(IsChildWithParents({tx_parent, tx_parent_also_child}));
         BOOST_CHECK(IsChildWithParents({tx_parent, tx_child}));
         BOOST_CHECK(IsChildWithParents({tx_parent, tx_parent_also_child, tx_child}));
+        BOOST_CHECK(!IsChildWithParentsTree({tx_parent, tx_parent_also_child, tx_child}));
         // IsChildWithParents does not detect unsorted parents.
         BOOST_CHECK(IsChildWithParents({tx_parent_also_child, tx_parent, tx_child}));
         BOOST_CHECK(CheckPackage({tx_parent, tx_parent_also_child, tx_child}, state));
diff --git a/src/validation.cpp b/src/validation.cpp
index d4297821c109..e7361c100686 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1509,6 +1509,12 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
     // Transactions must meet two minimum feerates: the mempool minimum fee and min relay fee.
     // For transactions consisting of exactly one child and its parents, it suffices to use the
     // package feerate (total modified fees / total size) to check this requirement.
+    // Note that this is an aggregate feerate; this function has not checked that there are transactions
+    // too low feerate to pay for themselves, or that the child transactions are higher feerate than
+    // their parents. Using aggregate feerate may allow "parents pay for child" behavior and permit
+    // a child that is below mempool minimum feerate. To avoid these behaviors, callers of
+    // AcceptMultipleTransactions need to restrict txns topology (e.g. to ancestor sets) and check
+    // the feerates of individuals and subsets.
     const auto m_total_vsize = std::accumulate(workspaces.cbegin(), workspaces.cend(), int64_t{0},
         [](int64_t sum, auto& ws) { return sum + ws.m_vsize; });
     const auto m_total_modified_fees = std::accumulate(workspaces.cbegin(), workspaces.cend(), CAmount{0},
diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py
index ff4fa01c931b..5e3e33ccd098 100755
--- a/test/functional/rpc_packages.py
+++ b/test/functional/rpc_packages.py
@@ -349,7 +349,7 @@ def test_submitpackage(self):
         self.log.info("Submitpackage only allows packages of 1 child with its parents")
         # Chain of 3 transactions has too many generations
         chain_hex = [t["hex"] for t in self.wallet.create_self_transfer_chain(chain_length=25)]
-        assert_raises_rpc_error(-25, "not-child-with-parents", node.submitpackage, chain_hex)
+        assert_raises_rpc_error(-25, "package topology disallowed", node.submitpackage, chain_hex)
 
 
 if __name__ == "__main__":

From 93bee73cb1b741f91ed08775df354da6e697ac6a Mon Sep 17 00:00:00 2001
From: Konstantin Akimov <github@knstqq.com>
Date: Wed, 11 Oct 2023 20:06:26 +0200
Subject: [PATCH 12/12] Merge bitcoin/bitcoin#28392: test: Use pathlib over os
 path
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

bfa0bd632a7ce5d04005e20cba79abb32aec8da8  test: Use pathlib over os.path #28362 (ns-xvrn)

Pull request description:

  In reference to issue  #28362  refactoring of functional tests to use pathlib over os.path to reduce verbosity and increase the intuitiveness of managing file access.

ACKs for top commit:
  maflcko:
    re-ACK  bfa0bd632a7ce5d04005e20cba79abb32aec8da8  🐨
  willcl-ark:
    ACK bfa0bd632a7ce5d04005e20cba79abb32aec8da8

Tree-SHA512: fb0833c4039d09758796514e47567a93ac831cb0776ff1a7ed8299792ad132e83282ef80bea098a88ae1551d906f2a56093d3e8de240412f9080735d00d496d9

Co-authored-by: fanquake <fanquake@gmail.com>
---
 test/functional/feature_blocksdir.py          | 16 ++--
 test/functional/feature_config_args.py        | 65 ++++++++--------
 test/functional/feature_filelock.py           |  2 +-
 test/functional/feature_loadblock.py          | 14 ++--
 test/functional/feature_settings.py           |  7 +-
 test/functional/interface_bitcoin_cli.py      |  2 +-
 test/functional/rpc_bind.py                   |  2 +-
 test/functional/rpc_dumptxoutset.py           |  3 +-
 .../test_framework/test_framework.py          |  8 +-
 test/functional/test_framework/test_node.py   | 36 ++++-----
 test/functional/test_framework/util.py        |  2 +-
 test/functional/tool_wallet.py                | 24 +++---
 test/functional/wallet_backup.py              | 74 ++++++++-----------
 test/functional/wallet_blank.py               |  3 +-
 test/functional/wallet_crosschain.py          | 10 +--
 test/functional/wallet_descriptor.py          |  3 +-
 test/functional/wallet_dump.py                | 12 +--
 test/functional/wallet_fast_rescan.py         |  3 +-
 test/functional/wallet_hd.py                  | 33 ++++-----
 test/functional/wallet_keypool_topup.py       |  5 +-
 test/functional/wallet_pruning.py             | 11 ++-
 test/functional/wallet_reorgsrestore.py       |  5 +-
 test/functional/wallet_upgradetohd.py         |  4 +-
 23 files changed, 159 insertions(+), 185 deletions(-)

diff --git a/test/functional/feature_blocksdir.py b/test/functional/feature_blocksdir.py
index ad066b566bcb..7285bbd7886c 100755
--- a/test/functional/feature_blocksdir.py
+++ b/test/functional/feature_blocksdir.py
@@ -5,8 +5,8 @@
 """Test the blocksdir option.
 """
 
-import os
 import shutil
+from pathlib import Path
 
 from test_framework.test_framework import BitcoinTestFramework, initialize_datadir
 
@@ -18,20 +18,20 @@ def set_test_params(self):
 
     def run_test(self):
         self.stop_node(0)
-        assert os.path.isdir(os.path.join(self.nodes[0].chain_path, "blocks"))
-        assert not os.path.isdir(os.path.join(self.nodes[0].datadir, "blocks"))
-        shutil.rmtree(self.nodes[0].datadir)
+        assert self.nodes[0].blocks_path.is_dir()
+        assert not (self.nodes[0].datadir_path / "blocks").is_dir()
+        shutil.rmtree(self.nodes[0].datadir_path)
         initialize_datadir(self.options.tmpdir, 0, self.chain)
         self.log.info("Starting with nonexistent blocksdir ...")
-        blocksdir_path = os.path.join(self.options.tmpdir, 'blocksdir')
+        blocksdir_path = Path(self.options.tmpdir) / 'blocksdir'
         self.nodes[0].assert_start_raises_init_error([f"-blocksdir={blocksdir_path}"], f'Error: Specified blocks directory "{blocksdir_path}" does not exist.')
-        os.mkdir(blocksdir_path)
+        blocksdir_path.mkdir()
         self.log.info("Starting with existing blocksdir ...")
         self.start_node(0, [f"-blocksdir={blocksdir_path}"])
         self.log.info("mining blocks..")
         self.generatetoaddress(self.nodes[0], 10, self.nodes[0].get_deterministic_priv_key().address)
-        assert os.path.isfile(os.path.join(blocksdir_path, self.chain, "blocks", "blk00000.dat"))
-        assert os.path.isdir(os.path.join(self.nodes[0].chain_path, "blocks", "index"))
+        assert (blocksdir_path / self.chain / "blocks" / "blk00000.dat").is_file()
+        assert (self.nodes[0].blocks_path / "index").is_dir()
 
 
 if __name__ == '__main__':
diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py
index 8cdb34d9596b..f200b2be9806 100755
--- a/test/functional/feature_config_args.py
+++ b/test/functional/feature_config_args.py
@@ -5,7 +5,7 @@
 """Test various command line arguments and configuration file parameters."""
 
 import os
-import pathlib
+from pathlib import Path
 import re
 import shutil
 import sys
@@ -40,8 +40,8 @@ def test_config_file_parser(self):
             extra_args=[f'-conf={bad_conf_file_path}'],
             expected_msg=conf_in_config_file_err,
         )
-        inc_conf_file_path = os.path.join(self.nodes[0].datadir, 'include.conf')
-        with open(os.path.join(self.nodes[0].datadir, 'dash.conf'), 'a', encoding='utf-8') as conf:
+        inc_conf_file_path = self.nodes[0].datadir_path / 'include.conf'
+        with open(self.nodes[0].datadir_path / 'dash.conf', 'a', encoding='utf-8') as conf:
             conf.write(f'includeconf={inc_conf_file_path}\n')
         with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
             conf.write('conf=some.conf\n')
@@ -98,8 +98,8 @@ def test_config_file_parser(self):
             conf.write('server=1\nrpcuser=someuser\n[main]\nrpcpassword=some#pass')
         self.nodes[0].assert_start_raises_init_error(expected_msg='Error: Error reading configuration file: parse error on line 4, using # in rpcpassword can be ambiguous and should be avoided')
 
-        inc_conf_file2_path = os.path.join(self.nodes[0].datadir, 'include2.conf')
-        with open(os.path.join(self.nodes[0].datadir, 'dash.conf'), 'a', encoding='utf-8') as conf:
+        inc_conf_file2_path = self.nodes[0].datadir_path / 'include2.conf'
+        with open(self.nodes[0].datadir_path / 'dash.conf', 'a', encoding='utf-8') as conf:
             conf.write(f'includeconf={inc_conf_file2_path}\n')
 
         with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
@@ -124,15 +124,15 @@ def test_config_file_log(self):
 
         # Create a temporary directory that will be treated as the default data
         # directory by bitcoind.
-        env, default_datadir = util.get_temp_default_datadir(pathlib.Path(self.options.tmpdir, "test_config_file_log"))
+        env, default_datadir = util.get_temp_default_datadir(Path(self.options.tmpdir, "test_config_file_log"))
         default_datadir.mkdir(parents=True)
 
         # Write a dash.conf file in the default data directory containing a
         # datadir= line pointing at the node datadir.
         node = self.nodes[0]
-        conf_text = pathlib.Path(node.bitcoinconf).read_text()
+        conf_text = node.bitcoinconf.read_text()
         conf_path = default_datadir / "dash.conf"
-        conf_path.write_text(f"datadir={node.datadir}\n{conf_text}")
+        conf_path.write_text(f"datadir={node.datadir_path}\n{conf_text}")
 
         # Drop the node -datadir= argument during this test, because if it is
         # specified it would take precedence over the datadir setting in the
@@ -219,7 +219,8 @@ def test_networkactive(self):
 
     def test_seed_peers(self):
         self.log.info('Test seed peers')
-        default_data_dir = self.nodes[0].datadir
+        default_data_dir = self.nodes[0].datadir_path
+        peer_dat = default_data_dir / 'peers.dat'
         # Only regtest has no fixed seeds. To avoid connections to random
         # nodes, regtest is the only network where it is safe to enable
         # -fixedseeds in tests
@@ -230,7 +231,7 @@ def test_seed_peers(self):
         # We expect the node will use DNS Seeds, but Regtest mode does not have
         # any valid DNS seeds. So after 60 seconds, the node should fallback to
         # fixed seeds
-        assert not os.path.exists(os.path.join(default_data_dir, "peers.dat"))
+        assert not peer_dat.exists()
         start = int(time.time())
         with self.nodes[0].assert_debug_log(
                 expected_msgs=[
@@ -249,7 +250,7 @@ def test_seed_peers(self):
 
         # No peers.dat exists and -dnsseed=0
         # We expect the node will fallback immediately to fixed seeds
-        assert not os.path.exists(os.path.join(default_data_dir, "peers.dat"))
+        assert not peer_dat.exists()
         with self.nodes[0].assert_debug_log(expected_msgs=[
                 "Loaded 0 addresses from peers.dat",
                 "DNS seeding disabled",
@@ -261,7 +262,7 @@ def test_seed_peers(self):
 
         # No peers.dat exists and dns seeds are disabled.
         # We expect the node will not add fixed seeds when explicitly disabled.
-        assert not os.path.exists(os.path.join(default_data_dir, "peers.dat"))
+        assert not peer_dat.exists()
         with self.nodes[0].assert_debug_log(expected_msgs=[
                 "Loaded 0 addresses from peers.dat",
                 "DNS seeding disabled",
@@ -272,7 +273,7 @@ def test_seed_peers(self):
 
         # No peers.dat exists and -dnsseed=0, but a -addnode is provided
         # We expect the node will allow 60 seconds prior to using fixed seeds
-        assert not os.path.exists(os.path.join(default_data_dir, "peers.dat"))
+        assert not peer_dat.exists()
         start = int(time.time())
         with self.nodes[0].assert_debug_log(
                 expected_msgs=[
@@ -324,16 +325,16 @@ def test_ignored_conf(self):
                       'because a conflicting -conf file argument is passed.')
         node = self.nodes[0]
         with tempfile.NamedTemporaryFile(dir=self.options.tmpdir, mode="wt", delete=False) as temp_conf:
-            temp_conf.write(f"datadir={node.datadir}\n")
+            temp_conf.write(f"datadir={node.datadir_path}\n")
         node.assert_start_raises_init_error([f"-conf={temp_conf.name}"], re.escape(
-            f'Error: Data directory "{node.datadir}" contains a "dash.conf" file which is ignored, because a '
+            f'Error: Data directory "{node.datadir_path}" contains a "dash.conf" file which is ignored, because a '
             f'different configuration file "{temp_conf.name}" from command line argument "-conf={temp_conf.name}" '
             f'is being used instead.') + r"[\s\S]*", match=ErrorMatch.FULL_REGEX)
 
         # Test that passing a redundant -conf command line argument pointing to
         # the same dash.conf that would be loaded anyway does not trigger an
         # error.
-        self.start_node(0, [f'-conf={node.datadir}/dash.conf'])
+        self.start_node(0, [f'-conf={node.datadir_path}/dash.conf'])
         self.stop_node(0)
 
     def test_missing_default_datadir(self):
@@ -346,7 +347,7 @@ def test_missing_default_datadir(self):
         node_args = node.args
         node.args = [arg for arg in node.args if not arg.startswith("-datadir=")]
         for name, extra_args in [("version", ["-version"]), ("help", ["-help"]), ("start", ["-regtest", "-listen=0", "-server=0"])]:
-            env, default_datadir = util.get_temp_default_datadir(pathlib.Path(self.options.tmpdir, "no_datadir", name))
+            env, default_datadir = util.get_temp_default_datadir(Path(self.options.tmpdir, "no_datadir", name))
             default_datadir.parent.mkdir(parents=True)
             node.start(extra_args, env=env)
             if name == "start":
@@ -368,7 +369,7 @@ def test_ignored_default_conf(self):
 
         # Create a temporary directory that will be treated as the default data
         # directory by bitcoind.
-        env, default_datadir = util.get_temp_default_datadir(pathlib.Path(self.options.tmpdir, "home"))
+        env, default_datadir = util.get_temp_default_datadir(Path(self.options.tmpdir, "home"))
         default_datadir.mkdir(parents=True)
 
         # Write a dash.conf file in the default data directory containing a
@@ -376,7 +377,7 @@ def test_ignored_default_conf(self):
         # startup error because the node datadir contains a different
         # dash.conf that would be ignored.
         node = self.nodes[0]
-        (default_datadir / "dash.conf").write_text(f"datadir={node.datadir}\n")
+        (default_datadir / "dash.conf").write_text(f"datadir={node.datadir_path}\n")
 
         # Drop the node -datadir= argument during this test, because if it is
         # specified it would take precedence over the datadir setting in the
@@ -384,7 +385,7 @@ def test_ignored_default_conf(self):
         node_args = node.args
         node.args = [arg for arg in node.args if not arg.startswith("-datadir=")]
         node.assert_start_raises_init_error([], re.escape(
-            f'Error: Data directory "{node.datadir}" contains a "dash.conf" file which is ignored, because a '
+            f'Error: Data directory "{node.datadir_path}" contains a "dash.conf" file which is ignored, because a '
             f'different configuration file "{default_datadir}/dash.conf" from data directory "{default_datadir}" '
             f'is being used instead.') + r"[\s\S]*", env=env, match=ErrorMatch.FULL_REGEX)
         node.args = node_args
@@ -422,16 +423,16 @@ def run_test(self):
         # Remove the -datadir argument so it doesn't override the config file
         self.nodes[0].args = [arg for arg in self.nodes[0].args if not arg.startswith("-datadir")]
 
-        default_data_dir = self.nodes[0].datadir
-        new_data_dir = os.path.join(default_data_dir, 'newdatadir')
-        new_data_dir_2 = os.path.join(default_data_dir, 'newdatadir2')
+        default_data_dir = self.nodes[0].datadir_path
+        new_data_dir = default_data_dir / 'newdatadir'
+        new_data_dir_2 = default_data_dir / 'newdatadir2'
 
         # Check that using -datadir argument on non-existent directory fails
-        self.nodes[0].datadir = new_data_dir
+        self.nodes[0].datadir_path = new_data_dir
         self.nodes[0].assert_start_raises_init_error([f'-datadir={new_data_dir}'], f'Error: Specified data directory "{new_data_dir}" does not exist.')
 
         # Check that using non-existent datadir in conf file fails
-        conf_file = os.path.join(default_data_dir, "dash.conf")
+        conf_file = default_data_dir / "dash.conf"
 
         # datadir needs to be set before [chain] section
         with open(conf_file, encoding='utf8') as f:
@@ -443,20 +444,20 @@ def run_test(self):
         self.nodes[0].assert_start_raises_init_error([f'-conf={conf_file}'], f'Error: Error reading configuration file: specified data directory "{new_data_dir}" does not exist.')
 
         # Check that an explicitly specified config file that cannot be opened fails
-        none_existent_conf_file = os.path.join(default_data_dir, "none_existent_dash.conf")
-        self.nodes[0].assert_start_raises_init_error(['-conf=' + none_existent_conf_file], 'Error: Error reading configuration file: specified config file "' + none_existent_conf_file + '" could not be opened.')
+        none_existent_conf_file = default_data_dir / "none_existent_dash.conf"
+        self.nodes[0].assert_start_raises_init_error(['-conf=' + f'{none_existent_conf_file}'], 'Error: Error reading configuration file: specified config file "' + f'{none_existent_conf_file}' + '" could not be opened.')
 
         # Create the directory and ensure the config file now works
-        os.mkdir(new_data_dir)
+        new_data_dir.mkdir()
         self.start_node(0, [f'-conf={conf_file}'])
         self.stop_node(0)
-        assert os.path.exists(os.path.join(new_data_dir, self.chain, 'blocks'))
+        assert (new_data_dir / self.chain / 'blocks').exists()
 
         # Ensure command line argument overrides datadir in conf
-        os.mkdir(new_data_dir_2)
-        self.nodes[0].datadir = new_data_dir_2
+        new_data_dir_2.mkdir()
+        self.nodes[0].datadir_path = new_data_dir_2
         self.start_node(0, [f'-datadir={new_data_dir_2}', f'-conf={conf_file}'])
-        assert os.path.exists(os.path.join(new_data_dir_2, self.chain, 'blocks'))
+        assert (new_data_dir_2 / self.chain / 'blocks').exists()
 
 
 if __name__ == '__main__':
diff --git a/test/functional/feature_filelock.py b/test/functional/feature_filelock.py
index ac343315b589..023b7e64f381 100755
--- a/test/functional/feature_filelock.py
+++ b/test/functional/feature_filelock.py
@@ -30,7 +30,7 @@ def run_test(self):
 
         self.log.info("Check that we can't start a second dashd instance using the same datadir")
         expected_msg = f"Error: Cannot obtain a lock on data directory {datadir}. {self.config['environment']['PACKAGE_NAME']} is probably already running."
-        self.nodes[1].assert_start_raises_init_error(extra_args=[f'-datadir={self.nodes[0].datadir}', '-noserver'], expected_msg=expected_msg)
+        self.nodes[1].assert_start_raises_init_error(extra_args=[f'-datadir={self.nodes[0].datadir_path}', '-noserver'], expected_msg=expected_msg)
 
         self.log.info("Check that cookie and PID file are not deleted when attempting to start a second dashd using the same datadir")
         cookie_file = Path(datadir) / ".cookie"
diff --git a/test/functional/feature_loadblock.py b/test/functional/feature_loadblock.py
index 666c289f216d..7a9745531d7f 100755
--- a/test/functional/feature_loadblock.py
+++ b/test/functional/feature_loadblock.py
@@ -10,7 +10,7 @@
 in contrib/linearize.
 """
 
-import os
+from pathlib import Path
 import subprocess
 import sys
 import tempfile
@@ -32,10 +32,10 @@ def run_test(self):
         self.generate(self.nodes[0], COINBASE_MATURITY, sync_fun=self.no_op)
 
         # Parsing the url of our node to get settings for config file
-        data_dir = self.nodes[0].datadir
+        data_dir = self.nodes[0].datadir_path
         node_url = urllib.parse.urlparse(self.nodes[0].url)
-        cfg_file = os.path.join(data_dir, "linearize.cfg")
-        bootstrap_file = os.path.join(self.options.tmpdir, "bootstrap.dat")
+        cfg_file = data_dir / "linearize.cfg"
+        bootstrap_file = Path(self.options.tmpdir) / "bootstrap.dat"
         genesis_block = self.nodes[0].getblockhash(0)
         blocks_dir = self.nodes[0].blocks_path
         hash_list = tempfile.NamedTemporaryFile(dir=data_dir,
@@ -58,16 +58,16 @@ def run_test(self):
             cfg.write(f"hashlist={hash_list.name}\n")
 
         base_dir = self.config["environment"]["SRCDIR"]
-        linearize_dir = os.path.join(base_dir, "contrib", "linearize")
+        linearize_dir = Path(base_dir) / "contrib" / "linearize"
 
         self.log.info("Run linearization of block hashes")
-        linearize_hashes_file = os.path.join(linearize_dir, "linearize-hashes.py")
+        linearize_hashes_file = linearize_dir / "linearize-hashes.py"
         subprocess.run([sys.executable, linearize_hashes_file, cfg_file],
                        stdout=hash_list,
                        check=True)
 
         self.log.info("Run linearization of block data")
-        linearize_data_file = os.path.join(linearize_dir, "linearize-data.py")
+        linearize_data_file = linearize_dir / "linearize-data.py"
         subprocess.run([sys.executable, linearize_data_file, cfg_file],
                        check=True)
 
diff --git a/test/functional/feature_settings.py b/test/functional/feature_settings.py
index 6ffa0fdc3906..c53ae2ce3f66 100755
--- a/test/functional/feature_settings.py
+++ b/test/functional/feature_settings.py
@@ -6,7 +6,6 @@
 
 import json
 
-from pathlib import Path
 
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.test_node import ErrorMatch
@@ -21,8 +20,8 @@ def set_test_params(self):
 
     def run_test(self):
         node, = self.nodes
-        settings = Path(node.chain_path, "settings.json")
-        conf = Path(node.datadir, "dash.conf")
+        settings = node.chain_path / "settings.json"
+        conf = node.datadir_path / "dash.conf"
 
         # Assert default settings file was created
         self.stop_node(0)
@@ -80,7 +79,7 @@ def run_test(self):
             self.stop_node(0)
 
         # Test alternate settings path
-        altsettings = Path(node.datadir, "altsettings.json")
+        altsettings = node.datadir_path / "altsettings.json"
         with altsettings.open("w") as fp:
             fp.write('{"key": "value"}')
         with node.assert_debug_log(expected_msgs=['Setting file arg: key = "value"']):
diff --git a/test/functional/interface_bitcoin_cli.py b/test/functional/interface_bitcoin_cli.py
index c1afe72b50b7..ac6482725aeb 100755
--- a/test/functional/interface_bitcoin_cli.py
+++ b/test/functional/interface_bitcoin_cli.py
@@ -94,7 +94,7 @@ def run_test(self):
         assert_equal(self.nodes[0].cli("-named", "echo", "arg0=0", "arg1=1", "arg2=2", "arg1=3").send_cli(), ['0', '3', '2'])
         assert_raises_rpc_error(-8, "Parameter args specified multiple times", self.nodes[0].cli("-named", "echo", "args=[0,1,2,3]", "4", "5", "6", ).send_cli)
 
-        user, password = get_auth_cookie(self.nodes[0].datadir, self.chain)
+        user, password = get_auth_cookie(self.nodes[0].datadir_path, self.chain)
 
         self.log.info("Test -stdinrpcpass option")
         assert_equal(BLOCKS, self.nodes[0].cli(f'-rpcuser={user}', '-stdinrpcpass', input=password).getblockcount())
diff --git a/test/functional/rpc_bind.py b/test/functional/rpc_bind.py
index 9a8990161568..07cb41c19a1f 100755
--- a/test/functional/rpc_bind.py
+++ b/test/functional/rpc_bind.py
@@ -58,7 +58,7 @@ def run_allowip_test(self, allow_ips, rpchost, rpcport):
         self.nodes[0].rpchost = None
         self.start_nodes([node_args])
         # connect to node through non-loopback interface
-        node = get_rpc_proxy(rpc_url(self.nodes[0].datadir, 0, self.chain, "%s:%d" % (rpchost, rpcport)), 0, coveragedir=self.options.coveragedir)
+        node = get_rpc_proxy(rpc_url(self.nodes[0].datadir_path, 0, self.chain, "%s:%d" % (rpchost, rpcport)), 0, coveragedir=self.options.coveragedir)
         node.getnetworkinfo()
         self.stop_nodes()
 
diff --git a/test/functional/rpc_dumptxoutset.py b/test/functional/rpc_dumptxoutset.py
index cdaa3eedf415..5008a8cd0621 100755
--- a/test/functional/rpc_dumptxoutset.py
+++ b/test/functional/rpc_dumptxoutset.py
@@ -4,7 +4,6 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test the generation of UTXO snapshots using `dumptxoutset`.
 """
-from pathlib import Path
 
 from test_framework.blocktools import COINBASE_MATURITY
 from test_framework.test_framework import BitcoinTestFramework
@@ -29,7 +28,7 @@ def run_test(self):
 
         FILENAME = 'txoutset.dat'
         out = node.dumptxoutset(FILENAME)
-        expected_path = Path(node.datadir) / self.chain / FILENAME
+        expected_path = node.datadir_path / self.chain / FILENAME
 
         assert expected_path.is_file()
 
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index bdd60662b2c8..a164380c6a2f 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -1200,6 +1200,10 @@ def is_bdb_compiled(self):
         """Checks whether the wallet module was compiled with BDB support."""
         return self.config["components"].getboolean("USE_BDB")
 
+    def has_blockfile(self, node, filenum: str):
+        blocksdir = node.datadir_path / self.chain / 'blocks'
+        return (blocksdir / f"blk{filenum}.dat").is_file()
+
 MASTERNODE_COLLATERAL = 1000
 EVONODE_COLLATERAL = 4000
 
@@ -1548,10 +1552,10 @@ def add_nodes(self, num_nodes: int, extra_args=None, *, rpchost=None, binary=Non
         old_num_nodes = len(self.nodes)
         super().add_nodes(num_nodes, extra_args, rpchost=rpchost, binary=binary, binary_cli=binary_cli, versions=versions)
         for i in range(old_num_nodes, old_num_nodes + num_nodes):
-            self.append_dip3_config(self.nodes[i].datadir)
+            self.append_dip3_config(self.nodes[i].datadir_path)
         if old_num_nodes == 0:
             # controller node is the only node that has an extra option allowing it to submit sporks
-            append_config(self.nodes[0].datadir, ["sporkkey=cP4EKFyJsHT39LDqgdcB43Y3YXjNyjb5Fuas1GQSeAtjnZWmZEQK"])
+            append_config(self.nodes[0].datadir_path, ["sporkkey=cP4EKFyJsHT39LDqgdcB43Y3YXjNyjb5Fuas1GQSeAtjnZWmZEQK"])
 
     def connect_nodes(self, a, b, *, peer_advertises_v2=None):
         for mn2 in self.mninfo: # type: MasternodeInfo
diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
index c8e20566b882..deb218a7774c 100755
--- a/test/functional/test_framework/test_node.py
+++ b/test/functional/test_framework/test_node.py
@@ -70,7 +70,7 @@ class TestNode():
     To make things easier for the test writer, any unrecognised messages will
     be dispatched to the RPC connection."""
 
-    def __init__(self, i, datadir, extra_args_from_options, *, chain, rpchost, timewait, timeout_factor, bitcoind, bitcoin_cli, mocktime, coverage_dir, cwd, extra_conf=None, extra_args=None, use_cli=False, start_perf=False, use_valgrind=False, version=None, descriptors=False, v2transport=False, uses_wallet=False):
+    def __init__(self, i, datadir_path, extra_args_from_options, *, chain, rpchost, timewait, timeout_factor, bitcoind, bitcoin_cli, mocktime, coverage_dir, cwd, extra_conf=None, extra_args=None, use_cli=False, start_perf=False, use_valgrind=False, version=None, descriptors=False, v2transport=False, uses_wallet=False):
         """
         Kwargs:
             start_perf (bool): If True, begin profiling the node with `perf` as soon as
@@ -79,11 +79,11 @@ def __init__(self, i, datadir, extra_args_from_options, *, chain, rpchost, timew
 
         self.index = i
         self.p2p_conn_index = 1
-        self.datadir = datadir
+        self.datadir_path = datadir_path
         self.chain = chain
-        self.bitcoinconf = os.path.join(self.datadir, "dash.conf")
-        self.stdout_dir = os.path.join(self.datadir, "stdout")
-        self.stderr_dir = os.path.join(self.datadir, "stderr")
+        self.bitcoinconf = self.datadir_path / "dash.conf"
+        self.stdout_dir = self.datadir_path / "stdout"
+        self.stderr_dir = self.datadir_path / "stderr"
         self.rpchost = rpchost
         self.rpc_timeout = timewait
         self.binary = bitcoind
@@ -93,7 +93,7 @@ def __init__(self, i, datadir, extra_args_from_options, *, chain, rpchost, timew
         self.descriptors = descriptors
         self.has_explicit_bind = False
         if extra_conf is not None:
-            append_config(datadir, extra_conf)
+            append_config(self.datadir_path, extra_conf)
             # Remember if there is bind=... in the config file.
             self.has_explicit_bind = any(e.startswith("bind=") for e in extra_conf)
         # Most callers will just need to add extra args to the standard list below.
@@ -107,7 +107,7 @@ def __init__(self, i, datadir, extra_args_from_options, *, chain, rpchost, timew
         # spam debug.log.
         self.args = [
             self.binary,
-            "-datadir=" + self.datadir,
+            f"-datadir={self.datadir_path}",
             "-logtimemicros",
             "-debug",
             "-debugexclude=libevent",
@@ -130,9 +130,7 @@ def __init__(self, i, datadir, extra_args_from_options, *, chain, rpchost, timew
 
         # Use valgrind, expect for previous release binaries
         if use_valgrind and version is None:
-            default_suppressions_file = os.path.join(
-                os.path.dirname(os.path.realpath(__file__)),
-                "..", "..", "..", "contrib", "valgrind.supp")
+            default_suppressions_file = Path(__file__).parents[3] / "contrib" / "valgrind.supp"
             suppressions_file = os.getenv("VALGRIND_SUPPRESSIONS_FILE",
                                           default_suppressions_file)
             self.args = ["valgrind", "--suppressions={}".format(suppressions_file),
@@ -159,7 +157,7 @@ def __init__(self, i, datadir, extra_args_from_options, *, chain, rpchost, timew
                 self.args.append("-v2transport=0")
         # if v2transport is requested via global flag but not supported for node version, ignore it
 
-        self.cli = TestNodeCLI(bitcoin_cli, self.datadir)
+        self.cli = TestNodeCLI(bitcoin_cli, self.datadir_path)
         self.use_cli = use_cli
         self.start_perf = start_perf
 
@@ -271,7 +269,7 @@ def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, env=None
         # Delete any existing cookie file -- if such a file exists (eg due to
         # unclean shutdown), it will get overwritten anyway by dashd, and
         # potentially interfere with our attempt to authenticate
-        delete_cookie_file(self.datadir, self.chain)
+        delete_cookie_file(self.datadir_path, self.chain)
 
         # add environment variable LIBC_FATAL_STDERR_=1 so that libc errors are written to stderr and not the terminal
         subp_env = dict(os.environ, LIBC_FATAL_STDERR_="1")
@@ -302,7 +300,7 @@ def wait_for_rpc_connection(self):
                     f'dashd exited with status {self.process.returncode} during initialization. {str_error}'))
             try:
                 rpc = get_rpc_proxy(
-                    rpc_url(self.datadir, self.index, self.chain, self.rpchost),
+                    rpc_url(self.datadir_path, self.index, self.chain, self.rpchost),
                     self.index,
                     timeout=self.rpc_timeout // 2,  # Shorter timeout to allow for one retry in case of ETIMEDOUT
                     coveragedir=self.coverage_dir,
@@ -366,7 +364,7 @@ def wait_for_cookie_credentials(self):
         poll_per_s = 4
         for _ in range(poll_per_s * self.rpc_timeout):
             try:
-                get_auth_cookie(self.datadir, self.chain)
+                get_auth_cookie(self.datadir_path, self.chain)
                 self.log.debug("Cookie credentials successfully retrieved")
                 return
             except ValueError:  # cookie file not found and no rpcuser or rpcpassword; bitcoind is still starting
@@ -480,13 +478,9 @@ def replace_in_config(self, replacements):
         with open(self.bitcoinconf, 'w', encoding='utf8') as conf:
             conf.write(conf_data)
 
-    @property
-    def datadir_path(self) -> Path:
-        return Path(self.datadir)
-
     @property
     def chain_path(self) -> Path:
-        return self.datadir_path / get_chain_folder(self.datadir, self.chain)
+        return self.datadir_path / get_chain_folder(self.datadir_path, self.chain)
 
     @property
     def debug_log_path(self) -> Path:
@@ -633,7 +627,7 @@ def test_success(cmd):
                 "perf output won't be very useful without debug symbols compiled into bitcoind")
 
         output_path = tempfile.NamedTemporaryFile(
-            dir=self.datadir,
+            dir=self.datadir_path,
             prefix="{}.perf.data.".format(profile_name or 'test'),
             delete=False,
         ).name
@@ -919,7 +913,7 @@ def send_cli(self, clicommand=None, *args, **kwargs):
         """Run dash-cli command. Deserializes returned string as python object."""
         pos_args = [arg_to_cli(arg) for arg in args]
         named_args = [str(key) + "=" + arg_to_cli(value) for (key, value) in kwargs.items()]
-        p_args = [self.binary, "-datadir=" + self.datadir] + self.options
+        p_args = [self.binary, f"-datadir={self.datadir}"] + self.options
         if named_args:
             p_args += ["-named"]
         if clicommand is not None:
diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
index 4c6d134018dc..3124e3cf2bf7 100644
--- a/test/functional/test_framework/util.py
+++ b/test/functional/test_framework/util.py
@@ -455,7 +455,7 @@ def write_config(config_path, *, n, chain, extra_config="", disable_autoconnect=
 
 
 def get_datadir_path(dirname, n):
-    return os.path.join(dirname, "node" + str(n))
+    return pathlib.Path(dirname) / f"node{n}"
 
 
 def get_temp_default_datadir(temp_dir: pathlib.Path) -> Tuple[dict, pathlib.Path]:
diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py
index b4c61be7846b..18ff93535571 100755
--- a/test/functional/tool_wallet.py
+++ b/test/functional/tool_wallet.py
@@ -31,7 +31,7 @@ def skip_test_if_missing_module(self):
         self.skip_if_no_wallet_tool()
 
     def dash_wallet_process(self, *args):
-        default_args = ['-datadir={}'.format(self.nodes[0].datadir), '-chain=%s' % self.chain]
+        default_args = ['-datadir={}'.format(self.nodes[0].datadir_path), '-chain=%s' % self.chain]
         if not self.options.descriptors and 'create' in args:
             default_args.append('-legacy')
 
@@ -151,8 +151,8 @@ def assert_dump(self, expected, received):
             assert_equal(v, r[k])
 
     def do_tool_createfromdump(self, wallet_name, dumpfile, file_format=None):
-        dumppath = os.path.join(self.nodes[0].datadir, dumpfile)
-        rt_dumppath = os.path.join(self.nodes[0].datadir, "rt-{}.dump".format(wallet_name))
+        dumppath = self.nodes[0].datadir_path / dumpfile
+        rt_dumppath = self.nodes[0].datadir_path / "rt-{}.dump".format(wallet_name)
 
         dump_data = self.read_dump(dumppath)
 
@@ -332,7 +332,7 @@ def test_dump_createfromdump(self):
         self.assert_raises_tool_error('No dump file provided. To use dump, -dumpfile=<filename> must be provided.', '-wallet=todump', 'dump')
 
         self.log.info('Checking basic dump')
-        wallet_dump = os.path.join(self.nodes[0].datadir, "wallet.dump")
+        wallet_dump = self.nodes[0].datadir_path / "wallet.dump"
         self.assert_tool_output('The dumpfile may contain private keys. To ensure the safety of your Bitcoin, do not share the dumpfile.\n', '-wallet=todump', '-dumpfile={}'.format(wallet_dump), 'dump')
 
         dump_data = self.read_dump(wallet_dump)
@@ -347,7 +347,7 @@ def test_dump_createfromdump(self):
 
         self.log.info('Checking createfromdump arguments')
         self.assert_raises_tool_error('No dump file provided. To use createfromdump, -dumpfile=<filename> must be provided.', '-wallet=todump', 'createfromdump')
-        non_exist_dump = os.path.join(self.nodes[0].datadir, "wallet.nodump")
+        non_exist_dump = self.nodes[0].datadir_path / "wallet.nodump"
         self.assert_raises_tool_error('Unknown wallet file format "notaformat" provided. Please provide one of "bdb" or "sqlite".', '-wallet=todump', '-format=notaformat', '-dumpfile={}'.format(wallet_dump), 'createfromdump')
         self.assert_raises_tool_error('Dump file {} does not exist.'.format(non_exist_dump), '-wallet=todump', '-dumpfile={}'.format(non_exist_dump), 'createfromdump')
         wallet_path = self.nodes[0].wallets_path / "todump2"
@@ -362,17 +362,17 @@ def test_dump_createfromdump(self):
             self.do_tool_createfromdump("load-sqlite", "wallet.dump", "sqlite")
 
         self.log.info('Checking createfromdump handling of magic and versions')
-        bad_ver_wallet_dump = os.path.join(self.nodes[0].datadir, "wallet-bad_ver1.dump")
+        bad_ver_wallet_dump = self.nodes[0].datadir_path / "wallet-bad_ver1.dump"
         dump_data["BITCOIN_CORE_WALLET_DUMP"] = "0"
         self.write_dump(dump_data, bad_ver_wallet_dump)
         self.assert_raises_tool_error('Error: Dumpfile version is not supported. This version of dash-wallet only supports version 1 dumpfiles. Got dumpfile with version 0', '-wallet=badload', '-dumpfile={}'.format(bad_ver_wallet_dump), 'createfromdump')
         assert not (self.nodes[0].wallets_path / "badload").is_dir()
-        bad_ver_wallet_dump = os.path.join(self.nodes[0].datadir, "wallet-bad_ver2.dump")
+        bad_ver_wallet_dump = self.nodes[0].datadir_path / "wallet-bad_ver2.dump"
         dump_data["BITCOIN_CORE_WALLET_DUMP"] = "2"
         self.write_dump(dump_data, bad_ver_wallet_dump)
         self.assert_raises_tool_error('Error: Dumpfile version is not supported. This version of dash-wallet only supports version 1 dumpfiles. Got dumpfile with version 2', '-wallet=badload', '-dumpfile={}'.format(bad_ver_wallet_dump), 'createfromdump')
         assert not (self.nodes[0].wallets_path / "badload").is_dir()
-        bad_magic_wallet_dump = os.path.join(self.nodes[0].datadir, "wallet-bad_magic.dump")
+        bad_magic_wallet_dump = self.nodes[0].datadir_path / "wallet-bad_magic.dump"
         del dump_data["BITCOIN_CORE_WALLET_DUMP"]
         dump_data["not_the_right_magic"] = "1"
         self.write_dump(dump_data, bad_magic_wallet_dump, "not_the_right_magic")
@@ -380,19 +380,19 @@ def test_dump_createfromdump(self):
         assert not (self.nodes[0].wallets_path / "badload").is_dir()
 
         self.log.info('Checking createfromdump handling of checksums')
-        bad_sum_wallet_dump = os.path.join(self.nodes[0].datadir, "wallet-bad_sum1.dump")
+        bad_sum_wallet_dump = self.nodes[0].datadir_path / "wallet-bad_sum1.dump"
         dump_data = orig_dump.copy()
         checksum = dump_data["checksum"]
         dump_data["checksum"] = "1" * 64
         self.write_dump(dump_data, bad_sum_wallet_dump)
         self.assert_raises_tool_error('Error: Dumpfile checksum does not match. Computed {}, expected {}'.format(checksum, "1" * 64), '-wallet=bad', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump')
         assert not (self.nodes[0].wallets_path / "badload").is_dir()
-        bad_sum_wallet_dump = os.path.join(self.nodes[0].datadir, "wallet-bad_sum2.dump")
+        bad_sum_wallet_dump = self.nodes[0].datadir_path / "wallet-bad_sum2.dump"
         del dump_data["checksum"]
         self.write_dump(dump_data, bad_sum_wallet_dump, skip_checksum=True)
         self.assert_raises_tool_error('Error: Missing checksum', '-wallet=badload', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump')
         assert not (self.nodes[0].wallets_path / "badload").is_dir()
-        bad_sum_wallet_dump = os.path.join(self.nodes[0].datadir, "wallet-bad_sum3.dump")
+        bad_sum_wallet_dump = self.nodes[0].datadir_path / "wallet-bad_sum3.dump"
         dump_data["checksum"] = "2" * 10
         self.write_dump(dump_data, bad_sum_wallet_dump)
         self.assert_raises_tool_error('Error: Checksum is not the correct size', '-wallet=badload', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump')
@@ -498,7 +498,7 @@ def test_nonhd(self):
         self.stop_node(0)
 
     def run_test(self):
-        self.wallet_path = os.path.join(self.nodes[0].wallets_path, self.default_wallet_name, self.wallet_data_filename)
+        self.wallet_path = self.nodes[0].wallets_path / self.default_wallet_name / self.wallet_data_filename
         self.test_invalid_tool_commands_and_args()
         # Warning: The following tests are order-dependent.
         self.test_tool_wallet_info()
diff --git a/test/functional/wallet_backup.py b/test/functional/wallet_backup.py
index 9048ec24d210..f297248c49d6 100755
--- a/test/functional/wallet_backup.py
+++ b/test/functional/wallet_backup.py
@@ -110,25 +110,24 @@ def stop_three(self):
         self.stop_node(2)
 
     def erase_three(self):
-        os.remove(os.path.join(self.nodes[0].wallets_path, self.default_wallet_name, self.wallet_data_filename))
-        os.remove(os.path.join(self.nodes[1].wallets_path, self.default_wallet_name, self.wallet_data_filename))
-        os.remove(os.path.join(self.nodes[2].wallets_path, self.default_wallet_name, self.wallet_data_filename))
+        for node_num in range(3):
+            (self.nodes[node_num].wallets_path / self.default_wallet_name / self.wallet_data_filename).unlink()
 
     def restore_invalid_wallet(self):
         node = self.nodes[3]
-        invalid_wallet_file = os.path.join(self.nodes[0].datadir, 'invalid_wallet_file.bak')
+        invalid_wallet_file = self.nodes[0].datadir_path / 'invalid_wallet_file.bak'
         open(invalid_wallet_file, 'a', encoding="utf8").write('invald wallet')
         wallet_name = "res0"
-        not_created_wallet_file = os.path.join(node.wallets_path, wallet_name)
+        not_created_wallet_file = node.wallets_path / wallet_name
         error_message = "Wallet file verification failed. Failed to load database path '{}'. Data is not in recognized format.".format(not_created_wallet_file)
         assert_raises_rpc_error(-18, error_message, node.restorewallet, wallet_name, invalid_wallet_file)
-        assert not os.path.exists(not_created_wallet_file)
+        assert not not_created_wallet_file.exists()
 
     def restore_corrupt_bdb_wallet(self):
         if self.options.descriptors:
             return
         node = self.nodes[3]
-        backup_file = os.path.join(self.nodes[0].datadir, 'wallet.bak')
+        backup_file = os.path.join(self.nodes[0].datadir_path, 'wallet.bak')
         corrupt_file = backup_file + '.corrupt'
         with open(backup_file, 'rb') as source, open(corrupt_file, 'wb') as dest:
             dest.write(source.read(4096))
@@ -138,18 +137,18 @@ def restore_corrupt_bdb_wallet(self):
 
     def restore_nonexistent_wallet(self):
         node = self.nodes[3]
-        nonexistent_wallet_file = os.path.join(self.nodes[0].datadir, 'nonexistent_wallet.bak')
+        nonexistent_wallet_file = self.nodes[0].datadir_path / 'nonexistent_wallet.bak'
         wallet_name = "res0"
         assert_raises_rpc_error(-8, "Backup file does not exist", node.restorewallet, wallet_name, nonexistent_wallet_file)
-        not_created_wallet_file = os.path.join(node.wallets_path, wallet_name)
-        assert not os.path.exists(not_created_wallet_file)
+        not_created_wallet_file = node.wallets_path / wallet_name
+        assert not not_created_wallet_file.exists()
 
     def restore_preserves_existing_directory(self):
         node = self.nodes[3]
         wallet_dir = node.wallets_path
-        backup_file = os.path.join(self.nodes[0].datadir, 'wallet.bak')
-        invalid_path = os.path.join(node.datadir, 'x' * 256)
-        for wallet_name in ('restore_existing', os.path.join(node.datadir, 'restore_absolute')):
+        backup_file = os.path.join(self.nodes[0].datadir_path, 'wallet.bak')
+        invalid_path = os.path.join(node.datadir_path, 'x' * 256)
+        for wallet_name in ('restore_existing', os.path.join(node.datadir_path, 'restore_absolute')):
             destination = os.path.join(wallet_dir, wallet_name)
             sentinel = os.path.join(destination, 'nested', 'sentinel')
             os.makedirs(os.path.dirname(sentinel))
@@ -169,7 +168,7 @@ def restore_preserves_existing_directory(self):
 
     def restore_wallet_existent_name(self):
         node = self.nodes[3]
-        backup_file = os.path.join(self.nodes[0].datadir, 'wallet.bak')
+        backup_file = self.nodes[0].datadir_path / 'wallet.bak'
         wallet_name = "res0"
         wallet_file = node.wallets_path / wallet_name
         error_message = "Failed to restore wallet. Database file exists in '{}'.".format(wallet_file / "wallet.dat")
@@ -237,7 +236,6 @@ def test_restore_into_unnamed_wallet(self):
             os.rename(node.wallets_path / "default.wallet.dat", node.wallets_path / "wallet.dat")
             node.loadwallet("")
 
-
     def run_test(self):
         self.log.info("Generating initial blockchain")
         self.generate(self.nodes[0], 1)
@@ -257,14 +255,12 @@ def run_test(self):
 
         self.log.info("Backing up")
 
-        self.nodes[0].backupwallet(os.path.join(self.nodes[0].datadir, 'wallet.bak'))
-        self.nodes[1].backupwallet(os.path.join(self.nodes[1].datadir, 'wallet.bak'))
-        self.nodes[2].backupwallet(os.path.join(self.nodes[2].datadir, 'wallet.bak'))
+        for node_num in range(3):
+            self.nodes[node_num].backupwallet(self.nodes[node_num].datadir_path / 'wallet.bak')
 
         if not self.options.descriptors:
-            self.nodes[0].dumpwallet(os.path.join(self.nodes[0].datadir, 'wallet.dump'))
-            self.nodes[1].dumpwallet(os.path.join(self.nodes[1].datadir, 'wallet.dump'))
-            self.nodes[2].dumpwallet(os.path.join(self.nodes[2].datadir, 'wallet.dump'))
+            for node_num in range(3):
+                self.nodes[node_num].dumpwallet(self.nodes[node_num].datadir_path / 'wallet.dump')
 
         self.log.info("More transactions")
         for _ in range(5):
@@ -293,17 +289,13 @@ def run_test(self):
         self.restore_preserves_existing_directory()
         self.restore_corrupt_bdb_wallet()
 
-        backup_file_0 = os.path.join(self.nodes[0].datadir, 'wallet.bak')
-        backup_file_1 = os.path.join(self.nodes[1].datadir, 'wallet.bak')
-        backup_file_2 = os.path.join(self.nodes[2].datadir, 'wallet.bak')
+        backup_files = []
+        for node_num in range(3):
+            backup_files.append(self.nodes[node_num].datadir_path / 'wallet.bak')
 
-        self.nodes[3].restorewallet("res0", backup_file_0)
-        self.nodes[3].restorewallet("res1", backup_file_1)
-        self.nodes[3].restorewallet("res2", backup_file_2)
-
-        assert os.path.exists(os.path.join(self.nodes[3].wallets_path, "res0"))
-        assert os.path.exists(os.path.join(self.nodes[3].wallets_path, "res1"))
-        assert os.path.exists(os.path.join(self.nodes[3].wallets_path, "res2"))
+        for idx, backup_file in enumerate(backup_files):
+            self.nodes[3].restorewallet(f'res{idx}', backup_file)
+            assert (self.nodes[3].wallets_path / f'res{idx}').exists()
 
         res0_rpc = self.nodes[3].get_wallet_rpc("res0")
         res1_rpc = self.nodes[3].get_wallet_rpc("res1")
@@ -323,25 +315,19 @@ def run_test(self):
             self.erase_three()
 
             #start node2 with no chain
-            shutil.rmtree(os.path.join(self.nodes[2].chain_path, 'blocks'))
-            shutil.rmtree(os.path.join(self.nodes[2].chain_path, 'chainstate'))
-            shutil.rmtree(os.path.join(self.nodes[2].chain_path, 'evodb'))
-            shutil.rmtree(os.path.join(self.nodes[2].chain_path, 'llmq'))
-            shutil.rmtree(os.path.join(self.nodes[2].chain_path, 'indexes'))
+            shutil.rmtree(self.nodes[2].blocks_path)
+            shutil.rmtree(self.nodes[2].chain_path / 'chainstate')
+            shutil.rmtree(self.nodes[2].chain_path / 'evodb')
+            shutil.rmtree(self.nodes[2].chain_path / 'llmq')
+            shutil.rmtree(self.nodes[2].chain_path / 'indexes')
 
             self.start_three(["-nowallet"])
             # Create new wallets for the three nodes.
             # We will use this empty wallets to test the 'importwallet()' RPC command below.
             for node_num in range(3):
                 self.nodes[node_num].createwallet(wallet_name=self.default_wallet_name, descriptors=self.options.descriptors, load_on_startup=True)
-
-            assert_equal(self.nodes[0].getbalance(), 0)
-            assert_equal(self.nodes[1].getbalance(), 0)
-            assert_equal(self.nodes[2].getbalance(), 0)
-
-            self.nodes[0].importwallet(os.path.join(self.nodes[0].datadir, 'wallet.dump'))
-            self.nodes[1].importwallet(os.path.join(self.nodes[1].datadir, 'wallet.dump'))
-            self.nodes[2].importwallet(os.path.join(self.nodes[2].datadir, 'wallet.dump'))
+                assert_equal(self.nodes[node_num].getbalance(), 0)
+                self.nodes[node_num].importwallet(self.nodes[node_num].datadir_path / 'wallet.dump')
 
             self.sync_blocks()
 
diff --git a/test/functional/wallet_blank.py b/test/functional/wallet_blank.py
index 69927dcc3074..308a3ba84f01 100755
--- a/test/functional/wallet_blank.py
+++ b/test/functional/wallet_blank.py
@@ -3,7 +3,6 @@
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or https://www.opensource.org/licenses/mit-license.php.
 
-import os
 
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.address import (
@@ -116,7 +115,7 @@ def test_importwallet(self):
         assert_equal(info["descriptors"], False)
         assert_equal(info["blank"], True)
 
-        wallet_dump_path = os.path.join(self.nodes[0].datadir, "wallet.dump")
+        wallet_dump_path = self.nodes[0].datadir_path / "wallet.dump"
         def_wallet.dumpwallet(wallet_dump_path)
 
         wallet.importwallet(wallet_dump_path)
diff --git a/test/functional/wallet_crosschain.py b/test/functional/wallet_crosschain.py
index 244f80fcb96b..1bb9271751d5 100755
--- a/test/functional/wallet_crosschain.py
+++ b/test/functional/wallet_crosschain.py
@@ -3,8 +3,6 @@
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-import os
-
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_raises_rpc_error
 
@@ -31,13 +29,13 @@ def setup_network(self):
     def run_test(self):
         self.log.info("Creating wallets")
 
-        node0_wallet = os.path.join(self.nodes[0].datadir, 'node0_wallet')
-        node0_wallet_backup = os.path.join(self.nodes[0].datadir, 'node0_wallet.bak')
+        node0_wallet = self.nodes[0].datadir_path / 'node0_wallet'
+        node0_wallet_backup = self.nodes[0].datadir_path / 'node0_wallet.bak'
         self.nodes[0].createwallet(node0_wallet)
         self.nodes[0].backupwallet(node0_wallet_backup)
         self.nodes[0].unloadwallet(node0_wallet)
-        node1_wallet = os.path.join(self.nodes[1].datadir, 'node1_wallet')
-        node1_wallet_backup = os.path.join(self.nodes[0].datadir, 'node1_wallet.bak')
+        node1_wallet = self.nodes[1].datadir_path / 'node1_wallet'
+        node1_wallet_backup = self.nodes[0].datadir_path / 'node1_wallet.bak'
         self.nodes[1].createwallet(node1_wallet)
         self.nodes[1].backupwallet(node1_wallet_backup)
         self.nodes[1].unloadwallet(node1_wallet)
diff --git a/test/functional/wallet_descriptor.py b/test/functional/wallet_descriptor.py
index 1cde6e1c7cfa..1d7192ba19cc 100755
--- a/test/functional/wallet_descriptor.py
+++ b/test/functional/wallet_descriptor.py
@@ -3,7 +3,6 @@
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test descriptor wallet function."""
-import os
 
 try:
     import sqlite3
@@ -198,7 +197,7 @@ def run_test(self):
         self.log.info("Test that loading descriptor wallet containing legacy key types throws error")
         self.nodes[0].createwallet(wallet_name="crashme", descriptors=True)
         self.nodes[0].unloadwallet("crashme")
-        wallet_db = os.path.join(self.nodes[0].wallets_path, "crashme", self.wallet_data_filename)
+        wallet_db = self.nodes[0].wallets_path / "crashme" / self.wallet_data_filename
         conn = sqlite3.connect(wallet_db)
         with conn:
             # add "cscript" entry: key type is uint160 (20 bytes), value type is CScript (zero-length here)
diff --git a/test/functional/wallet_dump.py b/test/functional/wallet_dump.py
index ec2868d72083..a52a723d64f5 100755
--- a/test/functional/wallet_dump.py
+++ b/test/functional/wallet_dump.py
@@ -109,8 +109,8 @@ def test_electrum_import(self):
         destination = node.get_wallet_rpc('electrum_destination')
         addresses = [source.getnewaddress() for _ in range(4)]
         secrets = [source.dumpprivkey(address) for address in addresses]
-        json_file = os.path.join(node.datadir, 'electrum.json')
-        csv_file = os.path.join(node.datadir, 'electrum.csv')
+        json_file = os.path.join(node.datadir_path, 'electrum.json')
+        csv_file = os.path.join(node.datadir_path, 'electrum.csv')
         with open(json_file, 'wb') as f:
             f.write(json.dumps({addresses[0]: secrets[0]}).encode() + b'\x00trailing bytes')
         assert_raises_rpc_error(-3, 'Cannot parse Electrum wallet export file', destination.importelectrumwallet, json_file)
@@ -136,8 +136,8 @@ def run_test(self):
         self.test_electrum_import()
         self.nodes[0].createwallet("dump")
 
-        wallet_unenc_dump = os.path.join(self.nodes[0].datadir, "wallet.unencrypted.dump")
-        wallet_enc_dump = os.path.join(self.nodes[0].datadir, "wallet.encrypted.dump")
+        wallet_unenc_dump = self.nodes[0].datadir_path / "wallet.unencrypted.dump"
+        wallet_enc_dump = self.nodes[0].datadir_path / "wallet.encrypted.dump"
 
         # generate 20 addresses to compare against the dump
         test_addr_count = 20
@@ -175,7 +175,7 @@ def run_test(self):
 
         self.log.info('Dump unencrypted wallet')
         result = self.nodes[0].dumpwallet(wallet_unenc_dump)
-        assert_equal(result['filename'], wallet_unenc_dump)
+        assert_equal(result['filename'], str(wallet_unenc_dump))
 
         found_comments, found_addr, found_script_addr, found_addr_chg, found_addr_rsv, hd_master_addr_unenc = \
             read_dump(wallet_unenc_dump, addrs, script_addrs, None)
@@ -239,7 +239,7 @@ def run_test(self):
         w3.sendtoaddress(w3.getnewaddress(), 10)
         w3.unloadwallet()
         self.nodes[0].loadwallet("w3")
-        w3.dumpwallet(os.path.join(self.nodes[0].datadir, "w3.dump"))
+        w3.dumpwallet(self.nodes[0].datadir_path / "w3.dump")
 
 if __name__ == '__main__':
     WalletDumpTest().main()
diff --git a/test/functional/wallet_fast_rescan.py b/test/functional/wallet_fast_rescan.py
index b39f12154913..fce99fdd12bb 100755
--- a/test/functional/wallet_fast_rescan.py
+++ b/test/functional/wallet_fast_rescan.py
@@ -4,7 +4,6 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test that fast rescan using block filters for descriptor wallets detects
    top-ups correctly and finds the same transactions than the slow variant."""
-import os
 from typing import List
 
 from test_framework.address import address_to_scriptpubkey
@@ -43,7 +42,7 @@ def run_test(self):
         wallet = MiniWallet(node)
 
         self.log.info("Create descriptor wallet with backup")
-        WALLET_BACKUP_FILENAME = os.path.join(node.datadir, 'wallet.bak')
+        WALLET_BACKUP_FILENAME = node.datadir_path / 'wallet.bak'
         node.createwallet(wallet_name='topup_test', descriptors=True)
         w = node.get_wallet_rpc('topup_test')
         fixed_key = get_generate_key()
diff --git a/test/functional/wallet_hd.py b/test/functional/wallet_hd.py
index 2d3d8d77ff89..a3b8b19fe541 100755
--- a/test/functional/wallet_hd.py
+++ b/test/functional/wallet_hd.py
@@ -5,7 +5,6 @@
 """Test Hierarchical Deterministic wallet function."""
 
 import shutil
-import os
 
 from test_framework.blocktools import COINBASE_MATURITY
 from test_framework.test_framework import BitcoinTestFramework
@@ -56,8 +55,8 @@ def run_test(self):
         self.nodes[1].importprivkey(non_hd_key)
 
         # This should be enough to keep the master key and the non-HD key
-        self.nodes[1].backupwallet(os.path.join(self.nodes[1].datadir, "hd.bak"))
-        #self.nodes[1].dumpwallet(os.path.join(self.nodes[1].datadir, "hd.dump"))
+        self.nodes[1].backupwallet(self.nodes[1].datadir_path / "hd.bak")
+        #self.nodes[1].dumpwallet(self.nodes[1].datadir_path / "hd.dump")
 
         # Derive some HD addresses and remember the last
         # Also send funds to each add
@@ -86,14 +85,14 @@ def run_test(self):
         self.stop_node(1)
         # we need to delete the complete chain directory
         # otherwise node1 would auto-recover all funds in flag the keypool keys as used
-        shutil.rmtree(os.path.join(self.nodes[1].chain_path, "blocks"))
-        shutil.rmtree(os.path.join(self.nodes[1].chain_path, "chainstate"))
-        shutil.rmtree(os.path.join(self.nodes[1].chain_path, "evodb"))
-        shutil.rmtree(os.path.join(self.nodes[1].chain_path, "llmq"))
-        shutil.rmtree(os.path.join(self.nodes[1].chain_path, "indexes"))
+        shutil.rmtree(self.nodes[1].blocks_path)
+        shutil.rmtree(self.nodes[1].chain_path / "chainstate")
+        shutil.rmtree(self.nodes[1].chain_path / "evodb")
+        shutil.rmtree(self.nodes[1].chain_path / "llmq")
+        shutil.rmtree(self.nodes[1].chain_path / "indexes")
         shutil.copyfile(
-            os.path.join(self.nodes[1].datadir, "hd.bak"),
-            os.path.join(self.nodes[1].wallets_path, self.default_wallet_name, self.wallet_data_filename),
+            self.nodes[1].datadir_path / "hd.bak",
+            self.nodes[1].wallets_path / self.default_wallet_name / self.wallet_data_filename
         )
         self.start_node(1)
 
@@ -114,14 +113,14 @@ def run_test(self):
 
         # Try a RPC based rescan
         self.stop_node(1)
-        shutil.rmtree(os.path.join(self.nodes[1].chain_path, "blocks"))
-        shutil.rmtree(os.path.join(self.nodes[1].chain_path, "chainstate"))
-        shutil.rmtree(os.path.join(self.nodes[1].chain_path, "evodb"))
-        shutil.rmtree(os.path.join(self.nodes[1].chain_path, "llmq"))
-        shutil.rmtree(os.path.join(self.nodes[1].chain_path, "indexes"))
+        shutil.rmtree(self.nodes[1].blocks_path)
+        shutil.rmtree(self.nodes[1].chain_path / "chainstate")
+        shutil.rmtree(self.nodes[1].chain_path / "evodb")
+        shutil.rmtree(self.nodes[1].chain_path / "llmq")
+        shutil.rmtree(self.nodes[1].chain_path / "indexes")
         shutil.copyfile(
-            os.path.join(self.nodes[1].datadir, "hd.bak"),
-            os.path.join(self.nodes[1].wallets_path, self.default_wallet_name, self.wallet_data_filename),
+            self.nodes[1].datadir_path / "hd.bak",
+            self.nodes[1].wallets_path / self.default_wallet_name / self.wallet_data_filename
         )
         self.start_node(1, extra_args=self.extra_args[1])
         self.connect_nodes(0, 1)
diff --git a/test/functional/wallet_keypool_topup.py b/test/functional/wallet_keypool_topup.py
index 8e5680fbd3db..aa49473d3cb9 100755
--- a/test/functional/wallet_keypool_topup.py
+++ b/test/functional/wallet_keypool_topup.py
@@ -10,7 +10,6 @@
 - Generate 110 keys (enough to drain the keypool). Store key 90 (in the initial keypool) and key 110 (beyond the initial keypool). Send funds to key 90 and key 110.
 - Stop node1, clear the datadir, move wallet file back into the datadir and restart node1.
 - connect node1 to node0. Verify that they sync and node1 receives its funds."""
-import os
 import shutil
 
 from test_framework.blocktools import COINBASE_MATURITY
@@ -33,8 +32,8 @@ def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
 
     def run_test(self):
-        wallet_path = os.path.join(self.nodes[1].wallets_path, self.default_wallet_name, self.wallet_data_filename)
-        wallet_backup_path = os.path.join(self.nodes[1].datadir, "wallet.bak")
+        wallet_path = self.nodes[1].wallets_path / self.default_wallet_name / self.wallet_data_filename
+        wallet_backup_path = self.nodes[1].datadir_path / "wallet.bak"
         self.generate(self.nodes[0], COINBASE_MATURITY + 1)
 
         self.log.info("Make backup of wallet")
diff --git a/test/functional/wallet_pruning.py b/test/functional/wallet_pruning.py
index bcbe9efb2c88..bae3f8ac05c7 100755
--- a/test/functional/wallet_pruning.py
+++ b/test/functional/wallet_pruning.py
@@ -4,7 +4,6 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 """Test wallet import on pruned node."""
-import os
 
 from test_framework.util import assert_equal, assert_raises_rpc_error
 from test_framework.blocktools import (
@@ -82,7 +81,7 @@ def test_wallet_import_pruned(self, wallet_name):
 
         # Import wallet into pruned node
         self.nodes[1].createwallet(wallet_name="wallet_pruned", descriptors=False, load_on_startup=True)
-        self.nodes[1].importwallet(os.path.join(self.nodes[0].datadir, wallet_file))
+        self.nodes[1].importwallet(self.nodes[0].datadir_path / wallet_file)
 
         # Make sure that prune node's wallet correctly accounts for balances
         assert_equal(self.nodes[1].getbalance(), self.nodes[0].getbalance())
@@ -101,12 +100,12 @@ def test_wallet_import_pruned_with_missing_blocks(self, wallet_name):
         # Make sure wallet cannot be imported because of missing blocks
         # This will try to rescan blocks `TIMESTAMP_WINDOW` (2h) before the wallet birthheight.
         # There are 6 blocks an hour, so 11 blocks (excluding birthheight).
-        assert_raises_rpc_error(-4, f"Pruned blocks from height {wallet_birthheight - 11} required to import keys. Use RPC call getblockchaininfo to determine your pruned height.", self.nodes[1].importwallet, os.path.join(self.nodes[0].datadir, wallet_file))
+        assert_raises_rpc_error(-4, f"Pruned blocks from height {wallet_birthheight - 11} required to import keys. Use RPC call getblockchaininfo to determine your pruned height.", self.nodes[1].importwallet, self.nodes[0].datadir_path / wallet_file)
         self.log.info("- Done")
 
     def get_birthheight(self, wallet_file):
         """Gets birthheight of a wallet on node0"""
-        with open(os.path.join(self.nodes[0].datadir, wallet_file), 'r', encoding="utf8") as f:
+        with open(self.nodes[0].datadir_path / wallet_file, 'r', encoding="utf8") as f:
             for line in f:
                 if line.startswith('# * Best block at time of backup'):
                     wallet_birthheight = int(line.split(' ')[9])
@@ -114,12 +113,12 @@ def get_birthheight(self, wallet_file):
 
     def has_block(self, block_index):
         """Checks if the pruned node has the specific blk0000*.dat file"""
-        return os.path.isfile(os.path.join(self.nodes[1].chain_path, "blocks", f"blk{block_index:05}.dat"))
+        return (self.nodes[1].blocks_path / f"blk{block_index:05}.dat").is_file()
 
     def create_wallet(self, wallet_name, *, unload=False):
         """Creates and dumps a wallet on the non-pruned node0 to be later import by the pruned node"""
         self.nodes[0].createwallet(wallet_name=wallet_name, descriptors=False, load_on_startup=True)
-        self.nodes[0].dumpwallet(os.path.join(self.nodes[0].datadir, wallet_name + ".dat"))
+        self.nodes[0].dumpwallet(self.nodes[0].datadir_path / f"{wallet_name}.dat")
         if (unload):
             self.nodes[0].unloadwallet(wallet_name)
 
diff --git a/test/functional/wallet_reorgsrestore.py b/test/functional/wallet_reorgsrestore.py
index af01b9439ff9..86a2905c723f 100755
--- a/test/functional/wallet_reorgsrestore.py
+++ b/test/functional/wallet_reorgsrestore.py
@@ -14,7 +14,6 @@
 """
 
 from decimal import Decimal
-import os
 import shutil
 
 from test_framework.test_framework import BitcoinTestFramework
@@ -88,8 +87,8 @@ def run_test(self):
 
         # Node0 wallet file is loaded on longest sync'ed node1
         self.stop_node(1)
-        self.nodes[0].backupwallet(os.path.join(self.nodes[0].datadir, 'wallet.bak'))
-        shutil.copyfile(os.path.join(self.nodes[0].datadir, 'wallet.bak'), os.path.join(self.nodes[1].chain_path, self.default_wallet_name, self.wallet_data_filename))
+        self.nodes[0].backupwallet(self.nodes[0].datadir_path / 'wallet.bak')
+        shutil.copyfile(self.nodes[0].datadir_path / 'wallet.bak', self.nodes[1].chain_path / self.default_wallet_name / self.wallet_data_filename)
         self.start_node(1)
         tx_after_reorg = self.nodes[1].gettransaction(txid)
         # Check that normal confirmed tx is confirmed again but with different blockhash
diff --git a/test/functional/wallet_upgradetohd.py b/test/functional/wallet_upgradetohd.py
index 2f00de4fa35f..47bfa398cf63 100755
--- a/test/functional/wallet_upgradetohd.py
+++ b/test/functional/wallet_upgradetohd.py
@@ -40,14 +40,14 @@ def recover_non_hd(self):
         self.log.info("Recover non-HD wallet to check different upgrade paths")
         node = self.nodes[0]
         self.stop_node(0)
-        shutil.copyfile(os.path.join(node.datadir, "non_hd.bak"), os.path.join(node.chain_path, self.default_wallet_name, self.wallet_data_filename))
+        shutil.copyfile(os.path.join(node.datadir_path, "non_hd.bak"), os.path.join(node.chain_path, self.default_wallet_name, self.wallet_data_filename))
         self.start_node(0)
         if not self.options.descriptors:
             assert 'hdchainid' not in node.getwalletinfo()
 
     def run_test(self):
         node = self.nodes[0]
-        node.backupwallet(os.path.join(node.datadir, "non_hd.bak"))
+        node.backupwallet(os.path.join(node.datadir_path, "non_hd.bak"))
 
         self.log.info("No mnemonic, no mnemonic passphrase, no wallet passphrase")
         assert 'hdchainid' not in node.getwalletinfo()