From 736c8db326a0d25cd9ced0fa38705f3e95f9f65b Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Wed, 22 Jun 2022 14:10:27 +0200 Subject: [PATCH 1/3] Proto: call directly Apply.apply_operation in run_operation RPC And stop exporting apply_contents_list and apply_manager_operations in apply.mli: they were only called by the run_operation RPC. --- src/proto_alpha/lib_plugin/RPC.ml | 69 ++++++++-------------- src/proto_alpha/lib_protocol/apply.mli | 79 +++++++++----------------- 2 files changed, 50 insertions(+), 98 deletions(-) diff --git a/src/proto_alpha/lib_plugin/RPC.ml b/src/proto_alpha/lib_plugin/RPC.ml index 43c635a3a319..621a3d1ad09b 100644 --- a/src/proto_alpha/lib_plugin/RPC.ml +++ b/src/proto_alpha/lib_plugin/RPC.ml @@ -836,13 +836,8 @@ module Scripts = struct let run_operation_service ctxt () ({shell; protocol_data = Operation_data protocol_data}, chain_id) = (* this code is a duplicate of Apply without signature check *) - let ret contents = - (Operation_data protocol_data, Apply_results.Operation_metadata {contents}) - in let operation : _ operation = {shell; protocol_data} in - let hash = Operation.hash {shell; protocol_data} in - let ctxt = Origination_nonce.init ctxt hash in - let payload_producer = Signature.Public_key_hash.zero in + let hash = Operation.hash operation in Validate_operation.TMP_for_plugin .precheck_manager__do_nothing_on_non_manager_op ctxt @@ -850,48 +845,32 @@ module Scripts = struct protocol_data.contents Skip_signature_check >>=? fun op_validated_stamp -> - match protocol_data.contents with + (match protocol_data.contents with | Single (Preendorsement _) | Single (Endorsement _) | Single (Dal_slot_availability _) -> - fail Run_operation_does_not_support_consensus_operations - | Single (Manager_operation _) as op -> - Apply.apply_manager_operations - ctxt - ~payload_producer - chain_id - ~mempool_mode:true - op_validated_stamp - op - >|=? fun (_ctxt, result) -> ret result - | Cons (Manager_operation _, _) as op -> - Apply.apply_manager_operations - ctxt - ~payload_producer - chain_id - ~mempool_mode:true - op_validated_stamp - op - >|=? fun (_ctxt, result) -> ret result - | _ -> - let apply_mode = - (* To simulate the injection of an operation in the mempool, - we want a mode that behaves similarly to - {!Apply.Partial_construction}. However, we don't have - access to information such as the [predecessor_round]. So - we use a mode that doesn't need this information, and - consequently doesn't support consensus operations. *) - Apply.Mempool_no_consensus_op - in - Apply.apply_contents_list - ctxt - chain_id - apply_mode - ~payload_producer - op_validated_stamp - operation - operation.protocol_data.contents - >|=? fun (_ctxt, result) -> ret result + error Run_operation_does_not_support_consensus_operations + | _ -> ok ()) + >>?= fun () -> + let apply_mode = + (* To simulate the injection of an operation in the mempool, + we want a mode that behaves similarly to + {!Apply.Partial_construction}. However, we don't have + access to information such as the [predecessor_round]. So + we use a mode that doesn't need this information, and + consequently doesn't support consensus operations. *) + Apply.Mempool_no_consensus_op + in + Apply.apply_operation + ctxt + chain_id + apply_mode + ~payload_producer:Signature.Public_key_hash.zero + op_validated_stamp + hash + operation + >|=? fun (_ctxt, op_metadata) -> + (Operation_data protocol_data, Apply_results.Operation_metadata op_metadata) (* diff --git a/src/proto_alpha/lib_protocol/apply.mli b/src/proto_alpha/lib_protocol/apply.mli index 32002c33140f..dca373aa8f8d 100644 --- a/src/proto_alpha/lib_protocol/apply.mli +++ b/src/proto_alpha/lib_protocol/apply.mli @@ -138,8 +138,32 @@ type apply_mode = been extended to every kind of operation, [apply_operation] should never return an error. - See {!apply_manager_operations} for additional information on the - application of manager operations. *) + For manager operations, the application has two stages. The first + stage consists in updating the context to: + + - take the fees; + + - increment the account's counter; + + - decrease of the available block gas by operation's [gas_limit]. + + These updates are mandatory. In particular, taking the fees is + critically important. The {!Validate_operation} module (from which + we get the {!Validate_opoeration.stamp} as explained above) is + responsible for ensuring that the operation is solvable, i.e. that + fees can be taken, i.e. that the first stage of manager operation + application cannot fail. If this stage fails nevertheless, the + function returns an error. + + The second stage of this function (still in the case of a manager + operation) consists in applying all the other effects, in + accordance with the semantic of the operation's kind. + + An error may happen during this second phase: in that case, the + function returns the context obtained at the end of the first + stage, and metadata that contain the error. This means that the + operation has no other effects than those described above during + the first phase. *) val apply_operation : context -> Chain_id.t -> @@ -170,57 +194,6 @@ val finalize_application : migration_balance_updates:Receipt.balance_updates -> (context * Fitness.t * block_metadata, error trace) result Lwt.t -(** Similar to {!apply_operation}, but a few initial and final steps - are skipped. This function is called in [lib_plugin/RPC.ml]. *) -val apply_contents_list : - context -> - Chain_id.t -> - apply_mode -> - payload_producer:public_key_hash -> - Validate_operation.stamp -> - 'kind operation -> - 'kind contents_list -> - (context * 'kind contents_result_list) tzresult Lwt.t - -(** Update the context to reflect the application of a manager - operation. - - This function first updates the context to: - - - take the fees; - - - increment the account's counter; - - - decrease of the available block gas by operation's [gas_limit]. - - These updates are mandatory. In particular, taking the fees is - critically important. That's why [apply_manager_operations] takes a - [Validate_operation.stamp] argument, so that it may only be called - after having validated the operation by calling - {!Validate_operation}. Indeed, this module is responsible for - ensuring that the operation is solvable, i.e. that fees can be - taken, i.e. that the first stage of [apply_manager_operations] - cannot fail. If this stage fails nevertheless, the function returns - an error. - - The second stage of this function consists in applying all the - other effects, in accordance with the semantic of the operation's - kind. - - An error may happen during this second phase: in that case, the - function returns the context obtained at the end of the first - stage, and a [contents_result_list] that contains the error. This - means that the operation has no other effects than those described - above during the first phase. *) -val apply_manager_operations : - context -> - payload_producer:public_key_hash -> - Chain_id.t -> - mempool_mode:bool -> - Validate_operation.stamp -> - 'a Kind.manager contents_list -> - (context * 'a Kind.manager contents_result_list) tzresult Lwt.t - (** [value_of_key ctxt k] builds a value identified by key [k] so that it can be put into the cache. *) val value_of_key : -- GitLab From ab9183450a6fdc2131486f725ea05a60647eec6d Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Wed, 22 Jun 2022 18:01:38 +0200 Subject: [PATCH 2/3] Proto: call directly validate_operation in run_operation RPC To make it possible, we add an argument [should_check_signature] to [validate_operation]. Also remove Validate_operation.TMP_for_plugin .precheck_manager__do_nothing_on_non_manager_op which is no longer needed. --- src/proto_alpha/lib_plugin/RPC.ml | 33 ++++++--- .../lib_protocol/validate_operation.ml | 69 +++++++++---------- .../lib_protocol/validate_operation.mli | 38 +++++----- 3 files changed, 71 insertions(+), 69 deletions(-) diff --git a/src/proto_alpha/lib_plugin/RPC.ml b/src/proto_alpha/lib_plugin/RPC.ml index 621a3d1ad09b..e74efd09c5c2 100644 --- a/src/proto_alpha/lib_plugin/RPC.ml +++ b/src/proto_alpha/lib_plugin/RPC.ml @@ -833,18 +833,14 @@ module Scripts = struct | _ -> None) (fun () -> Run_operation_does_not_support_consensus_operations) + (** Validate and apply the operation but skip signature checks; do + not support consensus operations. + + Return the unchanged operation protocol data, and the operation + receipt ie. metadata containing balance updates, consumed gas, + application success or failure, etc. *) let run_operation_service ctxt () ({shell; protocol_data = Operation_data protocol_data}, chain_id) = - (* this code is a duplicate of Apply without signature check *) - let operation : _ operation = {shell; protocol_data} in - let hash = Operation.hash operation in - Validate_operation.TMP_for_plugin - .precheck_manager__do_nothing_on_non_manager_op - ctxt - chain_id - protocol_data.contents - Skip_signature_check - >>=? fun op_validated_stamp -> (match protocol_data.contents with | Single (Preendorsement _) | Single (Endorsement _) @@ -852,6 +848,21 @@ module Scripts = struct error Run_operation_does_not_support_consensus_operations | _ -> ok ()) >>?= fun () -> + let operation : _ operation = {shell; protocol_data} in + let oph = Operation.hash operation in + let validate_operation_info, validate_operation_state = + Validate_operation.init_info_and_state + ctxt + Validate_operation.Mempool + chain_id + in + Validate_operation.validate_operation + validate_operation_info + validate_operation_state + ~should_check_signature:false + oph + operation + >>=? fun (_validate_operation_state, op_validated_stamp) -> let apply_mode = (* To simulate the injection of an operation in the mempool, we want a mode that behaves similarly to @@ -867,7 +878,7 @@ module Scripts = struct apply_mode ~payload_producer:Signature.Public_key_hash.zero op_validated_stamp - hash + oph operation >|=? fun (_ctxt, op_metadata) -> (Operation_data protocol_data, Apply_results.Operation_metadata op_metadata) diff --git a/src/proto_alpha/lib_protocol/validate_operation.ml b/src/proto_alpha/lib_protocol/validate_operation.ml index da2638afc1ac..1f118bea3a67 100644 --- a/src/proto_alpha/lib_protocol/validate_operation.ml +++ b/src/proto_alpha/lib_protocol/validate_operation.ml @@ -347,6 +347,16 @@ module Manager = struct let* balance = Contract.check_allocated_and_get_balance vi.ctxt source in let* () = Contract.check_counter_increment vi.ctxt source first_counter in let* pk = + (* Note that it is important to always retrieve the public + key. This includes the case where the key ends up not being + used because the signature check is skipped in + {!validate_manager_operation} called with + [~should_check_signature:false]. Indeed, the mempool may use + this argument when it has already checked the signature of + the operation in the past; but if there has been a branch + reorganization since then, the key might not be revealed in + the new branch anymore, in which case + {!Contract.get_manager_key} will return an error. *) match revealed_key with | Some pk -> return pk | None -> Contract.get_manager_key vi.ctxt source @@ -632,8 +642,8 @@ module Manager = struct | Block -> batch_state.remaining_block_gas | Mempool -> vs.manager_state.remaining_block_gas - let validate_manager_operation vi vs source oph (type kind) - (operation : kind Kind.manager operation) = + let validate_manager_operation vi vs ~should_check_signature source oph + (type kind) (operation : kind Kind.manager operation) = let open Lwt_result_syntax in let*? () = (* One-operation-per-manager-per-block restriction (1M). @@ -656,7 +666,11 @@ module Manager = struct check_sanity_and_find_public_key vi vs contents_list in let* batch_state = validate_contents_list vi batch_state contents_list in - let*? () = Operation.check_signature source_pk vi.chain_id operation in + let*? () = + if should_check_signature then + Operation.check_signature source_pk vi.chain_id operation + else ok () + in let managers_seen = Signature.Public_key_hash.Map.add source @@ -670,15 +684,27 @@ module Manager = struct end let validate_operation (vi : validate_operation_info) - (vs : validate_operation_state) oph (type kind) (operation : kind operation) - = + (vs : validate_operation_state) ?(should_check_signature = true) oph + (type kind) (operation : kind operation) = let open Lwt_result_syntax in let* vs = match operation.protocol_data.contents with | Single (Manager_operation {source; _}) -> - Manager.validate_manager_operation vi vs source oph operation + Manager.validate_manager_operation + vi + vs + ~should_check_signature + source + oph + operation | Cons (Manager_operation {source; _}, _) -> - Manager.validate_manager_operation vi vs source oph operation + Manager.validate_manager_operation + vi + vs + ~should_check_signature + source + oph + operation | Single (Preendorsement _) | Single (Endorsement _) | Single (Dal_slot_availability _) @@ -724,33 +750,4 @@ module TMP_for_plugin = struct | Skip_signature_check -> ok () in return Operation_validated_stamp - - let precheck_manager__do_nothing_on_non_manager_op ctxt chain_id (type kind) - (contents_list : kind contents_list) should_check_signature = - let handle_manager (type a) (contents_list : a Kind.manager contents_list) = - let vi, vs = init_info_and_state ctxt Mempool chain_id in - precheck_manager vi vs contents_list should_check_signature - in - match contents_list with - | Single (Manager_operation _) -> handle_manager contents_list - | Cons (Manager_operation _, _) -> handle_manager contents_list - | Single (Preendorsement _) - | Single (Endorsement _) - | Single (Dal_slot_availability _) - | Single (Seed_nonce_revelation _) - | Single (Vdf_revelation _) - | Single (Proposals _) - | Single (Ballot _) - | Single (Activate_account _) - | Single (Double_preendorsement_evidence _) - | Single (Double_endorsement_evidence _) - | Single (Double_baking_evidence _) - | Single (Failing_noop _) -> - (* TODO: https://gitlab.com/tezos/tezos/-/issues/2603 - - This should be updated when {!validate_operation} is - implemented on non-manager operations. (Alternatively, this - function might be removed first: - https://gitlab.com/tezos/tezos/-/issues/3245) *) - return Operation_validated_stamp end diff --git a/src/proto_alpha/lib_protocol/validate_operation.mli b/src/proto_alpha/lib_protocol/validate_operation.mli index d0d60956d6df..7bd3b065ef8c 100644 --- a/src/proto_alpha/lib_protocol/validate_operation.mli +++ b/src/proto_alpha/lib_protocol/validate_operation.mli @@ -129,6 +129,21 @@ end includes it being correctly signed: indeed, we can't take anything from a manager without having checked their signature. + @param should_check_signature indicates whether the signature + check should happen. It defaults to [true] because the signature + needs to be correct for the operation to be valid. This argument + exists for special cases where it is acceptable to bypass this + check, e.g.: + + - The mempool may keep track of operations whose signatures have + already been checked: if such an operation needs to be validated + again (typically when the head block changes), then the mempool may + call [validate_operation] with [should_check_signature:false]. + + - The [run_operation] RPC provided by the plugin explicitly + excludes signature checks: see its documentation in + [lib_plugin/RPC.Scripts.S.run_operation]. + TODO: https://gitlab.com/tezos/tezos/-/issues/2603 This function currently does nothing for non-manager operations @@ -139,6 +154,7 @@ end val validate_operation : validate_operation_info -> validate_operation_state -> + ?should_check_signature:bool -> Operation_hash.t -> 'kind Alpha_context.operation -> (validate_operation_state * stamp) tzresult Lwt.t @@ -186,26 +202,4 @@ module TMP_for_plugin : sig 'a Alpha_context.Kind.manager Alpha_context.contents_list -> 'a Alpha_context.Kind.manager should_check_signature -> stamp tzresult Lwt.t - - (** Same as {!precheck_manager}, except that: - - - This function does not require [validate_operation_info] and - [validate_operation_state] arguments. Instead, they are - constructed internally from the given context and chain_id. - - - This function accepts any kind of operation as its - [contents_list] argument rather than just manager - operations. However, on non-manager operations, this function - does not check anything. - - This function is called in [lib_plugin/RPC.ml], where we do not - have access to a {!Main.validation_state} containing - [validate_operation_info] and [_state], and where we need a - {!stamp} even for non-manager operations. *) - val precheck_manager__do_nothing_on_non_manager_op : - Alpha_context.t -> - Chain_id.t -> - 'kind Alpha_context.contents_list -> - 'kind should_check_signature -> - stamp tzresult Lwt.t end -- GitLab From 55a3bac2a609594bdd7ba12fed80bfedaf97b849 Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Fri, 15 Jul 2022 16:51:41 +0200 Subject: [PATCH 3/3] docs/protocols/alpha: log the refactoring of run_operation --- docs/protocols/alpha.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/protocols/alpha.rst b/docs/protocols/alpha.rst index 90ce04f6d4f0..7852c0bbbea1 100644 --- a/docs/protocols/alpha.rst +++ b/docs/protocols/alpha.rst @@ -66,3 +66,8 @@ Internal - Internal refactorings in Michelson typechecker and interpreter. (MRs :gl:`!5586`, :gl:`!5587`) + +- Refactor the ``run_operation`` RPC. This allowed us to remove a + function from ``Validate_operation.TMP_for_plugin`` and to no longer + expose ``apply_contents_list`` and ``apply_manager_operations`` in + ``apply.mli``. (MR :gl:`!5770`) -- GitLab