feat(avm): tag checking, raising errors and stop execution

[jeanmon]

Resolves #9745

[dbanks12]

Nice work! I commented with some questions and notes.

In general, I think this is a big step closer to what we want. I do wonder if we could avoid having this error return type and instead let opcodes raise a "reverted" field on the TraceBuilder class. This is similar to how in the simulator opcodes raise context.machineState.reverted. And then at the bottom of the execution loop, we check trace_builder.reverted before continuing?

I don't feel strongly that you need to do it this way, especially not as a prerequisite to merge. BUT it would be a much smaller changeset I think.

I'm also surprised that you can change the tag types in a handful of places without changing brillig generation.

[dbanks12]

what's the difference?

[dbanks12]

I doubt bytecode validation will check that the enum value is right. Same holds true about the runtime error thrown in the switch case statement in op_get_env_var. Both should raise error flag without throwing an exception.

[jeanmon]

Related to assert vs. ASSERT: It does not change anything AFAIK, but the latter was usually the adopted style throughout our code base.

It is really an assertion and not error handling because this function is called only from the externally exposed routines: op_l2gasleft() and op_dagasleft

In other words, this is a coding mistake.

Related to op_get_env_var, it is guarded by the first if. the "default" case in the switch is to make the compiler happy but we have the guarantee that it cannot happen. I will add a comment about this.

[dbanks12]

I'm surprised that we don't need to change Brillig for changes to opcodes like this?

[jeanmon]

Possibly ECADD has never been used in brillig.

[jeanmon]

Nice work! I commented with some questions and notes.

In general, I think this is a big step closer to what we want. I do wonder if we could avoid having this error return type and instead let opcodes raise a "reverted" field on the TraceBuilder class. This is similar to how in the simulator opcodes raise context.machineState.reverted. And then at the bottom of the execution loop, we check trace_builder.reverted before continuing?

I don't feel strongly that you need to do it this way, especially not as a prerequisite to merge. BUT it would be a much smaller changeset I think.

I'm also surprised that you can change the tag types in a handful of places without changing brillig generation.

@dbanks12 True, setting the error/revert in trace_builder and having execution loop checking it at the end of the loop would have been less tedious change. The current change which sets the error at each opcode function call could have been avoided. As we plan a big refactoring in any case, I would not revert this now (especially you mentioned you can live with it.)
.cc @fcarreiro for awareness.