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

mrmr1993

Partial review; still need to do a full pass. I've also proposed #16467, which is functionally the same, but with opinionated refactors to help me understand the code better.

mrmr1993 · 2025-01-14T14:40:45Z

src/lib/network_pool/transaction_pool.ml

@@ -1667,7 +1672,7 @@ let%test_module _ =
    let minimum_fee =
      Currency.Fee.to_nanomina_int genesis_constants.minimum_user_command_fee

-    let logger = Logger.null ()
+    let logger = Logger.create ()


This should be set back to Logger.null before this can be merged.

mrmr1993 · 2025-01-14T14:41:22Z

src/lib/network_pool/transaction_pool.ml

-          ~time_controller ~frontier_broadcast_pipe:frontier_pipe_r
-          ~log_gossip_heard:false ~on_remote_push:(Fn.const Deferred.unit)
-          ~block_window_duration
+        Test.create ~config ~logger:(Logger.create ()) ~constraint_constants


dkijania · 2025-01-15T16:10:08Z

!ci-build-me

dkijania · 2025-01-15T16:11:34Z

!ci-build-me

…ion_pool_tests

dkijania · 2025-01-16T16:34:50Z

!ci-build-me

georgeee

I like the current test implementation, very readable and precise!

I left more than a dozen review comments, but I doubt any of them prevents the test from doing its job alright in the current setting.

I still want these changes to be applied, mostly to make the test more robust (so that it would work practically on any initial ledger), slightly more readable and to test a slightly fuller distribution of cases. But I'm quite fine with applying these changes in a follow-up PR

src/lib/network_pool/transaction_pool.ml

georgeee · 2025-01-16T22:00:05Z

src/lib/network_pool/transaction_pool.ml

+      let%bind major_sequence_length = Int.gen_incl 2 10 in
+      let%bind minor_sequence_length =
+        let%map minor_sequence_length = Int.gen_incl 2 4 in
+        minor_sequence_length + major_sequence_length + initial_nonce


Why are we adding the nonce of account to minor sequence length? What if account has nonce of 100000 in the initial ledger?

Is it to ensure that extended minor sequence is longer than number of transactions associated with the account in pre-existing major sequence?

Exactly, we need to take into consideration case in which major branch has more transactions for particular account. Therefore on minor we need to generate more transaction to keep up with nonce

georgeee · 2025-01-16T22:05:57Z

src/lib/network_pool/transaction_pool.ml

+        let%map sequence =
+          Quickcheck_lib.init_gen_array len ~f:(fun _ ->
+              let%bind amount =
+                Int.gen_incl 5_000_000_000_000_000 (half_initial_balance / len)


What if (half_initial_balance / len) < 5_000_000_000_000_000? will it fail with an exception?

So, I just checked that initial balance is always 900mln. I still see little benefit in hardcoding the 5mln amount here

hm, so what should be the minimal amount ? should it be related to half_initial_balance?

yeah, let's make it proportional to half_initial_balance/len

georgeee · 2025-01-16T22:26:12Z

src/lib/network_pool/transaction_pool.ml

+        { prefix_commands
+        ; major_commands =
+            Array.append major_commands [| permission_change_cmd |]
+        ; minor_commands = Array.append minor_commands aux_minor_cmd


Is it possible to mix aux commands (which are expected to be dropped) with the regular commands from minor_commands? Or just generate some new random commands and run gen_merge on this new sequence and aux_commands. We want to ensure that no valid tx from other account is dropped together with aux commands that need to be dropped.

Co-authored-by: George Agapov <[email protected]>

…ease tx amount for limited capacity corner case by recieved amount

dkijania · 2025-01-20T21:34:45Z

!ci-build-me

dkijania · 2025-01-20T21:43:56Z

src/lib/network_pool/transaction_pool.ml

-                                   pk nonce ) )
-                          ] ;
-                      assert_pool_contains pool_state (pk, nonce) ) ;
+                  if sender.nonce < Simple_account.nonce account_spec then (


after changing find_by_key_idx i had to modify match back to if-else

It's okay as long as it is linear and not nested

dkijania · 2025-01-20T21:44:54Z

src/lib/network_pool/transaction_pool.ml

@@ -3502,6 +3512,20 @@ let%test_module _ =
      in
      let initial_balance = Simple_account.balance target_account in
      let half_initial_balance = Simple_account.balance target_account / 2 in
+      let recieved_amount =


Another safe guard to make sure we deduct correct amount from account balance

dkijania · 2025-01-21T12:02:33Z

!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

mrmr1993 approved these changes Jan 14, 2025

View reviewed changes

Use yojson

d21d983

This was referenced Jan 14, 2025

Fix a few bugs in revalidate (mini) #16401

Merged

[compatible] Fix a few bugs in revalidate (mini) #16465

Closed

mrmr1993 added 7 commits January 14, 2025 16:07

Hoist gen_merge

338ecda

Hoist get_random as get_random_from_array

206c8af

Rename get_random_acc

3dace23

Use to_yojson more

4e3aac6

Constrain mutability to iterator

ee64fa5

Rename s1 and s2 to major_sequence and minor_sequence

7f558ac

Inline definition in minor_sequence_length

fabb6d8

mrmr1993 added 8 commits January 15, 2025 01:55

Use stdlib functions

f7ef592

More descriptive names

d9a4810

Fixup typo in error message

232cb49

Combine ledger updates with sequence generation

6ee22e7

Rename locally to account

9b5dfc5

Rename sender to account

c970bfb

Hoist ledger mutability into helper

427b123

Rename account_with_limited_capacity to target_account

c26bd8c

mrmr1993 mentioned this pull request Jan 15, 2025

Opinionated refactoring of #16415 #16467

Merged

mrmr1993 requested changes Jan 15, 2025

View reviewed changes

dkijania added 2 commits January 15, 2025 14:21

Merge branch 'compatible' into dkijania/transaction_pool_tests

cb046cc

fix non sealed account selection

1d5d537

nullify logger

1d3a4f4

dkijania added 3 commits January 15, 2025 16:42

fix non sealed account selection

2372a97

nullify logger

caa9b60

Merge branch 'dkijania/transaction_pool_tests' into mrmr1993/transact…

914fe20

…ion_pool_tests

georgeee approved these changes Jan 16, 2025

View reviewed changes

dkijania and others added 2 commits January 20, 2025 20:05

Apply suggestions from georgee code review

ad2a595

Co-authored-by: George Agapov <[email protected]>

use gen_merge for aux commands sequence for zkapp blocking case. Incr…

c10d73b

…ease tx amount for limited capacity corner case by recieved amount

dkijania commented Jan 20, 2025

View reviewed changes

make lower generated amount proportional to initial_balance/len

290bef6

mrmr1993 approved these changes Jan 21, 2025

View reviewed changes

mrmr1993 merged commit ed5d316 into compatible Jan 21, 2025
44 checks passed

mrmr1993 deleted the dkijania/transaction_pool_tests branch January 21, 2025 13:17

Increase test coverage for transaction pool module #16415

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

mrmr1993 left a 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 15, 2025

dkijania commented Jan 15, 2025

dkijania commented Jan 16, 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

Choose a reason for hiding this comment

dkijania commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkijania commented Jan 21, 2025

dkijania commented Dec 12, 2024 •

edited

Loading

dkijania Jan 8, 2025 •

edited

Loading