Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix order of accounts in ledger coinbase witness #15267

Merged
merged 5 commits into from Mar 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/lib/mina_base/coinbase.ml
Expand Up @@ -56,7 +56,13 @@ module Make_str (A : Wire_types.Concrete) = struct
receiver t
:: List.map ~f:Fee_transfer.receiver (Option.to_list t.fee_transfer)
in
(* The order of this list will impact the order of new accounts in the
ledger witness. We use `List.rev` to have the same order as the tx
application (in `apply_coinbase`) where the "coinbase fee transfer"
receiver is created before the "coinbase" receiver.
*)
List.map account_ids ~f:(fun acct_id -> (acct_id, access_status))
|> List.rev
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this whole piece after let access_status definition can be simplified as something like:

let fee_transfer =
  let%map.Option transfer = t.fee_transfer in
  (Fee_transfer.receiver transfer, access_status)
in
Option.value_map ~default:ident ~f:List.cons fee_transfer [(receiver t, access_status)]

Current code is a bit hard to read
CC @mrmr1993


let accounts_referenced t =
List.map (account_access_statuses t Transaction_status.Applied)
Expand Down
3 changes: 2 additions & 1 deletion src/lib/mina_ledger/ledger.mli
Expand Up @@ -167,7 +167,8 @@ val gen_initial_ledger_state : init_state Quickcheck.Generator.t
(** Apply a generated state to a blank, concrete ledger. *)
val apply_initial_ledger_state : t -> init_state -> unit

module Ledger_inner : Ledger_intf.S with type t = t
module Ledger_inner :
Ledger_intf.S with type t = t and type location = Location.t

module For_tests : sig
open Currency
Expand Down
12 changes: 10 additions & 2 deletions src/lib/mina_ledger/test/helpers/ledger_helpers.ml
Expand Up @@ -16,11 +16,19 @@ let bin_log n =
in
find 0

let ledger_of_accounts accounts =
type ledger_depth = Fixed_depth of int | Inferred_depth

let ledger_of_accounts ?(depth = Inferred_depth) accounts =
let open Result.Let_syntax in
let open Test_account in
let module R = Monad_lib.Make_ext2 (Result) in
let depth = bin_log @@ List.length accounts in
let depth =
match depth with
| Inferred_depth ->
bin_log @@ List.length accounts
| Fixed_depth d ->
d
in
let ledger = Ledger.empty ~depth () in
let%map () =
R.iter_m accounts ~f:(fun a ->
Expand Down
12 changes: 12 additions & 0 deletions src/lib/transaction_logic/test/transaction_logic/main.ml
Expand Up @@ -7,5 +7,17 @@ let () =
, [ test_case "Simple funds transfer" `Quick simple_payment
; test_case "Fee payer cannot be different than sender" `Quick
simple_payment_signer_different_from_fee_payer
; test_case
"Coinbase transaction creates accounts in the correct order (no \
fee transfer)"
`Quick
(coinbase_order_of_created_accounts_is_correct
~with_fee_transfer:false )
; test_case
"Coinbase transaction creates accounts in the correct order \
(with fee transfer)"
`Quick
(coinbase_order_of_created_accounts_is_correct
~with_fee_transfer:true )
] )
]
Expand Up @@ -115,3 +115,44 @@ let simple_payment_signer_different_from_fee_payer () =
"Cannot pay fees from a public key that did not sign the transaction"
(Transaction_logic.apply_transactions ~constraint_constants ~global_slot
~txn_state_view ledger [ txn ] ) )

let coinbase_order_of_created_accounts_is_correct ~with_fee_transfer () =
let amount = Amount.of_mina_int_exn 720 in
let make_nondeterministic_pk () =
Private_key.create () |> Public_key.of_private_key_exn
|> Public_key.compress
in
let receiver = make_nondeterministic_pk () in
let fee_transfer =
if with_fee_transfer then
let receiver_pk = make_nondeterministic_pk () in
let fee = Fee.of_mina_int_exn 10 in
Some (Coinbase_fee_transfer.create ~receiver_pk ~fee)
else None
in
let coinbase_txn =
Or_error.ok_exn @@ Coinbase.create ~amount ~receiver ~fee_transfer
in
let accounts = [] (* All accounts are new *) in
let ledger =
match Ledger_helpers.ledger_of_accounts ~depth:(Fixed_depth 2) accounts with
| Ok l ->
l
| Error _ ->
assert false
in
let txn_global_slot = Global_slot_since_genesis.of_int 5 in
[%test_pred: Transaction_applied.Coinbase_applied.t Or_error.t] Or_error.is_ok
(Transaction_logic.apply_coinbase ~constraint_constants ~txn_global_slot
ledger coinbase_txn ) ;
let int_loc_of_account pk =
Ledger.location_of_account ledger pk
|> Option.value_exn |> Mina_ledger.Ledger.Location.to_path_exn
|> Mina_ledger.Ledger.Addr.to_int
in
let coinbase_accounts_referenced =
Coinbase.accounts_referenced coinbase_txn
in
List.iteri coinbase_accounts_referenced ~f:(fun idx pk ->
let actual_idx = int_loc_of_account pk in
[%test_eq: int] actual_idx idx )