feat: add a read-only mode to sends #1150
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1150 +/- ##
==========================================
+ Coverage 48.63% 49.75% +1.12%
==========================================
Files 128 128
Lines 11610 11385 -225
==========================================
+ Hits 5646 5665 +19
+ Misses 5964 5720 -244
|
Stebalien
force-pushed
the
feat/readonly
branch
5 times, most recently
from
0c155c5
to
89888c1
Compare
| // right now. It doesn't really cost anything anyways. | ||
| pub struct SendFlags: u64 { | ||
| /// Send in "read-only" mode. | ||
| const READ_ONLY = 0b00000001; |
There was a problem hiding this comment.
In the future, we may want other flags like:
- Isolated (can't make sub-calls).
- Silent (events are ignored). We could even split READ_ONLY into DENY_MUTATIONS and SILENT, but I don't want to do that now because there are some security concerns.
- "Pure" (can't read contextual information like the epoch, time, randomness, etc.).
| /// - Value transfers are forbidden. | ||
| /// - Events are discarded. | ||
| pub fn read_only() -> bool { | ||
| super::message::MESSAGE_CONTEXT.flags.read_only() |
There was a problem hiding this comment.
See #1151. This really doesn't belong in fvm_sdk::message.
| @@ -147,6 +147,8 @@ pub enum ErrorNumber { | |||
| Forbidden = 11, | |||
| /// The passed buffer is too small. | |||
| BufferTooSmall = 12, | |||
| /// The actor is executing in a read-only context. | |||
| ReadOnly = 13, | |||
There was a problem hiding this comment.
I considered using an existing error but:
- IllegalOperation is ambiguous on
set_root(returned both if the actor has already been deleted, or is in read-only mode). - Forbidden is ambiguous on
self_destructif the beneficiary is self.
In general, I'm starting to regret having "common" errors and am becoming convinced we want more specific ones. We can still share errors between syscalls, but errors like ActorDeleted is probably better than IllegalOperation.
Stebalien
force-pushed
the
feat/readonly
branch
2 times, most recently
from
4b68c86
to
6463bc3
Compare
|
Shouldnt events be allowed in readonly mode? |
| @@ -168,10 +168,11 @@ where | |||
|
|
|||
| fn with_transaction( | |||
| &mut self, | |||
| read_only: bool, | |||
There was a problem hiding this comment.
What's a reaonly transaction?
Kind of nonsensical, but i guess convenient in not having to change existing code.
There was a problem hiding this comment.
Yeah, it is kind of subtle... I'm effectively just counting how many times I've entered "read-only" mode so I can exit when I get back to the top.
| fn append_event(&mut self, evt: StampedEvent) { | ||
| self.events.push(evt) | ||
| if !self.is_read_only() { |
There was a problem hiding this comment.
So they are allowed, just not emitted.
| ) | ||
| }) | ||
| .or_fatal()? | ||
| self.assert_writable()?; |
They are, they're just dropped.
Eventually, I'd like to split "read-only" into multiple limits, but this is a good place to start. |
|
@raulk FYI, I kept it as "read-only" because it's now actually read-only (we're rejecting calls to |
We'll need this in some of the actors to convert syscall errors to "exit codes".
Stebalien
force-pushed
the
feat/readonly
branch
from
6463bc3
to
fe56bee
Compare
Adds a read-only mode for sends.
The general approach here is to reject mutations at the mutation site instead of early. That means we'll charge gas first. The alternative would be to check up-front at the top of the syscall, but that's non-trivial because a send may automatically create an actor in some cases.