Page MenuHomePhabricator

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

Authored by Fabien on Jun 16 2022, 21:35.

Details

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
Branch
avalanche_isworthpolling_proof
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 19408
Build 38549: Build Diffbuild-without-wallet · lint-circular-dependencies · build-clang-tidy · build-debug · build-clang · build-diff
Build 38548: arc lint + arc unit

Event Timeline

Fabien requested review of this revision.Jun 16 2022, 21:35
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

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
src/avalanche/processor.cpp
686 ↗(On Diff #34037)

Can simplify to remove the else clause:

if (!IsWorthPolling(it->first)) {
    w->erase(it);
}
++it;
src/avalanche/processor.cpp
686 ↗(On Diff #34037)

it becomes invalidated in the erase case, so this is actually wrong.

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

src/avalanche/processor.cpp
38

This does not follow the naming convention in this class.

366

Move down.

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.

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
This revision is now accepted and ready to land.Jun 26 2022, 23:35