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

Delay scheduling batch promises until end of method invocation #526

Closed
austinabell opened this issue Aug 10, 2021 · 4 comments
Closed

Delay scheduling batch promises until end of method invocation #526

austinabell opened this issue Aug 10, 2021 · 4 comments

Comments

@austinabell
Copy link
Contributor

austinabell commented Aug 10, 2021

The issue with the current API is that async function calls require specifying gas up-front, which requires users to be very aware of how much gas to attach to make the call succeed. This also requires a clunkier API for requiring to specify gas whenever an external function is called.

Because of this API, the common pattern used throughout examples is to split the amount of total prepaid gas among the current call and all scheduled function calls. This leads to inefficient allocation of gas because there likely will be gas leftover from the current method's invocation.


As brought up and discussed on the zulip threads there is a value add to having these scheduled after execution, to more efficiently utilize gas for async function calls.

The TLDR of the proposed solution is to have a builder pattern that allows not setting the gas value attached to the function call to allow this to be scheduled by splitting the remaining gas among unset function calls. PR #523 is a rough implementation of what was discussed, to have a reference point for discussion and to point out potential flaws.

The issues I have with this logic happening on the SDK side and thus compiled into each contract:

  • Because this calculation has to happen during the method execution, there is no way to know how much gas is needed for the scheduling of the remaining promises
    • You can delay getting the remaining gas and scheduling only the function calls, but it will never be perfect and the estimation will always require estimates which will either error or waste gas
  • Adds code bloat to every contract to have some globally mutable structure to track
    • This cannot be moved into the function without changing existing APIs and making it much less ergonomic by forcing users to be aware of this parameter and define it
    • For contracts wanting to avoid alloc/vec, this will greatly increase code size
  • Creates a potentially confusing API because now there are two indices for promises (scheduled on contract size and ones scheduled through syscalls)
    • Mitigated by having multiple types in the PR mentioned so it can't be misused
  • There is either a split between manually scheduling this at call time env::promise_* API and scheduling through Promise API or requiring everyone to follow this opinionated pattern
  • This pattern opens up to more possible bugs and bad programming patterns to maintain
    • Also makes it hard to have complete parity between language SDKs

And to mention PROs:

  • Removes balance and gas from required params of ext contract calls in favour of builder pattern
    • Will be less confusing to users calling into this code generated function
    • Can't get rid of account_id because that is required
  • Removes drop pattern for Promises, which can easily be a foot gun for someone unfamiliar with Rust's ownership model
  • Possibility to use gas more efficiently (not currently the case, in fact, it's more for the scheduling logic)

My suggestion/question:

A suggestion I mentioned on Zulip, but repeating here for convenience, is for this splitting remaining gas to happen on the runtime (if possible). Because this would happen after the execution of the function, there will not be unpredictable gas costs during gas splitting/scheduling as it can be taken into account by the runtime when calculating. How this would happen in practice is to have some u64 (gas) value, like u64::MAX, which would indicate for the runtime to assign the gas amount attached, and whenever the runtime encounters this value it would check for the number of unassigned (say n) and then assign remaining_gas/n gas to each of those.

This would solve basically all negatives mentioned above but comes with a few new cons, which I will note:

  • Potentially increases base cost (to have to check the u64 equality of all function call gas to check for unique value)
  • Protocol breaking change
    • Doesn't have to change the original syscall method signature (but can add one if it makes migration easier)
  • The splitting function would be opinionated (assuming splitting equally the gas) which is rigid and doesn't allow for users who may want to split using some ratio
    • Very valid use case, because one function call may be a simple one yet another one nests into much more logic and more async calls
    • Perhaps a reason to introduce a new syscall for this? Maybe there is a way to architect this so it doesn't affect the gas cost of existing syscalls if this isn't called?
@bowenwang1996
Copy link
Contributor

You can delay getting the remaining gas and scheduling only the function calls, but it will never be perfect and the estimation will always require estimates which will either error or waste gas

Could you explain more where the imperfection comes from? Is it due to the call to env::prepaid_gas() and env::used_gas()?

@austinabell
Copy link
Contributor Author

You can delay getting the remaining gas and scheduling only the function calls, but it will never be perfect and the estimation will always require estimates which will either error or waste gas

Could you explain more where the imperfection comes from? Is it due to the call to env::prepaid_gas() and env::used_gas()?

So it's not due to this call, because used_gas charges gas before returning the amount. The imperfection comes from:

  • Any compute cycles that are used to divide the "remaining gas" by the number of unspecified function calls and then copying this value to each function call
  • Syscalls to schedule these batch function calls with this gas

And there is no way for the SDK to statically predict the gas usage at compile-time, especially since variable and dependent on the network config, and there is no way to move these two to before the used_gas call.

This is even assuming that only the batch function calls are scheduled after this, which requires extra bookkeeping logic. In practice, I think it may be hard to limit the computation strictly to these two.

@austinabell
Copy link
Contributor Author

austinabell commented Aug 12, 2021

Cross-posting comment made on Zulip for visibility:

Following up from our meeting today where I said I would present a possible API on the protocol to satisfy our criteria and goals as a possible alternative to attempting to solve only on the SDK/contract side:

    /// Appends `FunctionCall` action to the batch of actions for the given promise pointed by
    /// `promise_idx`. This function allows not specifying a specific gas value and allowing the
    /// runtime to assign remaining gas based on a ratio.
    ///
    /// # Gas
    ///
    /// Gas can be specified using a static amount, a ratio of remaining prepaid gas, or a mixture
    /// of both. To omit a static gas amount, [`u64::MAX`] can be passed for the `gas` parameter.
    /// To omit assigning remaining gas, [`u64::MAX`] can be passed as the `gas_ratio` parameter.
    ///
    /// The gas ratio parameter works as the following:
    ///
    /// All unused prepaid gas from the current function call is split among all function calls
    /// which supply this gas ratio. The amount attached to each respective call depends on the
    /// value of the ratio.
    ///
    /// For example, if 40 gas is leftover from the current method call and three functions specify
    /// the ratios 1, 5, 2 then 5, 25, 10 gas will be added to each function call respectively,
    /// using up all remaining available gas.
    ///
    /// # Errors
    ///
    /// <...Ommitted previous errors as they do not change>
    /// - If the [`u64::MAX`] special value is passed for `gas` and `gas_ratio` parameters
    ///
    ///
    /// [`u64::MAX`]: std::u64::MAX
    pub fn promise_batch_action_function_call_ratio(
        promise_index: u64,
        method_name_len: u64,
        method_name_ptr: u64,
        arguments_len: u64,
        arguments_ptr: u64,
        amount_ptr: u64,
        gas: u64,
        gas_ratio: u64,
    );

This is just the function signature and description of the different functionality but can move closer to actual implementation if we can come to an agreement on some version of this. As I mentioned on the call, this proposed API can completely replace the previous promise_batch_action_function_call on the SDK side, meaning we don't have to worry about including extra functions.

The downside to this, aside from points mentioned like adding complexity to runtime and adding more immutable host functions, is that the function name/signature is larger so minimally increases code size using this and could increase the base cost of this functionality by performing logic to check for ratios of function calls.

This API allows on the SDK level all possible flexibility to not specify any gas values (and we can default to a ratio of 1), specify a static amount of gas only, a ratio only, or even a mixture of both if a developer is sure about a specific static portion of a function call.

@austinabell
Copy link
Contributor Author

Closed with #742 near/nearcore#6150

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants