From 4467d9ed8ef2b60ebb866cd61ab6c5e60054a5e6 Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Fri, 24 Jun 2022 15:01:28 +0200 Subject: [PATCH 1/3] Tezt: add a run_operation test on proposals The test calls run_operation on a proposals operation right after the activation of alpha from genesis --- tezt/lib_tezos/operation_core.ml | 56 ++++++++++++++++++++-- tezt/lib_tezos/operation_core.mli | 80 ++++++++++++++++++++++++++++++- tezt/tests/run_operation_RPC.ml | 56 ++++++++++++++++++++++ tezt/tests/voting.ml | 10 ++++ 4 files changed, 197 insertions(+), 5 deletions(-) diff --git a/tezt/lib_tezos/operation_core.ml b/tezt/lib_tezos/operation_core.ml index 888cf86724a5..04aa872bb8df 100644 --- a/tezt/lib_tezos/operation_core.ml +++ b/tezt/lib_tezos/operation_core.ml @@ -25,7 +25,7 @@ open Runnable.Syntax -type kind = Consensus of {chain_id : string} | Manager +type kind = Consensus of {chain_id : string} | Voting | Manager type t = { branch : string; @@ -70,7 +70,7 @@ let sign ({kind; signer; _} as t) client = | Consensus {chain_id} -> Tezos_crypto.Signature.Endorsement (Tezos_crypto.Chain_id.of_b58check_exn chain_id) - | Manager -> Tezos_crypto.Signature.Generic_operation + | Voting | Manager -> Tezos_crypto.Signature.Generic_operation in let* hex = hex t client in let bytes = Hex.to_bytes hex in @@ -149,7 +149,7 @@ let inject_operations ?(request = `Inject) ?(force = false) ?error t client : let* () = Process.check_error ~msg process in Lwt_list.map_s (fun op -> hash op client) t -let make_run_operation_input ?chain_id t client = +let make_run_operation_input ?chain_id ?(sign_correctly = false) t client = let* chain_id = match chain_id with | Some chain_id -> return chain_id @@ -157,6 +157,11 @@ let make_run_operation_input ?chain_id t client = in (* The [run_operation] RPC does not check the signature. *) let signature = Tezos_crypto.Signature.zero in + (* ...except that it is currently bugged, and it checks the + signature of non-manager operations + (https://gitlab.com/tezos/tezos/-/issues/3401). So we temporarily + provide the option to compute a correct signature. *) + let* signature = if sign_correctly then sign t client else return signature in return (`O [ @@ -211,6 +216,51 @@ module Consensus = struct inject ?request ?force ?error op client end +module Voting = struct + type t = + | Proposals of { + source : Account.key; + period : int; + proposals : string list; (** Candidate protocol hashes *) + } + + let proposals source period proposals = Proposals {source; period; proposals} + + let get_source_and_make_contents = function + | Proposals {source; period; proposals} -> + let proposals_contents = + [ + ("kind", `String "proposals"); + ("source", `String source.Account.public_key_hash); + ("period", Ezjsonm.int period); + ("proposals", Ezjsonm.list Ezjsonm.string proposals); + ] + in + let contents = `A [`O proposals_contents] in + (source, contents) + + let operation ?branch ?client ?signer t = + let* branch = + match branch with + | Some branch -> return branch + | None -> ( + match client with + | Some client -> get_branch ~offset:0 client + | None -> + raise + (Invalid_argument + "At least one of arguments [branch] and [client] must be \ + provided.")) + in + let source, contents = get_source_and_make_contents t in + let signer = Option.value signer ~default:source in + return (make ~branch ~signer ~kind:Voting contents) + + let inject ?request ?force ?signature ?error ?branch ?signer t client = + let* op = operation ?branch ?signer ~client t in + inject ?request ?force ?signature ?error op client +end + module Manager = struct type payload = | Reveal of Account.key diff --git a/tezt/lib_tezos/operation_core.mli b/tezt/lib_tezos/operation_core.mli index fff4c7fafb4f..018fb1fd4053 100644 --- a/tezt/lib_tezos/operation_core.mli +++ b/tezt/lib_tezos/operation_core.mli @@ -65,7 +65,7 @@ type operation := t operation which is necessary for signing an operation. This type aims to be extended when other kinds of operations are added into this module. *) -type kind = Consensus of {chain_id : string} | Manager +type kind = Consensus of {chain_id : string} | Voting | Manager (** [make ~branch ~signer ~kind json client] builds the representation of an unsigned operation. *) @@ -144,13 +144,29 @@ val inject_operations : The operation is signed with {!Tezos_crypto.Signature.zero}, because the [run_operation] RPC skips signature checks anyway. + @param sign_correctly If [true], use the correct signature of the + operation instead of [Signature.zero]. Defaults to [false]. This + parameter is temporary until the [run_operation] RPC is fixed to + actually skip signature checks for non-manager operations (see + https://gitlab.com/tezos/tezos/-/issues/3401) + @param chain_id Allows to manually provide the [chain_id]. If omitted, the [chain_id] is retrieved via RPC using the provided [client]. @param client The {!Client.t} argument is used to retrieve the [chain_id] when it is not provided. *) -val make_run_operation_input : ?chain_id:string -> t -> Client.t -> JSON.u Lwt.t +val make_run_operation_input : + ?chain_id:string -> + (* FIXME: https://gitlab.com/tezos/tezos/-/issues/3401 + + Remove the [sign_correctly] argument once the [run_operation] RPC + is fixed to actually skip signature checks for non-manager + operations. *) + ?sign_correctly:bool -> + t -> + Client.t -> + JSON.u Lwt.t module Consensus : sig (** A representation of a consensus operation. *) @@ -188,6 +204,66 @@ module Consensus : sig [`OpHash of string] Lwt.t end +(** Voting operations (validation pass [1]): [proposals] and [ballot]. + + Only the [proposals] operation is currently supported. Feel free to + add support for [ballot] as needed. *) +module Voting : sig + (** A representation of a voting operation. *) + type t + + (** [proposals source period protocol_hashes] crafts a [proposals] + operation, that is, an operation that submits candidate protocol + hashes for voting. + + @param source The account that submits the proposals. + + @param period An index that identifies the targeted voting + period. + + @params protocol_hashes A list of candidate protocol hashes. *) + val proposals : Account.key -> int -> string list -> t + + (** Contruct a voting operation from its representation. + + @param branch Allows to manually provide the branch. If omitted, + the branch it retrieved via RPC using the given client. + + @param client Used to retrieve the branch when it is not + provided. + + @param signer Allows to manually set the signer of the operation, + e.g. to craft an operation with a wrong signature. If omitted, + the signer is the operation's source. + + @raise Invalid_argument if neither the [branch] argument nor the + [client] one is provided. *) + val operation : + ?branch:string -> + ?client:Client.t -> + ?signer:Account.key -> + t -> + operation Lwt.t + + (** A wrapper for {!inject}ing a voting operation. + + See {!inject} for a description of arguments [request], [force], + [signature], and [error]. + + See [Voting.operation] right above for a description of arguments + [branch] and [signer]. *) + val inject : + ?request:[`Inject | `Notify] -> + ?force:bool -> + ?signature:Tezos_crypto.Signature.t -> + ?error:rex -> + ?branch:string -> + ?signer:Account.key -> + t -> + Client.t -> + [`OpHash of string] Lwt.t +end + module Manager : sig (** Payload of a manager operation. This excludes generic parameters common to all manager operations. See {!type:t}. *) diff --git a/tezt/tests/run_operation_RPC.ml b/tezt/tests/run_operation_RPC.ml index 711787a82f6d..fd8c8c0db394 100644 --- a/tezt/tests/run_operation_RPC.ml +++ b/tezt/tests/run_operation_RPC.ml @@ -86,6 +86,61 @@ let run_manager_operations_and_check_proto_error ~expected_proto_error check_response_contains_proto_error ~expected_proto_error response ; unit +(** This test checks that the [run_operation] RPC succeeds on a + correct [proposals] operation. + + It only supports protocol versions from 015 on. Before that, the + RPC crashed when called both on a non-manager operation and on a + level 1 block. + + This test needs the initial voting period (right after protocol + activation) to be a [proposal] period. If this changes in the + future, this test will need to be reworked. *) +let test_run_proposals = + Protocol.register_test + ~__FILE__ + ~supports:(Protocol.From_protocol 015) + ~title:"Run_operation proposals" + ~tags:(run_operation_tags @ ["voting"; "proposals"]) + @@ fun protocol -> + Log.info "Initialize a node and a client." ; + let* node, client = + Client.init_with_protocol + ~nodes_args:[Synchronisation_threshold 0] + ~protocol + `Client + () + in + Log.info + "Retrieve the current voting period and check that it is of the [proposal] \ + kind." ; + let* period = Voting.get_current_period client in + Log.info "period: %a" Voting.pp_period period ; + (match period.kind with + | Proposal -> () + | Exploration | Cooldown | Promotion | Adoption -> + Test.fail + "This test needs the voting period at level 1 to be of kind \ + [proposal], but a period of kind [%s] was found." + (Voting.period_kind_to_string period.kind)) ; + Log.info "Craft a [proposals] operation:" ; + let proposals_repr = + Operation.Voting.proposals + Constant.bootstrap1 + period.index + [Protocol.demo_counter_hash] + in + let* op = Operation.Voting.operation ~client proposals_repr in + let* op_json = + Operation.make_run_operation_input ~sign_correctly:true op client + in + Log.info "%s" (Ezjsonm.value_to_string ~minify:false op_json) ; + Log.info "Call the [run_operation] RPC on this operation." ; + let* _output = + RPC.(call node (post_chain_block_helpers_scripts_run_operation op_json)) + in + unit + (** This test checks that the [run_operation] RPC used to allow batches of manager operations containing different sources in protocol versions before Kathmandu (014), but rejects them from @@ -538,6 +593,7 @@ let test_misc_manager_ops_from_fresh_account = unit let register ~protocols = + test_run_proposals protocols ; test_batch_inconsistent_sources protocols ; test_inconsistent_counters protocols ; test_bad_revelations protocols ; diff --git a/tezt/tests/voting.ml b/tezt/tests/voting.ml index 1cfdca13401d..a0e52dca1fd3 100644 --- a/tezt/tests/voting.ml +++ b/tezt/tests/voting.ml @@ -95,6 +95,16 @@ type period = { remaining : int; } +let pp_period fmt period = + Format.fprintf + fmt + "{ index: %d; kind: %s; start_position: %d; position: %d; remaining: %d }" + period.index + (period_kind_to_string period.kind) + period.start_position + period.position + period.remaining + let period_type : period Check.typ = Check.convert (fun {index; kind; start_position; position; remaining} -> -- GitLab From 32870fc3534ce4c71209cc4fd041711e92cfe60b Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Fri, 24 Jun 2022 19:08:34 +0200 Subject: [PATCH 2/3] Proto/plugin: fix a minor problem with run_operation RPC in tests by no longer computing the predecessor_level, which was inaccurate anyway and could fail in tests at block level 1. Instead, the RPC now uses the new apply_mode Mempool_no_consensus_op. It also explicitly fails on consensus operations (it used to handle them incorrectly). Also add a couple of TODOs on run_operation linking to issues. --- src/proto_alpha/lib_plugin/RPC.ml | 55 +++++++++++++++++++------- src/proto_alpha/lib_protocol/apply.ml | 36 +++++++++++++---- src/proto_alpha/lib_protocol/apply.mli | 9 +++++ 3 files changed, 78 insertions(+), 22 deletions(-) diff --git a/src/proto_alpha/lib_plugin/RPC.ml b/src/proto_alpha/lib_plugin/RPC.ml index c670c8e211f8..43c635a3a319 100644 --- a/src/proto_alpha/lib_plugin/RPC.ml +++ b/src/proto_alpha/lib_plugin/RPC.ml @@ -369,12 +369,18 @@ module Scripts = struct ~query:RPC_query.empty RPC_path.(path / "normalize_type") + (* FIXME: https://gitlab.com/tezos/tezos/-/issues/3401 + + Contrary to its description, this RPC currently does check the + signature of voting operations. This is in the process of being + fixed. *) let run_operation = RPC_service.post_service ~description: "Run an operation with the context of the given block and without \ signature checks. Return the operation application result, \ - including the consumed gas." + including the consumed gas. This RPC does not support consensus \ + operations." ~query:RPC_query.empty ~input: (obj2 @@ -809,6 +815,24 @@ module Scripts = struct | ILog (_, _, _, _, instr) -> Format.fprintf fmt "log/%a" pp_instr_name instr + type error += Run_operation_does_not_support_consensus_operations + + let () = + let description = + "The run_operation RPC does not support consensus operations." + in + register_error_kind + `Permanent + ~id:"run_operation_does_not_support_consensus_operations" + ~title:"Run operation does not support consensus operations" + ~description + ~pp:(fun ppf () -> Format.fprintf ppf "%s" description) + Data_encoding.empty + (function + | Run_operation_does_not_support_consensus_operations -> Some () + | _ -> None) + (fun () -> Run_operation_does_not_support_consensus_operations) + let run_operation_service ctxt () ({shell; protocol_data = Operation_data protocol_data}, chain_id) = (* this code is a duplicate of Apply without signature check *) @@ -827,6 +851,10 @@ module Scripts = struct Skip_signature_check >>=? fun op_validated_stamp -> 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 @@ -846,23 +874,19 @@ module Scripts = struct op >|=? fun (_ctxt, result) -> ret result | _ -> - let predecessor_level = - match - Alpha_context.Level.pred ctxt (Alpha_context.Level.current ctxt) - with - | Some level -> level - | None -> assert false + 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 - Alpha_context.Round.get ctxt >>=? fun predecessor_round -> Apply.apply_contents_list ctxt chain_id - (Partial_construction - { - predecessor_level; - predecessor_round; - grand_parent_round = Round.zero; - }) + apply_mode ~payload_producer op_validated_stamp operation @@ -1451,6 +1475,9 @@ module Scripts = struct >>?= fun (Ex_ty typ, _ctxt) -> let normalized = Unparse_types.unparse_ty ~loc:() typ in return @@ Micheline.strip_locations normalized) ; + (* TODO: https://gitlab.com/tezos/tezos/-/issues/3364 + + Should [run_operation] be registered at successor level? *) Registration.register0 ~chunked:true S.run_operation run_operation_service ; Registration.register0_successor_level ~chunked:true diff --git a/src/proto_alpha/lib_protocol/apply.ml b/src/proto_alpha/lib_protocol/apply.ml index 3f9accf97124..e491c03c362a 100644 --- a/src/proto_alpha/lib_protocol/apply.ml +++ b/src/proto_alpha/lib_protocol/apply.ml @@ -125,6 +125,7 @@ type error += | Failing_noop_error | Zero_frozen_deposits of Signature.Public_key_hash.t | Invalid_transfer_to_sc_rollup_from_implicit_account + | Mode_forbids_consensus_operations let () = register_error_kind @@ -756,7 +757,19 @@ let () = (function | Invalid_transfer_to_sc_rollup_from_implicit_account -> Some () | _ -> None) - (fun () -> Invalid_transfer_to_sc_rollup_from_implicit_account) + (fun () -> Invalid_transfer_to_sc_rollup_from_implicit_account) ; + let mode_forbids_consens_description = + "The application mode does not support consensus operations." + in + register_error_kind + `Permanent + ~id:"mode_forbids_consensus_operations" + ~title:"Mode forbids consensus operations" + ~description:mode_forbids_consens_description + ~pp:(fun ppf () -> Format.fprintf ppf "%s" mode_forbids_consens_description) + Data_encoding.empty + (function Mode_forbids_consensus_operations -> Some () | _ -> None) + (fun () -> Mode_forbids_consensus_operations) open Apply_results open Apply_operation_result @@ -2392,12 +2405,14 @@ type apply_mode = predecessor_round : Round.t; grand_parent_round : Round.t; } + | Mempool_no_consensus_op let get_predecessor_level = function | Application {predecessor_level; _} | Full_construction {predecessor_level; _} | Partial_construction {predecessor_level; _} -> - predecessor_level + ok predecessor_level + | Mempool_no_consensus_op -> error Mode_forbids_consensus_operations let record_operation (type kind) ctxt hash (operation : kind operation) : context = @@ -2440,7 +2455,7 @@ let compute_expected_consensus_content (type consensus_op_kind) fail (Consensus_operation_for_future_level {expected = proposal_level.level; provided = operation_level}) - ) + | Mempool_no_consensus_op -> fail Mode_forbids_consensus_operations) | Some (branch, payload_hash) -> ( match application_mode with | Application {predecessor_round; _} @@ -2453,7 +2468,8 @@ let compute_expected_consensus_content (type consensus_op_kind) branch; level = proposal_level; round = predecessor_round; - } ))) + } ) + | Mempool_no_consensus_op -> fail Mode_forbids_consensus_operations)) | Preendorsement -> ( match application_mode with | Application {locked_round = None; _} -> @@ -2496,7 +2512,8 @@ let compute_expected_consensus_content (type consensus_op_kind) operation_round ) | Some round -> (ctxt, round) in - return (ctxt', {payload_hash; branch; level = current_level; round})) + return (ctxt', {payload_hash; branch; level = current_level; round}) + | Mempool_no_consensus_op -> fail Mode_forbids_consensus_operations) let check_level (apply_mode : apply_mode) ~expected ~provided = match apply_mode with @@ -2514,6 +2531,7 @@ let check_level (apply_mode : apply_mode) ~expected ~provided = error_when Raw_level.(expected < provided) (Consensus_operation_for_future_level {expected; provided}) + | Mempool_no_consensus_op -> error Mode_forbids_consensus_operations let check_payload_hash (apply_mode : apply_mode) ~expected ~provided = match apply_mode with @@ -2525,6 +2543,7 @@ let check_payload_hash (apply_mode : apply_mode) ~expected ~provided = error_unless (Block_payload_hash.equal expected provided) (Consensus_operation_on_competing_proposal {expected; provided}) + | Mempool_no_consensus_op -> error Mode_forbids_consensus_operations let check_operation_branch ~expected ~provided = error_unless @@ -2554,6 +2573,7 @@ let check_round (type kind) (operation_kind : kind consensus_operation_type) error_unless (Round.equal expected provided) (Wrong_round_for_consensus_operation {expected; provided}) + | Mempool_no_consensus_op -> error Mode_forbids_consensus_operations let check_consensus_content (type kind) (apply_mode : apply_mode) (content : consensus_content) (operation_branch : Block_hash.t) @@ -2590,7 +2610,7 @@ let validate_consensus_contents (type kind) ctxt chain_id (content : consensus_content) : (context * public_key_hash * int) tzresult Lwt.t = let current_level = Level.current ctxt in - let proposal_level = get_predecessor_level apply_mode in + get_predecessor_level apply_mode >>?= fun proposal_level -> let slot_map = match operation_kind with | Preendorsement -> Consensus.allowed_preendorsements ctxt @@ -2831,7 +2851,7 @@ let apply_contents_list (type kind) ctxt chain_id (apply_mode : apply_mode) (context * kind contents_result_list) tzresult Lwt.t = let mempool_mode = match apply_mode with - | Partial_construction _ -> true + | Partial_construction _ | Mempool_no_consensus_op -> true | Full_construction _ | Application _ -> false in match contents_list with @@ -2860,7 +2880,7 @@ let apply_contents_list (type kind) ctxt chain_id (apply_mode : apply_mode) preendorsement_power = voting_power; }) ) | Single (Endorsement consensus_content) -> ( - let proposal_level = get_predecessor_level apply_mode in + get_predecessor_level apply_mode >>?= fun proposal_level -> match apply_mode with | Partial_construction {grand_parent_round; _} when is_parent_endorsement diff --git a/src/proto_alpha/lib_protocol/apply.mli b/src/proto_alpha/lib_protocol/apply.mli index c57651730ade..32002c33140f 100644 --- a/src/proto_alpha/lib_protocol/apply.mli +++ b/src/proto_alpha/lib_protocol/apply.mli @@ -111,6 +111,15 @@ type apply_mode = predecessor_round : Round.t; grand_parent_round : Round.t; } + | Mempool_no_consensus_op + (** Similar to [Partial_construction], but does not have access to + information such as the [predecessor_level]. Makes the operations + which need this information (preendorsements and endorsements as + of July 2022) return the [Mode_forbids_consensus_operations] + error. + + This mode is used by the [run_operation] RPC in + [lib_plugin/RPC.ml]. *) (** Apply an operation, i.e. update the given context in accordance with the operation's semantic (or return an error if the operation -- GitLab From abc4a9c4f7d4b113e8f2b25dbe07c57d280b1cc3 Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Tue, 5 Jul 2022 16:58:02 +0200 Subject: [PATCH 3/3] docs/protocols/alpha: log the update of run_operation description --- docs/protocols/alpha.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/protocols/alpha.rst b/docs/protocols/alpha.rst index 5fc6c3f66e72..90ce04f6d4f0 100644 --- a/docs/protocols/alpha.rst +++ b/docs/protocols/alpha.rst @@ -38,6 +38,12 @@ Breaking Changes RPC Changes ----------- +- The ``run_operation`` RPC description has been updated to indicate + that the RPC does not support consensus operations. It could already + give inconsistent results on such operations, which was not + documented. It now returns on error when called on a consensus + operation. (MR :gl:`!5707`) + Operation receipts ------------------ -- GitLab