[avalanche] Add a cache for avalanche voting results of stake contenders
ClosedPublic
Actions

Authored by roqqit on Aug 29 2024, 18:09.

Details

Reviewers

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

rABCb632dee70503: [avalanche] Add a cache for avalanche voting results of stake contenders

Summary

It is not possible to guarantee knowledge of the entire proof set with avalanche. As such, converging on exactly one stake winner is not possible in all scenarios. We need a way to track stake contenders in such a way that will converge on as few winners as possible. This is the first piece of a cache to achieve that.

The cache also tracks manually added stake winners (ie. via RPC) since there is currently no memory of past stake winners once a new block is found. This memory is important so that nodes can reply to stake contender avalanche requests of recent past blocks (we cannot guarantee the entire network has finalized the same block in lock step).

Missing pieces that will appear in future diffs include cleanup of past blocks and integration with avalanche voting.

Depends on D16694

Test Plan

ninja check-avalanche

Diff Detail

Repository

rABC Bitcoin ABC

Branch

sw-cache

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 30196
Build 59920: Build Diff	build-diff · build-clang · build-debug · build-clang-tidy · build-without-wallet · lint-circular-dependencies
Build 59919: arc lint + arc unit

Event Timeline

roqqit created this revision.Aug 29 2024, 18:09

Owners added a reviewer: Restricted Owners Package.Aug 29 2024, 18:09

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 29 2024, 18:09

roqqit requested review of this revision.Aug 29 2024, 18:09

Harbormaster completed remote builds in B30133: Diff 49413.Aug 29 2024, 18:33

Fabien requested changes to this revision.Aug 30 2024, 20:47

Fabien added a subscriber: Fabien.

Fabien added inline comments.

src/avalanche/stakecontendercache.h
25 ↗	(On Diff #49413)	Nit: unknown is better than default here
39 ↗	(On Diff #49413)	that's an uint32_t ?
89 ↗	(On Diff #49413)	This should be a multimap ? There can be several acceptable winners set manually for the same block

This revision now requires changes to proceed.Aug 30 2024, 20:47

DEFAULT -> UNKNOWN
fix score type

roqqit edited the summary of this revision. (Show Details)Sep 3 2024, 17:06

roqqit added inline comments.

src/avalanche/stakecontendercache.h
39 ↗	(On Diff #49413)	yes. search and replace error
89 ↗	(On Diff #49413)	Multimap needs a comparator, but with manually added winners you want to mirror the behavior the RPC uses which is the first manual winner is always selected for GBT.

Harbormaster completed remote builds in B30164: Diff 49463.Sep 3 2024, 17:22

This memory is important so that nodes can reply to stake contender avalanche requests of recent past blocks (we cannot guarantee the entire network has finalized the same block in lock step).

Is it really ? If a block has finalized for some nodes but not others, what is the point of voting on past block rewards ? If the node votes idk the block will eventually be finalized by the poller anyway so the outcome of this stake vote doesn't matter. The only case I think it is helpful is if you get a bunch of blocks in a row, faster than finalization time. But as soon as a block is final you can forget about winners for it and past blocks.

src/avalanche/stakecontendercache.cpp
85 ↗	(On Diff #49463)	I think you'll need a lock on the ContenderSet. You are going to store pointers here and I double the call to getWinners and the one to cleanup the cache will be done in the same thread, so a null dereference is just a matter of time
100 ↗	(On Diff #49413)	This is incorrect, manualWinners.size() is equal to the number of blockhash keys. You only want +0 or +1
110 ↗	(On Diff #49413)
src/avalanche/stakecontendercache.h
89 ↗	(On Diff #49413)	I still don't get it. This means that if you manually added 2 winners, your node will only vote ok for the first one not the other ? This makes the RPC non functional then.

This revision now requires changes to proceed.Sep 4 2024, 07:52

In D16695#378856, @Fabien wrote:

This memory is important so that nodes can reply to stake contender avalanche requests of recent past blocks (we cannot guarantee the entire network has finalized the same block in lock step).

Is it really ? If a block has finalized for some nodes but not others, what is the point of voting on past block rewards ? If the node votes idk the block will eventually be finalized by the poller anyway so the outcome of this stake vote doesn't matter. The only case I think it is helpful is if you get a bunch of blocks in a row, faster than finalization time. But as soon as a block is final you can forget about winners for it and past blocks.

The past view is relative. Say you have nodes in the network in sets A and B and chain X <- Y (tip). A and B have both finalized X. Only B has finalized Y. A polls for block Y and for stake winners based on X. As this is happening, other nodes in set A are also polling. If set A is large, convergence will be slow if none of them can determine if the stake winners are valid for Y. By allowing them to poll B nodes for block-after-X stake winners, we are encouraging convergence for block Y. In a more extreme environment, the set B may be empty, so all nodes are polling for Y but unable to finalize it because not enough nodes agree on the stake winner.

Put another way, if the proof set is uneven across the network, preconsensus on stake winners encourages convergence for blocks.

src/avalanche/stakecontendercache.cpp
85 ↗	(On Diff #49463)	I was intending to have an avalanche processor level lock on the entire cache. Is it preferable for the cache to own it's own lock?
src/avalanche/stakecontendercache.h
89 ↗	(On Diff #49413)	Manually added winners are not intended to be polled at all. The RPC is an intervention by the user saying "I want to accept this stake winner regardless of what the network thinks"

Fabien added inline comments.Sep 4 2024, 20:41

src/avalanche/stakecontendercache.cpp
85 ↗	(On Diff #49463)	Depends on your design. If there is no sequence of operations that require the lock to be hold in between it's easier to have the lock at the cache level
src/avalanche/stakecontendercache.h
89 ↗	(On Diff #49413)	Say your node is missing the winner so you add it via RPC. Other can poll for this and there is no reason for your node to vote against it since you added it manually

roqqit added inline comments.Sep 5 2024, 16:03

src/avalanche/stakecontendercache.cpp
85 ↗	(On Diff #49463)	The sequences I know so far are finalize or invalidate a stake contender and then immediately call getWinners to regenerate the winner set. I will stick with the processor-level lock for now.
100 ↗	(On Diff #49413)	good catch, but the number of manual winners is entirely dependent on how many were added via RPC. can definitely be more than 1
src/avalanche/stakecontendercache.h
89 ↗	(On Diff #49413)	This is a good point and I can incorporate this into the voting logic. I don't think that changes the design here though.