Page MenuHomePhabricator
Differential D16782

[avalanche] Implement cleanup for stake contender cache
ClosedPublic
Actions

Authored by roqqit on Sep 18 2024, 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

Event Timeline

roqqit created this revision.Sep 18 2024, 16:24
Owners added a reviewer: Restricted Owners Package.Sep 18 2024, 16:24
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 18 2024, 16:24
roqqit requested review of this revision.Sep 18 2024, 16:24
Harbormaster completed remote builds in B30347: Diff 49715.Sep 18 2024, 16:47
Fabien requested changes to this revision.Sep 18 2024, 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.Sep 18 2024, 19:05
roqqit mentioned this in D16793: [avalanche] Use block index instead of blockhash for adding contenders to the cache.Sep 20 2024, 00:14
roqqit updated this revision to Diff 49754.Sep 20 2024, 00:18

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

roqqit added a parent revision: D16793: [avalanche] Use block index instead of blockhash for adding contenders to the cache.Sep 20 2024, 00:18
Harbormaster completed remote builds in B30372: Diff 49754.Sep 20 2024, 00:44
Fabien requested changes to this revision.Sep 20 2024, 09:52

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

src/avalanche/stakecontendercache.cpp
13

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

19

same

This revision now requires changes to proceed.Sep 20 2024, 09:52
roqqit mentioned this in rABCb5fe46c6e31d: [avalanche] Use block index instead of blockhash for adding contenders to the….Sep 20 2024, 16:57
roqqit updated this revision to Diff 49779.Sep 23 2024, 16:09

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

roqqit added a comment.Sep 23 2024, 16:13
In D16782#381036, @Fabien wrote:

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

Talked offline. The tests do this already

Harbormaster completed remote builds in B30389: Diff 49779.Sep 23 2024, 16:33
Fabien accepted this revision.Sep 23 2024, 16:34
This revision is now accepted and ready to land.Sep 23 2024, 16:34
Closed by commit rABC2f0cd61946ba: [avalanche] Implement cleanup for stake contender cache (authored by roqqit). · Explain WhySep 23 2024, 20:00
This revision was automatically updated to reflect the committed changes.
roqqit added a commit: rABC2f0cd61946ba: [avalanche] Implement cleanup for stake contender cache.

Revision Contents
Changeset List

PathSize
src/
avalanche/
54 lines
57 lines
test/
107 lines

Diff 49754

View Options

src/avalanche/stakecontendercache.h

Loading...
View Options

src/avalanche/stakecontendercache.cpp

Loading...
View Options

src/avalanche/test/stakecontendercache_tests.cpp

Loading...