From b3fbc94d4f2937bb682f2766cc9a8d4fde328a3f Mon Sep 17 00:00:00 2001 From: John Newbery Date: Sun, 31 May 2020 22:58:42 -0400 Subject: [PATCH 1/3] Apply cfilters review fixups --- src/net_processing.cpp | 51 ++++++++++++++--------------- test/functional/p2p_blockfilters.py | 2 +- 2 files changed, 26 insertions(+), 27 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 404b33a9776f8..321e54b3dc28f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1991,7 +1991,7 @@ void static ProcessOrphanTx(CConnman* connman, CTxMemPool& mempool, std::set(filter_type)); - pfrom.fDisconnect = true; + peer.GetId(), static_cast(filter_type)); + peer.fDisconnect = true; return false; } @@ -2024,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; + peer.GetId(), stop_hash.ToString()); + peer.fDisconnect = true; return false; } } @@ -2034,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; + 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; } @@ -2059,12 +2059,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; @@ -2077,13 +2077,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 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()); @@ -2091,9 +2090,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)); } } @@ -2102,12 +2101,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; @@ -2120,7 +2119,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; } @@ -2143,13 +2142,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)); } /** @@ -2157,12 +2156,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; @@ -2174,7 +2173,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::max(), stop_index, filter_index)) { return; @@ -2195,12 +2194,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)); } 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& interruptMsgProc) diff --git a/test/functional/p2p_blockfilters.py b/test/functional/p2p_blockfilters.py index 6d947ac660edf..6d26da38411ad 100755 --- a/test/functional/p2p_blockfilters.py +++ b/test/functional/p2p_blockfilters.py @@ -5,7 +5,7 @@ """Tests NODE_COMPACT_FILTERS (BIP 157/158). Tests that a node configured with -blockfilterindex and -peerblockfilters can serve -cfheaders and cfcheckpts. +cfilters, cfheaders and cfcheckpts. """ from test_framework.messages import ( From 132b30d9c84f2a8053714a438f227b583a89a9ea Mon Sep 17 00:00:00 2001 From: Jim Posen Date: Mon, 4 May 2020 11:13:13 -0400 Subject: [PATCH 2/3] [net] Signal NODE_COMPACT_FILTERS if we're serving compact filters. 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. --- src/init.cpp | 12 +++++++----- src/net_processing.cpp | 2 +- src/protocol.cpp | 1 + src/protocol.h | 3 +++ 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 37e625129550f..24ddaac066b40 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -996,11 +996,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 321e54b3dc28f..587b655d1c7e3 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2009,7 +2009,7 @@ static bool PrepareBlockFilterRequest(CNode& peer, const CChainParams& chain_par { 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", peer.GetId(), static_cast(filter_type)); diff --git a/src/protocol.cpp b/src/protocol.cpp index 83a24b9d95d5b..7f58125f00e45 100644 --- a/src/protocol.cpp +++ b/src/protocol.cpp @@ -213,6 +213,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 985f44640bdec..a68f30287d7f7 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -285,6 +285,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. From f5c003d3ead182335252558c5c6c9b9ca8968065 Mon Sep 17 00:00:00 2001 From: Jim Posen Date: Mon, 4 May 2020 14:29:00 -0400 Subject: [PATCH 3/3] [test] Add test for NODE_COMPACT_FILTER. Test that a node configured to serve compact filters will signal NODE_COMPACT_FILTER service bit. --- test/functional/p2p_blockfilters.py | 13 +++++++++++-- test/functional/test_framework/messages.py | 1 + 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/test/functional/p2p_blockfilters.py b/test/functional/p2p_blockfilters.py index 6d26da38411ad..a9e86bd2fc08e 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 -cfilters, 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 @@ def run_test(self): 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 4d1dd4422e599..9a6fa66d549b6 100755 --- a/test/functional/test_framework/messages.py +++ b/test/functional/test_framework/messages.py @@ -49,6 +49,7 @@ 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