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

function trapped on footprint insufficient #1113

Open
leighmcculloch opened this issue Oct 14, 2023 · 6 comments
Open

function trapped on footprint insufficient #1113

leighmcculloch opened this issue Oct 14, 2023 · 6 comments
Assignees

Comments

@leighmcculloch
Copy link
Member

I've been noticing that when my footprint is insufficient I get a generic function trapped error message.

However, when my cpu/read/write resources are insufficient I get a nice and specific resource error message.

Given that footprints are considered part of the resources, and even the footprint error event refers to resources being exceeded, it would be a better developer experience if there was a specific error message for footprints, or for at least the resource error message to be used in that case.

The function trapped seems too much like the application is broken, and points the developer in the wrong place.

@dmkozh
Copy link
Contributor

dmkozh commented Feb 15, 2024

This issue got lost and the change didn't make it in protocol 20. We could make the change in protocol 21, but I'm not sure if maintenance cost is worth it (we'll still need to maintain the old error code for protocol 20), not to mention that implementation might be a bit tricky as currently Core doesn't observe the exact HostError values at all.
Given that the operation result codes are super coarse and diagnostic events should be used in any case to figure out what happened, I would vote for closing this. Maybe we shouldn't really try to highlight operation error codes for Soroban at all as most of the time they don't tell much. FWIW most (if not all) of the footprint errors have diagnostic messages attached to them.

@dmkozh dmkozh closed this as completed Feb 26, 2024
@leighmcculloch
Copy link
Member Author

leighmcculloch commented Feb 26, 2024

I think we need to collect some more input. A couple folks have talked to me (cc @tomerweller I think you were one of them) about how the errors are extremely difficult to understand. Diagnostics help a lot, but this error being a function trapped error seems like a wrong categorisation of the error. Wrong categorisations are pretty confusing.

I'm not sure if maintenance cost is worth it

Could you elaborate on this? What is the significant maintenance cost in generating a new error?

(Apologies I seemed to miss your comment from two weeks ago and just saw this now when it was closed.)

@dmkozh
Copy link
Contributor

dmkozh commented Feb 26, 2024

Sorry, I've misinterpreted your thumbs up as agreement with my proposal to close this.

The maintenance cost is that since this has made it into protocol 20, we'll need to maintain the old behavior as well, so we'll need to keep both the old and the new semantics, which is of course not impossible, but it does make the code harder to maintain. If you feel like this is important, we can still do the change.

As for more broad issue with error readability, I would much rather prefer to not surface the error code at all. It probably has somewhat worked for classic, but for Soroban the error space is really unlimited and it just seems infeasible to represent with error codes.

@leighmcculloch
Copy link
Member Author

Sorry, I've misinterpreted your thumbs up as agreement with my proposal to close this.

I'm not sure how I thumbed that up, sorry!

@leighmcculloch
Copy link
Member Author

leighmcculloch commented Feb 27, 2024

I think the question we need to answer is do we need error categorisation.

Right now we seem to have approximately these buckets for errors:

  • Tx malformed - tx encoder/sender did something wrong
  • Soroban invalid - tx encoder/sender did something wrong (I'm not sure what hte full scope of errors exist here)
  • Resource exceeded - resource calc must have turned out different as expect
  • Function trapped - something happened during contract invocation

At a high level the categories help, although the first two I struggle to distinguish between.

If we don't need categorisation at all, would we squash the last 3 into a single error? From a developer/human reading point-of-view I think that'd be fine as long as diagnostic events are always available.

But what about software making decisions on a follow up action to take? What we lose without any categorisation is the ability for wallets to take some meaningful action based on a category of error.

@piyalbasu @fnando Is there a future where you see wallets taking different actions based on these types, or other types, of categories of errors? Or is that unlikely?

@dmkozh
Copy link
Contributor

dmkozh commented Feb 27, 2024

Tx malformed - tx encoder/sender did something wrong
Soroban invalid - tx encoder/sender did something wrong (I'm not sure what hte full scope of errors exist here)

FWIW these two are 'validation' time errors and they shouldn't appear at apply time unless validator tries to nominate a bad tx set. Also 'tx malformed' is a legacy error, we probably should attach diagnostic events to these as well.

But what about software making decisions on a follow up action to take?

Yes, that's a valid concern. In theory, 'resources exceeded' most of the time would mean that simulation was incorrect for some reason (even in case of perfect simulation that might happen due to ledger changes since simulation). But there are also trickier cases, like the contract might have inherently volatile behavior, or simulation might be wrong in some other way, e.g. produce invalid auth (which would result in 'trapped' error). So building logic based on the returned code value would be tricky (also most of the time the only available option is really to retry simulation and tx submission).

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

No branches or pull requests

3 participants
@leighmcculloch @dmkozh and others