Page MenuHomePhabricator

[avalanche] Implement cleanup for stake contender cache
ClosedPublic

Authored by roqqit on Wed, Sep 18, 16:24.

Details

Reviewers
Fabien
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC2f0cd61946ba: [avalanche] Implement cleanup for stake contender cache
Summary

The stake contender cache needs a way to be cleaned up so that it does not grow unbounded. The intent is to call cleanup() when a block becomes newly finalized since there is no need to maintain the cache for finalized blocks.

Test Plan
ninja check-avalanche

Diff Detail

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

Event Timeline

Owners added a reviewer: Restricted Owners Package.Wed, Sep 18, 16:24
roqqit requested review of this revision.Wed, Sep 18, 16:24
Fabien requested changes to this revision.Wed, Sep 18, 19:05
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/avalanche/stakecontendercache.cpp
27 ↗(On Diff #49715)

I think you can make this more effective and not require cs_main by adding the block height to the cache. Manual winners could also be made a multi index to achieve the same

This revision now requires changes to proceed.Wed, Sep 18, 19:05

changes per feedback to remove the cs_main lock. now depends on D16793

Fabien requested changes to this revision.Fri, Sep 20, 09:52

I see your test doesn't check for contiguous heights, you should add that

src/avalanche/stakecontendercache.cpp
13 ↗(On Diff #49754)

Should be mwHeightView.lower_bound(minHeight) ? Otherwise you will miss the last minHeight-1 entry.

19 ↗(On Diff #49754)

same

This revision now requires changes to proceed.Fri, Sep 20, 09:52

Use lower_bound instead since it is more technically correct and robust to future changes. There is no change in behavior.

I see your test doesn't check for contiguous heights, you should add that

Talked offline. The tests do this already

This revision is now accepted and ready to land.Mon, Sep 23, 16:34