net processing: Add support for getcfilters

jnewbery commented at 10:50 pm on May 21, 2020: member

Support getcfilters requests when -peerblockfilters is set.

Does not advertise compact filter support in version messages.

jnewbery commented at 10:51 pm on May 21, 2020: member

This is the 4th PR taken from #18876. Please see that PR for details of the full changes to support BIP 157.

DrahtBot added the label P2P on May 22, 2020

DrahtBot added the label Tests on May 22, 2020

DrahtBot added the label UTXO Db and Indexes on May 22, 2020

MarcoFalke removed the label Tests on May 22, 2020

MarcoFalke removed the label UTXO Db and Indexes on May 22, 2020

in src/net_processing.cpp:2051 in c528f3afb6 outdated

2046@@ -2045,6 +2047,49 @@ static bool PrepareBlockFilterRequest(CNode* pfrom, const CChainParams& chain_pa
2047     return true;
2048 }
2049 
2050+/**
2051+ * Handle a cffilters request.

theStack commented at 12:17 pm on May 22, 2020:

0 * Handle a cfilters request.

jnewbery commented at 4:36 pm on May 22, 2020:

Fixed. Thanks!

theStack commented at 12:17 pm on May 22, 2020: member

Concept ACK

jnewbery force-pushed on May 22, 2020

MarcoFalke added the label Needs rebase on May 26, 2020

DrahtBot removed the label Needs rebase on May 26, 2020

jnewbery force-pushed on May 26, 2020

jnewbery commented at 3:29 pm on May 26, 2020: member

rebased on master and fixed failing test.

in src/protocol.h:238 in 1231f6b3ba outdated

233+extern const char* GETCFILTERS;
234+/**
235+ * cfilter is a response to a getcfilters request containing a single compact
236+ * filter.
237+ */
238+extern const char *CFILTER;

MarcoFalke commented at 4:17 pm on May 26, 2020:

style-nit in commit ee8499d294

0extern const char* CFILTER;

jnewbery commented at 5:36 pm on May 26, 2020:

Fixed. Thanks!

MarcoFalke commented at 4:20 pm on May 26, 2020: member

ACK 1231f6b3ba 🔁

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 1231f6b3ba 🔁
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhYrQv+IxEs+9TDoo1Lq/ba4oUD2SnLQmHUQmfVTu/+MIjw/cwY9DoZIwg6/iGs
 86WwTmf8NwlNjAe9AHnrgDm1BhakIZYaXv+ZgCL7BRqpFujyh2jwjsDjMXWppMvjv
 9DSt1NsrRlinXQsjR7WCJXvNt6axnvCeRsqLeua+ZhIyF5kVwTPapaNVbNo91Wjgw
10fRmBwxhI39ZnZpjljKGOF9x7cB5y0ySCeDy2FQqRDpBEkdprUoSu5KloEjD9nNge
11t4dtogwO6rDARHPx9rZOekp5MnAAUS+hdUeUFV3Yiw08VVbTuEOCAeAsIB326fQm
12QORabhfya6K8guMuvCXyThmIi79GmAti93ybgwDREIWmsMJj88H7vHn8AM5adMR8
13Zu839huCzBaeEUk4SbD5CK7j/2E3cGMLQ5l63m+4AEkOCgj/nr5aXqZpIGuxKU4F
14ETGWpimT3U2VioNHBzoov/59uge5rPNoSpnKZukOaSYn00Uum1BFlgrpqDDjY1Pb
150D3QC8b5
16=uTse
17-----END PGP SIGNATURE-----

Timestamp of file with hash 9f5d9f1dfb8cd50d63725419784f2b0521db1d74441352040b1817c8b21ec56f -

jnewbery force-pushed on May 26, 2020

in src/net_processing.cpp:3519 in 90a0dfff4c outdated

3510@@ -3466,6 +3511,12 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec
3511         return true;
3512     }
3513 
3514+    if (msg_type == NetMsgType::GETCFILTERS) {
3515+        ProcessGetCFilters(pfrom, vRecv, chainparams, connman);
3516+        return true;
3517+    }
3518+
3519+

Empact commented at 8:42 pm on May 26, 2020:

nit: inconsistent whitespace

jnewbery commented at 9:38 pm on May 26, 2020:

Fixed.

in src/net_processing.cpp:2066 in 90a0dfff4c outdated

2061+ * @param[in]   pfrom           The peer that we received the request from
2062+ * @param[in]   vRecv           The raw message received
2063+ * @param[in]   chain_params    Chain parameters
2064+ * @param[in]   connman         Pointer to the connection manager
2065+ */
2066+static void ProcessGetCFilters(CNode* pfrom, CDataStream& vRecv, const CChainParams& chain_params,

Empact commented at 8:46 pm on May 26, 2020:

nit: since pfrom and connman are assumed, how about pass by reference?

jnewbery commented at 9:40 pm on May 26, 2020:

You’re the second reviewer to ask for this so I’ve now made the change (as well as to ProcessGetCFHeaders() and ProcessGetCFCheckPt).

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19053 (refactor: replace CNode pointers by references within net_processing.{h,cpp} by theStack)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/net_processing.cpp:2080 in 11106a4722 outdated

2075+    const BlockFilterType filter_type = static_cast<BlockFilterType>(filter_type_ser);
2076+
2077+    const CBlockIndex* stop_index;
2078+    BlockFilterIndex* filter_index;
2079+    if (!PrepareBlockFilterRequest(pfrom, chain_params, filter_type, start_height, stop_hash,
2080+                                   MAX_GETCFILTERS_SIZE, stop_index, filter_index)) {

jkczyz commented at 0:38 am on May 29, 2020:

Not sure if there is a mistake in BIP 157, but it reads that the difference must be strictly less than 1000 which implies not equal. Currently, PrepareBlockFilterRequest would allow 1000.

Likewise for headers.

jnewbery commented at 1:02 pm on May 29, 2020:

I think the BIP and code are correct/consistent. If the difference between the start height and stop height in the request is 999, then the number of filters served is 1000. For example, request(1, 1000) results in blocks 1-1000 (1000 blocks) being served.

if (stop_height - start_height >= max_height_diff) ensures the strictly-less-than.

jkczyz commented at 5:41 pm on May 29, 2020:

Right, the difference. 👍

in src/net_processing.cpp:2085 in 11106a4722 outdated

2080+                                   MAX_GETCFILTERS_SIZE, stop_index, filter_index)) {
2081+        return;
2082+    }
2083+
2084+    std::vector<BlockFilter> filters;
2085+

jkczyz commented at 0:54 am on May 29, 2020:

nit: Consider removing blank line for consistency with ProcessGetCFHeaders.

jnewbery commented at 1:03 pm on May 29, 2020:

I’ll do this if I need to retouch the branch.

in src/net_processing.cpp:2001 in bb911ae7f5 outdated

1997@@ -1998,7 +1998,7 @@ void static ProcessOrphanTx(CConnman* connman, CTxMemPool& mempool, std::set<uin
1998  * @param[out]  filter_index    The filter index, if the request can be serviced.
1999  * @return                      True if the request can be serviced.
2000  */
2001-static bool PrepareBlockFilterRequest(CNode* pfrom, const CChainParams& chain_params,
2002+static bool PrepareBlockFilterRequest(CNode& pfrom, const CChainParams& chain_params,

jkczyz commented at 1:07 am on May 29, 2020:

Does the “p” in pfrom stand for “pointer” or “peer”? Consider renaming if the former.

jnewbery commented at 1:05 pm on May 29, 2020:

I think you’re right that the p stands for pointer. I’ll rename these if I need to retouch the branch.

jkczyz commented at 1:11 am on May 29, 2020: contributor

ACK 9e36067d8cd02830c7e5a88a391dff6ac3adbe0c

MarcoFalke commented at 1:11 pm on May 29, 2020: member

re-ACK 9e36067d8c , only change is adding commit “[refactor] Pass CNode and CConnman by reference” 🥑

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 9e36067d8c , only change is adding commit "[refactor] Pass CNode and CConnman by reference" 🥑
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgbKwv+PwO//7eg2MRGhEr5p5hlfuT6GXbIG2KBBj2at35/XQLwuNfi/0S8Jv8a
 84oga+orbWYA4bNjZkEt2boyO3mCiML5pKPSHbjaDBaltE/lzzurIE0dfG/O+fPpk
 9+Smsj3HbC8BOXxkKdaqYXtQs5519LN9ypbCbIW6mY7SGyov1wG3XsBdNtMlU5Ybt
10G7KkpTsXpSYpXCUy+Q1Rh2+3s7fvYZQrpe3XZ3Z0Ztzyuct3XgMarj+ucJOEKZ6Y
11DKtXl0h4XBqfwlc4ngMZuXqN/6Zk1wEne94Q3Hr19SjbDLkp/oeBbsjqUi69y+5n
124CSmVaqhr8HUtTxqp/+ADSf+PaMchYx/jrOOh/f+2SsA0KLIsTvuGdWPxmDse0so
13LHxFrDmF5mHXt1hECV5zQQcWE5M/nS8WB6nGHEQQ0RO67nxPs6CaqIQmR1WZ1599
14/DAw2B1oagwy+KBQMAZFHjBFGKWa5TXXb7fZsVFSiUIXaMGl9p9xxfe2kJ1mzm5X
15BaTr8GqR
16=/Tog
17-----END PGP SIGNATURE-----

Timestamp of file with hash e987c18ac02e885972b23102e7c1c01949ebe68ab8a6bbda08846816f39e846a -

in test/functional/p2p_blockfilters.py:16 in 9e36067d8c

12@@ -13,6 +13,7 @@
13     hash256,
14     msg_getcfcheckpt,
15     msg_getcfheaders,
16+    msg_getcfilters,

fjahr commented at 9:28 pm on May 31, 2020:

nit-ish: the comment up top in p2p_blockfilters.py is still missing cfilters.

MarcoFalke commented at 10:17 pm on May 31, 2020:

Let’s fix this up in a follow-up

fjahr approved

fjahr commented at 9:46 pm on May 31, 2020: member

Code review ACK 9e36067d8cd02830c7e5a88a391dff6ac3adbe0c

If you have to retouch maybe also fix the commit message in 11106a4722558765a44ae45c7892724a73ce514c. It still says -peercfilter instead of -peerblockfilter.

MarcoFalke merged this on May 31, 2020

MarcoFalke closed this on May 31, 2020

jnewbery deleted the branch on Jun 1, 2020

sidhujag referenced this in commit f1b9021bb6 on Jun 1, 2020

jonatack commented at 9:02 am on June 2, 2020: member

ACK 9e36067d8cd02830c7e5a88a391dff6ac3adbe0c

Two optional nits for follow-ups:

 0--- a/src/protocol.cpp
 1+++ b/src/protocol.cpp
 2@@ -40,12 +40,12 @@ const char *SENDCMPCT="sendcmpct";
 3-const char *GETCFILTERS="getcfilters";
 4-const char *CFILTER="cfilter";
 5-const char *GETCFHEADERS="getcfheaders";
 6-const char *CFHEADERS="cfheaders";
 7 const char *GETCFCHECKPT="getcfcheckpt";
 8 const char *CFCHECKPT="cfcheckpt";
 9+const char *GETCFHEADERS="getcfheaders";
10+const char *CFHEADERS="cfheaders";
11+const char *GETCFILTERS="getcfilters";
12+const char *CFILTER="cfilter";
13
14--- a/test/functional/p2p_blockfilters.py
15+++ b/test/functional/p2p_blockfilters.py
16@@ -4,8 +4,8 @@
17 """Tests NODE_COMPACT_FILTERS (BIP 157/158).
18 
19-Tests that a node configured with -blockfilterindex and -peerblockfilters can serve
20-cfheaders and cfcheckpts.
21+Tests that a node configured with -blockfilterindex and -peerblockfilters can
22+serve cfcheckpt, cfheaders, and cfilters.
23 """

jnewbery commented at 2:32 pm on June 2, 2020: member

Thanks for the review @jonatack

the messages are defined in order [getcfilters, cfilter, getcfheaders, cfheaders, getcfcheckpt, cfcheckpt] in the BIP, so I think it makes sense to preserve that ordering in protocol.cpp.
test docstring is fixed in the next PR

theStack commented at 3:03 pm on June 2, 2020: member

post-merge Code Review ACK https://github.com/bitcoin/bitcoin/pull/19044/commits/9e36067d8cd02830c7e5a88a391dff6ac3adbe0c

jasonbcox referenced this in commit eb6fae5177 on Nov 19, 2020

kittywhiskers referenced this in commit 8e8396709b on Aug 22, 2021

kittywhiskers referenced this in commit eaf6847adb on Aug 22, 2021

kittywhiskers referenced this in commit f266a54381 on Sep 16, 2021

kittywhiskers referenced this in commit 88a81679a9 on Sep 18, 2021

kittywhiskers referenced this in commit 5474c85853 on Sep 19, 2021

UdjinM6 referenced this in commit 4ffd42de63 on Sep 21, 2021

thelazier referenced this in commit f956974e51 on Sep 25, 2021

kittywhiskers referenced this in commit e5538c7caf on Oct 12, 2021

DrahtBot locked this on Feb 15, 2022

net processing: Add support for getcfilters #19044

Conflicts