Skip to content
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

Update API/serialization time constraints #1139

Merged
merged 34 commits into from
May 16, 2023
Merged

Update API/serialization time constraints #1139

merged 34 commits into from
May 16, 2023

Conversation

heifner
Copy link
Member

@heifner heifner commented May 5, 2023

Update of API/serialization time constraints

Requirements:

  • an API node should be able to serve any atomic request (such as get_block, send_transaction* and push_transaction*) successfully
  • for non atomic API calls, such as get_table_rows, get_table_by_scope, get_scheduled_transactions and get_producers (where the user is free to request a smaller range if needed), the node operator should be able to specify a max request time.
  • some specific (user-provided) ABIs can be crafted to take an excessively long time (abi-bombs), and we must guard against that.

ABI serialization limit

The ABI deserialization for most ABI intensive requests (including get_block, send_transaction*, push_transaction*, get_account, get_table_rows) was moved to the HTTP thread pool, so ABI deserialization no longer can affect the responsiveness of nodeos's main thread.

Note that ABI serialization, when processing the API parameters (for example for send_transaction2), still occurs on the main thread and is limited by abi-serializer-max-time-ms. In case of serialization failure, the API will fail and return an error.

For the deserialization now occuring on the http thread pool, any single deserialization call will be limited by abi-serializer-max-time-ms as well. If that limit is hit for a specific object, the request will not fail, instead that object will be returned in the binary, non-deserialized form.

So when processing a request such as get_block, the request will always succeed (no timeout), but some specific objects may be returned in non-deserialized form. The max time for processing a get_block request will only be limited by the maximum number of actions in a block and abi-serializer-max-time-ms.

However because the deserialization is now done off the main thread, the absence of a lower limit should not impact the responsiveness of the node when serving legitimate requests.

Constraints on the number of items returned for non-atomic APIs

http_max_response_time now applies only to requests which specify a range of objects to be retrieved ( get_table_rows, get_table_by_scope, get_scheduled_transactions and get_producers), and specifies the maximum time spent on the main thread. The default of http-max-response-time-ms has been changed from 30ms to 15ms.

As before, API requests can also specify an optional time_limit_ms which now also provides a constraint on the maximum time spent on the main thread (excluding deserialization). If time_limit_ms is not specified then the nodeos configuration of http-max-response-time-ms will be used as the limit of time spent on the main thread.

As long as the request is valid and the requested range is not empty, a minimum of one object will always be returned (one row for get_table_rows for example). There is also a hard coded maximum of 1000 items returned, unless http_max_response_time is set to -1 in which case there is no limit.

Note: This PR changes /v1/chain/get_activated_protocol_features to be considered an atomic request. limit and time_limit_ms of the request will be ignored and all activated protocol features will always be returned.

So we define:

  • max_time is the minimum of http_max_response_time and time_limit_ms (negative values are equivalent to infinite time).
  • max_items is the requested range size, limited to 1000 unless http_max_response_time is negative.
  • min_items is 1 if the requested range is not empty, 0 otherwise

API processing occurs in two steps:

  1. On the main thread, retrieval (in binary format) of as many requested items as possible within the range [min_items, max_items], with a max_time processing limit.
  2. On the http thread pool, formatting of the http response, including the ABI deserialization of every object that can be deserialized withinabi-serializer-max-time-ms, and returning the response.

note: if http_max_response_time is set to -1 and the request does not set a time_limit_ms, the request will always return the complete requested range.

note: the protection against ABI bombs described above still applies, so if any specific object deserialization takes longer than abi-serializer-max-time-ms, it will be returned in binary form. Unlike prior behavior, this will not cause the request to fail and return an error code.

note: another change from previous behavior is that http_max_response_time now applies exclusively to the time spent on the main thread. If left unchanged, a node running 5.0 will be able to serve larger API requests than a 4.0 node.

Developer notes

  • safe_add added to time_point that avoids overflow.
  • Move constructor added to mutable_variant_object for variant_object to avoid copy.

Resolves #1062

@heifner heifner added this to the Leap v5.0.0-rc1 milestone May 5, 2023
@heifner heifner added the OCI Work exclusive to OCI team label May 5, 2023
@heifner heifner requested review from greg7mdp and linh2931 May 8, 2023 18:43
… constructor of mutable_variant_object since there is an explicit constructor for it.
@greg7mdp
Copy link
Contributor

greg7mdp commented May 12, 2023

in:

fc::scoped_exit<std::function<void()>> abi_traverse_context::enter_scope() {
         std::function<void()> callback = [old_recursion_depth=recursion_depth, this](){
            recursion_depth = old_recursion_depth;
         };
         ...

why don't we return at the end

fc::scoped_exit<std::function<void()>> abi_traverse_context::enter_scope() {


         ...
         return [this](){  --recursion_depth;  };

@greg7mdp
Copy link
Contributor

greg7mdp commented May 12, 2023

Kevin, in your last changes, I think the abi_traverse_context constructor should ypdate yield to take into account max_action_data_serialization.

never mind, I see it. However there is no need to pass max_action_data_serialization to abi_traverse_contextwhich does nothing with it. or at least if it is there for compatibility, remove the member variable which is unused.

Never mind again I see it now. I like it :-).

libraries/libfc/src/variant_object.cpp Outdated Show resolved Hide resolved
libraries/libfc/src/variant_object.cpp Outdated Show resolved Hide resolved
plugins/chain_plugin/chain_plugin.cpp Outdated Show resolved Hide resolved
plugins/chain_plugin/chain_plugin.cpp Outdated Show resolved Hide resolved
plugins/chain_plugin/chain_plugin.cpp Outdated Show resolved Hide resolved
plugins/chain_plugin/chain_plugin.cpp Outdated Show resolved Hide resolved
Comment on lines +2613 to +2614
make_resolver(chain(), get_abi_serializer_max_time(), throw_on_yield::no),
get_abi_serializer_max_time());
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems odd that we set the same get_abi_serializer_max_time both in the resolver (setting a yield function when creating the abi_serializer), and in the impl::abi_traverse_context. But I still have not wrapped my head about all the details of this so probably it is needed?

@heifner heifner requested review from greg7mdp and linh2931 May 15, 2023 17:10
@heifner heifner merged commit 203309b into main May 16, 2023
13 checks passed
@heifner heifner deleted the GH-1062-time-limits branch May 16, 2023 00:28
@heifner heifner added the documentation Improvements or additions to documentation label May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation OCI Work exclusive to OCI team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update handling of http-max-response-time-ms & abi-serializer-max-time-ms
3 participants