From 60b7c81b19a5165e800445f5df433a625e6ecaf4 Mon Sep 17 00:00:00 2001 From: "iguerNL@Functori" Date: Thu, 9 Jan 2025 16:18:27 +0100 Subject: [PATCH 1/3] ADAL/Proto: fixes ADAL proofs regarding the handling of publishers whitelist --- src/proto_alpha/lib_protocol/dal_slot_repr.ml | 108 +++++++++--------- .../lib_protocol/dal_slot_repr.mli | 7 +- .../lib_protocol/sc_rollup_proof_repr.ml | 30 ++--- .../lib_protocol/test/helpers/dal_helpers.ml | 2 +- 4 files changed, 76 insertions(+), 71 deletions(-) diff --git a/src/proto_alpha/lib_protocol/dal_slot_repr.ml b/src/proto_alpha/lib_protocol/dal_slot_repr.ml index 3c1a784daced..7472cb34da19 100644 --- a/src/proto_alpha/lib_protocol/dal_slot_repr.ml +++ b/src/proto_alpha/lib_protocol/dal_slot_repr.ml @@ -1001,10 +1001,10 @@ module History = struct (** In case of Adjustable DAL, [attestation_threshold_percent] is set to some attestation threshold defined by the rollup kernel. For regular DAL, it's set to None. *) - commitment_publisher : Contract_repr.t; - (** In case of confirmation proof, we also remember the - commitment publisher. It will be needed in case of Adjustable DAL - to check that the publisher is whitelised, if needed. *) + restricted_commitments_publishers : Contract_repr.t list option; + (** In case of Adjustable DAL, [restricted_commitments_publishers] + is set to the rollup's list of authorized addresses, if + any. For regular DAL, it's set to None. *) } (** The case where the slot's page is confirmed/attested on L1. *) | Page_unconfirmed of { target_cell : history; @@ -1013,11 +1013,10 @@ module History = struct (** In case of Adjustable DAL, [attestation_threshold_percent] is set to some attestation threshold defined by the rollup kernel. For regular DAL, it's set to None. *) - commitment_publisher_opt : Contract_repr.t option; - (** In case of an unconfirmation proof, we remember the commitment - publisher if the slot is published. It will be needed in case of - Adjustable DAL to check that the publisher is whitelisted, if - needed. *) + restricted_commitments_publishers : Contract_repr.t list option; + (** In case of Adjustable DAL, [restricted_commitments_publishers] + is set to the rollup's list of authorized addresses, if + any. For regular DAL, it's set to None. *) } (** The case where the slot's page doesn't exist or is not confirmed on L1. The fields are similar to {!Page_confirmed} case except @@ -1042,7 +1041,9 @@ module History = struct (req "page_data" (bytes Hex)) (req "page_proof" Page.proof_encoding) (opt "attestation_threshold_percent" uint8) - (req "commitment_publisher" Contract_repr.encoding)) + (opt + "restricted_commitments_publishers" + (list Contract_repr.encoding))) (function | Page_confirmed { @@ -1051,7 +1052,7 @@ module History = struct page_data; page_proof; attestation_threshold_percent; - commitment_publisher; + restricted_commitments_publishers; } -> Some ( (), @@ -1060,7 +1061,7 @@ module History = struct page_data, page_proof, attestation_threshold_percent, - commitment_publisher ) + restricted_commitments_publishers ) | _ -> None) (fun ( (), target_cell, @@ -1068,7 +1069,7 @@ module History = struct page_data, page_proof, attestation_threshold_percent, - commitment_publisher ) -> + restricted_commitments_publishers ) -> Page_confirmed { target_cell; @@ -1076,7 +1077,7 @@ module History = struct page_data; page_proof; attestation_threshold_percent; - commitment_publisher; + restricted_commitments_publishers; }) and case_page_unconfirmed = case @@ -1087,33 +1088,35 @@ module History = struct (req "target_cell" history_encoding) (req "inc_proof" (list history_encoding)) (opt "attestation_threshold_percent" uint8) - (opt "commitment_publisher" Contract_repr.encoding)) + (opt + "restricted_commitments_publishers" + (list Contract_repr.encoding))) (function | Page_unconfirmed { target_cell; inc_proof; attestation_threshold_percent; - commitment_publisher_opt; + restricted_commitments_publishers; } -> Some ( (), target_cell, inc_proof, attestation_threshold_percent, - commitment_publisher_opt ) + restricted_commitments_publishers ) | _ -> None) (fun ( (), target_cell, inc_proof, attestation_threshold_percent, - commitment_publisher_opt ) -> + restricted_commitments_publishers ) -> Page_unconfirmed { target_cell; inc_proof; attestation_threshold_percent; - commitment_publisher_opt; + restricted_commitments_publishers; }) in @@ -1166,7 +1169,19 @@ module History = struct | None -> tzfail Dal_invalid_proof_deserialization | Some deserialized_proof -> return deserialized_proof - let pp_inclusion_proof = Format.pp_print_list pp_history + let pp_inclusion_proof = + Format.pp_print_list + ~pp_sep:(fun fmt () -> Format.fprintf fmt " ") + pp_history + + let pp_whitelist fmt = function + | None -> Format.fprintf fmt "" + | Some l -> + (Format.pp_print_list + ~pp_sep:(fun fmt () -> Format.fprintf fmt " ") + Contract_repr.pp_short) + fmt + l let pp_proof ~serialized fmt p = if serialized then Format.pp_print_string fmt (Bytes.to_string p) @@ -1182,7 +1197,7 @@ module History = struct page_data; page_proof; attestation_threshold_percent; - commitment_publisher; + restricted_commitments_publishers; } -> Format.fprintf fmt @@ -1199,14 +1214,14 @@ module History = struct page_proof (Format.pp_print_option Format.pp_print_int) attestation_threshold_percent - Contract_repr.pp_short - commitment_publisher + pp_whitelist + restricted_commitments_publishers | Page_unconfirmed { target_cell; inc_proof; attestation_threshold_percent; - commitment_publisher_opt; + restricted_commitments_publishers; } -> Format.fprintf fmt @@ -1220,8 +1235,8 @@ module History = struct inc_proof (Format.pp_print_option Format.pp_print_int) attestation_threshold_percent - (Format.pp_print_option Contract_repr.pp_short) - commitment_publisher_opt) + pp_whitelist + restricted_commitments_publishers) type error += | Dal_page_proof_error of string @@ -1383,7 +1398,7 @@ module History = struct in match (page_info, is_commitment_attested) with | ( Some (page_data, page_proof), - Either.Right (commitment, commitment_publisher) ) -> + Either.Right (commitment, _commitment_publisher) ) -> (* The case where the slot to which the page is supposed to belong is found and the page's information are given. *) let*? () = @@ -1404,10 +1419,10 @@ module History = struct page_data; page_proof; attestation_threshold_percent; - commitment_publisher; + restricted_commitments_publishers; }, Some page_data ) - | None, Either.Left commitment_publisher_opt -> + | None, Either.Left _commitment_publisher_opt -> (* The slot corresponding to the given page's index is not found in the attested slots of the page's level, and no information is given for that page. So, we produce a proof that the page is not @@ -1418,7 +1433,7 @@ module History = struct target_cell; inc_proof; attestation_threshold_percent; - commitment_publisher_opt; + restricted_commitments_publishers; }, None ) | None, Either.Right _ -> @@ -1479,7 +1494,7 @@ module History = struct type verify_proof_result = { page_content_opt : Page.content option; attestation_threshold_percent : int option; - commitment_publisher_opt : Contract_repr.t option; + restricted_commitments_publishers : Contract_repr.t list option; } let verify_proof_repr ?with_migration dal_params page_id snapshot proof = @@ -1493,7 +1508,7 @@ module History = struct let ( target_cell, inc_proof, attestation_threshold_percent, - commitment_publisher_opt ) = + restricted_commitments_publishers ) = match proof with | Page_confirmed { @@ -1502,23 +1517,23 @@ module History = struct page_data = _; page_proof = _; attestation_threshold_percent; - commitment_publisher; + restricted_commitments_publishers; } -> ( target_cell, inc_proof, attestation_threshold_percent, - Some commitment_publisher ) + restricted_commitments_publishers ) | Page_unconfirmed { target_cell; inc_proof; attestation_threshold_percent; - commitment_publisher_opt; + restricted_commitments_publishers; } -> ( target_cell, inc_proof, attestation_threshold_percent, - commitment_publisher_opt ) + restricted_commitments_publishers ) in let cell_content = Skip_list.content target_cell in (* We check that the target cell has the same level and index than the @@ -1546,8 +1561,7 @@ module History = struct let is_commitment_attested = is_commitment_attested ~attestation_threshold_percent - ~restricted_commitments_publishers: - (Option.map (fun e -> [e]) commitment_publisher_opt) + ~restricted_commitments_publishers cell_content in match (proof, is_commitment_attested) with @@ -1556,28 +1570,20 @@ module History = struct @@ dal_proof_error "verify_proof_repr: the confirmation proof doesn't contain the \ attested slot." - | Page_unconfirmed _, Either.Left validated_commitment_publisher_opt -> + | Page_unconfirmed _, Either.Left _validated_commitment_publisher_opt -> return { page_content_opt = None; attestation_threshold_percent; - commitment_publisher_opt = validated_commitment_publisher_opt; + restricted_commitments_publishers; } | Page_confirmed _, Either.Left _ -> error @@ dal_proof_error "verify_proof_repr: the unconfirmation proof contains the \ target slot." - | ( Page_confirmed - { - target_cell = _; - inc_proof = _; - page_data; - page_proof; - attestation_threshold_percent = _; - commitment_publisher = _; - }, - Either.Right (commitment, validated_commitment_publisher) ) -> + | ( Page_confirmed {page_data; page_proof; _}, + Either.Right (commitment, _validated_commitment_publisher) ) -> (* We check that the page indeed belongs to the target slot at the given page index. *) let* () = @@ -1587,7 +1593,7 @@ module History = struct { page_content_opt = Some page_data; attestation_threshold_percent; - commitment_publisher_opt = Some validated_commitment_publisher; + restricted_commitments_publishers; } let verify_proof ?with_migration dal_params page_id snapshot diff --git a/src/proto_alpha/lib_protocol/dal_slot_repr.mli b/src/proto_alpha/lib_protocol/dal_slot_repr.mli index 97ec2120176b..bbe30e4751b6 100644 --- a/src/proto_alpha/lib_protocol/dal_slot_repr.mli +++ b/src/proto_alpha/lib_protocol/dal_slot_repr.mli @@ -494,13 +494,12 @@ module History : sig - Some attestation threshold percent in case the rollup uses Adjustable DAL, or [None] otherwise; - - Some contract address in case the slot related to the target page is - published, or [None] otherwise. - *) + - Some whitelisted commitments publisher list, if set by the rollup, in + case it uses Adjustable DAL. *) type verify_proof_result = { page_content_opt : Page.content option; attestation_threshold_percent : int option; - commitment_publisher_opt : Contract_repr.t option; + restricted_commitments_publishers : Contract_repr.t list option; } (** [verify_proof ?with_migration dal_params page_id snapshot proof] verifies diff --git a/src/proto_alpha/lib_protocol/sc_rollup_proof_repr.ml b/src/proto_alpha/lib_protocol/sc_rollup_proof_repr.ml index 187fea21d1ba..097bb837e4ef 100644 --- a/src/proto_alpha/lib_protocol/sc_rollup_proof_repr.ml +++ b/src/proto_alpha/lib_protocol/sc_rollup_proof_repr.ml @@ -317,14 +317,14 @@ module Dal_helpers = struct { page_content_opt; attestation_threshold_percent; - commitment_publisher_opt; + restricted_commitments_publishers; } = verify_proof_result in return ( Some (Sc_rollup_PVM_sig.Reveal (Dal_page page_content_opt)), attestation_threshold_percent, - commitment_publisher_opt ) + restricted_commitments_publishers ) else return (None, None, None) let produce ~metadata ~dal_activation_level ~dal_attestation_lag @@ -469,7 +469,9 @@ let valid (type state proof output) PVM is actually requesting a DAL page whose ID coincides with the one of the given proof. *) | Some (Reveal_proof (Dal_page_proof {proof; page_id})) -> - let*? input_opt, attestation_threshold_percent, commitment_publisher_opt = + let*? ( input_opt, + attestation_threshold_percent, + restricted_commitments_publishers ) = Dal_helpers.verify ~protocol_activation_level ~dal_number_of_slots @@ -530,19 +532,17 @@ let valid (type state proof output) "The provided attestation_threshold_percent in the proof \ doesn't match the one defined in the kernel." in - (* Finally, in case the ADAL proof is a confirmation of an - attested slot, we check that the publisher is indeed in the - whitelist stored in the input request (if any). *) + + (* Finally, we check that the provided value for + [restricted_commitments_publishers] in the proof matches the one + expected by the kernel. *) check - (Option.fold - ~none:true - ~some:(fun publisher -> - Dal_slot_repr.History.allowed_commitments_publisher - publisher - kernel_whitelist) - commitment_publisher_opt) - "The provided DAL slot publisher in the (DAL) confirmation \ - proof is not in the kernel's whitelisted publishers." + (Option.equal + (List.equal Contract_repr.equal) + kernel_whitelist + restricted_commitments_publishers) + "The provided restricted_commitments_publishers in the proof \ + doesn't match the one defined in the kernel." | _ -> inbox_proof_and_input_request_are_dissociated ()) (* Case where the provided proof pretends that the PVM is asking for DAL parameters. In this case, we just check that the PVM is requesting DAL diff --git a/src/proto_alpha/lib_protocol/test/helpers/dal_helpers.ml b/src/proto_alpha/lib_protocol/test/helpers/dal_helpers.ml index c549ab2ef2bd..773d985401e3 100644 --- a/src/proto_alpha/lib_protocol/test/helpers/dal_helpers.ml +++ b/src/proto_alpha/lib_protocol/test/helpers/dal_helpers.ml @@ -208,7 +208,7 @@ struct { page_content_opt; attestation_threshold_percent; - commitment_publisher_opt = _; + restricted_commitments_publishers = _; } -> (* These tests don't cover Adjustable DAL currently. *) assert (Option.is_none attestation_threshold_percent) ; -- GitLab From ab8ba9313c51c6992637b7eda6a255a949dfdc21 Mon Sep 17 00:00:00 2001 From: "iguerNL@Functori" Date: Thu, 9 Jan 2025 16:19:57 +0100 Subject: [PATCH 2/3] Proto/DAL: restore orignal implementation of is_commitment_attested --- .../lib_protocol/alpha_context.mli | 2 +- src/proto_alpha/lib_protocol/dal_slot_repr.ml | 31 ++++++++----------- .../lib_protocol/dal_slot_repr.mli | 9 ++---- .../lib_sc_rollup_node/dal_pages_request.ml | 2 +- 4 files changed, 18 insertions(+), 26 deletions(-) diff --git a/src/proto_alpha/lib_protocol/alpha_context.mli b/src/proto_alpha/lib_protocol/alpha_context.mli index f12c981df6bd..e1f7465add02 100644 --- a/src/proto_alpha/lib_protocol/alpha_context.mli +++ b/src/proto_alpha/lib_protocol/alpha_context.mli @@ -3008,7 +3008,7 @@ module Dal : sig attestation_threshold_percent:int option -> restricted_commitments_publishers:Contract.t list option -> cell_content -> - (Contract.t option, Slot.Commitment.t * Contract.t) Either.t + Slot.Commitment.t option type proof end diff --git a/src/proto_alpha/lib_protocol/dal_slot_repr.ml b/src/proto_alpha/lib_protocol/dal_slot_repr.ml index 7472cb34da19..fb61882d91ba 100644 --- a/src/proto_alpha/lib_protocol/dal_slot_repr.ml +++ b/src/proto_alpha/lib_protocol/dal_slot_repr.ml @@ -1323,12 +1323,9 @@ module History = struct ~restricted_commitments_publishers cell_content = let open Content_v2 in match (cell_content, attestation_threshold_percent) with - | Unpublished _, _ -> Either.Left None - | ( Published - {header = {commitment; id = _}; is_proto_attested; publisher; _}, - None ) -> - if is_proto_attested then Either.Right (commitment, publisher) - else Either.Left (Some publisher) + | Unpublished _, _ -> None + | Published {header = {commitment; id = _}; is_proto_attested; _}, None -> + if is_proto_attested then Some commitment else None | ( Published { header = {commitment; id = _}; @@ -1346,8 +1343,8 @@ module History = struct && allowed_commitments_publisher publisher restricted_commitments_publishers - then Either.Right (commitment, publisher) - else Either.Left (Some publisher) + then Some commitment + else None (** The [produce_proof_repr] function assumes that some invariants hold, such as: - The DAL has been activated, @@ -1397,8 +1394,7 @@ module History = struct ~restricted_commitments_publishers in match (page_info, is_commitment_attested) with - | ( Some (page_data, page_proof), - Either.Right (commitment, _commitment_publisher) ) -> + | Some (page_data, page_proof), Some commitment -> (* The case where the slot to which the page is supposed to belong is found and the page's information are given. *) let*? () = @@ -1422,7 +1418,7 @@ module History = struct restricted_commitments_publishers; }, Some page_data ) - | None, Either.Left _commitment_publisher_opt -> + | None, None -> (* The slot corresponding to the given page's index is not found in the attested slots of the page's level, and no information is given for that page. So, we produce a proof that the page is not @@ -1436,14 +1432,14 @@ module History = struct restricted_commitments_publishers; }, None ) - | None, Either.Right _ -> + | None, Some _ -> (* Mismatch: case where no page information are given, but the slot is attested. *) tzfail @@ dal_proof_error "The page ID's slot is confirmed, but no page content and \ proof are provided." - | Some _, Either.Left _ -> + | Some _, None -> (* Mismatch: case where page information are given, but the slot is not attested. *) tzfail @@ -1565,25 +1561,24 @@ module History = struct cell_content in match (proof, is_commitment_attested) with - | Page_unconfirmed _, Either.Right _ -> + | Page_unconfirmed _, Some _ -> error @@ dal_proof_error "verify_proof_repr: the confirmation proof doesn't contain the \ attested slot." - | Page_unconfirmed _, Either.Left _validated_commitment_publisher_opt -> + | Page_unconfirmed _, None -> return { page_content_opt = None; attestation_threshold_percent; restricted_commitments_publishers; } - | Page_confirmed _, Either.Left _ -> + | Page_confirmed _, None -> error @@ dal_proof_error "verify_proof_repr: the unconfirmation proof contains the \ target slot." - | ( Page_confirmed {page_data; page_proof; _}, - Either.Right (commitment, _validated_commitment_publisher) ) -> + | Page_confirmed {page_data; page_proof; _}, Some commitment -> (* We check that the page indeed belongs to the target slot at the given page index. *) let* () = diff --git a/src/proto_alpha/lib_protocol/dal_slot_repr.mli b/src/proto_alpha/lib_protocol/dal_slot_repr.mli index bbe30e4751b6..4eac48c00205 100644 --- a/src/proto_alpha/lib_protocol/dal_slot_repr.mli +++ b/src/proto_alpha/lib_protocol/dal_slot_repr.mli @@ -426,11 +426,8 @@ module History : sig val allowed_commitments_publisher : Contract_repr.t -> Contract_repr.t list option -> bool - (** This function returns a commitment and its publisher (wrapped in - Either.Right if and only if the skip list cell whose content is given is - attested. In case the commitment is not attested, the - function returns the publisher if the slot is published or None otherwise - (wrapped in Either.Left). + (** This function returns some commitment if and only if the skip list cell + whose content is given is supposed to be attested. Attestation status is either checked by inspecting the protocol's boolean flag stored in the content if [attestation_threshold_percent] is None @@ -441,7 +438,7 @@ module History : sig attestation_threshold_percent:int option -> restricted_commitments_publishers:Contract_repr.t list option -> cell_content -> - (Contract_repr.t option, Commitment.t * Contract_repr.t) Either.t + Commitment.t option (** [produce_proof dal_parameters page_id ~attestation_threshold_percent ~restricted_commitments_publishers page_info ~get_history slots_hist] diff --git a/src/proto_alpha/lib_sc_rollup_node/dal_pages_request.ml b/src/proto_alpha/lib_sc_rollup_node/dal_pages_request.ml index a3396b1050ee..e5062c124389 100644 --- a/src/proto_alpha/lib_sc_rollup_node/dal_pages_request.ml +++ b/src/proto_alpha/lib_sc_rollup_node/dal_pages_request.ml @@ -214,7 +214,7 @@ let slot_attestation_status ?attestation_threshold_percent ~restricted_commitments_publishers cell_content in - return @@ if Either.is_right commitment_res then `Attested else `Unattested + return @@ if Option.is_some commitment_res then `Attested else `Unattested let get_page node_ctxt ~inbox_level page_id = let open Lwt_result_syntax in -- GitLab From a11020e5051d1932baf0618877c7c6e8bac6268e Mon Sep 17 00:00:00 2001 From: "iguerNL@Functori" Date: Thu, 9 Jan 2025 13:46:43 +0100 Subject: [PATCH 3/3] Proto/DAL: harden Regular DAL proofs: there should be no publishers whitelist --- .../lib_protocol/sc_rollup_proof_repr.ml | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/proto_alpha/lib_protocol/sc_rollup_proof_repr.ml b/src/proto_alpha/lib_protocol/sc_rollup_proof_repr.ml index 097bb837e4ef..182730142528 100644 --- a/src/proto_alpha/lib_protocol/sc_rollup_proof_repr.ml +++ b/src/proto_alpha/lib_protocol/sc_rollup_proof_repr.ml @@ -495,15 +495,20 @@ let valid (type state proof output) "Dal proof's page ID is not the one expected in input \ request." in - (* For Regular DAL, we check [attestation_threshold_percent] is - set to None, as attestation status is decided by the protocol. - But, we don't have to check the value of - [commitment_publisher_opt]. (We could actually also omit the - verification of [attestation_threshold_percent]). *) + (* For Regular DAL, we check [attestation_threshold_percent] and + [restricted_commitments_publishers] are set to None. *) + let* () = + check + (Option.is_none attestation_threshold_percent) + "attestation_threshold_percent should be set None for \ + Regular DAL proofs. Setting it to Some _ is only valid for \ + Request_adal_page case." + in check - (Option.is_none attestation_threshold_percent) - "Cannot accept an Adjustable DAL proof in the context of \ - Regular Dal_page request" + (Option.is_none restricted_commitments_publishers) + "restricted_commitments_publishers should be set None for \ + Regular DAL proofs. Setting it to Some _ is only valid for \ + Request_adal_page case." | Needs_reveal (Request_adal_page { -- GitLab