From b429a2a588cf5fa5eddaa5cdf5dfc4ffc7ce865e Mon Sep 17 00:00:00 2001 From: Albin Coquereau Date: Mon, 28 Nov 2022 14:56:54 +0100 Subject: [PATCH 1/4] lib_shell: enforce that only prechecked block can be applied Co-authored-by: Zaynah Dargaye Co-authored-by: Diane Gallois-Wong --- src/lib_shell/block_validator_process.ml | 10 +++++++++- src/lib_shell_services/block_validator_errors.ml | 15 +++++++++++++++ src/lib_shell_services/block_validator_errors.mli | 1 + 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/lib_shell/block_validator_process.ml b/src/lib_shell/block_validator_process.ml index 0a5962ead07c..588600cdcef8 100644 --- a/src/lib_shell/block_validator_process.ml +++ b/src/lib_shell/block_validator_process.ml @@ -946,12 +946,20 @@ let apply_block ?(simulate = false) (E {validator_process = (module VP); validator}) chain_store ~predecessor header operations = let open Lwt_result_syntax in + let block_hash = Block_header.hash header in + let*! is_prechecked = + Store.Block.is_known_prechecked chain_store block_hash + in + let* () = + fail_unless + is_prechecked + (Block_validator_errors.Applying_non_prechecked_block block_hash) + in let* metadata = Store.Block.get_block_metadata chain_store predecessor in let max_operations_ttl = Store.Block.max_operations_ttl metadata in let* live_blocks, live_operations = Store.Chain.compute_live_blocks chain_store ~block:predecessor in - let block_hash = Block_header.hash header in let*? () = Block_validation.check_liveness ~live_operations diff --git a/src/lib_shell_services/block_validator_errors.ml b/src/lib_shell_services/block_validator_errors.ml index 72815b0d53a6..9431af68deb4 100644 --- a/src/lib_shell_services/block_validator_errors.ml +++ b/src/lib_shell_services/block_validator_errors.ml @@ -412,6 +412,7 @@ type error += expected : Tezos_crypto.Operation_list_list_hash.t; found : Tezos_crypto.Operation_list_list_hash.t; } + | Applying_non_prechecked_block of Tezos_crypto.Block_hash.t | Failed_to_checkout_context of Tezos_crypto.Context_hash.t | System_error of {errno : string; fn : string; msg : string} | Missing_test_protocol of Tezos_crypto.Protocol_hash.t @@ -487,6 +488,20 @@ let () = | _ -> None) (fun (block, expected, found) -> Inconsistent_operations_hash {block; expected; found}) ; + Error_monad.register_error_kind + `Permanent + ~id:"Block_validator_process.applying_non_prechecked_block" + ~title:"Applying non prechecked block" + ~description:"Applying non prechecked block" + ~pp:(fun ppf (hash : Tezos_crypto.Block_hash.t) -> + Format.fprintf + ppf + "Applying non prechecked block %a" + Tezos_crypto.Block_hash.pp_short + hash) + Data_encoding.(obj1 (req "hash" Tezos_crypto.Block_hash.encoding)) + (function Applying_non_prechecked_block bh -> Some bh | _ -> None) + (fun bh -> Applying_non_prechecked_block bh) ; Error_monad.register_error_kind `Permanent ~id:"Block_validator_process.failed_to_checkout_context" diff --git a/src/lib_shell_services/block_validator_errors.mli b/src/lib_shell_services/block_validator_errors.mli index e7da3cdea94a..c9d0a555012d 100644 --- a/src/lib_shell_services/block_validator_errors.mli +++ b/src/lib_shell_services/block_validator_errors.mli @@ -77,6 +77,7 @@ type error += expected : Tezos_crypto.Operation_list_list_hash.t; found : Tezos_crypto.Operation_list_list_hash.t; } + | Applying_non_prechecked_block of Tezos_crypto.Block_hash.t | Failed_to_checkout_context of Tezos_crypto.Context_hash.t | System_error of {errno : string; fn : string; msg : string} | Missing_test_protocol of Tezos_crypto.Protocol_hash.t -- GitLab From 2bce091eabfcf6aa703fcdb9866c45c4a539b789 Mon Sep 17 00:00:00 2001 From: Albin Coquereau Date: Tue, 29 Nov 2022 21:43:40 +0100 Subject: [PATCH 2/4] lib_shell: always precheck in apply_block Co-authored-by: Zaynah Dargaye Co-authored-by: Diane Gallois-Wong --- src/lib_shell/block_validator.ml | 110 ++++++++++++++----------------- 1 file changed, 49 insertions(+), 61 deletions(-) diff --git a/src/lib_shell/block_validator.ml b/src/lib_shell/block_validator.ml index ac24ec296e06..546c4d38da19 100644 --- a/src/lib_shell/block_validator.ml +++ b/src/lib_shell/block_validator.ml @@ -137,30 +137,27 @@ let check_chain_liveness chain_db hash (header : Block_header.t) = tzfail (invalid_block hash error) | None | Some _ -> return_unit -let precheck_block bvp chain_store ~predecessor block_header block_hash +let precheck_block bvp chain_db chain_store ~predecessor block_header block_hash operations = - Block_validator_process.precheck_block - bvp - chain_store - ~predecessor - block_header - block_hash - operations - -let precheck_block_and_advertise bvp chain_db chain_store ~predecessor hash - header operations = let open Lwt_result_syntax in + let*! () = Events.(emit prechecking_block) block_hash in let* () = - precheck_block bvp chain_store ~predecessor header hash operations + Block_validator_process.precheck_block + bvp + chain_store + ~predecessor + block_header + block_hash + operations in + let*! () = Events.(emit prechecked_block) block_hash in (* Add the block and operations to the cache of the ddb to make them available to our peers *) - let* () = - Distributed_db.inject_prechecked_block chain_db hash header operations - in - (* Headers which have been preapplied can be advertised before being fully applied. *) - Distributed_db.Advertise.prechecked_head chain_db header ; - return_unit + Distributed_db.inject_prechecked_block + chain_db + block_hash + block_header + operations let on_validation_request w { @@ -202,64 +199,55 @@ let on_validation_request w let* pred = Store.Block.read_block chain_store header.shell.predecessor in + let with_retry_to_load_protocol f = + let*! r = f () in + match r with + (* [Unavailable_protocol] is expected to be the + first error in the trace *) + | Error (Unavailable_protocol {protocol; _} :: _) -> + let* _ = + Protocol_validator.fetch_and_compile_protocol + bv.protocol_validator + ?peer + ~timeout:bv.limits.protocol_timeout + protocol + in + f () + | _ -> Lwt.return r + in let*! r = - if precheck_and_notify then - let*! () = Events.(emit prechecking_block) hash in - let* () = - precheck_block_and_advertise - bv.validation_process - chain_db - chain_store - ~predecessor:pred - hash - header - operations - in - let*! v = Events.(emit prechecked_block) hash in - return v - else return_unit + protect ~canceler:(Worker.canceler w) (fun () -> + protect ?canceler (fun () -> + with_retry_to_load_protocol (fun () -> + precheck_block + bv.validation_process + chain_db + chain_store + ~predecessor:pred + header + hash + operations))) in match r with - | Error errs -> - assert precheck_and_notify ; - return (Precheck_failed errs) + | Error errs -> return (Precheck_failed errs) | Ok () -> ( + if precheck_and_notify then + (* Headers which have been preapplied can be advertised + before being fully applied. *) + Distributed_db.Advertise.prechecked_head chain_db header ; let* result = protect ~canceler:(Worker.canceler w) (fun () -> protect ?canceler (fun () -> let*! () = Events.(emit validating_block) hash in - let*! r = - Block_validator_process.apply_block - bv.validation_process - chain_store - ~predecessor:pred - header - operations - in - match r with - | Ok x -> return x - (* [Unavailable_protocol] is expected to be the - first error in the trace *) - | Error (Unavailable_protocol {protocol; _} :: _) - -> - let* _ = - Protocol_validator - .fetch_and_compile_protocol - bv.protocol_validator - ?peer - ~timeout:bv.limits.protocol_timeout - protocol - in - (* Retry validating after fetching the protocol *) + with_retry_to_load_protocol (fun () -> Block_validator_process.apply_block bv.validation_process chain_store ~predecessor:pred header - operations - | Error _ as x -> Lwt.return x)) + operations))) in Shell_metrics.Block_validator .set_operation_per_pass_collector -- GitLab From 636ffde0d307933824cae7284a1de76589bce5b1 Mon Sep 17 00:00:00 2001 From: Albin Coquereau Date: Mon, 28 Nov 2022 18:01:14 +0100 Subject: [PATCH 3/4] lib_validation: add argument to precheck (default to true) block before applying it Co-authored-by: Zaynah Dargaye Co-authored-by: Diane Gallois-Wong --- src/lib_shell/block_validator.ml | 1 + src/lib_shell/block_validator_process.ml | 27 ++++---- src/lib_shell/block_validator_process.mli | 4 ++ src/lib_validation/block_validation.ml | 76 ++++++++++++++-------- src/lib_validation/block_validation.mli | 10 +-- src/lib_validation/external_validation.ml | 8 ++- src/lib_validation/external_validation.mli | 1 + src/lib_validation/external_validator.ml | 2 + 8 files changed, 86 insertions(+), 43 deletions(-) diff --git a/src/lib_shell/block_validator.ml b/src/lib_shell/block_validator.ml index 546c4d38da19..90e6f69a93e2 100644 --- a/src/lib_shell/block_validator.ml +++ b/src/lib_shell/block_validator.ml @@ -243,6 +243,7 @@ let on_validation_request w in with_retry_to_load_protocol (fun () -> Block_validator_process.apply_block + ~should_precheck:false bv.validation_process chain_store ~predecessor:pred diff --git a/src/lib_shell/block_validator_process.ml b/src/lib_shell/block_validator_process.ml index 588600cdcef8..3c54d4552824 100644 --- a/src/lib_shell/block_validator_process.ml +++ b/src/lib_shell/block_validator_process.ml @@ -52,6 +52,7 @@ module type S = sig val apply_block : simulate:bool -> + ?should_precheck:bool -> t -> Store.chain_store -> predecessor:Store.Block.t -> @@ -233,8 +234,8 @@ module Internal_validator_process = struct operation_metadata_size_limit; } - let apply_block ~simulate validator chain_store ~predecessor - ~max_operations_ttl block_header operations = + let apply_block ~simulate ?(should_precheck = true) validator chain_store + ~predecessor ~max_operations_ttl block_header operations = let open Lwt_result_syntax in let* env = make_apply_environment @@ -256,6 +257,7 @@ module Internal_validator_process = struct Block_validation.apply ~simulate ?cached_result:validator.preapply_result + ~should_precheck env block_header operations @@ -760,8 +762,8 @@ module External_validator_process = struct in return validator - let apply_block ~simulate validator chain_store ~predecessor - ~max_operations_ttl block_header operations = + let apply_block ~simulate ?(should_precheck = true) validator chain_store + ~predecessor ~max_operations_ttl block_header operations = let chain_id = Store.Chain.chain_id chain_store in let predecessor_block_header = Store.Block.header predecessor in let predecessor_block_metadata_hash = @@ -780,6 +782,7 @@ module External_validator_process = struct predecessor_ops_metadata_hash; operations; max_operations_ttl; + should_precheck; simulate; } in @@ -942,18 +945,19 @@ let reconfigure_event_logging (E {validator_process = (module VP); validator}) config = VP.reconfigure_event_logging validator config -let apply_block ?(simulate = false) +let apply_block ?(simulate = false) ?(should_precheck = true) (E {validator_process = (module VP); validator}) chain_store ~predecessor header operations = let open Lwt_result_syntax in let block_hash = Block_header.hash header in - let*! is_prechecked = - Store.Block.is_known_prechecked chain_store block_hash - in let* () = - fail_unless - is_prechecked - (Block_validator_errors.Applying_non_prechecked_block block_hash) + when_ (not should_precheck) (fun () -> + let*! is_prechecked = + Store.Block.is_known_prechecked chain_store block_hash + in + fail_unless + is_prechecked + (Block_validator_errors.Applying_non_prechecked_block block_hash)) in let* metadata = Store.Block.get_block_metadata chain_store predecessor in let max_operations_ttl = Store.Block.max_operations_ttl metadata in @@ -969,6 +973,7 @@ let apply_block ?(simulate = false) in VP.apply_block ~simulate + ~should_precheck validator chain_store ~predecessor diff --git a/src/lib_shell/block_validator_process.mli b/src/lib_shell/block_validator_process.mli index 7cc591d8fe4a..20c40a11f641 100644 --- a/src/lib_shell/block_validator_process.mli +++ b/src/lib_shell/block_validator_process.mli @@ -74,11 +74,15 @@ val reconfigure_event_logging : (** [apply_block bvp predecessor header os] checks the liveness of the operations and then call [Block_validation.apply] + [should_precheck] when set, triggers the block prechecking before applying + it, see [Block_validation.apply]. + If [simulate] is true, the context resulting from the application will not be committed to disk. Set to false by default. *) val apply_block : ?simulate:bool -> + ?should_precheck:bool -> t -> Store.chain_store -> predecessor:Store.Block.t -> diff --git a/src/lib_validation/block_validation.ml b/src/lib_validation/block_validation.ml index b458e20815cf..d47b390e6cb9 100644 --- a/src/lib_validation/block_validation.ml +++ b/src/lib_validation/block_validation.ml @@ -1282,7 +1282,30 @@ let recompute_metadata ~chain_id ~predecessor_block_header ~predecessor_context tzfail (System_error {errno = Unix.error_message errno; fn; msg}) | (Ok _ | Error _) as res -> Lwt.return res -let apply ?simulate ?cached_result +let precheck ~chain_id ~predecessor_block_header ~predecessor_block_hash + ~predecessor_context ~cache block_header operations = + let open Lwt_result_syntax in + let block_hash = Block_header.hash block_header in + let*! pred_protocol_hash = Context_ops.get_protocol predecessor_context in + let* (module Proto) = + match Registered_protocol.get pred_protocol_hash with + | None -> + tzfail + (Unavailable_protocol + {block = block_hash; protocol = pred_protocol_hash}) + | Some p -> return p + in + let module Block_validation = Make (Proto) in + Block_validation.precheck + chain_id + ~predecessor_block_header + ~predecessor_block_hash + ~predecessor_context + ~cache + ~block_header + operations + +let apply ?simulate ?cached_result ?(should_precheck = true) { chain_id; user_activated_upgrades; @@ -1304,6 +1327,18 @@ let apply ?simulate ?cached_result {block = block_hash; protocol = pred_protocol_hash}) | Some p -> return p in + let* () = + if should_precheck && Proto.(compare environment_version V7 >= 0) then + precheck + ~chain_id + ~predecessor_block_header + ~predecessor_block_hash:block_header.Block_header.shell.predecessor + ~predecessor_context + ~cache + block_header + operations + else return_unit + in let module Block_validation = Make (Proto) in Block_validation.apply ?simulate @@ -1321,14 +1356,23 @@ let apply ?simulate ?cached_result ~block_header operations -let apply ?simulate ?cached_result c ~cache block_header operations = +let apply ?simulate ?cached_result ?should_precheck apply_environment ~cache + block_header operations = let open Lwt_result_syntax in let block_hash = Block_header.hash block_header in let*! r = (* The cache might be inconsistent with the context. By forcing the reloading of the cache, we restore the consistency. *) let*! r = - apply ?simulate ?cached_result c ~cache block_hash block_header operations + apply + ?simulate + ?cached_result + ?should_precheck + apply_environment + ~cache + block_hash + block_header + operations in match r with | Error (Validation_errors.Inconsistent_hash _ :: _) -> @@ -1336,7 +1380,8 @@ let apply ?simulate ?cached_result c ~cache block_header operations = let*! () = Event.(emit inherited_inconsistent_cache) block_hash in apply ?cached_result - c + ?should_precheck + apply_environment ~cache:`Force_load block_hash block_header @@ -1348,29 +1393,6 @@ let apply ?simulate ?cached_result c ~cache block_header operations = tzfail (System_error {errno = Unix.error_message errno; fn; msg}) | (Ok _ | Error _) as res -> Lwt.return res -let precheck ~chain_id ~predecessor_block_header ~predecessor_block_hash - ~predecessor_context ~cache block_header operations = - let open Lwt_result_syntax in - let block_hash = Block_header.hash block_header in - let*! pred_protocol_hash = Context_ops.get_protocol predecessor_context in - let* (module Proto) = - match Registered_protocol.get pred_protocol_hash with - | None -> - tzfail - (Unavailable_protocol - {block = block_hash; protocol = pred_protocol_hash}) - | Some p -> return p - in - let module Block_validation = Make (Proto) in - Block_validation.precheck - chain_id - ~predecessor_block_header - ~predecessor_block_hash - ~predecessor_context - ~cache - ~block_header - operations - let preapply ~chain_id ~user_activated_upgrades ~user_activated_protocol_overrides ~operation_metadata_size_limit ~timestamp ~protocol_data ~live_blocks ~live_operations ~predecessor_context diff --git a/src/lib_validation/block_validation.mli b/src/lib_validation/block_validation.mli index 03ba74ddb0e0..0cc29f3d98dc 100644 --- a/src/lib_validation/block_validation.mli +++ b/src/lib_validation/block_validation.mli @@ -128,15 +128,17 @@ type apply_environment = { 2. [P.apply] 3. [P.finalize_block] + [should_precheck] when set (default), triggers the block prechecking before + applying it, see [precheck]. If it is set to [false] the given block must + have been prechecked. + If [simulate] is true, the context resulting from the application is not committed to disk using `Context.commit`, only the commit - hash is computed, using `Context.hash`. Set to false by default. - - Hypothesis: we assume that the given block has already been - validated -- E.g. by calling [precheck]. *) + hash is computed, using `Context.hash`. Set to false by default. *) val apply : ?simulate:bool -> ?cached_result:apply_result * Tezos_protocol_environment.Context.t -> + ?should_precheck:bool -> apply_environment -> cache:Tezos_protocol_environment.Context.source_of_cache -> Block_header.t -> diff --git a/src/lib_validation/external_validation.ml b/src/lib_validation/external_validation.ml index c12924d48b15..f5e8c595c06e 100644 --- a/src/lib_validation/external_validation.ml +++ b/src/lib_validation/external_validation.ml @@ -46,6 +46,7 @@ type request = Tezos_crypto.Operation_metadata_list_list_hash.t option; operations : Operation.t list list; max_operations_ttl : int; + should_precheck : bool; simulate : bool; } | Preapply of { @@ -183,7 +184,7 @@ let case_validate tag = case tag ~title:"validate" - (obj8 + (obj9 (req "chain_id" Tezos_crypto.Chain_id.encoding) (req "block_header" (dynamic_size Block_header.encoding)) (req "pred_header" (dynamic_size Block_header.encoding)) @@ -195,6 +196,7 @@ let case_validate tag = Tezos_crypto.Operation_metadata_list_list_hash.encoding) (req "max_operations_ttl" int31) (req "operations" (list (list (dynamic_size Operation.encoding)))) + (req "should_precheck" bool) (req "simulate" bool)) (function | Validate @@ -206,6 +208,7 @@ let case_validate tag = predecessor_ops_metadata_hash; max_operations_ttl; operations; + should_precheck; simulate; } -> Some @@ -216,6 +219,7 @@ let case_validate tag = predecessor_ops_metadata_hash, max_operations_ttl, operations, + should_precheck, simulate ) | _ -> None) (fun ( chain_id, @@ -225,6 +229,7 @@ let case_validate tag = predecessor_ops_metadata_hash, max_operations_ttl, operations, + should_precheck, simulate ) -> Validate { @@ -235,6 +240,7 @@ let case_validate tag = predecessor_ops_metadata_hash; max_operations_ttl; operations; + should_precheck; simulate; }) diff --git a/src/lib_validation/external_validation.mli b/src/lib_validation/external_validation.mli index 70c3f5ebc8ca..d89f6c33c0e8 100644 --- a/src/lib_validation/external_validation.mli +++ b/src/lib_validation/external_validation.mli @@ -46,6 +46,7 @@ type request = Tezos_crypto.Operation_metadata_list_list_hash.t option; operations : Operation.t list list; max_operations_ttl : int; + should_precheck : bool; simulate : bool; } | Preapply of { diff --git a/src/lib_validation/external_validator.ml b/src/lib_validation/external_validator.ml index ef77aff78b1e..21727db52ffb 100644 --- a/src/lib_validation/external_validator.ml +++ b/src/lib_validation/external_validator.ml @@ -260,6 +260,7 @@ let run ~readonly input output = predecessor_ops_metadata_hash; operations; max_operations_ttl; + should_precheck; simulate; } -> let*! () = Events.(emit validation_request block_header) in @@ -302,6 +303,7 @@ let run ~readonly input output = Block_validation.apply ~simulate ?cached_result + ~should_precheck env block_header operations -- GitLab From 86e2a1f2b72389fbd3557efcc4bf6f6c158b854e Mon Sep 17 00:00:00 2001 From: Albin Coquereau Date: Tue, 29 Nov 2022 21:56:44 +0100 Subject: [PATCH 4/4] changes: add entry for bootstrap pipeline fix --- CHANGES.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES.rst b/CHANGES.rst index 7279a17b1fa5..f508e723b241 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -68,6 +68,11 @@ Node of ``octez-node`` and that was already used internally (and that was not usable on its own). +- Fixed a bug that caused the bootstrap pipeline to apply a block without + prechecking it first. This issue only occurs for recent protocols (i.e., Lima + and later) where the validation of a block is dissociated from its + application. (MR :gl:`!7014`) + Client ------ -- GitLab