Open anorth opened 4 years ago
I closed this issue in error; reopened.
Some background information: We've been cleared to use the puppet actor for this.
@anorth (cc @frrist) - After digging into the actor code, I have a few observations:
The adt.Store#Put
and adt.Map#Put
methods, called from within actor code, return an error if the value they're provided cannot be CBOR encoded. The actor is then responsible for handling the error (typically by calling to runtime.Abortf
).
It's my belief that the actor should not be creating Sys
-prefixed errors - rather, those errors should be created by the runtime. Given the description of this issue ("which should result in a SysErrSerialization"), I have become convinced that using the two Put
methods is not the approach that we want.
If testing CBOR marshaling errors in the runtime is what we want, we could instead use Runtime#State#Transaction
to mutate an actor's state such that it failed to CBOR marshal (i.e. aborted the operation with a SysErrSerialization
). I have created a WIP PR to that end in specs-actors.
Thoughts?
@anorth @frrist - I met some resistance attempting to change lotus's use of ErrSerialization
to SysErrSerialization
within Runtime#Put
. I'm not sure how to proceed - please advise.
I am going to defer to @anorth on this.
I'm pretty sure I remember @anorth agreeing that things failing to serialize is an actor error not a runtime error. But we can wait for him to get back and give input here
The problem here was the issue description, which I either mistyped or predated our resolution to use ErrSerialization
for the parameter/return value serialization failures, a policy we should retain here.
the actor should not be creating Sys-prefixed errors
Correct.
we could instead use Runtime#State#Transaction to mutate an actor's state such that it failed to CBOR marshal
Yes, thank you, we should do that too -> filecoin-project/chain-validation#211. That should also result in ErrSerialization
, even though the error will in fact arise in the runtime. The serialisation is logically part of actor code but we've put it behind the runtime API for (possibly misguided) convenience.
I think you linked the wrong PR, but I am eager to see the WIP you referenced.
So in summary, ultimately the actor is responsible for choosing an exit code when Put()
returns an error. We should still exercise that case with the puppet actor, though this cannot enforce that other actors will respond with the same exit code (they'll probably use ErrIllegalState
). The test becomes a test that Put()
returns an handle-able error when serialization fails.
After reading the notes here, I'm not confident that this can be turned into a general test case that applies to all actors. What I'm concluding is that it's at the actors discretion to return an error code in actor space (not system space) when the internal store.Put
call errors.
@anorth Are there concrete builtin actor scenarios where a store.Put
can fail (maybe because it's embedding some user-provided input that can be erroneous)?
This is trying to test the runtime, not the actors, so not applying to all actors isn't necessarily a problem. We still need to exercise and demonstrate what happens when an actor attempts to put bad data.
I'm not aware of a builtin actor that will make a Put
that could fail, but even if there were one we shouldn't use it because these tests shouldn't depend on the actors that tightly. Use a puppet/chaos actor instead so the tests maintain direct control of the call to Put
.
As written right now, the store interface does not return errors, the VM handles the de/serialization: https://github.com/filecoin-project/specs-actors/blob/537f8df6696787c91d80b8b109bfd19a748522a6/actors/runtime/runtime.go#L154-L159
So this issue boils down to:
Put
aborts with ErrSerialization
when its argument returns an error from MarshalCBOR
Get
aborts with ErrSerialization
when its argument returns an error from UnmarshalCBOR
(assuming the key actually exists in the store)Get
returns false when the key does not exist in the store@anorth thanks for the clarification! We should be able to use the chaos actor to induce these circumstances.
See test cases to implement in https://github.com/filecoin-project/test-vectors/issues/54#issuecomment-676853617.
Actors can attempt to store data that cannot be serialized, which should result in ~a SysErrSerialization~ an error that the actor can handle by aborting with an appropriate exit code.
E.g. see https://github.com/filecoin-project/lotus/pull/1627