-
Notifications
You must be signed in to change notification settings - Fork 96
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
The schedule 0, 1, get_coin(), ... #76
Conversation
14072bc
to
2d899a1
Compare
2d899a1
to
7a4fc37
Compare
src/agreement/mod.rs
Outdated
@@ -201,6 +211,7 @@ impl<NodeUid: Clone + Debug + Ord> Agreement<NodeUid> { | |||
netinfo, | |||
Nonce::new(invocation_id.as_ref(), session_id, proposer_i, 0), | |||
), | |||
coin_schedule: CoinSchedule::False, |
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 there was an argument for starting with True
: In the best case scenario, where everyone gets their proposals delivered in time, everyone would vote true
in every instance, and it would be nice to terminate in the first round then.
Also, it is probably generally preferred to err on the true
side and include more elements in the common subset, and therefore more transactions in the epoch.
src/agreement/mod.rs
Outdated
fn coin_schedule(&self) -> CoinSchedule { | ||
match self.epoch % 3 { | ||
0 => CoinSchedule::False, | ||
1 => CoinSchedule::True, |
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.
As above, I'd swap True
and False
here.
self.common_coin = CommonCoin::new(self.netinfo.clone(), nonce); | ||
self.coin_schedule = self.coin_schedule(); |
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.
Do we need the coin schedule as a member variable at all? Isn't it easier to just call self.coin_schedule()
where it's needed?
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 didn't want to make a call on every Aux
message.
Close #69
This branch depends on PR #68.