feat(avm): error handling for address resolution #9994
Conversation
jeanmon
requested review from
fcarreiro,
dbanks12 and
IlyasRidhuan
as code owners
| resolved[i] += Number(mem.get(0).toBigInt()); | ||
| const baseAddr = Number(mem.get(0).toBigInt()); | ||
| resolved[i] += baseAddr; | ||
| if (resolved[i] > maxUint32) { |
There was a problem hiding this comment.
Hmm I'm not sure this should be done here. And I mean that twice:
(1) a relative address that is too big is probably not bad until you try to resolve it
(2) in principle I'd move this to the "INDIRECT" if statement but...
These things might be better handled inside mem.get, which is the part of the system knowing about the memory limit. A red flag is that you had to import this maxUint32, but what if we change the memory size?
Also in your followup work, you will need to handle memory accesses in each opcode. I think what you are doing is assuming "if the resolution already 'constrains' the addresses, then they should be right". That is partially ok. But think of the cases where we read slices. Then the slice+length is not sanitized and it falls back to the memory component to validate it. Which it does, with an assert.
Overall, on the simulator side, it might be better to just change the memory assertions to exceptions.
There was a problem hiding this comment.
In cpp code, we have the type safety (uint32_t) which guarantees that we cannot read an address out of range. It would be very artificial, to upcast memory address to larger integer types. Alternatively, how should we defer (not in address resolution) the cpp error handling if an address is out of range? Probably, wrapping the uint32_t into a "MemoryAddress" type and keep a special value when we are out of range.
TS and cpp need to be kept in sync and errors need to happen at the same step of execution. We might add a "safeguard" on TS, by throwing an error when a memory read happens out of range, but this should only serve to detect a coding mistake (like an assert in cpp).
| // TODO(#9131): Error handling needs to be done | ||
| ASSERT(mem_tag == AvmMemoryTag::U32); | ||
| resolved[i] = static_cast<uint32_t>(mem_builder.unconstrained_read(space_id, resolved[i])); | ||
| const auto mem_tag = mem_builder.unconstrained_get_memory_tag(space_id, res_addr); |
There was a problem hiding this comment.
Just like I mention in the TS file, it might be better to just throw OutOfRange in the mem_builder and catch in the opcode implementation
jeanmon
force-pushed
the
jm/9131-error-handling-addr-resolution
branch
2 times, most recently
from
c08c648
to
79b2573
Compare
jeanmon
force-pushed
the
jm/9131-error-handling-addr-resolution
branch
from
79b2573
to
8c9282f
Compare
Changes to circuit sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
Resolves #9131