stellar / rs-soroban-env

Rust environment for Soroban contracts.
Apache License 2.0
61 stars 42 forks source link

Add arithmetic host functions for i128 and u128 #697

Open christian-rogobete opened 1 year ago

christian-rogobete commented 1 year ago

What problem does your feature solve?

Developers who do not use the Rust SDK must implement arithmetic functions like add, sub, mul, <, > etc. for i128 and u128 themselves. This is difficult, error prone, and unnecessary since the data ends up back in the host anyway. It would be great if these functions are offered as host functions.

What would you like to see?

Arithmetic functions as they were offered before for bigint.

What alternatives are there?

graydon commented 1 year ago

We didn't do this in our first pass with {iu}128 because at the moment at least all developers are using Rust and {iu}128 are widely available in the guest, and when we profiled arithmetic on the host vs. guest, it seemed like it was actually faster to stay on the guest in these cases -- a u128 add is less work than making a host function call. But it's kinda "on the line" and I can see the point that some users might prefer to have the host do them (and this might apply even more firmly when we add {iu}256) so .. this seems reasonable to pursue, if perhaps not the highest current priority.

christian-rogobete commented 1 year ago

Thanks for the quick reply and for considering this request.

From my perspective it is like this: I am implementing it with AssemblyScript.

For example, I want to add two i128 values: c = ( a + b) (e.g. incr_allow in a token contract).

What do I have to do for this?

1. load a.lo from host (1st function call)
2. load a.hi from host (2nd function call)
3. load b.lo from host (3rd function call)
4. load b.hi from host (4th function call)
5. now I have four u64 values - so I have to implement a custom logic that adds them correctly
6. write the result back to the host (5th function call)

If I could do it in the host in comparison, it would be just one host function call (add) and I do not have to worry about implementing a custom logic to add them on the guest.

So, in my opinion, it depends a lot on the use case. Therefore, being able to choose depending on the use case would be very valuable in my opinion.

graydon commented 1 year ago

The "custom logic" is built into rust -- i128 is a standard type -- so you need not do anything for 5.

While you're right that for a single arithmetic operation in isolation the cost of transfering to guest and transferring back is larger than the savings. But what I'm saying is that if you do much more than a single operation -- if you do say a couple compares or a few adds, multiplies or subtracts -- it's cheaper to do guest-side and bracket the guest operations with loads and stores to the host.

I'm not 100% opposed to adding functions here (there's space to do it) but .. we can certainly survive without for a first version, and I think the only cases where it's a win are so small (single-op cases) that in a real contract you won't notice either way. Unless you have dozens or hundreds of contract functions that all do exactly one arithmetic op.

jayz22 commented 1 year ago

If I understood it correctly, the reason for this request is that you are trying build a WASM contract with AssemblyScript, or build the AssemblyScript SDK, and there is no native support for i128 in AS.

Given relative narrow use case of AS (and the broader use case of supporting other SDK types) we will move this to post launch in order to better focus on the Rust and soroban-sdk use cases.

tomerweller commented 1 year ago

@christian-rogobete can you use a 3rd party library like as-bignum to do arithmetics guest side?

christian-rogobete commented 1 year ago

Hello @jayz22 and @tomerweller, thanks for the update and for the info. I understand that a prioritization has to take place and it is of course more important to focus on the Rust use cases first.

As for the 3rd party libraries it is of course possible to use them. The as-bignum is currently the only one I have found. It works at least partially but not completely in the context of Soroban (e.g. you get - Status(VmError(Instantiation))- errors when calling some operations). Furthermore, many operations are not implemented, especially in connection with i128. Moreover, there has been no further development of the library for a longer time.

I integrated some operations into the as-soroban-sdk (mostly ported from as-bignum) that currently work well in the context of Soroban. For example: add, sub, mul, div, muldiv, sqrt, pow, eq, lt, gt, or, xor, and, shl, shr and others. They make it easier for developers to handle u128 and positive int128. Since soroban preview 9, the high words of i128 are now i64 (were u64 before). Thus the as-soroban-sdk can also be extended in this respect.

Developers can alternatively implement operations themselves if they need to use them intensively. However, I think it would be very good to provide host functions for this, since these are certainly more reliable and in my opinion sufficient for most cases. Self-implemented operations could lead to various problems such as overflows or they are not implemented performant enough, so that in my opinion it would be better to use reliable host functions.

leighmcculloch commented 1 year ago

have the host do them (and this might apply even more firmly when we add {iu}256)

If we're building a system that is intended to support a variety of languages building for WASM, then I think we should probably lean towards adding host functions.

And given that Rust itself doesn't have {iu}256 types, and yet we provide those types, it feels even more like we should do this for both types for consistency.

This will probably be important for folks who are building contracts that interact with ETH and the ETH ABI.

jayz22 commented 1 year ago

One of the troubles with the arithmetics host functions is there are too many of them. For i128 alone there are a few dozen unique methods (deduping checked_, unchecked_, overflowing_ version of the same operation). Plus the unsigned u128, the "interop" ones (operations between different number types). This can easily expand the total number of host functions several fold if we don't pick and choose. This could result in higher VmInstantiation cost which is already an issue.

That's the reason for {U,I}256 we've only implemented a few "basic building blocks" and leaving the rest for contract developers to handle on the guest side.

@christian-rogobete I'm curious from your perspective, what would be the minimum required set (and what would be nice-to-have set) of functions for AS support? Also it would be nice to know what causes the Vm(instantation) error you mentioned, or any other blockers so we could potentially address them.

leighmcculloch commented 1 year ago

This could result in higher VmInstantiation cost which is already an issue.

Does this result in a higher vm instantiation cost for all contracts, or just contracts that import (use) those specific host functions? I expect the latter. If it's the latter that doesn't seem like a problem.

jayz22 commented 1 year ago

This could result in higher VmInstantiation cost which is already an issue.

Does this result in a higher vm instantiation cost for all contracts, or just contracts that import (use) those specific host functions? I expect the latter. If it's the latter that doesn't seem like a problem.

From what I can tell the linker needs to go through all the host functions and define them for the wasmi module. https://github.com/stellar/rs-soroban-env/blob/main/soroban-env-host/src/vm.rs#L171-L178

christian-rogobete commented 1 year ago

@jayz22, from my perspective, the ones that were offered earlier for bignum make sense. See here: bignum host functions

Furthermore I think that muldiv, where there is no overflow in the multiplication part would be great. It is used in the liquidity pool contract example.

// Calculate multiply and division as `number * numerator / denominator` without overflow in multiplication part.
static muldiv(number: u128, numerator: u128, denominator: u128): u128

Another one would be mulsqrt.

As for the VM(instantiation) issues, I have now found a solution for most cases. The as-bignum implementation throws exceptions/errors in some places. This causes this import in the wasm code, that triggers VM(instantiation):

// wat representation
 (import "env" "abort" (func $~lib/builtins/abort (param i32 i32 i32 i32)))

Such special imports need support from the host environment. See AS special imports.

But this can also be circumvented by implementing a special abort function in the contract. I will describe this method in the documentation and examples of the sdk.

However, there are still problems, especially regarding working with strings, which I can't understand. e.g.

a = u128.muldiv(b,c,d);
let f = a.toString(); // Status(VmError(Validation))

I have documented this before in this issue.

Hope this helps.

jayz22 commented 1 year ago

Thanks @christian-rogobete, this is definitely helpful! I'll read up on the special imports and requirements on the host side. Will take this issue into consideration in our next planning.