Consensys / linea-arithmetization

17 stars 18 forks source link

Refactor Exceptions class - reduce number of allocations #800

Closed lu-pinto closed 1 week ago

lu-pinto commented 1 week ago

By looking at the Exceptions class I cannot see any change of state that's not through the prepare call which is only called once. This leads me to believe that we don't need to create new Exceptions objects every time with snapshot.

Also Exceptions can only have 1 boolean true at once, by looking at prepare so an Enum is the most suitable data structure for this case.

OlivierBBB commented 1 week ago

Also Exceptions can only have 1 boolean true at once, by looking at prepare so an Enum is the most suitable data structure for this case.

Could you shed light on this please ? What makes it so that only one may be active ?

Having at most one active exception is indeed what is intended when tracing exceptions at the end (i.e. when filling the stack columns for exceptions --- at most one may be on at any point in time) but AFAIK the exceptions object may have several exceptions active at any point in time. E.g. a staticException + the sstoreException (when the available gas prior to executing an SSTORE is $\leq G_\text{callstipend} = 2300$) for an SSTORE instruction.

My understanding was that we were logging all possible exceptions and then deciding a posteriori which to act upon, in any case that was what I remember explaining to Franklin who implemented this stuff. With some caveats e.g. the memoryExpansionException, a subexception of the outOfGasException, an invention of our arithmetization, would very likely always be disjoint.

lu-pinto commented 1 week ago

You're right @OlivierBBB, it was my oversight. Indeed prepare is being called before every opcode execution so all exceptions will be collected within Exceptions once the MessageFrame has finished executing. I reworked my code to now be (I believe) side effect free using a bit mask to record multiple exception types.

lu-pinto commented 1 week ago

I'm closing this PR to fix a few things to adhere to branch name and commit message standards after sync with @powerslider. Will reopen a new one.

lu-pinto commented 1 week ago

.