p2p: Reduce inv traffic during IBD

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.

MarcoFalke added the label P2P on Jun 7, 2020

DrahtBot commented at 11:50 am on June 8, 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:

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

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?

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.

luke-jr approved

luke-jr commented at 4:19 am on June 11, 2020: member

utACK

hebasto commented at 6:10 am on June 11, 2020: member

Concept ACK.

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.

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

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?

sipa commented at 7:21 am on June 11, 2020: member

utACK fa78c686ca5bcafa153d2f3813b77449fa31bb89. Nice idea.

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

practicalswift commented at 8:17 am on June 11, 2020: contributor

Concept ACK: neat idea! :)

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

MarcoFalke force-pushed on Jun 11, 2020

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

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.

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

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.

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.

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.

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.

luke-jr referenced this in commit 1aca74f524 on Jun 12, 2020

naumenkogs commented at 6:39 am on June 12, 2020: member

utACK fa8a66cf7e255884cb5cf061f43f42a7371e7ff4

hebasto approved

hebasto commented at 7:27 am on June 12, 2020: member

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

0$ ./test/functional/rpc_net.py
12020-06-12T07:26:00.331000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_nf7udywg
22020-06-12T07:26:00.801000Z TestFramework (INFO): Get out of IBD for the minfeefilter test
32020-06-12T07:26:00.805000Z TestFramework (INFO): Connect nodes both way
42020-06-12T07:26:01.008000Z TestFramework (INFO): Connect nodes both way
52020-06-12T07:26:01.453000Z TestFramework (INFO): Stopping nodes
62020-06-12T07:26:01.605000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_nf7udywg on exit
72020-06-12T07:26:01.605000Z TestFramework (INFO): Tests successful

ajtowns commented at 6:05 am on June 15, 2020: member

utACK fa8a66cf7e255884cb5cf061f43f42a7371e7ff4

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.

jonatack commented at 8:46 am on June 15, 2020: member

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

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.

fanquake referenced this in commit 28ce05d06f on Jun 16, 2020

DrahtBot added the label Needs rebase on Jun 19, 2020

test: Add FeeFilterRounder test fabf3d64ff

Add ChainstateManager::ActiveChainstate faba65e696

refactor: block import implies IsInitialBlockDownload fa06d7e934

net: Avoid wasting inv traffic during IBD fa525e4d1c

MarcoFalke force-pushed on Jun 19, 2020

DrahtBot removed the label Needs rebase on Jun 19, 2020

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):

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

0Tested on Linux-4.19.0-9-amd64-x86_64-with-glibc2.28
1
2Configured 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
3
4Compiled 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
5
6Compiler version: g++ (Debian 8.3.0-6) 8.3.0

0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 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))
4
5Haven't tested. Tried to build locally, but got a linker failure (which sounds
6like 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&)'

0
1<details><summary>Show platform data</summary>
2<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

 0
 1</p></details>
 2-----BEGIN PGP SIGNATURE-----
 3
 4iQIzBAEBCgAdFiEEpm2+LMqPsWj2rAwWyrywS1A+5mgFAl7s1u0ACgkQyrywS1A+
 55mha8A/8DH0r6iY6lZbK9pkU7JHO+z+AGrvHG+oNq6e7GiXQqNQdgAZifhh9kfiv
 6mH+z1JZPX4pTTiC9945BtfWLI1apjZsgOslFlhTFCwP0STbvGxcBYyGO7k/HqUHg
 7X03Au2iSRH02avbeyd4oCxbBIy2wZJZSkK7Aezhtq9m6C8rgNrFtDHi9JYAGDys/
 8fMj4p/z+3DaIldeFivYWcT6fXyy6AtNXSJlWlxJFk2divc46V/5haR0s0KtyyQyt
 9UNAAOf830rVEb6yl14HFov3HRWB2V6HHf8NTkP9vRo3GYuGqhyPqrEa4+MVIcnaA
10qAVQEzMfCuj6XTOyh3Yx8nterGG4L6+ak9TfagdjgoAme6lenfQN5IbLeEMUXsZL
11RxbT+bAwgAbZ1OP+J+XAt74jbAsnIw27pew+KpnnpaDwQAHiZLkFy7zRGA1LSYkI
12jm9NyNx7I0YPwSR2/K1q6OpsoVIubgMrP3Sm4boqOnmGSc3ZZEURdJDwxoFFnwc9
13pbGX4yRIxvUT31CvWnWIeF2zo9RN29Y4mzwufUMyLqoj3NqD6GZ6H/nuMkRbDT4d
14jr17KilECwi6IBMRthZn6a7el3fRQe4leGBEjwpbIh78q5SoU445VXLSJIS28Ts2
151Lrwngh3GkHq8UDMrIY/hpzsO9HOnP1Xvcpjn3RHdSFBcO03AGE=
16=hJ4k
17-----END PGP SIGNATURE-----

MarcoFalke commented at 4:12 pm on June 19, 2020: member

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

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.

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

naumenkogs commented at 7:03 am on June 20, 2020: member

utACK fa525e4

jonatack commented at 12:48 pm on June 29, 2020: member

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.

hebasto approved

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

MarcoFalke merged this on Jun 29, 2020

MarcoFalke closed this on Jun 29, 2020

MarcoFalke referenced this in commit 19aaf7945e on Jul 17, 2020

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

MarcoFalke deleted the branch on Aug 20, 2020

MarcoFalke commented at 3:34 pm on August 20, 2020: member

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

Fabcien referenced this in commit 5328130628 on Mar 22, 2021

kittywhiskers referenced this in commit 29266b5e6a on Apr 29, 2022

kittywhiskers referenced this in commit 6378759fa5 on May 21, 2022

kittywhiskers referenced this in commit faaf840016 on May 23, 2022

DrahtBot locked this on Aug 16, 2022

p2p: Reduce inv traffic during IBD #19204

Conflicts