net processing: Only send a getheaders for one block in an INV #18962
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Headers-first is the primary method of announcement on the network. If a node fell back sending blocks by inv, it's probably for a re-org. The final block hash provided should be the highest, so send a getheaders and then fetch the blocks we need to catch up.
Member
|
The code changes look good. I need to verify the concept more when it's not late, e.g. that the final block hash will be the highest and the variation/savings in bandwidth. |
Contributor
|
Can the "waste of bandwidth scenario" you describe ever occur? |
Contributor
Author
You're right that current Bitcoin Core software should only send a single block in the INV message. The point about wasting bandwidth is more for other software that may send us many blocks in a single INV, which would cause us to reply with multiple getheaders. |
Member
|
utACK 7467366 |
ajtowns
reviewed
May 14, 2020
Contributor
ajtowns
left a comment
ajtowns
left a comment
There was a problem hiding this comment.
ACK 7467366
I think this patch doesn't change behaviour when the peer is a bitcoind (because it will only potentially send a single block in an inv message), and is not normally relevant because most peers will do headers first download and compact blocks so will find out about blocks that way. As such, which block in the inv is the best one to query doesn't really seem decidable, so picking the last one seems as good a choice as any.
Contributor
|
utACK 7467366 as per ajtowns' reasoning. edit by laanwj: deleted a @ |
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
May 15, 2020
Headers-first is the primary method of announcement on the network. If a node fell back sending blocks by inv, it's probably for a re-org. The final block hash provided should be the highest, so send a getheaders and then fetch the blocks we need to catch up. Github-Pull: bitcoin#18962 Rebased-From: 7467366
maflcko
pushed a commit
that referenced
this pull request
May 15, 2020
245c862 test: disable script fuzz tests (fanquake) 9a8fb4c fuzz: Remove enumeration of expected deserialization exceptions in ProcessMessage(...) fuzzer (practicalswift) 6161c94 [net processing] Only send a getheaders for one block in an INV (John Newbery) cf2a6e2 test: Remove const to work around compiler error on xenial (Wladimir J. van der Laan) cc7d344 miner: Avoid stack-use-after-return in validationinterface (MarcoFalke) 37a6207 test: Add unregister_validation_interface_race test (MarcoFalke) ff4dc20 gui: Fix manual coin control with multiple wallets loaded (João Barbosa) ed0afe8 test: Add test for conflicted wallet tx notifications (Russell Yanofsky) 251e321 rpc: Relock wallet only if most recent callback (João Barbosa) ca4dac4 rpc: Add mutex to guard deadlineTimers (João Barbosa) a3fe458 [docs] Improve commenting in ProcessGetData() (John Newbery) 011532e [test] test that an invalid GETDATA doesn't prevent processing of future messages (Amiti Uttarwar) 1e73d72 [net processing] ignore unknown INV types in GETDATA messages (Amiti Uttarwar) fb82173 [net processing] ignore tx GETDATA from blocks-only peers (Amiti Uttarwar) 315ae14 gui: Fix itemWalletAddress leak when not tree mode (João Barbosa) Pull request description: Backports the following PRs to the 0.20 branch: * #18578: gui: Fix leak in CoinControlDialog::updateView * #18808: [net processing] Drop unknown types in getdata * #18814: rpc: Relock wallet only if most recent callback * #18878: test: Add test for conflicted wallet tx notifications * #18894: gui: Fix manual coin control with multiple wallets loaded * #18742: miner: Avoid stack-use-after-return in validationinterface * #18962: net processing: Only send a getheaders for one block in an INV * #18975: test: Remove const to work around compiler error on xenial ACKs for top commit: promag: Tested ACK 245c862 coin control with multiple wallets. laanwj: ACK 245c862 MarcoFalke: ACK 245c862 solved the conflicts myself as a sanity check. Did not re-review 🍷 Tree-SHA512: 285e5a5fad5bbeba6032742c65dc68836e8eccfcceda9e69fec4ddd162a3f61679a96f9bbe3d434267835af67c21ac4c05accf6f63e827c2eb47203c6daafe31
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 17, 2020
…lock in an INV 7467366 [net processing] Only send a getheaders for one block in an INV (John Newbery) Pull request description: Headers-first is the primary method of announcement on the network. If a node fell back sending blocks by inv, it's probably for a re-org. The final block hash provided should be the highest, so send a getheaders and then fetch the blocks we need to catch up. Sending many GETHEADERS messages to the peer would cause them to send a large number of potentially large HEADERS messages with redundant data, which is a waste of bandwidth. ACKs for top commit: sipa: utACK 7467366 mzumsande: utACK 7467366 as per ajtowns' reasoning. naumenkogs: utACK 7467366 ajtowns: ACK 7467366 jonatack: ACK 7467366 Tree-SHA512: 59e243b80d3f0873709dfacb2e4ffba34689aad7de31ec7f69a64e0e3a0756235a0150e4082ff5de823949ba4411ee1aed2344b4749b62e0eb1ea906e41f5ea9
Contributor
|
post merge ACK 7467366 |
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Feb 1, 2021
Summary: Headers-first is the primary method of announcement on the network. If a node fell back sending blocks by inv, it's probably for a re-org. The final block hash provided should be the highest, so send a getheaders and then fetch the blocks we need to catch up. This is a backport of Core [[bitcoin/bitcoin#18962 | PR18962]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D9113
backpacker69
referenced
this pull request
in peercoin/peercoin
Mar 28, 2021
Headers-first is the primary method of announcement on the network. If a node fell back sending blocks by inv, it's probably for a re-org. The final block hash provided should be the highest, so send a getheaders and then fetch the blocks we need to catch up. Github-Pull: #18962 Rebased-From: 7467366
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
11 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Headers-first is the primary method of announcement on the network. If a node fell back sending blocks by inv, it's probably for a re-org. The final block hash provided should be the highest, so send a getheaders and then fetch the blocks we need to catch up.
Sending many GETHEADERS messages to the peer would cause them to send a large number of potentially large HEADERS messages with redundant data, which is a waste of bandwidth.