Correctly handle badly encoded transactions in http dry-run API (#4195)

* Whitespace, unused variables, etc. * dry-run http api should correctly handle badly encoded transactions
aeternity · Sep 4, 2023 · a2667d2 · a2667d2
1 parent a0519aa
commit a2667d2
Show file tree

Hide file tree

Showing 7 changed files with 62 additions and 60 deletions.
diff --git a/apps/aecore/src/aec_consensus_hc.erl b/apps/aecore/src/aec_consensus_hc.erl
@@ -228,7 +228,6 @@ recent_cache_trim_key_header(_) -> ok.
 
 keyblocks_for_target_calc() -> 0.
 keyblock_create_adjust_target(Block0, []) ->
-    Height = aec_blocks:height(Block0),
     {ok, Stake} = aeu_ets_cache:lookup(?ETS_CACHE_TABLE, added_stake),
     Block = aec_blocks:set_target(Block0, aeminer_pow:integer_to_scientific(Stake)),
     {ok, Block}.
@@ -623,7 +622,6 @@ next_beneficiary() ->
     PCHeight = pc_height(NextHeight),
     case aec_parent_chain_cache:get_block_by_height(PCHeight) of
         {ok, Block} ->
-            {ok, Commitments} = aec_parent_chain_block:commitments(Block),
             Entropy = aec_parent_chain_block:hash(Block),
             CommitmentsSophia = encode_commitments(Block),
             NetworkId = aec_parent_chain_block:encode_network_id(aec_governance:get_network_id()),
@@ -635,7 +633,7 @@ next_beneficiary() ->
             CallData = aeser_api_encoder:encode(contract_bytearray, CD),
             try call_consensus_contract_(?ELECTION_CONTRACT, TxEnv, Trees, CallData, "elect_next", 0) of
                 {ok, _Trees1, Call} ->
-                    {tuple, {{address, Leader}, Stake}}  = aeb_fate_encoding:deserialize(aect_call:return_value(Call)),
+                    {tuple, {{address, Leader}, _Stake}}  = aeb_fate_encoding:deserialize(aect_call:return_value(Call)),
                     SignModule = get_sign_module(),
                     case SignModule:set_candidate(Leader) of
                         {error, key_not_found} ->
@@ -644,10 +642,10 @@ next_beneficiary() ->
                         ok ->
                             {ok, Leader}
                     end;
-                {error, What} ->
+                {error, _What} ->
                     timer:sleep(1000),
                     {error, not_leader}
-            catch error:{consensus_call_failed, {error, What}} ->
+            catch error:{consensus_call_failed, {error, _What}} ->
                     timer:sleep(1000),
                     {error, not_leader}
             end;

diff --git a/apps/aecore/src/aec_parent_chain_cache.erl b/apps/aecore/src/aec_parent_chain_cache.erl
@@ -121,7 +121,7 @@ init([StartHeight, Size, BlockProducing, EnabledCommitments]) ->
     aec_events:subscribe(stop_mining),
     aec_events:subscribe(chain_sync),
     TopHeader = aec_chain:top_header(),
-    ChildHeight = aec_headers:height(TopHeader), 
+    ChildHeight = aec_headers:height(TopHeader),
     {ok, ChildHash} = aec_headers:hash_header(TopHeader),
     true = is_integer(ChildHeight),
     self() ! initialize_cache,
@@ -136,14 +136,14 @@ init([StartHeight, Size, BlockProducing, EnabledCommitments]) ->
 
 -spec handle_call(any(), any(), state()) -> {reply, any(), state()}.
 handle_call({get_block_by_height, Height}, _From, State) ->
-    Reply = 
+    Reply =
         case get_block(Height, State) of
             {error, _} = Err -> Err;
             {ok, _Block} = OK -> OK
         end,
     {reply, Reply, State};
 handle_call({get_commitments, Hash}, _From, State) ->
-    Reply = 
+    Reply =
         case get_block_height_by_hash(Hash, State) of
             {error, _} = Err -> Err;
             {ok, Height} ->
@@ -168,7 +168,7 @@ handle_cast({post_block, Block}, #state{} = State0) ->
         case State#state.top_height > State0#state.top_height of
             true ->
                 maybe_post_initial_commitments(Block, State);
-            false -> 
+            false ->
                 State
         end,
     {noreply, State1};
@@ -216,12 +216,11 @@ handle_info({gproc_ps_event, top_changed, #{info := #{block_type := key,
     {noreply, State};
 handle_info({gproc_ps_event, chain_sync, #{info := {is_syncing, IsSyncing}}}, State0) ->
     State1 = State0#state{is_syncing = IsSyncing},
-    State = 
+    State =
         case IsSyncing =:= false andalso not State0#state.posted_commitment of
             true ->
                 {ok, TopBlock} = aec_chain:top_key_block(),
                 {ok, TopHash} = aec_blocks:hash_internal_representation(TopBlock),
-                Height = aec_blocks:height(TopBlock),
                 _State1 = maybe_post_commitments(TopHash, State1);
             false ->
                 lager:debug("Not posting commitment", []),
@@ -267,7 +266,7 @@ get_block(Height, #state{blocks = Blocks}) ->
         error ->
             {error, not_in_cache}
     end.
-    
+
 -spec get_block_height_by_hash(aec_parent_chain_block:hash(), state()) ->
     {ok, non_neg_integer()} | {error, not_in_cache}.
 get_block_height_by_hash(Hash, #state{blocks_hash_index = Index}) ->
@@ -276,13 +275,13 @@ get_block_height_by_hash(Hash, #state{blocks_hash_index = Index}) ->
         error ->
             {error, not_in_cache}
     end.
-    
+
 -spec max_block(state()) -> non_neg_integer() | empty_cache.
 max_block(#state{blocks = Blocks}) when map_size(Blocks) =:= 0 ->
     empty_cache;
 max_block(#state{blocks = Blocks}) ->
     lists:max(maps:keys(Blocks)).
-    
+
 -spec delete_block(non_neg_integer(), state()) -> state().
 delete_block(Height, #state{blocks = Blocks,
                             blocks_hash_index = Index} = State) ->
@@ -297,12 +296,12 @@ delete_block(Height, #state{blocks = Blocks,
 state_to_map(#state{child_start_height = StartHeight,
                     child_top_height   = ChildHeight,
                     max_size     = MaxSize,
-                    blocks       = Blocks, 
+                    blocks       = Blocks,
                     top_height   = TopHeight}) ->
     #{  child_start_height => StartHeight,
         child_top_height   => ChildHeight,
         max_size     => MaxSize,
-        blocks       => Blocks, 
+        blocks       => Blocks,
         top_height   => TopHeight}.
 
 target_parent_height(#state{child_start_height    = StartHeight,

diff --git a/apps/aecore/test/aec_parent_chain_cache_tests.erl b/apps/aecore/test/aec_parent_chain_cache_tests.erl
@@ -226,7 +226,7 @@ post_child_top_in_the_middle_of_cachable_heights() ->
             timer:sleep(20),
             {ok, #{ child_start_height := StartHeight,
                     child_top_height   := ChildTop2,
-                    top_height         := ParentTop} = Res} = ?TEST_MODULE:get_state(),
+                    top_height         := ParentTop} = _Res} = ?TEST_MODULE:get_state(),
             {ChildTop1, ChildTop1} = {ChildTop1, ChildTop2},
             %%assert_child_cache_consistency(Res),
             ?TEST_MODULE:stop()
@@ -697,26 +697,26 @@ filter_meck_events(Module, Function) ->
         end,
         meck:history(Module)).
 
-mock_commitments_list(_BlockHashesMap) ->
-    meck:expect(aec_parent_connector, request_block_by_height,
-                fun(Height) ->
-                    spawn(
-                        fun() ->
-                            Block = block_by_height(Height),
-                            ?TEST_MODULE:post_block(Block)
-                        end)
-            end).
-
-mock_commitments_list(all, L) ->
-    meck:expect(aec_parent_connector, request_block_by_height,
-                fun(Height) ->
-                    spawn(
-                        fun() ->
-                            Block0 = block_by_height(Height),
-                            Block = aec_parent_chain_block:set_commitments(Block0, L),
-                            ?TEST_MODULE:post_block(Block)
-                        end)
-            end).
+%% mock_commitments_list(_BlockHashesMap) ->
+%%     meck:expect(aec_parent_connector, request_block_by_height,
+%%                 fun(Height) ->
+%%                     spawn(
+%%                         fun() ->
+%%                             Block = block_by_height(Height),
+%%                             ?TEST_MODULE:post_block(Block)
+%%                         end)
+%%             end).
+
+%% mock_commitments_list(all, L) ->
+%%     meck:expect(aec_parent_connector, request_block_by_height,
+%%                 fun(Height) ->
+%%                     spawn(
+%%                         fun() ->
+%%                             Block0 = block_by_height(Height),
+%%                             Block = aec_parent_chain_block:set_commitments(Block0, L),
+%%                             ?TEST_MODULE:post_block(Block)
+%%                         end)
+%%             end).
 
 header(Height) ->
     Hash = <<Height:32/unit:8>>,

diff --git a/apps/aehttp/src/aehttp_helpers.erl b/apps/aehttp/src/aehttp_helpers.erl
@@ -661,8 +661,10 @@ do_dry_run() ->
 
 dry_run_err(Err) when is_list(Err) ->
     dry_run_err(list_to_binary(Err));
+dry_run_err(Err) when is_binary(Err) ->
+    {ok, {400, [], #{ reason => <<"Bad request: ", Err/binary>>}}};
 dry_run_err(Err) ->
-    {ok, {403, [], #{ reason => <<"Bad request: ", Err/binary>>}}}.
+    {ok, {400, [], #{ reason => iolist_to_binary(io_lib:format("Bad request: ~200p", [Err])) }}}.
 
 prepare_dry_run_param(top, #{ top := top }) ->
     case aec_chain:top_block_hash() of

diff --git a/apps/aehttp/test/aehttp_dryrun_SUITE.erl b/apps/aehttp/test/aehttp_dryrun_SUITE.erl
@@ -165,6 +165,10 @@ spend_txs(Config) ->
     {ok, 200, #{ <<"results">> := [#{ <<"result">> := <<"error">> }, #{ <<"result">> := <<"ok">> }] }} =
         dry_run(Config, TopHash, [Tx2, Tx1]),
 
+    %% Negative test - badly encoded Tx
+    BinTx1 =  aeser_api_encoder:encode(transaction, aetx:serialize_to_binary(element(2, Tx1))),
+    {ok, 400, #{ <<"reason">> := <<"Bad request: invalid_encoding">> }} = dry_run(Config, TopHash, [{tx, <<BinTx1/binary, 43>>}]),
+
     ok.
 
 identity_contract(Config) ->

diff --git a/apps/aehttp/test/aehttp_ga_SUITE.erl b/apps/aehttp/test/aehttp_ga_SUITE.erl
@@ -793,7 +793,7 @@ do_dry_run(STx, ExpRes) ->
         {ok, 200, #{ <<"results">> := [#{ <<"result">> := Res } = ResObj] }} ->
             ct:pal("ResObj: ~p", [ResObj]),
             ?assertMatch(ExpRes, binary_to_atom(Res, utf8));
-        {ok, 403, #{<<"reason">> := Reason}} ->
+        {ok, 400, #{<<"reason">> := Reason}} ->
             ct:pal("Dry-run call failed with reason: ~s", [Reason]),
             ?assertMatch(ExpRes, error)
     end.
diff --git a/apps/aehttp/test/aehttp_stake_contract_SUITE.erl b/apps/aehttp/test/aehttp_stake_contract_SUITE.erl
@@ -525,7 +525,7 @@ end_per_group(hc_doge, Config) ->
     Cmd = "dogecoin-cli -datadir="++DogeDataDir++" stop",
     os:cmd(Cmd),
     Config;
-end_per_group(lazy_leader, Config) -> 
+end_per_group(lazy_leader, Config) ->
     aecore_suite_utils:stop_node(?LAZY_NODE, Config);
 end_per_group(_Group, Config) ->
     Config.
@@ -903,7 +903,7 @@ verify_commitments(Config) ->
                     ct:log("Commitments: ~p", [ParsedComms]),
                     lists:map(
                         fun({ParentHeight, N, H}) ->
-                            {N, N} = {N, H}, 
+                            {N, N} = {N, H},
                             ExpectedParentHeight = H + ?CHILD_START_HEIGHT - 1,
                             {ParentHeight, ParentHeight} = {ParentHeight, ExpectedParentHeight}
                         end,
@@ -1703,28 +1703,28 @@ wait_for_commitments_in_pool(Node, CNode, Cnt) ->
                                                end).
 
 wait_for_at_least_commitments_in_pool(Node, CNode, Cnt) ->
-    wait_for_commitments_in_pool_(Node, CNode, fun(Pool) -> 
+    wait_for_commitments_in_pool_(Node, CNode, fun(Pool) ->
                                                     TxsCnt = length(Pool),
                                                     TxsCnt >= Cnt
                                                end).
 
-wait_for_commitments_in_pool_but_allow_other_txs(Node, CNode, Cnt) ->
-    wait_for_commitments_in_pool_(Node, CNode,
-        fun(Pool) -> 
-            {ok, TopH} = aec_headers:hash_header(rpc(Node, aec_chain, top_header, [])),
-            ExpectedCommitment = aeser_api_encoder:encode(key_block_hash, TopH),
-
-            TxsCnt =
-                length(
-                    lists:filter(
-                        fun(SignedTx) ->
-                            {spend_tx, SpendTx} = aetx:specialize_type(aetx_sign:tx(SignedTx)),
-                            ct:log("Spend payload ~p", [aec_spend_tx:payload(SpendTx)]),
-                            aec_spend_tx:payload(SpendTx) =:= ExpectedCommitment
-                        end,
-                        Pool)),
-            TxsCnt =:= Cnt
-        end).
+%% wait_for_commitments_in_pool_but_allow_other_txs(Node, CNode, Cnt) ->
+%%     wait_for_commitments_in_pool_(Node, CNode,
+%%         fun(Pool) ->
+%%             {ok, TopH} = aec_headers:hash_header(rpc(Node, aec_chain, top_header, [])),
+%%             ExpectedCommitment = aeser_api_encoder:encode(key_block_hash, TopH),
+
+%%             TxsCnt =
+%%                 length(
+%%                     lists:filter(
+%%                         fun(SignedTx) ->
+%%                             {spend_tx, SpendTx} = aetx:specialize_type(aetx_sign:tx(SignedTx)),
+%%                             ct:log("Spend payload ~p", [aec_spend_tx:payload(SpendTx)]),
+%%                             aec_spend_tx:payload(SpendTx) =:= ExpectedCommitment
+%%                         end,
+%%                         Pool)),
+%%             TxsCnt =:= Cnt
+%%         end).
 
 wait_for_commitments_in_pool_(Node, ChildNode, CompareFun) ->
     wait_for_commitments_in_pool_(Node, ChildNode, CompareFun, 100).
@@ -1839,7 +1839,6 @@ validate_expected_commitments(Node, Commitments) ->
                 rpc(?NODE1, aec_parent_chain_block, encode_commitment_btc, [pubkey(Staker), TopH, NetworkId])
             end,
             [?ALICE, ?BOB]),
-    GenesisEncoded = aeser_api_encoder:encode(key_block_hash, rpc(Node, aec_chain, genesis_hash, [])),
     ct:log("Child chain top hashes ~p", [ExpectedCommitments]),
     case lists:all(fun(SignedTx) ->
                         {spend_tx, SpendTx} = aetx:specialize_type(aetx_sign:tx(SignedTx)),