Skip to content

🧠 Logic: Lack of information when ErrorOutOfGas in interpreter #692

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

Closed
bdeneux opened this issue Jul 3, 2024 · 2 comments · Fixed by #694
Closed

🧠 Logic: Lack of information when ErrorOutOfGas in interpreter #692

bdeneux opened this issue Jul 3, 2024 · 2 comments · Fixed by #694
Assignees

Comments

@bdeneux
Copy link
Contributor

bdeneux commented Jul 3, 2024

📝 Description

When executing a predicate that results in an ErrorOutOfGas, the error message provided is misleading and lacks clarity. Specifically, the error message returned is a generic panic message (panic: {ValuePerByte}) that does not accurately reflect the underlying issue of running out of gas during the predicate's execution.

🔎 Example to Illustrate the Issue

Executing the following query results in an error message that does not clearly indicate the ErrorOutOfGas condition:

axoned query logic ask "findall(Pred, bank_balances(T, U), Pred)."

The response includes a vague error message:

answer:
  has_more: false
  results:
  - error: 'panic: {ValuePerByte}'
    substitutions: []
  variables:
  - Pred
  - T
  - U
gas_used: "100038"
height: "10194"
user_output: ""

The error message - error: 'panic: {ValuePerByte}' is not informative regarding the actual issue, which is an ErrorOutOfGas encountered during the retrieval of all balances within the interpreter.

⚙️ Technical Explanation and Current Handling

The mechanism for catching an ErrorOutOfGas is implemented but only triggers if the gas consumption limit is reached based on the predicate cost prior to the execution of the requested predicate.

defer func() {
if r := recover(); r != nil {
if gasError, ok := r.(storetypes.ErrorOutOfGas); ok {
err = engine.ResourceError(prolog2.ResourceGas(gasError.Descriptor, gasMeter.GasConsumed(), gasMeter.Limit()), env)
return
}
panic(r)
}
}()

However, this handling does not account for gas exhaustion that occurs during the execution of a predicate. As a result, when the gas limit is exceeded within the predicate execution, the error reported is the panic description catch by prolog interpreter.

FYI @ccamel @amimart

@bdeneux bdeneux self-assigned this Jul 3, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in 💻 Development Jul 3, 2024
@amimart
Copy link
Member

amimart commented Jul 4, 2024

Good catch! Indeed since this PR on the VM side panic are managed as prolog errors, what do you propose?

We can parse the prolog result to check if there is a panic and then check gas consumption to see if that was the failure source.

An alternative would be to consider panics in the prolog VM as exceptional behaviour managed outside the VM and make the call to Solutions.Next() panic as proposed in this PR: https://github.com/ichiban/prolog/pull/288/files. But that force us to make some changes in the VM.

The more we dig the more I think we should adapt radically the VM to our needs 🙄 ..

@bdeneux
Copy link
Contributor Author

bdeneux commented Jul 4, 2024

@amimart Exactly ! I've thinking about this solution for now, like it was done before https://github.com/axone-protocol/axoned/pull/555/files#diff-61dd11af86fdedcee9051d402c66b29ba0eb03373e9f20c4c9feb5dec6bc6e59L101-L104. It could be a good starting point without changes in VM; But, as you said, VM is beginning to show its limits :p.

PR in progress : #694

@bdeneux bdeneux linked a pull request Jul 4, 2024 that will close this issue
@bdeneux bdeneux changed the title Lack of information when ErrorOutOfGas in interpreter 🧠 Logic: Lack of information when ErrorOutOfGas in interpreter Jul 19, 2024
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in 💻 Development Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants