Merge #19070: p2p: Signal support for compact block filters with NODE_COMPACT_FILTERS

f5c003d3ead182335252558c5c6c9b9ca8968065 [test] Add test for NODE_COMPACT_FILTER. (Jim Posen)
132b30d9c84f2a8053714a438f227b583a89a9ea [net] Signal NODE_COMPACT_FILTERS if we're serving compact filters. (Jim Posen)
b3fbc94d4f2937bb682f2766cc9a8d4fde328a3f Apply cfilters review fixups (John Newbery)

Pull request description:

  If -peerblockfilters is configured, signal the `NODE_COMPACT_FILTERS` service bit to indicate that we are able to serve compact block filters, headers and checkpoints.

ACKs for top commit:
  MarcoFalke:
    re-review and Concept ACK f5c003d3ead182335252558c5c6c9b9ca8968065
  fjahr:
    Code review ACK f5c003d3ead182335252558c5c6c9b9ca8968065
  clarkmoody:
    Concept ACK f5c003d3ead182335252558c5c6c9b9ca8968065
  ariard:
    Concept and Code Review ACK f5c003d
  jonatack:
    ACK f5c003d3e

Tree-SHA512: 34d1c153530a0e55d09046fe548c9dc37344b5d6d50e00af1b4e1de1e7b49de770fca8471346a17c151de9fe164776296bb3dd5af331977f0c3ef1e6fc906f85

6 files changed, 49 insertions, 34 deletions

diff --git a/src/init.cpp b/src/init.cpp
index 966462a93f..f5aef08211 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1000,11 +1000,13 @@ bool AppInitParameterInteraction()
         }
     }
 
-    // Basic filters are the only supported filters. The basic filters index must be enabled
-    // to serve compact filters
-    if (gArgs.GetBoolArg("-peerblockfilters", DEFAULT_PEERBLOCKFILTERS) &&
-        g_enabled_filter_types.count(BlockFilterType::BASIC) != 1) {
-        return InitError(_("Cannot set -peerblockfilters without -blockfilterindex."));
+    // Signal NODE_COMPACT_FILTERS if peerblockfilters and basic filters index are both enabled.
+    if (gArgs.GetBoolArg("-peerblockfilters", DEFAULT_PEERBLOCKFILTERS)) {
+        if (g_enabled_filter_types.count(BlockFilterType::BASIC) != 1) {
+            return InitError(_("Cannot set -peerblockfilters without -blockfilterindex."));
+        }
+
+        nLocalServices = ServiceFlags(nLocalServices | NODE_COMPACT_FILTERS);
     }
 
     // if using block pruning, then disallow txindex
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 3e06f1fff0..7b83583e41 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2091,7 +2091,7 @@ void static ProcessOrphanTx(CConnman& connman, CTxMemPool& mempool, std::set<uin
  *
  * 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]   peer            The peer that we received the request from
  * @param[in]   chain_params    Chain parameters
  * @param[in]   filter_type     The filter type the request is for. Must be basic filters.
  * @param[in]   start_height    The start height for the request
@@ -2101,7 +2101,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& peer, const CChainParams& chain_params,
                                       BlockFilterType filter_type, uint32_t start_height,
                                       const uint256& stop_hash, uint32_t max_height_diff,
                                       const CBlockIndex*& stop_index,
@@ -2109,11 +2109,11 @@ static bool PrepareBlockFilterRequest(CNode& pfrom, const CChainParams& chain_pa
 {
     const bool supported_filter_type =
         (filter_type == BlockFilterType::BASIC &&
-         gArgs.GetBoolArg("-peerblockfilters", DEFAULT_PEERBLOCKFILTERS));
+         (peer.GetLocalServices() & NODE_COMPACT_FILTERS));
     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;
+                 peer.GetId(), static_cast<uint8_t>(filter_type));
+        peer.fDisconnect = true;
         return false;
     }
 
@@ -2124,8 +2124,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;
+                     peer.GetId(), stop_hash.ToString());
+            peer.fDisconnect = true;
             return false;
         }
     }
@@ -2134,14 +2134,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;
+                 peer.GetId(), start_height, stop_height);
+        peer.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;
+                 peer.GetId(), stop_height - start_height + 1, max_height_diff);
+        peer.fDisconnect = true;
         return false;
     }
 
@@ -2159,12 +2159,12 @@ static bool PrepareBlockFilterRequest(CNode& pfrom, const CChainParams& chain_pa
  *
  * 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]   peer            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,
+static void ProcessGetCFilters(CNode& peer, CDataStream& vRecv, const CChainParams& chain_params,
                                CConnman& connman)
 {
     uint8_t filter_type_ser;
@@ -2177,13 +2177,12 @@ static void ProcessGetCFilters(CNode& pfrom, CDataStream& vRecv, const CChainPar
 
     const CBlockIndex* stop_index;
     BlockFilterIndex* filter_index;
-    if (!PrepareBlockFilterRequest(pfrom, chain_params, filter_type, start_height, stop_hash,
+    if (!PrepareBlockFilterRequest(peer, 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());
@@ -2191,9 +2190,9 @@ static void ProcessGetCFilters(CNode& pfrom, CDataStream& vRecv, const CChainPar
     }
 
     for (const auto& filter : filters) {
-        CSerializedNetMsg msg = CNetMsgMaker(pfrom.GetSendVersion())
+        CSerializedNetMsg msg = CNetMsgMaker(peer.GetSendVersion())
             .Make(NetMsgType::CFILTER, filter);
-        connman.PushMessage(&pfrom, std::move(msg));
+        connman.PushMessage(&peer, std::move(msg));
     }
 }
 
@@ -2202,12 +2201,12 @@ static void ProcessGetCFilters(CNode& pfrom, CDataStream& vRecv, const CChainPar
  *
  * 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]   peer            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 ProcessGetCFHeaders(CNode& pfrom, CDataStream& vRecv, const CChainParams& chain_params,
+static void ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv, const CChainParams& chain_params,
                                 CConnman& connman)
 {
     uint8_t filter_type_ser;
@@ -2220,7 +2219,7 @@ static void ProcessGetCFHeaders(CNode& pfrom, CDataStream& vRecv, const CChainPa
 
     const CBlockIndex* stop_index;
     BlockFilterIndex* filter_index;
-    if (!PrepareBlockFilterRequest(pfrom, chain_params, filter_type, start_height, stop_hash,
+    if (!PrepareBlockFilterRequest(peer, chain_params, filter_type, start_height, stop_hash,
                                    MAX_GETCFHEADERS_SIZE, stop_index, filter_index)) {
         return;
     }
@@ -2243,13 +2242,13 @@ static void ProcessGetCFHeaders(CNode& pfrom, CDataStream& vRecv, const CChainPa
         return;
     }
 
-    CSerializedNetMsg msg = CNetMsgMaker(pfrom.GetSendVersion())
+    CSerializedNetMsg msg = CNetMsgMaker(peer.GetSendVersion())
         .Make(NetMsgType::CFHEADERS,
               filter_type_ser,
               stop_index->GetBlockHash(),
               prev_header,
               filter_hashes);
-    connman.PushMessage(&pfrom, std::move(msg));
+    connman.PushMessage(&peer, std::move(msg));
 }
 
 /**
@@ -2257,12 +2256,12 @@ static void ProcessGetCFHeaders(CNode& pfrom, CDataStream& vRecv, const CChainPa
  *
  * 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]   peer            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 ProcessGetCFCheckPt(CNode& pfrom, CDataStream& vRecv, const CChainParams& chain_params,
+static void ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv, const CChainParams& chain_params,
                                 CConnman& connman)
 {
     uint8_t filter_type_ser;
@@ -2274,7 +2273,7 @@ static void ProcessGetCFCheckPt(CNode& pfrom, CDataStream& vRecv, const CChainPa
 
     const CBlockIndex* stop_index;
     BlockFilterIndex* filter_index;
-    if (!PrepareBlockFilterRequest(pfrom, chain_params, filter_type, /*start_height=*/0, stop_hash,
+    if (!PrepareBlockFilterRequest(peer, chain_params, filter_type, /*start_height=*/0, stop_hash,
                                    /*max_height_diff=*/std::numeric_limits<uint32_t>::max(),
                                    stop_index, filter_index)) {
         return;
@@ -2295,12 +2294,12 @@ static void ProcessGetCFCheckPt(CNode& pfrom, CDataStream& vRecv, const CChainPa
         }
     }
 
-    CSerializedNetMsg msg = CNetMsgMaker(pfrom.GetSendVersion())
+    CSerializedNetMsg msg = CNetMsgMaker(peer.GetSendVersion())
         .Make(NetMsgType::CFCHECKPT,
               filter_type_ser,
               stop_index->GetBlockHash(),
               headers);
-    connman.PushMessage(&pfrom, std::move(msg));
+    connman.PushMessage(&peer, std::move(msg));
 }
 
 void ProcessMessage(
diff --git a/src/protocol.cpp b/src/protocol.cpp
index 5a91acee0f..c989aa3902 100644
--- a/src/protocol.cpp
+++ b/src/protocol.cpp
@@ -217,6 +217,7 @@ static std::string serviceFlagToStr(size_t bit)
     case NODE_GETUTXO:         return "GETUTXO";
     case NODE_BLOOM:           return "BLOOM";
     case NODE_WITNESS:         return "WITNESS";
+    case NODE_COMPACT_FILTERS: return "COMPACT_FILTERS";
     case NODE_NETWORK_LIMITED: return "NETWORK_LIMITED";
     // Not using default, so we get warned when a case is missing
     }
diff --git a/src/protocol.h b/src/protocol.h
index 1d0adaae6e..0bef12ee62 100644
--- a/src/protocol.h
+++ b/src/protocol.h
@@ -272,6 +272,9 @@ enum ServiceFlags : uint64_t {
     // NODE_WITNESS indicates that a node can be asked for blocks and transactions including
     // witness data.
     NODE_WITNESS = (1 << 3),
+    // NODE_COMPACT_FILTERS means the node will service basic block filter requests.
+    // See BIP157 and BIP158 for details on how this is implemented.
+    NODE_COMPACT_FILTERS = (1 << 6),
     // NODE_NETWORK_LIMITED means the same as NODE_NETWORK with the limitation of only
     // serving the last 288 (2 day) blocks
     // See BIP159 for details on how this is implemented.
diff --git a/test/functional/p2p_blockfilters.py b/test/functional/p2p_blockfilters.py
index 6d947ac660..a9e86bd2fc 100755
--- a/test/functional/p2p_blockfilters.py
+++ b/test/functional/p2p_blockfilters.py
@@ -4,12 +4,13 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Tests NODE_COMPACT_FILTERS (BIP 157/158).
 
-Tests that a node configured with -blockfilterindex and -peerblockfilters can serve
-cfheaders and cfcheckpts.
+Tests that a node configured with -blockfilterindex and -peerblockfilters signals
+NODE_COMPACT_FILTERS and can serve cfilters, cfheaders and cfcheckpts.
 """
 
 from test_framework.messages import (
     FILTER_TYPE_BASIC,
+    NODE_COMPACT_FILTERS,
     hash256,
     msg_getcfcheckpt,
     msg_getcfheaders,
@@ -70,6 +71,14 @@ class CompactFiltersTest(BitcoinTestFramework):
         self.nodes[1].generate(1001)
         wait_until(lambda: self.nodes[1].getblockcount() == 2000)
 
+        # Check that nodes have signalled NODE_COMPACT_FILTERS correctly.
+        assert node0.nServices & NODE_COMPACT_FILTERS != 0
+        assert node1.nServices & NODE_COMPACT_FILTERS == 0
+
+        # Check that the localservices is as expected.
+        assert int(self.nodes[0].getnetworkinfo()['localservices'], 16) & NODE_COMPACT_FILTERS != 0
+        assert int(self.nodes[1].getnetworkinfo()['localservices'], 16) & NODE_COMPACT_FILTERS == 0
+
         self.log.info("get cfcheckpt on chain to be re-orged out.")
         request = msg_getcfcheckpt(
             filter_type=FILTER_TYPE_BASIC,
diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
index da956a94de..5207b563a1 100755
--- a/test/functional/test_framework/messages.py
+++ b/test/functional/test_framework/messages.py
@@ -53,6 +53,7 @@ NODE_NETWORK = (1 << 0)
 NODE_GETUTXO = (1 << 1)
 NODE_BLOOM = (1 << 2)
 NODE_WITNESS = (1 << 3)
+NODE_COMPACT_FILTERS = (1 << 6)
 NODE_NETWORK_LIMITED = (1 << 10)
 
 MSG_TX = 1