Skip to content

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 by apply_external_manager_operation_content instead of by precheck_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 for add_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 to false 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.
Edited by Germán Delbianco

Merge request reports