Proto: fix semantics for failing Reveal operations
Context
TL;DR Fix #2774 (closed), #2386 (closed), reverts !587 (merged), re-fixes #338 (closed).
-
We restore the all or nothing semantics for manager operation batches by ensuring that failing Reveal operations don't take effect (cf. #2386 (closed)). We revert changes introduced by !587 (merged) (cf #338 (closed)).
-
For this, we refactor
apply_operation
so that the effect of revealing the manager is implemented byapply_external_manager_operation_content
instead of byprecheck_manager_contents
. -
We enforce a new structural condition on manager operations batches, expecting that Reveal operations are placed only at the head of the batch - a new Permanent error,
Incorrect_reveal_position
(see #2774 (closed)). -
This simplifies the work of the %(OKR 2022Q3 - 1.1) Pipelining project: the precondidition of
Reveal
becomes disentangled from its effect, easing the process of splitting pure preconditions (validate), from effects (apply proper). -
In order to better test Reveal operations in Protocol integration tests, we extend the signature of
manager_operation
and other derived helpers to disable attaching a Reveal operations - the current behaviour is preserved, by enabling?force_reval
by default. -
In the process of writing and rewriting tests, we inverted the
?expect_failure
and?expect_apply_failuree
optional handles foradd_operation
in Incremental mode helpers, as they implemented opposite semantics (cf. !4877 (6e40ebc6)).
Manually testing the MR
-
We have added several (~10) integration tests for the Reveal operation under (../test/integration/operation.ml), addressing the issues highlighted by #2386 (closed) and #2774 (closed).
-
The commit history of the MR is written to start with the work extending proto test helpers, including the swap of
?expect_failure
and?expect_apply_failure
, so manually testing that some of the new integration tests (e.g.test_no_reveal_when_gas_exhausted
) break on Alpha before the changes included in this MR requires a bit of work on the history. -
A few existing Operation integration tests were updated to better assert the new semantics, including reporting a new Branch error
Empty_implicit_contract
, when an account becomes empty in the middle of a batch (cf. !4877 (6e40ebc6)) -
We update previous tezts (#2774 (closed)) to show unified errors for repeated reveals in batches -- for protocols > Jakarta.
Checklist
-
Document the interface of any function added or modified (see the coding guidelines) [ ] Document any change to the user interface, including configuration parameters (see node configuration)-
Provide automatic testing (see the testing guide). -
For new features and bug fixes, add an item in the appropriate changelog ( docs/protocols/alpha.rst
for the protocol and the environment,CHANGES.rst
at the root of the repository for everything else). -
Select suitable reviewers using the Reviewers
field below. -
Select as Assignee
the next person who should take action on that MR
Follow ups
-
Update related tezos Stack exchange question to reflect fixes. -
#3111 (closed) to improve the documentation of the application results for (batches) of manager operations in the Docs. -
#3078 remove the optional argument from Reveal_manager
. -
#2979 (closed) make ?force_reveal
to default tofalse
and adapt integration tests. -
Advertise breaking changes in proto integration test suite w/ DevTeam/ProtoProposal channels. -
#3143 Refactor must_be_allcoated
to take a pkh source argument instead. -
#3144 re-unify the errors raised with get_manager_key
.