#"impl_bignum_host_fns" panic instead of return error

13 messages · Page 1 of 1 (latest)

vestal dove
#

👋 Had a question about the implementation of impl_bignum_host_fns in the env (https://github.com/stellar/rs-soroban-env/blob/main/soroban-env-host/src/host/num.rs#L28) that I ran into while trying to add more num functions to the SDK's [u/i]256 bit structs.

It looks like I should be able to perform a bad action (for example. env.i256_add(i256::MAX, 123))) and get an Error returned to the caller.

When testing adding checked_ variations of the i256 and u256 functions, I'm seeing it panic instead of return a Result(Error) with:


Event log (newest first):
   0: [Diagnostic Event] topics:[error, Error(Object, ArithDomain)], data:"escalating error to panic"
   1: [Diagnostic Event] topics:[error, Error(Object, ArithDomain)], data:["overflow has occured", 115792089237316195423570985008687907853269984665640564039457584007913129639935, 1]

Code (soroban-sdk/src/num.rs):

    pub fn checked_add(&self, other: &U256) -> Option<U256> {
        self.env.check_same_env(&other.env).unwrap_infallible();
        if let Ok(val) = self.env.u256_add(self.val, other.val) {
            Some(U256 {
                env: self.env.clone(),
                val,
            })
        } else {
            None
        }
    }

Mostly confused because the self.err code path doesn't look like it forces a panic anywhere. Would appreciate some thoughts!

#

@shy scarab - tagging you as it relates to our convo about extending the 256 bit nums to be able to perform fixed point math with them.

shy scarab
#

ah, yeah, that's by design

#

host functions errors are not recoverable from the guest side, which is a design choice that's not going to change at this point

#

we can make certain functions recoverable if needed, such as try_call. but otherwise the default behavior is panic

#

(from the guest POV)

vestal dove
#

Sounds good. I'll omit the checked stuff for now, as I don't want it to hold up other changes and it seems like a much larger can of worms than what I am requesting.

I'll include some relevant PRs here for the feature request once they are ready. Thanks for the info!

shy scarab
#

yeah... basically, if you want a fallible host function, you definitely can implement it and pick a way of error handling (e.g. return void or the respective error), but there should be a good reason for that (preconditions should be tricky enough to verify before calling the fn)

vestal dove
shy scarab
#

yeah, I think we probably should these ops to behave like checked ops in rust (i.e. return option)

vestal dove
#

Sounds good - I can update that when I respond to the PR feedback.

shy scarab
#

let's add the rem function separately and do checked fns separately