Increase test coverage for transaction pool module #16415

dkijania · 2024-12-12T23:01:11Z

Increase coverage for ledger reorg functionality with new property based tests. Apart from general all valid commands generator one can also configure generator to send some edge cases (zkapp which blocking sending further transactions or replace small amount transaction with big amount transaction which should prevent from applying entire chain of commands due to balance drain) . Last commit propose reorganization of all transaction tests since reorg tests require mini test framework.

PR related to #16401

Tests currently are failing but after we merge above PR they should be green.

Notes to reviewer:

I'm not sure if test reorganization is a good idea (commit e38d28d), no problem with reverting this to commit 9cdcb4d). My reasoning was that I added a quite big chunk of code and thought that maybe we should split tests for better readibility. There is already network_pool/tests folder which contains alco tests. Maybe we can put all of them there too ?
I've done one uber property test, which helped me understand how reorg is working. I'm wondering if we should add more fine grain tests which could be used also documentation for reorg mechanism
I had a big troubles to nicely modify already built User_command (adding nonces or create mirror transactions in other branch), so i decided to work on specification on minor/major sequences first and then create commands based on them.

dkijania · 2024-12-13T11:20:05Z

!ci-build-me

georgeee · 2025-01-07T10:39:19Z

I like the idea of extraction of transaction pool tests to a standalone file. Let's restructure the PR in the following way:

Create a separate predecessor PR that will move the test code out of transaction_pool.ml to test/ subdirectory
Retain in this PR only what's related to new tests

@dkijania

georgeee

Review is still in progress, posting the first few comments...

src/lib/network_pool/transacation_pool_reorg_tests.ml

georgeee · 2025-01-07T10:46:02Z

src/lib/network_pool/transacation_pool_reorg_tests.ml

+    if Keypair.equal test_keys.(n) x then n 
+    else find_in_test_keys x ~n:(n+1)
+
+  let create key_idx balance nonce = 


Let's remove this function and create values directly

georgeee · 2025-01-07T11:14:21Z

src/lib/network_pool/test_utils.ml

+      Mina_ledger.Ledger.apply_initial_ledger_state new_ledger init_ledger_state ;
+      t.best_tip_ref := new_ledger
+
+  let ledger_snapshot t =


Suggest to inline it into src/lib/network_pool/transacation_pool_reorg_tests.ml and return Account_spec.t instead of a tuple

georgeee · 2025-01-07T11:16:25Z

src/lib/network_pool/transacation_pool_reorg_tests.ml

+  let apply_payment amount fee t = 
+    create t.key_idx (t.balance - amount - fee) (t.nonce + 1)
+
+  let apply_zkapp fee t = create t.key_idx (t.balance - fee) (t.nonce + 1)


Are all zkapps in this test created with with zero account updates?

If so, this looks ok, but let's add a comment in the code about this matter.

Maybe name is misleading. apply_zkapp function in context of account_spec is only modify spec state which we are using for tracking balance and nonce. So no zkapp command or update is done. Just noting that from this account zkapp was sent and we are deducting fee and increases nonce

georgeee · 2025-01-07T11:35:19Z

src/lib/network_pool/transacation_pool_reorg_tests.ml

+  let minor_acc_opt = Array.find_map minor ~f:(fun minor_acc -> 
+      Array.find major ~f:(fun major_acc -> 
+            Int.equal minor_acc.nonce major_acc.nonce &&
+            minor_acc.balance - major_acc.balance < 100_000_000_000


What is the chance that such two accounts won't be found?

Won't the probability of the test being executed be too low?

What if we:

pick a random account with initial balance B (which won't be used in a generator for other transactions)

generate two random sequences s1 and s2 (small, 2-10 transactions) of transactions using it, which won't use more than B/2 MINA in total, let's assume |s1| < |s2|

calculate total balance spent in sequence s1 as b1

select a number i: |s1| < i < |s2|

calculate sum t2 as balance spent by transactions in s2 with index range [|s1|, i)

increase amount of a random transaction in s1 by B - b1 - t2

inject the resulting sequences s1 and s2 randomly into major and minor sequences (at random indices, preserving order)

This way we deterministically have a use case, not on the luck (later sounds a bit more complicated to analyze than it should be, to my taste). Also, I am not sure if the current method is correct at all.

src/lib/network_pool/transacation_pool_reorg_tests.ml

georgeee · 2025-01-07T14:47:01Z

src/lib/network_pool/transacation_pool_reorg_tests.ml

+          ; ("major accounts state", `List log_major_accounts_state)
+        ];
+
+        let prefix = gen_commands_from_specs (Array.concat [prefix_specs]) test in


Why not simply let prefix = gen_commands_from_specs prefix_specs test in ??

I didn't see it. thanks!

src/lib/network_pool/transacation_pool_reorg_tests.ml

georgeee · 2025-01-07T18:53:00Z

src/lib/network_pool/transacation_pool_reorg_tests.ml

+  let minor_acc_opt = Array.find_map minor ~f:(fun minor_acc -> 
+      Array.find major ~f:(fun major_acc -> 
+            Int.equal minor_acc.nonce major_acc.nonce &&
+            minor_acc.balance - major_acc.balance < 100_000_000_000


What if we:

pick a random account with initial balance B (which won't be used in a generator for other transactions)

generate two random sequences s1 and s2 (small, 2-10 transactions) of transactions using it, which won't use more than B/2 MINA in total, let's assume |s1| < |s2|

calculate total balance spent in sequence s1 as b1

select a number i: |s1| < i < |s2|

calculate sum t2 as balance spent by transactions in s2 with index range [|s1|, i)

increase amount of a random transaction in s1 by B - b1 - t2

inject the resulting sequences s1 and s2 randomly into major and minor sequences (at random indices, preserving order)

This way we deterministically have a use case, not on the luck (later sounds a bit more complicated to analyze than it should be, to my taste). Also, I am not sure if the current method is correct at all.

georgeee · 2025-01-07T18:55:26Z

src/lib/network_pool/transacation_pool_reorg_tests.ml

+let gen_branches init_ledger_state ~permission_change ~limited_capacity ?(sequence_max_length=3) () = 
+  let open Quickcheck.Generator.Let_syntax in
+  let%bind prefix_length = Int.gen_incl 1 sequence_max_length in
+  let%bind branch_length = Int.gen_incl 1 sequence_max_length in


Let's generate sequence length separately for minor and major, they do not have to be the same

georgeee · 2025-01-07T20:12:57Z

src/lib/network_pool/transacation_pool_reorg_tests.ml

+    Otherwise it will be dropped as we already have transaction with the same nonce from major sequence
+  *)
+  let sender = major.(sender.key_idx) in
+  let%bind aux_minor_cmd = Command_spec.gen_single_from minor (sender.key_idx,sender) in


this logic won't imitate the required behavior if maj1 >= min1, where maj1 and min1 represent the number of txs from sender in major and minor respectively.

I.e. aux_minor_cmd will simply be thrown away. We need to add as many aux cmds as necessary to ensure that resulting minor sequence has more transactions from sender than major.

georgeee · 2025-01-07T20:22:11Z

src/lib/network_pool/transacation_pool_reorg_tests.ml

+  *)
+  let%bind permission_change_cmd = Command_spec.gen_zkapp_blocking_send major in
+  let sender = Command_spec.sender permission_change_cmd in
+  (* We need to increase nonce so transaction has a chance to be placed in the pool.


where is the actual increase happening?

In gen_single_from function

georgeee · 2025-01-07T20:32:32Z

src/lib/network_pool/transacation_pool_reorg_tests.ml

+        let%bind receiver_idx = test_keys |> Array.mapi ~f:(fun i _-> i) |> Quickcheck_lib.of_array in
+        let%bind amount = Int.gen_incl 5_000_000_000_000 10_000_000_000_000  in
+        let new_account_spec = Account_spec.apply_payment amount minimum_fee account_spec in 
+        Array.set spec idx new_account_spec;


What if balance goes negative? Perhaps we set amount to 0 if amount is less than balance. And if balance is less than min fee, throw an exception.

Case of fee applies to zkapp tx above as well

georgeee · 2025-01-07T20:48:36Z

src/lib/network_pool/transacation_pool_reorg_tests.ml

+          ];
+
+          let assert_pool_contains pool_state (pk,nonce)= 
+            let actual_opt = List.find pool_state ~f:(fun (fee_payer_pk, actual_nonce ) -> 


Rewrite this and next function: extract calculation of actual_opt into a helper function

georgeee · 2025-01-07T20:55:14Z

src/lib/network_pool/transacation_pool_reorg_tests.ml

+              )
+            in
+            match account_spec with
+              | Some account_spec -> 


Extract sent_blocking_zkapp, total_cost out of match construction (put it before).

Rewrite match without ifs:

| Some account_spec when sender.nonce < account_spec.nonce -> (..) | Some account_spec when sent_blocking_zkapp major_specs pk -> (..) | Some account_spec when account_spec.balance > total_cost -> (..) | Some account_spec -> (..) | None -> (..)

georgeee · 2025-01-08T15:26:14Z

src/lib/network_pool/transacation_pool_reorg_tests.ml

@@ -17,7 +17,7 @@ module Account_spec = struct
    ; nonce: int
  } [@@deriving sexp]

-  let rec find_in_test_keys ?(n=0) (x:Keypair.t) =
+  let find_in_test_keys (x : Keypair.t) =  Array.findi_exn test_keys ~f:(fun _ -> Keypair.equal x)  |> fst


And remove the two lines below

…ds from limited account capacity with rest of commands

dkijania · 2025-01-13T14:06:38Z

!ci-build-me

dkijania · 2025-01-13T14:24:19Z

!ci-build-me

dkijania self-assigned this Dec 13, 2024

dkijania marked this pull request as ready for review December 13, 2024 11:23

dkijania requested a review from a team as a code owner December 13, 2024 11:23

georgeee requested changes Jan 7, 2025

View reviewed changes

dkijania mentioned this pull request Jan 8, 2025

refactor network tests #16454

Merged

georgeee reviewed Jan 8, 2025

View reviewed changes

dkijania changed the base branch from master to compatible January 8, 2025 19:34

dkijania requested a review from a team as a code owner January 8, 2025 19:34

Quickcheck tests for reorg

07d610b

dkijania force-pushed the dkijania/transaction_pool_tests branch from cb1b2ac to 07d610b Compare January 11, 2025 16:21

increase number of generated commands in each sequences. Merge comman…

9b1bb9e

…ds from limited account capacity with rest of commands

dkijania added 2 commits January 13, 2025 15:09

remove unused method

3521810

renamed logger functions

29415c3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase test coverage for transaction pool module #16415

Increase test coverage for transaction pool module #16415

dkijania commented Dec 12, 2024 •

edited

Loading

dkijania commented Dec 13, 2024

georgeee commented Jan 7, 2025

georgeee left a comment

georgeee Jan 7, 2025

georgeee Jan 7, 2025

georgeee Jan 7, 2025

dkijania Jan 8, 2025

georgeee Jan 7, 2025

georgeee Jan 7, 2025

georgeee Jan 7, 2025

dkijania Jan 8, 2025 •

edited

Loading

georgeee Jan 7, 2025

georgeee Jan 7, 2025

georgeee Jan 7, 2025

georgeee Jan 7, 2025

dkijania Jan 8, 2025

georgeee Jan 7, 2025

georgeee Jan 7, 2025

georgeee Jan 7, 2025

georgeee Jan 7, 2025

georgeee Jan 8, 2025

dkijania commented Jan 13, 2025

dkijania commented Jan 13, 2025

Increase test coverage for transaction pool module #16415

Are you sure you want to change the base?

Increase test coverage for transaction pool module #16415

Conversation

dkijania commented Dec 12, 2024 • edited Loading

dkijania commented Dec 13, 2024

georgeee commented Jan 7, 2025

georgeee left a 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

dkijania Jan 8, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkijania commented Jan 13, 2025

dkijania commented Jan 13, 2025

dkijania commented Dec 12, 2024 •

edited

Loading

dkijania Jan 8, 2025 •

edited

Loading