Test that a panic in actor code results in undefined execution

[anorth]

If an actor implementation panics directly (rather than calling Abortf) then the evaluation is undefined. There is no exit code corresponding to this. The result should not go on chain. A panic (which could also come from some actor dependency) may indicate a transient state or error that cannot be replicated by other nodes and thus cannot form part of consensus. E.g. an out-of-memory.

Test that when an actor panics, the VM indicates this by some kind of failure that is distinguished from an Abortf and has no risk of going on chain.

This could be a little tricky, and implementation specific.

[anorth]

FYI @Stebalien.

[raulk]

@anorth mind scoring? Usual drill (pick a discomfort factor, remove the needs-scoring label, unassign yourself). Thanks!

[Stebalien]

Test that when an actor panics, the VM indicates this by some kind of failure that is distinguished from an Abortf and has no risk of going on chain.

I believe the panic is currently caught and an exit status of 1 is recorded.

[anorth]

This one is pretty bad: it will lead to a fork (though probably one localised to one miner).

[raulk]

@alanshaw here's another 9! Should jump the queue from the 8's you had planned.

[raulk]

@alanshaw as @anorth duly points out, this is gonna be rather tricky because there is no exit code associated with this. Rather, we expect the VM to return an error.

Right now, test vectors don't record internal errors that are exogenous to the protocol, as these are implementation specific.

One way of doing this is recording an empty receipt in the vector, which the driver would interpret as an error. WDYT?

[alanshaw]

As we discussed in the colo, I had an idea for recording the error in a separate section of the postconditions to the receipts:

// 3 messages to apply but the 3rd causes a panic and fails entirely, with no receipt.
"apply_messages": [
  { "bytes": "...", "epoch": 0 },
  { "bytes": "...", "epoch": 0 },
  { "bytes": "...", "epoch": 0 }
],

"postconditions": {
  // Indexes into the apply_messages array, identifying which message(s) failed entirely, with no receipt.
  "apply_message_failures": [2],

  // Receipts as usual, in this case we'll have just 2.
  "receipts": [/* ... */],
}

The idea is that the driver could verify exactly which message(s) failed to be applied. There's probably no value in recording things like error messages since this will be implementation dependent. The driver could also verify the receipts for messages that did apply successfully (although see open questions below).

We need to use an index into the apply_messages array because we want to verify the intended message caused the failure. We may also want to test a case where we successfully apply 1 or more messages before failing, and we may also like to test a case where we load up 10 messages but assert that the vm quit on message 5, and didn't create receipts for the rest.

The questions I have are:

1. Does it need to be an array? Does/should the driver stop applying messages after the first failure?
   • EDIT: I think the answer is yes it should be an array and no the driver should just continute to apply messages
2. If a message fails, should we still record receipts for prior messages that did succeed even though the state root will not change?
   • EDIT: Yes because the state root will have changed after every message

For completeness, here's some reasons for not using an empty receipt in the receipts array:

• It's not actually a receipt!
• We'd have to heavily document this behaviour, since it wouldn't be obvious from simply looking at the vector JSON
• Not potentially confuse an empty receipt as a valid return receipt.
  • The nil value of ExitCode will be zero (success) and I believe implicit messages like those seen in the tipset vectors carry 0 exit code, 0 gas and "" return, which I think will be indistinguishable...
• We don't want to encode an "is_error" field (or something like that) in an empty receipt since it's not actually part of a real receipt.

[raulk]

The questions I have are:

1. Does it need to be an array? Does/should the driver stop applying messages after the first failure?
   • EDIT: I think the answer is yes it should be an array and no the driver should just continute to apply messages

Agree.

2. If a message fails, should we still record receipts for prior messages that did succeed even though the state root will not change?
   • EDIT: Yes because the state root will have changed after every message

Agree.

For completeness, here's some reasons for not using an empty receipt in the receipts array:

• It's not actually a receipt!
• We'd have to heavily document this behaviour, since it wouldn't be obvious from simply looking at the vector JSON
• Not potentially confuse an empty receipt as a valid return receipt.
• The nil value of ExitCode will be zero (success) and I believe implicit messages like those seen in the tipset vectors carry 0 exit code, 0 gas and "" return, which I think will be indistinguishable...
• We don't want to encode an "is_error" field (or something like that) in an empty receipt since it's not actually part of a real receipt.

Agree with all these points.

---

Conclusion: ship it! 🚀

---

cc @austinabell As discussed on Slack!

[austinabell]

Yeah I would say all is valid reasoning, lgtm. I would say that I don't think that having an array of invalid indexes and an incomplete receipt list is just as unintuitive as having a nullable receipt. I just think the benefit to having a null or even putting a string error in place of the receipt (can go deserialization do this easily of having a data type of two variants?) is that the indexes will match the message indexes, and otherwise would be confusing. Doesn't matter just sharing thoughts.

Question I have is: can there be more than one error or have the error as the non-last message? iirc the VM execution states something like if the error if fatal, then the resulting state is undefined and shouldn't be used. There won't be a functional issue for us, but would this potentially be testing functionality or hit an edge case that cannot happen on a network? No harm in trying I guess, just thinking out loud

[raulk]

I think @austinabell has a point as well. Is there a hybrid that could be best-of-both-worlds here? I'm thinking:

• Ideally message receipts would have the same cardinality as applied messages.
• We can leverage JSON null to mark receipts that do not exist.
• To make the reason why a receipt doesn't exist extremely explicit, the apply_messages_failures array indexes the messages that lead to failures.

[alanshaw]

I agree also, for some reason I didn't consider a null value.