feat: allow readonly delegatecalls to the call actor precompiles

[wjmelements]

Reviewer @rvagg

Changes

Allow readonly delegatecall

Currently we require call actor (by address or by id) to use delegatecall.
But if the delegatecaller context is readonly (it or one of its parents is a staticcall), that fails.
The reason seems to be that we do flush, so that reentrancy works (since it will reload).
However, we skip flush if there's nothing to flush, and I think that's the case in this situation.
So I think we can allow this case.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.57%. Comparing base (0e1b559) to head (af8c82a).
⚠️ Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1739      +/-   ##
==========================================
+ Coverage   90.56%   90.57%   +0.01%     
==========================================
  Files         140      140              
  Lines       27814    27840      +26     
==========================================
+ Hits        25189    25217      +28     
+ Misses       2625     2623       -2     

Files with missing lines	Coverage Δ
actors/evm/src/interpreter/precompiles/fvm.rs	99.29% <100.00%> (+0.97%)	⬆️
actors/evm/src/interpreter/system.rs	90.39% <ø> (+0.23%)	⬆️
runtime/src/test_utils.rs	84.81% <100.00%> (+0.08%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[rvagg]

@wjmelements can you write a test for this that fails without the fix? This change hinges on saved_state_root not being set to None. Avoiding a set_state_root when readonly seems like the right thing to do and saved_state_root is set to None by the mutation operations in interpreter/system.rs (increment_nonce, set_bytecode, set_storage, set_transient_storage, mark_selfdestructed) so it's an important signal that "there was a mutation". The other path is via new(), which is sus because you can only hit that with readonly true if the actor is_dead(). Maybe this is where you're hitting it? That might be able to happen for a future message hitting an actor that selfdestructed in a prior one, where we'd be artificially setting readonly (maybe that's why you have the flags |= SendFlags::READ_ONLY?) and leaving a None saved_state_root; execpt I can't see how you'd actually reach flush() from there. InvokeContract on a dead actor short-circuits before flush (empty bytecode) and InvokeContractDelegate is self-only so a dead actor can't dispatch it. Within the same message it's not even a question because a selfdestruct sets a tombstone matching the current message so is_dead() returns false (i.e. is_dead() only works on actors that were actually marked dead prior to the current message). What am I missing?

ext_opcodes.rs has some relevant tests, that might be a good place to add a new one but you'll need to override MockRuntime::read_only() to make it true.

[wjmelements]

I've added tests that would fail without this change and modified the MockRuntime::readonly to facilitate the testing. The test added in ba526ad passes on origin/master but fails on this branch. That test's correctness condition is updated in subsequent commits so that it passes on this branch.

I'll also add integration tests into ref-fvm, since without the explicit flag propagation, we depend on it to propagate the readonly state into subcalls.

What am I missing?

There are filecoin methods that are read only; we should be able to execute them within a readonly context. It's still a separate hurdle that we can't call non-InvokeEVM methods without delegatecall (which invalidates solidity's view qualifier).