From b91564b66c73af179a4dfe3113c904b0af12f960 Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Thu, 21 Jul 2022 17:27:19 +0200 Subject: [PATCH 1/9] Proto: rework the errors related to voting operations - Move these errors to Validate_errors.Voting. - Rename most of them to something more explicit. - Factorize, reorder, update the classifications, and/or add information to a few errors. - Adapt the protocol tests. --- src/proto_alpha/lib_protocol/amendment.ml | 131 ++------------- src/proto_alpha/lib_protocol/amendment.mli | 23 +-- src/proto_alpha/lib_protocol/apply.ml | 21 +-- src/proto_alpha/lib_protocol/apply.mli | 1 - .../lib_protocol/test/helpers/assert.ml | 19 +++ .../integration/operations/test_voting.ml | 145 +++++++++------- .../lib_protocol/validate_errors.ml | 159 ++++++++++++++++++ .../lib_protocol/validate_errors.mli | 67 +++++--- 8 files changed, 333 insertions(+), 233 deletions(-) diff --git a/src/proto_alpha/lib_protocol/amendment.ml b/src/proto_alpha/lib_protocol/amendment.ml index 9bc9a8d4829f..14600609a371 100644 --- a/src/proto_alpha/lib_protocol/amendment.ml +++ b/src/proto_alpha/lib_protocol/amendment.ml @@ -144,120 +144,11 @@ let start_new_voting_period ctxt = Vote.clear_current_proposal ctxt >>=? Voting_period.reset) >>=? fun ctxt -> Vote.update_listings ctxt -type error += - | (* `Branch *) - Invalid_proposal - | Unexpected_proposal - | Unauthorized_proposal - | Too_many_proposals - | Empty_proposal - | Unexpected_ballot - | Unauthorized_ballot - | Duplicate_ballot - | (* `Permanent *) - Invalid_dictator_proposal - -let () = - let open Data_encoding in - (* Invalid proposal *) - register_error_kind - `Branch - ~id:"invalid_proposal" - ~title:"Invalid proposal" - ~description:"Ballot provided for a proposal that is not the current one." - ~pp:(fun ppf () -> Format.fprintf ppf "Invalid proposal") - empty - (function Invalid_proposal -> Some () | _ -> None) - (fun () -> Invalid_proposal) ; - (* Unexpected proposal *) - register_error_kind - `Branch - ~id:"unexpected_proposal" - ~title:"Unexpected proposal" - ~description:"Proposal recorded outside of a proposal period." - ~pp:(fun ppf () -> Format.fprintf ppf "Unexpected proposal") - empty - (function Unexpected_proposal -> Some () | _ -> None) - (fun () -> Unexpected_proposal) ; - (* Unauthorized proposal *) - register_error_kind - `Branch - ~id:"unauthorized_proposal" - ~title:"Unauthorized proposal" - ~description: - "The delegate provided for the proposal is not in the voting listings." - ~pp:(fun ppf () -> Format.fprintf ppf "Unauthorized proposal") - empty - (function Unauthorized_proposal -> Some () | _ -> None) - (fun () -> Unauthorized_proposal) ; - (* Unexpected ballot *) - register_error_kind - `Branch - ~id:"unexpected_ballot" - ~title:"Unexpected ballot" - ~description:"Ballot recorded outside of a voting period." - ~pp:(fun ppf () -> Format.fprintf ppf "Unexpected ballot") - empty - (function Unexpected_ballot -> Some () | _ -> None) - (fun () -> Unexpected_ballot) ; - (* Unauthorized ballot *) - register_error_kind - `Branch - ~id:"unauthorized_ballot" - ~title:"Unauthorized ballot" - ~description: - "The delegate provided for the ballot is not in the voting listings." - ~pp:(fun ppf () -> Format.fprintf ppf "Unauthorized ballot") - empty - (function Unauthorized_ballot -> Some () | _ -> None) - (fun () -> Unauthorized_ballot) ; - (* Duplicate ballot *) - register_error_kind - `Branch - ~id:"duplicate_ballot" - ~title:"Duplicate ballot" - ~description:"The delegate has already submitted a ballot." - ~pp:(fun ppf () -> Format.fprintf ppf "Duplicate ballot") - empty - (function Duplicate_ballot -> Some () | _ -> None) - (fun () -> Duplicate_ballot) ; - (* Too many proposals *) - register_error_kind - `Branch - ~id:"too_many_proposals" - ~title:"Too many proposals" - ~description:"The delegate reached the maximum number of allowed proposals." - ~pp:(fun ppf () -> Format.fprintf ppf "Too many proposals") - empty - (function Too_many_proposals -> Some () | _ -> None) - (fun () -> Too_many_proposals) ; - (* Empty proposal *) - register_error_kind - `Branch - ~id:"empty_proposal" - ~title:"Empty proposal" - ~description:"Proposal lists cannot be empty." - ~pp:(fun ppf () -> Format.fprintf ppf "Empty proposal") - empty - (function Empty_proposal -> Some () | _ -> None) - (fun () -> Empty_proposal) ; - (* Invalid dictator proposal *) - register_error_kind - `Permanent - ~id:"invalid_dictator_proposal" - ~title:"Invalid dictator proposal" - ~description:"A testnet dictator can only submit one proposal at a time." - ~pp:(fun ppf () -> - Format.fprintf - ppf - "A testnet dictator can only submit one proposal at a time.") - empty - (function Invalid_dictator_proposal -> Some () | _ -> None) - (fun () -> Invalid_dictator_proposal) +open Validate_errors.Voting let record_delegate_proposals ctxt delegate proposals = (match proposals with - | [] -> error Empty_proposal + | [] -> error Empty_proposals | _ :: _ -> Result.return_unit) >>?= fun () -> Voting_period.get_current_kind ctxt >>=? function @@ -279,8 +170,9 @@ let record_delegate_proposals ctxt delegate proposals = (fun ctxt proposal -> Vote.record_proposal ctxt proposal delegate) ctxt proposals) - else fail Unauthorized_proposal - | Exploration | Cooldown | Promotion | Adoption -> fail Unexpected_proposal + else fail Source_not_in_vote_listings + | (Exploration | Cooldown | Promotion | Adoption) as current -> + fail (Wrong_voting_period_kind {current; expected = [Proposal]}) let record_testnet_dictator_proposals ctxt chain_id proposals = Vote.clear_ballots ctxt >>= fun ctxt -> @@ -298,7 +190,7 @@ let record_testnet_dictator_proposals ctxt chain_id proposals = ctxt chain_id Adoption - | _ :: _ :: _ -> fail Invalid_dictator_proposal + | _ :: _ :: _ -> fail Testnet_dictator_multiple_proposals let is_testnet_dictator ctxt chain_id delegate = (* This function should always, ALWAYS, return false on mainnet!!!! *) @@ -318,14 +210,17 @@ let record_ballot ctxt delegate proposal ballot = Vote.get_current_proposal ctxt >>=? fun current_proposal -> error_unless (Protocol_hash.equal proposal current_proposal) - Invalid_proposal + (Ballot_for_wrong_proposal + {current = current_proposal; submitted = proposal}) >>?= fun () -> Vote.has_recorded_ballot ctxt delegate >>= fun has_ballot -> - error_when has_ballot Duplicate_ballot >>?= fun () -> + error_when has_ballot Already_submitted_a_ballot >>?= fun () -> Vote.in_listings ctxt delegate >>= fun in_listings -> if in_listings then Vote.record_ballot ctxt delegate ballot - else fail Unauthorized_ballot - | Cooldown | Proposal | Adoption -> fail Unexpected_ballot + else fail Source_not_in_vote_listings + | (Cooldown | Proposal | Adoption) as current -> + fail + (Wrong_voting_period_kind {current; expected = [Exploration; Promotion]}) let may_start_new_voting_period ctxt = Voting_period.is_last_block ctxt >>=? fun is_last -> diff --git a/src/proto_alpha/lib_protocol/amendment.mli b/src/proto_alpha/lib_protocol/amendment.mli index fda070b1ac8d..6663dbfad001 100644 --- a/src/proto_alpha/lib_protocol/amendment.mli +++ b/src/proto_alpha/lib_protocol/amendment.mli @@ -75,8 +75,8 @@ open Alpha_context val may_start_new_voting_period : context -> context tzresult Lwt.t (** Records a list of proposals for a delegate. - @raise Unexpected_proposal if [ctxt] is not in a proposal period. - @raise Unauthorized_proposal if [delegate] is not in the listing. *) + @raise Wrong_voting_period_kind if [ctxt] is not in a proposal period. + @raise Source_not_in_vote_listings if [delegate] is not in the listing. *) val record_proposals : context -> Chain_id.t -> @@ -84,22 +84,13 @@ val record_proposals : Protocol_hash.t list -> context tzresult Lwt.t -type error += - | Invalid_proposal - | Unexpected_proposal - | Unauthorized_proposal - | Too_many_proposals - | Empty_proposal - | Unexpected_ballot - | Unauthorized_ballot - | Duplicate_ballot - (** Records a vote for a delegate if the current voting period is Exploration or Promotion. - @raise Invalid_proposal if [proposal] ≠ [current_proposal]. - @raise Duplicate_ballot if delegate already voted. - @raise Unauthorized_ballot if delegate is not listed to vote, - or if current period differs from Exploration or Promotion. + @raise Ballot_for_wrong_proposal if [proposal] ≠ [current_proposal]. + @raise Already_submitted_a_ballot if delegate already voted. + @raise Source_not_in_vote_listings if delegate is not listed to vote. + @raise Wrong_voting_period_kind if current period is not Exploration + or Promotion. *) val record_ballot : context -> diff --git a/src/proto_alpha/lib_protocol/apply.ml b/src/proto_alpha/lib_protocol/apply.ml index 92fc6e43f474..a19264ee8de0 100644 --- a/src/proto_alpha/lib_protocol/apply.ml +++ b/src/proto_alpha/lib_protocol/apply.ml @@ -40,7 +40,6 @@ type error += | Tx_rollup_invalid_transaction_ticket_amount | Cannot_transfer_ticket_to_implicit | Sc_rollup_feature_disabled - | Wrong_voting_period of {expected : int32; provided : int32} | Internal_operation_replay of Apply_internal_results.packed_internal_operation | Multiple_revelation @@ -197,20 +196,6 @@ let () = (function Sc_rollup_feature_disabled -> Some () | _ -> None) (fun () -> Sc_rollup_feature_disabled) ; - register_error_kind - `Temporary - ~id:"operation.wrong_voting_period" - ~title:"Wrong voting period" - ~description: - "Trying to include a proposal or ballot meant for another voting period" - ~pp:(fun ppf (e, p) -> - Format.fprintf ppf "Wrong voting period %ld, current is %ld" p e) - Data_encoding.( - obj2 (req "current_index" int32) (req "provided_index" int32)) - (function - | Wrong_voting_period {expected; provided} -> Some (expected, provided) - | _ -> None) - (fun (expected, provided) -> Wrong_voting_period {expected; provided}) ; register_error_kind `Permanent ~id:"internal_operation_replay" @@ -2148,7 +2133,8 @@ let apply_contents_list (type kind) ctxt chain_id (apply_mode : apply_mode) Voting_period.get_current ctxt >>=? fun {index = current_period; _} -> error_unless Compare.Int32.(current_period = period) - (Wrong_voting_period {expected = current_period; provided = period}) + (Validate_errors.Voting.Wrong_voting_period_index + {expected = current_period; provided = period}) >>?= fun () -> Amendment.record_proposals ctxt chain_id source proposals >|=? fun ctxt -> (ctxt, Single_result Proposals_result) @@ -2158,7 +2144,8 @@ let apply_contents_list (type kind) ctxt chain_id (apply_mode : apply_mode) Voting_period.get_current ctxt >>=? fun {index = current_period; _} -> error_unless Compare.Int32.(current_period = period) - (Wrong_voting_period {expected = current_period; provided = period}) + (Validate_errors.Voting.Wrong_voting_period_index + {expected = current_period; provided = period}) >>?= fun () -> Amendment.record_ballot ctxt source proposal ballot >|=? fun ctxt -> (ctxt, Single_result Ballot_result) diff --git a/src/proto_alpha/lib_protocol/apply.mli b/src/proto_alpha/lib_protocol/apply.mli index 9c401388e411..58de622e7180 100644 --- a/src/proto_alpha/lib_protocol/apply.mli +++ b/src/proto_alpha/lib_protocol/apply.mli @@ -43,7 +43,6 @@ type error += | Tx_rollup_invalid_transaction_ticket_amount | Sc_rollup_feature_disabled | Empty_transaction of Contract.t - | Wrong_voting_period of {expected : int32; provided : int32} val begin_partial_construction : context -> diff --git a/src/proto_alpha/lib_protocol/test/helpers/assert.ml b/src/proto_alpha/lib_protocol/test/helpers/assert.ml index 91f1ba7971f0..e4739337d3af 100644 --- a/src/proto_alpha/lib_protocol/test/helpers/assert.ml +++ b/src/proto_alpha/lib_protocol/test/helpers/assert.ml @@ -157,6 +157,25 @@ let not_equal_pkh ~loc (a : Signature.Public_key_hash.t) let module PKH = Signature.Public_key_hash in not_equal ~loc PKH.equal "Public key hashes are equal" PKH.pp a b +(* protocol hash *) +let equal_protocol_hash ~loc (a : Protocol_hash.t) (b : Protocol_hash.t) = + equal + ~loc + Protocol_hash.equal + "Protocol hashes aren't equal" + Protocol_hash.pp + a + b + +let not_equal_protocol_hash ~loc (a : Protocol_hash.t) (b : Protocol_hash.t) = + not_equal + ~loc + Protocol_hash.equal + "Protocol hashes are equal" + Protocol_hash.pp + a + b + let get_some ~loc = function | Some x -> return x | None -> failwith "Unexpected None (%s)" loc diff --git a/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml b/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml index c4076897eb58..809b9608b1d5 100644 --- a/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml +++ b/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml @@ -322,9 +322,11 @@ let invalid_signature loc = function | [Environment.Ecoproto_error Operation.Invalid_signature] -> return_unit | err -> wrong_error "Invalid_signature" err loc -let wrong_voting_period ~current_index ~op_index loc = function +open Validate_errors.Voting + +let wrong_voting_period_index ~current_index ~op_index loc = function | [ - Environment.Ecoproto_error (Apply.Wrong_voting_period {expected; provided}); + Environment.Ecoproto_error (Wrong_voting_period_index {expected; provided}); ] -> let open Lwt_result_syntax in let make_loc = append_loc ~caller_loc:loc in @@ -332,39 +334,44 @@ let wrong_voting_period ~current_index ~op_index loc = function Assert.equal_int32 ~loc:(make_loc __LOC__) expected current_index in Assert.equal_int32 ~loc:(make_loc __LOC__) provided op_index - | err -> wrong_error "Wrong_voting_period" err loc + | err -> wrong_error "Wrong_voting_period_index" err loc -let unexpected_proposal loc = function - | [Environment.Ecoproto_error Amendment.Unexpected_proposal] -> return_unit - | err -> wrong_error "Unexpected_proposal" err loc +let wrong_voting_period_kind loc = function + | [Environment.Ecoproto_error (Wrong_voting_period_kind _)] -> return_unit + | err -> wrong_error "Wrong_voting_period_kind" err loc -let unauthorized_proposal loc = function - | [Environment.Ecoproto_error Amendment.Unauthorized_proposal] -> return_unit - | err -> wrong_error "Unauthorized_proposal" err loc +let source_not_in_vote_listings loc = function + | [Environment.Ecoproto_error Source_not_in_vote_listings] -> return_unit + | err -> wrong_error "Source_not_in_vote_listings" err loc -let empty_proposal loc = function - | [Environment.Ecoproto_error Amendment.Empty_proposal] -> return_unit - | err -> wrong_error "Empty_proposal" err loc +let empty_proposals loc = function + | [Environment.Ecoproto_error Empty_proposals] -> return_unit + | err -> wrong_error "Empty_proposals" err loc let too_many_proposals loc = function - | [Environment.Ecoproto_error Amendment.Too_many_proposals] -> return_unit + | [Environment.Ecoproto_error Too_many_proposals] -> return_unit | err -> wrong_error "Too_many_proposals" err loc -let unexpected_ballot loc = function - | [Environment.Ecoproto_error Amendment.Unexpected_ballot] -> return_unit - | err -> wrong_error "Unexpected_ballot" err loc - -let invalid_proposal loc = function - | [Environment.Ecoproto_error Amendment.Invalid_proposal] -> return_unit - | err -> wrong_error "Invalid_proposal" err loc - -let duplicate_ballot loc = function - | [Environment.Ecoproto_error Amendment.Duplicate_ballot] -> return_unit - | err -> wrong_error "Duplicate_ballot" err loc +let ballot_for_wrong_proposal ~current_proposal ~op_proposal loc = function + | [ + Environment.Ecoproto_error (Ballot_for_wrong_proposal {current; submitted}); + ] -> + let open Lwt_result_syntax in + let* () = + Assert.equal_protocol_hash + ~loc:(append_loc ~caller_loc:loc __LOC__) + current_proposal + current + in + Assert.equal_protocol_hash + ~loc:(append_loc ~caller_loc:loc __LOC__) + op_proposal + submitted + | err -> wrong_error "Ballot_for_wrong_proposal" err loc -let unauthorized_ballot loc = function - | [Environment.Ecoproto_error Amendment.Unauthorized_ballot] -> return_unit - | err -> wrong_error "Unauthorized_ballot" err loc +let already_submitted_a_ballot loc = function + | [Environment.Ecoproto_error Already_submitted_a_ballot] -> return_unit + | err -> wrong_error "Already_submitted_a_ballot" err loc let assert_validate_proposals_fails ~expected_error ~proposer ~proposals ?period block loc = @@ -483,7 +490,7 @@ let test_successful_vote num_delegates () = >>=? fun () -> (* proposing less than one proposal fails *) assert_validate_proposals_fails - ~expected_error:empty_proposal + ~expected_error:empty_proposals ~proposer:del1 ~proposals:[] b @@ -533,7 +540,7 @@ let test_successful_vote num_delegates () = belongs to [delegates_p2], so they have already sent a ballot during the unanimous vote right above). *) assert_validate_ballot_fails - ~expected_error:duplicate_ballot + ~expected_error:already_submitted_a_ballot ~voter:del1 ~proposal:Protocol_hash.zero ~ballot:Vote.Nay @@ -1275,14 +1282,14 @@ let test_proposals_invalid_signature () = (** Test that a Proposals operation fails when the period index provided in the operation is not the current voting period index. *) -let test_proposals_wrong_voting_period () = +let test_proposals_wrong_voting_period_index () = let open Lwt_result_syntax in let* block, proposer = context_init1 () in let* current_period = Context.Vote.get_current_period (B block) in let current_index = current_period.voting_period.index in let op_index = Int32.succ current_index in assert_validate_proposals_fails - ~expected_error:(wrong_voting_period ~current_index ~op_index) + ~expected_error:(wrong_voting_period_index ~current_index ~op_index) ~proposer ~proposals:[Protocol_hash.zero] ~period:op_index @@ -1291,13 +1298,13 @@ let test_proposals_wrong_voting_period () = (** Test that a Proposals operation fails when it occurs in a non-Proposal voting period. *) -let test_unexpected_proposal () = +let test_proposals_wrong_voting_period_kind () = let open Lwt_result_syntax in let* block, proposer = context_init1 () in let proposal = protos.(0) in let assert_proposals_fails_with_unexpected_proposal = assert_validate_proposals_fails - ~expected_error:unexpected_proposal + ~expected_error:wrong_voting_period_kind ~proposer ~proposals:[proposal] in @@ -1332,7 +1339,7 @@ let test_unexpected_proposal () = (** Test that a Proposals operation fails when the proposer is not in the vote listings. *) -let test_unauthorized_proposal () = +let test_proposals_source_not_in_vote_listings () = let open Lwt_result_syntax in let* block, funder = context_init1 () in let account = Account.new_account () in @@ -1344,7 +1351,7 @@ let test_unauthorized_proposal () = in let* block = Block.bake block ~operation in assert_validate_proposals_fails - ~expected_error:unauthorized_proposal + ~expected_error:source_not_in_vote_listings ~proposer ~proposals:[Protocol_hash.zero] block @@ -1352,11 +1359,11 @@ let test_unauthorized_proposal () = (** Test that a Proposals operation fails when its proposal list is empty. *) -let test_empty_proposal () = +let test_empty_proposals () = let open Lwt_result_syntax in let* block, proposer = context_init1 () in assert_validate_proposals_fails - ~expected_error:empty_proposal + ~expected_error:empty_proposals ~proposer ~proposals:[] block @@ -1608,14 +1615,14 @@ let test_ballot_invalid_signature () = (** Test that a Ballot operation fails when the period index provided in the operation is not the current voting period index. *) -let test_ballot_wrong_voting_period () = +let test_ballot_wrong_voting_period_index () = let open Lwt_result_syntax in let* block, voter = context_init1 () in let* current_period = Context.Vote.get_current_period (B block) in let current_index = current_period.voting_period.index in let op_index = Int32.succ current_index in assert_validate_ballot_fails - ~expected_error:(wrong_voting_period ~current_index ~op_index) + ~expected_error:(wrong_voting_period_index ~current_index ~op_index) ~voter ~proposal:protos.(0) ~ballot:Vote.Yay @@ -1625,13 +1632,13 @@ let test_ballot_wrong_voting_period () = (** Test that a Ballot operation fails when it occurs outside of an Exploration or Promotion voting period. *) -let test_unexpected_ballot () = +let test_ballot_wrong_voting_period_kind () = let open Lwt_result_syntax in let* block, voter = context_init1 () in let proposal = protos.(0) in let assert_ballot_fails_with_unexpected_ballot = assert_validate_ballot_fails - ~expected_error:unexpected_ballot + ~expected_error:wrong_voting_period_kind ~voter ~proposal ~ballot:Vote.Nay @@ -1664,28 +1671,29 @@ let test_unexpected_ballot () = (** Test that a Ballot operation fails when its proposal is not the current proposal. *) -let test_ballot_on_invalid_proposal () = +let test_ballot_for_wrong_proposal () = let open Lwt_result_syntax in - let* block, voter, _proposal = + let* block, voter, current_proposal = context_init_exploration ~proposal:protos.(0) () in + let op_proposal = protos.(1) in assert_validate_ballot_fails - ~expected_error:invalid_proposal + ~expected_error:(ballot_for_wrong_proposal ~current_proposal ~op_proposal) ~voter - ~proposal:protos.(1) + ~proposal:op_proposal ~ballot:Vote.Yay block __LOC__ (** Test that a Ballot operation fails when its source has already submitted a Ballot. *) -let test_duplicate_ballot () = +let test_already_submitted_a_ballot () = let open Lwt_result_syntax in let* block, voter, proposal = context_init_exploration () in let* operation = Op.ballot (B block) voter proposal Vote.Yay in let* block = Block.bake ~operation block in assert_validate_ballot_fails - ~expected_error:duplicate_ballot + ~expected_error:already_submitted_a_ballot ~voter ~proposal ~ballot:Vote.Nay @@ -1694,7 +1702,7 @@ let test_duplicate_ballot () = (** Test that a Ballot operation fails when its source is not in the vote listings. *) -let test_unauthorized_ballot () = +let test_ballot_source_not_in_vote_listings () = let open Lwt_result_syntax in let* block, funder, proposal = context_init_exploration () in let account = Account.new_account () in @@ -1706,7 +1714,7 @@ let test_unauthorized_ballot () = in let* block = Block.bake block ~operation in assert_validate_ballot_fails - ~expected_error:unauthorized_ballot + ~expected_error:source_not_in_vote_listings ~voter ~proposal ~ballot:Vote.Yay @@ -1899,12 +1907,18 @@ let tests = `Quick test_proposals_invalid_signature; Tztest.tztest - "Proposals wrong voting period" + "Proposals wrong voting period index" + `Quick + test_proposals_wrong_voting_period_index; + Tztest.tztest + "Proposals wrong voting period kind" + `Quick + test_proposals_wrong_voting_period_kind; + Tztest.tztest + "Proposals source not in vote listings" `Quick - test_proposals_wrong_voting_period; - Tztest.tztest "Unexpected proposals" `Quick test_unexpected_proposal; - Tztest.tztest "Unauthorized proposal" `Quick test_unauthorized_proposal; - Tztest.tztest "Empty proposal" `Quick test_empty_proposal; + test_proposals_source_not_in_vote_listings; + Tztest.tztest "Empty proposals" `Quick test_empty_proposals; Tztest.tztest "Operation has too many proposals" `Quick @@ -1929,15 +1943,24 @@ let tests = `Quick test_ballot_invalid_signature; Tztest.tztest - "Ballot wrong voting period" + "Ballot wrong voting period index" + `Quick + test_ballot_wrong_voting_period_index; + Tztest.tztest + "Ballot wrong voting period kind" + `Quick + test_ballot_wrong_voting_period_kind; + Tztest.tztest + "Ballot for wrong proposal" + `Quick + test_ballot_for_wrong_proposal; + Tztest.tztest + "Delegate has already submitted a ballot" `Quick - test_ballot_wrong_voting_period; - Tztest.tztest "Unexpected ballot" `Quick test_unexpected_ballot; + test_already_submitted_a_ballot; Tztest.tztest - "Ballot on invalid proposal" + "Ballot source not in vote listings" `Quick - test_ballot_on_invalid_proposal; - Tztest.tztest "Duplicate ballot" `Quick test_duplicate_ballot; - Tztest.tztest "Unauthorized ballot" `Quick test_unauthorized_ballot; + test_ballot_source_not_in_vote_listings; Tztest.tztest "Valid Ballot operations" `Quick test_valid_ballot; ] diff --git a/src/proto_alpha/lib_protocol/validate_errors.ml b/src/proto_alpha/lib_protocol/validate_errors.ml index 0f4ab0674ff4..11fade8de448 100644 --- a/src/proto_alpha/lib_protocol/validate_errors.ml +++ b/src/proto_alpha/lib_protocol/validate_errors.ml @@ -380,6 +380,165 @@ module Consensus = struct (fun endorser -> Conflicting_dal_slot_availability {endorser}) end +module Voting = struct + type error += + | (* Shared voting errors *) + Wrong_voting_period_index of { + expected : int32; + provided : int32; + } + | Wrong_voting_period_kind of { + current : Voting_period.kind; + expected : Voting_period.kind list; + } + | Source_not_in_vote_listings + | (* Proposals errors *) + Empty_proposals + | Too_many_proposals + | Testnet_dictator_multiple_proposals + | (* Ballot errors *) + Ballot_for_wrong_proposal of { + current : Protocol_hash.t; + submitted : Protocol_hash.t; + } + | Already_submitted_a_ballot + + let () = + (* Shared voting errors *) + register_error_kind + `Temporary + ~id:"validate_operation.wrong_voting_period_index" + ~title:"Wrong voting period index" + ~description: + "The voting operation contains a voting period index different from \ + the current one." + ~pp:(fun ppf (expected, provided) -> + Format.fprintf + ppf + "The voting operation is meant for voting period %ld, whereas the \ + current period is %ld." + provided + expected) + Data_encoding.( + obj2 (req "current_index" int32) (req "provided_index" int32)) + (function + | Wrong_voting_period_index {expected; provided} -> + Some (expected, provided) + | _ -> None) + (fun (expected, provided) -> + Wrong_voting_period_index {expected; provided}) ; + register_error_kind + `Temporary + ~id:"validate_operation.wrong_voting_period_kind" + ~title:"Wrong voting period kind" + ~description: + "The voting operation is incompatible the current voting period kind." + ~pp:(fun ppf (current, expected) -> + Format.fprintf + ppf + "The voting operation is only valid during a %a voting period, but \ + we are currently in a %a period." + (Format.pp_print_list + ~pp_sep:(fun fmt () -> Format.fprintf fmt " or ") + Voting_period.pp_kind) + expected + Voting_period.pp_kind + current) + Data_encoding.( + obj2 + (req "current" Voting_period.kind_encoding) + (req "expected" (list Voting_period.kind_encoding))) + (function + | Wrong_voting_period_kind {current; expected} -> + Some (current, expected) + | _ -> None) + (fun (current, expected) -> Wrong_voting_period_kind {current; expected}) ; + let description = "The delegate is not in the vote listings." in + register_error_kind + `Temporary + ~id:"validate_operation.source_not_in_vote_listings" + ~title:"Source not in vote listings" + ~description + ~pp:(fun ppf () -> Format.fprintf ppf "%s" description) + Data_encoding.empty + (function Source_not_in_vote_listings -> Some () | _ -> None) + (fun () -> Source_not_in_vote_listings) ; + + (* Proposals errors *) + let description = "Proposal list cannot be empty." in + register_error_kind + `Permanent + ~id:"validate_operation.empty_proposals" + ~title:"Empty proposals" + ~description + ~pp:(fun ppf () -> Format.fprintf ppf "%s" description) + Data_encoding.empty + (function Empty_proposals -> Some () | _ -> None) + (fun () -> Empty_proposals) ; + let description = + "The proposer exceeded the maximum number of allowed proposals." + in + register_error_kind + `Branch + ~id:"validate_operation.too_many_proposals" + ~title:"Too many proposals" + ~description + ~pp:(fun ppf () -> Format.fprintf ppf "%s" description) + Data_encoding.empty + (function Too_many_proposals -> Some () | _ -> None) + (fun () -> Too_many_proposals) ; + let description = + "A testnet dictator cannot submit more than one proposal at a time." + in + register_error_kind + `Permanent + ~id:"validate_operation.testnet_dictator_multiple_proposals" + ~title:"Testnet dictator multiple proposals" + ~description + ~pp:(fun ppf () -> Format.fprintf ppf "%s" description) + Data_encoding.empty + (function Testnet_dictator_multiple_proposals -> Some () | _ -> None) + (fun () -> Testnet_dictator_multiple_proposals) ; + + (* Ballot errors *) + register_error_kind + `Branch + ~id:"validate_operation.ballot_for_wrong_proposal" + ~title:"Ballot for wrong proposal" + ~description:"Ballot provided for a proposal that is not the current one." + ~pp:(fun ppf (current, submitted) -> + Format.fprintf + ppf + "Ballot provided for proposal %a whereas the current proposal is %a." + Protocol_hash.pp + submitted + Protocol_hash.pp + current) + Data_encoding.( + obj2 + (req "current_proposal" Protocol_hash.encoding) + (req "ballot_proposal" Protocol_hash.encoding)) + (function + | Ballot_for_wrong_proposal {current; submitted} -> + Some (current, submitted) + | _ -> None) + (fun (current, submitted) -> + Ballot_for_wrong_proposal {current; submitted}) ; + let description = + "The delegate has already submitted a ballot for the current voting \ + period." + in + register_error_kind + `Branch + ~id:"validate_operation.already_submitted_a_ballot" + ~title:"Already submitted a ballot" + ~description + ~pp:(fun ppf () -> Format.fprintf ppf "%s" description) + Data_encoding.empty + (function Already_submitted_a_ballot -> Some () | _ -> None) + (fun () -> Already_submitted_a_ballot) +end + module Anonymous = struct type error += | Invalid_activation of {pkh : Ed25519.Public_key_hash.t} diff --git a/src/proto_alpha/lib_protocol/validate_errors.mli b/src/proto_alpha/lib_protocol/validate_errors.mli index 424c8bd820bf..358a6cf5dd6a 100644 --- a/src/proto_alpha/lib_protocol/validate_errors.mli +++ b/src/proto_alpha/lib_protocol/validate_errors.mli @@ -23,6 +23,8 @@ (* *) (*****************************************************************************) +open Alpha_context + (** Errors that may arise while validating a consensus operation. *) module Consensus : sig type consensus_operation_kind = @@ -36,23 +38,23 @@ module Consensus : sig | Consensus_operation_not_allowed | Consensus_operation_for_old_level of { kind : consensus_operation_kind; - expected : Alpha_context.Raw_level.t; - provided : Alpha_context.Raw_level.t; + expected : Raw_level.t; + provided : Raw_level.t; } | Consensus_operation_for_future_level of { kind : consensus_operation_kind; - expected : Alpha_context.Raw_level.t; - provided : Alpha_context.Raw_level.t; + expected : Raw_level.t; + provided : Raw_level.t; } | Consensus_operation_for_old_round of { kind : consensus_operation_kind; - expected : Alpha_context.Round.t; - provided : Alpha_context.Round.t; + expected : Round.t; + provided : Round.t; } | Consensus_operation_for_future_round of { kind : consensus_operation_kind; - expected : Alpha_context.Round.t; - provided : Alpha_context.Round.t; + expected : Round.t; + provided : Round.t; } | Wrong_consensus_operation_branch of { kind : consensus_operation_kind; @@ -67,8 +69,8 @@ module Consensus : sig | Unexpected_preendorsement_in_block | Unexpected_endorsement_in_block | Preendorsement_round_too_high of { - block_round : Alpha_context.Round.t; - provided : Alpha_context.Round.t; + block_round : Round.t; + provided : Round.t; } | Wrong_slot_used_for_consensus_operation of { kind : consensus_operation_kind; @@ -79,6 +81,31 @@ module Consensus : sig } end +(** Errors that may arise while validating a voting operation. *) +module Voting : sig + type error += + | (* Shared voting errors *) + Wrong_voting_period_index of { + expected : int32; + provided : int32; + } + | Wrong_voting_period_kind of { + current : Voting_period.kind; + expected : Voting_period.kind list; + } + | Source_not_in_vote_listings + | (* Proposals errors *) + Empty_proposals + | Too_many_proposals + | Testnet_dictator_multiple_proposals + | (* Ballot errors *) + Ballot_for_wrong_proposal of { + current : Protocol_hash.t; + submitted : Protocol_hash.t; + } + | Already_submitted_a_ballot +end + (** Errors that may arise while validating an anonymous operation. *) module Anonymous : sig type denunciation_kind = Preendorsement | Endorsement | Block @@ -89,11 +116,11 @@ module Anonymous : sig | Invalid_denunciation of denunciation_kind | Invalid_double_baking_evidence of { hash1 : Block_hash.t; - level1 : Alpha_context.Raw_level.t; - round1 : Alpha_context.Round.t; + level1 : Raw_level.t; + round1 : Round.t; hash2 : Block_hash.t; - level2 : Alpha_context.Raw_level.t; - round2 : Alpha_context.Round.t; + level2 : Raw_level.t; + round2 : Round.t; } | Inconsistent_denunciation of { kind : denunciation_kind; @@ -103,23 +130,23 @@ module Anonymous : sig | Already_denounced of { kind : denunciation_kind; delegate : Signature.Public_key_hash.t; - level : Alpha_context.Level.t; + level : Level.t; } | Conflicting_denunciation of { kind : denunciation_kind; delegate : Signature.Public_key_hash.t; - level : Alpha_context.Level.t; + level : Level.t; hash : Operation_hash.t; } | Too_early_denunciation of { kind : denunciation_kind; - level : Alpha_context.Raw_level.t; - current : Alpha_context.Raw_level.t; + level : Raw_level.t; + current : Raw_level.t; } | Outdated_denunciation of { kind : denunciation_kind; - level : Alpha_context.Raw_level.t; - last_cycle : Alpha_context.Cycle.t; + level : Raw_level.t; + last_cycle : Cycle.t; } | Conflicting_nonce_revelation end -- GitLab From bdd06842a7c3bc918a17f97ca4d348abb3477fe9 Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Thu, 7 Jul 2022 15:08:26 +0200 Subject: [PATCH 2/9] Proto: move application body of voting operations to amendment.ml --- src/proto_alpha/lib_protocol/amendment.ml | 36 ++++++- src/proto_alpha/lib_protocol/amendment.mli | 93 ++++++++++++++----- src/proto_alpha/lib_protocol/apply.ml | 24 +---- .../lib_protocol/voting_period_storage.ml | 3 + 4 files changed, 110 insertions(+), 46 deletions(-) diff --git a/src/proto_alpha/lib_protocol/amendment.ml b/src/proto_alpha/lib_protocol/amendment.ml index 14600609a371..6735708513cf 100644 --- a/src/proto_alpha/lib_protocol/amendment.ml +++ b/src/proto_alpha/lib_protocol/amendment.ml @@ -144,6 +144,12 @@ let start_new_voting_period ctxt = Vote.clear_current_proposal ctxt >>=? Voting_period.reset) >>=? fun ctxt -> Vote.update_listings ctxt +let may_start_new_voting_period ctxt = + Voting_period.is_last_block ctxt >>=? fun is_last -> + if is_last then start_new_voting_period ctxt else return ctxt + +(** {2 Validation and application of voting operations} *) + open Validate_errors.Voting let record_delegate_proposals ctxt delegate proposals = @@ -204,6 +210,20 @@ let record_proposals ctxt chain_id delegate proposals = record_testnet_dictator_proposals ctxt chain_id proposals else record_delegate_proposals ctxt delegate proposals +let apply_proposals ctxt chain_id (operation : Kind.proposals operation) = + let (Single (Proposals {source; period; proposals})) = + operation.protocol_data.contents + in + Delegate.pubkey ctxt source >>=? fun delegate -> + Operation.check_signature delegate chain_id operation >>?= fun () -> + Voting_period.get_current ctxt >>=? fun {index = current_period; _} -> + error_unless + Compare.Int32.(current_period = period) + (Wrong_voting_period_index {expected = current_period; provided = period}) + >>?= fun () -> + record_proposals ctxt chain_id source proposals >|=? fun ctxt -> + (ctxt, Apply_results.Single_result Proposals_result) + let record_ballot ctxt delegate proposal ballot = Voting_period.get_current_kind ctxt >>=? function | Exploration | Promotion -> @@ -222,6 +242,16 @@ let record_ballot ctxt delegate proposal ballot = fail (Wrong_voting_period_kind {current; expected = [Exploration; Promotion]}) -let may_start_new_voting_period ctxt = - Voting_period.is_last_block ctxt >>=? fun is_last -> - if is_last then start_new_voting_period ctxt else return ctxt +let apply_ballot ctxt chain_id (operation : Kind.ballot operation) = + let (Single (Ballot {source; period; proposal; ballot})) = + operation.protocol_data.contents + in + Delegate.pubkey ctxt source >>=? fun delegate -> + Operation.check_signature delegate chain_id operation >>?= fun () -> + Voting_period.get_current ctxt >>=? fun {index = current_period; _} -> + error_unless + Compare.Int32.(current_period = period) + (Wrong_voting_period_index {expected = current_period; provided = period}) + >>?= fun () -> + record_ballot ctxt source proposal ballot >|=? fun ctxt -> + (ctxt, Apply_results.Single_result Ballot_result) diff --git a/src/proto_alpha/lib_protocol/amendment.mli b/src/proto_alpha/lib_protocol/amendment.mli index 6663dbfad001..fd95be5d99c6 100644 --- a/src/proto_alpha/lib_protocol/amendment.mli +++ b/src/proto_alpha/lib_protocol/amendment.mli @@ -74,27 +74,78 @@ open Alpha_context the state machine of the amendment procedure. *) val may_start_new_voting_period : context -> context tzresult Lwt.t -(** Records a list of proposals for a delegate. - @raise Wrong_voting_period_kind if [ctxt] is not in a proposal period. - @raise Source_not_in_vote_listings if [delegate] is not in the listing. *) -val record_proposals : +(** {2 Validation and application of voting operations} + + There are two kinds of voting operations: + + - Proposals: A delegate submits a list of protocol amendment + proposals. This operation is only accepted during a Proposal period + (see above). + + - Ballot: A delegate casts a vote for/against the current proposal + (or pass). This operation is only accepted during an Exploration + or Promotion period (see above). *) + +(** Update the [context] with the effects of a Proposals operation. + + @return [Error Delegate_storage.Unregistered_delegate] if the + source's contract does not exist or its public key is unreavealed. + + @return [Error Operation.Missing_signature] or [Error + Operation.Invalid_signature] if the operation is unsigned or + incorrectly signed. + + @return [Error Wrong_voting_period_index] if the operation's + period and the [context]'s current period do not have the same + index. + + @return [Error Empty_proposals] if the list of proposals is empty. + + @return [Error Wrong_voting_period_kind] if the voting period is + not of the Proposal kind. + + @return [Error Source_not_in_vote_listings] if the source is not + in the vote listings. + + @return [Error Too_many_proposals] if the operation would make the + source's total number of proposals exceed + {!Constants.recorded_proposal_count_for_delegate}. + + @return [Error Testnet_dictator_multiple_proposals] if the source + is a testnet dictator and the operation contains more than one + proposal. *) +val apply_proposals : context -> Chain_id.t -> - public_key_hash -> - Protocol_hash.t list -> - context tzresult Lwt.t - -(** Records a vote for a delegate if the current voting period is - Exploration or Promotion. - @raise Ballot_for_wrong_proposal if [proposal] ≠ [current_proposal]. - @raise Already_submitted_a_ballot if delegate already voted. - @raise Source_not_in_vote_listings if delegate is not listed to vote. - @raise Wrong_voting_period_kind if current period is not Exploration - or Promotion. -*) -val record_ballot : + Kind.proposals operation -> + (context * Kind.proposals Apply_results.contents_result_list) tzresult Lwt.t + +(** Update the [context] with the effects of a Ballot operation. + + @return [Error Delegate_storage.Unregistered_delegate] if the + source's contract does not exist or its public key is unreavealed. + + @return [Error Operation.Missing_signature] or [Error + Operation.Invalid_signature] if the operation is unsigned or + incorrectly signed. + + @return [Error Wrong_voting_period_index] if the operation's + period and the [context]'s current period do not have the same + index. + + @return [Error Wrong_voting_period_kind] if the voting period is + not of the Exploration or Promotion kind. + + @return [Error Ballot_for_wrong_proposal] if the operation's + proposal is different from the [context]'s current proposal. + + @return [Error Already_submitted_a_ballot] if the source has + already voted. + + @return [Error Source_not_in_vote_listings] if the source is not + in the vote listings. *) +val apply_ballot : context -> - public_key_hash -> - Protocol_hash.t -> - Vote.ballot -> - context tzresult Lwt.t + Chain_id.t -> + Kind.ballot operation -> + (context * Kind.ballot Apply_results.contents_result_list) tzresult Lwt.t diff --git a/src/proto_alpha/lib_protocol/apply.ml b/src/proto_alpha/lib_protocol/apply.ml index a19264ee8de0..3917e642c298 100644 --- a/src/proto_alpha/lib_protocol/apply.ml +++ b/src/proto_alpha/lib_protocol/apply.ml @@ -2127,28 +2127,8 @@ let apply_contents_list (type kind) ctxt chain_id (apply_mode : apply_mode) Token.transfer ctxt src (`Contract contract) amount >>=? fun (ctxt, bupds) -> return (ctxt, Single_result (Activate_account_result bupds)) - | Single (Proposals {source; period; proposals}) -> - Delegate.pubkey ctxt source >>=? fun delegate -> - Operation.check_signature delegate chain_id operation >>?= fun () -> - Voting_period.get_current ctxt >>=? fun {index = current_period; _} -> - error_unless - Compare.Int32.(current_period = period) - (Validate_errors.Voting.Wrong_voting_period_index - {expected = current_period; provided = period}) - >>?= fun () -> - Amendment.record_proposals ctxt chain_id source proposals >|=? fun ctxt -> - (ctxt, Single_result Proposals_result) - | Single (Ballot {source; period; proposal; ballot}) -> - Delegate.pubkey ctxt source >>=? fun delegate -> - Operation.check_signature delegate chain_id operation >>?= fun () -> - Voting_period.get_current ctxt >>=? fun {index = current_period; _} -> - error_unless - Compare.Int32.(current_period = period) - (Validate_errors.Voting.Wrong_voting_period_index - {expected = current_period; provided = period}) - >>?= fun () -> - Amendment.record_ballot ctxt source proposal ballot >|=? fun ctxt -> - (ctxt, Single_result Ballot_result) + | Single (Proposals _) -> Amendment.apply_proposals ctxt chain_id operation + | Single (Ballot _) -> Amendment.apply_ballot ctxt chain_id operation | Single (Failing_noop _) -> (* Failing_noop _ always fails *) fail Failing_noop_error diff --git a/src/proto_alpha/lib_protocol/voting_period_storage.ml b/src/proto_alpha/lib_protocol/voting_period_storage.ml index b15dd5230096..cf080ff404d8 100644 --- a/src/proto_alpha/lib_protocol/voting_period_storage.ml +++ b/src/proto_alpha/lib_protocol/voting_period_storage.ml @@ -194,6 +194,9 @@ let get_rpc_succ_info ctxt = Voting_period_repr.{voting_period; position; remaining} module Testnet_dictator = struct + (* This error must never happen. It is deliberately unregistered so + that the execution fails loudly if [overwrite_current_kind] is + ever called on mainnet. *) type error += Forbidden_on_mainnet let overwrite_current_kind ctxt chain_id kind = -- GitLab From 741b6876f580536064bc0ba5099ac25d192e6144 Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Fri, 8 Jul 2022 18:18:45 +0200 Subject: [PATCH 3/9] Proto: forbid a delegate to propose the same protocol hash twice This a semantic change: the new Already_proposed error is now returned when a Proposals operation contains a proposal that has already been proposed by the delegate. The goal is to prevent replaying a Proposals operation. Moreover, for the sake of consistency, if the proposals contain a duplicate element, the operation is now rejected (with the new Proposals_contain_duplicate permanent error). --- .../lib_protocol/alpha_context.mli | 18 ++- src/proto_alpha/lib_protocol/amendment.ml | 42 +++++- src/proto_alpha/lib_protocol/amendment.mli | 6 + .../lib_protocol/test/helpers/context.ml | 4 +- .../lib_protocol/test/helpers/context.mli | 5 +- .../integration/operations/test_voting.ml | 136 ++++++++++-------- .../lib_protocol/validate_errors.ml | 33 +++++ .../lib_protocol/validate_errors.mli | 2 + src/proto_alpha/lib_protocol/vote_storage.ml | 14 +- src/proto_alpha/lib_protocol/vote_storage.mli | 38 ++++- 10 files changed, 211 insertions(+), 87 deletions(-) diff --git a/src/proto_alpha/lib_protocol/alpha_context.mli b/src/proto_alpha/lib_protocol/alpha_context.mli index cf70be6a5047..fc175800b670 100644 --- a/src/proto_alpha/lib_protocol/alpha_context.mli +++ b/src/proto_alpha/lib_protocol/alpha_context.mli @@ -2582,16 +2582,24 @@ end module Vote : sig type proposal = Protocol_hash.t - val record_proposal : - context -> Protocol_hash.t -> public_key_hash -> context tzresult Lwt.t + (** See {!Vote_storage.get_delegate_proposal_count}. *) + val get_delegate_proposal_count : + context -> public_key_hash -> int tzresult Lwt.t + + (** See {!Vote_storage.set_delegate_proposal_count}. *) + val set_delegate_proposal_count : + context -> public_key_hash -> int -> context Lwt.t + + (** See {!Vote_storage.has_proposed}. *) + val has_proposed : context -> public_key_hash -> proposal -> bool Lwt.t + + (** See {!Vote_storage.add_proposal}. *) + val add_proposal : context -> public_key_hash -> proposal -> context Lwt.t val get_proposals : context -> int64 Protocol_hash.Map.t tzresult Lwt.t val clear_proposals : context -> context Lwt.t - val recorded_proposal_count_for_delegate : - context -> public_key_hash -> int tzresult Lwt.t - val listings_encoding : (Signature.Public_key_hash.t * int64) list Data_encoding.t diff --git a/src/proto_alpha/lib_protocol/amendment.ml b/src/proto_alpha/lib_protocol/amendment.ml index 6735708513cf..424e37e687f8 100644 --- a/src/proto_alpha/lib_protocol/amendment.ml +++ b/src/proto_alpha/lib_protocol/amendment.ml @@ -152,17 +152,33 @@ let may_start_new_voting_period ctxt = open Validate_errors.Voting +let check_no_duplicates proposals = + let open Tzresult_syntax in + let* (_ : Protocol_hash.Set.t) = + List.fold_left_e + (fun previous_proposals proposal -> + let* () = + error_when + (Protocol_hash.Set.mem proposal previous_proposals) + (Proposals_contain_duplicate {proposal}) + in + return (Protocol_hash.Set.add proposal previous_proposals)) + Protocol_hash.Set.empty + proposals + in + return_unit + let record_delegate_proposals ctxt delegate proposals = (match proposals with | [] -> error Empty_proposals | _ :: _ -> Result.return_unit) >>?= fun () -> + check_no_duplicates proposals >>?= fun () -> Voting_period.get_current_kind ctxt >>=? function | Proposal -> Vote.in_listings ctxt delegate >>= fun in_listings -> if in_listings then ( - Vote.recorded_proposal_count_for_delegate ctxt delegate - >>=? fun count -> + Vote.get_delegate_proposal_count ctxt delegate >>=? fun count -> assert (Compare.Int.(Constants.max_proposals_per_delegate >= count)) ; error_when Compare.Int.( @@ -172,10 +188,24 @@ let record_delegate_proposals ctxt delegate proposals = > 0) Too_many_proposals >>?= fun () -> - List.fold_left_es - (fun ctxt proposal -> Vote.record_proposal ctxt proposal delegate) - ctxt - proposals) + (* The mix of syntactic styles will go away in the next commit *) + let open Lwt_tzresult_syntax in + let* ctxt, count = + List.fold_left_es + (fun (ctxt, count) proposal -> + let* () = + let*! already_proposed = + Vote.has_proposed ctxt delegate proposal + in + fail_when already_proposed (Already_proposed {proposal}) + in + let*! ctxt = Vote.add_proposal ctxt delegate proposal in + return (ctxt, count + 1)) + (ctxt, count) + proposals + in + let*! ctxt = Vote.set_delegate_proposal_count ctxt delegate count in + return ctxt) else fail Source_not_in_vote_listings | (Exploration | Cooldown | Promotion | Adoption) as current -> fail (Wrong_voting_period_kind {current; expected = [Proposal]}) diff --git a/src/proto_alpha/lib_protocol/amendment.mli b/src/proto_alpha/lib_protocol/amendment.mli index fd95be5d99c6..54b298d2768c 100644 --- a/src/proto_alpha/lib_protocol/amendment.mli +++ b/src/proto_alpha/lib_protocol/amendment.mli @@ -101,6 +101,9 @@ val may_start_new_voting_period : context -> context tzresult Lwt.t @return [Error Empty_proposals] if the list of proposals is empty. + @return [Error Proposals_contain_duplicate] if the list of + proposals contains a duplicate element. + @return [Error Wrong_voting_period_kind] if the voting period is not of the Proposal kind. @@ -111,6 +114,9 @@ val may_start_new_voting_period : context -> context tzresult Lwt.t source's total number of proposals exceed {!Constants.recorded_proposal_count_for_delegate}. + @return [Error Already_proposed] if one of the proposals has + already been proposed by the source. + @return [Error Testnet_dictator_multiple_proposals] if the source is a testnet dictator and the operation contains more than one proposal. *) diff --git a/src/proto_alpha/lib_protocol/test/helpers/context.ml b/src/proto_alpha/lib_protocol/test/helpers/context.ml index b102aa4a31ed..822d166f16d5 100644 --- a/src/proto_alpha/lib_protocol/test/helpers/context.ml +++ b/src/proto_alpha/lib_protocol/test/helpers/context.ml @@ -265,9 +265,9 @@ module Vote = struct remaining_proposals : int; } - let recorded_proposal_count_for_delegate ctxt pkh = + let get_delegate_proposal_count ctxt pkh = to_alpha_ctxt ctxt >>=? fun alpha_ctxt -> - Vote.recorded_proposal_count_for_delegate alpha_ctxt pkh + Vote.get_delegate_proposal_count alpha_ctxt pkh >|= Environment.wrap_tzresult end diff --git a/src/proto_alpha/lib_protocol/test/helpers/context.mli b/src/proto_alpha/lib_protocol/test/helpers/context.mli index 66fed3b9e5c5..071960e5a759 100644 --- a/src/proto_alpha/lib_protocol/test/helpers/context.mli +++ b/src/proto_alpha/lib_protocol/test/helpers/context.mli @@ -124,12 +124,11 @@ module Vote : sig remaining_proposals : int; } - (** See {!Vote_storage.recorded_proposal_count_for_delegate}. + (** See {!Vote_storage.get_delegate_proposal_count}. Note that unlike most functions in the current module, this one does not call an RPC. *) - val recorded_proposal_count_for_delegate : - t -> public_key_hash -> int tzresult Lwt.t + val get_delegate_proposal_count : t -> public_key_hash -> int tzresult Lwt.t end module Contract : sig diff --git a/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml b/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml index 809b9608b1d5..91d4acd1dd27 100644 --- a/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml +++ b/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml @@ -348,10 +348,26 @@ let empty_proposals loc = function | [Environment.Ecoproto_error Empty_proposals] -> return_unit | err -> wrong_error "Empty_proposals" err loc +let proposals_contain_duplicate duplicate_proposal loc = function + | [Environment.Ecoproto_error (Proposals_contain_duplicate {proposal})] -> + Assert.equal_protocol_hash + ~loc:(append_loc ~caller_loc:loc __LOC__) + proposal + duplicate_proposal + | err -> wrong_error "Proposals_contain_duplicate" err loc + let too_many_proposals loc = function | [Environment.Ecoproto_error Too_many_proposals] -> return_unit | err -> wrong_error "Too_many_proposals" err loc +let already_proposed already_proposed_proposal loc = function + | [Environment.Ecoproto_error (Already_proposed {proposal})] -> + Assert.equal_protocol_hash + ~loc:(append_loc ~caller_loc:loc __LOC__) + proposal + already_proposed_proposal + | err -> wrong_error "Already_proposed" err loc + let ballot_for_wrong_proposal ~current_proposal ~op_proposal loc = function | [ Environment.Ecoproto_error (Ballot_for_wrong_proposal {current; submitted}); @@ -810,37 +826,6 @@ let test_not_enough_quorum_in_promotion num_delegates () = assert_period ~expected_kind:Proposal b __LOC__ >>=? fun () -> assert_listings_not_empty b ~loc:__LOC__ >>=? fun () -> return_unit -(** Identical proposals (identified by their hash) must be counted as - one. *) -let test_multiple_identical_proposals_count_as_one () = - context_init 1 () >>=? fun (b, delegates) -> - assert_period ~expected_kind:Proposal b __LOC__ >>=? fun () -> - let proposer = WithExceptions.Option.get ~loc:__LOC__ @@ List.hd delegates in - Op.proposals (B b) proposer [Protocol_hash.zero; Protocol_hash.zero] - >>=? fun operation -> - Block.bake ~operation b >>=? fun b -> - (* compute the weight of proposals *) - Context.Vote.get_proposals (B b) >>=? fun ps -> - (* compute the voting power of proposer *) - let pkh = Context.Contract.pkh proposer in - Context.Vote.get_listings (B b) >>=? fun l -> - (match List.find_opt (fun (del, _) -> del = pkh) l with - | None -> failwith "%s - Missing delegate" __LOC__ - | Some (_, proposer_power) -> return proposer_power) - >>=? fun proposer_power -> - (* correctly count the double proposal for zero as one proposal *) - let expected_weight_proposer = proposer_power in - match Environment.Protocol_hash.(Map.find zero ps) with - | Some v -> - if v = expected_weight_proposer then return_unit - else - failwith - "%s - Wrong count %Ld is not %Ld; identical proposals count as one" - __LOC__ - v - expected_weight_proposer - | None -> failwith "%s - Missing proposal" __LOC__ - (** Assume the initial balance of accounts allocated by Context.init_n is at least 4 times the value of the minimal_stake constant. *) let test_supermajority_in_proposal there_is_a_winner () = @@ -1369,6 +1354,18 @@ let test_empty_proposals () = block __LOC__ +(** Test that a Proposals operation fails when its proposal list + contains multiple occurrences of the same proposal. *) +let test_proposals_contain_duplicate () = + let open Lwt_result_syntax in + let* block, proposer = context_init1 () in + assert_validate_proposals_fails + ~expected_error:(proposals_contain_duplicate protos.(1)) + ~proposer + ~proposals:[protos.(0); protos.(1); protos.(2); protos.(1); protos.(3)] + block + __LOC__ + (** Test that a Proposals operation fails when its proposal list is longer than the [max_proposals_per_delegate] protocol constant. *) let test_operation_has_too_many_proposals () = @@ -1404,6 +1401,45 @@ let test_too_many_proposals () = block __LOC__ +(** Test that a Proposals operation fails when one of its proposals has + already been submitted by the same proposer in an earlier block. *) +let test_already_proposed () = + let open Lwt_result_syntax in + let* block, proposer = context_init1 () in + let* operation = Op.proposals (B block) proposer [protos.(0); protos.(1)] in + let* block = Block.bake block ~operation in + (* The [proposer] cannot submit protocol [0] again. *) + let* () = + assert_validate_proposals_fails + ~expected_error:(already_proposed protos.(0)) + ~proposer + ~proposals:[protos.(0)] + block + __LOC__ + in + (* The [proposer] cannot submit protocol [1] again, even among other + new proposals. *) + let* () = + assert_validate_proposals_fails + ~expected_error:(already_proposed protos.(1)) + ~proposer + ~proposals:[protos.(2); protos.(1); protos.(3)] + block + __LOC__ + in + (* The initial [operation] cannot be replayed. *) + let* () = + Incremental.assert_validate_operation_fails + (already_proposed protos.(0) __LOC__) + operation + block + in + let* block = bake_until_first_block_of_next_period block in + Incremental.assert_validate_operation_fails + (wrong_voting_period_index ~current_index:1l ~op_index:0l __LOC__) + operation + block + (** {3 Proposals -- Positive test} A Proposals operation is valid when: @@ -1491,10 +1527,10 @@ let observe_proposals pre_state post_state op caller_loc = proposals) ; (* Check [Storage.Vote.Proposals_count] update. *) let* proposal_count_pre = - Context.Vote.recorded_proposal_count_for_delegate (B pre_state) source + Context.Vote.get_delegate_proposal_count (B pre_state) source in let* proposal_count_post = - Context.Vote.recorded_proposal_count_for_delegate (B post_state) source + Context.Vote.get_delegate_proposal_count (B post_state) source in let* () = Assert.equal_int @@ -1554,27 +1590,6 @@ let test_valid_proposals () = let* b4 = Block.bake b3 ~operation:op3 in observe_proposals b3 b4 op3 __LOC__ -(** {3 Proposals -- Incoming semantic change} *) - -(** Test that a Proposals operation can be replayed - (this will no longer be true in an upcoming commit). *) -let test_replay_proposals () = - let open Lwt_result_syntax in - let* bpre, proposer = context_init1 ~blocks_per_cycle:10l () in - let* operation = Op.proposals (B bpre) proposer [protos.(0)] in - let* bpost = Block.bake bpre ~operation in - let* () = observe_proposals bpre bpost operation __LOC__ in - (* We do not observe the effects of replayed operations because they - are different from fresh operations: since the proposals were - already recorded for the proposer, - [voting_infos.remaining_proposals] does not decrease, nor does - the total supporting weight increase. - - We simply check that [Block.bake] does not fail. *) - let* bpost = Block.bake bpost ~operation in - let* _bpost = Block.bake bpost ~operations:[operation; operation] in - return_unit - (** {3 Ballot -- Negative tests} *) (** Test that a Ballot operation fails when its source is not a @@ -1848,10 +1863,6 @@ let tests = "voting promotion, not enough quorum" `Quick (test_not_enough_quorum_in_promotion 432); - Tztest.tztest - "voting counting double proposal" - `Quick - test_multiple_identical_proposals_count_as_one; Tztest.tztest "voting proposal, with supermajority" `Quick @@ -1919,6 +1930,10 @@ let tests = `Quick test_proposals_source_not_in_vote_listings; Tztest.tztest "Empty proposals" `Quick test_empty_proposals; + Tztest.tztest + "Proposals contain a duplicate proposal" + `Quick + test_proposals_contain_duplicate; Tztest.tztest "Operation has too many proposals" `Quick @@ -1927,8 +1942,11 @@ let tests = "Too many proposals (over two operations)" `Quick test_too_many_proposals; + Tztest.tztest + "A proposal had already been proposed" + `Quick + test_already_proposed; Tztest.tztest "Valid Proposals operations" `Quick test_valid_proposals; - Tztest.tztest "Replay proposals" `Quick test_replay_proposals; (* Validity tests on Ballot *) Tztest.tztest "Ballot from unregistered delegate" diff --git a/src/proto_alpha/lib_protocol/validate_errors.ml b/src/proto_alpha/lib_protocol/validate_errors.ml index 11fade8de448..d50ee0a2557e 100644 --- a/src/proto_alpha/lib_protocol/validate_errors.ml +++ b/src/proto_alpha/lib_protocol/validate_errors.ml @@ -394,7 +394,9 @@ module Voting = struct | Source_not_in_vote_listings | (* Proposals errors *) Empty_proposals + | Proposals_contain_duplicate of {proposal : Protocol_hash.t} | Too_many_proposals + | Already_proposed of {proposal : Protocol_hash.t} | Testnet_dictator_multiple_proposals | (* Ballot errors *) Ballot_for_wrong_proposal of { @@ -475,6 +477,22 @@ module Voting = struct Data_encoding.empty (function Empty_proposals -> Some () | _ -> None) (fun () -> Empty_proposals) ; + register_error_kind + `Permanent + ~id:"validate_operation.proposals_contain_duplicate" + ~title:"Proposals contain duplicate" + ~description:"The list of proposals contains a duplicate element." + ~pp:(fun ppf proposal -> + Format.fprintf + ppf + "The list of proposals contains multiple occurrences of the proposal \ + %a." + Protocol_hash.pp + proposal) + Data_encoding.(obj1 (req "proposal" Protocol_hash.encoding)) + (function + | Proposals_contain_duplicate {proposal} -> Some proposal | _ -> None) + (fun proposal -> Proposals_contain_duplicate {proposal}) ; let description = "The proposer exceeded the maximum number of allowed proposals." in @@ -487,6 +505,21 @@ module Voting = struct Data_encoding.empty (function Too_many_proposals -> Some () | _ -> None) (fun () -> Too_many_proposals) ; + register_error_kind + `Branch + ~id:"validate_operation.already_proposed" + ~title:"Already proposed" + ~description: + "The delegate has already submitted one of the operation's proposals." + ~pp:(fun ppf proposal -> + Format.fprintf + ppf + "The delegate has already submitted the proposal %a." + Protocol_hash.pp + proposal) + Data_encoding.(obj1 (req "proposal" Protocol_hash.encoding)) + (function Already_proposed {proposal} -> Some proposal | _ -> None) + (fun proposal -> Already_proposed {proposal}) ; let description = "A testnet dictator cannot submit more than one proposal at a time." in diff --git a/src/proto_alpha/lib_protocol/validate_errors.mli b/src/proto_alpha/lib_protocol/validate_errors.mli index 358a6cf5dd6a..951ff8cddcb8 100644 --- a/src/proto_alpha/lib_protocol/validate_errors.mli +++ b/src/proto_alpha/lib_protocol/validate_errors.mli @@ -96,7 +96,9 @@ module Voting : sig | Source_not_in_vote_listings | (* Proposals errors *) Empty_proposals + | Proposals_contain_duplicate of {proposal : Protocol_hash.t} | Too_many_proposals + | Already_proposed of {proposal : Protocol_hash.t} | Testnet_dictator_multiple_proposals | (* Ballot errors *) Ballot_for_wrong_proposal of { diff --git a/src/proto_alpha/lib_protocol/vote_storage.ml b/src/proto_alpha/lib_protocol/vote_storage.ml index a37403dc565c..e258d261c602 100644 --- a/src/proto_alpha/lib_protocol/vote_storage.ml +++ b/src/proto_alpha/lib_protocol/vote_storage.ml @@ -23,13 +23,17 @@ (* *) (*****************************************************************************) -let recorded_proposal_count_for_delegate ctxt proposer = +let get_delegate_proposal_count ctxt proposer = Storage.Vote.Proposals_count.find ctxt proposer >|=? Option.value ~default:0 -let record_proposal ctxt proposal proposer = - recorded_proposal_count_for_delegate ctxt proposer >>=? fun count -> - Storage.Vote.Proposals_count.add ctxt proposer (count + 1) >>= fun ctxt -> - Storage.Vote.Proposals.add ctxt (proposal, proposer) >|= ok +let set_delegate_proposal_count ctxt proposer count = + Storage.Vote.Proposals_count.add ctxt proposer count + +let has_proposed ctxt proposer proposal = + Storage.Vote.Proposals.mem ctxt (proposal, proposer) + +let add_proposal ctxt proposer proposal = + Storage.Vote.Proposals.add ctxt (proposal, proposer) let get_proposals ctxt = Storage.Vote.Proposals.fold diff --git a/src/proto_alpha/lib_protocol/vote_storage.mli b/src/proto_alpha/lib_protocol/vote_storage.mli index b814c8a54d9a..f52c3bd18cc0 100644 --- a/src/proto_alpha/lib_protocol/vote_storage.mli +++ b/src/proto_alpha/lib_protocol/vote_storage.mli @@ -26,15 +26,39 @@ (** Manages all the voting related storage in Storage.Vote. *) -(** Records a protocol proposal with the delegate that proposed it. *) -val record_proposal : +(** [get_delegate_proposal_count ctxt proposer] returns the number of + proposals already made by [proposer] in the current voting cycle. + + This number of proposals, aka [count], has its own storage bucket. + + @return [0] if the [count] of the proposer was not initialized. + + @return [Error Storage_error] if the deserialization of [count] + fails. *) +val get_delegate_proposal_count : + Raw_context.t -> Signature.public_key_hash -> int tzresult Lwt.t + +(** [set_delegate_proposal_count ctxt proposer count] sets + [proposer]'s number of submitted proposals to [count]. + + More precisely, the relevant storage bucket is allocated and + initialized to [count] if it didn't exist; otherwise it is simply + updated. *) +val set_delegate_proposal_count : + Raw_context.t -> Signature.public_key_hash -> int -> Raw_context.t Lwt.t + +(** [has_proposed ctxt proposer proposal] indicates whether the + [proposer] has already proposed the [proposal]. *) +val has_proposed : + Raw_context.t -> Signature.public_key_hash -> Protocol_hash.t -> bool Lwt.t + +(** [add_proposal ctxt proposer proposal] records the submission of + [proposal] by [proposer]. *) +val add_proposal : Raw_context.t -> + Signature.public_key_hash -> Protocol_hash.t -> - Signature.Public_key_hash.t -> - Raw_context.t tzresult Lwt.t - -val recorded_proposal_count_for_delegate : - Raw_context.t -> Signature.Public_key_hash.t -> int tzresult Lwt.t + Raw_context.t Lwt.t (** Computes for each proposal how many delegates proposed it. *) val get_proposals : Raw_context.t -> int64 Protocol_hash.Map.t tzresult Lwt.t -- GitLab From 1d174ba84a82845008f44fcad920d025480eed64 Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Thu, 7 Jul 2022 18:03:31 +0200 Subject: [PATCH 4/9] Proto: split validate_proposals from apply_proposals --- .../lib_protocol/alpha_context.mli | 10 +- src/proto_alpha/lib_protocol/amendment.ml | 252 ++++++++++++------ src/proto_alpha/lib_protocol/amendment.mli | 13 +- .../integration/operations/test_voting.ml | 53 ++-- src/proto_alpha/lib_protocol/vote_storage.ml | 5 +- src/proto_alpha/lib_protocol/vote_storage.mli | 20 +- .../lib_protocol/voting_period_storage.mli | 9 +- 7 files changed, 234 insertions(+), 128 deletions(-) diff --git a/src/proto_alpha/lib_protocol/alpha_context.mli b/src/proto_alpha/lib_protocol/alpha_context.mli index fc175800b670..d7dfee4067b7 100644 --- a/src/proto_alpha/lib_protocol/alpha_context.mli +++ b/src/proto_alpha/lib_protocol/alpha_context.mli @@ -2573,6 +2573,7 @@ module Voting_period : sig val get_rpc_succ_info : context -> info tzresult Lwt.t module Testnet_dictator : sig + (** See {!Voting_period_storage.Testnet_dictator.overwrite_current_kind}. *) val overwrite_current_kind : context -> Chain_id.t -> Voting_period_repr.kind -> context tzresult Lwt.t end @@ -2672,13 +2673,20 @@ module Vote : sig val set_participation_ema : context -> int32 -> context tzresult Lwt.t + (** See {!Vote_storage.current_proposal_exists}. *) + val current_proposal_exists : context -> bool Lwt.t + + (** See {!Vote_storage.get_current_proposal}. *) val get_current_proposal : context -> proposal tzresult Lwt.t + (** See {!Vote_storage.find_current_proposal}. *) val find_current_proposal : context -> proposal option tzresult Lwt.t + (** See {!Vote_storage.init_current_proposal}. *) val init_current_proposal : context -> proposal -> context tzresult Lwt.t - val clear_current_proposal : context -> context tzresult Lwt.t + (** See {!Vote_storage.clear_current_proposal}. *) + val clear_current_proposal : context -> context Lwt.t end module Dal : sig diff --git a/src/proto_alpha/lib_protocol/amendment.ml b/src/proto_alpha/lib_protocol/amendment.ml index 424e37e687f8..1d79d167e35e 100644 --- a/src/proto_alpha/lib_protocol/amendment.ml +++ b/src/proto_alpha/lib_protocol/amendment.ml @@ -130,18 +130,18 @@ let start_new_voting_period ctxt = >>=? fun (ctxt, approved) -> if approved then Voting_period.succ ctxt else - Vote.clear_current_proposal ctxt >>=? fun ctxt -> + Vote.clear_current_proposal ctxt >>= fun ctxt -> Voting_period.reset ctxt | Cooldown -> Voting_period.succ ctxt | Promotion -> get_approval_and_update_participation_ema ctxt >>=? fun (ctxt, approved) -> if approved then Voting_period.succ ctxt - else Vote.clear_current_proposal ctxt >>=? Voting_period.reset + else Vote.clear_current_proposal ctxt >>= Voting_period.reset | Adoption -> Vote.get_current_proposal ctxt >>=? fun proposal -> activate ctxt proposal >>= fun ctxt -> - Vote.clear_current_proposal ctxt >>=? Voting_period.reset) + Vote.clear_current_proposal ctxt >>= Voting_period.reset) >>=? fun ctxt -> Vote.update_listings ctxt let may_start_new_voting_period ctxt = @@ -152,107 +152,191 @@ let may_start_new_voting_period ctxt = open Validate_errors.Voting -let check_no_duplicates proposals = +(** Helpers to validate and apply Proposals operations from a + registered dictator of a test chain. These operations let the + dictator immediately change the current voting period's kind, and + the current proposal if applicable. Of course, there must never be + such a dictator on mainnet. *) +module Testnet_dictator = struct + let is_testnet_dictator ctxt chain_id delegate = + (* This function should always, ALWAYS, return false on mainnet!!!! *) + match Constants.testnet_dictator ctxt with + | Some pkh when Chain_id.(chain_id <> Constants.mainnet_id) -> + Signature.Public_key_hash.equal pkh delegate + | _ -> false + + (** Check that [record_proposals] below will not fail. + + This function is designed to be exclusively called by + [validate_proposals] further down this file. + + @return [Error Multiple_proposals] if [proposals] has more than + one element. *) + let check_proposals chain_id proposals = + (* This assertion should be ensured by the fact that + {!is_testnet_dictator} cannot be [true] on mainnet, but we + double check it because it is critical. *) + assert (Chain_id.(chain_id <> Constants.mainnet_id)) ; + match proposals with + | [] | [_] -> + (* In [record_proposals] below, the call to + {!Vote.init_current_proposal} (in the singleton list case) + cannot fail because {!Vote.clear_current_proposal} is called + right before. + + The calls to + {!Voting_period.Testnet_dictator.overwrite_current_kind} may + usually fail when the voting period is not + initialized. However, this cannot happen because the current + function is only called in [validate_proposals] after a + successful call to {!Voting_period.get_current}. *) + ok () + | _ :: _ :: _ -> error Testnet_dictator_multiple_proposals + + (** Forcibly update the voting period according to a voting + dictator's Proposals operation. + + {!check_proposals} should guarantee that this function cannot + return an error. *) + let record_proposals ctxt chain_id proposals = + let open Lwt_tzresult_syntax in + let*! ctxt = Vote.clear_ballots ctxt in + let*! ctxt = Vote.clear_proposals ctxt in + let*! ctxt = Vote.clear_current_proposal ctxt in + match proposals with + | [] -> + Voting_period.Testnet_dictator.overwrite_current_kind + ctxt + chain_id + Proposal + | [proposal] -> + let* ctxt = Vote.init_current_proposal ctxt proposal in + Voting_period.Testnet_dictator.overwrite_current_kind + ctxt + chain_id + Adoption + | _ :: _ :: _ -> fail Testnet_dictator_multiple_proposals +end + +let check_period_index ~expected period_index = + error_unless + Compare.Int32.(expected = period_index) + (Wrong_voting_period_index {expected; provided = period_index}) + +(** Check that the list of proposals is not empty and does not contain + duplicates. *) +let check_proposal_list_sanity proposals = let open Tzresult_syntax in + let* () = + match proposals with [] -> error Empty_proposals | _ :: _ -> ok () + in let* (_ : Protocol_hash.Set.t) = List.fold_left_e - (fun previous_proposals proposal -> + (fun previous_elements proposal -> let* () = error_when - (Protocol_hash.Set.mem proposal previous_proposals) + (Protocol_hash.Set.mem proposal previous_elements) (Proposals_contain_duplicate {proposal}) in - return (Protocol_hash.Set.add proposal previous_proposals)) + return (Protocol_hash.Set.add proposal previous_elements)) Protocol_hash.Set.empty proposals in return_unit -let record_delegate_proposals ctxt delegate proposals = - (match proposals with - | [] -> error Empty_proposals - | _ :: _ -> Result.return_unit) - >>?= fun () -> - check_no_duplicates proposals >>?= fun () -> - Voting_period.get_current_kind ctxt >>=? function - | Proposal -> - Vote.in_listings ctxt delegate >>= fun in_listings -> - if in_listings then ( - Vote.get_delegate_proposal_count ctxt delegate >>=? fun count -> - assert (Compare.Int.(Constants.max_proposals_per_delegate >= count)) ; - error_when - Compare.Int.( - List.compare_length_with - proposals - (Constants.max_proposals_per_delegate - count) - > 0) - Too_many_proposals - >>?= fun () -> - (* The mix of syntactic styles will go away in the next commit *) - let open Lwt_tzresult_syntax in - let* ctxt, count = - List.fold_left_es - (fun (ctxt, count) proposal -> - let* () = - let*! already_proposed = - Vote.has_proposed ctxt delegate proposal - in - fail_when already_proposed (Already_proposed {proposal}) - in - let*! ctxt = Vote.add_proposal ctxt delegate proposal in - return (ctxt, count + 1)) - (ctxt, count) - proposals - in - let*! ctxt = Vote.set_delegate_proposal_count ctxt delegate count in - return ctxt) - else fail Source_not_in_vote_listings +let check_period_kind_for_proposals current_period = + match current_period.Voting_period.kind with + | Proposal -> ok () | (Exploration | Cooldown | Promotion | Adoption) as current -> - fail (Wrong_voting_period_kind {current; expected = [Proposal]}) + error (Wrong_voting_period_kind {current; expected = [Proposal]}) -let record_testnet_dictator_proposals ctxt chain_id proposals = - Vote.clear_ballots ctxt >>= fun ctxt -> - Vote.clear_proposals ctxt >>= fun ctxt -> - Vote.clear_current_proposal ctxt >>=? fun ctxt -> - match proposals with - | [] -> - Voting_period.Testnet_dictator.overwrite_current_kind - ctxt - chain_id - Proposal - | [proposal] -> - Vote.init_current_proposal ctxt proposal >>=? fun ctxt -> - Voting_period.Testnet_dictator.overwrite_current_kind - ctxt - chain_id - Adoption - | _ :: _ :: _ -> fail Testnet_dictator_multiple_proposals +let check_in_listings ctxt source = + let open Lwt_tzresult_syntax in + let*! in_listings = Vote.in_listings ctxt source in + fail_unless in_listings Source_not_in_vote_listings -let is_testnet_dictator ctxt chain_id delegate = - (* This function should always, ALWAYS, return false on mainnet!!!! *) - match Constants.testnet_dictator ctxt with - | Some pkh when Chain_id.(chain_id <> Constants_repr.mainnet_id) -> - Signature.Public_key_hash.equal pkh delegate - | _ -> false +let check_count ctxt proposer proposals = + let open Lwt_tzresult_syntax in + let* count = Vote.get_delegate_proposal_count ctxt proposer in + (* [count] should never have been increased above + [max_proposals_per_delegate]. *) + assert (Compare.Int.(count <= Constants.max_proposals_per_delegate)) ; + fail_unless + Compare.List_length_with.( + proposals <= Constants.max_proposals_per_delegate - count) + Too_many_proposals -let record_proposals ctxt chain_id delegate proposals = - if is_testnet_dictator ctxt chain_id delegate then - record_testnet_dictator_proposals ctxt chain_id proposals - else record_delegate_proposals ctxt delegate proposals +let check_already_proposed ctxt proposer proposals = + let open Lwt_tzresult_syntax in + List.iter_es + (fun proposal -> + let*! already_proposed = Vote.has_proposed ctxt proposer proposal in + fail_when already_proposed (Already_proposed {proposal})) + proposals -let apply_proposals ctxt chain_id (operation : Kind.proposals operation) = +(* For now, this function is still called directly by + [apply_proposals], and so [ctxt] is still the context at + application time (where previous operations from the current block + have been applied). + + In the next commit, this function will be called by + [Validate_operation.validate_operation] instead (and will need to + be updated because [ctxt] will not be the same). *) +let validate_proposals ctxt chain_id ~should_check_signature + (operation : Kind.proposals operation) = + let open Lwt_tzresult_syntax in let (Single (Proposals {source; period; proposals})) = operation.protocol_data.contents in - Delegate.pubkey ctxt source >>=? fun delegate -> - Operation.check_signature delegate chain_id operation >>?= fun () -> - Voting_period.get_current ctxt >>=? fun {index = current_period; _} -> - error_unless - Compare.Int32.(current_period = period) - (Wrong_voting_period_index {expected = current_period; provided = period}) - >>?= fun () -> - record_proposals ctxt chain_id source proposals >|=? fun ctxt -> - (ctxt, Apply_results.Single_result Proposals_result) + let* current_period = Voting_period.get_current ctxt in + let*? () = check_period_index ~expected:current_period.index period in + let* () = + if Testnet_dictator.is_testnet_dictator ctxt chain_id source then + Lwt.return (Testnet_dictator.check_proposals chain_id proposals) + else + let*? () = check_proposal_list_sanity proposals in + let*? () = check_period_kind_for_proposals current_period in + let* () = check_in_listings ctxt source in + let* () = check_count ctxt source proposals in + check_already_proposed ctxt source proposals + in + (* The signature check is done last because it is more costly than + most checks. *) + when_ should_check_signature (fun () -> + (* Retrieving the public key cannot fail. Indeed, we have + already checked that the delegate is in the vote listings + (or is a testnet dictator), which implies that it is a + manager with a revealed key. *) + let* public_key = Delegate.pubkey ctxt source in + Lwt.return (Operation.check_signature public_key chain_id operation)) + +let record_proposals ctxt proposer proposals = + let open Lwt_tzresult_syntax in + let* count = Vote.get_delegate_proposal_count ctxt proposer in + let new_count = count + List.length proposals in + let*! ctxt = Vote.set_delegate_proposal_count ctxt proposer new_count in + let*! ctxt = + List.fold_left_s + (fun ctxt proposal -> Vote.add_proposal ctxt proposer proposal) + ctxt + proposals + in + return ctxt + +let apply_proposals ctxt chain_id (operation : Kind.proposals operation) = + let open Lwt_tzresult_syntax in + let* () = + validate_proposals ctxt chain_id ~should_check_signature:true operation + in + let (Single (Proposals {source; period = _; proposals})) = + operation.protocol_data.contents + in + let* ctxt = + if Testnet_dictator.is_testnet_dictator ctxt chain_id source then + Testnet_dictator.record_proposals ctxt chain_id proposals + else record_proposals ctxt source proposals + in + return (ctxt, Apply_results.Single_result Proposals_result) let record_ballot ctxt delegate proposal ballot = Voting_period.get_current_kind ctxt >>=? function diff --git a/src/proto_alpha/lib_protocol/amendment.mli b/src/proto_alpha/lib_protocol/amendment.mli index 54b298d2768c..a31abbf86fe0 100644 --- a/src/proto_alpha/lib_protocol/amendment.mli +++ b/src/proto_alpha/lib_protocol/amendment.mli @@ -88,13 +88,6 @@ val may_start_new_voting_period : context -> context tzresult Lwt.t (** Update the [context] with the effects of a Proposals operation. - @return [Error Delegate_storage.Unregistered_delegate] if the - source's contract does not exist or its public key is unreavealed. - - @return [Error Operation.Missing_signature] or [Error - Operation.Invalid_signature] if the operation is unsigned or - incorrectly signed. - @return [Error Wrong_voting_period_index] if the operation's period and the [context]'s current period do not have the same index. @@ -119,7 +112,11 @@ val may_start_new_voting_period : context -> context tzresult Lwt.t @return [Error Testnet_dictator_multiple_proposals] if the source is a testnet dictator and the operation contains more than one - proposal. *) + proposal. + + @return [Error Operation.Missing_signature] or [Error + Operation.Invalid_signature] if the operation is unsigned or + incorrectly signed. *) val apply_proposals : context -> Chain_id.t -> diff --git a/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml b/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml index 91d4acd1dd27..6cc29bab3652 100644 --- a/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml +++ b/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml @@ -1230,19 +1230,6 @@ let test_voting_period_pp () = (** {3 Proposal -- Negative tests} *) -(** Test that a Proposals operation fails when its source is not a - registered delegate. *) -let test_proposals_unregistered_delegate () = - let open Lwt_result_syntax in - let* block, _delegate = context_init1 () in - let fresh_account = Account.new_account () in - assert_validate_proposals_fails - ~expected_error:(unregistered_delegate fresh_account.pkh) - ~proposer:(Contract.Implicit fresh_account.pkh) - ~proposals:[Protocol_hash.zero] - block - __LOC__ - (** Test that a Proposals operation fails when it is unsigned. *) let test_proposals_missing_signature () = let open Lwt_result_syntax in @@ -1323,24 +1310,34 @@ let test_proposals_wrong_voting_period_kind () = assert_proposals_fails_with_unexpected_proposal block __LOC__ (** Test that a Proposals operation fails when the proposer is not in - the vote listings. *) + the vote listings (with the same error, no matter how far the + source is from being a delegate with voting rights). *) let test_proposals_source_not_in_vote_listings () = let open Lwt_result_syntax in - let* block, funder = context_init1 () in - let account = Account.new_account () in - let proposer = Contract.Implicit account.pkh in + let* block, funder = context_init1 ~blocks_per_cycle:10l () in + let fresh_account = Account.new_account () in + let proposer = Contract.Implicit fresh_account.pkh in + let assert_fails_with_source_not_in_vote_listings block = + assert_validate_proposals_fails + ~expected_error:source_not_in_vote_listings + ~proposer + ~proposals:[Protocol_hash.zero] + block + in + (* Fail when the source has no contract in the storage. *) + let* () = assert_fails_with_source_not_in_vote_listings block __LOC__ in let* operation = Op.transaction (B block) funder proposer Tez.one in let* block = Block.bake block ~operation in - let* operation = - Op.delegation ~force_reveal:true (B block) proposer (Some account.pkh) - in + (* Fail when the contract's public key is unreavealed. *) + let* () = assert_fails_with_source_not_in_vote_listings block __LOC__ in + let* operation = Op.revelation (B block) fresh_account.pk in let* block = Block.bake block ~operation in - assert_validate_proposals_fails - ~expected_error:source_not_in_vote_listings - ~proposer - ~proposals:[Protocol_hash.zero] - block - __LOC__ + (* Fail when the source is not a delegate. *) + let* () = assert_fails_with_source_not_in_vote_listings block __LOC__ in + let* operation = Op.delegation (B block) proposer (Some fresh_account.pkh) in + let* block = Block.bake block ~operation in + (* Fail when the source is a delegate, but not yet in the vote listings. *) + assert_fails_with_source_not_in_vote_listings block __LOC__ (** Test that a Proposals operation fails when its proposal list is empty. *) @@ -1905,10 +1902,6 @@ let tests = test_voting_power_updated_each_voting_period; Tztest.tztest "voting period pretty print" `Quick test_voting_period_pp; (* Validity tests on Proposals *) - Tztest.tztest - "Proposals from unregistered delegate" - `Quick - test_proposals_unregistered_delegate; Tztest.tztest "Proposals missing signature" `Quick diff --git a/src/proto_alpha/lib_protocol/vote_storage.ml b/src/proto_alpha/lib_protocol/vote_storage.ml index e258d261c602..f2d110f8844b 100644 --- a/src/proto_alpha/lib_protocol/vote_storage.ml +++ b/src/proto_alpha/lib_protocol/vote_storage.ml @@ -252,14 +252,15 @@ let get_participation_ema = Storage.Vote.Participation_ema.get let set_participation_ema = Storage.Vote.Participation_ema.update +let current_proposal_exists = Storage.Vote.Current_proposal.mem + let get_current_proposal = Storage.Vote.Current_proposal.get let find_current_proposal = Storage.Vote.Current_proposal.find let init_current_proposal = Storage.Vote.Current_proposal.init -let clear_current_proposal ctxt = - Storage.Vote.Current_proposal.remove ctxt >|= ok +let clear_current_proposal = Storage.Vote.Current_proposal.remove let init ctxt ~start_position = (* participation EMA is in centile of a percentage *) diff --git a/src/proto_alpha/lib_protocol/vote_storage.mli b/src/proto_alpha/lib_protocol/vote_storage.mli index f52c3bd18cc0..69d0619f781a 100644 --- a/src/proto_alpha/lib_protocol/vote_storage.mli +++ b/src/proto_alpha/lib_protocol/vote_storage.mli @@ -151,15 +151,33 @@ val get_participation_ema : Raw_context.t -> int32 tzresult Lwt.t val set_participation_ema : Raw_context.t -> int32 -> Raw_context.t tzresult Lwt.t +(** Indicates whether there is a current proposal in the storage. *) +val current_proposal_exists : Raw_context.t -> bool Lwt.t + +(** Retrieves the current proposal. + + @return [Error Storage_error] if there is no current proposal, or + if the deserialization fails. *) val get_current_proposal : Raw_context.t -> Protocol_hash.t tzresult Lwt.t +(** Retrieves the current proposal. + + @return [None] if there is no current proposal. + + @return [Error Storage_error] if the deserialization fails. *) val find_current_proposal : Raw_context.t -> Protocol_hash.t option tzresult Lwt.t +(** Registers a current proposal. + + @return [Error (Storage_error Existing_key)] if there was already + a current proposal. *) val init_current_proposal : Raw_context.t -> Protocol_hash.t -> Raw_context.t tzresult Lwt.t -val clear_current_proposal : Raw_context.t -> Raw_context.t tzresult Lwt.t +(** Removes the current proposal. Does nothing if there was no current + proposal. *) +val clear_current_proposal : Raw_context.t -> Raw_context.t Lwt.t (** Sets the initial quorum to 80% and period kind to proposal. *) val init : diff --git a/src/proto_alpha/lib_protocol/voting_period_storage.mli b/src/proto_alpha/lib_protocol/voting_period_storage.mli index 06a6997aa1db..2d8a4ea2d8a1 100644 --- a/src/proto_alpha/lib_protocol/voting_period_storage.mli +++ b/src/proto_alpha/lib_protocol/voting_period_storage.mli @@ -61,8 +61,13 @@ val get_rpc_current_info : val get_rpc_succ_info : Raw_context.t -> Voting_period_repr.info tzresult Lwt.t module Testnet_dictator : sig - (** Overwrites the kind of the current voting period WITHOUT incrementing the index. - Must ONLY be called by the testnet dictator on a testnet. *) + (** Overwrites the kind of the current voting period WITHOUT + incrementing the index. + + Must ONLY be called by the testnet dictator on a testnet. + + @return [Error Storage_error] if the current voting period is + not set or its deserialization fails. *) val overwrite_current_kind : Raw_context.t -> Chain_id.t -> -- GitLab From 7dfaa3110b8d15a80aefbaaeda8021a42844cf3d Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Tue, 12 Jul 2022 18:53:25 +0200 Subject: [PATCH 5/9] Proto: handle proposals in validate_operation --- src/proto_alpha/lib_protocol/amendment.ml | 253 ++++++++++++++---- src/proto_alpha/lib_protocol/amendment.mli | 64 ++++- src/proto_alpha/lib_protocol/apply.ml | 3 +- .../integration/operations/test_voting.ml | 79 ++++++ .../lib_protocol/validate_errors.ml | 133 +++++++++ .../lib_protocol/validate_errors.mli | 13 + .../lib_protocol/validate_operation.ml | 25 +- 7 files changed, 516 insertions(+), 54 deletions(-) diff --git a/src/proto_alpha/lib_protocol/amendment.ml b/src/proto_alpha/lib_protocol/amendment.ml index 1d79d167e35e..8af0b9c077c0 100644 --- a/src/proto_alpha/lib_protocol/amendment.ml +++ b/src/proto_alpha/lib_protocol/amendment.ml @@ -152,6 +152,165 @@ let may_start_new_voting_period ctxt = open Validate_errors.Voting +(** This state is maintained in memory during the validation of a + block, or until a change of head block in mempool mode. This + state's purpose is to detect potential conflicts between the new + voting operation to validate, and all the voting operations that + have already been validated in the current block or mempool. *) +module Validation_state = struct + (** Summary of previously validated Proposals operations by a given + proposer in the current block/mempool. *) + type proposer_history = { + count : int; + (** Total number of protocols submitted by the proposer in + previously validated operations. *) + operations : Operation_hash.t list; + (** Hashes of the previously validated Proposals operations from + the proposer. *) + proposed : Operation_hash.t Protocol_hash.Map.t; + (** A map indexed by the protocols that have been submitted by the + proposer in previously validated operations. Each protocol + points to the operation in which it was proposed. *) + } + + type t = { + proposals_validated : proposer_history Signature.Public_key_hash.Map.t; + (** Summary of all Proposals operations validated in the current + block/mempool, indexed by the operation's source aka + proposer. *) + dictator_proposals_validated : Operation_hash.t option; + (** If a testnet dictotor Proposals operation has been validated + in the current block/mempool, then its hash is recorded + here. Since such an operation can change the voting period + kind, it is mutually exclusive with any other voting operation + in a single block (otherwise we would loose the commutativity + of validated operation application: see + {!Validate_operation}). *) + } + + let empty = + { + proposals_validated = Signature.Public_key_hash.Map.empty; + dictator_proposals_validated = None; + } + + let check_count_conflict ~count_previous_blocks ~count_operation + proposer_history = + let max_allowed = Constants.max_proposals_per_delegate in + let count_before_op = count_previous_blocks + proposer_history.count in + (* [count_before_op] should never have been increased above + [max_proposals_per_delegate]. *) + assert (Compare.Int.(count_before_op <= max_allowed)) ; + error_unless + Compare.Int.(count_before_op + count_operation <= max_allowed) + (Conflict_too_many_proposals + { + max_allowed; + count_previous_blocks; + count_current_block = proposer_history.count; + count_operation; + conflicting_operations = proposer_history.operations; + }) + + (** Check that a regular (ie. non-dictator) Proposals operation is + compatible with previously validated voting operations in the + current block/mempool, and update the [state] with this + operation. + + @return [Error Conflict_too_many_proposals] if the total number + of proposals by [proposer] in previously applied operations in + [ctxt], in previously validated operations in the current + block/mempool, and in the operation to validate, exceeds + {!Constants.max_proposals_per_delegate}. + + @return [Error Conflict_already_proposed] if one of the + operation's [proposals] has already been submitted by [proposer] + in the current block/mempool. + + @return [Error Conflicting_dictator_proposals] if the current + block/mempool already contains a testnet dictator Proposals + operation (see {!recfield:dictator_proposals_validated}). + + Note that this function is designed to be called in addition to + {!check_proposal_list_sanity} and {!check_count} further below, + not instead of them: that's why nothing is done when the + [proposer] is not in {!recfield:proposals_validated}. More + precisely, this function should be called {e after} the + aforementioned functions, whose potential errors + e.g. [Proposals_contain_duplicate] or [Too_many_proposals] should + take precedence because they are independent from the validation + [state]. *) + let check_proposals_and_update (state : t) oph proposer proposals + ~count_in_ctxt ~proposals_length = + let open Tzresult_syntax in + let* new_proposer_history = + match + Signature.Public_key_hash.Map.find proposer state.proposals_validated + with + | None -> + let proposed = + List.fold_left + (fun acc proposal -> Protocol_hash.Map.add proposal oph acc) + Protocol_hash.Map.empty + proposals + in + return {count = proposals_length; operations = [oph]; proposed} + | Some proposer_history -> + let* () = + check_count_conflict + ~count_previous_blocks:count_in_ctxt + ~count_operation:proposals_length + proposer_history + in + let add_proposal proposed_map proposal = + match Protocol_hash.Map.find proposal proposer_history.proposed with + | Some conflicting_operation -> + error + (Conflict_already_proposed {proposal; conflicting_operation}) + | None -> ok (Protocol_hash.Map.add proposal oph proposed_map) + in + let* proposed = + List.fold_left_e add_proposal proposer_history.proposed proposals + in + return + { + count = proposer_history.count + proposals_length; + operations = oph :: proposer_history.operations; + proposed; + } + in + let* () = + match state.dictator_proposals_validated with + | None -> ok () + | Some dictator_oph -> error (Conflicting_dictator_proposals dictator_oph) + in + let proposals_validated = + Signature.Public_key_hash.Map.add + proposer + new_proposer_history + state.proposals_validated + in + return {state with proposals_validated} + + (** Check that a Proposals operation from a testnet dictator is + compatible with previously validated voting operations in the + current block/mempool (ie. that no other voting operation has + been validated), and update the [state] with this operation. + + @return [Error Testnet_dictator_conflicting_operation] if the + current block or mempool already contains any validated voting + operation. *) + let check_dictator_proposals_and_update state oph = + let open Tzresult_syntax in + let* () = + error_unless + (Signature.Public_key_hash.Map.is_empty state.proposals_validated + && Option.is_none state.dictator_proposals_validated) + Testnet_dictator_conflicting_operation + in + return {state with dictator_proposals_validated = Some oph} +end + (** Helpers to validate and apply Proposals operations from a registered dictator of a test chain. These operations let the dictator immediately change the current voting period's kind, and @@ -255,15 +414,13 @@ let check_in_listings ctxt source = let*! in_listings = Vote.in_listings ctxt source in fail_unless in_listings Source_not_in_vote_listings -let check_count ctxt proposer proposals = - let open Lwt_tzresult_syntax in - let* count = Vote.get_delegate_proposal_count ctxt proposer in - (* [count] should never have been increased above - [max_proposals_per_delegate]. *) - assert (Compare.Int.(count <= Constants.max_proposals_per_delegate)) ; - fail_unless - Compare.List_length_with.( - proposals <= Constants.max_proposals_per_delegate - count) +let check_count ~count_in_ctxt ~proposals_length = + (* The proposal count of the proposer in the context should never + have been increased above [max_proposals_per_delegate]. *) + assert (Compare.Int.(count_in_ctxt <= Constants.max_proposals_per_delegate)) ; + error_unless + Compare.Int.( + count_in_ctxt + proposals_length <= Constants.max_proposals_per_delegate) Too_many_proposals let check_already_proposed ctxt proposer proposals = @@ -274,15 +431,7 @@ let check_already_proposed ctxt proposer proposals = fail_when already_proposed (Already_proposed {proposal})) proposals -(* For now, this function is still called directly by - [apply_proposals], and so [ctxt] is still the context at - application time (where previous operations from the current block - have been applied). - - In the next commit, this function will be called by - [Validate_operation.validate_operation] instead (and will need to - be updated because [ctxt] will not be the same). *) -let validate_proposals ctxt chain_id ~should_check_signature +let validate_proposals ctxt chain_id state ~should_check_signature oph (operation : Kind.proposals operation) = let open Lwt_tzresult_syntax in let (Single (Proposals {source; period; proposals})) = @@ -290,51 +439,57 @@ let validate_proposals ctxt chain_id ~should_check_signature in let* current_period = Voting_period.get_current ctxt in let*? () = check_period_index ~expected:current_period.index period in - let* () = + let* state = if Testnet_dictator.is_testnet_dictator ctxt chain_id source then - Lwt.return (Testnet_dictator.check_proposals chain_id proposals) + let*? () = Testnet_dictator.check_proposals chain_id proposals in + Lwt.return + (Validation_state.check_dictator_proposals_and_update state oph) else let*? () = check_proposal_list_sanity proposals in let*? () = check_period_kind_for_proposals current_period in let* () = check_in_listings ctxt source in - let* () = check_count ctxt source proposals in - check_already_proposed ctxt source proposals + let* count_in_ctxt = Vote.get_delegate_proposal_count ctxt source in + let proposals_length = List.length proposals in + let*? () = check_count ~count_in_ctxt ~proposals_length in + let* () = check_already_proposed ctxt source proposals in + Lwt.return + (Validation_state.check_proposals_and_update + state + oph + source + proposals + ~count_in_ctxt + ~proposals_length) in (* The signature check is done last because it is more costly than most checks. *) - when_ should_check_signature (fun () -> - (* Retrieving the public key cannot fail. Indeed, we have - already checked that the delegate is in the vote listings - (or is a testnet dictator), which implies that it is a - manager with a revealed key. *) - let* public_key = Delegate.pubkey ctxt source in - Lwt.return (Operation.check_signature public_key chain_id operation)) - -let record_proposals ctxt proposer proposals = - let open Lwt_tzresult_syntax in - let* count = Vote.get_delegate_proposal_count ctxt proposer in - let new_count = count + List.length proposals in - let*! ctxt = Vote.set_delegate_proposal_count ctxt proposer new_count in - let*! ctxt = - List.fold_left_s - (fun ctxt proposal -> Vote.add_proposal ctxt proposer proposal) - ctxt - proposals + let* () = + when_ should_check_signature (fun () -> + (* Retrieving the public key cannot fail. Indeed, we have + already checked that the delegate is in the vote listings + (or is a testnet dictator), which implies that it is a + manager with a revealed key. *) + let* public_key = Delegate.pubkey ctxt source in + Lwt.return (Operation.check_signature public_key chain_id operation)) in - return ctxt + return state -let apply_proposals ctxt chain_id (operation : Kind.proposals operation) = +let apply_proposals ctxt chain_id (Proposals {source; period = _; proposals}) = let open Lwt_tzresult_syntax in - let* () = - validate_proposals ctxt chain_id ~should_check_signature:true operation - in - let (Single (Proposals {source; period = _; proposals})) = - operation.protocol_data.contents - in let* ctxt = if Testnet_dictator.is_testnet_dictator ctxt chain_id source then Testnet_dictator.record_proposals ctxt chain_id proposals - else record_proposals ctxt source proposals + else + let* count = Vote.get_delegate_proposal_count ctxt source in + let new_count = count + List.length proposals in + let*! ctxt = Vote.set_delegate_proposal_count ctxt source new_count in + let*! ctxt = + List.fold_left_s + (fun ctxt proposal -> Vote.add_proposal ctxt source proposal) + ctxt + proposals + in + return ctxt in return (ctxt, Apply_results.Single_result Proposals_result) diff --git a/src/proto_alpha/lib_protocol/amendment.mli b/src/proto_alpha/lib_protocol/amendment.mli index a31abbf86fe0..9d82a3d07967 100644 --- a/src/proto_alpha/lib_protocol/amendment.mli +++ b/src/proto_alpha/lib_protocol/amendment.mli @@ -86,7 +86,20 @@ val may_start_new_voting_period : context -> context tzresult Lwt.t (or pass). This operation is only accepted during an Exploration or Promotion period (see above). *) -(** Update the [context] with the effects of a Proposals operation. +(** A state containing a summary of previously validated voting + operations. It should be maintained in memory during the validation + of a block, or until a change of head block in mempool mode. It is + used to check for conflicts between an operation to validate and + the already validated operations of the current block/mempool. *) +module Validation_state : sig + (** A state as described right above. *) + type t + + (** The empty state (for the initialization of a new block or mempool). *) + val empty : t +end + +(** Check that a Proposals operation can be safely applied. @return [Error Wrong_voting_period_index] if the operation's period and the [context]'s current period do not have the same @@ -110,17 +123,64 @@ val may_start_new_voting_period : context -> context tzresult Lwt.t @return [Error Already_proposed] if one of the proposals has already been proposed by the source. + @return [Error Conflict_too_many_proposals] if the total count of + proposals submitted by the source in previous blocks, in previously + validated operations of the current block/mempool, and in the + operation to validate, exceeds + {!Constants.max_proposals_per_delegate}. + + @return [Error Conflict_already_proposed] if one of the + operation's proposals has already been submitted by the source in + the current block/mempool. + + @return [Error Conflicting_dictator_proposals] if a testnet + dictator Proposals operation has already been validated in the + current block/mempool. + @return [Error Testnet_dictator_multiple_proposals] if the source is a testnet dictator and the operation contains more than one proposal. + @return [Error Testnet_dictator_conflicting_operation] if the + source is a testnet dictator and the current block or mempool + already contains any validated voting operation. + @return [Error Operation.Missing_signature] or [Error Operation.Invalid_signature] if the operation is unsigned or incorrectly signed. *) -val apply_proposals : +val validate_proposals : context -> Chain_id.t -> + Validation_state.t -> + should_check_signature:bool -> + Operation_hash.t -> Kind.proposals operation -> + Validation_state.t tzresult Lwt.t + +(** Update the [context] with the effects of a Proposals operation: + + - Its proposals are added to the source's recorded proposals. + + - The recorded proposal count of the source is increased by the + number of proposals in the operation. + + Note that a Proposals operation from a testnet dictator (which may + be set up when a test chain is initialized) has completely + different effects: + + - If the operation contains no proposal, then the current voting + period is immediately and forcibly set to a Proposal period. + + - If the operation contains exactly one proposal, then the current + voting period is immediately and forcibly set to an Adoption period + for this proposal. + + {!validate_proposals} must have been called beforehand, and is + responsible for ensuring that [apply_proposals] cannot fail. *) +val apply_proposals : + context -> + Chain_id.t -> + Kind.proposals contents -> (context * Kind.proposals Apply_results.contents_result_list) tzresult Lwt.t (** Update the [context] with the effects of a Ballot operation. diff --git a/src/proto_alpha/lib_protocol/apply.ml b/src/proto_alpha/lib_protocol/apply.ml index 3917e642c298..cb4cbb157fef 100644 --- a/src/proto_alpha/lib_protocol/apply.ml +++ b/src/proto_alpha/lib_protocol/apply.ml @@ -2127,7 +2127,8 @@ let apply_contents_list (type kind) ctxt chain_id (apply_mode : apply_mode) Token.transfer ctxt src (`Contract contract) amount >>=? fun (ctxt, bupds) -> return (ctxt, Single_result (Activate_account_result bupds)) - | Single (Proposals _) -> Amendment.apply_proposals ctxt chain_id operation + | Single (Proposals _ as contents) -> + Amendment.apply_proposals ctxt chain_id contents | Single (Ballot _) -> Amendment.apply_ballot ctxt chain_id operation | Single (Failing_noop _) -> (* Failing_noop _ always fails *) diff --git a/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml b/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml index 6cc29bab3652..34cb0bba757f 100644 --- a/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml +++ b/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml @@ -368,6 +368,18 @@ let already_proposed already_proposed_proposal loc = function already_proposed_proposal | err -> wrong_error "Already_proposed" err loc +let conflict_too_many_proposals loc = function + | [Environment.Ecoproto_error (Conflict_too_many_proposals _)] -> return_unit + | err -> wrong_error "Conflict_too_many_proposals" err loc + +let conflict_already_proposed already_proposed_proposal loc = function + | [Environment.Ecoproto_error (Conflict_already_proposed {proposal; _})] -> + Assert.equal_protocol_hash + ~loc:(append_loc ~caller_loc:loc __LOC__) + proposal + already_proposed_proposal + | err -> wrong_error "Conflict_already_proposed" err loc + let ballot_for_wrong_proposal ~current_proposal ~op_proposal loc = function | [ Environment.Ecoproto_error (Ballot_for_wrong_proposal {current; submitted}); @@ -1437,6 +1449,65 @@ let test_already_proposed () = operation block +(** Test that a Proposals operation fails when it would make the total + count of proposals submitted by the proposer exceed the + [max_proposals_per_delegate] protocol constant, because of + previously validated operations in the current block/mempool. *) +let test_conflict_too_many_proposals () = + let open Lwt_result_syntax in + let* block, proposer = context_init1 () in + let n_proposals_in_previous_blocks = 5 in + assert (Array.length protos >= Constants.max_proposals_per_delegate + 1) ; + let proposals_in_previous_blocks = + List.map (Array.get protos) (1 -- n_proposals_in_previous_blocks) + in + let* operation = + Op.proposals (B block) proposer proposals_in_previous_blocks + in + let* block = Block.bake block ~operation in + let* current_block_state = Incremental.begin_construction block in + let proposals_in_current_block = + List.map + (Array.get protos) + (n_proposals_in_previous_blocks + 1 + -- Constants.max_proposals_per_delegate) + in + let* op_in_current_block = + Op.proposals (B block) proposer proposals_in_current_block + in + let* current_block_state = + Incremental.validate_operation current_block_state op_in_current_block + in + let* op = Op.proposals (B block) proposer [protos.(0)] in + let* _i = + Incremental.validate_operation + ~expect_failure:(conflict_too_many_proposals __LOC__) + current_block_state + op + in + return_unit + +(** Test that a Proposals operation fails when one of its proposals + has already been submitted by the same proposer in a previously + validated operation of the current block/mempool. *) +let test_conflict_already_proposed () = + let open Lwt_result_syntax in + let* block, proposer = context_init1 () in + let proposal = protos.(0) in + let* current_block_state = Incremental.begin_construction block in + let* op_in_current_block = Op.proposals (B block) proposer [proposal] in + let* current_block_state = + Incremental.validate_operation current_block_state op_in_current_block + in + let* op = Op.proposals (B block) proposer [proposal] in + let* _i = + Incremental.validate_operation + ~expect_failure:(conflict_already_proposed proposal __LOC__) + current_block_state + op + in + return_unit + (** {3 Proposals -- Positive test} A Proposals operation is valid when: @@ -1939,6 +2010,14 @@ let tests = "A proposal had already been proposed" `Quick test_already_proposed; + Tztest.tztest + "Conflict: too many proposals in current block/mempool" + `Quick + test_conflict_too_many_proposals; + Tztest.tztest + "Conflict: proposal already proposed in current block/mempool" + `Quick + test_conflict_already_proposed; Tztest.tztest "Valid Proposals operations" `Quick test_valid_proposals; (* Validity tests on Ballot *) Tztest.tztest diff --git a/src/proto_alpha/lib_protocol/validate_errors.ml b/src/proto_alpha/lib_protocol/validate_errors.ml index d50ee0a2557e..4e2f5ca9d6a5 100644 --- a/src/proto_alpha/lib_protocol/validate_errors.ml +++ b/src/proto_alpha/lib_protocol/validate_errors.ml @@ -397,7 +397,20 @@ module Voting = struct | Proposals_contain_duplicate of {proposal : Protocol_hash.t} | Too_many_proposals | Already_proposed of {proposal : Protocol_hash.t} + | Conflict_too_many_proposals of { + max_allowed : int; + count_previous_blocks : int; + count_current_block : int; + count_operation : int; + conflicting_operations : Operation_hash.t list; + } + | Conflict_already_proposed of { + proposal : Protocol_hash.t; + conflicting_operation : Operation_hash.t; + } + | Conflicting_dictator_proposals of Operation_hash.t | Testnet_dictator_multiple_proposals + | Testnet_dictator_conflicting_operation | (* Ballot errors *) Ballot_for_wrong_proposal of { current : Protocol_hash.t; @@ -520,6 +533,113 @@ module Voting = struct Data_encoding.(obj1 (req "proposal" Protocol_hash.encoding)) (function Already_proposed {proposal} -> Some proposal | _ -> None) (fun proposal -> Already_proposed {proposal}) ; + register_error_kind + `Temporary + ~id:"validate_operation.conflict_too_many_proposals" + ~title:"Conflict too many proposals" + ~description: + "The delegate exceeded the maximum number of allowed proposals due to, \ + among others, previous Proposals operations in the current \ + block/mempool." + ~pp: + (fun ppf + ( max_allowed, + count_previous_blocks, + count_current_block, + count_operation, + conflicting_operations ) -> + Format.fprintf + ppf + "The delegate has submitted too many proposals (the maximum allowed \ + is %d): %d in previous blocks, %d in the considered operation, and \ + %d in the following validated operations of the current \ + block/mempool: %a." + max_allowed + count_previous_blocks + count_operation + count_current_block + (Format.pp_print_list Operation_hash.pp) + conflicting_operations) + Data_encoding.( + obj5 + (req "max_allowed" int8) + (req "count_previous_blocks" int8) + (req "count_current_block" int8) + (req "count_operation" int8) + (req "conflicting_operations" (list Operation_hash.encoding))) + (function + | Conflict_too_many_proposals + { + max_allowed; + count_previous_blocks; + count_current_block; + count_operation; + conflicting_operations; + } -> + Some + ( max_allowed, + count_previous_blocks, + count_current_block, + count_operation, + conflicting_operations ) + | _ -> None) + (fun ( max_allowed, + count_previous_blocks, + count_current_block, + count_operation, + conflicting_operations ) -> + Conflict_too_many_proposals + { + max_allowed; + count_previous_blocks; + count_current_block; + count_operation; + conflicting_operations; + }) ; + register_error_kind + `Temporary + ~id:"validate_operation.conflict_already_proposed" + ~title:"Conflict already proposed" + ~description: + "The delegate has already submitted one of the operation's proposals \ + in a previously validated operation of the current block or mempool." + ~pp:(fun ppf (proposal, conflicting_oph) -> + Format.fprintf + ppf + "The delegate has already proposed the protocol hash %a in the \ + previously validated operation %a of the current block or mempool." + Protocol_hash.pp + proposal + Operation_hash.pp + conflicting_oph) + Data_encoding.( + obj2 + (req "proposal" Protocol_hash.encoding) + (req "conflicting_operation" Operation_hash.encoding)) + (function + | Conflict_already_proposed {proposal; conflicting_operation} -> + Some (proposal, conflicting_operation) + | _ -> None) + (fun (proposal, conflicting_operation) -> + Conflict_already_proposed {proposal; conflicting_operation}) ; + register_error_kind + `Branch + ~id:"validate_operation.conflicting_dictator_proposals" + ~title:"Conflicting dictator proposals" + ~description: + "The current block/mempool already contains a testnest dictator \ + proposals operation, so it cannot have any other voting operation." + ~pp:(fun ppf dictator_operation -> + Format.fprintf + ppf + "The current block/mempool already contains the testnest dictator \ + proposals operation %a, so it cannot have any other voting \ + operation." + Operation_hash.pp + dictator_operation) + Data_encoding.(obj1 (req "dictator_operation" Operation_hash.encoding)) + (function Conflicting_dictator_proposals oph -> Some oph | _ -> None) + (fun oph -> Conflicting_dictator_proposals oph) ; let description = "A testnet dictator cannot submit more than one proposal at a time." in @@ -532,6 +652,19 @@ module Voting = struct Data_encoding.empty (function Testnet_dictator_multiple_proposals -> Some () | _ -> None) (fun () -> Testnet_dictator_multiple_proposals) ; + let description = + "A testnet dictator proposals operation cannot be included in a block or \ + mempool that already contains any other voting operation." + in + register_error_kind + `Branch + ~id:"validate_operation.testnet_dictator_conflicting_operation" + ~title:"Testnet dictator conflicting operation" + ~description + ~pp:(fun ppf () -> Format.fprintf ppf "%s" description) + Data_encoding.empty + (function Testnet_dictator_conflicting_operation -> Some () | _ -> None) + (fun () -> Testnet_dictator_conflicting_operation) ; (* Ballot errors *) register_error_kind diff --git a/src/proto_alpha/lib_protocol/validate_errors.mli b/src/proto_alpha/lib_protocol/validate_errors.mli index 951ff8cddcb8..c781cb1aa335 100644 --- a/src/proto_alpha/lib_protocol/validate_errors.mli +++ b/src/proto_alpha/lib_protocol/validate_errors.mli @@ -99,7 +99,20 @@ module Voting : sig | Proposals_contain_duplicate of {proposal : Protocol_hash.t} | Too_many_proposals | Already_proposed of {proposal : Protocol_hash.t} + | Conflict_too_many_proposals of { + max_allowed : int; + count_previous_blocks : int; + count_current_block : int; + count_operation : int; + conflicting_operations : Operation_hash.t list; + } + | Conflict_already_proposed of { + proposal : Protocol_hash.t; + conflicting_operation : Operation_hash.t; + } + | Conflicting_dictator_proposals of Operation_hash.t | Testnet_dictator_multiple_proposals + | Testnet_dictator_conflicting_operation | (* Ballot errors *) Ballot_for_wrong_proposal of { current : Protocol_hash.t; diff --git a/src/proto_alpha/lib_protocol/validate_operation.ml b/src/proto_alpha/lib_protocol/validate_operation.ml index 6327d6ae8714..c5c5633ce216 100644 --- a/src/proto_alpha/lib_protocol/validate_operation.ml +++ b/src/proto_alpha/lib_protocol/validate_operation.ml @@ -222,7 +222,7 @@ let init_manager_state ctxt = type mode = Block | Mempool type validate_operation_info = { - ctxt : t; (** The context at the beginning of the block. *) + ctxt : t; (** The context at the beginning of the block or mempool. *) mode : mode; chain_id : Chain_id.t; (** Needed for signature checks. *) current_level : Level.t; @@ -232,6 +232,7 @@ type validate_operation_info = { type validate_operation_state = { consensus_state : consensus_state; + voting_state : Amendment.Validation_state.t; anonymous_state : anonymous_state; manager_state : manager_state; } @@ -251,6 +252,7 @@ let init_validate_operation_info ctxt mode chain_id let init_validate_operation_state ctxt = { consensus_state = empty_consensus_state; + voting_state = Amendment.Validation_state.empty; anonymous_state = empty_anonymous_state; manager_state = init_manager_state ctxt; } @@ -727,6 +729,23 @@ module Consensus = struct return (update_validity_state_dal_slot_availabitiy vs endorser) end +(** Validation of voting operations: proposals and ballot. + (Validation pass [1]) *) +module Voting = struct + let validate_proposals vi vs ~should_check_signature oph operation = + let open Lwt_tzresult_syntax in + let* voting_state = + Amendment.validate_proposals + vi.ctxt + vi.chain_id + vs.voting_state + ~should_check_signature + oph + operation + in + return {vs with voting_state} +end + module Anonymous = struct open Validate_errors.Anonymous @@ -1506,6 +1525,8 @@ let validate_operation (vi : validate_operation_info) vs ~should_check_signature operation + | Single (Proposals _) -> + Voting.validate_proposals vi vs ~should_check_signature oph operation | Single (Activate_account _ as contents) -> Anonymous.validate_activate_account vi vs oph contents | Single (Double_preendorsement_evidence _ as contents) -> @@ -1534,7 +1555,7 @@ let validate_operation (vi : validate_operation_info) source oph operation - | Single (Proposals _) | Single (Ballot _) | Single (Failing_noop _) -> + | Single (Ballot _) | Single (Failing_noop _) -> (* TODO: https://gitlab.com/tezos/tezos/-/issues/2603 There is no separate validation phase for non-manager -- GitLab From e43238ac3a440fe9b42e7becad36f1972d6ba0ec Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Mon, 18 Jul 2022 15:12:49 +0200 Subject: [PATCH 6/9] Proto: handle ballot in validate_operation The ballot operation is simpler to process than the proposals one. Therefore we split it into validate and apply functions, then move the validate call to Validate_operation.validate_operation, in a single commit. --- src/proto_alpha/lib_protocol/amendment.ml | 109 +++++++++++++----- src/proto_alpha/lib_protocol/amendment.mli | 34 ++++-- src/proto_alpha/lib_protocol/apply.ml | 26 +---- src/proto_alpha/lib_protocol/apply.mli | 9 -- .../operations/test_failing_noop.ml | 7 +- .../integration/operations/test_voting.ml | 101 +++++++++------- .../lib_protocol/validate_errors.ml | 37 +++++- .../lib_protocol/validate_errors.mli | 3 + .../lib_protocol/validate_operation.ml | 30 ++--- .../lib_protocol/validate_operation.mli | 15 +-- tests_python/tests_alpha/test_basic.py | 2 +- 11 files changed, 240 insertions(+), 133 deletions(-) diff --git a/src/proto_alpha/lib_protocol/amendment.ml b/src/proto_alpha/lib_protocol/amendment.ml index 8af0b9c077c0..3902801242aa 100644 --- a/src/proto_alpha/lib_protocol/amendment.ml +++ b/src/proto_alpha/lib_protocol/amendment.ml @@ -186,12 +186,16 @@ module Validation_state = struct in a single block (otherwise we would loose the commutativity of validated operation application: see {!Validate_operation}). *) + ballots_validated : Operation_hash.t Signature.Public_key_hash.Map.t; + (** To each delegate that has submitted a ballot in a previously + validated operation, associates the hash of this operation. *) } let empty = { proposals_validated = Signature.Public_key_hash.Map.empty; dictator_proposals_validated = None; + ballots_validated = Signature.Public_key_hash.Map.empty; } let check_count_conflict ~count_previous_blocks ~count_operation @@ -305,10 +309,41 @@ module Validation_state = struct let* () = error_unless (Signature.Public_key_hash.Map.is_empty state.proposals_validated - && Option.is_none state.dictator_proposals_validated) + && Option.is_none state.dictator_proposals_validated + && Signature.Public_key_hash.Map.is_empty state.ballots_validated) Testnet_dictator_conflicting_operation in return {state with dictator_proposals_validated = Some oph} + + (** Check that a Ballot operation is compatible with previously + validated voting operations in the current block/mempool. + + @return [Error Conflicting_ballot] if the [delegate] has already + submitted a ballot in the current block/mempool. + + @return [Error Conflicting_dictator_proposals] if the current + block/mempool already contains a testnet dictator Proposals + operation (see {!recfield:dictator_proposals_validated}). *) + let check_ballot state voter = + let open Tzresult_syntax in + let* () = + match + Signature.Public_key_hash.Map.find voter state.ballots_validated + with + | None -> ok () + | Some conflicting_operation -> + error (Conflicting_ballot {conflicting_operation}) + in + match state.dictator_proposals_validated with + | None -> ok () + | Some dictator_oph -> error (Conflicting_dictator_proposals dictator_oph) + + (** Update the [state] when a Ballot operation is validated. *) + let update_on_ballot state oph voter = + let ballots_validated = + Signature.Public_key_hash.Map.add voter oph state.ballots_validated + in + {state with ballots_validated} end (** Helpers to validate and apply Proposals operations from a @@ -469,7 +504,7 @@ let validate_proposals ctxt chain_id state ~should_check_signature oph already checked that the delegate is in the vote listings (or is a testnet dictator), which implies that it is a manager with a revealed key. *) - let* public_key = Delegate.pubkey ctxt source in + let* public_key = Contract.get_manager_key ctxt source in Lwt.return (Operation.check_signature public_key chain_id operation)) in return state @@ -493,34 +528,52 @@ let apply_proposals ctxt chain_id (Proposals {source; period = _; proposals}) = in return (ctxt, Apply_results.Single_result Proposals_result) -let record_ballot ctxt delegate proposal ballot = - Voting_period.get_current_kind ctxt >>=? function - | Exploration | Promotion -> - Vote.get_current_proposal ctxt >>=? fun current_proposal -> - error_unless - (Protocol_hash.equal proposal current_proposal) - (Ballot_for_wrong_proposal - {current = current_proposal; submitted = proposal}) - >>?= fun () -> - Vote.has_recorded_ballot ctxt delegate >>= fun has_ballot -> - error_when has_ballot Already_submitted_a_ballot >>?= fun () -> - Vote.in_listings ctxt delegate >>= fun in_listings -> - if in_listings then Vote.record_ballot ctxt delegate ballot - else fail Source_not_in_vote_listings +let check_period_kind_for_ballot current_period = + match current_period.Voting_period.kind with + | Exploration | Promotion -> ok () | (Cooldown | Proposal | Adoption) as current -> - fail + error (Wrong_voting_period_kind {current; expected = [Exploration; Promotion]}) -let apply_ballot ctxt chain_id (operation : Kind.ballot operation) = - let (Single (Ballot {source; period; proposal; ballot})) = +let check_current_proposal ctxt op_proposal = + let open Lwt_tzresult_syntax in + let* current_proposal = Vote.get_current_proposal ctxt in + fail_unless + (Protocol_hash.equal op_proposal current_proposal) + (Ballot_for_wrong_proposal + {current = current_proposal; submitted = op_proposal}) + +let check_source_has_not_already_voted ctxt source = + let open Lwt_tzresult_syntax in + let*! has_ballot = Vote.has_recorded_ballot ctxt source in + fail_when has_ballot Already_submitted_a_ballot + +let validate_ballot ctxt chain_id state ~should_check_signature oph + (operation : Kind.ballot operation) = + let open Lwt_tzresult_syntax in + let (Single (Ballot {source; period; proposal; ballot = _})) = operation.protocol_data.contents in - Delegate.pubkey ctxt source >>=? fun delegate -> - Operation.check_signature delegate chain_id operation >>?= fun () -> - Voting_period.get_current ctxt >>=? fun {index = current_period; _} -> - error_unless - Compare.Int32.(current_period = period) - (Wrong_voting_period_index {expected = current_period; provided = period}) - >>?= fun () -> - record_ballot ctxt source proposal ballot >|=? fun ctxt -> - (ctxt, Apply_results.Single_result Ballot_result) + let*? () = Validation_state.check_ballot state source in + let* current_period = Voting_period.get_current ctxt in + let*? () = check_period_index ~expected:current_period.index period in + let*? () = check_period_kind_for_ballot current_period in + let* () = check_current_proposal ctxt proposal in + let* () = check_source_has_not_already_voted ctxt source in + let* () = check_in_listings ctxt source in + (* The signature check is done last because it is more costly than + most checks. *) + let* () = + when_ should_check_signature (fun () -> + (* Retrieving the public key cannot fail. Indeed, we have + already checked that the delegate is in the vote listings, + which implies that it is a manager with a revealed key. *) + let* public_key = Contract.get_manager_key ctxt source in + Lwt.return (Operation.check_signature public_key chain_id operation)) + in + return (Validation_state.update_on_ballot state oph source) + +let apply_ballot ctxt (Ballot {source; period = _; proposal = _; ballot}) = + let open Lwt_tzresult_syntax in + let* ctxt = Vote.record_ballot ctxt source ballot in + return (ctxt, Apply_results.Single_result Ballot_result) diff --git a/src/proto_alpha/lib_protocol/amendment.mli b/src/proto_alpha/lib_protocol/amendment.mli index 9d82a3d07967..a843efa26b74 100644 --- a/src/proto_alpha/lib_protocol/amendment.mli +++ b/src/proto_alpha/lib_protocol/amendment.mli @@ -183,14 +183,14 @@ val apply_proposals : Kind.proposals contents -> (context * Kind.proposals Apply_results.contents_result_list) tzresult Lwt.t -(** Update the [context] with the effects of a Ballot operation. +(** Check that a Ballot operation can be safely applied. - @return [Error Delegate_storage.Unregistered_delegate] if the - source's contract does not exist or its public key is unreavealed. + @return [Error Conflicting_ballot] if the source has already + submitted a ballot in the current block/mempool. - @return [Error Operation.Missing_signature] or [Error - Operation.Invalid_signature] if the operation is unsigned or - incorrectly signed. + @return [Error Conflicting_dictator_proposals] if the current + block/mempool already contains a validated testnet dictator + Proposals operation. @return [Error Wrong_voting_period_index] if the operation's period and the [context]'s current period do not have the same @@ -206,9 +206,27 @@ val apply_proposals : already voted. @return [Error Source_not_in_vote_listings] if the source is not - in the vote listings. *) -val apply_ballot : + in the vote listings. + + @return [Error Operation.Missing_signature] or [Error + Operation.Invalid_signature] if the operation is unsigned or + incorrectly signed. *) +val validate_ballot : context -> Chain_id.t -> + Validation_state.t -> + should_check_signature:bool -> + Operation_hash.t -> Kind.ballot operation -> + Validation_state.t tzresult Lwt.t + +(** Update the [context] with the effects of a Ballot operation: + + The couple (source of the operation, submitted ballot) is recorded. + + {!validate_ballot} must have been called beforehand, and is + responsible for ensuring that [apply_ballot] cannot fail. *) +val apply_ballot : + context -> + Kind.ballot contents -> (context * Kind.ballot Apply_results.contents_result_list) tzresult Lwt.t diff --git a/src/proto_alpha/lib_protocol/apply.ml b/src/proto_alpha/lib_protocol/apply.ml index cb4cbb157fef..03d7399fc9a8 100644 --- a/src/proto_alpha/lib_protocol/apply.ml +++ b/src/proto_alpha/lib_protocol/apply.ml @@ -43,7 +43,6 @@ type error += | Internal_operation_replay of Apply_internal_results.packed_internal_operation | Multiple_revelation - | Failing_noop_error | Zero_frozen_deposits of Signature.Public_key_hash.t | Invalid_transfer_to_sc_rollup_from_implicit_account @@ -222,20 +221,6 @@ let () = Data_encoding.empty (function Multiple_revelation -> Some () | _ -> None) (fun () -> Multiple_revelation) ; - register_error_kind - `Permanent - ~id:"operation.failing_noop" - ~title:"Failing_noop operations are not executed by the protocol" - ~description: - "The failing_noop operation is an operation that is not and never will \ - be executed by the protocol." - ~pp:(fun ppf () -> - Format.fprintf - ppf - "The failing_noop operation cannot be executed by the protocol") - Data_encoding.empty - (function Failing_noop_error -> Some () | _ -> None) - (fun () -> Failing_noop_error) ; register_error_kind `Permanent ~id:"delegate.zero_frozen_deposits" @@ -2066,8 +2051,7 @@ let punish_double_baking ctxt (bh1 : Block_header.t) ~payload_producer = (fun balance_updates -> Double_baking_evidence_result balance_updates) let apply_contents_list (type kind) ctxt chain_id (apply_mode : apply_mode) - ~payload_producer op_validated_stamp (operation : kind operation) - (contents_list : kind contents_list) : + ~payload_producer op_validated_stamp (contents_list : kind contents_list) : (context * kind contents_result_list) tzresult Lwt.t = let mempool_mode = match apply_mode with @@ -2129,10 +2113,11 @@ let apply_contents_list (type kind) ctxt chain_id (apply_mode : apply_mode) return (ctxt, Single_result (Activate_account_result bupds)) | Single (Proposals _ as contents) -> Amendment.apply_proposals ctxt chain_id contents - | Single (Ballot _) -> Amendment.apply_ballot ctxt chain_id operation + | Single (Ballot _ as contents) -> Amendment.apply_ballot ctxt contents | Single (Failing_noop _) -> - (* Failing_noop _ always fails *) - fail Failing_noop_error + (* This operation always fails. It should already have been + rejected by {!Validate_operation.validate_operation}. *) + fail Validate_errors.Failing_noop_error | Single (Manager_operation _) -> apply_manager_operations ctxt @@ -2160,7 +2145,6 @@ let apply_operation ctxt chain_id (apply_mode : apply_mode) ~payload_producer apply_mode ~payload_producer op_validated_stamp - operation operation.protocol_data.contents >|=? fun (ctxt, result) -> let ctxt = Gas.set_unlimited ctxt in diff --git a/src/proto_alpha/lib_protocol/apply.mli b/src/proto_alpha/lib_protocol/apply.mli index 58de622e7180..fbd99dc7f530 100644 --- a/src/proto_alpha/lib_protocol/apply.mli +++ b/src/proto_alpha/lib_protocol/apply.mli @@ -108,15 +108,6 @@ type apply_mode = For non-manager operations, the application of a validated operation should always fully succeed. - TODO: https://gitlab.com/tezos/tezos/-/issues/2603 - - Currently, {!Validate_operation.validate_operation} does nothing - on voting operations. The "validation" of these operations is - instead handled by [apply_operation], which may thus return an - error if the operation is ill-formed. Once [validate_operation] has - been extended to every kind of operation, [apply_operation] should - never return an error. - For manager operations, the application has two stages. The first stage consists in updating the context to: diff --git a/src/proto_alpha/lib_protocol/test/integration/operations/test_failing_noop.ml b/src/proto_alpha/lib_protocol/test/integration/operations/test_failing_noop.ml index 798c949977fe..917fe3dec5f4 100644 --- a/src/proto_alpha/lib_protocol/test/integration/operations/test_failing_noop.ml +++ b/src/proto_alpha/lib_protocol/test/integration/operations/test_failing_noop.ml @@ -43,10 +43,9 @@ let failing_noop_must_fail_when_injected () = let source = Context.Contract.pkh contract in Op.failing_noop (B blk) source "tezos" >>=? fun operation -> Block.bake ~operation blk >>= fun res -> - Assert.proto_error_with_info - ~loc:__LOC__ - res - "Failing_noop operations are not executed by the protocol" + Assert.proto_error ~loc:__LOC__ res (function + | Protocol.Validate_errors.Failing_noop_error -> true + | _ -> false) let tests = [Tztest.tztest "injection fails" `Quick failing_noop_must_fail_when_injected] diff --git a/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml b/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml index 34cb0bba757f..772137cfa475 100644 --- a/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml +++ b/src/proto_alpha/lib_protocol/test/integration/operations/test_voting.ml @@ -287,9 +287,9 @@ let context_init2 = context_init_tup Context.T2 blocks in order to move on to an Exploration period. Return a block, a delegate (distinct from the one who submitted the Proposals), and the current proposal. *) -let context_init_exploration ?(proposal = protos.(0)) () = +let context_init_exploration ?(proposal = protos.(0)) ?blocks_per_cycle () = let open Lwt_result_syntax in - let* block, (proposer, other_delegate) = context_init2 () in + let* block, (proposer, other_delegate) = context_init2 ?blocks_per_cycle () in let* operation = Op.proposals (B block) proposer [proposal] in let* block = Block.bake block ~operation in let* block = bake_until_first_block_of_next_period block in @@ -309,11 +309,6 @@ let wrong_error expected_error_name actual_error_trace loc = Error_monad.pp_print_trace actual_error_trace -let unregistered_delegate delegate loc = function - | [Environment.Ecoproto_error (Delegate_storage.Unregistered_delegate pkh)] -> - Assert.equal_pkh ~loc:(append_loc ~caller_loc:loc __LOC__) pkh delegate - | err -> wrong_error "Unregistered_delegate" err loc - let missing_signature loc = function | [Environment.Ecoproto_error Operation.Missing_signature] -> return_unit | err -> wrong_error "Missing_signature" err loc @@ -401,6 +396,10 @@ let already_submitted_a_ballot loc = function | [Environment.Ecoproto_error Already_submitted_a_ballot] -> return_unit | err -> wrong_error "Already_submitted_a_ballot" err loc +let conflicting_ballot loc = function + | [Environment.Ecoproto_error (Conflicting_ballot _)] -> return_unit + | err -> wrong_error "Conflicting_ballot" err loc + let assert_validate_proposals_fails ~expected_error ~proposer ~proposals ?period block loc = let open Lwt_result_syntax in @@ -1326,6 +1325,8 @@ let test_proposals_wrong_voting_period_kind () = source is from being a delegate with voting rights). *) let test_proposals_source_not_in_vote_listings () = let open Lwt_result_syntax in + (* The chosen [blocks_per_cycle] is an arbitrary value that we will + not reach with the blocks baked in this test. *) let* block, funder = context_init1 ~blocks_per_cycle:10l () in let fresh_account = Account.new_account () in let proposer = Contract.Implicit fresh_account.pkh in @@ -1660,20 +1661,6 @@ let test_valid_proposals () = (** {3 Ballot -- Negative tests} *) -(** Test that a Ballot operation fails when its source is not a - registered delegate. *) -let test_ballot_unregistered_delegate () = - let open Lwt_result_syntax in - let* block, _delegate, proposal = context_init_exploration () in - let fresh_account = Account.new_account () in - assert_validate_ballot_fails - ~expected_error:(unregistered_delegate fresh_account.pkh) - ~voter:(Contract.Implicit fresh_account.pkh) - ~proposal - ~ballot:Vote.Yay - block - __LOC__ - (** Test that a Ballot operation fails when it is unsigned. *) let test_ballot_missing_signature () = let open Lwt_result_syntax in @@ -1784,25 +1771,59 @@ let test_already_submitted_a_ballot () = __LOC__ (** Test that a Ballot operation fails when its source is not in the - vote listings. *) + vote listings (with the same error, no matter how far the source is + from being a delegate with voting rights). *) let test_ballot_source_not_in_vote_listings () = let open Lwt_result_syntax in - let* block, funder, proposal = context_init_exploration () in - let account = Account.new_account () in - let voter = Contract.Implicit account.pkh in + let* block, funder, proposal = + (* The chosen [blocks_per_cycle] is an arbitrary value that we + will not reach with the blocks baked in this test. *) + context_init_exploration ~blocks_per_cycle:10l () + in + let fresh_account = Account.new_account () in + let voter = Contract.Implicit fresh_account.pkh in + let assert_fails_with_source_not_in_vote_listings block = + assert_validate_ballot_fails + ~expected_error:source_not_in_vote_listings + ~voter + ~proposal + ~ballot:Vote.Yay + block + in + (* Fail when the source has no contract in the storage. *) + let* () = assert_fails_with_source_not_in_vote_listings block __LOC__ in let* operation = Op.transaction (B block) funder voter Tez.one in let* block = Block.bake block ~operation in - let* operation = - Op.delegation ~force_reveal:true (B block) voter (Some account.pkh) - in + (* Fail when the contract's public key is unreavealed. *) + let* () = assert_fails_with_source_not_in_vote_listings block __LOC__ in + let* operation = Op.revelation (B block) fresh_account.pk in let* block = Block.bake block ~operation in - assert_validate_ballot_fails - ~expected_error:source_not_in_vote_listings - ~voter - ~proposal - ~ballot:Vote.Yay - block - __LOC__ + (* Fail when the source is not a delegate. *) + let* () = assert_fails_with_source_not_in_vote_listings block __LOC__ in + let* operation = Op.delegation (B block) voter (Some fresh_account.pkh) in + let* block = Block.bake block ~operation in + (* Fail when the source is a delegate, but not yet in the vote listings. *) + assert_fails_with_source_not_in_vote_listings block __LOC__ + +(** Test that a Ballot operation fails when its source has already + submitted a Ballot in a previously validated operation of the + current block. *) +let test_conflicting_ballot () = + let open Lwt_result_syntax in + let* block, voter, proposal = context_init_exploration () in + let* current_block_state = Incremental.begin_construction block in + let* op_in_current_block = Op.ballot (B block) voter proposal Vote.Yay in + let* current_block_state = + Incremental.validate_operation current_block_state op_in_current_block + in + let* op = Op.ballot (B block) voter proposal Vote.Nay in + let* _i = + Incremental.validate_operation + ~expect_failure:(conflicting_ballot __LOC__) + current_block_state + op + in + return_unit (** {3 Ballot -- Positive test} @@ -1899,6 +1920,8 @@ let observe_ballot pre_state post_state op caller_loc = let test_valid_ballot () = let open Lwt_result_syntax in + (* The chosen [blocks_per_cycle] is an arbitrary value that we will + not reach with the blocks baked in this test. *) let* block, delegates = context_init ~blocks_per_cycle:10l 4 () in let* proposer, voter1, voter2, voter3 = match delegates with @@ -2020,10 +2043,6 @@ let tests = test_conflict_already_proposed; Tztest.tztest "Valid Proposals operations" `Quick test_valid_proposals; (* Validity tests on Ballot *) - Tztest.tztest - "Ballot from unregistered delegate" - `Quick - test_ballot_unregistered_delegate; Tztest.tztest "Ballot missing signature" `Quick @@ -2052,5 +2071,9 @@ let tests = "Ballot source not in vote listings" `Quick test_ballot_source_not_in_vote_listings; + Tztest.tztest + "Conflicting ballot in current block/mempool" + `Quick + test_conflicting_ballot; Tztest.tztest "Valid Ballot operations" `Quick test_valid_ballot; ] diff --git a/src/proto_alpha/lib_protocol/validate_errors.ml b/src/proto_alpha/lib_protocol/validate_errors.ml index 4e2f5ca9d6a5..52b0f78dbf07 100644 --- a/src/proto_alpha/lib_protocol/validate_errors.ml +++ b/src/proto_alpha/lib_protocol/validate_errors.ml @@ -417,6 +417,7 @@ module Voting = struct submitted : Protocol_hash.t; } | Already_submitted_a_ballot + | Conflicting_ballot of {conflicting_operation : Operation_hash.t} let () = (* Shared voting errors *) @@ -702,7 +703,27 @@ module Voting = struct ~pp:(fun ppf () -> Format.fprintf ppf "%s" description) Data_encoding.empty (function Already_submitted_a_ballot -> Some () | _ -> None) - (fun () -> Already_submitted_a_ballot) + (fun () -> Already_submitted_a_ballot) ; + register_error_kind + `Temporary + ~id:"validate_operation.conflicting_ballot" + ~title:"Conflicting ballot" + ~description: + "The delegate has already submitted a ballot in a previously validated \ + operation of the current block or mempool." + ~pp:(fun ppf conflicting_oph -> + Format.fprintf + ppf + "The delegate has already submitted a ballot in the previously \ + validated operation %a of the current block or mempool." + Operation_hash.pp + conflicting_oph) + Data_encoding.(obj1 (req "conflicting_operation" Operation_hash.encoding)) + (function + | Conflicting_ballot {conflicting_operation} -> + Some conflicting_operation + | _ -> None) + (fun conflicting_operation -> Conflicting_ballot {conflicting_operation}) end module Anonymous = struct @@ -1141,3 +1162,17 @@ module Manager = struct (function Sc_rollup_feature_disabled -> Some () | _ -> None) (fun () -> Sc_rollup_feature_disabled) end + +type error += Failing_noop_error + +let () = + let description = "A failing_noop operation can never be validated." in + register_error_kind + `Permanent + ~id:"validate_operation.failing_noop_error" + ~title:"Failing_noop error" + ~description + ~pp:(fun ppf () -> Format.fprintf ppf "%s" description) + Data_encoding.empty + (function Failing_noop_error -> Some () | _ -> None) + (fun () -> Failing_noop_error) diff --git a/src/proto_alpha/lib_protocol/validate_errors.mli b/src/proto_alpha/lib_protocol/validate_errors.mli index c781cb1aa335..2ff8a6403301 100644 --- a/src/proto_alpha/lib_protocol/validate_errors.mli +++ b/src/proto_alpha/lib_protocol/validate_errors.mli @@ -119,6 +119,7 @@ module Voting : sig submitted : Protocol_hash.t; } | Already_submitted_a_ballot + | Conflicting_ballot of {conflicting_operation : Operation_hash.t} end (** Errors that may arise while validating an anonymous operation. *) @@ -178,3 +179,5 @@ module Manager : sig | Tx_rollup_feature_disabled | Sc_rollup_feature_disabled end + +type error += Failing_noop_error diff --git a/src/proto_alpha/lib_protocol/validate_operation.ml b/src/proto_alpha/lib_protocol/validate_operation.ml index c5c5633ce216..9f7df65930db 100644 --- a/src/proto_alpha/lib_protocol/validate_operation.ml +++ b/src/proto_alpha/lib_protocol/validate_operation.ml @@ -744,6 +744,19 @@ module Voting = struct operation in return {vs with voting_state} + + let validate_ballot vi vs ~should_check_signature oph operation = + let open Lwt_tzresult_syntax in + let* voting_state = + Amendment.validate_ballot + vi.ctxt + vi.chain_id + vs.voting_state + ~should_check_signature + oph + operation + in + return {vs with voting_state} end module Anonymous = struct @@ -1508,7 +1521,7 @@ let begin_no_predecessor_info ctxt chain_id = let validate_operation (vi : validate_operation_info) (vs : validate_operation_state) ?(should_check_signature = true) oph (type kind) (operation : kind operation) = - let open Lwt_result_syntax in + let open Lwt_tzresult_syntax in let* vs = match operation.protocol_data.contents with | Single (Preendorsement _) -> @@ -1527,6 +1540,8 @@ let validate_operation (vi : validate_operation_info) operation | Single (Proposals _) -> Voting.validate_proposals vi vs ~should_check_signature oph operation + | Single (Ballot _) -> + Voting.validate_ballot vi vs ~should_check_signature oph operation | Single (Activate_account _ as contents) -> Anonymous.validate_activate_account vi vs oph contents | Single (Double_preendorsement_evidence _ as contents) -> @@ -1555,18 +1570,7 @@ let validate_operation (vi : validate_operation_info) source oph operation - | Single (Ballot _) | Single (Failing_noop _) -> - (* TODO: https://gitlab.com/tezos/tezos/-/issues/2603 - - There is no separate validation phase for non-manager - operations yet: all checks are currently done during - application in {!Apply}. - - When the validation of other operations is implemented, we - should also update - {!TMP_for_plugin.precheck_manager__do_nothing_on_non_manager_op} - (if has not been removed yet). *) - return vs + | Single (Failing_noop _) -> fail Validate_errors.Failing_noop_error in return (vs, Operation_validated_stamp) diff --git a/src/proto_alpha/lib_protocol/validate_operation.mli b/src/proto_alpha/lib_protocol/validate_operation.mli index 58bddc900af3..d4c61fb60aa9 100644 --- a/src/proto_alpha/lib_protocol/validate_operation.mli +++ b/src/proto_alpha/lib_protocol/validate_operation.mli @@ -142,6 +142,11 @@ type stamp includes it being correctly signed: indeed, we can't take anything from a manager without having checked their signature. + For non-manager operations, any error during the operation + application causes the whole block to fail. Therefore, the + validation of such an operation must ensure that its application + will fully succeed. + @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 @@ -155,15 +160,7 @@ type stamp - 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 voting operations. - (instead, the validity of a voting operation is decided by calling - {!Apply.apply_operation} to check whether it returns an error). - We should specify and implement the validation of every kind of - operation. *) + [lib_plugin/RPC.Scripts.S.run_operation]. *) val validate_operation : validate_operation_info -> validate_operation_state -> diff --git a/tests_python/tests_alpha/test_basic.py b/tests_python/tests_alpha/test_basic.py index f40471aa5bfd..54bdf0887ad6 100644 --- a/tests_python/tests_alpha/test_basic.py +++ b/tests_python/tests_alpha/test_basic.py @@ -152,7 +152,7 @@ class TestRawContext: '/chains/main/blocks/head/helpers/scripts/run_operation' ) with assert_run_failure( - 'The failing_noop operation cannot be executed by the protocol' + 'A failing_noop operation can never be validated' ): client.rpc('post', run_operation_path, data=run_json) -- GitLab From 4d24fed22f1f879e2227ca15cd20dd0683ac119c Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Wed, 27 Jul 2022 12:10:19 +0200 Subject: [PATCH 7/9] Plugin & tezt: remove mentions to #3401 Implementing the validation of voting operations has fixed #3401 ie. the run_operation RPC now correctly skips signature checks for voting operations. - Remove a FIXME in the plugin - In tezt, remove the temporary argument sign_correctly of make_run_operation_input, which is no longer needed. --- src/proto_alpha/lib_plugin/RPC.ml | 5 ----- tezt/lib_tezos/operation_core.ml | 7 +------ tezt/lib_tezos/operation_core.mli | 18 +----------------- tezt/tests/run_operation_RPC.ml | 4 +--- 4 files changed, 3 insertions(+), 31 deletions(-) diff --git a/src/proto_alpha/lib_plugin/RPC.ml b/src/proto_alpha/lib_plugin/RPC.ml index fc28a53f0c82..80323ac05ecd 100644 --- a/src/proto_alpha/lib_plugin/RPC.ml +++ b/src/proto_alpha/lib_plugin/RPC.ml @@ -373,11 +373,6 @@ 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: diff --git a/tezt/lib_tezos/operation_core.ml b/tezt/lib_tezos/operation_core.ml index 528b838d2a09..13a17cfa2f3a 100644 --- a/tezt/lib_tezos/operation_core.ml +++ b/tezt/lib_tezos/operation_core.ml @@ -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 ?(sign_correctly = false) t client = +let make_run_operation_input ?chain_id t client = let* chain_id = match chain_id with | Some chain_id -> return chain_id @@ -157,11 +157,6 @@ let make_run_operation_input ?chain_id ?(sign_correctly = false) 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 [ diff --git a/tezt/lib_tezos/operation_core.mli b/tezt/lib_tezos/operation_core.mli index fc90f67e9c26..3db2246e0e84 100644 --- a/tezt/lib_tezos/operation_core.mli +++ b/tezt/lib_tezos/operation_core.mli @@ -144,29 +144,13 @@ 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 -> - (* 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 +val make_run_operation_input : ?chain_id:string -> t -> Client.t -> JSON.u Lwt.t module Consensus : sig (** A representation of a consensus operation. *) diff --git a/tezt/tests/run_operation_RPC.ml b/tezt/tests/run_operation_RPC.ml index fd8c8c0db394..1e81839bf257 100644 --- a/tezt/tests/run_operation_RPC.ml +++ b/tezt/tests/run_operation_RPC.ml @@ -131,9 +131,7 @@ let test_run_proposals = [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 + let* op_json = Operation.make_run_operation_input 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 = -- GitLab From 12a8f1a1c0327931e87ff39097d8438d5694c04e Mon Sep 17 00:00:00 2001 From: vbot Date: Fri, 12 Aug 2022 17:21:55 +0200 Subject: [PATCH 8/9] Proto: move amendment validation to validate_operation --- src/proto_alpha/lib_protocol/TEZOS_PROTOCOL | 2 +- src/proto_alpha/lib_protocol/amendment.ml | 389 +-------------- src/proto_alpha/lib_protocol/amendment.mli | 115 +---- src/proto_alpha/lib_protocol/dune | 8 +- .../lib_protocol/validate_operation.ml | 468 +++++++++++++++++- 5 files changed, 471 insertions(+), 511 deletions(-) diff --git a/src/proto_alpha/lib_protocol/TEZOS_PROTOCOL b/src/proto_alpha/lib_protocol/TEZOS_PROTOCOL index 781e110a3ecd..97a71eb9497f 100644 --- a/src/proto_alpha/lib_protocol/TEZOS_PROTOCOL +++ b/src/proto_alpha/lib_protocol/TEZOS_PROTOCOL @@ -216,8 +216,8 @@ "Dal_apply", "Baking", - "Amendment", "Validate_errors", + "Amendment", "Validate_operation", "Apply", diff --git a/src/proto_alpha/lib_protocol/amendment.ml b/src/proto_alpha/lib_protocol/amendment.ml index 3902801242aa..08cb2a151633 100644 --- a/src/proto_alpha/lib_protocol/amendment.ml +++ b/src/proto_alpha/lib_protocol/amendment.ml @@ -148,245 +148,21 @@ let may_start_new_voting_period ctxt = Voting_period.is_last_block ctxt >>=? fun is_last -> if is_last then start_new_voting_period ctxt else return ctxt -(** {2 Validation and application of voting operations} *) +let is_testnet_dictator ctxt chain_id delegate = + (* This function should always, ALWAYS, return false on mainnet!!!! *) + match Constants.testnet_dictator ctxt with + | Some pkh when Chain_id.(chain_id <> Constants.mainnet_id) -> + Signature.Public_key_hash.equal pkh delegate + | _ -> false -open Validate_errors.Voting +(** {2 Application of voting operations} *) -(** This state is maintained in memory during the validation of a - block, or until a change of head block in mempool mode. This - state's purpose is to detect potential conflicts between the new - voting operation to validate, and all the voting operations that - have already been validated in the current block or mempool. *) -module Validation_state = struct - (** Summary of previously validated Proposals operations by a given - proposer in the current block/mempool. *) - type proposer_history = { - count : int; - (** Total number of protocols submitted by the proposer in - previously validated operations. *) - operations : Operation_hash.t list; - (** Hashes of the previously validated Proposals operations from - the proposer. *) - proposed : Operation_hash.t Protocol_hash.Map.t; - (** A map indexed by the protocols that have been submitted by the - proposer in previously validated operations. Each protocol - points to the operation in which it was proposed. *) - } - - type t = { - proposals_validated : proposer_history Signature.Public_key_hash.Map.t; - (** Summary of all Proposals operations validated in the current - block/mempool, indexed by the operation's source aka - proposer. *) - dictator_proposals_validated : Operation_hash.t option; - (** If a testnet dictotor Proposals operation has been validated - in the current block/mempool, then its hash is recorded - here. Since such an operation can change the voting period - kind, it is mutually exclusive with any other voting operation - in a single block (otherwise we would loose the commutativity - of validated operation application: see - {!Validate_operation}). *) - ballots_validated : Operation_hash.t Signature.Public_key_hash.Map.t; - (** To each delegate that has submitted a ballot in a previously - validated operation, associates the hash of this operation. *) - } - - let empty = - { - proposals_validated = Signature.Public_key_hash.Map.empty; - dictator_proposals_validated = None; - ballots_validated = Signature.Public_key_hash.Map.empty; - } - - let check_count_conflict ~count_previous_blocks ~count_operation - proposer_history = - let max_allowed = Constants.max_proposals_per_delegate in - let count_before_op = count_previous_blocks + proposer_history.count in - (* [count_before_op] should never have been increased above - [max_proposals_per_delegate]. *) - assert (Compare.Int.(count_before_op <= max_allowed)) ; - error_unless - Compare.Int.(count_before_op + count_operation <= max_allowed) - (Conflict_too_many_proposals - { - max_allowed; - count_previous_blocks; - count_current_block = proposer_history.count; - count_operation; - conflicting_operations = proposer_history.operations; - }) - - (** Check that a regular (ie. non-dictator) Proposals operation is - compatible with previously validated voting operations in the - current block/mempool, and update the [state] with this - operation. - - @return [Error Conflict_too_many_proposals] if the total number - of proposals by [proposer] in previously applied operations in - [ctxt], in previously validated operations in the current - block/mempool, and in the operation to validate, exceeds - {!Constants.max_proposals_per_delegate}. - - @return [Error Conflict_already_proposed] if one of the - operation's [proposals] has already been submitted by [proposer] - in the current block/mempool. - - @return [Error Conflicting_dictator_proposals] if the current - block/mempool already contains a testnet dictator Proposals - operation (see {!recfield:dictator_proposals_validated}). - - Note that this function is designed to be called in addition to - {!check_proposal_list_sanity} and {!check_count} further below, - not instead of them: that's why nothing is done when the - [proposer] is not in {!recfield:proposals_validated}. More - precisely, this function should be called {e after} the - aforementioned functions, whose potential errors - e.g. [Proposals_contain_duplicate] or [Too_many_proposals] should - take precedence because they are independent from the validation - [state]. *) - let check_proposals_and_update (state : t) oph proposer proposals - ~count_in_ctxt ~proposals_length = - let open Tzresult_syntax in - let* new_proposer_history = - match - Signature.Public_key_hash.Map.find proposer state.proposals_validated - with - | None -> - let proposed = - List.fold_left - (fun acc proposal -> Protocol_hash.Map.add proposal oph acc) - Protocol_hash.Map.empty - proposals - in - return {count = proposals_length; operations = [oph]; proposed} - | Some proposer_history -> - let* () = - check_count_conflict - ~count_previous_blocks:count_in_ctxt - ~count_operation:proposals_length - proposer_history - in - let add_proposal proposed_map proposal = - match Protocol_hash.Map.find proposal proposer_history.proposed with - | Some conflicting_operation -> - error - (Conflict_already_proposed {proposal; conflicting_operation}) - | None -> ok (Protocol_hash.Map.add proposal oph proposed_map) - in - let* proposed = - List.fold_left_e add_proposal proposer_history.proposed proposals - in - return - { - count = proposer_history.count + proposals_length; - operations = oph :: proposer_history.operations; - proposed; - } - in - let* () = - match state.dictator_proposals_validated with - | None -> ok () - | Some dictator_oph -> error (Conflicting_dictator_proposals dictator_oph) - in - let proposals_validated = - Signature.Public_key_hash.Map.add - proposer - new_proposer_history - state.proposals_validated - in - return {state with proposals_validated} - - (** Check that a Proposals operation from a testnet dictator is - compatible with previously validated voting operations in the - current block/mempool (ie. that no other voting operation has - been validated), and update the [state] with this operation. - - @return [Error Testnet_dictator_conflicting_operation] if the - current block or mempool already contains any validated voting - operation. *) - let check_dictator_proposals_and_update state oph = - let open Tzresult_syntax in - let* () = - error_unless - (Signature.Public_key_hash.Map.is_empty state.proposals_validated - && Option.is_none state.dictator_proposals_validated - && Signature.Public_key_hash.Map.is_empty state.ballots_validated) - Testnet_dictator_conflicting_operation - in - return {state with dictator_proposals_validated = Some oph} - - (** Check that a Ballot operation is compatible with previously - validated voting operations in the current block/mempool. - - @return [Error Conflicting_ballot] if the [delegate] has already - submitted a ballot in the current block/mempool. - - @return [Error Conflicting_dictator_proposals] if the current - block/mempool already contains a testnet dictator Proposals - operation (see {!recfield:dictator_proposals_validated}). *) - let check_ballot state voter = - let open Tzresult_syntax in - let* () = - match - Signature.Public_key_hash.Map.find voter state.ballots_validated - with - | None -> ok () - | Some conflicting_operation -> - error (Conflicting_ballot {conflicting_operation}) - in - match state.dictator_proposals_validated with - | None -> ok () - | Some dictator_oph -> error (Conflicting_dictator_proposals dictator_oph) - - (** Update the [state] when a Ballot operation is validated. *) - let update_on_ballot state oph voter = - let ballots_validated = - Signature.Public_key_hash.Map.add voter oph state.ballots_validated - in - {state with ballots_validated} -end - -(** Helpers to validate and apply Proposals operations from a +(** Helpers to apply [Proposals] operations from a registered dictator of a test chain. These operations let the dictator immediately change the current voting period's kind, and the current proposal if applicable. Of course, there must never be such a dictator on mainnet. *) module Testnet_dictator = struct - let is_testnet_dictator ctxt chain_id delegate = - (* This function should always, ALWAYS, return false on mainnet!!!! *) - match Constants.testnet_dictator ctxt with - | Some pkh when Chain_id.(chain_id <> Constants.mainnet_id) -> - Signature.Public_key_hash.equal pkh delegate - | _ -> false - - (** Check that [record_proposals] below will not fail. - - This function is designed to be exclusively called by - [validate_proposals] further down this file. - - @return [Error Multiple_proposals] if [proposals] has more than - one element. *) - let check_proposals chain_id proposals = - (* This assertion should be ensured by the fact that - {!is_testnet_dictator} cannot be [true] on mainnet, but we - double check it because it is critical. *) - assert (Chain_id.(chain_id <> Constants.mainnet_id)) ; - match proposals with - | [] | [_] -> - (* In [record_proposals] below, the call to - {!Vote.init_current_proposal} (in the singleton list case) - cannot fail because {!Vote.clear_current_proposal} is called - right before. - - The calls to - {!Voting_period.Testnet_dictator.overwrite_current_kind} may - usually fail when the voting period is not - initialized. However, this cannot happen because the current - function is only called in [validate_proposals] after a - successful call to {!Voting_period.get_current}. *) - ok () - | _ :: _ :: _ -> error Testnet_dictator_multiple_proposals - (** Forcibly update the voting period according to a voting dictator's Proposals operation. @@ -409,110 +185,16 @@ module Testnet_dictator = struct ctxt chain_id Adoption - | _ :: _ :: _ -> fail Testnet_dictator_multiple_proposals + | _ :: _ :: _ -> + (* This does not fail if validate proposal was previously + called. *) + fail Validate_errors.Voting.Testnet_dictator_multiple_proposals end -let check_period_index ~expected period_index = - error_unless - Compare.Int32.(expected = period_index) - (Wrong_voting_period_index {expected; provided = period_index}) - -(** Check that the list of proposals is not empty and does not contain - duplicates. *) -let check_proposal_list_sanity proposals = - let open Tzresult_syntax in - let* () = - match proposals with [] -> error Empty_proposals | _ :: _ -> ok () - in - let* (_ : Protocol_hash.Set.t) = - List.fold_left_e - (fun previous_elements proposal -> - let* () = - error_when - (Protocol_hash.Set.mem proposal previous_elements) - (Proposals_contain_duplicate {proposal}) - in - return (Protocol_hash.Set.add proposal previous_elements)) - Protocol_hash.Set.empty - proposals - in - return_unit - -let check_period_kind_for_proposals current_period = - match current_period.Voting_period.kind with - | Proposal -> ok () - | (Exploration | Cooldown | Promotion | Adoption) as current -> - error (Wrong_voting_period_kind {current; expected = [Proposal]}) - -let check_in_listings ctxt source = - let open Lwt_tzresult_syntax in - let*! in_listings = Vote.in_listings ctxt source in - fail_unless in_listings Source_not_in_vote_listings - -let check_count ~count_in_ctxt ~proposals_length = - (* The proposal count of the proposer in the context should never - have been increased above [max_proposals_per_delegate]. *) - assert (Compare.Int.(count_in_ctxt <= Constants.max_proposals_per_delegate)) ; - error_unless - Compare.Int.( - count_in_ctxt + proposals_length <= Constants.max_proposals_per_delegate) - Too_many_proposals - -let check_already_proposed ctxt proposer proposals = - let open Lwt_tzresult_syntax in - List.iter_es - (fun proposal -> - let*! already_proposed = Vote.has_proposed ctxt proposer proposal in - fail_when already_proposed (Already_proposed {proposal})) - proposals - -let validate_proposals ctxt chain_id state ~should_check_signature oph - (operation : Kind.proposals operation) = - let open Lwt_tzresult_syntax in - let (Single (Proposals {source; period; proposals})) = - operation.protocol_data.contents - in - let* current_period = Voting_period.get_current ctxt in - let*? () = check_period_index ~expected:current_period.index period in - let* state = - if Testnet_dictator.is_testnet_dictator ctxt chain_id source then - let*? () = Testnet_dictator.check_proposals chain_id proposals in - Lwt.return - (Validation_state.check_dictator_proposals_and_update state oph) - else - let*? () = check_proposal_list_sanity proposals in - let*? () = check_period_kind_for_proposals current_period in - let* () = check_in_listings ctxt source in - let* count_in_ctxt = Vote.get_delegate_proposal_count ctxt source in - let proposals_length = List.length proposals in - let*? () = check_count ~count_in_ctxt ~proposals_length in - let* () = check_already_proposed ctxt source proposals in - Lwt.return - (Validation_state.check_proposals_and_update - state - oph - source - proposals - ~count_in_ctxt - ~proposals_length) - in - (* The signature check is done last because it is more costly than - most checks. *) - let* () = - when_ should_check_signature (fun () -> - (* Retrieving the public key cannot fail. Indeed, we have - already checked that the delegate is in the vote listings - (or is a testnet dictator), which implies that it is a - manager with a revealed key. *) - let* public_key = Contract.get_manager_key ctxt source in - Lwt.return (Operation.check_signature public_key chain_id operation)) - in - return state - let apply_proposals ctxt chain_id (Proposals {source; period = _; proposals}) = let open Lwt_tzresult_syntax in let* ctxt = - if Testnet_dictator.is_testnet_dictator ctxt chain_id source then + if is_testnet_dictator ctxt chain_id source then Testnet_dictator.record_proposals ctxt chain_id proposals else let* count = Vote.get_delegate_proposal_count ctxt source in @@ -528,51 +210,6 @@ let apply_proposals ctxt chain_id (Proposals {source; period = _; proposals}) = in return (ctxt, Apply_results.Single_result Proposals_result) -let check_period_kind_for_ballot current_period = - match current_period.Voting_period.kind with - | Exploration | Promotion -> ok () - | (Cooldown | Proposal | Adoption) as current -> - error - (Wrong_voting_period_kind {current; expected = [Exploration; Promotion]}) - -let check_current_proposal ctxt op_proposal = - let open Lwt_tzresult_syntax in - let* current_proposal = Vote.get_current_proposal ctxt in - fail_unless - (Protocol_hash.equal op_proposal current_proposal) - (Ballot_for_wrong_proposal - {current = current_proposal; submitted = op_proposal}) - -let check_source_has_not_already_voted ctxt source = - let open Lwt_tzresult_syntax in - let*! has_ballot = Vote.has_recorded_ballot ctxt source in - fail_when has_ballot Already_submitted_a_ballot - -let validate_ballot ctxt chain_id state ~should_check_signature oph - (operation : Kind.ballot operation) = - let open Lwt_tzresult_syntax in - let (Single (Ballot {source; period; proposal; ballot = _})) = - operation.protocol_data.contents - in - let*? () = Validation_state.check_ballot state source in - let* current_period = Voting_period.get_current ctxt in - let*? () = check_period_index ~expected:current_period.index period in - let*? () = check_period_kind_for_ballot current_period in - let* () = check_current_proposal ctxt proposal in - let* () = check_source_has_not_already_voted ctxt source in - let* () = check_in_listings ctxt source in - (* The signature check is done last because it is more costly than - most checks. *) - let* () = - when_ should_check_signature (fun () -> - (* Retrieving the public key cannot fail. Indeed, we have - already checked that the delegate is in the vote listings, - which implies that it is a manager with a revealed key. *) - let* public_key = Contract.get_manager_key ctxt source in - Lwt.return (Operation.check_signature public_key chain_id operation)) - in - return (Validation_state.update_on_ballot state oph source) - let apply_ballot ctxt (Ballot {source; period = _; proposal = _; ballot}) = let open Lwt_tzresult_syntax in let* ctxt = Vote.record_ballot ctxt source ballot in diff --git a/src/proto_alpha/lib_protocol/amendment.mli b/src/proto_alpha/lib_protocol/amendment.mli index a843efa26b74..328b30691826 100644 --- a/src/proto_alpha/lib_protocol/amendment.mli +++ b/src/proto_alpha/lib_protocol/amendment.mli @@ -74,7 +74,12 @@ open Alpha_context the state machine of the amendment procedure. *) val may_start_new_voting_period : context -> context tzresult Lwt.t -(** {2 Validation and application of voting operations} +(** Check whether the given public key hash corresponds to the + registered testchain dictator, if any. This function will always + return false on mainnet. *) +val is_testnet_dictator : context -> Chain_id.t -> public_key_hash -> bool + +(** {2 Application of voting operations} There are two kinds of voting operations: @@ -86,77 +91,6 @@ val may_start_new_voting_period : context -> context tzresult Lwt.t (or pass). This operation is only accepted during an Exploration or Promotion period (see above). *) -(** A state containing a summary of previously validated voting - operations. It should be maintained in memory during the validation - of a block, or until a change of head block in mempool mode. It is - used to check for conflicts between an operation to validate and - the already validated operations of the current block/mempool. *) -module Validation_state : sig - (** A state as described right above. *) - type t - - (** The empty state (for the initialization of a new block or mempool). *) - val empty : t -end - -(** Check that a Proposals operation can be safely applied. - - @return [Error Wrong_voting_period_index] if the operation's - period and the [context]'s current period do not have the same - index. - - @return [Error Empty_proposals] if the list of proposals is empty. - - @return [Error Proposals_contain_duplicate] if the list of - proposals contains a duplicate element. - - @return [Error Wrong_voting_period_kind] if the voting period is - not of the Proposal kind. - - @return [Error Source_not_in_vote_listings] if the source is not - in the vote listings. - - @return [Error Too_many_proposals] if the operation would make the - source's total number of proposals exceed - {!Constants.recorded_proposal_count_for_delegate}. - - @return [Error Already_proposed] if one of the proposals has - already been proposed by the source. - - @return [Error Conflict_too_many_proposals] if the total count of - proposals submitted by the source in previous blocks, in previously - validated operations of the current block/mempool, and in the - operation to validate, exceeds - {!Constants.max_proposals_per_delegate}. - - @return [Error Conflict_already_proposed] if one of the - operation's proposals has already been submitted by the source in - the current block/mempool. - - @return [Error Conflicting_dictator_proposals] if a testnet - dictator Proposals operation has already been validated in the - current block/mempool. - - @return [Error Testnet_dictator_multiple_proposals] if the source - is a testnet dictator and the operation contains more than one - proposal. - - @return [Error Testnet_dictator_conflicting_operation] if the - source is a testnet dictator and the current block or mempool - already contains any validated voting operation. - - @return [Error Operation.Missing_signature] or [Error - Operation.Invalid_signature] if the operation is unsigned or - incorrectly signed. *) -val validate_proposals : - context -> - Chain_id.t -> - Validation_state.t -> - should_check_signature:bool -> - Operation_hash.t -> - Kind.proposals operation -> - Validation_state.t tzresult Lwt.t - (** Update the [context] with the effects of a Proposals operation: - Its proposals are added to the source's recorded proposals. @@ -183,43 +117,6 @@ val apply_proposals : Kind.proposals contents -> (context * Kind.proposals Apply_results.contents_result_list) tzresult Lwt.t -(** Check that a Ballot operation can be safely applied. - - @return [Error Conflicting_ballot] if the source has already - submitted a ballot in the current block/mempool. - - @return [Error Conflicting_dictator_proposals] if the current - block/mempool already contains a validated testnet dictator - Proposals operation. - - @return [Error Wrong_voting_period_index] if the operation's - period and the [context]'s current period do not have the same - index. - - @return [Error Wrong_voting_period_kind] if the voting period is - not of the Exploration or Promotion kind. - - @return [Error Ballot_for_wrong_proposal] if the operation's - proposal is different from the [context]'s current proposal. - - @return [Error Already_submitted_a_ballot] if the source has - already voted. - - @return [Error Source_not_in_vote_listings] if the source is not - in the vote listings. - - @return [Error Operation.Missing_signature] or [Error - Operation.Invalid_signature] if the operation is unsigned or - incorrectly signed. *) -val validate_ballot : - context -> - Chain_id.t -> - Validation_state.t -> - should_check_signature:bool -> - Operation_hash.t -> - Kind.ballot operation -> - Validation_state.t tzresult Lwt.t - (** Update the [context] with the effects of a Ballot operation: The couple (source of the operation, submitted ballot) is recorded. diff --git a/src/proto_alpha/lib_protocol/dune b/src/proto_alpha/lib_protocol/dune index 5bb4173c8a56..1ca2458363a8 100644 --- a/src/proto_alpha/lib_protocol/dune +++ b/src/proto_alpha/lib_protocol/dune @@ -227,8 +227,8 @@ Sc_rollup_operations Dal_apply Baking - Amendment Validate_errors + Amendment Validate_operation Apply Services_registration @@ -473,8 +473,8 @@ sc_rollup_operations.ml sc_rollup_operations.mli dal_apply.ml dal_apply.mli baking.ml baking.mli - amendment.ml amendment.mli validate_errors.ml validate_errors.mli + amendment.ml amendment.mli validate_operation.ml validate_operation.mli apply.ml apply.mli services_registration.ml services_registration.mli @@ -699,8 +699,8 @@ sc_rollup_operations.ml sc_rollup_operations.mli dal_apply.ml dal_apply.mli baking.ml baking.mli - amendment.ml amendment.mli validate_errors.ml validate_errors.mli + amendment.ml amendment.mli validate_operation.ml validate_operation.mli apply.ml apply.mli services_registration.ml services_registration.mli @@ -930,8 +930,8 @@ sc_rollup_operations.ml sc_rollup_operations.mli dal_apply.ml dal_apply.mli baking.ml baking.mli - amendment.ml amendment.mli validate_errors.ml validate_errors.mli + amendment.ml amendment.mli validate_operation.ml validate_operation.mli apply.ml apply.mli services_registration.ml services_registration.mli diff --git a/src/proto_alpha/lib_protocol/validate_operation.ml b/src/proto_alpha/lib_protocol/validate_operation.ml index 9f7df65930db..522b12506d02 100644 --- a/src/proto_alpha/lib_protocol/validate_operation.ml +++ b/src/proto_alpha/lib_protocol/validate_operation.ml @@ -130,6 +130,46 @@ let empty_consensus_state = dal_slot_availability_seen = Signature.Public_key_hash.Set.empty; } +(** Summary of previously validated Proposals operations by a given + proposer in the current block/mempool. *) +type proposer_history = { + count : int; + (** Total number of protocols submitted by the proposer in + previously validated operations. *) + operations : Operation_hash.t list; + (** Hashes of the previously validated Proposals operations from + the proposer. *) + proposed : Operation_hash.t Protocol_hash.Map.t; + (** A map indexed by the protocols that have been submitted by the + proposer in previously validated operations. Each protocol + points to the operation in which it was proposed. *) +} + +type voting_state = { + proposals_validated : proposer_history Signature.Public_key_hash.Map.t; + (** Summary of all Proposals operations validated in the current + block/mempool, indexed by the operation's source aka + proposer. *) + dictator_proposals_validated : Operation_hash.t option; + (** If a testnet dictator Proposals operation has been validated + in the current block/mempool, then its hash is recorded + here. Since such an operation can change the voting period + kind, it is mutually exclusive with any other voting operation + in a single block (otherwise we would loose the commutativity + of validated operation application: see + {!Validate_operation}). *) + ballots_validated : Operation_hash.t Signature.Public_key_hash.Map.t; + (** To each delegate that has submitted a ballot in a previously + validated operation, associates the hash of this operation. *) +} + +let empty_voting_state = + { + proposals_validated = Signature.Public_key_hash.Map.empty; + dictator_proposals_validated = None; + ballots_validated = Signature.Public_key_hash.Map.empty; + } + module Double_evidence = Map.Make (struct type t = Signature.Public_key_hash.t * Level.t @@ -232,7 +272,7 @@ type validate_operation_info = { type validate_operation_state = { consensus_state : consensus_state; - voting_state : Amendment.Validation_state.t; + voting_state : voting_state; anonymous_state : anonymous_state; manager_state : manager_state; } @@ -252,7 +292,7 @@ let init_validate_operation_info ctxt mode chain_id let init_validate_operation_state ctxt = { consensus_state = empty_consensus_state; - voting_state = Amendment.Validation_state.empty; + voting_state = empty_voting_state; anonymous_state = empty_anonymous_state; manager_state = init_manager_state ctxt; } @@ -729,33 +769,419 @@ module Consensus = struct return (update_validity_state_dal_slot_availabitiy vs endorser) end -(** Validation of voting operations: proposals and ballot. - (Validation pass [1]) *) +(** {2 Validation of voting operations} + + There are two kinds of voting operations: + + - Proposals: A delegate submits a list of protocol amendment + proposals. This operation is only accepted during a Proposal period + (see above). + + - Ballot: A delegate casts a vote for/against the current proposal + (or pass). This operation is only accepted during an Exploration + or Promotion period (see above). *) + module Voting = struct - let validate_proposals vi vs ~should_check_signature oph operation = + open Validate_errors.Voting + + let check_count_conflict ~count_previous_blocks ~count_operation + proposer_history = + let max_allowed = Constants.max_proposals_per_delegate in + let count_before_op = count_previous_blocks + proposer_history.count in + (* [count_before_op] should never have been increased above + [max_proposals_per_delegate]. *) + assert (Compare.Int.(count_before_op <= max_allowed)) ; + error_unless + Compare.Int.(count_before_op + count_operation <= max_allowed) + (Conflict_too_many_proposals + { + max_allowed; + count_previous_blocks; + count_current_block = proposer_history.count; + count_operation; + conflicting_operations = proposer_history.operations; + }) + + (** Check that a regular (ie. non-dictator) Proposals operation is + compatible with previously validated voting operations in the + current block/mempool, and update the [state] with this + operation. + + @return [Error Conflict_too_many_proposals] if the total number + of proposals by [proposer] in previously applied operations in + [ctxt], in previously validated operations in the current + block/mempool, and in the operation to validate, exceeds + {!Constants.max_proposals_per_delegate}. + + @return [Error Conflict_already_proposed] if one of the + operation's [proposals] has already been submitted by [proposer] + in the current block/mempool. + + @return [Error Conflicting_dictator_proposals] if the current + block/mempool already contains a testnet dictator Proposals + operation (see {!recfield:dictator_proposals_validated}). + + Note that this function is designed to be called in addition to + {!check_proposal_list_sanity} and {!check_count} further below, + not instead of them: that's why nothing is done when the + [proposer] is not in {!recfield:proposals_validated}. More + precisely, this function should be called {e after} the + aforementioned functions, whose potential errors + e.g. [Proposals_contain_duplicate] or [Too_many_proposals] should + take precedence because they are independent from the validation + [state]. *) + let check_proposals_conflicts_and_update_state state oph proposer proposals + ~count_in_ctxt ~proposals_length = + let open Tzresult_syntax in + let* new_proposer_history = + match + Signature.Public_key_hash.Map.find proposer state.proposals_validated + with + | None -> + let proposed = + List.fold_left + (fun acc proposal -> Protocol_hash.Map.add proposal oph acc) + Protocol_hash.Map.empty + proposals + in + return {count = proposals_length; operations = [oph]; proposed} + | Some proposer_history -> + let* () = + check_count_conflict + ~count_previous_blocks:count_in_ctxt + ~count_operation:proposals_length + proposer_history + in + let add_proposal proposed_map proposal = + match Protocol_hash.Map.find proposal proposer_history.proposed with + | Some conflicting_operation -> + error + (Conflict_already_proposed {proposal; conflicting_operation}) + | None -> ok (Protocol_hash.Map.add proposal oph proposed_map) + in + let* proposed = + List.fold_left_e add_proposal proposer_history.proposed proposals + in + return + { + count = proposer_history.count + proposals_length; + operations = oph :: proposer_history.operations; + proposed; + } + in + let* () = + match state.dictator_proposals_validated with + | None -> ok () + | Some dictator_oph -> error (Conflicting_dictator_proposals dictator_oph) + in + let proposals_validated = + Signature.Public_key_hash.Map.add + proposer + new_proposer_history + state.proposals_validated + in + return {state with proposals_validated} + + (** Check that a Proposals operation from a testnet dictator is + compatible with previously validated voting operations in the + current block/mempool (ie. that no other voting operation has + been validated), and update the [state] with this operation. + + @return [Error Testnet_dictator_conflicting_operation] if the + current block or mempool already contains any validated voting + operation. *) + let check_dictator_proposals_conflicts_and_update_state state oph = + let open Tzresult_syntax in + let* () = + error_unless + (Signature.Public_key_hash.Map.is_empty state.proposals_validated + && Option.is_none state.dictator_proposals_validated + && Signature.Public_key_hash.Map.is_empty state.ballots_validated) + Testnet_dictator_conflicting_operation + in + return {state with dictator_proposals_validated = Some oph} + + (** Check that a Ballot operation is compatible with previously + validated voting operations in the current block/mempool. + + @return [Error Conflicting_ballot] if the [delegate] has already + submitted a ballot in the current block/mempool. + + @return [Error Conflicting_dictator_proposals] if the current + block/mempool already contains a testnet dictator Proposals + operation (see {!recfield:dictator_proposals_validated}). *) + let check_ballot_conflicts state voter = + let open Tzresult_syntax in + let* () = + match + Signature.Public_key_hash.Map.find voter state.ballots_validated + with + | None -> ok () + | Some conflicting_operation -> + error (Conflicting_ballot {conflicting_operation}) + in + match state.dictator_proposals_validated with + | None -> ok () + | Some dictator_oph -> error (Conflicting_dictator_proposals dictator_oph) + + (** Update the [state] when a Ballot operation is validated. *) + let update_state_on_ballot state oph voter = + let ballots_validated = + Signature.Public_key_hash.Map.add voter oph state.ballots_validated + in + {state with ballots_validated} + + (** Check that [record_proposals] below will not fail. + + This function is designed to be exclusively called by + [validate_proposals] further down this file. + + @return [Error Multiple_proposals] if [proposals] has more than + one element. *) + let check_testnet_dictator_proposals chain_id proposals = + (* This assertion should be ensured by the fact that + {!is_testnet_dictator} cannot be [true] on mainnet, but we + double check it because it is critical. *) + assert (Chain_id.(chain_id <> Constants.mainnet_id)) ; + match proposals with + | [] | [_] -> + (* In [record_proposals] below, the call to + {!Vote.init_current_proposal} (in the singleton list case) + cannot fail because {!Vote.clear_current_proposal} is called + right before. + + The calls to + {!Voting_period.Testnet_dictator.overwrite_current_kind} may + usually fail when the voting period is not + initialized. However, this cannot happen because the current + function is only called in [validate_proposals] after a + successful call to {!Voting_period.get_current}. *) + ok () + | _ :: _ :: _ -> error Testnet_dictator_multiple_proposals + + let check_period_index ~expected period_index = + error_unless + Compare.Int32.(expected = period_index) + (Wrong_voting_period_index {expected; provided = period_index}) + + (** Check that the list of proposals is not empty and does not contain + duplicates. *) + let check_proposal_list_sanity proposals = + let open Tzresult_syntax in + let* () = + match proposals with [] -> error Empty_proposals | _ :: _ -> ok () + in + let* (_ : Protocol_hash.Set.t) = + List.fold_left_e + (fun previous_elements proposal -> + let* () = + error_when + (Protocol_hash.Set.mem proposal previous_elements) + (Proposals_contain_duplicate {proposal}) + in + return (Protocol_hash.Set.add proposal previous_elements)) + Protocol_hash.Set.empty + proposals + in + return_unit + + let check_period_kind_for_proposals current_period = + match current_period.Voting_period.kind with + | Proposal -> ok () + | (Exploration | Cooldown | Promotion | Adoption) as current -> + error (Wrong_voting_period_kind {current; expected = [Proposal]}) + + let check_in_listings ctxt source = + let open Lwt_tzresult_syntax in + let*! in_listings = Vote.in_listings ctxt source in + fail_unless in_listings Source_not_in_vote_listings + + let check_count ~count_in_ctxt ~proposals_length = + (* The proposal count of the proposer in the context should never + have been increased above [max_proposals_per_delegate]. *) + assert (Compare.Int.(count_in_ctxt <= Constants.max_proposals_per_delegate)) ; + error_unless + Compare.Int.( + count_in_ctxt + proposals_length <= Constants.max_proposals_per_delegate) + Too_many_proposals + + let check_already_proposed ctxt proposer proposals = + let open Lwt_tzresult_syntax in + List.iter_es + (fun proposal -> + let*! already_proposed = Vote.has_proposed ctxt proposer proposal in + fail_when already_proposed (Already_proposed {proposal})) + proposals + + let check_period_kind_for_ballot current_period = + match current_period.Voting_period.kind with + | Exploration | Promotion -> ok () + | (Cooldown | Proposal | Adoption) as current -> + error + (Wrong_voting_period_kind + {current; expected = [Exploration; Promotion]}) + + let check_current_proposal ctxt op_proposal = + let open Lwt_tzresult_syntax in + let* current_proposal = Vote.get_current_proposal ctxt in + fail_unless + (Protocol_hash.equal op_proposal current_proposal) + (Ballot_for_wrong_proposal + {current = current_proposal; submitted = op_proposal}) + + let check_source_has_not_already_voted ctxt source = let open Lwt_tzresult_syntax in + let*! has_ballot = Vote.has_recorded_ballot ctxt source in + fail_when has_ballot Already_submitted_a_ballot + + (** Check that a Proposals operation can be safely applied. + + @return [Error Wrong_voting_period_index] if the operation's + period and the [context]'s current period do not have the same + index. + + @return [Error Empty_proposals] if the list of proposals is empty. + + @return [Error Proposals_contain_duplicate] if the list of + proposals contains a duplicate element. + + @return [Error Wrong_voting_period_kind] if the voting period is + not of the Proposal kind. + + @return [Error Source_not_in_vote_listings] if the source is not + in the vote listings. + + @return [Error Too_many_proposals] if the operation would make the + source's total number of proposals exceed + {!Constants.recorded_proposal_count_for_delegate}. + + @return [Error Already_proposed] if one of the proposals has + already been proposed by the source. + + @return [Error Conflict_too_many_proposals] if the total count of + proposals submitted by the source in previous blocks, in previously + validated operations of the current block/mempool, and in the + operation to validate, exceeds + {!Constants.max_proposals_per_delegate}. + + @return [Error Conflict_already_proposed] if one of the + operation's proposals has already been submitted by the source in + the current block/mempool. + + @return [Error Conflicting_dictator_proposals] if a testnet + dictator Proposals operation has already been validated in the + current block/mempool. + + @return [Error Testnet_dictator_multiple_proposals] if the source + is a testnet dictator and the operation contains more than one + proposal. + + @return [Error Testnet_dictator_conflicting_operation] if the + source is a testnet dictator and the current block or mempool + already contains any validated voting operation. + + @return [Error Operation.Missing_signature] or [Error + Operation.Invalid_signature] if the operation is unsigned or + incorrectly signed. *) + let validate_proposals vi vs ~should_check_signature oph + (operation : Kind.proposals operation) = + let open Lwt_tzresult_syntax in + let (Single (Proposals {source; period; proposals})) = + operation.protocol_data.contents + in + let* current_period = Voting_period.get_current vi.ctxt in + let*? () = check_period_index ~expected:current_period.index period in let* voting_state = - Amendment.validate_proposals - vi.ctxt - vi.chain_id - vs.voting_state - ~should_check_signature - oph - operation + if Amendment.is_testnet_dictator vi.ctxt vi.chain_id source then + let*? () = check_testnet_dictator_proposals vi.chain_id proposals in + Lwt.return + (check_dictator_proposals_conflicts_and_update_state + vs.voting_state + oph) + else + let*? () = check_proposal_list_sanity proposals in + let*? () = check_period_kind_for_proposals current_period in + let* () = check_in_listings vi.ctxt source in + let* count_in_ctxt = Vote.get_delegate_proposal_count vi.ctxt source in + let proposals_length = List.length proposals in + let*? () = check_count ~count_in_ctxt ~proposals_length in + let* () = check_already_proposed vi.ctxt source proposals in + Lwt.return + (check_proposals_conflicts_and_update_state + vs.voting_state + oph + source + proposals + ~count_in_ctxt + ~proposals_length) + in + (* The signature check is done last because it is more costly than + most checks. *) + let* () = + when_ should_check_signature (fun () -> + (* Retrieving the public key cannot fail. Indeed, we have + already checked that the delegate is in the vote listings + (or is a testnet dictator), which implies that it is a + manager with a revealed key. *) + let* public_key = Contract.get_manager_key vi.ctxt source in + Lwt.return + (Operation.check_signature public_key vi.chain_id operation)) in return {vs with voting_state} - let validate_ballot vi vs ~should_check_signature oph operation = + (** Check that a Ballot operation can be safely applied. + + @return [Error Conflicting_ballot] if the source has already + submitted a ballot in the current block/mempool. + + @return [Error Conflicting_dictator_proposals] if the current + block/mempool already contains a validated testnet dictator + Proposals operation. + + @return [Error Wrong_voting_period_index] if the operation's + period and the [context]'s current period do not have the same + index. + + @return [Error Wrong_voting_period_kind] if the voting period is + not of the Exploration or Promotion kind. + + @return [Error Ballot_for_wrong_proposal] if the operation's + proposal is different from the [context]'s current proposal. + + @return [Error Already_submitted_a_ballot] if the source has + already voted. + + @return [Error Source_not_in_vote_listings] if the source is not + in the vote listings. + + @return [Error Operation.Missing_signature] or [Error + Operation.Invalid_signature] if the operation is unsigned or + incorrectly signed. *) + let validate_ballot vi vs ~should_check_signature oph + (operation : Kind.ballot operation) = let open Lwt_tzresult_syntax in - let* voting_state = - Amendment.validate_ballot - vi.ctxt - vi.chain_id - vs.voting_state - ~should_check_signature - oph - operation + let (Single (Ballot {source; period; proposal; ballot = _})) = + operation.protocol_data.contents in + let*? () = check_ballot_conflicts vs.voting_state source in + let* current_period = Voting_period.get_current vi.ctxt in + let*? () = check_period_index ~expected:current_period.index period in + let*? () = check_period_kind_for_ballot current_period in + let* () = check_current_proposal vi.ctxt proposal in + let* () = check_source_has_not_already_voted vi.ctxt source in + let* () = check_in_listings vi.ctxt source in + (* The signature check is done last because it is more costly than + most checks. *) + let* () = + when_ should_check_signature (fun () -> + (* Retrieving the public key cannot fail. Indeed, we have + already checked that the delegate is in the vote listings, + which implies that it is a manager with a revealed key. *) + let* public_key = Contract.get_manager_key vi.ctxt source in + Lwt.return + (Operation.check_signature public_key vi.chain_id operation)) + in + let voting_state = update_state_on_ballot vs.voting_state oph source in return {vs with voting_state} end -- GitLab From 9537681182e83a308bbb7f8113e023143f2206bc Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Mon, 1 Aug 2022 17:45:21 +0200 Subject: [PATCH 9/9] docs/protocols/alpha: update the changelog --- docs/protocols/alpha.rst | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/docs/protocols/alpha.rst b/docs/protocols/alpha.rst index cb81495d1cdb..54bf92b78aa7 100644 --- a/docs/protocols/alpha.rst +++ b/docs/protocols/alpha.rst @@ -87,6 +87,18 @@ Minor Changes - Errors related to consensus operations have been reworked. See ``Validate_errors.Consensus``. (MR :gl:`!5927`) +- A delegate can no longer propose the same protocol hash multiple + times in Proposals operations. An operation that contains a proposal + which has already been proposed by the delegate in the same voting + period will now fail (and so will an operation that contains + multiple occurrences of the same proposal). This prevents the replay + of a Proposals operation. (MR :gl:`!5828`) + +- Change the names and types of errors related to voting operations + (Proposals and Ballot), and move them to ``Validate_errors``. + (MR :gl:`!5828`) + + Internal -------- @@ -126,3 +138,19 @@ Internal - Move the checks part of consensus operation to ``validate_operation.ml``. The effects part remains in ``apply_operation``. (MR :gl:`!5927`) + +- Implement ``Validate_operation.validate_operation`` on voting + operations (Proposals and Ballot). The checks are now done there, + while ``Apply.apply_operation`` only applies the effects. + (MR :gl:`!5828`) + +- A Testnet Dictator Proposals operation is now mutually exclusive + with any other voting operation inside a same block or mempool. + (MR :gl:`!5828`) + +- Remove redundant ``Delegate_storage.pubkey`` and use directly + ``Contract_manager_storage.get_manager_key`` instead. In situations + where the later used to fail with ``Unregistered_delegate``, we now + get either ``Missing_manager_contract`` or + ``Unrevealed_manager_key``, which describe the issue more + precisely. (MR :gl:`!5828`) -- GitLab