Page MenuHomePhabricator

[avalanche] Limit polling of finalized items of all types
ClosedPublic

Authored by sdulfari on Mar 1 2023, 23:58.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC478b00b20c20: [avalanche] Limit polling of finalized items of all types
Summary

There is currently no case where polling a finalized item more than once is desirable.
This patch limits the polling for all finalized types, not just invalidated blocks. Invalidated
items of types other than blocks are not tracked due to transient factors (for example,
immature proofs may be invalidated but once mature they can be finalized). And future changes
to voting policies on non-block items makes this challenging to navigate. However we can be sure
that once a block is invalidated we don't care about descendants. We will not reorg to an
invalidated chain.

Rationale on the filter size:
Using BTC as a proxy, block propagation for 90 percentile has been under 12 seconds for the last few years: https://www.dsn.kastel.kit.edu/bitcoin/#propagation
Assuming each cycle of polling finalizes a set of items in ~3 seconds, we assume 15 seconds is a window of time we can be reasonably sure a <1MB block will finalize across the network.
It is desirable to not allow repolling of recently finalized blocks (see D13207), so it is essential that this 15 second window be covered by the filter and not allow
the block to roll out of it prematurely. 20 polling cycles (128+ rounds to reach finalization for each "cycle") should take ~60 seconds so this should be sufficient even if you tweak assumptions, such as:

  • eCash can have larger blocks and therefore slower block propagation time.
  • The eCash network is smaller than BTC and may have less optimal paths that impact 90 percentile propagation time.

Depends on D13207

Test Plan
ninja check check-functional

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

OK I see, so the idea is to have all the items being tracked when they are in a final vote state (being finalized or invalidated) and if so don't add them to reconcile anymore.

refactors and unit testing

sdulfari published this revision for review.Mar 9 2023, 00:56
sdulfari edited the test plan for this revision. (Show Details)

Why not use the registerVotes and addToReconcile API like the other tests, rather than massaging the internals ?

src/avalanche/processor.cpp
558 ↗(On Diff #38358)

That's a bit of a shame we have to call GetVoteItemId here as the above code is doing the exact opposite already

986 ↗(On Diff #38358)

That was the right place

src/avalanche/processor.h
363 ↗(On Diff #38358)

That's the wrong place for this test

src/avalanche/voterecord.h
50 ↗(On Diff #38358)

that's not needed

Fabien requested changes to this revision.Mar 9 2023, 11:10
This revision now requires changes to proceed.Mar 9 2023, 11:10
  • Address feedback
  • Restrict filtering to finalized items of any type and invalidated blocks only. See comments for rationale.

Why not use the registerVotes and addToReconcile API like the other tests, rather than massaging the internals ?

Because the number of rounds is very large the test becomes painfully slow. Massaging the internals allows quick finalization of many items.

Fabien requested changes to this revision.Mar 15 2023, 09:13

20 polling cycles should take ~60 seconds

That has to be wrong for finalization (that can take more than 20 rounds) to occur within 3s

You should keep the filters separated, it's hard to follow when you have all the finalized items but a single invalidated kind.

src/avalanche/test/processor_tests.cpp
434 ↗(On Diff #38575)

It's first used in item_reconcile_twice, so move it above that test

This revision now requires changes to proceed.Mar 15 2023, 09:13

Feedback + reworking some comments

Will clarify the summary.

sdulfari retitled this revision from [avalanche] Generalize the limiting of polling already polled items for all types to [avalanche] Limit polling of finalized items of all types.
This revision is now accepted and ready to land.Mar 16 2023, 13:45