From e1d781dff57aa468557aafa62045d9a4d4525e2d Mon Sep 17 00:00:00 2001 From: vbot Date: Tue, 29 Mar 2022 18:23:45 +0200 Subject: [PATCH 1/4] Proto: fail with an error instead of an exception --- src/proto_alpha/lib_protocol/apply.mli | 2 +- .../lib_protocol/contract_manager_storage.ml | 19 +++++++++++++++++-- .../lib_protocol/contract_manager_storage.mli | 1 + src/proto_alpha/lib_protocol/main.mli | 2 +- 4 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/proto_alpha/lib_protocol/apply.mli b/src/proto_alpha/lib_protocol/apply.mli index 4ef2b1a3ce88..b807f23dd4f6 100644 --- a/src/proto_alpha/lib_protocol/apply.mli +++ b/src/proto_alpha/lib_protocol/apply.mli @@ -200,7 +200,7 @@ val check_minimum_endorsements : @return [Error Invalid_signature] if the signature check fails @return [Error Unrevealed_manager_key] if the manager has not yet been revealed - @return [Error Failure "get_manager_key"] if the key is not found in the + @return [Error Missing_manager_contract] if the key is not found in the context @return [Error Inconsistent_sources] if the operations in a batch are not from the same manager *) diff --git a/src/proto_alpha/lib_protocol/contract_manager_storage.ml b/src/proto_alpha/lib_protocol/contract_manager_storage.ml index 43464734c5aa..71bf78b566a0 100644 --- a/src/proto_alpha/lib_protocol/contract_manager_storage.ml +++ b/src/proto_alpha/lib_protocol/contract_manager_storage.ml @@ -32,6 +32,7 @@ type error += provided_hash : Signature.Public_key_hash.t; } | (* `Branch *) Previously_revealed_key of Contract_repr.t + | (* `Branch *) Missing_manager_contract of Contract_repr.t let () = register_error_kind @@ -89,7 +90,21 @@ let () = s) Data_encoding.(obj1 (req "contract" Contract_repr.encoding)) (function Previously_revealed_key s -> Some s | _ -> None) - (fun s -> Previously_revealed_key s) + (fun s -> Previously_revealed_key s) ; + register_error_kind + `Branch + ~id:"contract.missing_manager_contract" + ~title:"Missing manager contract" + ~description:"The manager contract is missing from the storage" + ~pp:(fun ppf s -> + Format.fprintf + ppf + "The contract %a is missing from the storage." + Contract_repr.pp + s) + Data_encoding.(obj1 (req "contract" Contract_repr.encoding)) + (function Missing_manager_contract s -> Some s | _ -> None) + (fun s -> Missing_manager_contract s) let init = Storage.Contract.Manager.init @@ -119,7 +134,7 @@ let get_manager_key ?error ctxt pkh = Storage.Contract.Manager.find ctxt contract >>=? function | None -> ( match error with - | None -> failwith "get_manager_key" + | None -> fail (Missing_manager_contract contract) | Some error -> fail error) | Some (Manager_repr.Hash _) -> ( match error with diff --git a/src/proto_alpha/lib_protocol/contract_manager_storage.mli b/src/proto_alpha/lib_protocol/contract_manager_storage.mli index 8982e89a7699..99d8cd19a064 100644 --- a/src/proto_alpha/lib_protocol/contract_manager_storage.mli +++ b/src/proto_alpha/lib_protocol/contract_manager_storage.mli @@ -32,6 +32,7 @@ type error += provided_hash : Signature.Public_key_hash.t; } | (* `Branch *) Previously_revealed_key of Contract_repr.t + | (* `Branch *) Missing_manager_contract of Contract_repr.t (** [init ctxt contract manager] associates [manager] to [contract]. This function is undefined if [contract] has already a manager associated to it. diff --git a/src/proto_alpha/lib_protocol/main.mli b/src/proto_alpha/lib_protocol/main.mli index 29b97330afae..6e2b8deff3cc 100644 --- a/src/proto_alpha/lib_protocol/main.mli +++ b/src/proto_alpha/lib_protocol/main.mli @@ -121,7 +121,7 @@ type operation = Alpha_context.packed_operation = { @return [Error Invalid_signature] if the signature check fails @return [Error Unrevealed_manager_key] if the manager has not yet been revealed - @return [Error Failure "get_manager_key"] if the key is not found in the + @return [Error Missing_manager_contract] if the key is not found in the context @return [Error Inconsistent_sources] if the operations in a batch are not from the same manager *) -- GitLab From 118ffe5ab8c33099e6b66a5912b61a804522e819 Mon Sep 17 00:00:00 2001 From: vbot Date: Tue, 29 Mar 2022 18:24:38 +0200 Subject: [PATCH 2/4] Proto: retrieve the public key before prechecking This patch retrieve the public key used in check signature *before* applying the side-effects of 'precheck_manager_contents' that could cause an implicit contract being removed when fees equal to balance are debited which would make the, a posteriori, public key retrieval fail. --- src/proto_alpha/lib_protocol/apply.ml | 29 +++++++++++++++++---------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/src/proto_alpha/lib_protocol/apply.ml b/src/proto_alpha/lib_protocol/apply.ml index a6527475ca07..0d07ccafadda 100644 --- a/src/proto_alpha/lib_protocol/apply.ml +++ b/src/proto_alpha/lib_protocol/apply.ml @@ -2251,20 +2251,18 @@ let precheck_manager_contents_list ctxt contents_list ~mempool_mode = check_counters_consistency contents_list >>=? fun () -> rec_precheck_manager_contents_list ctxt contents_list -let check_manager_signature ctxt chain_id (op : _ Kind.manager contents_list) - raw_operation = +let find_manager_public_key ctxt (op : _ Kind.manager contents_list) = (* Currently, the [op] only contains one signature, so all operations are required to be from the same manager. This may change in the future, allowing several managers to group-sign a sequence of transactions. *) let check_same_manager (source, source_key) manager = match manager with - | None -> - (* Consistency already checked by - [reveal_manager_key] in [precheck_manager_contents]. *) - ok (source, source_key) + | None -> ok (source, source_key) | Some (manager, manager_key) -> if Signature.Public_key_hash.equal source manager then + (* Consistency will be checked by + [reveal_manager_key] in [precheck_manager_contents]. *) ok (source, Option.either manager_key source_key) else error Inconsistent_sources in @@ -2287,10 +2285,13 @@ let check_manager_signature ctxt chain_id (op : _ Kind.manager contents_list) find_source rest (Some manager) in find_source op None >>?= fun (source, source_key) -> - (match source_key with + match source_key with | Some key -> return key - | None -> Contract.get_manager_key ctxt source) - >>=? fun public_key -> + | None -> Contract.get_manager_key ctxt source + +let check_manager_signature ctxt chain_id (op : _ Kind.manager contents_list) + raw_operation = + find_manager_public_key ctxt op >>=? fun public_key -> Lwt.return (Operation.check_signature public_key chain_id raw_operation) let rec apply_manager_contents_list_rec : @@ -2967,9 +2968,12 @@ let apply_contents_list (type kind) ctxt chain_id (apply_mode : apply_mode) mode (* Failing_noop _ always fails *) fail Failing_noop_error | Single (Manager_operation _) as op -> + (* Use the initial context, the contract may be deleted by the + fee transfer in [precheck_manager_contents] *) + find_manager_public_key ctxt op >>=? fun public_key -> precheck_manager_contents_list ctxt op ~mempool_mode >>=? fun (ctxt, prechecked_contents_list) -> - check_manager_signature ctxt chain_id op operation >>=? fun () -> + Operation.check_signature public_key chain_id operation >>?= fun () -> apply_manager_contents_list ctxt mode @@ -2978,9 +2982,12 @@ let apply_contents_list (type kind) ctxt chain_id (apply_mode : apply_mode) mode prechecked_contents_list >|= ok | Cons (Manager_operation _, _) as op -> + (* Use the initial context, the contract may be deleted by the + fee transfer in [precheck_manager_contents] *) + find_manager_public_key ctxt op >>=? fun public_key -> precheck_manager_contents_list ctxt op ~mempool_mode >>=? fun (ctxt, prechecked_contents_list) -> - check_manager_signature ctxt chain_id op operation >>=? fun () -> + Operation.check_signature public_key chain_id operation >>?= fun () -> apply_manager_contents_list ctxt mode -- GitLab From 6e40ebc63d5c6e02e71bf3066dcc5be0d297a9cc Mon Sep 17 00:00:00 2001 From: vbot Date: Tue, 29 Mar 2022 18:30:13 +0200 Subject: [PATCH 3/4] Proto/Tests: test validity of emptying-accounts operations --- .../integration/consensus/test_delegation.ml | 32 +++++++++++++++++++ .../integration/operations/test_reveal.ml | 24 ++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/src/proto_alpha/lib_protocol/test/integration/consensus/test_delegation.ml b/src/proto_alpha/lib_protocol/test/integration/consensus/test_delegation.ml index 986ab90b3c0d..2c65f914acbd 100644 --- a/src/proto_alpha/lib_protocol/test/integration/consensus/test_delegation.ml +++ b/src/proto_alpha/lib_protocol/test/integration/consensus/test_delegation.ml @@ -1276,6 +1276,34 @@ let test_unregistered_and_revealed_self_delegate_key_init_delegation ~fee () = >>=? fun i -> Assert.balance_was_debited ~loc:__LOC__ (I i) contract balance fee +(** Self-delegation emptying a fresh contract. *) +let test_self_delegation_emptying_contract () = + Context.init 1 >>=? fun (b, bootstrap_contracts) -> + Incremental.begin_construction b >>=? fun i -> + let bootstrap = + WithExceptions.Option.get ~loc:__LOC__ @@ List.hd bootstrap_contracts + in + let {Account.pkh; pk; _} = Account.new_account () in + let {Account.pkh = delegate_pkh; _} = Account.new_account () in + let contract = Alpha_context.Contract.implicit_contract pkh in + let amount = of_int 10 in + Op.transaction (I i) bootstrap contract amount >>=? fun op -> + Incremental.add_operation i op >>=? fun i -> + Op.revelation ~fee:Tez.zero (I i) pk >>=? fun op -> + Incremental.add_operation i op >>=? fun i -> + Op.delegation ~fee:amount (I i) contract (Some delegate_pkh) >>=? fun op -> + (Context.Contract.is_manager_key_revealed (I i) contract >>=? function + | false -> failwith "contract should exist" + | true -> return_unit) + >>=? fun () -> + (* The delegation operation should be applied and the fees + debited but it is expected to fail in the apply-part. *) + Incremental.add_operation ~expect_failure:(fun _ -> return_unit) i op + >>=? fun i -> + Context.Contract.is_manager_key_revealed (I i) contract >>=? function + | false -> return_unit + | true -> failwith "contract should have been removed" + (** Self-delegation on revealed and registered contract. *) let test_registered_self_delegate_key_init_delegation () = Context.init 1 >>=? fun (b, bootstrap_contracts) -> @@ -1446,6 +1474,10 @@ let tests_delegate_registration = `Quick (test_unregistered_and_revealed_self_delegate_key_init_delegation ~fee:max_tez); + Tztest.tztest + "unregistered and revealed self-delegation (fee = balance)" + `Quick + test_self_delegation_emptying_contract; (* self delegation on registered contract *) Tztest.tztest "registered and revealed self-delegation" diff --git a/src/proto_alpha/lib_protocol/test/integration/operations/test_reveal.ml b/src/proto_alpha/lib_protocol/test/integration/operations/test_reveal.ml index 1847e619543a..7af3e8f9fbbc 100644 --- a/src/proto_alpha/lib_protocol/test/integration/operations/test_reveal.ml +++ b/src/proto_alpha/lib_protocol/test/integration/operations/test_reveal.ml @@ -98,6 +98,26 @@ let test_not_enough_found_for_reveal () = Block.bake blk ~operation >>= fun res -> Assert.proto_error_with_info ~loc:__LOC__ res "Balance too low" +let test_transfer_fees_emptying_after_reveal_batched () = + Context.init 1 >>=? fun (blk, contracts) -> + let c = WithExceptions.Option.get ~loc:__LOC__ @@ List.hd contracts in + let new_c = Account.new_account () in + let new_contract = Alpha_context.Contract.implicit_contract new_c.pkh in + (* Create the contract *) + Op.transaction (B blk) c new_contract Tez.one >>=? fun operation -> + Block.bake blk ~operation >>=? fun blk -> + Incremental.begin_construction blk >>=? fun inc -> + Op.revelation ~fee:Tez.zero (I inc) new_c.pk >>=? fun reveal -> + Incremental.add_operation inc reveal >>=? fun tmp_inc -> + Op.transaction ~fee:Tez.one (I tmp_inc) new_contract c Tez.one + >>=? fun transaction -> + Op.batch_operations ~source:new_contract (I inc) [reveal; transaction] + >>=? fun op -> + (* This operation is expected to fail at application time, not + during validation. *) + Incremental.add_operation ~expect_failure:(fun _ -> return_unit) inc op + >>=? fun _inc -> return_unit + let tests = [ Tztest.tztest "simple reveal" `Quick test_simple_reveal; @@ -106,4 +126,8 @@ let tests = "not enough found for reveal" `Quick test_not_enough_found_for_reveal; + Tztest.tztest + "transfer fees emptying balance after reveal in batch" + `Quick + test_transfer_fees_emptying_after_reveal_batched; ] -- GitLab From 1bb8d80c7c283936015de3a75cb2baaf37b26463 Mon Sep 17 00:00:00 2001 From: vbot Date: Tue, 29 Mar 2022 18:47:16 +0200 Subject: [PATCH 4/4] Doc/Alpha: document the change --- docs/protocols/alpha.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/protocols/alpha.rst b/docs/protocols/alpha.rst index 19e9e95e08b5..427e29c20b3f 100644 --- a/docs/protocols/alpha.rst +++ b/docs/protocols/alpha.rst @@ -170,6 +170,8 @@ Minor Changes - Change ``blocks_per_voting period`` in context with ``cycles_per_voting_period`` (MR :gl:`!4456`) +- Retrieve a contract's public key before prechecking an operation. (MR :gl:`!4877`) + Michelson --------- -- GitLab