Page MenuHomePhabricator
Differential D9582

[avalanche] Answer proof invs with getdata proof requests
ClosedPublic
Actions

Authored by Fabien on May 25 2021, 15:41.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Restricted Maniphest Task
Commits
rABCcef8cbca39d8: [avalanche] Answer proof invs with getdata proof requests
Summary

This makes use of the InvRequestTracker for tracking proof inventories.
This is similar to what is done for the transactions as demonstrated by
running the same functional tests.

This is missing the peer disconnection and the notfound management which
will be in their own diff.

Depends on D9598 and D9608.

Test Plan
ninja all check-all
./test/functional/test_runner.py p2p_inv_download
../contrib/teamcity/build-configurations.py build-tsan

Also run p2p_inv_download under TSAN.

Diff Detail

Event Timeline

Fabien created this revision.May 25 2021, 15:41
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 25 2021, 15:41
Fabien requested review of this revision.May 25 2021, 15:41
Harbormaster completed remote builds in B15803: Diff 28598.May 25 2021, 15:55
Fabien updated this revision to Diff 28602.May 25 2021, 18:12

Comment on the tsan exception so we don't forget

Harbormaster completed remote builds in B15807: Diff 28602.May 25 2021, 18:23
Mengerian added a task: Restricted Maniphest Task.May 26 2021, 15:35
deadalnix requested changes to this revision.May 26 2021, 15:37
deadalnix added a subscriber: deadalnix.

This is too big. You are touching a ton of thing everywhere here. I'm sure you can extract many refactoring from there. It doesn't matter if they are trivial, in fact that's the point, they can be landed right away and non trivial things aren't drowned into noise.

src/avalanche/peermanager.h
178 ↗(On Diff #28602)

You should send patch for these independently. Mixing them only make these fix land later at best, at worse create horrible merge conflicts.

This revision now requires changes to proceed.May 26 2021, 15:37
Fabien edited the summary of this revision. (Show Details)May 28 2021, 12:34
Fabien edited the test plan for this revision. (Show Details)
Fabien added parent revisions: D9598: Extract inv saturation check and data request time computation, D9595: [avalanche] Add an API to store and retrieve a proof.
Fabien removed parent revisions: D9562: Rename public interfaces from Tx to Inv in InvRequestTracker, D9580: Rename p2p_tx_download to p2p_inv_download.
Fabien updated this revision to Diff 28660.May 28 2021, 12:37

Rebase, split out the disconnection and notfound management to keep it as tiny as possible

Harbormaster completed remote builds in B15841: Diff 28660.May 28 2021, 12:49
Fabien added a child revision: D9599: [avalanche] Manage the notfound messages from a proof request.May 28 2021, 13:11
deadalnix requested changes to this revision.May 28 2021, 13:11
deadalnix added inline comments.
src/init.cpp
1240

Why?

src/net_processing.cpp
953

This where this breaks down, doesn't it?

I pointed it in the other patch, so why do I have to point it again here?

This revision now requires changes to proceed.May 28 2021, 13:11
Mengerian added a task: Restricted Maniphest Task.May 28 2021, 16:31
Fabien updated this revision to Diff 28730.Jun 2 2021, 19:40

Remove the no longer used flag and rebase on top of D9608 to fix the permission inconsistency

Fabien edited the summary of this revision. (Show Details)Jun 2 2021, 19:40
Fabien added a parent revision: D9608: [avalanche] Add an avaproof network permission for bypassing the inv tracking limit.
Fabien removed a parent revision: D9595: [avalanche] Add an API to store and retrieve a proof.
Harbormaster completed remote builds in B15890: Diff 28730.Jun 2 2021, 19:52
Fabien updated this revision to Diff 28735.Jun 3 2021, 10:13

Also download the proofs during IBD. They will likely end up in the orphan pool but that should not be an issue.

Fabien added a child revision: D9625: [avalanche] Add a facility to check if a peer is preferred for DL.Jun 3 2021, 10:19
Harbormaster completed remote builds in B15894: Diff 28735.Jun 3 2021, 10:30
deadalnix requested changes to this revision.Jun 3 2021, 15:42
deadalnix added inline comments.
src/init.cpp
1240 ↗(On Diff #28735)

???

test/functional/p2p_inv_download.py
271 ↗(On Diff #28735)

Why are these skipped?

This revision now requires changes to proceed.Jun 3 2021, 15:42
Fabien updated this revision to Diff 28744.Jun 3 2021, 15:50

Revert the empty line change in init.cpp

src/init.cpp
1240 ↗(On Diff #28735)

oversight from my self review after removing the flag that used to be here

test/functional/p2p_inv_download.py
271 ↗(On Diff #28735)

These skipped tests are the parts I splitted out in D9599 and D9600 which enable them back.

Harbormaster completed remote builds in B15901: Diff 28744.Jun 3 2021, 16:11
deadalnix requested changes to this revision.Jun 4 2021, 10:01
deadalnix added inline comments.
test/functional/p2p_inv_download.py
271 ↗(On Diff #28735)

That doesn't explain why. Disabling tests in this way is suspicious, there needs to be a good reason.

This revision now requires changes to proceed.Jun 4 2021, 10:01
Fabien requested review of this revision.Jun 4 2021, 11:54
Fabien added inline comments.
test/functional/p2p_inv_download.py
271 ↗(On Diff #28735)

They are not disabled, but not enabled yet. This does not remove any coverage since the tests did not run on proofs before this diff.
The reason I split the diff in 3 is solely to make the review easier (because it was pretty large otherwise). You greened the 2 other parts already but I can always squash if you prefer.

deadalnix accepted this revision.Jun 4 2021, 16:50
This revision is now accepted and ready to land.Jun 4 2021, 16:50
Closed by commit rABCcef8cbca39d8: [avalanche] Answer proof invs with getdata proof requests (authored by Fabien). · Explain WhyJun 4 2021, 19:34
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rABCcef8cbca39d8: [avalanche] Answer proof invs with getdata proof requests.
Fabien mentioned this in D9498: [avalanche] Reply to proof invs with getdata requests.Jun 4 2021, 19:56

Revision Contents
Changeset List

PathSize
src/
4 lines
14 lines
85 lines
4 lines
test/
functional/
106 lines

Diff 28660

View Options

src/init.cpp

Loading...
View Options

src/net_processing.h

Loading...
View Options

src/net_processing.cpp

Loading...
View Options

src/protocol.h

Loading...
View Options

test/functional/p2p_inv_download.py

Loading...