0xPolygonMiden / miden-vm

STARK-based virtual machine
MIT License
631 stars 161 forks source link

Include the constant name for assert error messages #1289

Open hackaugusto opened 7 months ago

hackaugusto commented 7 months ago

We are using assert.err=<code> to give context for a given error. <code> may be a integer or a constant, a failing assert like assert.err=ERR_INVALID_NOTE_TYPE will then result in an error similar to:

ExecuteTransactionProgramFailed(FailedAssertion { clk: 10244, err_code: 131140, err_msg: None })

There are a few issues with the above:

The above makes it unnecessarily convoluted to find the corresponding error. A simple fix for this is to add the string <code> in the error message. So the error above would be:

ExecuteTransactionProgramFailed(FailedAssertion { clk: 10244, err_code: 131140, err: "ERR_INVALID_NOTE_TYPE" err_msg: None })

If defined with a hex directly, i.e. assert.err=0x00020044:

ExecuteTransactionProgramFailed(FailedAssertion { clk: 10244, err_code: 131140, err: "0x00020044" err_msg: None })

And if defined with a number, i.e. assert.err=131140:

ExecuteTransactionProgramFailed(FailedAssertion { clk: 10244, err_code: 131140, err: "131140" err_msg: None })
bitwalker commented 7 months ago

I think this would be better solved by having proper source-level debug information associated with a program, which I think should be a pretty high priority in general because of how critical it is to user experience (and for us, in terms of efficient troubleshooting/debugging). To implement this as requested, we'd need to implement the same kind of auxiliary structure as would be needed for debug info anyway, so I see it as essentially the same amount of effort involved.

Source-level debug info also works equally well for code generated by the compiler. The vast majority of our users are more than likely going to be working in a higher-level language, not MASM, so we want to be able to emit errors/diagnostics that speak in terms the user understands. By using source spans, we can show the snippet of code associated with a particular error in whatever language it was actually written (assuming a program was distributed with debug info), whereas an error specific to MASM/MAST is not going to be particularly helpful.

I've been wanting to write up a proposal for debug info, but haven't been able to get to it, but this is a reminder to do that sooner rather than later I think.

PhilippGackstatter commented 4 days ago

I think having error messages as described in the original issue would be helpful, but I would like to see it done somewhat differently. I think the current error approach has mainly two issues:

I think source level debug information would be very valuable for the developer experience when debugging code, be it MASM or a higher-level language. But I think it would primarily help with adding context (e.g. I'm thinking about a stacktrace here with human-readable procedure names). What remains is that a developer still needs to lookup the error code themselves and that by itself can be a frustrating experience ("where do I start looking for this?"). I had exactly that experience when I was using another Smart Contract VM that only gave error codes and it was really frustrating having to lookup the error code every time it was failing. Once the error code is found and mapped to its human-readable error, it must still be interpreted, likely at the source itself to understand what condition failed the assertion.

Consider this procedure in the miden-base transaction kernel:

# Provided storage slot index is out of bounds
const.ERR_STORAGE_SLOT_INDEX_OUT_OF_BOUNDS=0x0002000D

#! Applies storage offset to provided storage slot index for storage access
#!
#! Panics:
#! - computed index is out of bounds
#!
#! Stack: [storage_offset, storage_size, slot_index]
#! Output: [offset_slot_index]
export.apply_storage_offset
    # offset index
    dup movup.3 add
    # => [offset_slot_index, storage_offset, storage_size]

    # verify that slot_index is in bounds
    movdn.2 add dup.1 gt assert.err=ERR_STORAGE_SLOT_INDEX_OUT_OF_BOUNDS
    # => [offset_slot_index]
end

This example is one of a typical procedure pretty deep in the call stack and it is likely called many times during the kernel execution. If one now reads "Provided storage slot index is out of bounds" as the error message of a program execution (which we have as a special case), it is not very helpful for debugging. Source-level debug information with a stacktrace would be very valuable here to get an idea at which point the error occurred and which of the procedures the developer was writing was being executed at that time.

But I still think that two things could improve the developer experience by a lot at this point:

  1. Not having to lookup the error message yourself.
  2. A better error message, that is, one that can be formatted with erroneous parameters.

As a side note, the kernel errors are special in the sense that we have some compile-time tools to create a map of error code -> error message, which is the comment above the error code. This is only a solution for the transaction kernel and is not a generic solution for third party libraries.

Native error messages

I think point 1 implies that the error message should be defined together with the assertion and needs MASM-level support at the language level. I'm thinking of something like this:

# verify that slot_index is in bounds
movdn.2 add dup.1 gt assert.err=ERR_STORAGE_SLOT_INDEX_OUT_OF_BOUNDS.msg="Provided storage slot index is out of bounds"

I imagine this would be debug-only information, could perhaps be stored as a decorator and would therefore not affect execution. It is most useful and needed at development time so it being debug-only seems totally fine.

Formatting errors The second point means being able to format error messages to provide much better errors, e.g. being able to include items from the stack in the message (here item at index 0):

"Provided storage slot index {0} is out of bounds"

We might then want some additional format specifiers like hex-format, e.g. {0:x} to lean on Rust's format language or {w0} to print the first word, and things like that.

A really good error message here would really be something like:

"storage slot index 2 is out of bounds as only access in range 0..=1 is allowed"

This would give the developer the information that they were trying to access the third slot (index 2) somewhere but their account component might only have been defined with two slots in total.

One issue is that formatting this message requires having those values on the stack and, as the function is written, they are not. I imagine this would be a fairly frequent problem. So we would either produce subpar messages or we would have to change the program to leave some items on the stack in case the assertion fails so that they are available for formatting. Changing the program might not be desirable as it would affect performance I imagine. Perhaps there are other solutions to this that I can't think about.

But I think the general points stand:

Overall I think this would complement source-level debug information very nicely. Knowing which line failed is very helpful but also having a parameterized error message would go quite far in helping the developer understand what the problem is. Quite frequently, a good error message can give the right hint and save time without having to set breakpoints, insert trace statements and start a full debug session.

Do you have any thoughts on this @bitwalker?

bitwalker commented 4 days ago

@PhilippGackstatter I agree that having a way to convert error codes to meaningful descriptions of what they mean is hugely helpful!

I think one example of doing this well, is with Rust itself. rustc allows one to obtain rich descriptive text explaining error codes it produces, e.g. rustc --explain E0433 documents the "use of undeclared type" error and potential causes/fixes. It has been quite helpful to me in a few cases where the error message itself was not descriptive enough.

The compiler currently provides source-level debugging capabilities like I described in my previous comment, via the midenc debug command, and eventually I imagine we'll be able to upstream its capabilities to the VM itself (once all the packaging stuff is upstreamed first). It provides:

That has vastly improved the debugging experience IMO. There is still some major room for improvement though (and naturally, the UI itself would benefit from better UX). I've found the following to be the biggest areas that I would personally like to see us focus on:

Regarding formatting, I have a few thoughts on this:

I think the most sensible solution is to make use of the existing Host::on_assert_failed callback to render pretty messages (this is basically why it exists). The problem is that the current APIs available in that callback (namely those provided from ProcessState) are insufficient to look at the stack items that were consumed by the instruction that the assert is relevant to.

As a toy example, consider is_odd assertz.err=1234. Let's say that we have introduced mappings from error codes to messages/format strings, and the error code 1234 is mapped to the format string "expected {0} to be an even number". We are immediately faced with an insurmountable problem - the value we care about was consumed by is_odd, and so is not visible to the assertion itself. One could rewind the state to recover the value, but determining what value(s) you need (and how many steps away they are) is the hard part.

One thing I'd like to try and get us in the mindset of, is that Miden Assembly being the primary way to interface with Miden should be considered a temporary, short-term state of affairs. My opinion is that our priority should be to add capabilities to the tooling and infrastructure of libraries/packages and runtime APIs for introspecting the state of programs and why they fail. Adding surface syntax to Miden Assembly should really be a secondary priority.

As an example of why using MASM as a starting point produces less-than-ideal results - let's say we introduce the error formatting infrastructure to the syntax. This is not going to be particularly useful to high-level languages being compiled to Miden Assembly. Rust, for example, provides panic formatting, which conceptually seems like it could take advantage of that functionality, but there is significant distance in semantics between Rust source-level panics and MASM assertions. Furthermore, we are compiling Rust to WebAssembly, which is a panic=abort target, so none of the formatting code comes through in the compiled Wasm anyway. In practice though, with source-level debugging, when a user-written panic is hit, the debugger is going to be sitting on the line containing the panic! itself, so threading the formatting code all the way through compilation isn't of any particular value IMO.

The debugger provides me with all the tools I need to examine values on the operand stack or in memory, and format them how I want them. What I don't have, and really want, is an explanation for compiler/assembler-generated assertions that may not have any associated source code, but are nevertheless associated with some invariant being enforced (think arithmetic overflow, bounds checks, etc.). We could specify error codes used by the compiler/assembler for such things, and in conjunction with user-defined error codes, be able to provide better contextual information about why a particular assertion was hit. I think it is questionable whether formatting facilities at the MASM level are really worth the complexity cost though, considering that you can just use the debugger to poke at the values yourself.

Anyway, I think that about summarizes my take on the current state of affairs and where the biggest wins are in the near future. I am basing all of this on the debugger in the compiler though, and the state of things without that is much worse. It is a priority (to me) to get the midenc debugger upstreamed, but some work remains before that can happen. Similarly, you can't really use midenc debug for non-Rust programs at the moment, as it relies on an experimental package format (in the process of being upstreamed) that is not yet supported by the assembler. Long story short, there is a spectrum of capabilities available for debugging Miden programs right now, but most of the fancy features are not generally available yet, but I think we should assume that those will get landed soon for purposes of designing/discussing future work in this area.

plafer commented 3 days ago

For example, if you are trying to reason about whether or not you have made some kind of off-by-one error in your operand stack management, we currently don't provide a useful notion of stack depth, and if your code involves valid uses of 0, you can't look at an array of 16 elements containing mostly zeros and make any kind of sense of what is going on there. I think we should improve this situation somehow, ideally it should be possible to distinguish (and interrogate) the "true" depth of the operand stack.

Totally agree. We could change how the "stack helper columns" work so that they also track the "true depth" of the stack (as opposed to only tracking when it exceeds 16).

Related: #1537