net processing: Add support for getcfheaders #19010

jnewbery · 2020-05-18T18:58:20Z

Support getcfheaders requests when -peerblockfilters is set.

Does not advertise compact filter support in version messages.

jnewbery · 2020-05-18T18:59:40Z

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

maflcko

Just a style question, feel free to ignore.

DrahtBot · 2020-05-18T23:57:00Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Use shared pointers only in validation interface #18354 (Protect wallet by using shared pointers by bvbfan)

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.

maflcko · 2020-05-18T23:58:18Z

src/net_processing.cpp

Suggested change

CConnman* connman)

CConnman& connman)

Any reason why this is a pointer, but missing the nullptr check?

Either this should be a proper pointer with nullptr check, or

it should be a reference

Mixing the two combines the worst of each option.

(Same for pfrom)

This usage is the same as all the other Process*() functions in net_processing. Rather than introduce a new style for just this function, I think it makes sense to change it everywhere in net_processing in a refactor PR.

Generally new code should be style-conformant. See for example RelayTransaction, which properly passes connman. Obviously a non-blocking nit, so feel free to close this conversation.

jkczyz

Concept ACK

For testing, what's the general approach been for testing bad requests? BIP 157 says to not respond, but we are disconnecting the peer in these cases.

jkczyz · 2020-05-21T15:43:05Z

src/net_processing.cpp

Given these checks aren't necessary for ProcessGetCFCheckPt, would they be more suitable in separate function? Then the caller wouldn't have to know which values to pass to make the checks succeed.

Downside is you might fetch the BlockFilterIndex even though these checks may later fail.

I think this makes more sense if you also look at the full changes in #18876, where this function is common logic for getcfheaders and getcfilters, and the max number of headers that can be requested is 2000, and the max number of filters that can be requested in 1000.

I agree that the getcfcheckpt logic passing in 0 and std::numeric_limits<uint32_t>::max() is a bit ugly. I could change it to pass in optionals, but that seems more aesthetic than anything.

Another option would be to simply use default parameters, i.e. if start_height and max_height diff are not passed, the whole range is allowed. That would need to change the order of the parameters though.

test/functional/p2p_blockfilters.py

theStack · 2020-05-21T16:51:23Z

Concept ACK

jnewbery · 2020-05-21T19:25:45Z

Force pushed the branch to:

rebase on master now that indexes: Add compact block filter headers cache #18960 is merged
Added a comment to m_headers_cache that didn't make it into indexes: Add compact block filter headers cache #18960 (indexes: Add compact block filter headers cache #18960 (comment))
Addressed @jkczyz's comment on the functional test (net processing: Add support for getcfheaders #19010 (comment))

fjahr

ACK 4376797

Reviewed the code, played with the tests, and ran them locally.

FWIW, I have added the tests I suggested in the review club, feel free to copy them from there: fjahr@36d6854

fjahr · 2020-05-21T22:12:22Z

src/net_processing.cpp

 * @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
 * @param[in]   stop_hash       The stop_hash for the request
+ * @param[in]   max_height_diff The maximum number of items permitted to request, as specified in BIP 157


nit: This threw me off initially (similar to what @jkczyz noted below). The BIP gives a specific number, why do we not just use the constant? I think this could help make it a little less confusing.

Suggested change

* @param[in] max_height_diff The maximum number of items permitted to request, as specified in BIP 157

* @param[in] max_height_diff The maximum number of headers or filters permitted to request, as specified in BIP 157

I agree with @fjahr's comment and suggested change.

I'll add this in #19070 if I need to retouch that branch

jnewbery · 2020-05-21T23:16:26Z

what's the general approach been for testing bad requests? BIP 157 says to not respond, but we are disconnecting the peer in these cases.

I don't have a strong opinion about whether to disconnect or drop bad requests. Generally I prefer simply dropping bad requests to prevent bugs becoming network partition possibilities, but in this case:

the message type is entirely new.
the peers sending us these requests are light clients, so the downside of disconnecting them is small.

I'm inclined to keep it as a disconnect since that's the only way we have to communicate to the client that there's a problem with the request, but I'm happy to change it if others disagree.

maflcko · 2020-05-22T10:37:26Z

the peers sending us these requests are light clients, so the downside of disconnecting them is small.

I agree here. Generally the nicest thing you can do to misbehaving light clients is to disconnect them as early as possible. This way the misbehaviour bug will be hopefully caught early in the design and testing process of the light client. If not, at least in production it will be another indicator that something is wrong and users can file bugs. If the connection was kept alive, and without replying it might just time out on the light client and they'd think we are the ones misbehaving.

I was about to suggest to put a small comment in the source code to document this, but I wasn't exactly sure where to put it.

maflcko

Some style nits that can be ignored

re-ACK 4376797, only change since my previous review #18876 (comment) is adding a spurious new line and a new commit with documentation 🐣

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK 4376797f6c, only change since my previous review https://github.com/bitcoin/bitcoin/pull/18876#issuecomment-627322015 is adding a spurious new line and a new commit with documentation 🐣
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjc3gv+MB5VIjvGWjT85TlCzHERqu35XeRFo+DTLBUhkp1NGk1Uymar3cF68jOW
0W0Keide4ZEBCc1sz7mnhsXBBL6XhijSgGS7fTdjw9Iab8944dW11IzDmMFcHWwc
cglhfriS8Rpa3FUeq+FPrNI7zEA4UyHrXcIiUUzXkXBRK0yooTrpFzGMlS6HnB8+
MmzIuUaFTEKnIJA3exIAls8225TzmnIuPrSHtpNi+91RVpTnV1ZMAo++jBbcPE/p
riNswV/Yc/zG06xiO2YgleTyfip8FwjydEMaekkQUSL62IucTFQY0rHqeEWvysFr
L69g2vdnVDUgVLxiOrzLIUdhLjYxsjvyOUdXgC36UxpUERi2TuXzmMu0OWFSkChs
soAvIWL0hFt4oV/PTgBpxJPKrFtWgifwVy3l8iyZ07eVsGr0Z+YfNDwY6DWCJsM/
d+RG0asZEtOjdkwZF/PAc9tTWPsP/fLXiHO3z7928dASyT5+pKdaKOCSCeKH1+6p
lg1V3U+3
=0qw1
-----END PGP SIGNATURE-----

Timestamp of file with hash 40b1dadd7d8694410c23fb4d6f045b454bee5f2bf365f49994dc7f713b58f816 -

src/net_processing.cpp

src/protocol.h

src/index/blockfilterindex.h

src/protocol.h

if -peerblockfilters is configured, handle requests for cfheaders.

jnewbery · 2020-05-22T16:30:09Z

Thanks for review @MarcoFalke and @theStack. I've addressed all of your review comments, and also removed some redundant wording from the comment for CFCHECKPOINT in protocol.h

maflcko

re-ACK 5308c97 , only change is doc related 🗂

Show signature and timestamp

Signature:

Timestamp of file with hash eed11e440452664f3996d83b9c085e9a4c0ab77e69ea219b1fe7181017bc09a1 -

src/protocol.h

theStack

ACK 5308c97 🚀

jkczyz · 2020-05-25T06:15:13Z

ACK 5308c97

5308c97 [test] Add test for cfheaders (Jim Posen) f6b58c1 [net processing] Message handling for getcfheaders. (Jim Posen) 3bdc7c2 [doc] Add comment for m_headers_cache (John Newbery) Pull request description: Support `getcfheaders` requests when `-peerblockfilters` is set. Does not advertise compact filter support in version messages. ACKs for top commit: jkczyz: ACK 5308c97 MarcoFalke: re-ACK 5308c97 , only change is doc related 🗂 theStack: ACK 5308c97 🚀 Tree-SHA512: 240fc654f6f634c191d9f7628b6c4801f87ed514a1dd55c7de5d454d4012d1c09509a2d5a246bc7da445cd920252b4cd56a493c060cdb207b04af4ffe53b95f7

jonatack

ACK 5308c97 with some optional style nits for follow-ups. @fjahr's tests at fjahr@36d6854 look worthwhile.

jonatack · 2020-06-02T08:10:40Z

test/functional/p2p_blockfilters.py

+        )
+        node0.send_and_ping(request)
+        response = node0.last_message['cfheaders']
+        assert_equal(len(response.hashes), 1000)


style nit: replace the multiple values of 1000 and of 16 in this file with well-named constants

The 16 is used to interpret a hex string into an int (RPC return values are strings), so we shouldn't have a constant for that. I tried replacing the 1000 with a constant, but I don't think it made things any clearer.

jonatack · 2020-06-02T08:11:58Z

test/functional/test_framework/mininode.py

    def on_addr(self, message): pass
    def on_block(self, message): pass
    def on_blocktxn(self, message): pass
+    def on_cfheaders(self, message): pass


nit: sort here and lines 34 and 72

This was fixed in the following PR.

jonatack · 2020-06-02T08:13:11Z

src/net_processing.cpp

 * @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
 * @param[in]   stop_hash       The stop_hash for the request
+ * @param[in]   max_height_diff The maximum number of items permitted to request, as specified in BIP 157


I agree with @fjahr's comment and suggested change.

…et_processing.{h,cpp} 8b3136b refactor: replace CNode pointers by references within net_processing.{h,cpp} (Sebastian Falbesoner) Pull request description: This PR is inspired by a [recent code review comment](#19010 (comment)) on a PR that introduced new functions to the net_processing module. The point of the discussion was basically that whenever we pass something not by value (in the concrete example it was about `CNode*` and `CConnman*`) we should either use * a pointer (```CType*```) with null pointer check or * a reference (```CType&```) To keep things simple, this PR for a first approach * only tackles `CNode*` pointers * only within the net_processing module, i.e. no changes that would need adaption in other modules * keeps the names of the variables as they are I'm aware that PRs like this are kind of a PITA to review, but I think the code quality would increase if we get rid of pointers without nullptr check -- bloating up the code by adding all the missing checks would be the worse alternative, in my opinion. Possible follow-up PRs, in case this is received well: * replace CNode pointers by references for net module * replace CConnman pointers by references for net_processing module * ... ACKs for top commit: MarcoFalke: ACK 8b3136b 🔻 practicalswift: ACK 8b3136b Tree-SHA512: 15b6a569ecdcb39341002b9f4e09b38ed4df077e3a3a50dfb1b72d98bdc9f9769c7c504f106456aa7748af8591af7bb836b72d46086df715ab116e4ac3224b3b

…ithin net_processing.{h,cpp} 8b3136b refactor: replace CNode pointers by references within net_processing.{h,cpp} (Sebastian Falbesoner) Pull request description: This PR is inspired by a [recent code review comment](bitcoin#19010 (comment)) on a PR that introduced new functions to the net_processing module. The point of the discussion was basically that whenever we pass something not by value (in the concrete example it was about `CNode*` and `CConnman*`) we should either use * a pointer (```CType*```) with null pointer check or * a reference (```CType&```) To keep things simple, this PR for a first approach * only tackles `CNode*` pointers * only within the net_processing module, i.e. no changes that would need adaption in other modules * keeps the names of the variables as they are I'm aware that PRs like this are kind of a PITA to review, but I think the code quality would increase if we get rid of pointers without nullptr check -- bloating up the code by adding all the missing checks would be the worse alternative, in my opinion. Possible follow-up PRs, in case this is received well: * replace CNode pointers by references for net module * replace CConnman pointers by references for net_processing module * ... ACKs for top commit: MarcoFalke: ACK 8b3136b 🔻 practicalswift: ACK 8b3136b Tree-SHA512: 15b6a569ecdcb39341002b9f4e09b38ed4df077e3a3a50dfb1b72d98bdc9f9769c7c504f106456aa7748af8591af7bb836b72d46086df715ab116e4ac3224b3b

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

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

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

maflcko reviewed May 18, 2020

View reviewed changes

This was referenced May 19, 2020

[WIP] Serve BIP 157 compact filters #18876

Closed

Use shared pointers only in validation interface #18354

Closed

jkczyz reviewed May 21, 2020

View reviewed changes

jnewbery force-pushed the 2020-05-cfheaders branch from befc63d to 4376797 Compare May 21, 2020 18:58

jnewbery mentioned this pull request May 21, 2020

net processing: Add support for getcfilters #19044

Merged

fjahr approved these changes May 21, 2020

View reviewed changes

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

maflcko approved these changes May 22, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/protocol.h Outdated Show resolved Hide resolved

theStack reviewed May 22, 2020

View reviewed changes

src/index/blockfilterindex.h Outdated Show resolved Hide resolved

src/protocol.h Outdated Show resolved Hide resolved

jnewbery and others added 3 commits May 22, 2020 11:59

[doc] Add comment for m_headers_cache

3bdc7c2

[net processing] Message handling for getcfheaders.

f6b58c1

if -peerblockfilters is configured, handle requests for cfheaders.

[test] Add test for cfheaders

5308c97

theStack mentioned this pull request May 22, 2020

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

Merged

jnewbery force-pushed the 2020-05-cfheaders branch from 4376797 to 5308c97 Compare May 22, 2020 16:20

maflcko reviewed May 23, 2020

View reviewed changes

src/protocol.h Show resolved Hide resolved

theStack approved these changes May 24, 2020

View reviewed changes

maflcko merged commit 7d32cce into bitcoin:master May 26, 2020

jnewbery deleted the 2020-05-cfheaders branch May 26, 2020 13:50

jonatack reviewed Jun 2, 2020

View reviewed changes

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

merge bitcoin#19010: Add support for getcfheaders

216036b

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

	* @param[in] max_height_diff The maximum number of items permitted to request, as specified in BIP 157
	* @param[in] max_height_diff The maximum number of headers or filters permitted to request, as specified in BIP 157

net processing: Add support for getcfheaders #19010

net processing: Add support for getcfheaders #19010

Uh oh!

Conversation

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

Uh oh!

Uh oh!

jnewbery commented May 18, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theStack commented May 21, 2020

Uh oh!

jnewbery commented May 21, 2020

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented May 21, 2020

Uh oh!

maflcko commented May 22, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery commented May 22, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

jkczyz commented May 25, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented May 18, 2020 •

edited

Loading

DrahtBot commented May 18, 2020 •

edited

Loading