-
Notifications
You must be signed in to change notification settings - Fork 145
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
SCP align implementation with spec #745
base: main
Are you sure you want to change the base?
Conversation
if let Some(h) = self.ballots_confirmed_prepared().into_iter().max() { | ||
if let Some(h_old) = &self.H { | ||
if h_old > h { | ||
return Some(h_old.X); | ||
} | ||
} | ||
return Some(h.X); | ||
} | ||
else if let Some(h_old) = &self.H { | ||
return Some(h_old.X); | ||
} |
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.
A simpler alternative would be returning H if it exists. Calling ballots_confirmed_prepared()
is fairly expensive (or looks expensive).
if let Some(p_old) = &self.P { | ||
if p_old > p { | ||
return Some(p_old.X); | ||
} | ||
} | ||
return Some(p.X); | ||
} | ||
else if let Some(p_old) = &self.P { | ||
return Some(p_old.X); | ||
} |
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.
Similarly, simpler to return P if it exists.
// Otherwise, values are unchanged. | ||
if !self.B.is_zero() { |
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 do not believe this conditional will ever be true. B can only be non-zero if: Z is set, H is set, or P is set. Whichever way it gets set, the earlier conditionals in this function will trigger before we get this far (assuming commit changes are added).
332eb25
to
cba23ba
Compare
Soundtrack of this PR: link to song that really fits the mood of this PR
Motivation
Align ballot value in ballot to vote to prepare with spec
The current implementation of SCP in MobileCoin is very close to the protocol description in the IETF draft and whitepaper. However one nuance was overlooked.
When setting the ballot value
B
, which corresponds to the ballot the node will vote to prepare,get_next_ballot_values()
uses this priority list:B
None
However the available documentation on SCP does not indicate the {with available messages from the network} should be used. Instead they say
Since nodes only store very limited historical information in their state (only
B
,P
,PP
,C:H
), it is possible for future messages from a node to 'forget' about old information that a different node once used to set theirH
orP
. This means afterH
is set, the other node could see a future state of the network that would not permit it to confirmH
is prepared, andget_next_ballot_values()
will fail to return the highest ballot confirmed prepared previously.Add missing
maybe_set_ballot_timer()
In
do_prepare_phase()
step 9 there is amaybe_set_ballot_timer()
after updating the counter, however it is missing from step 9 indo_commit_phase()
without explanation.In this PR
maybe_set_ballot_timer()
Future Work