net processing: Add support for getcfilters by jnewbery · Pull Request #19044 · bitcoin/bitcoin

jnewbery · 2020-05-21T22:50:46Z

Support getcfilters requests when -peerblockfilters is set.

Does not advertise compact filter support in version messages.

jnewbery · 2020-05-21T22:51:07Z

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

theStack

Concept ACK

src/net_processing.cpp

jnewbery · 2020-05-26T15:29:57Z

rebased on master and fixed failing test.

maflcko

ACK 1231f6b3ba 🔁

Show signature and timestamp

Signature:

Timestamp of file with hash 9f5d9f1dfb8cd50d63725419784f2b0521db1d74441352040b1817c8b21ec56f -

src/protocol.h

Empact

Code Review ACK 90a0dff

nit: for 0139a1b, how about noting in the commit message where this was introduced?

src/net_processing.cpp

Pass CNode and CConnman by reference instead of by pointer to ProcessGetCFCheckPt() and ProcessGetCFHeaders().

This only changes network serialization. Disk serialization does not include the filter_type and is defined in ReadFilterFromDisk()/WriteFilterToDisk().

Handle getcfilters request if -peercfilter is configured.

jnewbery · 2020-05-26T21:42:27Z

Thanks for the review @MarcoFalke and @Empact. I've addressed all of your review comments.

nit: for 0139a1b, how about noting in the commit message where this was introduced?

The default [de]serialization wasn't previously used anywhere, so I don't think this is necessary. It's trivial to look up the git blame for that line.

Empact · 2020-05-26T23:33:43Z

re-Code Review ACK 9e36067

DrahtBot · 2020-05-27T20:12:02Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: replace CNode pointers by references within net_processing.{h,cpp} #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.

jkczyz

ACK 9e36067

src/net_processing.cpp

jkczyz · 2020-05-29T00:54:42Z

src/net_processing.cpp

+    }
+
+    std::vector<BlockFilter> filters;
+


nit: Consider removing blank line for consistency with ProcessGetCFHeaders.

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

jkczyz · 2020-05-29T01:07:03Z

src/net_processing.cpp

 * @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,


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

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

maflcko · 2020-05-29T13:11:15Z

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

Show signature and timestamp

Signature:

Timestamp of file with hash e987c18ac02e885972b23102e7c1c01949ebe68ab8a6bbda08846816f39e846a -

fjahr

Code review ACK 9e36067

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

fjahr · 2020-05-31T21:28:04Z

test/functional/p2p_blockfilters.py

    hash256,
    msg_getcfcheckpt,
    msg_getcfheaders,
+    msg_getcfilters,


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

Let's fix this up in a follow-up

9e36067 [test] Add test for cfilters. (Jim Posen) 11106a4 [net processing] Message handling for getcfilters. (Jim Posen) e535670 [indexes] Fix default [de]serialization of BlockFilter. (Jim Posen) bb911ae [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 bitcoin@9e36067 MarcoFalke: re-ACK 9e36067 , only change is adding commit "[refactor] Pass CNode and CConnman by reference" 🥑 jkczyz: ACK 9e36067 fjahr: Code review ACK 9e36067 Tree-SHA512: b45b42a25905ef0bd9e195029185300c86856c87f78cbe17921f4a25e159ae0f6f003e61714fa43779017eb97cd89d3568419be88e47d19dc8095562939e7887

jonatack

ACK 9e36067

Two optional nits for follow-ups:

--- a/src/protocol.cpp
+++ b/src/protocol.cpp
@@ -40,12 +40,12 @@ const char *SENDCMPCT="sendcmpct";
-const char *GETCFILTERS="getcfilters";
-const char *CFILTER="cfilter";
-const char *GETCFHEADERS="getcfheaders";
-const char *CFHEADERS="cfheaders";
 const char *GETCFCHECKPT="getcfcheckpt";
 const char *CFCHECKPT="cfcheckpt";
+const char *GETCFHEADERS="getcfheaders";
+const char *CFHEADERS="cfheaders";
+const char *GETCFILTERS="getcfilters";
+const char *CFILTER="cfilter";

--- a/test/functional/p2p_blockfilters.py
+++ b/test/functional/p2p_blockfilters.py
@@ -4,8 +4,8 @@
 """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 can
+serve cfcheckpt, cfheaders, and cfilters.
 """

jnewbery · 2020-06-02T14:32:43Z

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

post-merge Code Review ACK 9e36067

Summary: ``` Support getcfilters requests when -peerblockfilters is set. Does not advertise compact filter support in version messages. ``` Backport of core [[bitcoin/bitcoin#19044 | PR19044]]. Depends on D8465. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8469

merge bitcoin#16787, bitcoin#17474, bitcoin#17687, bitcoin#18165, bitcoin#18877, bitcoin#18960, bitcoin#19010, bitcoin#19044, bitcoin#19070: block filters

jnewbery mentioned this pull request May 21, 2020

[WIP] Serve BIP 157 compact filters #18876

Closed

DrahtBot added P2P Tests UTXO Db and Indexes labels May 22, 2020

maflcko removed Tests UTXO Db and Indexes labels May 22, 2020

theStack reviewed May 22, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

jnewbery force-pushed the 2020-05-cfilters branch from 6432b05 to c93c94a Compare May 22, 2020 16:36

DrahtBot mentioned this pull request May 22, 2020

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

Merged

maflcko added the Needs rebase label May 26, 2020

DrahtBot removed the Needs rebase label May 26, 2020

jnewbery force-pushed the 2020-05-cfilters branch from c93c94a to 1231f6b Compare May 26, 2020 15:29

maflcko reviewed May 26, 2020

View reviewed changes

src/protocol.h Outdated Show resolved Hide resolved

jnewbery mentioned this pull request May 26, 2020

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

Merged

jnewbery force-pushed the 2020-05-cfilters branch from 1231f6b to 90a0dff Compare May 26, 2020 17:37

Empact reviewed May 26, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

jnewbery and others added 4 commits May 26, 2020 17:24

[refactor] Pass CNode and CConnman by reference

bb911ae

Pass CNode and CConnman by reference instead of by pointer to ProcessGetCFCheckPt() and ProcessGetCFHeaders().

[indexes] Fix default [de]serialization of BlockFilter.

e535670

This only changes network serialization. Disk serialization does not include the filter_type and is defined in ReadFilterFromDisk()/WriteFilterToDisk().

[net processing] Message handling for getcfilters.

11106a4

Handle getcfilters request if -peercfilter is configured.

[test] Add test for cfilters.

9e36067

jnewbery force-pushed the 2020-05-cfilters branch from 90a0dff to 9e36067 Compare May 26, 2020 21:42

jkczyz reviewed May 29, 2020

View reviewed changes

fjahr approved these changes May 31, 2020

View reviewed changes

maflcko merged commit 07d0e0d into bitcoin:master May 31, 2020

jnewbery deleted the 2020-05-cfilters branch June 1, 2020 02:54

jonatack reviewed Jun 2, 2020

View reviewed changes

theStack reviewed Jun 2, 2020

View reviewed changes

kwvg added a commit to kwvg/dash that referenced this pull request Sep 19, 2021

merge bitcoin#19044: Add support for getcfilters

5474c85

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

jnewbery commented May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented May 21, 2020

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery commented May 26, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Empact left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery commented May 26, 2020

Uh oh!

Empact commented May 26, 2020

Uh oh!

DrahtBot commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkczyz May 29, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery May 29, 2020

Choose a reason for hiding this comment

Uh oh!

jkczyz May 29, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery May 29, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented May 29, 2020

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr May 31, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko May 31, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jun 2, 2020

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jnewbery commented May 21, 2020 •

edited

Loading

DrahtBot commented May 27, 2020 •

edited

Loading

jonatack left a comment •

edited

Loading