Fix two bugs in revalidate

1. Rewrite revalidate to enhance readability
2. Fix two similar issues originating from confusion between previous
   variable names `t` and `t'` ("Account no longer has permission to
   send" and "Current account nonce precedes first nonce in queue")
3. Fix the issue #16397 by ensuring removal from `applicable_by_fee` is
   done only for the previous head of queue.

Author: georgeee

src/lib/network_pool/indexed_pool.ml

@@ -682,29 +682,24 @@ let revalidate :
     -> [ `Entire_pool | `Subset of Account_id.Set.t ]
     -> (Account_id.t -> Account.t)
     -> t * Transaction_hash.User_command_with_valid_signature.t Sequence.t =
- fun t ~logger scope get_account_by_id ->
+ fun t_initial ~logger scope get_account_by_id ->
   let requires_revalidation =
     match scope with
     | `Entire_pool ->
-        t.all_by_sender
+        t_initial.all_by_sender
     | `Subset subset ->
         (* intersection of scope and all_by_sender *)
-        Account_id.Map.merge t.all_by_sender
+        Account_id.Map.merge t_initial.all_by_sender
           (Account_id.Set.to_map subset ~f:(const ()))
           ~f:(fun ~key:_ -> function `Both (v, ()) -> Some v | _ -> None)
   in
-  Map.fold requires_revalidation ~init:(t, Sequence.empty)
-    ~f:(fun
-         ~key:sender
-         ~data:(queue, currency_reserved)
-         ((t', dropped_acc) as acc)
-       ->
+  let global_slot = global_slot_since_genesis t_initial.config in
+  Map.fold requires_revalidation ~init:(t_initial, Sequence.empty)
+    ~f:(fun ~key:sender ~data:(queue, currency_reserved) (t, dropped_acc) ->
       let account : Account.t = get_account_by_id sender in
       let current_balance =
         Currency.Balance.to_amount
-          (Account.liquid_balance_at_slot
-             ~global_slot:(global_slot_since_genesis t.config)
-             account )
+          (Account.liquid_balance_at_slot ~global_slot account)
       in
       [%log debug]
         "Revalidating account $account in transaction pool ($account_nonce, \
@@ -726,13 +721,13 @@ let revalidate :
           && Account.has_permission_to_increment_nonce account )
       then (
         [%log debug] "Account no longer has permission to send; dropping queue" ;
-        let dropped, t'' = remove_with_dependents_exn' t first_cmd in
-        (t'', Sequence.append dropped_acc dropped) )
+        let dropped, t_updated = remove_with_dependents_exn' t first_cmd in
+        (t_updated, Sequence.append dropped_acc dropped) )
       else if Account_nonce.(account.nonce < first_nonce) then (
         [%log debug]
           "Current account nonce precedes first nonce in queue; dropping queue" ;
-        let dropped, t'' = remove_with_dependents_exn' t first_cmd in
-        (t'', Sequence.append dropped_acc dropped) )
+        let dropped, t_updated = remove_with_dependents_exn' t first_cmd in
+        (t_updated, Sequence.append dropped_acc dropped) )
       else
         (* current_nonce >= first_nonce *)
         let first_applicable_nonce_index =
@@ -759,52 +754,63 @@ let revalidate :
               )
             currency_reserved drop_queue
         in
+        (* NB: dropped_for_balance is ordered by nonce *)
         let keep_queue', currency_reserved'', dropped_for_balance =
           drop_until_sufficient_balance
             (keep_queue, currency_reserved')
             current_balance
         in
-        (* NB: to_drop is ordered by nonce *)
         let to_drop =
           Sequence.append (F_sequence.to_seq drop_queue) dropped_for_balance
         in
-        match Sequence.next to_drop with
-        | None ->
-            acc
-        | Some (head, tail) ->
-            let t'' =
-              Sequence.fold tail
-                ~init:
-                  (remove_all_by_fee_and_hash_and_expiration_exn
-                     (remove_applicable_exn t' head)
-                     head )
-                ~f:remove_all_by_fee_and_hash_and_expiration_exn
-            in
-            let t''' =
-              match F_sequence.uncons keep_queue' with
-              | None ->
-                  { t'' with
-                    all_by_sender = Map.remove t''.all_by_sender sender
-                  }
-              | Some (first_kept, _) ->
-                  let first_kept_unchecked =
-                    Transaction_hash.User_command_with_valid_signature.command
-                      first_kept
-                  in
-                  { t'' with
-                    all_by_sender =
-                      Map.set t''.all_by_sender ~key:sender
-                        ~data:(keep_queue', currency_reserved'')
-                  ; applicable_by_fee =
-                      Map_set.insert
-                        ( module Transaction_hash
-                                 .User_command_with_valid_signature )
-                        t''.applicable_by_fee
-                        (User_command.fee_per_wu first_kept_unchecked)
-                        first_kept
-                  }
-            in
-            (t''', Sequence.append dropped_acc to_drop) )
+        (* t with all_by_sender and applicable_by_fee fields updated *)
+        let t_partially_updated =
+          match
+            ( F_sequence.uncons drop_queue
+            , Sequence.hd dropped_for_balance
+            , F_sequence.uncons keep_queue' )
+          with
+          | None, None, _ ->
+              (* Nothing dropped, nothing needs to be updated *)
+              t
+          | Some (first_dropped, _), _, None | None, Some first_dropped, None ->
+              (* We drop the entire queue, first element needs to be removed from
+                 applicable_by_fee *)
+              let t' = remove_applicable_exn t first_dropped in
+              { t' with all_by_sender = Map.remove t'.all_by_sender sender }
+          | None, _, Some _ ->
+              (* We drop only some transactions from the end of queue, keeping
+                 the head untouched, no need to update applicable_by_fee *)
+              { t with
+                all_by_sender =
+                  Map.set t.all_by_sender ~key:sender
+                    ~data:(keep_queue', currency_reserved'')
+              }
+          | Some (first_dropped, _), _, Some (first_kept, _) ->
+              (* We need to replace old queue head with the new queue head
+                 in applicable_by_fee *)
+              let first_kept_unchecked =
+                Transaction_hash.User_command_with_valid_signature.command
+                  first_kept
+              in
+              let t' = remove_applicable_exn t first_dropped in
+              { t' with
+                all_by_sender =
+                  Map.set t'.all_by_sender ~key:sender
+                    ~data:(keep_queue', currency_reserved'')
+              ; applicable_by_fee =
+                  Map_set.insert
+                    (module Transaction_hash.User_command_with_valid_signature)
+                    t'.applicable_by_fee
+                    (User_command.fee_per_wu first_kept_unchecked)
+                    first_kept
+              }
+        in
+        let t_updated =
+          Sequence.fold ~init:t_partially_updated
+            ~f:remove_all_by_fee_and_hash_and_expiration_exn to_drop
+        in
+        (t_updated, Sequence.append dropped_acc to_drop) )
 
 let expired_by_global_slot (t : t) :
     Transaction_hash.User_command_with_valid_signature.t Sequence.t =