impr: Allow to receive multiple /block/current requests and use highest height.#1010
impr: Allow to receive multiple /block/current requests and use highest height.#1010speeddragon wants to merge 4 commits into
Conversation
…arweave nodes for /block/current
| case current_height_responses(Responses) of | ||
| [] -> best_response(Responses); | ||
| Heights -> current_height_best_response(Heights) | ||
| end; |
There was a problem hiding this comment.
What about removing the current_height_responses and having current_height_best_response(Responses) do the json:decode and then either return {ok, Response} or {error, no_viable_responses}. That would then be the switch to delgate to best_response(Responses)
| %% ascending by HTTP status code. Returns the first (best) response tuple. | ||
| best_response(Path, {error, {no_viable_responses, Responses}}) -> | ||
| best_response(Path, Responses); | ||
| best_response(<<"/height">>, Responses) when is_list(Responses) -> |
There was a problem hiding this comment.
/height Also needs to be added to the templates I think?
| %% @doc Parse responses that contains block height to return the one with the | ||
| %% highest value. This helps when one node lags behind. | ||
| current_height_best_response(Responses) -> | ||
| FilteredResponsesHeight = lists:filtermap( | ||
| fun | ||
| ({ok, #{ <<"body">> := Body }} = Response) -> | ||
| try hb_json:decode(Body) of | ||
| #{ <<"height">> := Height } -> | ||
| {true, {hb_util:int(Height), Response}}; | ||
| Height when is_integer(Height) -> | ||
| {true, {Height, Response}}; | ||
| _ -> | ||
| false | ||
| catch _:_ -> | ||
| case hb_util:safe_int(Body) of | ||
| {ok, Height} -> {true, {Height, Response}}; | ||
| {error, invalid} -> false | ||
| end | ||
| end; | ||
| (_) -> | ||
| false | ||
| end, | ||
| Responses | ||
| ), | ||
| case FilteredResponsesHeight of | ||
| [] -> | ||
| {error, no_viable_responses}; | ||
| [{Height, Response} | Rest] -> | ||
| {_, BestResponse} = | ||
| lists:foldl( | ||
| fun({NextHeight, NextResponse}, {BestHeight, Best}) -> | ||
| case NextHeight > BestHeight of | ||
| true -> {NextHeight, NextResponse}; | ||
| false -> {BestHeight, Best} | ||
| end | ||
| end, | ||
| {Height, Response}, | ||
| Rest | ||
| ), | ||
| BestResponse | ||
| end. |
There was a problem hiding this comment.
Ah maybe your old implementation was better? I thought by combining them we could collapse the logic - one loop that queries the height and maintains the best response rather than 2 loops. But if that's not possible, then your old is probably cleaner.
Also the new implementation lost the delegation to best_response/1 which was probably a good delegation (but I guess could be retinaed by delegating to best_response/1 instead of returning {error, no_viable_responses}
There was a problem hiding this comment.
Ok, my bad. I've updated the function to merge both loops into one. In terms of readability, I think the first version was the best. Performance-wise, we don't expect many requests, so we can keep the old version, or we can go with this one.
There was a problem hiding this comment.
Got it, yeah first version best. Sorry for the detour. It was the 2 separate data structures (list of Responses and list of {Height, Response}) that I was trying to simplify, but I agree the old one had some extra encapsulation that made it more readable than this one.
2 participants
Nodes can fall behind the current block height. Currently, the code only receives one response when asking for the current block, so if the node with the faster reply is the one that lags, the copycat Arweave index will not index the newest blocks.
This PR allows receiving two responses (2) from nodes when requesting Arweave nodes for
/block/current.Alternative solution
An alternative solution could be to implement a device that uses
<<"admissible">>(or another property) to filter the entire response list. This is too complicated, and this is the only case that fits it.