vyperlang / titanoboa

a vyper interpreter
https://titanoboa.readthedocs.io
Other
236 stars 37 forks source link

Transient storage is persisting #216

Closed sajal closed 2 months ago

sajal commented 2 months ago

I'm not sure if this is a bug with boa, or pyevm, or that I'm doing something silly., here is the py-evm bug report ethereum/py-evm#2177

Basically at end of transaction, transient storage is not being reset.

py-evm==0.10.1b1 titanoboa @ git+https://github.com/vyperlang/titanoboa@4eaa59e6c4d884e6280e0d970e566044271942a6

Reproduce step:

import boa

def test_transient():
    cont = boa.loads("""
#pragma version 0.3.10
#pragma optimize codesize
#pragma evm-version cancun
cache: transient(uint256)

@external
def __init__():
    pass

@external
@nonpayable
def set(num: uint256):
    self.cache = num

@external
@nonpayable
def get() -> uint256:
    return self.cache
            """)

    assert cont.get() == 0
    cont.set(100)
    assert cont.get() == 0
charles-cooper commented 2 months ago

it's expected behavior, not a bug. titanoboa executes all code within a single transaction context. the correct thing you want to do is call clear_transient_storage() in between contract calls, but we didn't add any machinery to titanoboa yet to do it.

closing this since it's not really a bug, but you can keep commenting here if you still need help.

sajal commented 2 months ago

This could be problematic.

Since we use transient storage as a cache, we now need to add command to flush it after each operation, which we can do because its our own contract in Vyper, however our goal is to have a lot of tests integrating with external protocols (in a forked network), we have low visibility into these protocols who may or maynot be using transient storage themselves, and the values not resetting will cause a lot of hard-to-debug issues in the future.

charles-cooper commented 2 months ago

this is by design - you have a higher guarantee of correctness from testing if you are forced to work against the assumption that each call is isolated in a single transaction. in real EVM, you don't have this guarantee, so IMO your tests should not bake in the assumption.

sajal commented 2 months ago

you have a higher guarantee of correctness from testing if you are forced to work against the assumption that each call is isolated in a single transaction

Thats fair, however in our contract, during the transaction, we invalidate the transient cache whenever we expect it to change. This issue only manifested itself because in my test I time traveled to the future, it wouldn't have been an issue if it was in a subsequent call within the same tx (or same block even).

Perhaps consider a feature to flush transient if we time travel?

charles-cooper commented 2 months ago

you have a higher guarantee of correctness from testing if you are forced to work against the assumption that each call is isolated in a single transaction

Thats fair, however in our contract, during the transaction, we invalidate the transient cache whenever we expect it to change. This issue only manifested itself because in my test I time traveled to the future, it wouldn't have been an issue if it was in a subsequent call within the same tx (or same block even).

Perhaps consider a feature to flush transient if we time travel?

that's a good idea. can you create an issue for this? i think there are some edge cases, like what happens if you travel backwards to a previous block.

sajal commented 2 months ago

Will do. BTW I didn't realize backwards time travel was possible!

charles-cooper commented 2 months ago

Will do. BTW I didn't realize backwards time travel was possible!

yes, it is, and in fact maybe we should ban it in the high level api :smiling_face_with_tear: