net processing: Add support for getcfheaders #19010

jnewbery commented at 6:58 PM on May 18, 2020: member

Support getcfheaders requests when -peerblockfilters is set.

Does not advertise compact filter support in version messages.

jnewbery commented at 6:59 PM on May 18, 2020: member

This is the 3rd 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 18, 2020

DrahtBot added the label Tests on May 18, 2020

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

MarcoFalke commented at 11:55 PM on May 18, 2020: member

Just a style question, feel free to ignore.

DrahtBot commented at 11:57 PM on May 18, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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.

in src/net_processing.cpp:2059 in 4b139e1832 outdated

2050 | + * @param[in]   vRecv           The raw message received
2051 | + * @param[in]   chain_params    Chain parameters
2052 | + * @param[in]   connman         Pointer to the connection manager
2053 | + */
2054 | +static void ProcessGetCFHeaders(CNode* pfrom, CDataStream& vRecv, const CChainParams& chain_params,
2055 | +                                CConnman* connman)

MarcoFalke commented at 11:58 PM on May 18, 2020:

                                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)

jnewbery commented at 6:29 PM on May 21, 2020:

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.

MarcoFalke commented at 10:44 AM on May 22, 2020:

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.

in src/net_processing.cpp:2038 in 4b139e1832 outdated

2029 | +        LogPrint(BCLog::NET, "peer %d requested too many cfilters/cfheaders: %d / %d\n",
2030 | +                 pfrom->GetId(), stop_height - start_height + 1, max_height_diff);
2031 | +        pfrom->fDisconnect = true;
2032 | +        return false;
2033 | +    }
2034 | +

jkczyz commented at 3:43 PM on May 21, 2020:

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.

jnewbery commented at 6:47 PM on May 21, 2020:

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.

theStack commented at 6:55 PM on May 21, 2020:

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.

in test/functional/p2p_blockfilters.py:119 in befc63d5b0 outdated

 113 | +        node0.send_and_ping(request)
 114 | +        response = node0.last_message['cfheaders']
 115 | +        assert_equal(
 116 | +            compute_last_header(response.prev_header, response.hashes),
 117 | +            int(main_cfcheckpt, 16)
 118 | +        )

jkczyz commented at 4:15 PM on May 21, 2020:

Is it worth checking how many hashes are returned? Or would you consider it checked indirectly by compute_last_header?

jnewbery commented at 7:26 PM on May 21, 2020:

Technically, it's covered indirectly, but I think it's a good idea to explicitly test this so failures are easier to identify. I've added the check.

jkczyz commented at 4:20 PM on May 21, 2020: contributor

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.

theStack commented at 4:51 PM on May 21, 2020: member

Concept ACK

jnewbery force-pushed on May 21, 2020

jnewbery commented at 7:25 PM on May 21, 2020: member

Force pushed the branch to:

rebase on master now that #18960 is merged
Added a comment to m_headers_cache that didn't make it into #18960 (https://github.com/bitcoin/bitcoin/pull/18960#discussion_r428395728)
Addressed @jkczyz's comment on the functional test (https://github.com/bitcoin/bitcoin/pull/19010#discussion_r428759826)

in src/net_processing.cpp:1990 in db20d456f0 outdated

1984 | @@ -1983,14 +1985,16 @@ void static ProcessOrphanTx(CConnman* connman, CTxMemPool& mempool, std::set<uin
1985 |   * @param[in]   pfrom           The peer that we received the request from
1986 |   * @param[in]   chain_params    Chain parameters
1987 |   * @param[in]   filter_type     The filter type the request is for. Must be basic filters.
1988 | + * @param[in]   start_height    The start height for the request
1989 |   * @param[in]   stop_hash       The stop_hash for the request
1990 | + * @param[in]   max_height_diff The maximum number of items permitted to request, as specified in BIP 157

fjahr commented at 10:12 PM on May 21, 2020:

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.

 * [@param](/bitcoin-bitcoin/contributor/param/)[in]   max_height_diff The maximum number of headers or filters permitted to request, as specified in BIP 157

jonatack commented at 8:13 AM on June 2, 2020:

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

jnewbery commented at 3:23 PM on June 2, 2020:

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

fjahr approved

fjahr commented at 11:09 PM on May 21, 2020: member

ACK 4376797f6c9962ec92b5ce6e61dc622b56d2a6f7

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: https://github.com/fjahr/bitcoin/commit/36d6854d2b4ed0afa556ccbe0ec72c4cb141a4c9

jnewbery commented at 11:16 PM on May 21, 2020: member

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.

MarcoFalke removed the label Tests on May 22, 2020

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

MarcoFalke commented at 10:37 AM on May 22, 2020: member

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.

in src/net_processing.cpp:2127 in 4376797f6c outdated

2121 | @@ -2048,7 +2122,9 @@ static void ProcessGetCFCheckPt(CNode* pfrom, CDataStream& vRecv, const CChainPa
2122 |  
2123 |      const CBlockIndex* stop_index;
2124 |      BlockFilterIndex* filter_index;
2125 | -    if (!PrepareBlockFilterRequest(pfrom, chain_params, filter_type, stop_hash,
2126 | +    if (!PrepareBlockFilterRequest(pfrom, chain_params, filter_type, /*start_height=*/0, stop_hash,
2127 | +                                   /*max_height_diff=*/std::numeric_limits<uint32_t>::max(),
2128 | +

MarcoFalke commented at 11:15 AM on May 22, 2020:

in commit db20d456f0 [net processing] Message handling for getcfheaders.

Why add an empty newline in the params?

jnewbery commented at 4:22 PM on May 22, 2020:

Because I have fat fingers. Now fixed.

in src/protocol.h:233 in 4376797f6c outdated

 224 | @@ -225,6 +225,17 @@ extern const char* GETBLOCKTXN;
 225 |   * @since protocol version 70014 as described by BIP 152
 226 |   */
 227 |  extern const char* BLOCKTXN;
 228 | +/**
 229 | + * getcfheaders requests compact filter headers for a range of blocks.
 230 | + * Only available with service bit NODE_COMPACT_FILTERS as described by
 231 | + * BIP 157 & 158.
 232 | + */
 233 | +extern const char *GETCFHEADERS;

MarcoFalke commented at 11:17 AM on May 22, 2020:

style-nit in commit db20d456f0 (according to clang-format):

extern const char* GETCFHEADERS;

jnewbery commented at 4:22 PM on May 22, 2020:

Whoops. Missed this out in the rebase. Fixed.

MarcoFalke approved

MarcoFalke commented at 11:19 AM on May 22, 2020: member

Some style nits that can be ignored

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

<details><summary>Show signature and timestamp</summary>

Signature:

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

re-ACK 4376797f6c, only change since my previous review [#18876 (comment)](/bitcoin-bitcoin/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 -

</details>

in src/index/blockfilterindex.h:42 in 616c8b3f6d outdated

  38 | @@ -39,6 +39,7 @@ class BlockFilterIndex final : public BaseIndex
  39 |      size_t WriteFilterToDisk(FlatFilePos& pos, const BlockFilter& filter);
  40 |  
  41 |      Mutex m_cs_headers_cache;
  42 | +    //* cache of block hash to filter header, to avoid disk access when responding to getcfcheckpt. */

theStack commented at 11:52 AM on May 22, 2020:

To match the other comments in the class, this one should also use the Doxygen starting delimiter (= C-Style comment with two '*'s, see http://www.doxygen.nl/manual/docblocks.html#cppblock):

    /** cache of block hash to filter header, to avoid disk access when responding to getcfcheckpt. */

jnewbery commented at 4:22 PM on May 22, 2020:

Yes, this is a typo. Fixed!

in src/protocol.h:236 in db20d456f0 outdated

 231 | + * BIP 157 & 158.
 232 | + */
 233 | +extern const char *GETCFHEADERS;
 234 | +/**
 235 | + * cfheaders is a response to a getcfheaders request containing a vector of
 236 | + * filter headers for each block in the requested range.

theStack commented at 12:00 PM on May 22, 2020:

nit: technically, it's only one filter header plus a vector of filter hashes

jnewbery commented at 4:23 PM on May 22, 2020:

Thanks. I've corrected the comment.

[doc] Add comment for m_headers_cache 3bdc7c2d39

[net processing] Message handling for getcfheaders.

if -peerblockfilters is configured, handle requests for cfheaders.

f6b58c1506

[test] Add test for cfheaders 5308c97cca

jnewbery force-pushed on May 22, 2020

jnewbery commented at 4:30 PM on May 22, 2020: member

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

in src/protocol.h:239 in 5308c97cca

 247 | @@ -235,8 +248,6 @@ extern const char* GETCFCHECKPT;
 248 |  /**
 249 |   * cfcheckpt is a response to a getcfcheckpt request containing a vector of
 250 |   * evenly spaced filter headers for blocks on the requested chain.
 251 | - * Only available with service bit NODE_COMPACT_FILTERS as described by
 252 | - * BIP 157 & 158.

MarcoFalke commented at 4:33 PM on May 23, 2020:

Why are you removing this?

jnewbery commented at 5:18 PM on May 23, 2020:

It doesn't appear in the comments for cfheaders and cfilter, so I'm removing it from here for consistency. It seems obvious that NODE_COMPACT_FILTERS is required since it's a response to getcfcheckpt, which requires NODE_COMPACT_FILTERS.

MarcoFalke commented at 5:49 PM on May 23, 2020:

Fair enough

MarcoFalke commented at 4:33 PM on May 23, 2020: member

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

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash eed11e440452664f3996d83b9c085e9a4c0ab77e69ea219b1fe7181017bc09a1 -

</details>

theStack approved

theStack commented at 9:59 AM on May 24, 2020: member

ACK 5308c97ccaf0955e5840956bc1636108a43e6f46 :rocket:

jkczyz commented at 6:15 AM on May 25, 2020: contributor

ACK 5308c97ccaf0955e5840956bc1636108a43e6f46

MarcoFalke merged this on May 26, 2020

MarcoFalke closed this on May 26, 2020

jnewbery deleted the branch on May 26, 2020

sidhujag referenced this in commit 96359f8f51 on May 27, 2020

in test/functional/p2p_blockfilters.py:129 in 5308c97cca

 124 | +            start_height=1,
 125 | +            stop_hash=int(stale_block_hash, 16)
 126 | +        )
 127 | +        node0.send_and_ping(request)
 128 | +        response = node0.last_message['cfheaders']
 129 | +        assert_equal(len(response.hashes), 1000)

jonatack commented at 8:10 AM on June 2, 2020:

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

jnewbery commented at 2:35 PM on June 2, 2020:

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.

in test/functional/test_framework/mininode.py:335 in 5308c97cca

 331 | @@ -330,6 +332,7 @@ def on_close(self):
 332 |      def on_addr(self, message): pass
 333 |      def on_block(self, message): pass
 334 |      def on_blocktxn(self, message): pass
 335 | +    def on_cfheaders(self, message): pass

jonatack commented at 8:11 AM on June 2, 2020:

nit: sort here and lines 34 and 72

jnewbery commented at 2:38 PM on June 2, 2020:

This was fixed in the following PR.

jonatack commented at 8:18 AM on June 2, 2020: member

ACK 5308c97ccaf0955e5840956bc1636108a43e6f46 with some optional style nits for follow-ups. @fjahr's tests at https://github.com/fjahr/bitcoin/commit/36d6854d2b4ed0afa556ccbe0ec72c4cb141a4c9 look worthwhile.

MarcoFalke referenced this in commit 01b45b2e01 on Jun 4, 2020

sidhujag referenced this in commit 04b1107aac on Jun 4, 2020

jasonbcox referenced this in commit a2ee2a5058 on Nov 19, 2020

kittywhiskers referenced this in commit 18d9c296fe on Aug 22, 2021

kittywhiskers referenced this in commit cbe0b336a3 on Aug 22, 2021

kittywhiskers referenced this in commit ad88922073 on Sep 16, 2021

kittywhiskers referenced this in commit 3b7d835abf on Sep 18, 2021

kittywhiskers referenced this in commit 216036b1af on Sep 19, 2021

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

thelazier referenced this in commit 7968888e6e on Sep 25, 2021

kittywhiskers referenced this in commit cb0d7e4325 on Oct 12, 2021

DrahtBot locked this on Feb 15, 2022