pack: fix compute units #4113

jherrera-jump · 2025-02-06T19:20:49Z

The pack tile maintains compute cost for transactions in order to properly fill block. This PR some issues with the current calculation

Use loaded accounts data cost
Use the correct number of units for simple votes
Use default 200k units for config program, which will be migrated to BPF

src/ballet/pack/fd_pack_cost.h

mmcgee-jump · 2025-02-10T23:18:49Z

Can you squash and name commit like pack: <description>, thanks!

ptaffet-jump

Review in progress

src/ballet/txn/fd_txn.h

ptaffet-jump · 2025-02-11T19:10:36Z

src/ballet/txn/fd_txn.h

@@ -107,6 +107,12 @@
   transaction, using fd_txn_parse() verification rules. */
 #define FD_TXN_MIN_SERIALIZED_SZ     (134UL)

+/* base58 decode of Vote111111111111111111111111111111111111111 */
+static const uchar FD_TXN_VOTE_PROGRAM_ID[FD_TXN_ACCT_ADDR_SZ] = {


Can you move this into the is_simple_vote function so that the scoping is more restricted?

ptaffet-jump · 2025-02-11T19:12:13Z

src/ballet/txn/fd_txn.h

+  if( txn->instr_cnt!=1UL ) return 0;
+  if( txn->transaction_version!=FD_TXN_VLEGACY ) return 0;
+  if( txn->signature_cnt>2UL ) return 0;


Can you align the returns and add LIKELY/UNLIKELY? I'd make them unlikely. Might make sense to only make it one branch though, not sure.

ptaffet-jump · 2025-02-11T19:13:21Z

src/ballet/txn/fd_txn.h

+  if( txn->signature_cnt>2UL ) return 0;
+  ulong prog_id_idx = (ulong)txn->instr[0].program_id;
+  fd_acct_addr_t const * prog_id = addr_base + prog_id_idx;
+  return 0 == memcmp(prog_id->b, FD_TXN_VOTE_PROGRAM_ID, FD_TXN_ACCT_ADDR_SZ);


nit: spaces around parens. Also, there's fd_memeq if you'd like

src/ballet/txn/fd_txn.h

ptaffet-jump · 2025-02-11T19:16:56Z

src/disco/pack/fd_compute_budget_program.h

+  ulong loaded_accounts_data_cost = FD_COMPUTE_BUDGET_ACCOUNT_DATA_COST_PAGE_SIZE - 1UL;
+  loaded_accounts_data_cost     = loaded_accounts_data_cost + loaded_accounts_data_size;
+  loaded_accounts_data_cost     = loaded_accounts_data_cost / FD_COMPUTE_BUDGET_ACCOUNT_DATA_COST_PAGE_SIZE;
+  loaded_accounts_data_cost     = loaded_accounts_data_cost * FD_COMPUTE_BUDGET_HEAP_COST;


I'd just make this one expression. It's just a round up, times a constant. Something like
FD_COMPUTE_BUDGET_HEAP_COST * ((loaded_account_data_size + (FD_COMPUTE_BUDGET_ACCOUNT_DATA_COST_PAGE_SIZE-1UL)/FD_COMPUTE_BUDGET_ACCOUNT_DATA_COST_PAGE_SIZE )
should be clear enough

ptaffet-jump · 2025-02-11T19:27:54Z

src/disco/pack/fd_pack_cost.h

  }

  ulong instr_data_cost = instr_data_sz / FD_PACK_INV_COST_PER_INSTR_DATA_BYTE; /* <= 320 */

  ulong fee[1];
  uint compute[1];
-  fd_compute_budget_program_finalize( cbp, txn->instr_cnt, fee, compute );
+  ulong loaded_account_data_cost[1];
+  fd_compute_budget_program_finalize( cbp, txn->instr_cnt, fee, compute, loaded_account_data_cost);


Suggested change

fd_compute_budget_program_finalize( cbp, txn->instr_cnt, fee, compute, loaded_account_data_cost);

fd_compute_budget_program_finalize( cbp, txn->instr_cnt, fee, compute, loaded_account_data_cost );

ptaffet-jump

Overall looking good! I'm sorry you had to deal with test_pack, but thanks for making it much more robust!

ptaffet-jump · 2025-02-11T19:30:34Z

src/disco/pack/fd_pack_cost.h

+#define FD_PACK_COST_PER_ED25519_SIGNATURE   ( 2400UL)
+#define FD_PACK_COST_PER_SECP256K1_SIGNATURE ( 6690UL)
+#define FD_PACK_COST_PER_WRITABLE_ACCT       (  300UL)
+#define FD_PACK_INV_COST_PER_INSTR_DATA_BYTE (    4UL)

 /* The computation here is similar to the computation for the max
   fd_txn_t size.  There are various things a transaction can include


We need to update FD_PACK_MAX_TXN_COST and FD_PACK_MIN_TXN_COST. I'm happy to help with those.

ptaffet-jump · 2025-02-11T19:32:00Z

src/disco/pack/test_compute_budget_program.c

+  ulong loaded_accounts_data_cost = 0UL;
+  fd_compute_budget_program_finalize( &state, txn->instr_cnt, &rewards, &compute, &loaded_accounts_data_cost);
+  FD_TEST( rewards                   == expected_fee_lamports              );
+  FD_TEST( (ulong)compute            == expected_max_cu || 1               );


I'm confused by this one. Why the || 1?

ptaffet-jump · 2025-02-11T19:36:03Z

src/disco/pack/test_pack.c

          pack_outcome_t * outcome     ) {
  fd_pack_limits_t limits[1] = { {
    .max_cost_per_block        = FD_PACK_MAX_COST_PER_BLOCK,
    .max_vote_cost_per_block   = FD_PACK_MAX_VOTE_COST_PER_BLOCK,
    .max_write_cost_per_acct   = FD_PACK_MAX_WRITE_COST_PER_ACCT,
-    .max_data_bytes_per_block  = MAX_DATA_PER_BLOCK,
+    .max_data_bytes_per_block  = max_data_bytes_per_block,


Can you revert this change please?

ptaffet-jump · 2025-02-11T19:39:34Z

src/disco/pack/test_pack.c

-  schedule_validate_microblock( pack, 30000UL, 0.0f, 1UL, 0UL, 0UL, &outcome ); /* conflict gone.*/
+  ulong pack_cost_estimate = 0UL;
+  uint flags = 0UL;
+  rewards += make_transaction( i,  500U, 500U, 11.0, "A",    "B" ); pack_cost_estimate += fd_pack_compute_cost((fd_txn_t const *)txn_scratch[ i ], payload_scratch[ i ], &flags, NULL, NULL, NULL, NULL); insert( i++, pack );


Maybe have make_transaction provide the cost estimate via an out parameter? The way you have it is a bit clumsy

ptaffet-jump · 2025-02-11T19:41:29Z

src/app/fdctl/run/tiles/fd_bank.c

@@ -95,8 +96,8 @@ before_frag( fd_bank_ctx_t * ctx,
 }

 extern void * fd_ext_bank_pre_balance_info( void const * bank, void * txns, ulong txn_cnt );
-extern void * fd_ext_bank_load_and_execute_txns( void const * bank, void * txns, ulong txn_cnt, int * out_processing_results, int * out_transaction_err, uint * out_consumed_cus );
 extern int    fd_ext_bank_execute_and_commit_bundle( void const * bank, void * txns, ulong txn_cnt, uint * out_consumed_cus );


Do we also need this for bundles?

mmcgee-jump · 2025-02-11T22:03:00Z

Can you squash commits to keep stack tidy

jherrera-jump force-pushed the jherrera/pack-fix-cus branch 4 times, most recently from 06fa4fa to c4f8a00 Compare February 7, 2025 15:05

jherrera-jump marked this pull request as ready for review February 7, 2025 15:05

jherrera-jump force-pushed the jherrera/pack-fix-cus branch from c4f8a00 to 19f7610 Compare February 7, 2025 15:12

ripatel-fd reviewed Feb 7, 2025

View reviewed changes

src/ballet/pack/fd_pack_cost.h Outdated Show resolved Hide resolved

ripatel-fd reviewed Feb 7, 2025

View reviewed changes

src/ballet/pack/fd_pack_cost.h Outdated Show resolved Hide resolved

jherrera-jump force-pushed the jherrera/pack-fix-cus branch 8 times, most recently from fae6d65 to 9ea8ac9 Compare February 10, 2025 17:22

jherrera-jump requested a review from ptaffet-jump February 10, 2025 21:08

jherrera-jump force-pushed the jherrera/pack-fix-cus branch 2 times, most recently from 1e567bd to fb75b50 Compare February 10, 2025 23:57

ptaffet-jump reviewed Feb 11, 2025

View reviewed changes

jherrera-jump force-pushed the jherrera/pack-fix-cus branch from 84dd376 to 0bee8b1 Compare February 11, 2025 22:01

jherrera-jump force-pushed the jherrera/pack-fix-cus branch from 0bee8b1 to dad2f55 Compare February 11, 2025 22:50

jherrera-jump requested a review from ptaffet-jump February 11, 2025 22:58

mmcgee-jump mentioned this pull request Feb 11, 2025

New cost model for signatures #2315

Closed

jherrera-jump force-pushed the jherrera/pack-fix-cus branch 2 times, most recently from eb85c86 to b48aa06 Compare February 12, 2025 21:11

jherrera-jump force-pushed the jherrera/pack-fix-cus branch 2 times, most recently from 38878dd to 215cb8b Compare February 12, 2025 22:34

ptaffet-jump previously approved these changes Feb 12, 2025

View reviewed changes

pack: fix cost model calculation

31ef576

jherrera-jump dismissed ptaffet-jump’s stale review via 31ef576 February 12, 2025 22:36

jherrera-jump force-pushed the jherrera/pack-fix-cus branch from 215cb8b to 31ef576 Compare February 12, 2025 22:36

ptaffet-jump approved these changes Feb 12, 2025

View reviewed changes

jherrera-jump enabled auto-merge February 12, 2025 22:37

jherrera-jump added this pull request to the merge queue Feb 12, 2025

Merged via the queue into main with commit 4d173a0 Feb 12, 2025
10 of 11 checks passed

jherrera-jump deleted the jherrera/pack-fix-cus branch February 12, 2025 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pack: fix compute units #4113

pack: fix compute units #4113

jherrera-jump commented Feb 6, 2025 •

edited

Loading

mmcgee-jump commented Feb 10, 2025

ptaffet-jump left a comment

ptaffet-jump Feb 11, 2025

ptaffet-jump Feb 11, 2025

ptaffet-jump Feb 11, 2025

ptaffet-jump Feb 11, 2025

ptaffet-jump Feb 11, 2025

ptaffet-jump left a comment

ptaffet-jump Feb 11, 2025

ptaffet-jump Feb 11, 2025

ptaffet-jump Feb 11, 2025

ptaffet-jump Feb 11, 2025

ptaffet-jump Feb 11, 2025

mmcgee-jump commented Feb 11, 2025

	fd_compute_budget_program_finalize( cbp, txn->instr_cnt, fee, compute, loaded_account_data_cost);
	fd_compute_budget_program_finalize( cbp, txn->instr_cnt, fee, compute, loaded_account_data_cost );

pack: fix compute units #4113

pack: fix compute units #4113

Conversation

jherrera-jump commented Feb 6, 2025 • edited Loading

mmcgee-jump commented Feb 10, 2025

ptaffet-jump 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

ptaffet-jump 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

mmcgee-jump commented Feb 11, 2025

jherrera-jump commented Feb 6, 2025 •

edited

Loading