Skip to content
This repository has been archived by the owner on Jun 12, 2023. It is now read-only.

Make skip messages 'portable' such that they can be gossipped #895

Merged
merged 4 commits into from
Jul 13, 2022

Conversation

Vagabond
Copy link
Member

@Vagabond Vagabond commented Jul 6, 2021

No description provided.

@Vagabond Vagabond requested a review from evanmcc July 6, 2021 16:40
ToSend = fixup_msgs(ToSend1 ++ ToSend2),
%% we retain the skip votes here because we may not succeed and we want the
%% algorithm to converge more quickly in that case
{(state_reset(HBBFT2, S0))#state{skip_votes = S0#state.skip_votes},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use the skip votes we got in this skip message here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do we even still need to do this? the skip was confirmed at this point?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to retain votes if we have echoable votes, no

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so set skip_votes to empty here?

@@ -297,6 +297,40 @@ handle_message(<<BinMsgIn/binary>>, Index, #state{hbbft = HBBFT, skip_votes = Sk
{ok, {proposed_skip, ProposedRound, IndexRound}} ->
Skips1 = Skips#{Index => {ProposedRound, IndexRound}},
{S0#state{skip_votes = Skips1}, []};
{ok, {skip, Round, Sigs}} when Round > CurRound ->
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round > CurRound is correct, right? we can never skip backwards?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can skip backwards, we just can't skip onto the existing round, or we won't make any progress.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would we skip backwards?

@@ -804,6 +844,9 @@ bin_to_msg(<<Bin/binary>>) ->
{error, truncated}
end.

sign_skip({proposed_skip, ProposedRound, MyRound}, #state{swarm_keys={_Key, SigFun}}) ->
{proposed_skip, ProposedRound, MyRound, SigFun(t2b({proposed_skip, ProposedRound}))}.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only sign over the proposed round, not myround, is that ok?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this will allow some minor adversarial behavior but I am not sure if we care? If you're not being honest about your round and want the group to stop, you don't plan on executing anyway?

%% we retain the skip votes here because we may not succeed and we want the
%% algorithm to converge more quickly in that case
{(state_reset(HBBFT2, S0))#state{skip_votes = S0#state.skip_votes},
[ new_epoch | ToSend]};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we send the skip message to all the nodes who did not sign the skip here?

@vihu vihu force-pushed the adt/portable-skip-messages branch from 2d5a09e to b96b2d9 Compare May 25, 2022 22:29
@evanmcc evanmcc force-pushed the adt/portable-skip-messages branch from b96b2d9 to fb6746f Compare July 11, 2022 18:10
@evanmcc evanmcc merged commit 595cd9e into master Jul 13, 2022
@evanmcc evanmcc deleted the adt/portable-skip-messages branch July 13, 2022 23:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants