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

teor2345 · 2022-02-04T07:17:16Z

Motivation

Currently, Zebra just ignores block or transaction requests it can't answer.
But we should tell peers we don't have that inventory, so they can try another peer.

Solution

send notfound when Zebra doesn't have a block or transaction
allow different available and missing types inside ResponseStatus
stop sending unsupported missing inventory types to the registry

Related non-feature fixes:

move address sanitization into the response future
add a block_hash convenience method to InventoryHash
improve failure logs for connection tests
move module docs to the top of each module

Tests:

inbound messages are forwarded to the registry
Zebra responds with notfound block messages to peers - single block request
Zebra responds with notfound transaction messages to peers (including Tx and Wtx) - multi-transaction request

Closes #2726.

Review

I'd like reviews to focus on the functionality changes.
(I will open another PR to deal with some edge cases around peers not sending notfound, and Zebra cancelling requests.)

@jvff can review this PR.

This PR is based on PR #3465.

Reviewer Checklist

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

Follow Up Work

Security: Send notfound inv items to the inv collector, Credit: Equilibrium #2156
- if a block or transaction request fails, times out, or is dropped, send a synthetic notfound message to the inv collector

codecov · 2022-02-07T01:55:27Z

Codecov Report

Merging #3466 (6c9994c) into main (499ae89) will increase coverage by 1.95%.
The diff coverage is 77.56%.

@@            Coverage Diff             @@
##             main    #3466      +/-   ##
==========================================
+ Coverage   78.34%   80.29%   +1.95%     
==========================================
  Files         267      275       +8     
  Lines       31526    32054     +528     
==========================================
+ Hits        24698    25738    +1040     
+ Misses       6828     6316     -512

zebrad/src/components/inbound.rs

teor2345 · 2022-02-08T09:01:31Z

@jvff this should be ready for review now.

I'd like to focus on the functionality changes, because this PR series has taken longer than I expected.
So if we have to leave cleanups and refactors for later, that's ok.

teor2345 · 2022-02-08T23:04:08Z

Failed due to #3489:
https://github.com/ZcashFoundation/zebra/runs/5115656563?check_suite_focus=true#step:10:1148

zebra-network/src/peer/minimum_peer_version.rs

zebra-network/src/protocol/internal/response_status.rs

zebra-network/src/peer_set/inventory_registry.rs

zebra-network/src/peer_set/inventory_registry/tests/prop.rs

zebrad/src/components/inbound.rs

zebra-network/src/peer/connection.rs

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

jvff

I left a lot of comments, but I don't think any of them should block merging the PR 🤔

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

zebra-network/src/protocol/internal/response_status.rs

```sh fastmod Advertised Available zebra* fastmod advertised available zebra* ```

… an InventoryStatus And rename it to ResponseStatus. Split the methods between ResponseStatus and an InventoryStatus alias.

…action

This makes them more likely to get updated when the module changes.

… registry

…nd via TCP

The code that these tests use hasn't actually changed much, and they are only failing on some platforms (coverage, macOS). So it seems like the extra concurrent inbound tests have pushed them past their time limit. (Perhaps due to TCP system calls, or extra serialization work.)

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

This prevents `MockService<zebra_state>` timeouts in the `sync_block_too_high_extend_tips` test, at the cost of reducing coverage of different execution orders.

teor2345 · 2022-02-13T22:44:51Z

@jvff thanks for the reviews.

This PR series took much longer than I expected to write and test, so I'd really like to get it merged.
Are there any major issues with the design, or any bugs you can see?

teor2345 added C-enhancement Category: This is an improvement P-Medium ⚡ A-network Area: Network protocol updates or fixes labels Feb 4, 2022

teor2345 self-assigned this Feb 4, 2022

teor2345 force-pushed the notfound-send branch from 0efc271 to f6217cf Compare February 4, 2022 07:23

teor2345 mentioned this pull request Feb 6, 2022

1. Create an API for a missing inventory registry, but don't register any missing inventory yet #3255

Merged

8 tasks

teor2345 force-pushed the notfound-route branch from b99d195 to cb561d3 Compare February 7, 2022 00:35

teor2345 force-pushed the notfound-send branch from f6217cf to 389c828 Compare February 7, 2022 00:37

teor2345 requested a review from jvff February 7, 2022 02:31

teor2345 force-pushed the notfound-send branch from 2fad60e to 09d0f12 Compare February 7, 2022 03:02

teor2345 marked this pull request as ready for review February 7, 2022 03:03

teor2345 commented Feb 7, 2022

View reviewed changes

zebrad/src/components/inbound.rs Show resolved Hide resolved

teor2345 force-pushed the notfound-send branch from 09d0f12 to 0a32208 Compare February 7, 2022 20:31

teor2345 force-pushed the notfound-route branch 2 times, most recently from 2e7f394 to e049a17 Compare February 7, 2022 22:25

teor2345 force-pushed the notfound-send branch from 0a32208 to ab9b09c Compare February 7, 2022 22:26

Base automatically changed from notfound-route to main February 8, 2022 01:16

teor2345 force-pushed the notfound-send branch 4 times, most recently from 45fce08 to 47880aa Compare February 8, 2022 09:00

teor2345 force-pushed the notfound-send branch from 47880aa to 70395ee Compare February 8, 2022 20:24

jvff reviewed Feb 9, 2022

View reviewed changes

zebra-network/src/peer/minimum_peer_version.rs Outdated Show resolved Hide resolved

teor2345 force-pushed the notfound-send branch from f00e190 to dfeda1c Compare February 9, 2022 04:46

jvff reviewed Feb 10, 2022

View reviewed changes

zebra-network/src/protocol/internal/response_status.rs Show resolved Hide resolved

zebra-network/src/protocol/internal/response_status.rs Show resolved Hide resolved

teor2345 mentioned this pull request Feb 10, 2022

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

Merged

3 tasks

jvff reviewed Feb 10, 2022

View reviewed changes

jvff previously approved these changes Feb 10, 2022

View reviewed changes

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

teor2345 dismissed jvff’s stale review via 7f81526 February 11, 2022 00:28

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

teor2345 requested a review from jvff February 11, 2022 00:57

jvff reviewed Feb 12, 2022

View reviewed changes

teor2345 and others added 15 commits February 14, 2022 08:41

refactor(network): rename Advertised to Available

c8e847e

```sh fastmod Advertised Available zebra* fastmod advertised available zebra* ```

refactor(network): allow different available and missing types inside…

11a4ba9

… an InventoryStatus And rename it to ResponseStatus. Split the methods between ResponseStatus and an InventoryStatus alias.

refactor(network): add a block_hash convenience method to InventoryHash

3ce4337

test(network): improve failure logs for connection tests

bdba11b

fix(inbound): move address sanitization into the response future

b1c2cc3

feat(network): send notfound when Zebra doesn't have a block or trans…

2f2869f

…action

doc(network): move module docs to the top of each module

9b565cd

This makes them more likely to get updated when the module changes.

fix(network): stop sending unsupported missing inventory types to the…

9d43afe

… registry

test(network): inbound messages are forwarded to the registry

9b0f243

test(inbound): test Peers requests to the inbound service, directly a…

4afd681

…nd via TCP

test(network): notfound block responses are sent by the inbound service

9e9dbf5

test(network): notfound tx responses are sent by the inbound service

c161385

doc(network): fix typo

5e196a7

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

test(network): remove unnecessary multi-threaded runtime from tests

6c9994c

This prevents `MockService<zebra_state>` timeouts in the `sync_block_too_high_extend_tips` test, at the cost of reducing coverage of different execution orders.

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

jvff approved these changes Feb 14, 2022

View reviewed changes

mergify bot merged commit 9f2028f into main Feb 14, 2022

mergify bot deleted the notfound-send branch February 14, 2022 01:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

teor2345 commented Feb 4, 2022 •

edited

Loading

codecov bot commented Feb 7, 2022 •

edited

Loading

teor2345 commented Feb 8, 2022

teor2345 commented Feb 8, 2022

jvff left a comment

teor2345 commented Feb 13, 2022

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

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

Conversation

teor2345 commented Feb 4, 2022 • edited Loading

Motivation

Solution

Review

Reviewer Checklist

Follow Up Work

codecov bot commented Feb 7, 2022 • edited Loading

Codecov Report

teor2345 commented Feb 8, 2022

teor2345 commented Feb 8, 2022

jvff left a comment

Choose a reason for hiding this comment

teor2345 commented Feb 13, 2022

teor2345 commented Feb 4, 2022 •

edited

Loading

codecov bot commented Feb 7, 2022 •

edited

Loading