FIP-0088: Add support for upgradable actors #873
Conversation
fridrik01
force-pushed
the
actor-upgrades
branch
2 times, most recently
from
f67f17f
to
b3cf913
Compare
fridrik01
force-pushed
the
actor-upgrades
branch
from
6e3fa66
to
876db52
Compare
fridrik01
changed the title
fridrik01
requested review from
momack2,
arajasek,
jennijuju,
kaitlin-beegle,
anorth,
raulk and
jsoares
as code owners
|
Peer q: how can we leverage this with buitin actors? anyways to simplify network upgrade process when simple actor code change involved? |
|
editor: FIP itself has all the components needed clearly written. Approving this as a DRAFT with one editorial clarification request. However, I think we need more discussion on whether this is a desired feature on Filecoin network (before it can be considered for last call)& have better understanding on security implications for network application users when user actors aren't immutable. i.e: Defi and so on. See here |
|
|
||
| ## Incentive Considerations | ||
|
|
||
| This FIP does not materially impact incentives in any way. |
There was a problem hiding this comment.
Is this true? I'm curious how this change may impact the logic around timing and resource use for network upgrades, as well as the prioritization of certain changes.
If actors' code can be much more efficiently upgraded, shouldn't we technically be able to push more updates/small changes more quickly?
This change may not effect cryptoeconomics, but I might request we add a line about affecting the logic of what gets included for upgrades. Ultimately a small nit; the approval of the draft shouldn't be gated on this topic.
There was a problem hiding this comment.
@kaitlin-beegle I don't think I get why those are incentive considerations rather than product considerations.
please discard this comment - thanks to the clarification here |
|
|
||
| ## Incentive Considerations | ||
|
|
||
| This FIP does not materially impact incentives in any way. |
There was a problem hiding this comment.
@kaitlin-beegle I don't think I get why those are incentive considerations rather than product considerations.
fridrik01
force-pushed
the
actor-upgrades
branch
from
876db52
to
30339ac
Compare
| The `UpgradeInfo` struct is defined as follows: | ||
|
|
||
| ```rust | ||
| #[derive(Clone, Debug, Copy, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)] |
There was a problem hiding this comment.
Nit: I don't think these Rust codegen things are relevant to the FIP.
|
|
||
| When a target actor's `upgrade` Wasm entrypoint is called, it can make necessary state tree changes from the calling if needed to its actor code. The `UpgradeInfo` struct provided by the FVM runtime can be used to check what code CID its upgrading from. A successful return from the `upgrade` entrypoint instructs the FVM that it should proceed with the upgrade. The target actor can reject the upgrade by calling `sdk::vm::exit()`` before returning from the upgrade entrypoint. | ||
|
|
||
| ### New upgrade_actor syscall |
There was a problem hiding this comment.
Nit: the entry point and syscall are presented in the opposite order to which they happen in during an upgrade, which makes grokking the flow a bit harder. I drew a completely wrong idea about the upgrade method until I finished reading the syscall spec.
| 6. Start a new Call Manager transaction: | ||
| 1. Validate that the calling actor has not been deleted. If so, the syscall fails with a "IllegalOperation" (2) syscall error. | ||
| 2. Update the actor in the state tree with the new `new_code_cid` keeping the same `state`, `sequence` and `balance`. | ||
| 3. Invoke the target actor's `upgrade` entrypoint. |
There was a problem hiding this comment.
Please be very clear which code is invoked here. Since the prior step changed the code CID, I'm assuming it's the new code, but "target actor" also makes it sound like the old code.
|
|
||
| ### New upgrade Wasm Entrypoint | ||
|
|
||
| We introduce a new optional `upgrade` Wasm entrypoint. Deployed actors must implement this entrypoint to be a valid upgrade target. It is defined as follows: |
There was a problem hiding this comment.
Why must deployed actors implement it? It's not invoked on deployed actors, it's invoked on the new code. The algorithm below does not specify any check that the deployed actors implement this entry point
| } | ||
| ``` | ||
|
|
||
| When a target actor's `upgrade` Wasm entrypoint is called, it can make necessary state tree changes from the calling if needed to its actor code. The `UpgradeInfo` struct provided by the FVM runtime can be used to check what code CID its upgrading from. A successful return from the `upgrade` entrypoint instructs the FVM that it should proceed with the upgrade. The target actor can reject the upgrade by calling `sdk::vm::exit()`` before returning from the upgrade entrypoint. |
There was a problem hiding this comment.
This first sentence is a bit unclear
| When a target actor's `upgrade` Wasm entrypoint is called, it can make necessary state tree changes from the calling if needed to its actor code. The `UpgradeInfo` struct provided by the FVM runtime can be used to check what code CID its upgrading from. A successful return from the `upgrade` entrypoint instructs the FVM that it should proceed with the upgrade. The target actor can reject the upgrade by calling `sdk::vm::exit()`` before returning from the upgrade entrypoint. | |
| When the `upgrade` entry point is invoked in the new code, it can make any necessary state tree changes required by that new code. The `UpgradeInfo` struct provided by the FVM runtime can be used to check what code CID its upgrading from. A successful return from the `upgrade` entrypoint instructs the FVM that it should proceed with the upgrade. The new code can abort the upgrade operation by calling `sdk::vm::exit()`. |
|
|
||
| ### New upgrade_actor syscall | ||
|
|
||
| We introduce a new `upgrade_actor` syscall which calls the `upgrade` Wasm entrypoint of the calling actor and then atomically replaces the code CID of the calling actor with the provided code CID, and returns the exit code and block of the return. It is defined as follows: |
There was a problem hiding this comment.
The algorithm below specifies the opposite order of things: the code is changed then upgrade is invoked (in the new code). The opposite order makes sense, since the old code can't possibly know what state schema is required by the new code, while the new code can know both schemas.
|
|
||
| Upgradable actors pose potential security risks, as users can replace deployed actors' code. However, measures are in place to minimize these risks: | ||
|
|
||
| - Upgradable actors are opt-in by default, ensuring no impact on currently deployed actors. |
There was a problem hiding this comment.
I don't understand from the text above how this is enforced.
There was a problem hiding this comment.
Update: the text above doesn't say this, but I now do understand. It's opt-in because an actor code must have some path that invokes the upgrade syscall in order to be upgradeable. The stuff about requiring the upgrade entrypoint still seems wrong, but the fact that actors can only upgrade themselves means that an actor can be determined to be non-upgradeable by lack of such a call (up to malicious obfuscation, etc).
I suggest this mechanism and property be spelled out more clearly.
jsoares
changed the title
|
Assigned FIP-0088; please update the FIP frontmatter, title heading, and readme.md accordingly. @anorth's review also raises important points that should be addressed before merging. |
|
@fridrik01 Review comments have been pending for 5 weeks now. Could you clarify the status and ETA? |
|
Closing, will open once I have addressed the comments, likely next week |
Discussion: #396
See filecoin-project/ref-fvm#717
Implementation in filecoin-project/ref-fvm#1866