p2p: Reduce inv traffic during IBD #19204

pull MarcoFalke wants to merge 4 commits into bitcoin:master from MarcoFalke:2006-netInvWaste changing 6 files +60 −11
  1. MarcoFalke commented at 11:52 PM on June 7, 2020: member

    Tx-inv messages are ignored during IBD, so it would be nice if we told peers to not send them in the first place. Do that by sending two feefilter messages: One when the connection is made (and the node is in IBD), and another one when the node leaves IBD.

  2. MarcoFalke added the label P2P on Jun 7, 2020
  3. DrahtBot commented at 11:50 AM on June 8, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19268 (refactor: Make FeeFilterRounder thread safe by hebasto)
    • #19184 (Overhaul transaction request logic by sipa)

    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.

  4. DrahtBot cross-referenced this on Jun 8, 2020 from issue Refactor: Improve setup_clean_chain semantics by fjahr
  5. naumenkogs commented at 1:06 PM on June 10, 2020: member

    Good observation. Concept ACK on solving this inefficiency. I'm not sure this is an adequate solution.

    Why we don't pretend it's a blocks-only connection at the beginning? I guess maybe because it's unknown before handshake.

    Maybe we can have a new message for this, but that's too much of an overkill...

    Do you have any thoughts?

  6. MarcoFalke commented at 1:27 PM on June 10, 2020: member

    I'd feel uncomfortable using bip37 for this. bloomfilters have been hidden behind a run time flag. It seems fragile to assume that full node implementations in the future are more likely to support bip37 message types than the feefilter message type.

  7. luke-jr approved
  8. luke-jr commented at 4:19 AM on June 11, 2020: member

    utACK

  9. hebasto commented at 6:10 AM on June 11, 2020: member

    Concept ACK.

  10. in src/net_processing.cpp:4397 in fa78c686ca outdated 
    4393 | @@ -4394,9 +4394,20 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
4394 |              !pto->HasPermission(PF_FORCERELAY)) {
4395 |              CAmount currentFilter = m_mempool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFeePerK();
4396 |              int64_t timeNow = GetTimeMicros();
4397 | +            static FeeFilterRounder filterRounder{CFeeRate{DEFAULT_MIN_RELAY_TX_FEE}};
    sipa commented at 7:17 AM on June 11, 2020:

    Nit: as you're touching all places where this variable is used, convert it to current naming style?

    Also: can this be const?

    MarcoFalke commented at 10:52 AM on June 11, 2020:

    filterRounder has a fast random context, which is not const. Calling round on it will change the state.

    sipa commented at 6:17 PM on June 11, 2020:

    Uh, that means it needs a mutex (or very clearly stated assumptions that no two PeerLogicValidation objects can exist that could be accessed from separate threads).

    MarcoFalke commented at 6:52 PM on June 11, 2020:

    This seems unrelated to the changes here. Filed under a new issue: #19254

    sipa commented at 6:53 PM on June 11, 2020:

    Cool, thanks. It's indeed unrelated; no need to address it here.

  11. in src/net_processing.cpp:4399 in fa78c686ca outdated 
    4393 | @@ -4394,9 +4394,20 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
4394 |              !pto->HasPermission(PF_FORCERELAY)) {
4395 |              CAmount currentFilter = m_mempool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFeePerK();
4396 |              int64_t timeNow = GetTimeMicros();
4397 | +            static FeeFilterRounder filterRounder{CFeeRate{DEFAULT_MIN_RELAY_TX_FEE}};
4398 | +            if (m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
4399 | +                // Reveived tx-inv messages are discarded when the active
    sipa commented at 7:18 AM on June 11, 2020:

    Typo: reveived

  12. in test/functional/rpc_net.py:47 in fa78c686ca outdated 
      43 | @@ -44,7 +44,6 @@ def assert_net_servicesnames(servicesflag, servicenames):
  44 |  
  45 |  class NetTest(BitcoinTestFramework):
  46 |      def set_test_params(self):
  47 | -        self.setup_clean_chain = True
    sipa commented at 7:19 AM on June 11, 2020:

    Unrelated change?

  13. sipa commented at 7:21 AM on June 11, 2020: member

    utACK fa78c686ca5bcafa153d2f3813b77449fa31bb89. Nice idea.

  14. in src/validation.h:803 in fa35b8b48d outdated 
     798 | @@ -799,7 +799,8 @@ class ChainstateManager
 799 |      std::vector<CChainState*> GetAll();
 800 |  
 801 |      //! The most-work chain.
 802 | -    CChain& ActiveChain() const;
 803 | +    CChainState& ActiveChainstate() const;
 804 | +    CChain& ActiveChain() const { return ActiveChainstate().m_chain; }
    hebasto commented at 7:37 AM on June 11, 2020:

    If ActiveChain() definition is placed in header, why ActiveChainstate() definition is not placed here too?

    MarcoFalke commented at 11:03 AM on June 11, 2020:

    To minimize the diff and aid review

  15. practicalswift commented at 8:17 AM on June 11, 2020: contributor

    Concept ACK: neat idea! :)

  16. in src/net_processing.cpp:4396 in fa78c686ca outdated 
    4393 | @@ -4394,9 +4394,20 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
4394 |              !pto->HasPermission(PF_FORCERELAY)) {
4395 |              CAmount currentFilter = m_mempool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFeePerK();
4396 |              int64_t timeNow = GetTimeMicros();
4397 | +            static FeeFilterRounder filterRounder{CFeeRate{DEFAULT_MIN_RELAY_TX_FEE}};
4398 | +            if (m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
    hebasto commented at 8:18 AM on June 11, 2020:

    The code that actually rejects tx-inv messages https://github.com/bitcoin/bitcoin/blob/6762a627ecb89ba8d4ed81a049a5d802e6dd75c2/src/net_processing.cpp#L2584-L2586

    contains fImporting and fReindex.

    Should block importing and reindexing be considered as well here?

    MarcoFalke commented at 11:11 AM on June 11, 2020:

    IsInitialBlockDownload does consider when blocks are imported

  17. MarcoFalke force-pushed on Jun 11, 2020
  18. MarcoFalke force-pushed on Jun 11, 2020
  19. MarcoFalke force-pushed on Jun 11, 2020
  20. in src/test/policy_fee_tests.cpp:23 in fa8a66cf7e outdated 
      18 | +    // check that 1000 rounds to 974 or 1071
  19 | +    std::set<CAmount> results;
  20 | +    while (results.size() < 2) {
  21 | +        results.emplace(fee_rounder.round(1000));
  22 | +    }
  23 | +    BOOST_CHECK_EQUAL(*results.begin(), 974);
    rajarshimaitra commented at 12:55 PM on June 11, 2020:

    What is the purpose of fee rounder and why it will return 974 and 1071?

    MarcoFalke commented at 1:09 PM on June 11, 2020:

    There are privacy concerns with deanonymizing a node by the fact that it is broadcasting identifying information about its mempool min fee. To help ameliorate this concern, the implementation quantizes the filter value broadcast with a small amount of randomness, in addition, the messages are broadcast to different peers at individually randomly distributed times.

    Source: https://github.com/bitcoin/bips/blob/master/bip-0133.mediawiki#considerations

  21. rajarshimaitra commented at 12:57 PM on June 11, 2020: contributor

    Concept ACK. Neat Idea. Compiled without issue, all tests passing. Was just wondering rpc_net is not really checking this situation, probably an unrelated change? Also, would it be possible to try having a functional test for this behavior?

    Following is just a question for my own understanding.

  22. MarcoFalke commented at 1:11 PM on June 11, 2020: member

    Thanks for the review so far! The latest force push comes with those changes:

    • Fix typo and global variable naming
    • Document why one of the functional tests needs to change
    • Add a new refactoring cleanup commit, removing three global symbols and replacing them with one local
  23. DrahtBot cross-referenced this on Jun 11, 2020 from issue Overhaul transaction request logic by sipa
  24. laanwj commented at 5:48 PM on June 11, 2020: member

    Concept ACK

    Why we don't pretend it's a blocks-only connection at the beginning? I guess maybe because it's unknown before handshake.

    One problem with that is that it's impossible to switch a blocks-only connection to a full connection. So it'd require dropping and reconnecting all connections after IBD. I'm not sure that's a good idea.

    I'd feel uncomfortable using bip37 for this. bloomfilters have been hidden behind a run time flag. It seems fragile to assume that full node implementations in the future are more likely to support bip37 message types than the feefilter message type.

    However I don't think @naumenkogs is suggesting to use BIP37 messages here, but the fRelay bit in the version message.

  25. MarcoFalke commented at 5:53 PM on June 11, 2020: member

    Jup, fRelay can't be changed while keeping the connection because subsequent version messages are ignored. Recycling the connection seems overly complex and potentially fragile.

  26. glozow commented at 8:02 PM on June 11, 2020: member

    Giant Concept ACK. I think doing this via feefilter is pretty elegant, since we can't change connection type, can't send a second version message with a new fRelay, and can't switch from blocksonly mode. There's apparently a way to do this via fRelay by putting fRelay=False in the version message and then sending a filterclearlater (#8709). In my opinion, nodes not serving bloomfilters should just disconnect on filterclear. An idea that I'm not super confident in: what about adding a P2P message to toggle fRelay on and off? (I think this is mentioned in gmaxwell's comment on #8709) and seems naumenkogs mentioned it too. I'm not sure what does/doesn't warrant a P2P protocol change.

  27. MarcoFalke commented at 8:21 PM on June 11, 2020: member

    A new message type wouldn't be understood by old nodes, making it less effective than the feefilter approach. Also, it wouldn't be more flexible, because feefilter can already be used to effectively turn off and on transaction relay at any time.

  28. luke-jr referenced this in commit 1aca74f524 on Jun 12, 2020
  29. naumenkogs commented at 6:39 AM on June 12, 2020: member

    utACK fa8a66cf7e255884cb5cf061f43f42a7371e7ff4

  30. hebasto approved
  31. hebasto commented at 7:27 AM on June 12, 2020: member

    ACK fa8a66cf7e255884cb5cf061f43f42a7371e7ff4, tested on Linux Mint 19.3 (x86_64):

    $ ./test/functional/rpc_net.py
2020-06-12T07:26:00.331000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_nf7udywg
2020-06-12T07:26:00.801000Z TestFramework (INFO): Get out of IBD for the minfeefilter test
2020-06-12T07:26:00.805000Z TestFramework (INFO): Connect nodes both way
2020-06-12T07:26:01.008000Z TestFramework (INFO): Connect nodes both way
2020-06-12T07:26:01.453000Z TestFramework (INFO): Stopping nodes
2020-06-12T07:26:01.605000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_nf7udywg on exit
2020-06-12T07:26:01.605000Z TestFramework (INFO): Tests successful
  32. glozow cross-referenced this on Jun 12, 2020 from issue p2p: disconnect peers that send filterclear + update existing filter msg disconnect logic by glozow
  33. DrahtBot cross-referenced this on Jun 14, 2020 from issue doc: Add non-thread-safe note to FeeFilterRounder::round() by hebasto
  34. ajtowns commented at 6:05 AM on June 15, 2020: contributor

    utACK fa8a66cf7e255884cb5cf061f43f42a7371e7ff4

  35. in src/net_processing.cpp:2579 in fa8a66cf7e outdated 
    2580 | @@ -2581,7 +2581,7 @@ bool ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRec
2581 |                      LogPrint(BCLog::NET, "transaction (%s) inv sent in violation of protocol, disconnecting peer=%d\n", inv.hash.ToString(), pfrom.GetId());
2582 |                      pfrom.fDisconnect = true;
2583 |                      return true;
2584 | -                } else if (!fAlreadyHave && !fImporting && !fReindex && !::ChainstateActive().IsInitialBlockDownload()) {
2585 | +                } else if (!fAlreadyHave && !chainman.ActiveChainstate().IsInitialBlockDownload()) {
    jonatack commented at 8:41 AM on June 15, 2020:

    fa3b6974 I think the commit message would be clearer if it stated that !fImporting && !fReindex can be removed here because they are checked in IsInitialBlockDownload which returns true if fImporting || fReindex

    MarcoFalke commented at 10:21 AM on June 15, 2020:

    I keep my commit messages as brief as possible and try to only include meta information that can not be derived from the raw source code. My goal is to encourage reviewers to give closer looks instead of trusting the commit message.

  36. jonatack commented at 8:46 AM on June 15, 2020: contributor

    ACK fa8a66cf7e255884 a functional test in p2p_feefilter.py for the changes in fa8a66cf would be a good idea

  37. MarcoFalke commented at 10:17 AM on June 15, 2020: member

    If someone wants to write a functional test for this, I am happy to review. Otherwise, I will write one myself after merge.

  38. fanquake referenced this in commit 28ce05d06f on Jun 16, 2020
  39. DrahtBot cross-referenced this on Jun 16, 2020 from issue net: Avoid redundant and confusing FAILED log by MarcoFalke
  40. DrahtBot added the label Needs rebase on Jun 19, 2020
  41. test: Add FeeFilterRounder test fabf3d64ff
  42. Add ChainstateManager::ActiveChainstate faba65e696
  43. refactor: block import implies IsInitialBlockDownload fa06d7e934
  44. net: Avoid wasting inv traffic during IBD fa525e4d1c
  45. MarcoFalke force-pushed on Jun 19, 2020
  46. DrahtBot removed the label Needs rebase on Jun 19, 2020
  47. jamesob commented at 3:17 PM on June 19, 2020: member

    ACK fa525e4d1cfda8c1924d2c69f43bd7ae3b98fb72 (jamesob/ackr/19204.1.MarcoFalke.p2p_reduce_inv_traffic_d)

    Haven't tested. Tried to build locally, but got a linker failure (which sounds like is not unique to this branch):

    /usr/bin/ld: bitcoin_wallet-bitcoin-wallet.o:(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'

    <details><summary>Show platform data</summary> <p>

    Tested on Linux-4.19.0-9-amd64-x86_64-with-glibc2.28

Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-std=c++11 --enable-wallet --enable-debug --with-daemon

Compiled with /usr/bin/ccache /usr/bin/g++ -std=c++11 -mavx -mavx2 -std=c++11 -fno-extended-identifiers -O0 -g3 -ftrapv  -fstack-reuse=none -Wstack-protector -fstack-protector-all -msse4 -msha -msse4.1 -msse4.2  i

Compiler version: g++ (Debian 8.3.0-6) 8.3.0

    </p></details>

    <details><summary>Show signature data</summary> <p>

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

ACK fa525e4d1cfda8c1924d2c69f43bd7ae3b98fb72 ([`jamesob/ackr/19204.1.MarcoFalke.p2p_reduce_inv_traffic_d`](https://github.com/jamesob/bitcoin/tree/ackr/19204.1.MarcoFalke.p2p_reduce_inv_traffic_d))

Haven't tested. Tried to build locally, but got a linker failure (which sounds
like is not unique to this branch):

    /usr/bin/ld: bitcoin_wallet-bitcoin-wallet.o:(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'

    
<details><summary>Show platform data</summary>
<p>

    Tested on Linux-4.19.0-9-amd64-x86_64-with-glibc2.28

    Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-std=c++11 --enable-wallet --enable-debug --with-daemon

    Compiled with /usr/bin/ccache /usr/bin/g++ -std=c++11 -mavx -mavx2 -std=c++11 -fno-extended-identifiers -O0 -g3 -ftrapv -fstack-reuse=none -Wstack-protector -fstack-protector-all -msse4 -msha -msse4.1 -msse4.2 i

    Compiler version: g++ (Debian 8.3.0-6) 8.3.0

    
</p></details>
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEpm2+LMqPsWj2rAwWyrywS1A+5mgFAl7s1u0ACgkQyrywS1A+
5mha8A/8DH0r6iY6lZbK9pkU7JHO+z+AGrvHG+oNq6e7GiXQqNQdgAZifhh9kfiv
mH+z1JZPX4pTTiC9945BtfWLI1apjZsgOslFlhTFCwP0STbvGxcBYyGO7k/HqUHg
X03Au2iSRH02avbeyd4oCxbBIy2wZJZSkK7Aezhtq9m6C8rgNrFtDHi9JYAGDys/
fMj4p/z+3DaIldeFivYWcT6fXyy6AtNXSJlWlxJFk2divc46V/5haR0s0KtyyQyt
UNAAOf830rVEb6yl14HFov3HRWB2V6HHf8NTkP9vRo3GYuGqhyPqrEa4+MVIcnaA
qAVQEzMfCuj6XTOyh3Yx8nterGG4L6+ak9TfagdjgoAme6lenfQN5IbLeEMUXsZL
RxbT+bAwgAbZ1OP+J+XAt74jbAsnIw27pew+KpnnpaDwQAHiZLkFy7zRGA1LSYkI
jm9NyNx7I0YPwSR2/K1q6OpsoVIubgMrP3Sm4boqOnmGSc3ZZEURdJDwxoFFnwc9
pbGX4yRIxvUT31CvWnWIeF2zo9RN29Y4mzwufUMyLqoj3NqD6GZ6H/nuMkRbDT4d
jr17KilECwi6IBMRthZn6a7el3fRQe4leGBEjwpbIh78q5SoU445VXLSJIS28Ts2
1Lrwngh3GkHq8UDMrIY/hpzsO9HOnP1Xvcpjn3RHdSFBcO03AGE=
=hJ4k
-----END PGP SIGNATURE-----

    </p></details>

  48. MarcoFalke commented at 4:12 PM on June 19, 2020: member

    Jup, the gcc bug is unrelated and tracked in #19309

  49. glozow commented at 4:58 PM on June 19, 2020: member

    ACK https://github.com/bitcoin/bitcoin/commit/fa525e4d1cfda8c1924d2c69f43bd7ae3b98fb72 I tested it functionally with this, confirms that the minfeefilter is set before/after IBD.

    If someone wants to write a functional test for this, I am happy to review. Otherwise, I will write one myself after merge.

    I'm not sure if this counts as a complete functional test 😅 this is kind of a draft that just checks minfeefilter (not any txrelay behavior) but I'd be down to do this.

  50. MarcoFalke commented at 5:02 PM on June 19, 2020: member

    For anyone who reviewed before the rebase, it should be trivial to re-ACK with git range-diff bitcoin-core/master A B

  51. naumenkogs commented at 7:03 AM on June 20, 2020: member

    utACK fa525e4

  52. jonatack commented at 12:48 PM on June 29, 2020: contributor

    re-ACK fa525e4 checked diff git range-diff 19612ca fa8a66c fa525e4, re-reviewed, ran tests, ran a custom p2p IBD behavior test at https://github.com/jonatack/bitcoin/commit/9321e0f223ea87d21f6fa42b61bcc8d40a0943de. @gzhao408 in that link I adapted your test, feel free to use any or all of it for a PR.

  53. hebasto approved
  54. hebasto commented at 1:11 PM on June 29, 2020: member

    re-ACK fa525e4d1cfda8c1924d2c69f43bd7ae3b98fb72, only rebased since the previous review (verified with git range-diff).

  55. MarcoFalke merged this on Jun 29, 2020
  56. MarcoFalke closed this on Jun 29, 2020
  57. glozow cross-referenced this on Jul 1, 2020 from issue test: add functional test for txrelay during and after IBD by glozow
  58. MarcoFalke referenced this in commit 19aaf7945e on Jul 17, 2020
  59. rebroad commented at 2:31 PM on August 20, 2020: contributor

    Wouldn't the filterclear message be better to use for when tx invs are desired? I could be wrong, but I think this would (or should) request tx invs even if the connection was made with relay=0 (in the version message).

  60. MarcoFalke deleted the branch on Aug 20, 2020
  61. MarcoFalke commented at 3:34 PM on August 20, 2020: member

    Have you seen #19204 (comment) and #19260 ?

  62. Fabcien referenced this in commit 5328130628 on Mar 22, 2021
  63. github-actions[bot] cross-referenced this on Apr 14, 2022 from issue Link Checker Report by github-actions[bot]
  64. github-actions[bot] cross-referenced this on Apr 14, 2022 from issue Link Checker Report by github-actions[bot]
  65. github-actions[bot] cross-referenced this on Apr 14, 2022 from issue Link Checker Report by github-actions[bot]
  66. kwvg referenced this in commit 29266b5e6a on Apr 29, 2022
  67. kwvg referenced this in commit 6378759fa5 on May 21, 2022
  68. kwvg referenced this in commit faaf840016 on May 23, 2022
  69. bitcoin locked this on Aug 16, 2022
Contributors
MarcoFalkeDrahtBotnaumenkogsluke-jrhebastosipapracticalswiftrajarshimaitralaanwjglozowajtownsjonatackjamesobrebroad
Labels
P2P
Linked (view graph)
#19254 static FeeFilterRounder in net_processing needs a mutex (or documentation that concurrent access is not allowed)#19260 p2p: disconnect peers that send filterclear + update existing filter msg disconnect logic#19309 refactor: Fix link error with --enable-debug#19423 test: add functional test for txrelay during and after IBD
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:53 UTC