Page MenuHomePhabricator

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

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

Details

Reviewers
Fabien
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

Event Timeline

Owners added a reviewer: Restricted Owners Package.Aug 29 2024, 18:09
roqqit requested review of this revision.Aug 29 2024, 18:09
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

Nit: unknown is better than default here

39

that's an uint32_t ?

89

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 added inline comments.
src/avalanche/stakecontendercache.h
39

yes. search and replace error

89

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.

Fabien requested changes to this revision.Sep 4 2024, 07:52

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

This is incorrect, manualWinners.size() is equal to the number of blockhash keys. You only want +0 or +1

110
src/avalanche/stakecontendercache.h
89

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

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

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"

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

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

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

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

This is a good point and I can incorporate this into the voting logic. I don't think that changes the design here though.

  • const auto & where appropriate
  • fix payouts.reserve() sizing
This revision is now accepted and ready to land.Sep 5 2024, 16:27