Mutation of actor state is allowed when readonly or outside of transaction

[alanshaw]

These test vectors show that lotus responds with exitcode.Ok when readonly state is modified or state acquired for a transaction is modified after the transaction has completed.

Don't panic. These vectors also show that the illegally mutated state is not persisted 😅, but the exit code is expected to be SysErrorIllegalActor and I assume also that any legal state mutations should be discarded. @anorth maybe you can confirm on that last point?

Version (run lotus version):

v0.5.8-0.20200903221953-ada5e6ae68cf

Additional context

• https://github.com/filecoin-project/specs-actors/blob/8aa5fa79633950f8e245713298ad5e12502410c7/actors/runtime/runtime.go#L227-L253
• https://github.com/filecoin-project/test-vectors/issues/56

[austinabell]

There are no cases in specs-actors which mutate the retrieved state, I'm not sure if an outdated comment but I don't see modifying retrieved state as an abort (or panic in case of outside of the transaction).

I also don't see how you enforce that state is not mutated outside of the transaction (specs-actors only reads state after a transaction in two places https://github.com/filecoin-project/specs-actors/blob/8aa5fa79633950f8e245713298ad5e12502410c7/actors/builtin/multisig/multisig_actor.go#L235 and https://github.com/filecoin-project/specs-actors/blob/8aa5fa79633950f8e245713298ad5e12502410c7/actors/builtin/multisig/multisig_actor.go#L448) and does not ever mutate outside of the transaction, those are only read only accesses. There might be other cases but there certainly isn't any state modifications outside of a transaction.

As for mutating "ReadOnly" state, there is currently no examples of this in specs-actors either. (can easily confirm because in rust the state would have to be marked explicitly as mutable, and there are none).

We would just skip or add custom logic to match functionality just for the test because in our impl we have our transactions setup to not keep the object around after the transaction without an explicit clone, and I don't see any benefit in testing/ enforcing this.

Just giving my two cents

[raulk]

@austinabell I see what you mean. I think the reason why these test vectors were requested was purely to ensure pragmatic VM correctness, in isolation with the actual usage of the VM today. The disconnect between "let's test this case" and "there are no current instances of this kind of usage" is rather intentional here.

Maybe what's missing here is a proper VM specification. If we're are creating this test case it's because in that hypothetical correct-and-complete VM spec, there would be a requirement stating that VM execution must fail when the method mutates state improperly.

As for how would you implement that constraint, I had exactly the same question. @anorth pointed out that go-filecoin implemented this check by recording/tracking the CID of the retrieved object in various calls to the VM. I guess you would:

• record the actor head CID before calling the method.
• record the CID before a transation.
• record the CID after the transaction.
• check the actor head when the method exits.

That probably gives you enough data to validate this constraint?

[raulk]

@austinabell small follow-up to my comment. specs-actors is intended to be an executable spec. There should probably be a written spec doc for the VM, but in lieu of that, these godocs serve as a spec:

https://github.com/filecoin-project/specs-actors/blob/8aa5fa79633950f8e245713298ad5e12502410c7/actors/runtime/runtime.go#L227-L253

Thanks @alanshaw for reiterating the pointer to me separately.

[austinabell]

As for how would you implement that constraint, I had exactly the same question. @anorth pointed out that go-filecoin implemented this check by recording/tracking the CID of the retrieved object in various calls to the VM. I guess you would:

• record the actor head CID before calling the method.
• record the CID before a transation.
• record the CID after the transaction.
• check the actor head when the method exits.

That probably gives you enough data to validate this constraint?

I'm just missing the point of why you would need to load a pointer to state that would get persisted automatically by the VM. I see the two cases:

1. Retrieving readonly -> This is not persisted in any way, so who cares if it's mutated in memory (even though it never is)
2. Transaction -> it's loaded at the start of the transaction, and persisted at the transaction end not at the end of the call, so why does it matter what happens with the variable if you decide to keep it around in your implementation?

The only time you should be able to modify the state that is persisted is at the end of the transaction or with a create.

I'm not saying you shouldn't test it, there may be a use case in Lotus to ensure certain functionality I'm missing, I'm just sharing that we would just skip these tests because we cannot run into this issue and it isn't really testing anything wrt the protocol. I would think if you modify the readonly state pointer and it gets persisted, that's probably bad design

Edit: Ignore previous edit, I don't see how this could be possible, unless your in memory blockstore (from the buffered blockstore) can allow you to modify a retrieved value, does go even let you do this? If so, yes it would be worth to add the test I suppose, but this is just testing functionality that is memory unsafe if you can get it to fail

[anorth]

Yes, I expect SysErrorIllegalActor. There are no cases of in in actor code because go-filecoin enforced this, which immediately weeded out any instances. The point of this is to force actor code to follow good patterns. Modifying state that won't be persisted is almost certainly a bug.