Merge #19044: net processing: Add support for getcfilters

9e36067d8cd02830c7e5a88a391dff6ac3adbe0c [test] Add test for cfilters. (Jim Posen)
11106a4722558765a44ae45c7892724a73ce514c [net processing] Message handling for getcfilters. (Jim Posen)
e535670726952e43483763dfca6fc6ec2f4b0691 [indexes] Fix default [de]serialization of BlockFilter. (Jim Posen)
bb911ae7f5cbe4974ec61266d2334b95067fa49d [refactor] Pass CNode and CConnman by reference (John Newbery)

Pull request description:

  Support `getcfilters` requests when `-peerblockfilters` is set.

  Does not advertise compact filter support in version messages.

ACKs for top commit:
  Empact:
    re-Code Review ACK https://github.com/bitcoin/bitcoin/pull/19044/commits/9e36067d8cd02830c7e5a88a391dff6ac3adbe0c
  MarcoFalke:
    re-ACK 9e36067d8c , only change is adding commit "[refactor] Pass CNode and CConnman by reference" 🥑
  jkczyz:
    ACK 9e36067d8cd02830c7e5a88a391dff6ac3adbe0c
  fjahr:
    Code review ACK 9e36067d8cd02830c7e5a88a391dff6ac3adbe0c

Tree-SHA512: b45b42a25905ef0bd9e195029185300c86856c87f78cbe17921f4a25e159ae0f6f003e61714fa43779017eb97cd89d3568419be88e47d19dc8095562939e7887

7 files changed, 215 insertions, 30 deletions

diff --git a/src/blockfilter.h b/src/blockfilter.h
index ff8744b217..96cefbf3b2 100644
--- a/src/blockfilter.h
+++ b/src/blockfilter.h
@@ -144,8 +144,8 @@ public:
 
     template <typename Stream>
     void Serialize(Stream& s) const {
-        s << m_block_hash
-          << static_cast<uint8_t>(m_filter_type)
+        s << static_cast<uint8_t>(m_filter_type)
+          << m_block_hash
           << m_filter.GetEncoded();
     }
 
@@ -154,8 +154,8 @@ public:
         std::vector<unsigned char> encoded_filter;
         uint8_t filter_type;
 
-        s >> m_block_hash
-          >> filter_type
+        s >> filter_type
+          >> m_block_hash
           >> encoded_filter;
 
         m_filter_type = static_cast<BlockFilterType>(filter_type);
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 6b9b960552..404b33a977 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -129,6 +129,8 @@ static constexpr unsigned int INVENTORY_BROADCAST_MAX = 7 * INVENTORY_BROADCAST_
 static constexpr unsigned int AVG_FEEFILTER_BROADCAST_INTERVAL = 10 * 60;
 /** Maximum feefilter broadcast delay after significant change. */
 static constexpr unsigned int MAX_FEEFILTER_CHANGE_DELAY = 5 * 60;
+/** Maximum number of compact filters that may be requested with one getcfilters. See BIP 157. */
+static constexpr uint32_t MAX_GETCFILTERS_SIZE = 1000;
 /** Maximum number of cf hashes that may be requested with one getcfheaders. See BIP 157. */
 static constexpr uint32_t MAX_GETCFHEADERS_SIZE = 2000;
 
@@ -1999,7 +2001,7 @@ void static ProcessOrphanTx(CConnman* connman, CTxMemPool& mempool, std::set<uin
  * @param[out]  filter_index    The filter index, if the request can be serviced.
  * @return                      True if the request can be serviced.
  */
-static bool PrepareBlockFilterRequest(CNode* pfrom, const CChainParams& chain_params,
+static bool PrepareBlockFilterRequest(CNode& pfrom, const CChainParams& chain_params,
                                       BlockFilterType filter_type, uint32_t start_height,
                                       const uint256& stop_hash, uint32_t max_height_diff,
                                       const CBlockIndex*& stop_index,
@@ -2010,8 +2012,8 @@ static bool PrepareBlockFilterRequest(CNode* pfrom, const CChainParams& chain_pa
          gArgs.GetBoolArg("-peerblockfilters", DEFAULT_PEERBLOCKFILTERS));
     if (!supported_filter_type) {
         LogPrint(BCLog::NET, "peer %d requested unsupported block filter type: %d\n",
-                 pfrom->GetId(), static_cast<uint8_t>(filter_type));
-        pfrom->fDisconnect = true;
+                 pfrom.GetId(), static_cast<uint8_t>(filter_type));
+        pfrom.fDisconnect = true;
         return false;
     }
 
@@ -2022,8 +2024,8 @@ static bool PrepareBlockFilterRequest(CNode* pfrom, const CChainParams& chain_pa
         // Check that the stop block exists and the peer would be allowed to fetch it.
         if (!stop_index || !BlockRequestAllowed(stop_index, chain_params.GetConsensus())) {
             LogPrint(BCLog::NET, "peer %d requested invalid block hash: %s\n",
-                     pfrom->GetId(), stop_hash.ToString());
-            pfrom->fDisconnect = true;
+                     pfrom.GetId(), stop_hash.ToString());
+            pfrom.fDisconnect = true;
             return false;
         }
     }
@@ -2032,14 +2034,14 @@ static bool PrepareBlockFilterRequest(CNode* pfrom, const CChainParams& chain_pa
     if (start_height > stop_height) {
         LogPrint(BCLog::NET, "peer %d sent invalid getcfilters/getcfheaders with " /* Continued */
                  "start height %d and stop height %d\n",
-                 pfrom->GetId(), start_height, stop_height);
-        pfrom->fDisconnect = true;
+                 pfrom.GetId(), start_height, stop_height);
+        pfrom.fDisconnect = true;
         return false;
     }
     if (stop_height - start_height >= max_height_diff) {
         LogPrint(BCLog::NET, "peer %d requested too many cfilters/cfheaders: %d / %d\n",
-                 pfrom->GetId(), stop_height - start_height + 1, max_height_diff);
-        pfrom->fDisconnect = true;
+                 pfrom.GetId(), stop_height - start_height + 1, max_height_diff);
+        pfrom.fDisconnect = true;
         return false;
     }
 
@@ -2053,6 +2055,49 @@ static bool PrepareBlockFilterRequest(CNode* pfrom, const CChainParams& chain_pa
 }
 
 /**
+ * Handle a cfilters request.
+ *
+ * May disconnect from the peer in the case of a bad request.
+ *
+ * @param[in]   pfrom           The peer that we received the request from
+ * @param[in]   vRecv           The raw message received
+ * @param[in]   chain_params    Chain parameters
+ * @param[in]   connman         Pointer to the connection manager
+ */
+static void ProcessGetCFilters(CNode& pfrom, CDataStream& vRecv, const CChainParams& chain_params,
+                               CConnman& connman)
+{
+    uint8_t filter_type_ser;
+    uint32_t start_height;
+    uint256 stop_hash;
+
+    vRecv >> filter_type_ser >> start_height >> stop_hash;
+
+    const BlockFilterType filter_type = static_cast<BlockFilterType>(filter_type_ser);
+
+    const CBlockIndex* stop_index;
+    BlockFilterIndex* filter_index;
+    if (!PrepareBlockFilterRequest(pfrom, chain_params, filter_type, start_height, stop_hash,
+                                   MAX_GETCFILTERS_SIZE, stop_index, filter_index)) {
+        return;
+    }
+
+    std::vector<BlockFilter> filters;
+
+    if (!filter_index->LookupFilterRange(start_height, stop_index, filters)) {
+        LogPrint(BCLog::NET, "Failed to find block filter in index: filter_type=%s, start_height=%d, stop_hash=%s\n",
+                     BlockFilterTypeName(filter_type), start_height, stop_hash.ToString());
+        return;
+    }
+
+    for (const auto& filter : filters) {
+        CSerializedNetMsg msg = CNetMsgMaker(pfrom.GetSendVersion())
+            .Make(NetMsgType::CFILTER, filter);
+        connman.PushMessage(&pfrom, std::move(msg));
+    }
+}
+
+/**
  * Handle a cfheaders request.
  *
  * May disconnect from the peer in the case of a bad request.
@@ -2062,8 +2107,8 @@ static bool PrepareBlockFilterRequest(CNode* pfrom, const CChainParams& chain_pa
  * @param[in]   chain_params    Chain parameters
  * @param[in]   connman         Pointer to the connection manager
  */
-static void ProcessGetCFHeaders(CNode* pfrom, CDataStream& vRecv, const CChainParams& chain_params,
-                                CConnman* connman)
+static void ProcessGetCFHeaders(CNode& pfrom, CDataStream& vRecv, const CChainParams& chain_params,
+                                CConnman& connman)
 {
     uint8_t filter_type_ser;
     uint32_t start_height;
@@ -2098,13 +2143,13 @@ static void ProcessGetCFHeaders(CNode* pfrom, CDataStream& vRecv, const CChainPa
         return;
     }
 
-    CSerializedNetMsg msg = CNetMsgMaker(pfrom->GetSendVersion())
+    CSerializedNetMsg msg = CNetMsgMaker(pfrom.GetSendVersion())
         .Make(NetMsgType::CFHEADERS,
               filter_type_ser,
               stop_index->GetBlockHash(),
               prev_header,
               filter_hashes);
-    connman->PushMessage(pfrom, std::move(msg));
+    connman.PushMessage(&pfrom, std::move(msg));
 }
 
 /**
@@ -2117,8 +2162,8 @@ static void ProcessGetCFHeaders(CNode* pfrom, CDataStream& vRecv, const CChainPa
  * @param[in]   chain_params    Chain parameters
  * @param[in]   connman         Pointer to the connection manager
  */
-static void ProcessGetCFCheckPt(CNode* pfrom, CDataStream& vRecv, const CChainParams& chain_params,
-                                CConnman* connman)
+static void ProcessGetCFCheckPt(CNode& pfrom, CDataStream& vRecv, const CChainParams& chain_params,
+                                CConnman& connman)
 {
     uint8_t filter_type_ser;
     uint256 stop_hash;
@@ -2150,12 +2195,12 @@ static void ProcessGetCFCheckPt(CNode* pfrom, CDataStream& vRecv, const CChainPa
         }
     }
 
-    CSerializedNetMsg msg = CNetMsgMaker(pfrom->GetSendVersion())
+    CSerializedNetMsg msg = CNetMsgMaker(pfrom.GetSendVersion())
         .Make(NetMsgType::CFCHECKPT,
               filter_type_ser,
               stop_index->GetBlockHash(),
               headers);
-    connman->PushMessage(pfrom, std::move(msg));
+    connman.PushMessage(&pfrom, std::move(msg));
 }
 
 bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRecv, int64_t nTimeReceived, const CChainParams& chainparams, ChainstateManager& chainman, CTxMemPool& mempool, CConnman* connman, BanMan* banman, const std::atomic<bool>& interruptMsgProc)
@@ -3467,13 +3512,18 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec
         return true;
     }
 
+    if (msg_type == NetMsgType::GETCFILTERS) {
+        ProcessGetCFilters(*pfrom, vRecv, chainparams, *connman);
+        return true;
+    }
+
     if (msg_type == NetMsgType::GETCFHEADERS) {
-        ProcessGetCFHeaders(pfrom, vRecv, chainparams, connman);
+        ProcessGetCFHeaders(*pfrom, vRecv, chainparams, *connman);
         return true;
     }
 
     if (msg_type == NetMsgType::GETCFCHECKPT) {
-        ProcessGetCFCheckPt(pfrom, vRecv, chainparams, connman);
+        ProcessGetCFCheckPt(*pfrom, vRecv, chainparams, *connman);
         return true;
     }
 
diff --git a/src/protocol.cpp b/src/protocol.cpp
index 93e76f1f13..83a24b9d95 100644
--- a/src/protocol.cpp
+++ b/src/protocol.cpp
@@ -40,6 +40,8 @@ const char *SENDCMPCT="sendcmpct";
 const char *CMPCTBLOCK="cmpctblock";
 const char *GETBLOCKTXN="getblocktxn";
 const char *BLOCKTXN="blocktxn";
+const char *GETCFILTERS="getcfilters";
+const char *CFILTER="cfilter";
 const char *GETCFHEADERS="getcfheaders";
 const char *CFHEADERS="cfheaders";
 const char *GETCFCHECKPT="getcfcheckpt";
@@ -75,6 +77,8 @@ const static std::string allNetMessageTypes[] = {
     NetMsgType::CMPCTBLOCK,
     NetMsgType::GETBLOCKTXN,
     NetMsgType::BLOCKTXN,
+    NetMsgType::GETCFILTERS,
+    NetMsgType::CFILTER,
     NetMsgType::GETCFHEADERS,
     NetMsgType::CFHEADERS,
     NetMsgType::GETCFCHECKPT,
diff --git a/src/protocol.h b/src/protocol.h
index b720a6ce91..985f44640b 100644
--- a/src/protocol.h
+++ b/src/protocol.h
@@ -226,6 +226,17 @@ extern const char* GETBLOCKTXN;
  */
 extern const char* BLOCKTXN;
 /**
+ * getcfilters requests compact filters for a range of blocks.
+ * Only available with service bit NODE_COMPACT_FILTERS as described by
+ * BIP 157 & 158.
+ */
+extern const char* GETCFILTERS;
+/**
+ * cfilter is a response to a getcfilters request containing a single compact
+ * filter.
+ */
+extern const char* CFILTER;
+/**
  * getcfheaders requests a compact filter header and the filter hashes for a
  * range of blocks, which can then be used to reconstruct the filter headers
  * for those blocks.
diff --git a/test/functional/p2p_blockfilters.py b/test/functional/p2p_blockfilters.py
index 9ff76b4b3d..6d947ac660 100755
--- a/test/functional/p2p_blockfilters.py
+++ b/test/functional/p2p_blockfilters.py
@@ -13,6 +13,7 @@ from test_framework.messages import (
     hash256,
     msg_getcfcheckpt,
     msg_getcfheaders,
+    msg_getcfilters,
     ser_uint256,
     uint256_from_str,
 )
@@ -25,6 +26,21 @@ from test_framework.util import (
     wait_until,
 )
 
+class CFiltersClient(P2PInterface):
+    def __init__(self):
+        super().__init__()
+        # Store the cfilters received.
+        self.cfilters = []
+
+    def pop_cfilters(self):
+        cfilters = self.cfilters
+        self.cfilters = []
+        return cfilters
+
+    def on_cfilter(self, message):
+        """Store cfilters received in a list."""
+        self.cfilters.append(message)
+
 class CompactFiltersTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
@@ -37,8 +53,8 @@ class CompactFiltersTest(BitcoinTestFramework):
 
     def run_test(self):
         # Node 0 supports COMPACT_FILTERS, node 1 does not.
-        node0 = self.nodes[0].add_p2p_connection(P2PInterface())
-        node1 = self.nodes[1].add_p2p_connection(P2PInterface())
+        node0 = self.nodes[0].add_p2p_connection(CFiltersClient())
+        node1 = self.nodes[1].add_p2p_connection(CFiltersClient())
 
         # Nodes 0 & 1 share the same first 999 blocks in the chain.
         self.nodes[0].generate(999)
@@ -112,7 +128,8 @@ class CompactFiltersTest(BitcoinTestFramework):
         )
         node0.send_and_ping(request)
         response = node0.last_message['cfheaders']
-        assert_equal(len(response.hashes), 1000)
+        main_cfhashes = response.hashes
+        assert_equal(len(main_cfhashes), 1000)
         assert_equal(
             compute_last_header(response.prev_header, response.hashes),
             int(main_cfcheckpt, 16)
@@ -126,12 +143,50 @@ class CompactFiltersTest(BitcoinTestFramework):
         )
         node0.send_and_ping(request)
         response = node0.last_message['cfheaders']
-        assert_equal(len(response.hashes), 1000)
+        stale_cfhashes = response.hashes
+        assert_equal(len(stale_cfhashes), 1000)
         assert_equal(
             compute_last_header(response.prev_header, response.hashes),
             int(stale_cfcheckpt, 16)
         )
 
+        self.log.info("Check that peers can fetch cfilters.")
+        stop_hash = self.nodes[0].getblockhash(10)
+        request = msg_getcfilters(
+            filter_type=FILTER_TYPE_BASIC,
+            start_height=1,
+            stop_hash=int(stop_hash, 16)
+        )
+        node0.send_message(request)
+        node0.sync_with_ping()
+        response = node0.pop_cfilters()
+        assert_equal(len(response), 10)
+
+        self.log.info("Check that cfilter responses are correct.")
+        for cfilter, cfhash, height in zip(response, main_cfhashes, range(1, 11)):
+            block_hash = self.nodes[0].getblockhash(height)
+            assert_equal(cfilter.filter_type, FILTER_TYPE_BASIC)
+            assert_equal(cfilter.block_hash, int(block_hash, 16))
+            computed_cfhash = uint256_from_str(hash256(cfilter.filter_data))
+            assert_equal(computed_cfhash, cfhash)
+
+        self.log.info("Check that peers can fetch cfilters for stale blocks.")
+        request = msg_getcfilters(
+            filter_type=FILTER_TYPE_BASIC,
+            start_height=1000,
+            stop_hash=int(stale_block_hash, 16)
+        )
+        node0.send_message(request)
+        node0.sync_with_ping()
+        response = node0.pop_cfilters()
+        assert_equal(len(response), 1)
+
+        cfilter = response[0]
+        assert_equal(cfilter.filter_type, FILTER_TYPE_BASIC)
+        assert_equal(cfilter.block_hash, int(stale_block_hash, 16))
+        computed_cfhash = uint256_from_str(hash256(cfilter.filter_data))
+        assert_equal(computed_cfhash, stale_cfhashes[999])
+
         self.log.info("Requests to node 1 without NODE_COMPACT_FILTERS results in disconnection.")
         requests = [
             msg_getcfcheckpt(
@@ -143,6 +198,11 @@ class CompactFiltersTest(BitcoinTestFramework):
                 start_height=1000,
                 stop_hash=int(main_block_hash, 16)
             ),
+            msg_getcfilters(
+                filter_type=FILTER_TYPE_BASIC,
+                start_height=1000,
+                stop_hash=int(main_block_hash, 16)
+            ),
         ]
         for request in requests:
             node1 = self.nodes[1].add_p2p_connection(P2PInterface())
@@ -151,6 +211,12 @@ class CompactFiltersTest(BitcoinTestFramework):
 
         self.log.info("Check that invalid requests result in disconnection.")
         requests = [
+            # Requesting too many filters results in disconnection.
+            msg_getcfilters(
+                filter_type=FILTER_TYPE_BASIC,
+                start_height=0,
+                stop_hash=int(main_block_hash, 16)
+            ),
             # Requesting too many filter headers results in disconnection.
             msg_getcfheaders(
                 filter_type=FILTER_TYPE_BASIC,
diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
index d178e79541..4d1dd4422e 100755
--- a/test/functional/test_framework/messages.py
+++ b/test/functional/test_framework/messages.py
@@ -1516,6 +1516,57 @@ class msg_no_witness_blocktxn(msg_blocktxn):
     def serialize(self):
         return self.block_transactions.serialize(with_witness=False)
 
+
+class msg_getcfilters:
+    __slots__ = ("filter_type", "start_height", "stop_hash")
+    msgtype =  b"getcfilters"
+
+    def __init__(self, filter_type, start_height, stop_hash):
+        self.filter_type = filter_type
+        self.start_height = start_height
+        self.stop_hash = stop_hash
+
+    def deserialize(self, f):
+        self.filter_type = struct.unpack("<B", f.read(1))[0]
+        self.start_height = struct.unpack("<I", f.read(4))[0]
+        self.stop_hash = deser_uint256(f)
+
+    def serialize(self):
+        r = b""
+        r += struct.pack("<B", self.filter_type)
+        r += struct.pack("<I", self.start_height)
+        r += ser_uint256(self.stop_hash)
+        return r
+
+    def __repr__(self):
+        return "msg_getcfilters(filter_type={:#x}, start_height={}, stop_hash={:x})".format(
+            self.filter_type, self.start_height, self.stop_hash)
+
+class msg_cfilter:
+    __slots__ = ("filter_type", "block_hash", "filter_data")
+    msgtype =  b"cfilter"
+
+    def __init__(self, filter_type=None, block_hash=None, filter_data=None):
+        self.filter_type = filter_type
+        self.block_hash = block_hash
+        self.filter_data = filter_data
+
+    def deserialize(self, f):
+        self.filter_type = struct.unpack("<B", f.read(1))[0]
+        self.block_hash = deser_uint256(f)
+        self.filter_data = deser_string(f)
+
+    def serialize(self):
+        r = b""
+        r += struct.pack("<B", self.filter_type)
+        r += ser_uint256(self.block_hash)
+        r += ser_string(self.filter_data)
+        return r
+
+    def __repr__(self):
+        return "msg_cfilter(filter_type={:#x}, block_hash={:x})".format(
+            self.filter_type, self.block_hash)
+
 class msg_getcfheaders:
     __slots__ = ("filter_type", "start_height", "stop_hash")
     msgtype =  b"getcfheaders"
diff --git a/test/functional/test_framework/mininode.py b/test/functional/test_framework/mininode.py
index 337939909e..45063aaff2 100755
--- a/test/functional/test_framework/mininode.py
+++ b/test/functional/test_framework/mininode.py
@@ -31,8 +31,9 @@ from test_framework.messages import (
     msg_block,
     MSG_BLOCK,
     msg_blocktxn,
-    msg_cfheaders,
     msg_cfcheckpt,
+    msg_cfheaders,
+    msg_cfilter,
     msg_cmpctblock,
     msg_feefilter,
     msg_filteradd,
@@ -69,8 +70,9 @@ MESSAGEMAP = {
     b"addr": msg_addr,
     b"block": msg_block,
     b"blocktxn": msg_blocktxn,
-    b"cfheaders": msg_cfheaders,
     b"cfcheckpt": msg_cfcheckpt,
+    b"cfheaders": msg_cfheaders,
+    b"cfilter": msg_cfilter,
     b"cmpctblock": msg_cmpctblock,
     b"feefilter": msg_feefilter,
     b"filteradd": msg_filteradd,
@@ -332,8 +334,9 @@ class P2PInterface(P2PConnection):
     def on_addr(self, message): pass
     def on_block(self, message): pass
     def on_blocktxn(self, message): pass
-    def on_cfheaders(self, message): pass
     def on_cfcheckpt(self, message): pass
+    def on_cfheaders(self, message): pass
+    def on_cfilter(self, message): pass
     def on_cmpctblock(self, message): pass
     def on_feefilter(self, message): pass
     def on_filteradd(self, message): pass