Merge pull request #27 from helium/adt/acs-not-return

Attempt to fix bug where ACS does not complete
helium · Nov 13, 2018 · 0ca3afd · 0ca3afd
2 parents 8c35423 + 5d29352
commit 0ca3afd
Show file tree

Hide file tree

Showing 5 changed files with 11 additions and 8 deletions.
diff --git a/rebar.config b/rebar.config
@@ -21,7 +21,7 @@
 {profiles, [
             {test, [
                {deps, [
-                       {relcast, ".*", {git, "https://github.com/helium/relcast.git", {branch, "madninja/review"}}}
+                       {relcast, ".*", {git, "https://github.com/helium/relcast.git", {branch, "master"}}}
                       ]}
               ]}
            ]}.
diff --git a/src/hbbft_acs.erl b/src/hbbft_acs.erl
@@ -109,7 +109,8 @@ handle_msg(Data, J, {{rbc, I}, RBCMsg}) ->
                     BBA = get_bba(NewData, I),
                     case hbbft_bba:input(BBA#bba_state.bba_data, 1) of
                         {DoneBBA, ok} ->
-                            {store_bba_state(NewData, I, DoneBBA), ok};
+                            %% this BBA probably already completed, check if ACS has completed
+                            check_completion(store_bba_state(NewData, I, DoneBBA));
                         {NewBBA, {send, ToSend}} ->
                             {store_bba_input(store_bba_state(NewData, I, NewBBA), I, 1),
                             {send, hbbft_utils:wrap({bba, I}, ToSend)}}
@@ -147,6 +148,7 @@ handle_msg(Data = #acs_data{n=N, f=F}, J, {{bba, I}, BBAMsg}) ->
                                                               end
                                                       end, {NewData, hbbft_utils:wrap({bba, I}, ToSend0)}, lists:seq(0, N - 1)),
                     %% each BBA is independant, so the total ordering here is unimportant
+                    %% but we sort to try to get the messages from the earlier rounds delivered first
                     {NextData#acs_data{done=true}, {send, sort_bba_msgs(lists:flatten(Replies))}};
                 false ->
                     check_completion(NewData)

diff --git a/src/hbbft_cc.erl b/src/hbbft_cc.erl
@@ -52,8 +52,8 @@ init(SecretKeyShard, Sid, N, F) ->
 %% Figure12. Bullet2
 %% on input GetCoin, multicast ThresholdSignpk (ski, sid)
 -spec get_coin(cc_data()) -> {cc_data(), ok | {send, [hbbft_utils:multicast(share_msg())]}}.
-get_coin(#cc_data{state=done}) ->
-    ignore;
+get_coin(Data = #cc_data{state=done}) ->
+    {Data, ok};
 get_coin(Data) ->
     Share = tpke_privkey:sign(Data#cc_data.sk, Data#cc_data.sid),
     SerializedShare = hbbft_utils:share_to_binary(Share),
@@ -70,7 +70,7 @@ handle_msg(Data, J, {share, Share}) ->
     share(Data, J, Share).
 
 %% TODO: more specific return type than an integer?
--spec share(cc_data(), non_neg_integer(), binary()) -> {cc_data(), ok | {result, integer()}}.
+-spec share(cc_data(), non_neg_integer(), binary()) -> {cc_data(), ok | {result, integer()}} | ignore.
 share(#cc_data{state=done}, _J, _Share) ->
     ignore;
 share(Data, J, Share) ->

diff --git a/test/hbbft_relcast_SUITE.erl b/test/hbbft_relcast_SUITE.erl
@@ -192,7 +192,7 @@ one_actor_missing_test(Config) ->
     %% start it on runnin'
     {_, ConvergedResults} = hbbft_test_utils:do_send_outer(Module, Replies, NewStates, sets:new()),
     %% check no actors returned a result
-    ?assertEqual(4, sets:size(ConvergedResults)),
+    ?assertEqual(N - 1, sets:size(ConvergedResults)),
     DistinctResults = sets:from_list([BVal || {result, {_, BVal}} <- sets:to_list(ConvergedResults)]),
     %% check all N actors returned the same result
     ?assertEqual(1, sets:size(DistinctResults)),

diff --git a/test/hbbft_relcast_worker.erl b/test/hbbft_relcast_worker.erl
@@ -77,9 +77,10 @@ handle_cast({ack, Sender}, State) ->
     %% ct:pal("ack, Sender: ~p", [Sender]),
     {ok, NewRelcast} = relcast:ack(Sender, maps:get(Sender, State#state.peers, undefined), State#state.relcast),
     {noreply, do_send(State#state{relcast=NewRelcast, peers=maps:put(Sender, undefined, State#state.peers)})};
-handle_cast({block, Block, PubKey}, State) ->
+handle_cast({block, Block, SerializedPubKey}, State) ->
     case lists:member(Block, State#state.blocks) of
         false ->
+            PubKey = tpke_pubkey:deserialize(SerializedPubKey),
             case verify_block_fit([Block | State#state.blocks], PubKey) of
                 true ->
                     {ok, Relcast} = relcast:command(next_round, State#state.relcast),
@@ -112,7 +113,7 @@ handle_info({signature, Sig, Pubkey}, State=#state{tempblock=TempBlock, peers=Pe
         false ->
             case verify_block_fit([NewBlock | State#state.blocks], Pubkey) of
                 true ->
-                    _ = [gen_server:cast({global, name(I)}, {block, NewBlock, Pubkey}) || {I, _} <- maps:to_list(Peers)],
+                    _ = [gen_server:cast({global, name(I)}, {block, NewBlock, tpke_pubkey:serialize(Pubkey)}) || {I, _} <- maps:to_list(Peers)],
                     {ok, Relcast} = relcast:command(next_round, State#state.relcast),
                     {noreply, do_send(State#state{relcast=Relcast,
                                                   tempblock=undefined,