Paths

Table of Contentst

Differential D16782

[avalanche] Implement cleanup for stake contender cache
ClosedPublic
Actions

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

Tags

None

Subscribers

Details

Reviewers

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

Branch

sw-cleanup

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 30389
Build 60298: Build Diff	lint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang · build-clang-tidy
Build 60297: arc lint + arc unit

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

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

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.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

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

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

Path

Size

src/

avalanche/

stakecontendercache.h

54 lines

stakecontendercache.cpp

57 lines

test/

stakecontendercache_tests.cpp

107 lines

Diff 49779

src/avalanche/stakecontendercache.h

Loading...

src/avalanche/stakecontendercache.cpp

Loading...

src/avalanche/test/stakecontendercache_tests.cpp

Loading...