feat(authority): support failing validation effects commit post-consensus #8927
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 6 Skipped Deployments
|
iota-ci
added
sc-platform
…for_invalid_authentication
… fix it in the latest executor call
There was a problem hiding this comment.
In this PR, we change the conception of how a transaction with a move authenticator is executed. Since an authenticator can produce the result, I think we need to refactor the way it is called. We need a stronger connection between an authenticator and the transaction, and don't consider the authenticator call as something independent.
There was a problem hiding this comment.
I checked the code mostly from technical perspective. Can't really comment on the design choices, will trust @valery to handle that.
Only thing concerning was the O(n^2) object merger, but lack the deep knowledge on the domain to suggest something better.
With the above disclaimers I approve.
| // if additional_mutable is true and | ||
| // input_mutable is false, then swap | ||
| if additional_mutable && !input_mutable { | ||
| existing_object.input_object_kind = | ||
| InputObjectKind::SharedMoveObject { | ||
| id: input_id, | ||
| initial_shared_version: | ||
| input_initial_shared_version, | ||
| mutable: true, | ||
| }; | ||
| } |
There was a problem hiding this comment.
What is a use case for this?
Authenticator inputs are always immutable, so it might make sense to create a more specific function to extend transaction inputs with authenticator ones?
Can we consider this function(check_and_extend_input_objects) as a method of the InputObjects struct? It looks generic enough to be added as extend_no_duplicates, like it was done before.
There was a problem hiding this comment.
In this PR, we will need to push the account object into the list as well: #8986
There was a problem hiding this comment.
About the if additional_mutable && !input_mutable check:
- this extensions mutates the left side (input) and reads from the right side (additional);
- where we now call it, we now that the left side is tx_inputs and right side is auth_inputs;
- however, in a more general case, if we call
check_and_extend_input_objectswe assume the left and right have been checked already to be good for their cases (now that I am thinking about it, the function signature should be (input_objects: &mut CheckedInputObjects,additional_objects: &CheckedInputObjects), changed in a5589fc); - this means that the right side, might have a shared object which is mutable (maybe in the authenticatorV2?) and we are good with it;
- then, from the point of view of the check_and_extend_input_objects function, if one of the sides has
mutable:true, then in the final result the mutable must be true; this because keepingmutable:falsein the final result when one of the two sides hadmutable:truewill break the check that happened before callingcheck_and_extend_input_objects.
There was a problem hiding this comment.
About the rest of the comments: yes, we need to push the account object as well and potentially also the sponsor authenticator input objects; this I think it could be better handled in a dedicated issue.
There was a problem hiding this comment.
idk, for me this logic is misleading, because in our case, if the right side(auth inputs) contains mutable shared objects, it is an error which shouldn't happen. We should have at least a debug assert here, since we don't really expect such inputs here. We can remove this assert when it is needed.
…nch' into vm-lang/aa-auth/8908-move-auth-inputs
miker83z
merged commit 821de7b
into
vm-lang/aa-auth/8116-feature-branch
5 participants
Description of change
This makes so that a move authentication passing pre-consensus (validators sign the TX) and failing post-consensus produces some effects (gas charging).
I think starting from
test_abstract_account_post_consensus_failure()is a good way to understand the logic behind the change.Basically, what is needed, is to make it so that this can happen:
This PR also merges input objects in the case of a successful execution.
In order to produce effects, the method
authenticate_then_execute_transaction_to_effects()was added to the execution engine. This new method executes all the steps necessary to create the TransactionEffects after the authentication execution. This means that, after runningauthenticate_transaction_inner()and failing (as well as succeeding), the execution continues withexecute_transaction_to_effects_inner()in order to finish the job, i.e., creating effects for a failing (or successful) transaction.Links to any relevant issues
Fixes #8908
How the change has been tested