Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract validation out of External_transition #10780

Merged

Conversation

georgeee
Copy link
Member

@georgeee georgeee commented Apr 26, 2022

Refactoring that further reorganizes External_transition module and associated types

Explain your changes:

  • Extract Validated and Validation submodules out of External_transition

Explain how you tested your changes: no functionality is touched, existing tests will cover it well.

Checklist:

  • Modified the current draft of release notes with details on what is completed or incomplete within this project
  • Document code purpose, how to use it
    • Mention expected invariants, implicit constraints
  • Tests were added for the new behavior
    • Document test purpose, significance of failures
    • Test names should reflect their purpose
  • All tests pass (CI will check this if you didn't)
  • Serialized types are in stable-versioned modules
  • Does this close issues? None

@georgeee georgeee requested review from a team as code owners April 26, 2022 20:12
@georgeee georgeee added the ci-build-me Add this label to trigger a circle+buildkite build for this branch label Apr 26, 2022
* 'staged_ledger_diff
* 'protocol_versions

(* TODO commented out because of weird type errors *)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussing with you are reviewing, I think it is fine to just delete these constraints altogether.

in
validated_transition
(* TODO: the delta transition chain proof is incorrect (same behavior the daemon used to have, but we should probably fix this?) *)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's log an issue to review this. It's fine to merge as is, but we should review this because it could be hiding a subtle bug (if we serve these proofs to bootstrapping nodes or something).

@georgeee georgeee force-pushed the georgeee/extract-validated-out-of-external-transition branch from ffe4d1a to 3839ef9 Compare April 29, 2022 11:17
@georgeee georgeee force-pushed the georgeee/extract-validated-out-of-external-transition branch from af76a6a to 2f4f657 Compare May 5, 2022 12:33
@georgeee georgeee force-pushed the georgeee/extract-validated-out-of-external-transition branch from a19ddec to 192d567 Compare May 5, 2022 18:21
@@ -82,7 +82,7 @@ let g:syntastic_ocaml_checkers=['merlin']
- Now `/usr/bin/opam install merlin ocp-indent core async ppx_jane ppx_deriving` (everything we depend on, that you want autocompletes for) for doc reasons
- Make sure you have `au FileType ocaml set omnifunc=merlin#Complete` in your vimrc
- Install an auto-completer (such as YouCompleteMe) and a syntastic (such syntastic or ALE)
- If you use vscode, you might like these extensions
- If you use vscode, you might like this extension
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be these extensions, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, this is an accidental change. I will fix it as a separate micro-PR

let b1 = Transition_frontier.Breadcrumb.validated_transition br1 in
let b2 = Transition_frontier.Breadcrumb.validated_transition br2 in
(* We force evaluation of state body hash for both blocks for further equality check *)
let _hash1 = Mina_block.Validated.state_body_hash b1 in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I follow the necessity of this. If the state body hashes are equal, that implies that the state body hashes are equal, even if we don't have those values cached.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

State body hash is stored in a mutable variable which is set once the value is actually calculated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem arises when we compare this field for blocks computed in different code paths: one might have mutable variable set to None (because state body hash was not ever calculated up until the moment of execution of the line) and the other is Some x. Hence I force that both state hashes are evaluated and equal to Some x.

@nholland94 nholland94 merged commit ea01bc7 into compatible May 9, 2022
@nholland94 nholland94 deleted the georgeee/extract-validated-out-of-external-transition branch May 9, 2022 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-build-me Add this label to trigger a circle+buildkite build for this branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants