-
Notifications
You must be signed in to change notification settings - Fork 29
feat: keep older vote messages for leaders in consensus pool #514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| } | ||
| None => vote_slot, | ||
| }; | ||
| if slot_to_check < root_slot { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this < here, whereas we use <= in BLSVerifier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, I wanted to ask the original authors about that as well.
Looking at: 308af20#diff-2077122be05c942393f87ed53352bb244607026554c88290d4d76b98c1124801R197 and 7e8e58b#diff-ea37759cc14ea84779ba69701cf8244e8314ad695cb412f76a4e3f103d57ffa0R129, it seems like @wen-coding is the author in both cases.
@wen-coding: do you know why we use < here and <= in BLSVerifier? I would've expected us to use <= in both cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was worried that changing <= to < in BLSVerifier will block all the standstill messages related to root so we don't know how many others are agreeing with me. We don't use it for now, but maybe we want to output a warning "I'm in standstill and how many people are at the same slot with me".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have finalized a block in slot s, even during standstill there is no relevant information gained by seeing a finalization from someone for slot s. Sending the finalization during standstill only matters for nodes that are actually behind to catch up to the latest finalized slot.
|
Some ideas I had about packing aggregates into the block footer:
Separates the rewards tracking from the main consensus tracking so we don't have to mess with the ConsensusPool. Maybe wrap the |
|
I like the idea, not polluting consensus to handle rewards makes sense to me! |
This was the original plan, to handle the rewards with a separate program. Somebody told us not to do that. In any case, I'm still a bit insecure about the hole one vote is enough for 100% rewards plan. I'm trying to cook something better. |
@rogerANZA : by separate program do you mean a separate smart contract? The proposal from Ashwin is to use a separate rust component inside anza.
Cool, let us know how you proceed. |
Problem
If we discard vote messages for any slots earlier than root slot, then the node that will be the leader in 8 slots will not be able to include them in the certs for rewards.
Summary of Changes
Similar to #513, this updates the consensus pool to keep older votes.
Part of #406