[avalanche] Add a way to promote old entries in the contender cache to a new chain tip
ClosedPublic
Actions

Authored by roqqit on Oct 21 2024, 18:36.

Details

Reviewers

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

rABCff7cabede390: [avalanche] Add a way to promote old entries in the contender cache to a new…

Summary

Remote proofs are only tracked by proofid in the peer manager, so the contender cache needs to continuously "promote" cache entries since it does not store the proofs themselves. Each time promotion is attempted, an indirect validity check is done to ensure the cache is not tracking invalid proofs.

Test Plan

ninja check-avalanche-stakecontendercache_tests

Diff Detail

Repository

rABC Bitcoin ABC

Branch

sw-promotetoblock

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 30783
Build 61080: Build Diff	lint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang-tidy · build-clang
Build 61079: arc lint + arc unit

Event Timeline

roqqit created this revision.Oct 21 2024, 18:36

Owners added a reviewer: Restricted Owners Package.Oct 21 2024, 18:36

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 21 2024, 18:36

roqqit requested review of this revision.Oct 21 2024, 18:36

Harbormaster completed remote builds in B30713: Diff 50252.Oct 21 2024, 18:55

Fabien requested changes to this revision.Oct 22 2024, 13:22

Fabien added a subscriber: Fabien.

Fabien added inline comments.

src/avalanche/stakecontendercache.h
144 ↗	(On Diff #50252)	I don't understand the point of this function. Do you need to do something different that what is in the summary ? Otherwise can't you just inline the function ? If it's a dependency issue then it's a hint that the check doesn't belong to the stake contender but rather the peer manager.
src/avalanche/test/stakecontendercache_tests.cpp
383 ↗	(On Diff #50252)	Nit, here and below

This revision now requires changes to proceed.Oct 22 2024, 13:22

roqqit planned changes to this revision.Oct 22 2024, 20:15

roqqit added inline comments.

src/avalanche/stakecontendercache.h
144 ↗	(On Diff #50252)	The point is to check the peer manager if the proof is still valid. An alternative is to pass the peer manager in and let the cache do the checks that it wants, but that didn't seem right for two reasons: Unit testing is more complicated since it needs to introduce a peer manager where right now there is none. The cs_peerManager lock would be taken at the callsite to promoteToBlock. This seemed like a more important reason when I wrote this iteration, but now with fresh eyes I do think it's a weak argument.