Pass validation_callback through catchup process #10895

georgeee · 2022-05-09T12:11:06Z

Pass validation callback along with the block to make sure it's triggered as a result of block validation process.

Explain how you tested your changes:

Existing test suite is sufficient

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? Block propagation fail on blocks which enter the catchup subsystem #10867

nholland94

I think more work needs to be done here with how we handle errors when processing rose trees of blocks. When we have a rose tree of blocks, we don't want to reject an entire subtree just because a single block was invalid. This ends up being a bit tricky in practice. I think, as a general refactor, we should have any functions yielding errors while operating on trees of blocks return a tree of errors, so that we can map over both the input tree of blocks and output tree of errors together and make proper decisions about how to fill the validation callbacks in such scenarios. Reading the code, even besides your changes, I think there are additional bugs hidden here in how we handle errors, but those can be addressed in additional PRs (example given: if a single block in a rose tree is invalid, I believe we will drop all the valid blocks in the tree as well and not actually add them to our frontier).

nholland94 · 2022-05-24T17:28:35Z

src/lib/transition_handler/catchup_scheduler.ml

+  Option.value_map valid_cb ~default:() ~f:(fun data ->
+      match Hashtbl.add t.validation_callbacks ~key:hash ~data with
+      | `Ok ->
+          don't_wait_for


NIT: can be more cleanly written as upon (Deferred.ignore_m @@ Mina_net2.Validation_callback.await data) (fun () -> Hashtbl.remove t.validation_callbacks hash).

nholland94 · 2022-05-24T17:50:36Z

src/lib/transition_handler/catchup_scheduler.ml

@@ -45,45 +47,40 @@ type t =
      Cached.t
      list
      State_hash.Table.t
+        (* Validation callbacks for state hashes that are being processed *)
+  ; validation_callbacks : Mina_net2.Validation_callback.t State_hash.Table.t


Wouldn't it be simpler to just include this in the collected_transitions hash table? Then we don't have to maintain the state of 2 different hash tables.

The code would be slightly trickier, now clean-up is a dumb thing: upon resolution of callback, an entry is removed

But it's possible for sure

nholland94 · 2022-05-24T17:52:04Z

src/lib/transition_handler/catchup_scheduler.ml

-          List.iter cached_transitions
-            ~f:(Fn.compose ignore Cached.invalidate_with_failure) ) ;
+      Hashtbl.iter collected_transitions
+        ~f:(List.iter ~f:(Fn.compose ignore Cached.invalidate_with_failure)) ;


We should also fill the validation callbacks here with Ignore.

nholland94 · 2022-05-24T17:53:13Z

src/lib/transition_handler/catchup_scheduler.ml

@@ -107,7 +104,8 @@ let create ~logger ~precomputed_values ~verifier ~trust_system ~frontier
                $error"
              ~metadata:[ ("error", Error_json.error_to_yojson err) ] ;
            List.iter transition_branches ~f:(fun subtree ->
-                Rose_tree.iter subtree ~f:(fun cached_transition ->
+                Rose_tree.iter subtree ~f:(fun (cached_transition, _vc) ->
+                    (* TODO reject callback? *)


We need to Reject blocks here depending on the reason they failed when building the tree of breadcrumbs.

nholland94 · 2022-05-24T19:26:14Z

src/lib/ledger_catchup/normal_catchup.ml

-              ( List.map acc ~f:Cached.invalidate_with_failure
-                : Mina_block.initial_valid_block Envelope.Incoming.t list ) ;
+            List.iter acc ~f:(fun (node, _vc) ->
+                (* TODO reject callback? *)


Yes, but it depends on the type of error triggered in verify_transition. We probably need to change the error type of that function into something more meaningful in order to handle this.

nholland94 · 2022-05-24T19:39:59Z

src/lib/transition_frontier/full_catchup_tree.ml

@@ -293,11 +293,13 @@ let remove_node' t (node : Node.t) =
      ()
  | To_initial_validate _ ->
      ()
-  | To_verify c ->
+  | To_verify (c, _vc) ->
+      (* TODO should we reject the validation callback? *)


We should fill the validation callback with Ignore here.

nholland94 · 2022-05-24T19:45:13Z