Details

Reviewers

deadalnix
sdulfari

Group Reviewers

Restricted Project

Commits

rABCc510b7864d32: [avalanche] Don't process proofs that are not worth polling

Summary

It is possible for a proof to turn orphan while being voted on if a block is connected in the meantime. In this case it becomes pointless to keep polling for it. More generally it's more robust to also discard polls for proofs that get dropped, even if there should be no such case.

Depends on D11641.

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Fabien created this revision.Jun 16 2022, 21:35

Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 16 2022, 21:35

Fabien requested review of this revision.Jun 16 2022, 21:35

Harbormaster completed remote builds in B19396: Diff 34033.Jun 16 2022, 21:51

deadalnix requested changes to this revision.Jun 17 2022, 12:33

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

src/avalanche/processor.cpp
339 ↗	(On Diff #34033)	Why would you take the lock before doing a null check? That's a no.

This revision now requires changes to proceed.Jun 17 2022, 12:33

Move the null check outside of the lock

Harbormaster completed remote builds in B19400: Diff 34037.Jun 17 2022, 14:14

Fabien added a child revision: D11637: [avalanche] Remove obsolete orphan proofs.Jun 17 2022, 15:54

sdulfari requested changes to this revision.Jun 17 2022, 17:52

sdulfari added a subscriber: sdulfari.

sdulfari added inline comments.

src/avalanche/test/processor_tests.cpp
777 ↗	(On Diff #34037)	After invalidating, it doesn't look like the test is actually checking to see if the polling for itemB stopped. Only that further votes on itemA succeed. In the least, shouldn't this check that votes on itemB fail?
1125 ↗	(On Diff #34037)	nit: other tests organize buildProof args as (outpoint, sequence)
1134 ↗	(On Diff #34037)	remove
1143 ↗	(On Diff #34037)	This isn't registering proofs, but attempting to poll
1151 ↗	(On Diff #34037)	Did you intend to check the state?

This revision now requires changes to proceed.Jun 17 2022, 17:52

sdulfari added inline comments.Jun 17 2022, 18:41

src/avalanche/processor.cpp
686 ↗	(On Diff #34037)	Can simplify to remove the else clause: if (!IsWorthPolling(it->first)) { w->erase(it); } ++it;

sdulfari added inline comments.Jun 17 2022, 18:54

src/avalanche/processor.cpp
686 ↗	(On Diff #34037)	`it` becomes invalidated in the erase case, so this is actually wrong.

Fabien added inline comments.Jun 17 2022, 19:30

src/avalanche/test/processor_tests.cpp
777 ↗	(On Diff #34037)	it does indirectly. It it was expecting the itemB being voted on then registerVotes would have returned an error
1151 ↗	(On Diff #34037)	A debugging leftover that helped me figure out that I needed to disable the replacement cooldown

Feedback

Harbormaster completed remote builds in B19408: Diff 34045.Jun 17 2022, 19:50

deadalnix added inline comments.Jun 18 2022, 21:56

src/avalanche/processor.cpp
38 ↗	(On Diff #34045)	This does not follow the naming convention in this class.
366 ↗	(On Diff #34045)	Move down.

Fabien mentioned this in D11641: [avalanche] Make IsWorthPolling a private method of the processor.Jun 20 2022, 08:18

Move the boolean declaration closer to its use and rebase on top of D11641 to extract the renaming of the existing method out of this diff.

Fabien edited the summary of this revision. (Show Details)Jun 20 2022, 08:24

Fabien added a parent revision: D11641: [avalanche] Make IsWorthPolling a private method of the processor.

Harbormaster completed remote builds in B19418: Diff 34057.Jun 20 2022, 09:00

sdulfari accepted this revision.Jun 20 2022, 14:36

deadalnix requested changes to this revision.Jun 21 2022, 00:50

deadalnix added inline comments.

src/avalanche/processor.cpp
940 ↗	(On Diff #34057)	This just breaks everything. As soon as you invalidate a proof, you'll stop polling. the network has no chance to reconciliate anything.

This revision now requires changes to proceed.Jun 21 2022, 00:50

Fabien requested review of this revision.Jun 22 2022, 20:10

Fabien added inline comments.

src/avalanche/processor.cpp
940 ↗	(On Diff #34057)	As per our discussion, this code considers the proof is not worth polling if: there is a PeerManager::Peer attached to the proof (which is equivalent to the proof being valid, whether there are nodes attached to it or not) the proof is conflicting So it's not worth polling: orphans proofs that we don't have anymore at all, like after a cleanup