-
Notifications
You must be signed in to change notification settings - Fork 30
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
[HOTFIX] Check Relay Number when Accumulating View Sync Votes #2470
Conversation
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.
This seems to implement exactly what we talked about yesterday. Someone with consensus expertise should also probably review for any unintended side effects.
Also a question about combining the maps indexed by vote commitment.
crates/types/src/vote.rs
Outdated
pub vote_outcomes: VoteMap2<Commitment<VOTE::Commitment>>, | ||
/// A list of valid signatures for certificate aggregation | ||
pub sig_lists: Vec<<TYPES::SignatureKey as SignatureKey>::PureAssembledSignatureType>, | ||
pub sig_lists: HashMap< | ||
Commitment<VOTE::Commitment>, | ||
Vec<<TYPES::SignatureKey as SignatureKey>::PureAssembledSignatureType>, | ||
>, | ||
/// A bitvec to indicate which node is active and send out a valid signature for certificate aggregation, this automatically do uniqueness check | ||
pub signers: BitVec, | ||
pub signers: HashMap<Commitment<VOTE::Commitment>, BitVec>, |
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.
Does it make sense to combine these all into one map?
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 debating this I think it makes sense
debug!("Forwarding message"); | ||
let result = relay_task.handle_event(event.clone()).await; | ||
|
||
if result.0 == Some(HotShotTaskCompleted::ShutDown) { | ||
// The protocol has finished | ||
return; | ||
} | ||
|
||
map.insert(vote_view, result.1); | ||
relay_map.insert(relay, result.1); |
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.
Should we also remove the vote_view
from the map
if the relay_map
is empty? We did that in the previous code, but now it looks like we only clear the inner relay_map
.
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 think if remove the whole view, we'd just end up creating it again on the next vote from this view. We still do the Garbage collection on view change to remove views from the map
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.
Makes sense!
This PR:
Keys the relay maps by relay number as well. This makes it so that relays will accumulate votes separately for each time they are the relay.
It also changes the vote accumulator to map vote commitment to the signers (sig_list and signers bitvec). This mirrors how we map vote commitment to the vote outcomes and should make it impossible to form an invalid cert by accident.
This PR does not:
No garbage collection for old relay rounds (they will be clean up when the view moves on)
Key places to review:
In view_sync.rs make sure the new mapping makes sense.
In the vote accumulator, check the new mapping as well