From b768bfdab73ac9af4defc2a851f3e1caa4c5b7c5 Mon Sep 17 00:00:00 2001 From: georgeee Date: Thu, 5 Dec 2024 14:16:28 +0000 Subject: [PATCH] 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. --- src/lib/network_pool/indexed_pool.ml | 114 ++++++++++++++------------- 1 file changed, 60 insertions(+), 54 deletions(-) diff --git a/src/lib/network_pool/indexed_pool.ml b/src/lib/network_pool/indexed_pool.ml index 29bb2fa9f32..7385880babd 100644 --- a/src/lib/network_pool/indexed_pool.ml +++ b/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 =