feat: always enforce state transitions from

[maht0rz]

Fixes #20

The relevant test case can be found in Balances.test.ts#194.

[maht0rz]

Reply to one of @rpanic review comments, posted as a separate comment to persist it down the line:

What was toValue doing before this changes? I'd like to understand it so I can judge whether the change makes sense. Bc in my understand the value is inside the Option already

Good catch, let me explain how i perceive the concept of Option, State and StateTransition currently:

Before this PR, we had State that'd be internally represented by an Option. Option was responsible for determining if a dummy value is used via isSome, and there was always a value + treeValue derived from isSome & value.

A StateTransition was created from two Options, and these two options were coming from the internal representation of the State itself. This resulted in the issue where if State was none, then the state transition e.g. from was also none. Because the two states were just merged into a StateTransition. This design worked just fine under the (wrong) assumption about how the StateTransitionProver should work.

The StateTransition was meant to be originated with its own new Option, but i just re-used the State's Option since it made sense at the time. Given that StateTransition has its own from/to Option values, i also attached a toValue to capture the actual value that needs to be written to storage in the end (even tho it was basically duplicate - which wasn't apparent to me at the time)

This PR aims to address the logic discrepancy between the original implementation of StateTransitions and the StateTransitionsProver.

I have a couple of issues with how i approached the implementation in the current PR and i actually think i can do it in a better way. First let me explain how the current PR implementation handles it and then i'll explain how i think it can be further improved for better conceptual health of the codebase.

Now the StateTransition is still based on the original State's Option for both from/to, but the 'from' part of the StateTransition is converted into isSome: true after it is 'serialised' to provable. ⚠️ This allows us to enforce the state transition after it was serialised into a ProvableOption, which means the treeValue was converted already respecting the original Option's isSome status. ⚠️

We need to support the following State variations, while always enforcing the StateTransition from:

Value in the merkle tree:

• Field(0) - value in the tree is considered empty
• h(Field(0)) - value in the tree was explicitly set to Field(0)
• h(AnyProvableValue) - value in the tree was explicitly set to AnyProvableValue

If the StateTransition re-uses State Options, the treeValue ends up being hashed depending on isSome: true/false, which will result into a wrong state transition being produced since we want the from state transition to always be isSome: true.

If you wanted to issue a State Transition where the original from state is isSome: false, value: Field(0), then the tree value would have been Field(0). But if you force the state transition from option to be isSome: true, because you want to enforce it, then the tree value becomes h(Field(0)), which is incorrect. As a temporary workaround i don't convert the Option itself to isSome: true, but only the ProvableOption where the treeValue was already processed.

This means that if we e.g. need to debug StateTransition itself, the state transition carries the full State's Option value, including the value & tree value.

Problem with the PR implementation is that you can't really get the real value of the from option of the StateTransition, since it will reflect the State's Option isSome status, since the conversion to isSome: true only happens after stateTransition.toProvable().

What i wanted to do:

To clear all of the concerns above, i was thinking of introducing frozen to Option, which would skip the hashing when treeValue is computed. Or alternatively a separate Optional interface with a FrozenOption implementation that extends the original Option. This would allow us to create an option isSome: true, value: Field(0), where the treeValue would stay Field(0) instead of h(Field(0)).

What i ended up doing:

I've implemented a property on Option called isForcedSome, which allows us to 'cast' the option to isSome: true, but skip the hashing when treeValue is obtained. This allows support for the one important edge case, where we want enforce a state transition from an empty value Field(0).

Looking forward to hearing your thoughts, but i believe the isForcedSome implementation is the most simple for now.

[rpanic]

I still don't fully understand why you think that we always need to enforce the "from" state. If the runtime calls state.get(), we need to enforce it, otherwise we don't. Why is that? Because in order for a runtime to access, and therefore manipulate, the current appchain state, it first has to get the state. If it doesn't do that it has no access to the current state (except by providing it via witnesses manually, but that is something we can't cover anyways) and the .set() call would be overwrites to the current state, which might be a intentional use-case. So I think that the risk of accidentially forgetting .get() inside business logic and therefore having an attack vector is non-existent and therefore it only adds complexity to our logic

[maht0rz]

I still don't fully understand why you think that we always need to enforce the "from" state. If the runtime calls state.get(), we need to enforce it, otherwise we don't. Why is that? Because in order for a runtime to access, and therefore manipulate, the current appchain state, it first has to get the state. If it doesn't do that it has no access to the current state (except by providing it via witnesses manually, but that is something we can't cover anyways) and the .set() call would be overwrites to the current state, which might be a intentional use-case. So I think that the risk of accidentially forgetting .get() inside business logic and therefore having an attack vector is non-existent and therefore it only adds complexity to our logic

The current implementation of State, always calls .get() internally when calling .set() to anchor the .set() call to the current state. This was done on purpose and we discussed it during the review of #7 and the result of that was #20.

Now in a case where you .get() for the first time and the Option isSome: false occurs, in that case you still want a StateTransition from: isSome: true, but you don't want to h(value) but just value (Field(0)), even if its a Struct with more fields, you just want Field(0). This concept is called forcedSome in the latest commit of the PR at the time of writing.

This can occur if you call .get() or .set() for the first time on for that given path/key. (remembering that get is called by set internally as i described above). So this PR would still make sense even if we did not want to enforce from on set calls.

If we don't want to enforce from on set calls, i think that could be just fine as well, because the state transitions are linked together via their parent hashlist, so you can't apply then in a wrong order - which was my main motivation to call .get() within .set() - to prevent miss-ordering of STs. But thanks to the hashlist that concern is definitely not relevant.

WDYT? keep this PR since it solves the initial .get() painpoint, and a separate PR to remove the from transition for a .set() It'd mean keeping what i implemented for Option with forcedSome, but would require a small change to StateTransition, where from can be empty and not forced some. But i think it warrants a separate PR right after this one.

[rpanic]

Thanks for your view, that makes sense! I think we can remove the implicit .get() in the future, but that is a performance optimization anyways and has no immediate practical impact. So now the PR lgtm!