4. Avoid repeated requests to peers after partial responses or errors #3505

teor2345 · 2022-02-10T03:13:24Z

Motivation

We want to avoid repeated requests to peers that don't have a block or transaction:

zcashd doesn't respond to requests for missing blocks with notfound messages.
So we have to check its responses, and track missing inventory in them.
When Zebra cancels a request because a peer is slow, or when a peer times out, we don't want to try that peer again.

We want to retry missing blocks or transactions after a few minutes:

Zebra's inventory rotation interval was a bit long.

Solution

register peer partial responses and errors as missing inventory
- add an inventory collector to Client
- split synthetic NotFoundRegistry errors from NotFoundResponse errors created from peer messages
- add tests for partial missing and error inventory registration
make the inventory rotation interval a bit shorter, to improve performance

Related changes (needed for tests):

add a proptest-impl feature to zebra-network
add a test-only connect_isolated_with_inbound function

Related Fixes:

minor review fixes from PR 3. Send notfound when Zebra doesn't have a block or transaction #3466

Closes #2156.

Review

@oxarbitrage can review this PR, I'm happy to do a video review if that would help.

During reviews, I'd like to focus on any design issues or bugs in this PR.

Reviewer Checklist

Code implements Specs and Designs
Tests for Expected Behaviour
Tests for Errors

Follow Up Work

We're finally done with these network fixes!

codecov · 2022-02-10T04:25:04Z

Codecov Report

Merging #3505 (de4ccc8) into main (499ae89) will increase coverage by 2.11%.
The diff coverage is 80.15%.

❗ Current head de4ccc8 differs from pull request most recent head f8474f2. Consider uploading reports for the commit f8474f2 to get more accurate results

@@            Coverage Diff             @@
##             main    #3505      +/-   ##
==========================================
+ Coverage   78.34%   80.45%   +2.11%     
==========================================
  Files         267      274       +7     
  Lines       31526    32229     +703     
==========================================
+ Hits        24698    25931    +1233     
+ Misses       6828     6298     -530

zebra-network/src/peer/client.rs

zebra-network/src/peer_set/set.rs

zebra-network/src/peer_set/inventory_registry.rs

teor2345 · 2022-02-11T09:04:16Z

The rest of the PR is:

getting the channels to the place where we need to use them
test support
cleanups after the review of 3. Send notfound when Zebra doesn't have a block or transaction #3466

zebrad/src/components/sync/tests/timing.rs

zebra-network/src/isolated/tor.rs

zebra-network/src/isolated.rs

…ests

…ry errors

This stops us waiting 3-4 sync resets (4 minutes) before we retry a missing block. Now we wait 1-2 sync resets (2 minutes), which is still a reasonable rate limit. This should speed up syncing near the tip, and on testnet.

…nsistent

zebrad/src/components/inbound/tests/real_peer_set.rs

zebra-network/Cargo.toml

Co-authored-by: Janito Vaqueiro Ferreira Filho <janito.vff@gmail.com>

teor2345 · 2022-02-15T01:45:48Z

The deny.toml CI failed due to temporary alpine linux Docker network errors.

We can just ignore them for this PR, because this PR doesn't change any dependencies.
(It only adds features that activate dependencies.)

* ZIPs were updated to remove ambiguity, this was tracked in #1267. * #2105 was fixed by #3039 and #2379 was closed by #3069 * #2230 was a duplicate of #2231 which was closed by #2511 * #3235 was obsoleted by #2156 which was fixed by #3505 * #1850 was fixed by #2944, #1851 was fixed by #2961 and #2902 was fixed by #2969 * We migrated to Rust 2021 edition in Jan 2022 with #3332 * #1631 was closed as not needed * #338 was fixed by #3040 and #1162 was fixed by #3067 * #2079 was fixed by #2445 * #4794 was fixed by #6122 * #1678 stopped being an issue * #3151 was fixed by #3934 * #3204 was closed as not needed * #1213 was fixed by #4586 * #1774 was closed as not needed * #4633 was closed as not needed * Clarify behaviour of difficulty spacing Co-authored-by: teor <teor@riseup.net> * Update comment to reflect implemented behaviour Co-authored-by: teor <teor@riseup.net> * Update comment to reflect implemented behaviour when retrying block downloads Co-authored-by: teor <teor@riseup.net> * Update `TODO` to remove closed issue and clarify when we might want to fix Co-authored-by: teor <teor@riseup.net> * Update `TODO` to remove closed issue and clarify what we might want to change in future Co-authored-by: teor <teor@riseup.net> * Clarify benefits of how we do block verification Co-authored-by: teor <teor@riseup.net> * Fix rustfmt errors --------- Co-authored-by: teor <teor@riseup.net>

teor2345 added C-bug Category: This is a bug P-Medium ⚡ C-security Category: Security issues I-slow Problems with performance or responsiveness I-remote-node-overload Zebra can overload other nodes on the network A-network Area: Network protocol updates or fixes labels Feb 10, 2022

teor2345 requested review from jvff and oxarbitrage February 10, 2022 03:13

teor2345 self-assigned this Feb 10, 2022

teor2345 changed the title ~~4. Avoid repeated requests to peers with previous request errors or partial responses~~ 4. Avoid repeated requests to peers after partial responses or errors Feb 10, 2022

teor2345 force-pushed the notfound-error branch from d0f2697 to 288a90b Compare February 10, 2022 03:15

This comment was marked as resolved.

Sign in to view

teor2345 mentioned this pull request Feb 11, 2022

3. Send notfound when Zebra doesn't have a block or transaction #3466

Merged

7 tasks

teor2345 force-pushed the notfound-send branch from dfeda1c to 7f81526 Compare February 11, 2022 00:28

teor2345 force-pushed the notfound-error branch from 288a90b to c438be0 Compare February 11, 2022 00:28

teor2345 marked this pull request as ready for review February 11, 2022 08:45

teor2345 removed the request for review from jvff February 11, 2022 08:45

teor2345 marked this pull request as draft February 11, 2022 08:51

teor2345 commented Feb 11, 2022

View reviewed changes

zebra-network/src/peer/client.rs Show resolved Hide resolved

teor2345 commented Feb 11, 2022

View reviewed changes

zebra-network/src/peer_set/set.rs Show resolved Hide resolved

teor2345 commented Feb 11, 2022

View reviewed changes

zebra-network/src/peer_set/inventory_registry.rs Show resolved Hide resolved

jvff reviewed Feb 13, 2022

View reviewed changes

zebrad/src/components/sync/tests/timing.rs Outdated Show resolved Hide resolved

zebrad/src/components/sync/tests/timing.rs Outdated Show resolved Hide resolved

jvff reviewed Feb 13, 2022

View reviewed changes

zebra-network/src/isolated/tor.rs Show resolved Hide resolved

zebra-network/src/isolated/tor.rs Outdated Show resolved Hide resolved

jvff reviewed Feb 13, 2022

View reviewed changes

zebra-network/src/isolated.rs Show resolved Hide resolved

zebra-network/src/isolated.rs Show resolved Hide resolved

teor2345 force-pushed the notfound-send branch from 7f81526 to 6c9994c Compare February 13, 2022 22:42

teor2345 force-pushed the notfound-error branch from df2f220 to 0c5f18d Compare February 13, 2022 22:42

Base automatically changed from notfound-send to main February 14, 2022 01:51

teor2345 force-pushed the notfound-error branch from 276fb97 to 961edc0 Compare February 14, 2022 02:39

teor2345 added 15 commits February 15, 2022 05:23

feat(network): add a proptest-impl feature to zebra-network

075cecb

feat(network): add a test-only connect_isolated_with_inbound function

e71f592

test(network): allow a response on the isolated peer test connection

fd016ab

test(network): fix failures in test synthetic notfound

f342b47

test(network): Simplify SharedPeerError test assertions

c29ec63

test(network): test synthetic notfound from partially successful requ…

ddac9f3

…ests

test(network): MissingInventoryCollector ignores local NotFoundRegist…

671d140

…ry errors

fix(network): decrease the inventory rotation interval

470e14e

This stops us waiting 3-4 sync resets (4 minutes) before we retry a missing block. Now we wait 1-2 sync resets (2 minutes), which is still a reasonable rate limit. This should speed up syncing near the tip, and on testnet.

fmt(network): cargo fmt --all

ac9ad41

cleanup(network): remove unnecessary allow(dead_code)

cbb3b73

cleanup(network): stop importing the whole sync module into tests

18eb78e

doc(network): clarify syncer inventory retry constraint

7a49d45

doc(network): add a TODO for a fix to ensure API behaviour remains co…

60e9cba

…nsistent

doc(network): fix a function doc typo

9b92289

doc(network): clarify how we handle peers that don't send notfound

a84efd3

teor2345 force-pushed the notfound-error branch from 2237ee7 to a84efd3 Compare February 14, 2022 19:23

jvff previously approved these changes Feb 14, 2022

View reviewed changes

zebrad/src/components/inbound/tests/real_peer_set.rs Outdated Show resolved Hide resolved

teor2345 commented Feb 15, 2022

View reviewed changes

zebra-network/Cargo.toml Show resolved Hide resolved

teor2345 dismissed jvff’s stale review via de4ccc8 February 15, 2022 00:02

docs(network): clarify a test comment

de4ccc8

Co-authored-by: Janito Vaqueiro Ferreira Filho <janito.vff@gmail.com>

teor2345 requested a review from jvff February 15, 2022 00:41

jvff approved these changes Feb 15, 2022

View reviewed changes

This was referenced Feb 15, 2022

merge-queue: embarking main (495e1ec), #3531 and #3532 together #3535

Closed

merge-queue: embarking main (b4d7080), #3532 and #3529 together #3538

Closed

Merge branch 'main' into notfound-error

f8474f2

mergify bot merged commit a4dd3b7 into main Feb 15, 2022

mergify bot deleted the notfound-error branch February 15, 2022 01:44

mpguerra added a commit that referenced this pull request May 19, 2023

#3235 was obsoleted by #2156 which was fixed by #3505

2e362d9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

4. Avoid repeated requests to peers after partial responses or errors #3505

4. Avoid repeated requests to peers after partial responses or errors #3505

teor2345 commented Feb 10, 2022 •

edited

Loading

codecov bot commented Feb 10, 2022 •

edited

Loading

This comment was marked as resolved.

teor2345 commented Feb 11, 2022

teor2345 commented Feb 15, 2022

4. Avoid repeated requests to peers after partial responses or errors #3505

4. Avoid repeated requests to peers after partial responses or errors #3505

Conversation

teor2345 commented Feb 10, 2022 • edited Loading

Motivation

Solution

Review

Reviewer Checklist

Follow Up Work

codecov bot commented Feb 10, 2022 • edited Loading

Codecov Report

This comment was marked as resolved.

teor2345 commented Feb 11, 2022

teor2345 commented Feb 15, 2022

teor2345 commented Feb 10, 2022 •

edited

Loading

codecov bot commented Feb 10, 2022 •

edited

Loading