fix(fvm): fix (and prevent) two bugs in out of gas handling. #1830
Conversation
|
Syncing mainnet just to double check. |
|
Ok, the network is syncing just fine. |
|
So I think you're right that the tipset_cid change will behave exactly the same way, i'm just not SURE. Syncing mainnet seems like a weak test -- you're only gonna notice this if we actually have a message that invokes a contract that runs out of gas as it's querying a tipset_cid, right? We probably want to test this out more if we strongly care. The question is -- do we? We expect FVM master to break compatibility with mainnet soon anyway, right (as we prep for nv21). Can we just say now is that point? Or stage this in some |
|
I'm trying to keep the breaking changes out of master this time till the FIPs start landing, if at all possible. I guess this is a bug fix so not a FIP-able change? I'm going to just test this manually by explicitly setting the gas to zero in this method to make sure that these two cases are indistinguishable. |
Stebalien
force-pushed
the
steb/fix-gas-early-return
branch
from
0d457d3
to
a65b014
Compare
My last commit introduced a bug where I ignored the error from a call to `CallManager::charge_gas` when we _meant_ to simply ignore the gas timer, not the entire error. This bug was in the charge for `on_method_return`. So I decided to remove the `must_use` marker from gas timers _in general_ so we didn't have to explicitly ignore the results. We generally don't care about these timers anyways. While doing that, I found another bug in the `tipset_cid` syscall where we made the same mistake. this issue is tricker because this code is already deployed to mainnet, but we should be able to fix it without any consensus issues. Basically, right now: 1. We won't detect the out of gas condition when we should. 2. We'll fetch the requested tipset CID from state. 3. We'll return it to the user. 4. The actor (the EVM) will try to charge a bit of gas when handling the return value from the syscall. 5. The actor will run out of gas and abort with an OutOfGas. Now, we'll just abort with an OutOfGas error immediately.
Stebalien
force-pushed
the
steb/fix-gas-early-return
branch
from
a65b014
to
fb39cae
Compare
|
Ok, I've validated that the two cases are indistinguishable due to the SDK. I.e., we'll always run out of gas immediately anyways, even if we don't return an explicit out of gas error. |
My last commit introduced a bug where I ignored the error from a call to `CallManager::charge_gas` when we _meant_ to simply ignore the gas timer, not the entire error. This bug was in the charge for `on_method_return`. So I decided to remove the `must_use` marker from gas timers _in general_ so we didn't have to explicitly ignore the results. We generally don't care about these timers anyways. While doing that, I found another bug in the `tipset_cid` syscall where we made the same mistake. this issue is tricker because this code is already deployed to mainnet, but we should be able to fix it without any consensus issues. Basically, right now: 1. We won't detect the out of gas condition when we should. 2. We'll fetch the requested tipset CID from state. 3. We'll return it to the user. 4. The actor (the EVM) will try to charge a bit of gas when handling the return value from the syscall. 5. The actor will run out of gas and abort with an OutOfGas. Now, we'll just abort with an OutOfGas error immediately.
My last commit introduced a bug where I ignored the error from a call to
CallManager::charge_gaswhen we meant to simply ignore the gas timer, not the entire error. This bug was in the charge foron_method_return.So I decided to remove the
must_usemarker from gas timers in general so we didn't have to explicitly ignore the results. We generally don't care about these timers anyways.While doing that, I found another bug in the
tipset_cidsyscall where we made the same mistake. this issue is tricker because this code is already deployed to mainnet, but we should be able to fix it without any consensus issues. Basically, right now:Now, we'll just abort with an OutOfGas error immediately.