p2p: Reduce inv traffic during IBD #19204

maflcko · 2020-06-07T23:52:40Z

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.

DrahtBot · 2020-06-08T11:50:38Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

doc: Add non-thread-safe note to FeeFilterRounder::round() #19268 (refactor: Make FeeFilterRounder thread safe by hebasto)
Overhaul transaction request logic #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 · 2020-06-10T13:06:52Z

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?

maflcko · 2020-06-10T13:27:45Z

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

utACK

hebasto · 2020-06-11T06:10:06Z

Concept ACK.

sipa · 2020-06-11T07:21:30Z

utACK fa78c686ca5bcafa153d2f3813b77449fa31bb89. Nice idea.

src/net_processing.cpp

test/functional/rpc_net.py

src/validation.h

practicalswift · 2020-06-11T08:17:48Z

Concept ACK: neat idea! :)

src/net_processing.cpp

rajarshimaitra

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.

src/test/policy_fee_tests.cpp

maflcko · 2020-06-11T13:11:55Z

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 · 2020-06-11T17:48:43Z

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.

maflcko · 2020-06-11T17:53:25Z

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

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.

maflcko · 2020-06-11T20:21:14Z

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.

Github-Pull: bitcoin#19204 Rebased-From: fa8a66cf7e255884cb5cf061f43f42a7371e7ff4

naumenkogs · 2020-06-12T06:39:09Z

utACK fa8a66cf7e255884cb5cf061f43f42a7371e7ff4

hebasto

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

ajtowns · 2020-06-15T06:05:31Z

utACK fa8a66cf7e255884cb5cf061f43f42a7371e7ff4

jonatack

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

src/net_processing.cpp

maflcko · 2020-06-15T10:17:34Z

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

…isting filter msg disconnect logic 3a10d93 [p2p/refactor] move disconnect logic and remove misbehaving (gzhao408) ff8c430 [test] test disconnect for filterclear (gzhao408) 1c6b787 [netprocessing] disconnect node that sends filterclear (gzhao408) Pull request description: Nodes that don't have bloomfilters turned on (i.e. no `NODE_BLOOM` service) should disconnect peers that send them `filterclear` P2P messages. Non-bloomfilter nodes already disconnect peers for [`filteradd` and `filterload`](https://github.com/bitcoin/bitcoin/blob/19e919217e6d62e3640525e4149de1a4ae04e74f/src/net_processing.cpp#L2218), but #8709 removed `filterclear` so it could be used to reset tx relay. This isn't needed now because using `feefilter` message is much better for this purpose (See #19204). Also refactors existing disconnect logic for `filteradd` and `filterload` into respective message handlers and removes banning for them. ACKs for top commit: jnewbery: Code review ACK 3a10d93 naumenkogs: utACK 3a10d93 gillichu: tested ACK: quick test_runner on macOS [`3a10d93`](3a10d93) MarcoFalke: re-ACK 3a10d93 only change is replacing false with true 🚝 Tree-SHA512: 7aad8b3c0b0e776a47ad52544f0c1250feb242320f9a2962542f5905042f77e297a1486f8cdc3bf0fb93cd00c1ab66a67b2ec426eb6da3fe4cda56b5e623620f

jamesob · 2020-06-19T15:17:24Z

ACK fa525e4 (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&)'

Show platform data

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

Show signature data

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

maflcko · 2020-06-19T16:12:40Z

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

glozow

ACK fa525e4
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.

maflcko · 2020-06-19T17:02:46Z

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

naumenkogs · 2020-06-20T07:03:59Z

utACK fa525e4

jonatack

re-ACK fa525e4 checked diff git range-diff 19612ca fa8a66c fa525e4, re-reviewed, ran tests, ran a custom p2p IBD behavior test at jonatack@9321e0f.

@gzhao408 in that link I adapted your test, feel free to use any or all of it for a PR.

hebasto

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

cb31ee0 [test] feefilter during and after IBD (gzhao408) Pull request description: This is a followup to #19204 which uses `minfeefilter=MAX_MONEY` to effectively shut off txrelay, thereby reducing inv traffic, when nodes are in IBD. It was [missing](bitcoin/bitcoin#19204 (comment)) a functional test. ACKs for top commit: jnewbery: utACK cb31ee0 Tree-SHA512: a9effc8193fa95fb42a2f9c66b258cc7b0941fc04c1ce3a6092f4426c9bfc7e72f702aca559b3e30e90652497f411f22fae3cf5cdb6cfd6ef6d37fed712cda67

rebroad · 2020-08-20T14:31:58Z

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

maflcko · 2020-08-20T15:34:42Z

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

Summary: ``` 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. ``` Backport of [[bitcoin/bitcoin#19204 | core#19204]]. Test Plan: ninja all check-all Run IBD. Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9330

maflcko added the P2P label Jun 7, 2020

DrahtBot mentioned this pull request Jun 8, 2020

Refactor: Improve setup_clean_chain semantics #19168

Closed

luke-jr approved these changes Jun 11, 2020

View reviewed changes

sipa reviewed Jun 11, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

test/functional/rpc_net.py Outdated Show resolved Hide resolved

hebasto reviewed Jun 11, 2020

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

hebasto reviewed Jun 11, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 2006-netInvWaste branch 3 times, most recently from fac0135 to fa8a66c Compare June 11, 2020 12:47

rajarshimaitra reviewed Jun 11, 2020

View reviewed changes

src/test/policy_fee_tests.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jun 11, 2020

Overhaul transaction request logic #19184

Closed

glozow reviewed Jun 11, 2020

View reviewed changes

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 12, 2020

net: Avoid wasting inv traffic during IBD

1aca74f

Github-Pull: bitcoin#19204 Rebased-From: fa8a66cf7e255884cb5cf061f43f42a7371e7ff4

hebasto approved these changes Jun 12, 2020

View reviewed changes

glozow mentioned this pull request Jun 12, 2020

p2p: disconnect peers that send filterclear + update existing filter msg disconnect logic #19260

Merged

DrahtBot mentioned this pull request Jun 14, 2020

doc: Add non-thread-safe note to FeeFilterRounder::round() #19268

Merged

jonatack reviewed Jun 15, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jun 16, 2020

net: Avoid redundant and confusing FAILED log #19293

Merged

DrahtBot added the Needs rebase label Jun 19, 2020

MarcoFalke added 4 commits June 19, 2020 09:26

test: Add FeeFilterRounder test

fabf3d6

Add ChainstateManager::ActiveChainstate

faba65e

refactor: block import implies IsInitialBlockDownload

fa06d7e

net: Avoid wasting inv traffic during IBD

fa525e4

maflcko force-pushed the 2006-netInvWaste branch from fa8a66c to fa525e4 Compare June 19, 2020 13:29

DrahtBot removed the Needs rebase label Jun 19, 2020

glozow reviewed Jun 19, 2020

View reviewed changes

jonatack reviewed Jun 29, 2020

View reviewed changes

hebasto approved these changes Jun 29, 2020

View reviewed changes

maflcko merged commit 8edfc17 into bitcoin:master Jun 29, 2020

glozow mentioned this pull request Jul 1, 2020

test: add functional test for txrelay during and after IBD #19423

Merged

maflcko deleted the 2006-netInvWaste branch August 20, 2020 15:31

This was referenced Apr 14, 2022

Link Checker Report doitian/knowledge-base#3

Closed

Link Checker Report doitian/knowledge-base#4

Closed

Link Checker Report doitian/knowledge-base#5

Closed

kwvg added a commit to kwvg/dash that referenced this pull request May 23, 2022

partial bitcoin#19204: Reduce inv traffic during IBD

faaf840

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

p2p: Reduce inv traffic during IBD #19204

p2p: Reduce inv traffic during IBD #19204

Uh oh!

Conversation

maflcko commented Jun 7, 2020

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

naumenkogs commented Jun 10, 2020

Uh oh!

maflcko commented Jun 10, 2020

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jun 11, 2020

Uh oh!

sipa commented Jun 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Jun 11, 2020

Uh oh!

Uh oh!

rajarshimaitra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Jun 11, 2020

Uh oh!

laanwj commented Jun 11, 2020

Uh oh!

maflcko commented Jun 11, 2020

Uh oh!

glozow 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!

maflcko commented Jun 11, 2020

Uh oh!

naumenkogs commented Jun 12, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns commented Jun 15, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Jun 15, 2020

Uh oh!

jamesob commented Jun 19, 2020

Uh oh!

maflcko commented Jun 19, 2020

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 19, 2020

Uh oh!

naumenkogs commented Jun 20, 2020

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!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

rebroad commented Aug 20, 2020

Uh oh!

maflcko commented Aug 20, 2020

Uh oh!

Reviewers

DrahtBot commented Jun 8, 2020 •

edited

Loading

glozow left a comment •

edited

Loading

jonatack left a comment •

edited

Loading