Refactor transition router #16145

anne-laure-s · 2024-09-25T15:29:46Z

This PR rebases the following commit of this PR :

Simplify transition_router: remove unnecessary reader refs
Do not ignore validation_callbacks for transition gossips received
during bootstrap
Add support for header gossip collection during boostrap

anne-laure-s · 2024-09-25T15:30:12Z

!ci-build-me

georgeee · 2024-09-26T19:32:37Z

src/lib/transition_frontier_controller/transition_frontier_controller.ml

+        | Bootstrap_controller.Transition_cache.Block b ->
+            Some (map x ~f:(const b), vc)
+        | _ ->
+            (* TODO: handle headers too *)


It might be good to note that headers can't be actually received in this PR, so this TODO is safe to be left for future.

I added this in the comment.

georgeee · 2024-09-26T19:43:54Z

src/lib/transition_router/transition_router.ml

-                          ~persistent_frontier ~initial_root_transition
-                          ~best_seen_transition:(Some enveloped_transition)
-                          ~catchup_mode )
+                        Fn.const ()


Replace Fn.const () with ignore

georgeee · 2024-09-26T19:53:17Z

src/lib/transition_router/transition_router.ml

@@ -138,34 +138,34 @@ let start_bootstrap_controller ~context:(module Context : CONTEXT) ~trust_system
          ~pipe_name:name ~logger ?valid_cb )
      ()
  in
-  transition_reader_ref := bootstrap_controller_reader ;
-  transition_writer_ref := bootstrap_controller_writer ;
-  let producer_transition_reader, producer_transition_writer =


Note on producer_transition_writer_ref: type of this parameter is changed from _ Strict_pipe.Writer.t ref to _ Strict_pipe.Writer.t option ref.

This makes sense because no block production happens when bootstrap is running. This pipe was created just to substitute a value for the type and was never actually used. It could have been read only by the transition frontier controller, and it's not running when bootstrap controller is active.

I added this as a comment in the code for the future.

georgeee · 2024-09-26T20:28:37Z

src/lib/bootstrap_controller/bootstrap_controller.ml

@@ -318,17 +325,8 @@ let run ~context:(module Context : CONTEXT) ~trust_system ~verifier ~network
               Sync_ledger.Db.create temp_snarked_ledger ~logger ~trust_system
             in
             don't_wait_for
-               (sync_ledger t
-                  ~preferred:
-                    ( Option.to_list best_seen_transition


This code has been moved to start_bootstrap_controller to highlight that we don't really depend on best seen transition itself, but only on the knowledge of which peer shared it.

georgeee · 2024-09-26T20:32:18Z

src/lib/bootstrap_controller/transition_cache.mli

+type element =
+  initial_valid_block_or_header Envelope.Incoming.t
+  * Mina_net2.Validation_callback.t option
+


Note: big change in this module is that transition cache may hold not only blocks, but headers too.

Also validation callbacks are not discarded any more

georgeee · 2024-09-26T20:39:17Z

src/lib/bootstrap_controller/transition_cache.ml

-        then children
-        else new_child :: children )
+        let children', b =
+          List.fold children ~init:([], false) ~f:(fun (acc, b) child ->


Note: instead of just checking whether a state hash already was among children, we'll merge an existing child with the new transition.

It's needed to maintain consistency about transitions that may come from gossip or other ways (not sure if there could be some, but interfaces seem to allow that from the first glance), we want to preserve some validation callback (and we don't expect two gossips for the same).

Also if we received a block after receiving a header (definitely possible when we'll simultaneously support both old and new gossip topics), we want to preserve a block because it's more than a header.

I added this as a comment in the code for the future.

georgeee · 2024-09-26T20:48:44Z

src/lib/transition_frontier_controller/transition_frontier_controller.ml

+    List.filter_map collected_transitions ~f:(fun (x, vc) ->
+        let open Network_peer.Envelope.Incoming in
+        match data x with
+        | Bootstrap_controller.Transition_cache.Block b ->


Consider removing the type containing Block constructor and just use:

[ `Block of Mina_block.t | `Header of Mina_block.Header.t ]

Then we'll no longer need dependency on the bootstrap controller here

Done in this commit

georgeee · 2024-09-26T20:49:07Z

src/lib/transition_router/transition_router.ml

+  producer_transition_writer_ref := None ;
+  let f block =
+    Strict_pipe.Writer.write bootstrap_controller_writer
+      (`Block block, `Valid_cb None) ;


Note: code below moved here from bootstrap controller.

New function combines writing to boostrap controller writer (which was here already) and extracting preferred peers (freshly moved).

src/lib/transition_router/transition_router.ml

georgeee · 2024-09-26T21:37:46Z

src/lib/transition_router/transition_router.ml

@@ -93,34 +92,35 @@ let start_transition_frontier_controller ~context:(module Context : CONTEXT)
          ?valid_cb ~pipe_name:name ~logger )
      ()
  in
-  transition_reader_ref := transition_frontier_controller_reader ;
-  transition_writer_ref := transition_frontier_controller_writer ;
+  let transition_writer_ref =


Arguably change of semantics behind transition_writer_ref is the most non-trivial change of the PR.

I strongly advise to extract this change as a separate commit.

Also, I think there is a bug here.

Original intention seemed to have been to remove the need to pass transition_reader_ref everywhere (because where it's used what's inside is always a reader created locally, hence we don't need a ref to use the correct value). And also to remove redundant pipe creation to start initialization (a pipe that gets discarded immediately). For this reason the transition_writer_ref was made an optional argument (and no argument in case of initialize function).

So the code below does it right that it uses ref transition_frontier_controller_writer as the default value. It does it wrong in not setting ref to transition_frontier_controller_writer when transition_writer_ref is not None.

So the code I think should be here is Option.value_map ~f:(fun r -> r := transition_frontier_controller_writer; r) transition_writer_ref ~default:(ref transition_frontier_controller_writer)

And same in start_bootstrap_controller.

Isn't Option.value_map ~f:(fun r -> r := transition_frontier_controller_writer; r) transition_writer_ref ~default:(ref transition_frontier_controller_writer) the same as ref transition_frontier_controller_writer ?

I think I’m missing something because to me this makes the ?transition_writer_ref argument given to the start_transition_frontier_controller function useless, as it is just replaced with transition_frontier_controller_writer

A very good question!

When ?transition_writer_ref:None is passed, following codes are identical by definition:

(* code 1 *) Option.value_map ~f:(fun r -> r := transition_frontier_controller_writer; r) transition_writer_ref ~default:(ref transition_frontier_controller_writer) (* code 2 *) ref transition_frontier_controller_writer

However when ?transition_writer_ref:(Some myref) is passed, behavior changes. Code that calls start_transition_frontier_controller may use myref in some other thread. In particular we use it the following way:

don't_wait_for @@ Strict_pipe.Reader.iter_without_pushback valid_transition_reader2 ~f:(fun (`Block enveloped_transition, `Valid_cb vc) -> don't_wait_for @@ let%map () = (* some code removed for clarity *) in Strict_pipe.Writer.write !transition_writer_ref (`Block enveloped_transition, `Valid_cb (Some vc)) ) )

We have an infinite loop of start_transition_frontier_controller transitioning to start_bootstrap_controller and then back, depending on state of sync. However the async thread above is started once with some ref. It's important that we write to that same variable instead of creating a new one as the code let transition_writer_ref = ref transition_frontier_controller_writer would do.

Thanks a lot for this explaination, it makes more sense now.

I made a separate commit including your new version : 0cbd947

georgeee · 2024-09-26T21:49:39Z

src/lib/transition_router/transition_router.ml

@@ -93,34 +92,35 @@ let start_transition_frontier_controller ~context:(module Context : CONTEXT)
          ?valid_cb ~pipe_name:name ~logger )
      ()
  in
-  transition_reader_ref := transition_frontier_controller_reader ;
-  transition_writer_ref := transition_frontier_controller_writer ;


So, this line removal wasn't part of original fbc8bf0

georgeee · 2024-09-26T21:49:56Z

src/lib/transition_router/transition_router.ml

@@ -138,34 +138,34 @@ let start_bootstrap_controller ~context:(module Context : CONTEXT) ~trust_system
          ~pipe_name:name ~logger ?valid_cb )
      ()
  in
-  transition_reader_ref := bootstrap_controller_reader ;
-  transition_writer_ref := bootstrap_controller_writer ;


So, this line removal wasn't part of original fbc8bf0

I think I removed it because I was confused that the new code was redundant :

let transition_writer_ref = Option.value transition_writer_ref ~default:(ref bootstrap_controller_writer) in transition_writer_ref := bootstrap_controller_writer ;

Was it a mistake ?

Nevermind, I had not read the previous comment about transition_writer_ref.

If I understood correctly, whatever the value in transition_writer_ref is when given as an argument, we want to replace it with ref transition_frontier_controller_writer. Is that correct ?

I'll reply to https://github.com/MinaProtocol/mina/pull/16145/files#r1781433238, there you formulated your concern excellently!

georgeee · 2024-09-26T21:50:58Z

Reviewing this PR was an interesting exercise. This code is more than two years old, and I only faintly remembered the "why", but not the "how" about writing it.

I'd prefer it was split to 6 separate commits, but it's okay. I asked to extract one change as a separate commit because it seems to contain a bug introduced during the rebase process.

georgeee · 2024-10-01T14:26:40Z

!ci-build-me

georgeee · 2024-10-01T14:26:58Z

!ci-nightly-me

Co-authored-by: georgeee <[email protected]>

… blocks Co-authored-by: georgeee <[email protected]>

…_header & remove bootstrap_controller dependancy from Transition_frontier_controller Co-authored-by: georgeee <[email protected]>

…ter: add & update comments Co-authored-by: georgeee <[email protected]>

anne-laure-s · 2024-10-01T16:01:11Z

!ci-build-me

coveralls · 2024-10-01T17:00:07Z

coverage: 49.792% (+16.4%) from 33.411%
when pulling 69eee47 on al/transition-router
into 3a00659 on compatible.

anne-laure-s added the bitswap label Sep 25, 2024

anne-laure-s self-assigned this Sep 25, 2024

anne-laure-s requested a review from a team as a code owner September 25, 2024 15:29

anne-laure-s requested a review from georgeee September 26, 2024 08:35

anne-laure-s changed the title ~~Al/transition router~~ Refactor transition router Sep 26, 2024

georgeee requested changes Sep 26, 2024

View reviewed changes

anne-laure-s force-pushed the al/transition-router branch 3 times, most recently from a72c1e7 to cbb1a38 Compare September 30, 2024 16:35

anne-laure-s requested a review from georgeee September 30, 2024 16:38

anne-laure-s force-pushed the al/transition-router branch from cbb1a38 to 69eee47 Compare October 1, 2024 11:23

georgeee approved these changes Oct 1, 2024

View reviewed changes

georgeee and others added 5 commits October 1, 2024 17:15

Mina_block: add Header handling functions and types

8fc03d7

Transition_router: change transition_writer_ref semantic

ab89bc3

Co-authored-by: georgeee <[email protected]>

Bootstrap_controller/Transition_cache: handle headers instead of just…

f625651

… blocks Co-authored-by: georgeee <[email protected]>

Bootstrap_controller/Transition_cache: change intitial_valid_block_or…

95ce6e1

…_header & remove bootstrap_controller dependancy from Transition_frontier_controller Co-authored-by: georgeee <[email protected]>

Bootstrap_controller, Transition_frontier_controller & Transition_rou…

b84d5ac

…ter: add & update comments Co-authored-by: georgeee <[email protected]>

anne-laure-s force-pushed the al/transition-router branch from 69eee47 to b84d5ac Compare October 1, 2024 15:17

svv232 merged commit 279fea3 into compatible Oct 1, 2024
46 checks passed

svv232 deleted the al/transition-router branch October 1, 2024 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor transition router #16145

Refactor transition router #16145

anne-laure-s commented Sep 25, 2024

anne-laure-s commented Sep 25, 2024

georgeee Sep 26, 2024

anne-laure-s Sep 30, 2024

georgeee Sep 26, 2024

anne-laure-s Sep 30, 2024

georgeee Sep 26, 2024

anne-laure-s Sep 30, 2024

georgeee Sep 26, 2024

georgeee Sep 26, 2024

georgeee Sep 26, 2024

anne-laure-s Sep 30, 2024

georgeee Sep 26, 2024

anne-laure-s Sep 30, 2024 •

edited

Loading

georgeee Sep 26, 2024

georgeee Sep 26, 2024

anne-laure-s Sep 30, 2024

georgeee Sep 30, 2024

anne-laure-s Oct 1, 2024

georgeee Sep 26, 2024

georgeee Sep 26, 2024

anne-laure-s Sep 30, 2024 •

edited

Loading

anne-laure-s Sep 30, 2024

georgeee Sep 30, 2024

georgeee commented Sep 26, 2024

georgeee commented Oct 1, 2024

georgeee commented Oct 1, 2024

anne-laure-s commented Oct 1, 2024

coveralls commented Oct 1, 2024

Refactor transition router #16145

Refactor transition router #16145

Conversation

anne-laure-s commented Sep 25, 2024

anne-laure-s commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anne-laure-s Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anne-laure-s Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgeee commented Sep 26, 2024

georgeee commented Oct 1, 2024

georgeee commented Oct 1, 2024

anne-laure-s commented Oct 1, 2024

coveralls commented Oct 1, 2024

anne-laure-s Sep 30, 2024 •

edited

Loading

anne-laure-s Sep 30, 2024 •

edited

Loading